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 :-)
Comment | File | Size | Author |
---|---|---|---|
#28 | filter-query-comment-1105848-28.patch | 10.07 KB | cafuego |
#24 | filter-query-comment-1105848-24.patch | 9.46 KB | cafuego |
#22 | filter-query-comment-1105848-22.patch | 9.33 KB | cafuego |
#20 | filter-query-comment-1105848-19.patch | 9.15 KB | cafuego |
#16 | filter-query-comment-1105848-16.patch | 8.28 KB | cafuego |
Comments
Comment #1
cafuego CreditAttribution: cafuego commentedSetting to 'needs review' so the patch can be tested.
Comment #3
cafuego CreditAttribution: cafuego commentedWhoops, 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.
Comment #5
Dave ReidComment #6
Crell CreditAttribution: Crell commentedHm, 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.
Comment #7
Crell CreditAttribution: Crell commentedComment #8
Dave ReidClarifying for future reference that this issue has been cleared to remain a public hardening issue.
Comment #9
cafuego CreditAttribution: cafuego commentedUpdated 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.
Comment #10
Crell CreditAttribution: Crell commentedThe 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.
Comment #11
catchComment #12
aspilicious CreditAttribution: aspilicious commentedMy first D8 reroll. Changed the format of the comments a bit because the standards didn't like it ;)
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedWhy are we only removing
*/
that are preceded by a space? Wouldn't any*/
terminate the comment?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).
Comment #14
Flowster CreditAttribution: Flowster commentedfilter-query-comment.patch queued for re-testing.
Comment #15
Crell CreditAttribution: Crell commentedHm. Actually yes, Damien is correct.
Comment #16
cafuego CreditAttribution: cafuego commentedNew patch with:
filterComment()
moved from thecomment()
function to the various__toString()
methods. I decided against calling filterComment() in-line with the implode() call to aid readability.Comment #18
cafuego CreditAttribution: cafuego commentedOh that was clever wasn't it.
Is it OK to rewrite this functionality a bit, so that the
filterComments(string)
method becomesformatComments(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.
Comment #19
cafuego CreditAttribution: cafuego commentedComment #20
cafuego CreditAttribution: cafuego commented... 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.
Comment #21
aspilicious CreditAttribution: aspilicious commentedFlattens
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.
Comment #22
cafuego CreditAttribution: cafuego commentedThe 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.
Comment #23
aspilicious CreditAttribution: aspilicious commentedI don't think you're understanding what I'm saying. Please read http://drupal.org/node/1354#functions.
Comment #24
cafuego CreditAttribution: cafuego commentedRight, that makes more sense. Updated patch attached. Also americaniZed spelling in the comments.
Comment #25
Damien Tournoud CreditAttribution: Damien Tournoud commentedLooks good to me.
Tagging as a DB-layer API change (all the third party drivers will have to be updated).
Comment #26
Dries CreditAttribution: Dries commentedThis patch looks good but we could do a better job with the documentation. Some suggestions below:
Why do we write 'star-space-slash' (vs 'star-slash'). Typo?
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!
Comment #27
cafuego CreditAttribution: cafuego commentedWhy 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?
Comment #28
cafuego CreditAttribution: cafuego commentedHow 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.
Comment #29
Dries CreditAttribution: Dries commentedCommitted 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!
Comment #30
Crell CreditAttribution: Crell commentedI 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. :-)
Comment #31
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt'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.
Comment #32
Crell CreditAttribution: Crell commentedHm, 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.
Comment #33
cafuego CreditAttribution: cafuego commentedI 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.
Comment #34
webchickIf 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.
Comment #35
chx CreditAttribution: chx commentedIt's not mongodb but mongodb_dbtng that's affected. Here's the mongodb_dbtng version: function makeComment() {} go for it.
Comment #36
mikey_p CreditAttribution: mikey_p commented@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.
Comment #37
webchickOk 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. :)