See: http://drupal.org/node/167284

%s should never be used without quotes around it. There are some bad uses in project*, but Chad and I looked at the code and none look like actual vulnerabilities.

grep turns up these 2 in project_issue:

issue.inc:444: 

$result = db_query(db_rewrite_sql("SELECT n.nid, n.title FROM {node} n WHERE n.type = 'project_project' AND n.status = 1". ($nids ?  " AND n.nid NOT IN (%s)" : "") ." ORDER BY n.title"), implode(',', $nids));

mail.inc:320: 

$result = db_query('SELECT p.*, u.name, u.mail FROM {project_subscriptions} p INNER JOIN {users} u ON p.uid = u.uid WHERE u.status = 1 AND p.nid = %d AND (p.level = 2 OR (p.level = 1 AND (%s)))', $node->pid, implode(' OR ', $uids));
CommentFileSizeAuthor
#12 s_fix_0.patch2.94 KBhunmonk
#9 s_fix.patch1.92 KBhunmonk
#3 fix_pi_query_2.patch1.99 KBpwolanin
#2 fix_pi_query_1.patch2.04 KBpwolanin

Comments

hunmonk’s picture

hunmonk commented
Priority: Normal » Critical

since this is a future defensive programming change, i'm not going to hold up our pending release for it. up'ing the priority to make sure it gets taken care of soon, though.

pwolanin’s picture

pwolanin commented
Status: Active » Needs work
StatusFileSize
new fix_pi_query_1.patch2.04 KB

here's a start

pwolanin’s picture

pwolanin commented
StatusFileSize
new fix_pi_query_2.patch1.99 KB

or this is simpler and just as safe.

hunmonk’s picture

hunmonk commented

i'm not a fan of this approach at all. we need to use "uid = %d", implode those, and put the uid values into an array along w/ $node->pid, which can then be passed into the query for proper filtering.

pwolanin’s picture

pwolanin commented

@hunmonk - ok, so I'll use the patch in #2 as the starting point for any further testing/changes.

dww’s picture

dww
we/he/they
commented

Yeah, in spite of it being a little more complex, i think i prefer #2 as well. Why is it CNW? Only because there's no equivalent change for issue.inc, only mail.inc? Anything wrong with #2 for the mail.inc case? I don't see it based on a quick review.

Although... it's not clear why you're bothering to use indexes for the $uid_conditions and $uids arrays. Doesn't look like the code depends on them at all. It'd take less space and would be more readable to write, for example:

   $uid_conditions[] = "u.uid = %d";
   $uids[] = $node->assigned;

Any reason you didn't go that route?

dww’s picture

dww
we/he/they
commented

(presumably because the existing code needlessly uses an index there...) ;)

hunmonk’s picture

hunmonk commented
Status: Needs work » Needs review

the implementation felt a bit heavy to me. cleaned it up a little based on dww's work in the initial %s fix commit.

untested.

hunmonk’s picture

hunmonk commented
StatusFileSize
new s_fix.patch1.92 KB

y el patcho...

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

Looks ok to me. FYI: hunmonk and I discussed why we use the index: it prevents duplicate SQL clauses when a given user participated in the same issue more than once. So, it's good we're keeping that.

However, we're still missing a patch for issue.inc here...

pwolanin’s picture

pwolanin commented

@dww - I used the indices simply to mimic the original code - I didn't look that closely, but I assumed the original code was that way since the same nid may be referenced multiple times in the loop.

hunmonk’s picture

hunmonk commented
Status: Needs work » Needs review
StatusFileSize
new s_fix_0.patch2.94 KB

with issue.inc rolled in...

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

Looks great, and light testing on a local server went fine. I only tested the email stuff via devel.module's logging of drupal_mail(), but I'm confident enough in the visual inspection of the code that this should just go in. Thanks!

hunmonk’s picture

hunmonk commented
Status: Reviewed & tested by the community » Fixed

committed to 4.7.x-1.x, 4.7.x-2.x, HEAD.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)