Last Comment Bug 842038 - (CVE-2013-0785) [SECURITY] XSS in show_bug.cgi when using an invalid page format
(CVE-2013-0785)
: [SECURITY] XSS in show_bug.cgi when using an invalid page format
Status: RESOLVED FIXED
: sec-high, wsec-xss
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 2.10
: All All
: -- critical (vote)
: Bugzilla 3.6
Assigned To: Frédéric Buclin
: default-qa
:
Mentors:
https://bugzilla.mozilla.org/show_bug...
: 842096 842298 (view as bug list)
Depends on:
Blocks: 835424 832267
  Show dependency treegraph
 
Reported: 2013-02-16 08:30 PST by SimranJeet Singh
Modified: 2014-07-24 16:08 PDT (History)
16 users (show)
LpSolit: approval+
LpSolit: approval4.4+
LpSolit: blocking4.4+
LpSolit: approval4.2+
LpSolit: blocking4.2.5+
LpSolit: approval4.0+
LpSolit: blocking4.0.10+
LpSolit: approval3.6+
LpSolit: blocking3.6.13+
dchanm+bugzilla: sec‑bounty+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch for 4.4 and trunk, v1 (1.92 KB, patch)
2013-02-16 11:49 PST, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch for 4.2, v1 (1.44 KB, patch)
2013-02-16 11:59 PST, Frédéric Buclin
glob: review+
Details | Diff | Splinter Review
patch for 3.6 and 4.0, v1 (1.44 KB, patch)
2013-02-16 12:08 PST, Frédéric Buclin
glob: review+
Details | Diff | Splinter Review
patch for trunk, v2 (3.50 KB, patch)
2013-02-18 06:33 PST, Frédéric Buclin
glob: review+
Details | Diff | Splinter Review
patch for 4.4, v2 (2.69 KB, patch)
2013-02-18 06:39 PST, Frédéric Buclin
glob: review+
Details | Diff | Splinter Review

Description SimranJeet Singh 2013-02-16 08:30:18 PST
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20130201065344

Steps to reproduce:

Hi,
I would like to report Cross site scripting vulnerability.

Steps to reproduce : 
Navigate
https://bugzilla.mozilla.org/show_bug.cgi?id=%22%3E%3Cimg%20src=%22aaa.jpg%22%20onerror=javascript:alert%28%27HappyBirthdayDone%27%29%3E&format=123

Actual results:
JavaScript get executed.


Expected results:
Site should not execute the script and should filter metacharacter in
order to prevent XSS attacks and Bounty  :P
Comment 1 Frédéric Buclin 2013-02-16 09:10:29 PST
I can reproduce upstream, in all versions.
Comment 2 Frédéric Buclin 2013-02-16 09:12:44 PST
I will write a better bug summary once I have identified what's going on exactly. Thanks for the bug report!
Comment 3 SimranJeet Singh 2013-02-16 09:21:08 PST
In all version? wow that's gud..Le me check too..
Comment 5 Frédéric Buclin 2013-02-16 09:40:41 PST
Bugzilla 3.6, 4.0 and 4.2 throws:

Insecure dependency in parameter 3 of DBI::db=HASH(0xa196540)->selectcol_arrayref method call while running with -T switch

This error should not happen but at least there is no XSS.

For 4.4 and trunk, XSS is triggered, but only if JavaScript is enabled. I'm on it!
Comment 6 Gervase Markham [:gerv] 2013-02-16 09:48:05 PST
Blimey... how on earth did we let this through? :-|

Not only is it a problem, it's a problem in about 5 places. It's in the title, various <link rel=""> places and the header text.

And why does it work on BMO if 3.6, 4.0 and 4.2 throw?

Gerv
Comment 7 Frédéric Buclin 2013-02-16 09:49:21 PST
(In reply to Gervase Markham [:gerv] from comment #6)
> And why does it work on BMO if 3.6, 4.0 and 4.2 throw?

Because bmo probably has some extensions which blindly trust the bug ID and do not filter it.
Comment 9 Frédéric Buclin 2013-02-16 10:07:20 PST
There are tons of places in templates where the bug ID is assumed to be an integer, i.e. something safe which you don't need to filter. Rather than fixing all these places individually, and taking the risk to miss some of them, or to rely on localizers to filter all these places themselves, I'm going to force show_bug.cgi to filter invalid bug IDs itself. This is much safer. And I honestly don't care if there are some places which already filter the bug ID themselves, resulting in double escaping.
Comment 10 Frédéric Buclin 2013-02-16 11:49:12 PST
Created attachment 714823 [details] [diff] [review]
patch for 4.4 and trunk, v1

Actually, the problem is that show_bug.cgi looks at the 'format' and 'ctype' parameters entered by the user *before* they are validated. Bugzilla::Template::get_format() detects that they are invalid and sanitized them, meaning that it will fall back to the default "show single bug" page, but show_bug.cgi already looked at them to decide how to display data. What this means is that show_bug.cgi thinks that the invalid bug will be passed to the "show multiple bugs" page, which correctly filters everything because it knows it can get untrusted data, but in fact Bugzilla::Template will use the classic "show single bug" page, which expects to get a valid bug object and so trust the bug ID.

So my fix is to make show_bug.cgi use sanitized data from get_format() so that it correctly calls Bugzilla::Bug->check() if the "show single bug" page is going to be called. This patch is for 4.4 and trunk.
Comment 11 Frédéric Buclin 2013-02-16 11:59:49 PST
Created attachment 714829 [details] [diff] [review]
patch for 4.2, v1

Same patch as for trunk (modulo bitrot fixed).
Comment 12 Frédéric Buclin 2013-02-16 12:08:13 PST
Created attachment 714830 [details] [diff] [review]
patch for 3.6 and 4.0, v1

This patch applies to both the 3.6 and 4.0 branches.
Comment 13 Frédéric Buclin 2013-02-16 12:19:34 PST
All Bugzilla versions are affected, since Bugzilla 1.0. The source code in Bugzilla 1.0 (Tcl) and 2.0 (Perl) is frightening! :)
Comment 14 Frédéric Buclin 2013-02-16 13:10:53 PST
(In reply to Gervase Markham [:gerv] from comment #6)
> And why does it work on BMO if 3.6, 4.0 and 4.2 throw?

I found the reason: if your installation has a custom multi-select field, you are lucky enough and get the DBI error I pasted in comment 5 before XSS is triggered. If you have no custom multi-select field, then XSS is triggered, even on the branches above. So in fact all versions are vulnerable to this XSS issue (I can reproduce with 2.22, all 3.x and 4.x versions, and 5.0; I know no older installations).
Comment 15 Frédéric Buclin 2013-02-16 18:27:26 PST
*** Bug 842096 has been marked as a duplicate of this bug. ***
Comment 16 Daniel Veditz [:dveditz] 2013-02-17 10:33:59 PST
Assigned CVE-2013-0785 to this vulnerability
Comment 17 SimranJeet Singh 2013-02-17 10:50:29 PST
Thanks ..Can this bug be eligible for bounty?
Comment 18 Frédéric Buclin 2013-02-17 10:52:01 PST
(In reply to SimranJeet Singh from comment #17)
> Thanks ..Can this bug be eligible for bounty?

Don't you notice the sec-bounty flag set to "?"? ;)
Comment 19 SimranJeet Singh 2013-02-17 11:01:02 PST
I knw as all bug i submitted got this flag with ?,out of few bugs later on gets a minus sign..thatswhy i'm asking ^_^
Comment 20 Frédéric Buclin 2013-02-17 11:04:05 PST
Well, once the flag is set to + or -, you will know the answer. :)
Comment 21 SimranJeet Singh 2013-02-17 11:14:52 PST
Np :)
But from today onwards i hate minus :|
@Frederic: I must say you are awsm..i luv ur work ,patch ,analyse and all..RESPECT!
Comment 22 Gervase Markham [:gerv] 2013-02-18 05:42:34 PST
I'm also amazed no-one found this before... Well done for making the patches! Although I'm not sure I understand how they actually work.

Gerv
Comment 23 Frédéric Buclin 2013-02-18 06:33:32 PST
Created attachment 715129 [details] [diff] [review]
patch for trunk, v2

Let's be a bit more proactive and throw an error if the format or ctype arguments contain unexpected characters. It doesn't make sense to sanitize them by removing unwanted characters. Either format and ctype have the expected syntax, or they are invalid. Period.

For 4.2 and older, I'm not sure we should enforce this rule in case this breaks some customizations.
Comment 24 Frédéric Buclin 2013-02-18 06:36:01 PST
Comment on attachment 715129 [details] [diff] [review]
patch for trunk, v2

Actually, I cannot use // in 4.4 as we must still support oldish Perl 5.8.x. So this patch is for trunk only.
Comment 25 Frédéric Buclin 2013-02-18 06:39:25 PST
Created attachment 715132 [details] [diff] [review]
patch for 4.4, v2
Comment 26 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-02-18 06:42:09 PST
Comment on attachment 714830 [details] [diff] [review]
patch for 3.6 and 4.0, v1

r=glob
Comment 27 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-02-18 06:47:07 PST
Comment on attachment 714829 [details] [diff] [review]
patch for 4.2, v1

r=glob
Comment 28 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-02-18 06:58:09 PST
Comment on attachment 715132 [details] [diff] [review]
patch for 4.4, v2

r=glob
Comment 29 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-02-18 07:00:53 PST
Comment on attachment 715129 [details] [diff] [review]
patch for trunk, v2

r=glob
Comment 30 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-02-18 07:12:20 PST
*** Bug 842298 has been marked as a duplicate of this bug. ***
Comment 31 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-02-18 08:18:37 PST
*** Bug 842298 has been marked as a duplicate of this bug. ***
Comment 32 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-02-18 08:21:19 PST
*** Bug 842298 has been marked as a duplicate of this bug. ***
Comment 33 Frédéric Buclin 2013-02-19 09:31:20 PST
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified show_bug.cgi
modified Bugzilla/Template.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 8587.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.4/
modified show_bug.cgi
modified Bugzilla/Template.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 8525.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified show_bug.cgi
modified Bugzilla/Template.pm
Committed revision 8191.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified show_bug.cgi
modified Bugzilla/Template.pm
Committed revision 7744.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/
modified show_bug.cgi
modified Bugzilla/Template.pm
Committed revision 7311.
Comment 34 Frédéric Buclin 2013-02-19 17:05:43 PST
Security advisory sent. Removing sec flag.
Comment 36 Denis Roy 2013-02-28 11:39:43 PST
> Security advisory sent. Removing sec flag.

I really don't understand why the sec flag is removed the moment the advisory is sent out to your announce list. This leaves us unsuspecting admins vulnerable and exposed.

I think it would be a greater service to your community if you announced the release and linked to the CVE, which does not necessarily point out how easily this is issue is exploitable, and remove the sec flag after a reasonable amount of time.
Comment 37 Frédéric Buclin 2013-02-28 12:20:54 PST
(In reply to Denis Roy from comment #36)
> I really don't understand why the sec flag is removed the moment the
> advisory is sent out to your announce list. This leaves us unsuspecting
> admins vulnerable and exposed.

Bugzilla admins are supposed to subscribe to the announce mailing-list, where announcements and security advisories are sent to. So "unsuspecting admins" doesn't stand here. Also, patches are attached to the security bug itself, so leaving the bug restricted to the security team only wouldn't let admins download patches if they don't want to or don't have the time to do a full upgrade but only want to patch the security issue. I also announce new releases on Google+ and on my blog, which is mirrored on planet.bugzilla.org, which are other ways to get info about Bugzilla. And finally, all new releases are pre-announced if you CC yourself to the meta bug 286269 which exists since 2005 to let admins know that new releases are coming soon.


> I think it would be a greater service to your community if you announced the
> release and linked to the CVE, which does not necessarily point out how
> easily this is issue is exploitable, and remove the sec flag after a
> reasonable amount of time.

We *always* announce the releases to the community, and we always did. As I said above, announcements are sent to the announce mailing-list, but also to the support mailing-list. Moreover, security advisories are also sent to BugTraq.

And for your reminder, we did CC you and other Bugzilla admins to very critical security bugs while they were still restricted, such as bug 515191 and bug 621591. For bug 515191, we waited for your confirmation that your Bugzilla installation was patched before making the security fix and the bug itself public. For bug 621591, you got access to a fix one month before anyone else, and the original security bug with all the details of the vulnerability became public only 6 months later (that was bug 619594). So I think we take care of security bugs and announcements pretty seriously.
Comment 38 Denis Roy 2013-02-28 12:42:55 PST
> Bugzilla admins are supposed to subscribe to the announce mailing-list,
> where announcements and security advisories are sent to. So "unsuspecting
> admins" doesn't stand here. 

Frederic, I think you misread my previous comment.  

I did not make this statement: "you do not announce releases" 

I made this statement: "Removing the security flag on security bugs at the same time you make the announcement that there is a flaw exposes the install base".

Since the security bugs often contain details on the exact procedure to exploit, my suggestion is to simply delay the removal of the security flag for a reasonable amount of time.  This would allow admins to react to the announcement that there is a flaw.

As for obtaining the patches, surely they can be made available for download by other means than this bug.

This comment is simply an attempt at providing feedback on the current process. FWIW, I am subscribed to the announce, and David's email is sitting in my inbox and tagged as "things to do ASAP".  Unfortunately, $DAY_JOB = ($Bugzilla + @OTHER_STUFF) / $SMALL_TEAM.
Comment 39 Gervase Markham [:gerv] 2013-03-01 01:39:42 PST
I think this is a reasonable request.

I believe the security policy for Firefox implements a delay between releasing a fixed build and opening the bugs in question. dveditz: can you fill us in on what it is?

From what Frederic said, the only reason I can see for opening the bug straight away is to give people access to the patches if they don't want to do a full upgrade. Can we find some other way of making patches available if we think people actually do use them?

Gerv

Note You need to log in before you can comment on or make changes to this bug.