I was just working on adding query comment support into Views 3.x and did some testing when I discovered that really, the query comments need to be sanitised... and ideally core does that.

For example, with my patch to Views, a user could add the comment string: "Comment */ DROP TABLE users; -- /* Haha, you're hosed!" to a view. The result is badness, as views would process the full query into eg:


"/* Comment */ DROP TABLE users; -- /* Haha, you're hosed! SELECT nid FROM node ..."
/code>

Which is bad. It's fair enough that a coder with access to the API or even a user with PHP Imput format access could randomly drop tables, but someone with point & click access to Views maybe shouldn't be able to.

I had written a patch that uses a filter defined in common.inc to remove the /* and */ tags from comments, but the security team suggest the fix should go in the DatabaseConnection class - on account of comments potentially being implemented differently on different DBs - or at least in the Database layer) and not elsewhere in the codebase.

I'm not sure how I'd call a function on the DatabaseConnection class from within the Query class at the time the comment is set, so in the attached patch I've added the filter function to the Query class instead, which means it should be override-able in each DB's query.inc file anyway (I think :-)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cafuego’s picture

Status: Active » Needs review

Setting to 'needs review' so the patch can be tested.

Status: Needs review » Needs work

The last submitted patch, filter-query-comment.patch, failed testing.

cafuego’s picture

Status: Needs work » Needs review
FileSize
2.19 KB

Whoops, stupid brace.

I've updated the patch and also added a test case to the database tests, to ensure the comments are indeed stripped properly.

Dave Reid’s picture

Version: 7.0 » 7.x-dev
Crell’s picture

Version: 7.x-dev » 7.0
Priority: Normal » Major
Status: Needs review » Needs work

Hm, I don't know why this didn't show up on my issue queue before.

We definitely should fix this. I would put the escaping method in the connection object, though, for consistency. That is available at $this->connection from within any query object so you can escape things as needed.

Aside from that this patch looks OK to me.

Crell’s picture

Version: 7.0 » 7.x-dev
Dave Reid’s picture

Issue tags: +Security improvements

Clarifying for future reference that this issue has been cleared to remain a public hardening issue.

cafuego’s picture

Status: Needs work » Needs review
FileSize
2.86 KB

Updated patch attached. The filterComment() function is now a public method on the DatabaseConnection class. Also elaborated a little bit in the comment on the comment() function in the Query class.

Crell’s picture

Priority: Major » Critical
Status: Needs review » Needs work

The extra comment is actually a bit confusing. It's unclear if the caller is expected to sanitize the string first or not. I'd suggest changing it to "The comment string will be sanitized to remove */ and other characters that may terminate the string early so as to avoid SQL injection attacks." The code itself looks fine now.

I'm also bumping this to critical due to the security implications, even though the Sec team doesn't feel it's high enough a risk to warrant an SA. Better safe than sorry.

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
aspilicious’s picture

Status: Needs work » Needs review
FileSize
2.91 KB

My first D8 reroll. Changed the format of the comments a bit because the standards didn't like it ;)

Damien Tournoud’s picture

Status: Needs review » Needs work
+  public function filterComment($string = '') {
+    return preg_replace('/(\/\*\s*)|(\s\*\/)/', '', $string);
+  }

Why are we only removing */ that are preceded by a space? Wouldn't any */ terminate the comment?

   public function comment($comment) {
-    $this->comments[] = $comment;
+    $this->comments[] = $this->connection->filterComment($comment);
     return $this;
   }

This break the "filter on output" pattern that is common both in core and in the database layer. The code to filterComment() should move where we actually use the comment (in the various __toString() methods).

Flowster’s picture

Status: Needs work » Needs review

filter-query-comment.patch queued for re-testing.

Crell’s picture

Status: Needs review » Needs work

Hm. Actually yes, Damien is correct.

cafuego’s picture

Status: Needs work » Needs review
FileSize
8.28 KB

New patch with:

  • The regex fixed to it'll strip "*/" and any preceding spaces from the comment. I did have that in my regex test code, but didn't bring it across properly. Nice catch :-)
  • The call to filterComment() moved from the comment() function to the various __toString() methods. I decided against calling filterComment() in-line with the implode() call to aid readability.

Status: Needs review » Needs work

The last submitted patch, filter-query-comment-1105848-16.patch, failed testing.

cafuego’s picture

Oh that was clever wasn't it.

Is it OK to rewrite this functionality a bit, so that the filterComments(string) method becomes formatComments(array) or something instead? Or use two methods in the DatabaseConnection class - one to flatten the array and one to strip erroneous comment characters?

That way one call can turn an array of strings into a single sanitised comment string, rather than splitting up the functionality and needing to have the (currently) same code in all Query classes. A specific DB could still override that method if it needed to.

cafuego’s picture

Status: Needs work » Needs review
cafuego’s picture

... as per this patch.

My preference is to keep the methods to filter the strings and create the comment separate. Don't know why, though.

aspilicious’s picture

+++ b/includes/database/database.incundefined
@@ -541,6 +541,40 @@ abstract class DatabaseConnection extends PDO {
+   * Flatten an array of query comments into a single formatted comment string.

Flattens

+++ b/includes/database/database.incundefined
@@ -541,6 +541,40 @@ abstract class DatabaseConnection extends PDO {
+   * Process a query comment string and ensure it does not include strings that
+   * might terminate the comment early.

This sentence can not be longer than 80 chars. I alrdy fixed that in my reroll so you can copy that. This is core, please know and use the standards.

Powered by Dreditor.

cafuego’s picture

The comments in that patch were all already 80 or less characters (including CR) according to my text editor.

Anyway, attached is a new version with shortified comments.

aspilicious’s picture

I don't think you're understanding what I'm saying. Please read http://drupal.org/node/1354#functions.

cafuego’s picture

Right, that makes more sense. Updated patch attached. Also americaniZed spelling in the comments.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +DB API change

Looks good to me.

Tagging as a DB-layer API change (all the third party drivers will have to be updated).

Dries’s picture

Status: Reviewed & tested by the community » Needs work

This patch looks good but we could do a better job with the documentation. Some suggestions below:

+++ b/includes/database/database.inc
@@ -541,6 +541,46 @@ abstract class DatabaseConnection extends PDO {
+   * The comment string will be sanitized to remove * / and other

Why do we write 'star-space-slash' (vs 'star-slash'). Typo?

+++ b/includes/database/database.inc
@@ -541,6 +541,46 @@ abstract class DatabaseConnection extends PDO {
+   * Ensure a query comment does not include strings that might terminate
+   * the comment early.

I think this need a bit more explanation; for example we should mention '*/' and maybe give a quick example to help people understand the security problem. The explanation seems to be part of makeComment(), and not part of filterComment().

I think this patch is really close -- just needs a bit more work on the phpDoc. Thanks!

cafuego’s picture

Why do we write 'star-space-slash' (vs 'star-slash'). Typo?

Because as "*/" it terminates the comment and causes a parse error. Ironic, I know ;-) That in turns makes it impossible to use */ in the in-line example. Is it OK to use "* /" instead? or should I use "*\/" or something else?

cafuego’s picture

Status: Needs work » Needs review
FileSize
10.07 KB

How about this one? I've moved most of the explanation to filterComment() and added a basic example of a bad comment and what would be seen by the SQL server in such a case.

Dries’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community

Committed to 8.x.

I think we want to commit this to 7.x but it is a bit of an API change. Hence, moving to 7.x for @webchick to double check.

Thanks!

Crell’s picture

I believe this is an "internal only" change; no existing code should break, unless it was doing something insecure in the first place in which case we want it to break. :-)

Damien Tournoud’s picture

It's an API change all right. All the contrib DB modules will have to be updated if they override any query (they usually do). But this is for the better.

Crell’s picture

Hm, right, good point. It's only an API change for DB drivers, of which there are... 2 in contrib? 3 if you include Mongo. :-)

Still, as this is security related I would say it's worth it.

cafuego’s picture

I think we want to commit this to 7.x but it is a bit of an API change. Hence, moving to 7.x for @webchick to double check

Yes, Views 3.x for Drupal 7 will get support to set a query comment via the UI, so it would be extremely good to have those sanity checked :-) I did the patch against 7.x, so it should apply without any hassle at all.

webchick’s picture

Issue tags: +API change

If this is going into Views 3 as a user-facing feature soon, it seems like we should fix this ASAP.

I hate to break the various DB drivers in a stable release, even for a good reason. :( But the usage stats show 6 users for http://drupal.org/project/oracle, 0 for http://drupal.org/project/sqlsrv and 30 for http://drupal.org/project/mongodb. So **hopefully** the impact would be minimal.

I'd still like to see chx chime in here as one of (I assume?) the active users of the mongodb project. The author of two of them has given it the sign-off, so that's probably good enough, but one more +1 wouldn't hurt.

chx’s picture

It's not mongodb but mongodb_dbtng that's affected. Here's the mongodb_dbtng version: function makeComment() {} go for it.

mikey_p’s picture

@webchick, I'm surprised their even reporting any usage, as there is no update status for db drivers in core. See #916420: Third-party database drivers are not tracked by update-status for more info.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok then. Committed to 7.x.

There's still a week or so for for DB drivers to update their code in preparation of 7.1.

/me wonders if rfay still tracks this tag. :)

Status: Fixed » Closed (fixed)
Issue tags: -Security improvements, -API change, -Needs backport to D7, -DB API change

Automatically closed -- issue fixed for 2 weeks with no activity.