Last Comment Bug 924802 - (CVE-2013-1742) [SECURITY] (XSS) "id" and "sortkey" are not sanitized when editing flag types if categoryAction-foo is set
(CVE-2013-1742)
: [SECURITY] (XSS) "id" and "sortkey" are not sanitized when editing flag types...
Status: RESOLVED FIXED
: sec-critical, wsec-xss
Product: Bugzilla
Classification: Server Software
Component: Administration (show other bugs)
: 2.17.1
: All All
: -- normal (vote)
: Bugzilla 4.0
Assigned To: Frédéric Buclin
: default-qa
Mentors:
editflagtypes.cgi?id="><script>alert(...
Depends on:
Blocks: 912643
  Show dependency treegraph
 
Reported: 2013-10-09 02:00 PDT by Mateusz Goik
Modified: 2014-07-24 16:55 PDT (History)
5 users (show)
glob: approval+
glob: approval4.4+
LpSolit: blocking4.4.1+
glob: approval4.2+
LpSolit: blocking4.2.7+
glob: approval4.0+
LpSolit: blocking4.0.11+
dveditz: sec‑bounty+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch for trunk, v1 (2.15 KB, patch)
2013-10-09 06:29 PDT, Frédéric Buclin
dkl: review+
Details | Diff | Splinter Review
patch for 4.2 and 4.4, v1 (1.73 KB, patch)
2013-10-09 06:36 PDT, Frédéric Buclin
dkl: review+
Details | Diff | Splinter Review
patch for 4.0 (1.74 KB, patch)
2013-10-09 06:45 PDT, Frédéric Buclin
dkl: review+
Details | Diff | Splinter Review

Description Mateusz Goik 2013-10-09 02:00:34 PDT
PoC:

http://localhost/bugzilla/editflagtypes.cgi?action=insert&can_fully_edit=1&id="><script>alert(1)</script>&token=&target_type=bug&check_clusions=1&name=test1&description=test2&product=TestProduct&component=TestComponent&categoryAction-include=Include&sortkey=1&is_active=on&is_requestable=on&cc_list=&is_requesteeble=on&is_multiplicable=on&grant_group=&request_group=

http://localhost/bugzilla/editflagtypes.cgi?action=insert&can_fully_edit=1&id=&token=&target_type=bug&check_clusions=1&name=test&description=test2&product=TestProduct&component=TestComponent&categoryAction-include=Include&sortkey=1"><script>alert(2)</script>&is_active=on&is_requestable=on&cc_list=&is_requesteeble=on&is_multiplicable=on&grant_group=&request_group=
Comment 1 Gervase Markham [:gerv] 2013-10-09 02:45:44 PDT
The problem is here, starting on line 108 of editflagtypes.cgi:

    my $type = {};
    $type->{$_} = $cgi->param($_) foreach $cgi->param();
...
    $vars->{'type'} = $type;

We don't require the filtering of ".id" in templates in our XSS-checking test because we assume it will always be a property of an object which Bugzilla has validated the creation of. Here, we instead make up a fake object, trusting all the parameters on the URL. This is bad.

This code was added by LpSolit in bug 523205, and checked into the trunk (only) in October 2010. It is not present in 4.0, but is present in 4.2 and 4.4.

Gerv
Comment 2 Frédéric Buclin 2013-10-09 05:53:49 PDT
(In reply to Gervase Markham [:gerv] from comment #1)
> This code was added by LpSolit in bug 523205

Not true. I didn't write this code. I only moved the code out of a subroutine when refactoring editflagtypes.cgi, but the code was already there since flags have been implemented in Bugzilla 2.18, see bug 98801. I can reproduce with 4.0 and 3.6 (didn't try with older releases, but the code didn't change).

I pasted in the URL field a reduced testcase.
Comment 3 Gervase Markham [:gerv] 2013-10-09 06:13:42 PDT
(In reply to Frédéric Buclin from comment #2)
> (In reply to Gervase Markham [:gerv] from comment #1)
> > This code was added by LpSolit in bug 523205
> 
> Not true. 

Ah, my apologies.

Anyway, assuming the admin is logged in (if he's not, the PoC fails), if he can be persuaded to load this URL then his cookies can be stolen. :-|

Should we eliminate all exceptions to the filtering requirements? Or would that be overkill?

Gerv
Comment 4 Frédéric Buclin 2013-10-09 06:14:27 PDT
Neither "id" nor "sortkey" are sanitized or HTML-escaped when categoryAction-foo is set (it doesn't need to be a legal name as long as it starts with "categoryAction-"). So this URL works too (I replaced id by sortkey):

editflagtypes.cgi?sortkey="><script>alert(1)<%2Fscript>&categoryAction-foo=1
Comment 5 Frédéric Buclin 2013-10-09 06:16:36 PDT
(In reply to Gervase Markham [:gerv] from comment #3)
> can be persuaded to load this URL then his cookies can be stolen. :-|

Do not forget that credentials cannot be stolen due to the HttpOnly attribute set on cookies.
Comment 6 Frédéric Buclin 2013-10-09 06:29:54 PDT
Created attachment 814861 [details] [diff] [review]
patch for trunk, v1
Comment 7 Frédéric Buclin 2013-10-09 06:36:55 PDT
Created attachment 814867 [details] [diff] [review]
patch for 4.2 and 4.4, v1
Comment 8 Frédéric Buclin 2013-10-09 06:45:13 PDT
Created attachment 814872 [details] [diff] [review]
patch for 4.0

Things are worse in 4.0. "action" and "target_type" weren't HTML-escaped either.
Comment 9 David Lawrence [:dkl] 2013-10-11 09:02:14 PDT
Comment on attachment 814861 [details] [diff] [review]
patch for trunk, v1

Review of attachment 814861 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl
Comment 10 David Lawrence [:dkl] 2013-10-11 09:14:50 PDT
Comment on attachment 814867 [details] [diff] [review]
patch for 4.2 and 4.4, v1

Review of attachment 814867 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl
Comment 11 David Lawrence [:dkl] 2013-10-11 09:18:29 PDT
Comment on attachment 814872 [details] [diff] [review]
patch for 4.0

Review of attachment 814872 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl
Comment 12 Daniel Veditz [:dveditz] 2013-10-11 21:26:44 PDT
Assigning CVE-2013-1742 to this bug
Comment 13 Frédéric Buclin 2013-10-16 10:21:13 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified template/en/default/filterexceptions.pl
modified template/en/default/admin/flag-type/edit.html.tmpl
Committed revision 8779.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.4/
modified template/en/default/filterexceptions.pl
modified template/en/default/admin/flag-type/edit.html.tmpl
Committed revision 8625.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified template/en/default/filterexceptions.pl
modified template/en/default/admin/flag-type/edit.html.tmpl
Committed revision 8233.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified template/en/default/filterexceptions.pl
modified template/en/default/admin/flag-type/edit.html.tmpl
Committed revision 7761.
Comment 14 Frédéric Buclin 2013-10-17 07:58:59 PDT
Security advisory sent.

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