SA-CORE-2009-001: possible SQL injection risk in node_access()
pwolanin - July 4, 2008 - 20:58
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | node.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | security.drupal.org |
Description
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.
#1
Trivial patch, but not tested.
#2
here's the 6.x backport
#3
I 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.
#4
I'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.
#5
Ok, I'm convinced. Best to go with #2 for now.
#6
Reviewed both patches (#1 and #2) and verified that they work on their respective branches. Both look good.
#7
#1 and #2 still apply.
#8
Needs a re-roll for dbtng.
#9
so 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.
#10
Crell 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.
#11
This is already in 6.x as of http://drupal.org/node/358957 so needs forward porting from there.
#12
here's the patch that went into D6
#13
Rerolled along with some trivial whitespace changes in node.module picked up by my IDE.
#14
Committed to CVS HEAD. Thanks!
#15
Automatically closed -- issue fixed for 2 weeks with no activity.
#16
tagging