Column name in node access query is not checked for corr
pwolanin - July 4, 2008 - 20:58
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | node.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (code needs work) |
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.