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

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
Query/Bug List
P1
critical
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: bbaetz, Assigned: Frédéric Buclin)

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

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

1.90 KB, patch
Max Kanat-Alexander
: review+
Frédéric Buclin
: review+
Wurblzap
: review+
Details | Diff | Splinter Review
2.58 KB, patch
Max Kanat-Alexander
: review+
Wurblzap
: review+
Details | Diff | Splinter Review
1.61 KB, patch
Max Kanat-Alexander
: review+
Wurblzap
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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?
(Assignee)

Updated

10 years ago
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
(Reporter)

Comment 1

10 years ago
Created attachment 312265 [details] [diff] [review]
patch against 3.0

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)
(Assignee)

Comment 2

10 years ago
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-
(Assignee)

Comment 3

10 years ago
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-
(Assignee)

Comment 5

10 years ago
This bad behavior also exists in Bugzilla 2.20.5, so this is not a regression, at least not in the last 4 years.

Comment 6

10 years ago
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

(Reporter)

Comment 7

10 years ago
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?
(Reporter)

Updated

10 years ago
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?
(Reporter)

Comment 9

10 years ago
Created attachment 312273 [details] [diff] [review]
v2

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)
(Assignee)

Comment 10

10 years ago
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.
(Assignee)

Comment 11

10 years ago
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.
(Assignee)

Comment 12

10 years ago
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-

Comment 13

10 years ago
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
(Assignee)

Comment 14

10 years ago
Created attachment 312398 [details] [diff] [review]
patch, v3

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)
(Reporter)

Comment 15

10 years ago
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 16

10 years ago
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+

Comment 17

10 years ago
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.
(Assignee)

Comment 18

10 years ago
(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.
(Reporter)

Comment 19

10 years ago
Created attachment 312406 [details] [diff] [review]
patch for 3.0.x, v2.1

Like this?
Attachment #312406 - Flags: review?(mkanat)
(Assignee)

Comment 20

10 years ago
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)

Updated

10 years ago
Assignee: LpSolit → bbaetz
Status: ASSIGNED → NEW
(Reporter)

Comment 21

10 years ago
As discussed, I dno't have time to backport it ATM
Assignee: bbaetz → LpSolit
(Assignee)

Comment 22

10 years ago
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
(Assignee)

Comment 23

10 years ago
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
(Assignee)

Comment 24

10 years ago
Created attachment 312411 [details] [diff] [review]
patch for 3.1.x, v1

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)
(Assignee)

Comment 25

10 years ago
Created attachment 312418 [details] [diff] [review]
patch for 2.20.x and 2.22.x, v1
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 29

9 years ago
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 30

9 years ago
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 31

9 years ago
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+

Comment 32

9 years ago
Okay, holding approval until release.
Flags: approval?
Flags: approval3.0?
Flags: approval2.22?
Flags: approval2.20?
(Assignee)

Comment 33

9 years ago
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
(Assignee)

Updated

9 years ago
Flags: approval?
Flags: approval3.0?
Flags: approval3.0+
Flags: approval2.22?
Flags: approval2.22+
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
(Assignee)

Comment 34

9 years ago
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
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 35

9 years ago
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.