Closed Bug 355728 Opened 13 years ago Closed 13 years ago

[SECURITY] XSS in the "id" parameter of showdependencygraph when "doall" is set

Categories

(Bugzilla :: Creating/Changing Bugs, defect, critical)

2.15
defect
Not set
critical

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)

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-
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
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+
Blocks: 346525
Attached patch v1 (all branches) (obsolete) — Splinter Review
Okay. Here's the least invasive fix possible.
Assignee: create-and-change → mkanat
Status: NEW → ASSIGNED
Attachment #241461 - Flags: review?(LpSolit)
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 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-
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?
(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.
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.
Attached patch v2 (tip and 2.22) (obsolete) — Splinter Review
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)
Here's the Release Notes part of the patch for 2.20.
Attachment #241531 - Flags: review?(LpSolit)
And this fixes the Release Notes for 2.18.
Attachment #241532 - Flags: review?(LpSolit)
Attached patch v3Splinter Review
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 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+
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 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 on attachment 241532 [details] [diff] [review]
Release Notes Fix (2.18)

r=LpSolit
Attachment #241532 - Flags: review?(LpSolit) → review+
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
Flags: approval?
Flags: approval2.22?
Flags: approval2.22+
Flags: approval2.20?
Flags: approval2.20+
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
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: 13 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
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.