reported by: Giles Kennedy
this code caught my eye:
$sql = "SELECT COUNT(*) FROM {node_access} WHERE (nid = 0 OR nid = %d) $grants_sql AND grant_$op >= 1";(from http://api.drupal.org/api/function/node_access/6).
$op *should* be one of "view", "update", "create" or "delete" and so *should* be safe ... if a variable is used (e.g. in a custom/contrib module) to pass $op to node_access(), and if that variable is derived from user input, then we are relying on the module to sanitise $op correctly to prevent SQL injection.
it should actually use http://api.drupal.org/api/function/db_escape_table/6 to make sure that the constructed table name is well-formed.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 278675-SA-2009-01-D7.patch | 1.52 KB | dave reid |
| #12 | node-access-sdo590-D6-7.patch | 784 bytes | pwolanin |
| #9 | node_278675.patch | 1.11 KB | drewish |
| #3 | node_access.patch | 736 bytes | robertdouglass |
| #2 | grant-op-278675-6x-2.patch | 854 bytes | pwolanin |
Comments
Comment #1
pwolanin commentedTrivial patch, but not tested.
Comment #2
pwolanin commentedhere's the 6.x backport
Comment #3
robertdouglass commentedI wonder if we shouldn't somehow validate $op? Perhaps both changes are advisable, but here's another patch to consider. Taking the time to validate $op seems like a better way to handle the issue to me. If we had the concept of error handling in Drupal we'd actually throw an error if $op were not one of the allowed values. I think it's rather critical to not only ensure no SQL injection can occur (like the suggested patch), but that the allowed values actually match the expected columns of the database.
The question remains whether anyone extends the {node_access} table with other grants, in which case this patch is undesirable, but other than that, I prefer it.
Comment #4
pwolanin commentedI'd rather not fix the op values - is there anything that says another module can't define another op? I guess new columns would have to be added? But hook_schema_alter is available for just such a reason.
Comment #5
robertdouglass commentedOk, I'm convinced. Best to go with #2 for now.
Comment #6
paul.lovvik commentedReviewed both patches (#1 and #2) and verified that they work on their respective branches. Both look good.
Comment #7
catch#1 and #2 still apply.
Comment #8
catchNeeds a re-roll for dbtng.
Comment #9
drewish commentedso here's the minimal DBTNG udpate. looking at the conditionals code I'm not seeing a good way to ensure the order of operations on nested OR and AND operators. i'm not convinced that the conditions framework is expressive enough at this point. maybe we can get this committed then revisit the full query.
i started looking for the unit tests for node_access and it doesn't look like there are any... that's really freaking scary... i'll see if there's an issue for that some place.
Comment #10
drewish commentedCrell explained to me how to get the conditions to do what they'd need to do but I'm not going to say we should hold up the patch over it. If i get a chance I'll pick this back up and work on converting it to a full DBTNG dynamic query.
And look at adding some unit tests... eek.
Comment #11
catchThis is already in 6.x as of http://drupal.org/node/358957 so needs forward porting from there.
Comment #12
pwolanin commentedhere's the patch that went into D6
Comment #13
dave reidRerolled along with some trivial whitespace changes in node.module picked up by my IDE.
Comment #14
dries commentedCommitted to CVS HEAD. Thanks!
Comment #16
pwolanin commentedtagging