235265 - Url-parameter XSS vulnerability in edit*.cgi

Status: RESOLVED FIXED

Description

[Jouni Heikniemi]

This is a minor one, but still:

If you manage to get an Bugzilla administrator to click an url like
http://bugzilla.mozilla.org/editmilestones.cgi?product=foo&milestone=bar&action=baz&hack=%3Cscript%3Ealert('hax0red')%3C/script%3E

The JavaScript is run in Bugzilla's security context, potentially leaking
cookies and so on.

Comment 1

[Dave Miller [:justdave] ([e-mail])]

Yipee.  That's rather stupid. :)

editcomponents.cgi:860:    print "$_: $::FORM{$_}<BR>\n";
editgroups.cgi:646:    print "$_: $::FORM{$_}<BR>\n";
editmilestones.cgi:573:    print "$_: $::FORM{$_}<BR>\n";
editproducts.cgi:1516:    print "$_: $::FORM{$_}<BR>\n";
editusers.cgi:873:    print "$_: $::FORM{$_}<BR>\n";
editversions.cgi:548:    print "$_: $::FORM{$_}<BR>\n";

Comment 2

[Dave Miller [:justdave] ([e-mail])]

Suggested fix: since the "I have no clue what you want" is based on the action
param, only dump the action param, and not the entire param list.  (And escape
it, too).

Comment 3

[Jouni Heikniemi]

I might even go further and remove the parameter printing totally. Who would
ever benefit even from printing out the action param? Duh.

Comment 4

[Dave Miller [:justdave] ([e-mail])]

Yeah, I agree.  Get rid of all of them.

Comment 5

[Jouni Heikniemi]

I'll patch.

Comment 6

[Jouni Heikniemi]

Created attachment 142351 [details] [diff] [review]
v1

Duh.

Comment 7

[Jouni Heikniemi]

The patch fails for those admin files that have their loops already removed (see
bug 235222 patch v3). That's probably not relevant, since the bitrot is only
about removing some lines that have already been removed elsewhere; the patch
errors can just be skipped.

Comment 8

[Christian Reis]

Comment on attachment 142351 [details] [diff] [review]
v1

no-brainer

Comment 9

[Vlad Dascalu]

Since this is ready now, are we still pushing this for 2.18rc1 or we're doing it
for 2.17.7?

Or maybe 2.17.7 got renamed to rc1? I assume we'll have to do the security
advisory thing and stuff for this before checking it in.

Comment 10

[Dave Miller [:justdave] ([e-mail])]

holding approval for security advisory/release.  This will get checked in last
thing prior to the release getting tagged.

Comment 11

[Dave Miller [:justdave] ([e-mail])]

OK, check let's check this in NOW on the trunk (but hold off on 2.16 for now). 
We have too many other patches pending on the trunk that are going to conflict
with this, and it's getting in the way big time.  Just keep the commit log
message low profile for now.  "removing un-needed form value display code" or
something along those lines.

Comment 12

[Dave Miller [:justdave] ([e-mail])]

ok, checked in on the trunk.

Comment 13

[Joel Peshkin]

checked in on 2.16 branch

Comment 14

[Dave Miller [:justdave] ([e-mail])]

Clearing the security flag on disclosed bugs