Edit: Sorry, the question was marked as unclear. In a nutshell, there are two questions:

  1. Does the code below indeed create an sql injection vector? (It seems clear that the answer is yes, even though I was unable to craft a successful attack on my current server architecture.)

  2. Assuming someone successfully exploited that sql injection vector on the old server, what would I have to do to make sure that there are no leftover attack vectors? Would it be sufficient, for example, if I took the following actions:

    • Move to a new server
    • Build a new Drupal codebase from scratch
    • Remove the insecure code from the theme
    • Block or delete all users except those that are recognized
    • Force password resets for the recognized users
    • Check the menu_router table for bad items
    • Check all node and comment titles, bodies and fields for tags

...Or is the situation even more hopeless than that, and the site should simply be abandoned or rebuilt from scratch? For various reasons I don't think it is possible to restore from a backup, mostly because the attempted exploits go back 6 years.

=== Original post ===

I'm looking at an old D6 site that was done in a very non-standard way. What concerns me most are some .tpl.php files from the theme that use $_POST parameters to insert data into some tables, e.g.:

  $uid = $user->uid;
  $name = $user->name;
  $cid=db_result(db_query("select  max(cid) from  comments"));
  db_query("insert  into  comments  values($cid2, 0, $nid,$uid,'$title' , '$comment', '', $timestamp, 0, 3,'01/', '$name', null, null)");

This seems like very straightforward sql injection vector, but I'm not really an expert at exploiting such things, and my relatively feeble attacks have been caught by the new server. However, there are 1000 entries in the database where the titles are pretty clear attempts to execute injection attacks, and worse yet are the 1000 more with empty titles which might represent attacks that were successful.

So how bad is this? Here is my scale:

  1. 37GB of spam! Ha ha ha! No problem!
  2. Removing the insecure code and moving to a new server should be safe.
  3. The site will not really be safe until you switch to a new server, remove all the insecure code, and run some automated tests on the database and/or file structure to check for residual attack vectors.
  4. The site will not really be safe until you switch to a new server, remove all the insecure code, and then go through each file and each database row and manually check to make sure that it isn't an attack vector — in other words, it's probably easier to give up and start over, no matter the size of the site.
  • Yeah it's not great. Given that it's a comment and will be displayed on the site, an attacker could get (for example) the mysql root user password hash by sending something like ', (select password from mysql.user where user='root' limit 0,1), '', 0, 0, 3, '', '', null, null)-- - in the title post var. Security aside, this logic should never be in a template, so other development practices are also suspect. If it was me, I'd only consider option 4 – Clive Jul 31 '16 at 0:50
  • Hi Clive, thanks for the comment. Yeah the site was built by people who did not know or chose not to follow Drupal development practices. It's hardly a Drupal site really; more of a custom PHP/JS site built atop a nominal Drupal database using 100+ .tpl.php files in the theme. There were also a couple of functions that PHP didn't provide natively, so they added them to index.php, etc. To me, just because work is not done the Drupal way doesn't mean that it's necessarily bad development, but with these sql injection vectors I think it looks quite bad. – David Hunt Aug 1 '16 at 14:44

Theoretically, that SQL query could be used to do anything on that database server, including changing the password for a Drupal account not used so often, or creating a new account in the users table.

Suppose that $_POST['title'] contains the following.

"ddddd', 'comment',, 1469987843, 0, 3, '01/', 'Tintin', NULL NULL); DELETE FROM {users} u WHERE u.uid > 0; -- "

This would change ""INSERT INTO {comments} VALUES($cid2, 0, $nid, $uid, '$title', '$comment', '', $timestamp, 0, 3,'01/', '$name', NULL, NULL)" into the following.

"INSERT INTO {comments} VALUES($cid2, 0, $nid, $uid, 'ddddd', 'comment',, 1469987843, 0, 3, '01/', 'Tintin', NULL NULL); DELETE FROM {users} u WHERE u.uid > 0; -- ', '$comment', '', $timestamp, 0, 3,'01/', '$name', NULL, NULL)"`

I added a query to delete the users table, but I could have added any query, including the one to alter the password of an existing account, or creating a new user.

How that type of attack is effective depends only from how much the attackers know about your site: which software you are running, which version, etc. Once they have access as administrator, they could do enable the PHP input format, and execute arbitrary code. I would say that spam is the last of your problems.

As side note, as Clive said, template files should never contain SQL queries; eventually, those could be in preprocess functions, but in most of the cases, they should be in a module.
Also, in Drupal SQL query, you should wrap the database table names in {}. This allows Drupal to use prefixes in the table names, and even use tables from different databases.

| improve this answer | |
  • Thanks Kiam. I tried almost exactly these attacks on the site but they didn't work for some reason, I think because my server software is automatically set up to block POST parameters with sql statements in them. But, the fact that you and Clive are suggesting the same things that I thought about confirms that the code is indeed as problematic as I suspected it was, even though I don't have the expertise to actually exploit it on modern server architecture. – David Hunt Aug 1 '16 at 14:33

Not the answer you're looking for? Browse other questions tagged or ask your own question.