Closed Bug 842038 (CVE-2013-0785) Opened 11 years ago Closed 5 months ago

[SECURITY] XSS in show_bug.cgi when using an invalid page format

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

2.10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Bugzilla 3.6

People

(Reporter: simranjeet61, Assigned: LpSolit)

References

()

Details

(Keywords: sec-high, wsec-xss)

Attachments

(4 files, 1 obsolete file)

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
I can reproduce upstream, in all versions.
Assignee: nobody → create-and-change
Severity: normal → critical
Status: UNCONFIRMED → NEW
Component: General → Creating/Changing Bugs
Ever confirmed: true
Product: bugzilla.mozilla.org → Bugzilla
QA Contact: default-qa
Target Milestone: --- → Bugzilla 3.6
Version: Development/Staging → unspecified
I will write a better bug summary once I have identified what's going on exactly. Thanks for the bug report!
Flags: blocking4.4+
Flags: blocking4.2.5+
Flags: blocking4.0.10+
Flags: blocking3.6.13+
Summary: Bugzilla.mozilla.org Cross site scripting[XSS] → [SECURITY] XSS is possible in show_bug.cgi
In all version? wow that's gud..Le me check too..
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!
Assignee: create-and-change → LpSolit
Status: NEW → ASSIGNED
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
(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.
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.
Flags: sec-bounty?
Keywords: sec-high, wsec-xss
Attached patch patch for 4.4 and trunk, v1 (obsolete) — Splinter Review
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.
Attachment #714823 - Flags: review?(dkl)
Same patch as for trunk (modulo bitrot fixed).
Attachment #714829 - Flags: review?(dkl)
This patch applies to both the 3.6 and 4.0 branches.
Attachment #714830 - Flags: review?(dkl)
All Bugzilla versions are affected, since Bugzilla 1.0. The source code in Bugzilla 1.0 (Tcl) and 2.0 (Perl) is frightening! :)
Summary: [SECURITY] XSS is possible in show_bug.cgi → [SECURITY] Possible XSS in show_bug.cgi when using an invalid page format
Version: unspecified → 2.10
(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).
Blocks: 832267
Assigned CVE-2013-0785 to this vulnerability
Alias: CVE-2013-0785
Summary: [SECURITY] Possible XSS in show_bug.cgi when using an invalid page format → [SECURITY] XSS in show_bug.cgi when using an invalid page format
Thanks ..Can this bug be eligible for bounty?
(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 "?"? ;)
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 ^_^
Well, once the flag is set to + or -, you will know the answer. :)
Np :)
But from today onwards i hate minus :|
@Frederic: I must say you are awsm..i luv ur work ,patch ,analyse and all..RESPECT!
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
Attachment #714823 - Flags: review?(glob)
Attachment #714829 - Flags: review?(glob)
Attachment #714830 - Flags: review?(glob)
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.
Attachment #714823 - Attachment is obsolete: true
Attachment #714823 - Flags: review?(glob)
Attachment #714823 - Flags: review?(dkl)
Attachment #715129 - Flags: review?(glob)
Attachment #715129 - Flags: review?(dkl)
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.
Attachment #715129 - Attachment description: patch for 4.4 and trunk, v2 → patch for trunk, v2
Attachment #715132 - Flags: review?(glob)
Attachment #715132 - Flags: review?(dkl)
Comment on attachment 714830 [details] [diff] [review]
patch for 3.6 and 4.0, v1

r=glob
Attachment #714830 - Flags: review?(glob)
Attachment #714830 - Flags: review?(dkl)
Attachment #714830 - Flags: review+
Comment on attachment 714829 [details] [diff] [review]
patch for 4.2, v1

r=glob
Attachment #714829 - Flags: review?(glob)
Attachment #714829 - Flags: review?(dkl)
Attachment #714829 - Flags: review+
Comment on attachment 715132 [details] [diff] [review]
patch for 4.4, v2

r=glob
Attachment #715132 - Flags: review?(glob)
Attachment #715132 - Flags: review?(dkl)
Attachment #715132 - Flags: review+
Comment on attachment 715129 [details] [diff] [review]
patch for trunk, v2

r=glob
Attachment #715129 - Flags: review?(glob)
Attachment #715129 - Flags: review?(dkl)
Attachment #715129 - Flags: review+
Flags: approval?
Flags: approval4.4?
Flags: approval4.2?
Flags: approval4.0?
Flags: approval3.6?
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval4.2?
Flags: approval4.2+
Flags: approval4.0?
Flags: approval4.0+
Flags: approval3.6?
Flags: approval3.6+
Flags: approval+
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: sec-bounty? → sec-bounty+
Security advisory sent. Removing sec flag.
Group: bugzilla-security
> 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.
(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.
> 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.
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
Resolution: FIXED → INCOMPLETE
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: INCOMPLETE → ---
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago5 months ago
Resolution: --- → WORKSFORME
Resolution: WORKSFORME → FIXED
You need to log in before you can comment on or make changes to this bug.