Closed
Bug 355728
Opened 18 years ago
Closed 18 years ago
[SECURITY] XSS in the "id" parameter of showdependencygraph when "doall" is set
Categories
(Bugzilla :: Creating/Changing Bugs, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: mkanat, Assigned: mkanat)
References
()
Details
(Whiteboard: [ready for 2.18.6][ready for 2.20.3][ready for 2.22.1][ready for 2.23.3])
Attachments
(3 files, 2 obsolete files)
858 bytes,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
969 bytes,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
1.70 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
Reported to support-bugzilla: It looks like if you pass JavaScript in the 'id' parameter it gets placed verbatim into the 'value' parameter of the 'id' input field. I adapted a URL from their warning text to try to highlight the problem: showdependencygraph.cgi?doall=80000001&id=%27%22%3E%3Cscript+id%3D%2280000000%22%3Ealert%28%22hi%21%22%29%3B%3C%2Fscript%3E+&rankdir=80000001&showsummary=%27+having+1%3D1-
Updated•18 years ago
|
Comment 1•18 years ago
|
||
I can reproduce the JS injection on both 2.18.6 and tip. As it seems very specific to showdependencygraph.cgi, it's fine to take it now during our QA process. And I know mkanat is working on it already.
Flags: blocking3.0?
Flags: blocking2.22.1?
Flags: blocking2.20.3?
Flags: blocking2.18.6?
Version: 2.23 → 2.18.5
Assignee | ||
Updated•18 years ago
|
Flags: blocking3.0?
Flags: blocking3.0+
Flags: blocking2.22.1?
Flags: blocking2.22.1+
Flags: blocking2.20.3?
Flags: blocking2.20.3+
Flags: blocking2.18.6?
Flags: blocking2.18.6+
Assignee | ||
Comment 2•18 years ago
|
||
Okay. Here's the least invasive fix possible.
Assignee: create-and-change → mkanat
Status: NEW → ASSIGNED
Attachment #241461 -
Flags: review?(LpSolit)
Assignee | ||
Comment 3•18 years ago
|
||
Comment on attachment 241461 [details] [diff] [review] v1 (all branches) Actually, this file applies cleanly on every branch, not just on the tip.
Attachment #241461 -
Attachment description: v1 (tip) → v1 (all branches)
Comment 4•18 years ago
|
||
Comment on attachment 241461 [details] [diff] [review] v1 (all branches) >-$vars->{'bug_id'} = $cgi->param('id'); >+my @bugs = map { detaint_natural($_) } ($cgi->param('id')); >+$vars->{'bug_id'} = join(', ', @bugs); This is not the right fix. In the "Bug numbers:" text field, if you enter a comma separated list of bug IDs (which is a valid action), this list is cleared by your patch as $cgi->param('id') is a scalar, not an array. IMO, what we should do is to delete $cgi->param('id') if $cgi->param('doall') is defined and relax the if (!defined $cgi->param('id') && !defined $cgi->param('doall')) { ThrowCodeError("missing_bug_id"); } test a bit, replacing && by ||. If we want all bugs, then we shouldn't care about bug_id. If 'doall' is not given, then } else { foreach my $i (split('[\s,]+', $cgi->param('id'))) { $i = trim($i); ValidateBugID($i); $baselist{$i} = 1; } my @stack = keys(%baselist); is called and $cgi->param('id') should be regenerated: $cgi->param('id') = join(',', @stack);
Attachment #241461 -
Flags: review?(LpSolit) → review-
Comment 5•18 years ago
|
||
Maybe I'm to naïve, but shouldn't it be possible to simply filter the bugger on output? Worst you'd end up when clicking such a doctored link would be an input field with suspicious content, right?
Assignee | ||
Comment 6•18 years ago
|
||
(In reply to comment #5) > Maybe I'm to naïve, but shouldn't it be possible to simply filter the bugger > on output? Worst you'd end up when clicking such a doctored link would be an > input field with suspicious content, right? Yeah. I already have a patch that filters the output, I just thought that the two-line solution to filter the input was simpler, since it's a security bug. Filtering the output is much messier. But I can attach that instead, possibly.
Comment 7•18 years ago
|
||
I don't like the idea of filtering the output. The data given by the user should be sanitized. That's how we do in all other scripts and is by far the safest solution.
Assignee | ||
Comment 8•18 years ago
|
||
Okay, here's the patch. I tested this one, and it works fine. The code part works on all branches. The release notes part only applies to 2.22 and the tip, of course.
Attachment #241461 -
Attachment is obsolete: true
Attachment #241529 -
Flags: review?(LpSolit)
Assignee | ||
Comment 9•18 years ago
|
||
Here's the Release Notes part of the patch for 2.20.
Attachment #241531 -
Flags: review?(LpSolit)
Assignee | ||
Comment 10•18 years ago
|
||
And this fixes the Release Notes for 2.18.
Attachment #241532 -
Flags: review?(LpSolit)
Assignee | ||
Comment 11•18 years ago
|
||
LpSolit pointed out on IRC that the previous patch could throw undef warnings during the join, if detaint_natural failed. He suggested this fix, which I thought was pretty clever. :-)
Attachment #241529 -
Attachment is obsolete: true
Attachment #241535 -
Flags: review?(LpSolit)
Attachment #241529 -
Flags: review?(LpSolit)
Comment 12•18 years ago
|
||
Comment on attachment 241535 [details] [diff] [review] v3 Tested on all 4 branches. This correctly fixes the problem and the web server error log doesn't report any error anymore. r=LpSolit for all branches.
Attachment #241535 -
Flags: review?(LpSolit) → review+
Updated•18 years ago
|
Flags: approval?
Flags: approval2.22?
Flags: approval2.20?
Flags: approval2.18?
Whiteboard: [ready for 2.18.6][ready for 2.20.3][ready for 2.22.1][ready for 2.23.3]
Comment 13•18 years ago
|
||
Comment on attachment 241531 [details] [diff] [review] Release Notes Fix (2.20) >-prior to 2.20.3. None of them are considered to be of critical >-severity, but we still strongly recommend that you update any >+prior to 2.20.3. We still strongly recommend that you update any > 2.20.x installation to 2.20.3. Nit: not sure that "still" should be kept. R=LpSolit
Attachment #241531 -
Flags: review?(LpSolit) → review+
Comment 14•18 years ago
|
||
Comment on attachment 241532 [details] [diff] [review] Release Notes Fix (2.18) r=LpSolit
Attachment #241532 -
Flags: review?(LpSolit) → review+
Assignee | ||
Comment 15•18 years ago
|
||
For reference, this was introduced during the templatization of showdependencygraph.cgi, in bug 126793. Previous to the templatization, there was a value_quote() around the id. In the original template, bug_id was filtered! But bbaetz commented that it was unnecessary to filter the id, here: https://bugzilla.mozilla.org/show_bug.cgi?id=126793#c10 justdave replied that it didn't hurt. However, it looks like on checkin, the filter was removed. I'm not trying to put blame on anybody--we've all made mistakes, and some of them worse than this. I just wanted to note the story so that we could avoid it in the future. Bugzilla has been affected since 2.15. So, the lesson is, when in doubt, FILTER html.
Depends on: 126793
Version: 2.18.5 → 2.15
Updated•18 years ago
|
Flags: approval?
Flags: approval2.22?
Flags: approval2.22+
Flags: approval2.20?
Flags: approval2.20+
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
Comment 16•18 years ago
|
||
tip: Checking in showdependencygraph.cgi; /cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v <-- showdependencygraph.cgi new revision: 1.54; previous revision: 1.53 done Checking in docs/rel_notes.txt; /cvsroot/mozilla/webtools/bugzilla/docs/rel_notes.txt,v <-- rel_notes.txt new revision: 1.42; previous revision: 1.41 done 2.22: Checking in showdependencygraph.cgi; /cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v <-- showdependencygraph.cgi new revision: 1.48.2.1; previous revision: 1.48 done Checking in docs/rel_notes.txt; /cvsroot/mozilla/webtools/bugzilla/docs/rel_notes.txt,v <-- rel_notes.txt new revision: 1.36.2.6; previous revision: 1.36.2.5 done 2.20.2: Checking in showdependencygraph.cgi; /cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v <-- showdependencygraph.cgi new revision: 1.38.4.3; previous revision: 1.38.4.2 done Checking in docs/rel_notes.txt; /cvsroot/mozilla/webtools/bugzilla/docs/rel_notes.txt,v <-- rel_notes.txt new revision: 1.32.2.7; previous revision: 1.32.2.6 done 2.18.5: Checking in showdependencygraph.cgi; /cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v <-- showdependencygraph.cgi new revision: 1.35.2.2; previous revision: 1.35.2.1 done Checking in docs/rel_notes.txt; /cvsroot/mozilla/webtools/bugzilla/docs/rel_notes.txt,v <-- rel_notes.txt new revision: 1.24.2.15; previous revision: 1.24.2.14 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Summary: XSS in the "id" parameter of showdependencygraph when "doall" is set → [SECURITY] XSS in the "id" parameter of showdependencygraph when "doall" is set
Assignee | ||
Comment 17•18 years ago
|
||
Security Advisory has been sent, so this bug is no longer private.
Group: webtools-security
You need to log in
before you can comment on or make changes to this bug.
Description
•