Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
- $table is used to directly generate an alias, the base table name is escaped but the alias is not
- column names are not escaped at all
Comment | File | Size | Author |
---|---|---|---|
#44 | escape_fields2.patch | 8.37 KB | Berdir |
#42 | escape_fields.patch | 8.37 KB | Berdir |
#38 | 769554.patch | 6.51 KB | cha0s |
#36 | 769554-36.patch | 6.51 KB | jpmckinney |
#34 | 769554-31.patch | 6 KB | Berdir |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedI don't believe this is exploitable in core. We cannot guess what usage contrib will do of the API. We need to make sure that, as long as you use the query builder (and do not craft SQL queries by hand) you are completely protected against injections.
Comment #2
Gábor HojtsyAgreed. Mixed escaping of items is not a good idea as certain form API elements showed. I'd vote to consistently escape items passed to the query builder.
Comment #3
Crell CreditAttribution: Crell commentedAs would I, with this caveat: #715132: Over aggressive escaping of table names
Comment #4
BerdirEDIT: Screw that, we obviously want to use http://api.drupal.org/api/function/db_escape_table/7. But the point below is still true.
Also, allowing not validated user input as table/columns into a query sounds like a recipe for disaster to me anyway. For example, even if the table name is escaped, it would still allow access to every table in your system and maybe reading of data like passwords and so on. So we need to document/clarify that it is *never* safe to use user submitted values directly for building a query. You always need to use a whitelist approach.
Comment #5
catchYeah I'm not really sure how it's possible to protect people entirely from this, seems more a documentation issue, for the points Berdir raised. Although db_escape_table() obviously makes sense.
Comment #6
Crell CreditAttribution: Crell commentedUnder no circumstances do we call db_escape_table() from within the query builder. Calling the appropriate connection object's escapeTable() method, however, is a good idea.
We should also make a note of this fact in the appropriate handbook pages at least.
Comment #7
andyposttable_sort is first candidate for injections
Comment #8
BerdirOk, attached is a first patch. I escaped the table alias and the column name in all places I could find. I tried to do the escaping as late as possible (read: in __toString()/compile()).
Let's see what the test bot thinks about this. Not sure about the docs part yet...
Comment #9
bshort CreditAttribution: bshort commentedSubscribe
Comment #11
jpmckinney CreditAttribution: jpmckinney commentedIn openid.module:
escapeTable() will turn "created + expires_in" into "createdexpires_in".
In database_test.test:
escapeTable() will turn "COUNT(task)" into "COUNTtask".
In short, don't use escapeTable() to escape fields.
Comment #12
jpmckinney CreditAttribution: jpmckinney commentedRe-rolled without offending lines.
Comment #13
BerdirYep, and the other one is a havingCondition() in the tests. So we need to convert the above into where() and the test case to having().
Or we could re-think if we really want this... As stated above, it doesn't make things secure but it might confuse developers unless it's very well documented.
EDIT: #12 only escapes the alias, which is basically the Or...
Comment #14
BerdirComment #15
jpmckinney CreditAttribution: jpmckinney commentedI don't understand. What do you mean?
Anyway, I re-rolled it with updated documentation (havingCondition didn't even have a PHPDoc) and substitution of where() and having() for condition() and havingCondition(). I am +1 for doing this.
Not immediately relevant to this issue, but related:
QueryConditionInterface::condition was lying in saying that the default operator is =; it's actually IN or =. I don't think havingCondition should have a default operator of =. If $values is an array, it should use IN. I made these changes as well.
Also, I have no idea why havingCondition and SelectQuery::condition count the number of args and pass it around, seeing as, as far as I can tell, that argument is never used. So, I cut it out. I'm prepared to be corrected, or asked to create a new issue!
Comment #16
jpmckinney CreditAttribution: jpmckinney commentedWrong patch in #15. This is correct.
Comment #17
jpmckinney CreditAttribution: jpmckinney commentedForgot to include patch #8 in my patch. I should not write patches and listen to sessions at the same time.
Comment #19
jpmckinney CreditAttribution: jpmckinney commentedFix syntax.
Comment #21
jpmckinney CreditAttribution: jpmckinney commentedOne more time.
Comment #22
jpmckinney CreditAttribution: jpmckinney commentedComment #23
BerdirI think we currently only document interfaces, not the implementations (I'm not 100% sure how this will change with #718596: Lacking standards for documenting classes, interfaces, methods). So if havingCondition() isn't documented in SelectQueryInterface, we should add it there.
What I meant in #13 was, that your patch in #12 means not to escape column names and #21 means to escape column names. Both are possible approaches as we can't have a 100% secure implementation anyway. So we need to decide which one we want to proceed. Any comments on this?
114 critical left. Go review some!
Comment #24
jpmckinney CreditAttribution: jpmckinney commentedI think we should escape column names in condition() and havingCondition(). If a developer wants to circumvent that, there are where() and having(). Here is an updated patch with the PHPDoc in the right place.
Comment #26
Berdir#24: 769554-24.patch queued for re-testing.
Comment #28
jpmckinney CreditAttribution: jpmckinney commentedThe only change from the previous patch was adding havingCondition to QueryConditionInterface. Could this cause installation to fail? I'm not as familiar with PHP interfaces.
Comment #29
BerdirIt should be added to SelectQueryInterface. QueryConditionInterface is then used internally in SelectQuery.
Comment #30
jpmckinney CreditAttribution: jpmckinney commentedOk, did that now.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedi'm fine with this as long we don't regress on #715132: Over aggressive escaping of table names
Comment #32
jpmckinney CreditAttribution: jpmckinney commented#715132: Over aggressive escaping of table names should not be affected, but we can add tests for that, I think.
Comment #34
BerdirRemoved the .htaccess part that you accidently added, no other changes.
Comment #35
Crell CreditAttribution: Crell commentedWell #34 fixes a half dozen brain dead bugs I'm embarrassed were even still in there (lack of havingCondition docs, $num_args, etc.). I agree with #11, though, that we should not be using escapeTable() on a field. There's on guarantee that the escaping rules will always be the same and on every database. Let's introduce an escapeField() method and use that instead, even if its body is the same as escapeTable()'s. That should be a cleaner, more forward-looking approach.
Comment #36
jpmckinney CreditAttribution: jpmckinney commentedAs far as I can tell, the last patch didn't escape any fields. Maybe we want to start doing that? Anyway, here's a patch that adds escapeField().
Comment #37
Crell CreditAttribution: Crell commentedHm. OK, I misread the previous patch then. Yeah, we should be escaping field names, for the same reason we should be escaping table names. Let's add that. Also, I'm pretty sure field names can't (or at least really shouldn't) contain periods, so we can remove that from the allowed character list for fields.
Comment #38
cha0s CreditAttribution: cha0s commentedRerolled per #37
Comment #39
jpmckinney CreditAttribution: jpmckinney commentedI think #38 just removes the period from escapeField.
Comment #40
cha0s CreditAttribution: cha0s commentedOh, I must have misread the patch... I saw the escapeField() function and assumed it had already been implemented. My bad.
Comment #41
jpmckinney CreditAttribution: jpmckinney commentedHmm? Removing the period from the allowed character list was part of the request in #37, so that's good.
But, we also want to start using escapeField to escape fields. That's all that's missing from the patch now.
Comment #42
BerdirEscaped fields in conditions and SelectQuery, also added a db_escape_field() (similiar to db_escape_table).
Comment #44
BerdirField names not, but we obviously also want to allow to specify the table in a condition and for that, you need the . :)
This caused all kind of funny exceptions because it removed the ....
Comment #45
BerdirComment #46
Crell CreditAttribution: Crell commentedI like it. Fixes a couple of issues all at once. Rock on!
*Turns to Dries, who is sitting next to him at the CommerceGuys table at a conference.* All yours.
Comment #47
Dries CreditAttribution: Dries commentedThanks! Committed to CVS HEAD. Now: lunch time!