[SECURITY] XSS in show_bug.cgi: id isn't filtered for format=multiple

RESOLVED FIXED in Bugzilla 2.20

Status

()

defect
P1
critical
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: bbaetz, Assigned: LpSolit)

Tracking

2.17.2
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +
blocking3.2 +
approval3.0 +
blocking3.0.4 +
approval2.22 +
blocking2.22.4 +
approval2.20 +
blocking2.20.6 +

Details

()

Attachments

(3 attachments, 3 obsolete attachments)

id isn't filtered for format=multiple.

Noted by lbutler on IRC, via some automated XSS checker thing (lbutler - can you add details?).

lpsolit verfied on 2.20 :(

will look into it and do patch in a sec
Flags: blocking3.2?
Flags: blocking3.0.4?
Flags: blocking2.22.4?
Flags: blocking2.20.6?
Flags: blocking3.2?
Flags: blocking3.2+
Flags: blocking3.0.4?
Flags: blocking3.0.4+
Flags: blocking2.22.4?
Flags: blocking2.22.4+
Flags: blocking2.20.6?
Flags: blocking2.20.6+
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.20
Version: 3.0.3 → 2.20.5
Posted patch patch against 3.0 (obsolete) — Splinter Review
This is against 3.0. Can someone else do the back/forward ports - its after midnight here, and others are in the right timezone....
Attachment #312265 - Flags: review?(LpSolit)
Comment on attachment 312265 [details] [diff] [review]
patch against 3.0

You have to remove bug.bug_id from filterexceptions.pl, else runtests.pl complains.
Attachment #312265 - Flags: review?(LpSolit) → review-
Note that despite $bug is an object, $bug->bug_id returns a *string* if the bug doesn't exist, instead of the expected bug ID taken from the DB. That's a pretty bad behavior. We should rather add the invalid string into another object attribute, like $bug->{'invalid_bug_alias'} or something, so that we know we cannot trust it. I don't want to do a full scan of all templates to see where bug.bug_id is assumes trusted.
Comment on attachment 312265 [details] [diff] [review]
patch against 3.0

Something needs to be done to template/en/default/global/per-bug-queries.html.tmpl, too. The vulnerable part is $bugids there:

        <input type="text" name="bug_ids" size="12" maxlength="80"
         [%- " value=\"$bugids\"" IF bugids %]>
        <input type="submit" value="Commit" id="commit_list_of_bugs">
Attachment #312265 - Flags: review-
This bad behavior also exists in Bugzilla 2.20.5, so this is not a regression, at least not in the last 4 years.
Path:	/bugzilla/show_bug.cgi
Headers:	Content-Type=application%2Fx-www-form-urlencoded
Body:	id=>"><script>alert(123)</script><"
id=81
id=82
id=59
id=47
id=52
id=99
id=57
id=60
id=63
format=multiple

=========================================================

in the html output it spits out:

<div id="bugzilla-body"><h1>Bug
<a href="show_bug.cgi?id=>"><script>alert(123)</script><"">>"><script>alert(123)</script><"</a>

and thus shows a script window displaying 123, and a warning (if you have a blocker) about cross site scripting


Running version (direct output of rel-notes)::
bugzilla.org links for release notes
------------------------------------
3.0.2: http://www.bugzilla.org/releases/3.0.2/release-notes.html

this comes out of the error handling stuff in bug.pm, which is done to handle XML stuff (which does filter correctly)

As lpsolit mentioned on IRC, for safety we should probably return an unblessed object with { error => 'foo', 'requested_id' => $orig_bug_id } to avoid this sort of confusion, and change the few templates that need to care. Alternately we could just throw from within the Bug constructor and let the caller do whatever it needs to.....

I don't think that per-bug-queries is the same issue - it doesn't trigger for me - not sure why?
Blocks: 425110
(In reply to comment #7)
> I don't think that per-bug-queries is the same issue - it doesn't trigger for
> me - not sure why?

Maybe you have your “Enable tags for bugs” user preference turned off?
Posted patch v2 (obsolete) — Splinter Review
ok, new go. This is for 3.0 (and for backporting) only. HEAD will get safety checks.
Attachment #312265 - Attachment is obsolete: true
Attachment #312273 - Flags: review?(LpSolit)
Maybe is it time for Bugzilla::Bug->new() to return undef if the param doesn't match anything, and let pages handle this correctly. This would fix bug 348397 (another security bug) at the same time.
Comment on attachment 312273 [details] [diff] [review]
v2

>Index: template/en/default/global/per-bug-queries.html.tmpl

>-         [%- " value=\"$bugids\"" IF bugids %]>

You have to remove this entry from filterexceptions.pl as well. I still think the right fix is to not return a fake bug.bug_id if the pseudo-alias doesn't match anything.
Comment on attachment 312273 [details] [diff] [review]
v2

>Index: template/en/default/global/per-bug-queries.html.tmpl

>+         [% IF bugids %]
>+           value="[% bugids FILTER html %]"
>+         [% END %]

This won't work. It will still have the invalid string, and clicking "Commit" will throw an error. Only valid bug IDs should be passed here.
Attachment #312273 - Flags: review?(LpSolit) → review-
bug_id should always return a number. Always, always.

Yes, Bugzilla::Bug->new should return undef, but we can't backport that to 2.20, or even to 3.0.

Bugzilla::Bug->new should not return an unblessed reference. That would lead to a lot of confusion in the calling code.

The simple solution is just to have bug_id always return a number and deal with the error string another way.
Severity: blocker → critical
Summary: XSS in show_bug.cgi → XSS in show_bug.cgi: id isn't filtered for format=multiple
Posted patch patch, v3 (obsolete) — Splinter Review
This is the less invasive fix possible, and also the safest one. Now we are sure you cannot use XSS here. And I *really* don't mind if the string appears double-escaped somewhere. 99% of aliases don't use <, >, & or " anyway (I would even tend to forbid them in _check_alias(), but that's another story).
Assignee: query-and-buglist → LpSolit
Attachment #312273 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #312398 - Flags: review?(mkanat)
Comment on attachment 312398 [details] [diff] [review]
patch, v3

This technically breaks the xml output; if someone passes in a string (to be treated as an alias), the returned error object won't match what they passed in due to the extra escaping, so code can't report a useful error.

What I had, when an extra grep { !$_->error } before populating the tags thing (rather than doing it in the filter) I think is less invasive.
Comment on attachment 312398 [details] [diff] [review]
patch, v3

For the tip I'd like to fix it in some better way, but this is fine for branches.

This will need a backport to older branches, since they have slightly different code, I think.

Is bug.bug_id already in filterexceptions for show-multiple?
Attachment #312398 - Flags: review?(mkanat) → review+
Well, I wouldn't object to a fix that I was sure only affected *this*--that's the one thing that does make me slightly nervous about LpSolit's patch, is that it affects all of Bugzilla.
(In reply to comment #15)
> This technically breaks the xml output; if someone passes in a string (to be

I know, and I *really* don't mind, as discussed on IRC. Implementing $bug->{bug_id} this way to return an unsafe string is a big error. This has always been so, since rev 1.1 by endico in June 2000. And as I said, 99% of aliases don't use <, >, " or & anyway, so that's really a *minor* annoyance (not important enough to fight against this XSS attack).


> What I had, when an extra grep { !$_->error } before populating the tags thing
> (rather than doing it in the filter) I think is less invasive.

It's not less invasive, and it's not safer as we would have to make sure we caught all unfiltered places.


(In reply to comment #16)
> For the tip I'd like to fix it in some better way, but this is fine for
> branches.

I would prefer to go this way for now, so that we can release asap, and file a separate bug to think a bit more about how to fix it in a clever way for 3.2.


> This will need a backport to older branches, since they have slightly different
> code, I think.

Sure, though not very different. :)


> Is bug.bug_id already in filterexceptions for show-multiple?

Yes, which is why you can trigger the XSS attack.
Like this?
Attachment #312406 - Flags: review?(mkanat)
Comment on attachment 312406 [details] [diff] [review]
patch for 3.0.x, v2.1

OK, I'm fine with this fix too. r=LpSolit
Attachment #312406 - Flags: review+
Assignee: LpSolit → bbaetz
Status: ASSIGNED → NEW
As discussed, I dno't have time to backport it ATM
Assignee: bbaetz → LpSolit
Note that there is a 2nd XSS hole in 3.2, due to bug 389541:

http://mxr.mozilla.org/mozilla/source/webtools/bugzilla/template/en/default/bug/show-multiple.html.tmpl#44

[% ids.join(",") FILTER none %] contains the unsafe string, and it's not filtered, so the XSS attack is still possible. Two ways to fix them:
1) Write [% ids.push(bug.bug_id) UNLESS bug.error %]
2) Write [% ids.join(",") FILTER html %]

I suggest to combine them both.

(now you understand why I like fixing the problem at its root rather than tracking all places which don't filter bug IDs).
Status: NEW → ASSIGNED
Comment on attachment 312406 [details] [diff] [review]
patch for 3.0.x, v2.1

This patch only works for 3.0 as 3.2 needs an additional fix, and 2.x don't have tags.
Attachment #312406 - Attachment description: 2b → patch for 3.0.x, v2.1
Patch for tip, which requires an additional fix, as explained in my previous comment above.
Attachment #312398 - Attachment is obsolete: true
Attachment #312411 - Flags: review?(mkanat)
Attachment #312418 - Flags: review?(mkanat)
Comment on attachment 312411 [details] [diff] [review]
patch for 3.1.x, v1

This correctly fixes the issue at hand: tested, works; r=Wurblzap. Did we agree to fix bug_id in a follow-up bug yet?
Attachment #312411 - Flags: review+
Comment on attachment 312406 [details] [diff] [review]
patch for 3.0.x, v2.1

r=Wurblzap by comparing it to the tip patch. (Untested -- the 3.0 branch apparently stopped working on my computer some time ago, which may be related to me installing Perl 5.10.)
Attachment #312406 - Flags: review+
Comment on attachment 312418 [details] [diff] [review]
patch for 2.20.x and 2.22.x, v1

Same here -- r=Wurblzap by comparing it to the tip patch; untested.
Attachment #312418 - Flags: review+
Comment on attachment 312411 [details] [diff] [review]
patch for 3.1.x, v1

Looks great. Tested it and it seems to work just fine and not break anything.
Attachment #312411 - Flags: review?(mkanat) → review+
Comment on attachment 312406 [details] [diff] [review]
patch for 3.0.x, v2.1

Works great and looks fine. :-)
Attachment #312406 - Flags: review?(mkanat) → review+
Comment on attachment 312418 [details] [diff] [review]
patch for 2.20.x and 2.22.x, v1

Looks great and works by testing!
Attachment #312418 - Flags: review?(mkanat) → review+
Okay, holding approval until release.
Flags: approval?
Flags: approval3.0?
Flags: approval2.22?
Flags: approval2.20?
For the Sec Adv: security hole introduced by bug 158499 in Bugzilla 2.17.2.
Depends on: 158499
Summary: XSS in show_bug.cgi: id isn't filtered for format=multiple → [SECURITY] XSS in show_bug.cgi: id isn't filtered for format=multiple
Version: 2.20.5 → 2.17.2
Flags: approval?
Flags: approval3.0?
Flags: approval3.0+
Flags: approval2.22?
Flags: approval2.22+
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
tip:

Checking in show_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
new revision: 1.53; previous revision: 1.52
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.111; previous revision: 1.110
done
Checking in template/en/default/bug/show-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show-multiple.html.tmpl,v  <--  show-multiple.html.tmpl
new revision: 1.42; previous revision: 1.41
done


3.0.3:

Checking in show_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
new revision: 1.50.2.1; previous revision: 1.50
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.95.2.2; previous revision: 1.95.2.1
done
Checking in template/en/default/bug/show-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show-multiple.html.tmpl,v  <--  show-multiple.html.tmpl
new revision: 1.34.2.1; previous revision: 1.34
done


2.22.3:

Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.61.2.7; previous revision: 1.61.2.6
done
Checking in template/en/default/bug/show-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show-multiple.html.tmpl,v  <--  show-multiple.html.tmpl
new revision: 1.26.2.1; previous revision: 1.26
done


2.20.5:

Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.43.2.8; previous revision: 1.43.2.7
done
Checking in template/en/default/bug/show-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show-multiple.html.tmpl,v  <--  show-multiple.html.tmpl
new revision: 1.24.6.3; previous revision: 1.24.6.2
done
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Security advisory sent, removing bugs from security group.
Group: webtools-security
You need to log in before you can comment on or make changes to this bug.