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

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect, P1)

Description

[Bradley Baetz (:bbaetz)]

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

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: 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....

Comment 2

[Frédéric Buclin]

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.

Comment 3

[Frédéric Buclin]

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 4

[Marc Schumann [:Wurblzap]]

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">

Comment 5

[Frédéric Buclin]

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

[loren]

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

Comment 7

[Bradley Baetz (:bbaetz)]

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?

Comment 8

[Marc Schumann [:Wurblzap]]

(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?

Comment 9

[Bradley Baetz (:bbaetz)]

Attachment: v2

ok, new go. This is for 3.0 (and for backporting) only. HEAD will get safety checks.

Comment 10

[Frédéric Buclin]

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 11

[Frédéric Buclin]

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 12

[Frédéric Buclin]

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.

Comment 13

[Max Kanat-Alexander]

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.

Comment 14

[Frédéric Buclin]

Attachment: 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).

Comment 15

[Bradley Baetz (:bbaetz)]

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

[Max Kanat-Alexander]

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?

Comment 17

[Max Kanat-Alexander]

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.

Comment 18

[Frédéric Buclin]

(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.

Comment 19

[Bradley Baetz (:bbaetz)]

Attachment: patch for 3.0.x, v2.1

Like this?

Comment 20

[Frédéric Buclin]

Comment on attachment 312406 [details] [diff] [review]
patch for 3.0.x, v2.1

OK, I'm fine with this fix too. r=LpSolit

Comment 21

[Bradley Baetz (:bbaetz)]

As discussed, I dno't have time to backport it ATM

Comment 22

[Frédéric Buclin]

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).

Comment 23

[Frédéric Buclin]

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.

Comment 24

[Frédéric Buclin]

Attachment: patch for 3.1.x, v1

Patch for tip, which requires an additional fix, as explained in my previous comment above.

Comment 25

[Frédéric Buclin]

Attachment: patch for 2.20.x and 2.22.x, v1

Comment 26

[Marc Schumann [:Wurblzap]]

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?

Comment 27

[Marc Schumann [:Wurblzap]]

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.)

Comment 28

[Marc Schumann [:Wurblzap]]

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.

Comment 29

[Max Kanat-Alexander]

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.

Comment 30

[Max Kanat-Alexander]

Comment on attachment 312406 [details] [diff] [review]
patch for 3.0.x, v2.1

Works great and looks fine. :-)

Comment 31

[Max Kanat-Alexander]

Comment on attachment 312418 [details] [diff] [review]
patch for 2.20.x and 2.22.x, v1

Looks great and works by testing!

Comment 32

[Max Kanat-Alexander]

Okay, holding approval until release.

Comment 33

[Frédéric Buclin]

For the Sec Adv: security hole introduced by bug 158499 in Bugzilla 2.17.2.

Comment 34

[Frédéric Buclin]

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

Comment 35

[Max Kanat-Alexander]

Security advisory sent, removing bugs from security group.