| Project: | Privatemsg |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Currently, this function does use the following code to insert parameters into the query:
[php]
$query = vsprintf($query, $QUERY_ARGS);
$count = vsprintf($count, $QUERY_ARGS);
[/php]
This does have two problems.
a) Using arbitrary % (for queries using LIKE as it is done in #314327: Tagging architecture.) becomes *very* ugly, because the % have to be escaped twice to get past privatemsg_assemble_query AND db_query)
b) It's a security problem as soon as we use string arguments (AFAIK we currently don't do that but again, the filter module does change this) because they are not escaped in any way.
Suggested solution:
I think, the best solution is to give that task to the functions it belongs... db_query, pager_query and so on. The paging-implementation already changed the return value to an array, we can simply add another key 'args' and forward it to the db_query function.
Comments
#1
Solution reported by litwol attached as a patch.
This does solve b) fine. It does not help with a) but this is not so important with the current solution. This would change it we wanted to allow the users to decide themself if they want to use placeholders or not. And LIKE queries with % are not often used.
#2
I assume this still needs to go in? ASAP?
#3
Suppose it's security hole so query builder should be completely reviewed against escaping values that comes from user input
#4
There is no open security hole in the current version, because privatemsg_filter does escape the search string itself (AFAIK this is currently the only string parameter in the quiery builder).
However, the use of placeholder should be secure because people writing sub-modules and extensions will probably rely on it.
An important note: When the query builder is changed, the explicit escape in privatemsg_filter needs to be removed or parameters will get escaped twice. I'll update my patch as soon as i have time.
#5
Update:
I've already removed the db_escape_string calls as I used the patched version of privatemsgapi.inc und thought this would go in before the tagging issue.
What actually means yes, this needs to go in asap.
#6
Its in.
For educational purposes notice this commit which went in right after: http://drupal.org/cvs?commit=159005
Every time we run a new preg_replace_callback(DB_QUERY_REGEXP, '_db_query_callback', $query) we must initialize the variables for replacement.
#7
Automatically closed -- issue fixed for two weeks with no activity.