924802 - (CVE-2013-1742) [SECURITY] (XSS) "id" and "sortkey" are not sanitized when editing flag types if categoryAction-foo is set

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Mateusz Goik]

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

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]

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

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

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]

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

Attachment: patch for trunk, v1

Comment 7

[Frédéric Buclin]

Attachment: patch for 4.2 and 4.4, v1

Comment 8

[Frédéric Buclin]

Attachment: patch for 4.0

Things are worse in 4.0. "action" and "target_type" weren't HTML-escaped either.

Comment 9

[David Lawrence [:dkl]]

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

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

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

Assigning CVE-2013-1742 to this bug

Comment 13

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/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://[e-mail]/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://[e-mail]/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://[e-mail]/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]

Security advisory sent.