246743 - "category" isn't escaped (XSS security hole)

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

[Jesse Ruderman]

http://update.mozilla.org/extensions/moreinfo.php?id=35&vid=36&category=%22%3Cscript%3Ealert(5)%3C/script%3E
demonstrates an XSS hole in update.mozilla.org.

Impact: 
* XPI whitelisting is useless until this hole is fixed.
* Attackers can spoof content on update.mozilla.org (including the
update.mozilla.org front page).

Comment 1

[Wolf [:wolf]]

I put a fix for this online.. 

Jesse, since you're alot better at security than me, does that fix appear to
have solved the issue?

Comment 2

[Jesse Ruderman]

It seems like you fixed it by stripping out <> tags.  I can still break the
page:
http://update.mozilla.org/extensions/moreinfo.php?id=35&vid=36&category=%22%3Efoo

Maybe I can't inject scripts anymore, but I'm not sure.  It would be easier to
convince me that you've fixed the hole if you fixed it by making the page
correct -- by escaping <>&" as HTML entities when they appear in hrefs.

Comment 3

[alanjstr]

Does the fix you've implemented apply to every field being read in?

A HREF="moreinfo.php?id=35&vid=36&category="alert(5)&page=releases"

That has three quotes in it.  My prefereence would be a sanitize() function that
runs through all variables.

Comment 4

[Wolf [:wolf]]

The fix is.
//Attempt to fix Bug 246743
foreach ($_GET as $key => $val) {
$_GET["$key"] = str_replace("\\","",strip_tags($_GET["$key"]));
}
Applied globally before any data is output.
It's not an ideal fix, more of a "prevent anything bad from being done" method
to limit the security implications of the bug quickly type thing, I'm realizing
now. :-/

I really should not print anything from $_get in the page w/o verifying it
matches something expected, you broke the page because of that. :-/

So, this bug remains open.. better fix forthcoming.

Comment 5

[alanjstr]

You need to do this for POST, too.

Comment 6

[Wolf [:wolf]]

*** Bug 247318 has been marked as a duplicate of this bug. ***

Comment 7

[Wolf [:wolf]]

Bug 247318 and this one both appear to have the same root cause to me.. lacking
data integrity checking. Bug 247318 deals with the unlaunched admin portions of
the site, which are known untested and incomplete. (which is why they're not up
yet. )

Comment 8

[alanjstr]

The reason I filed a separeate bug is that these are separate issues in terms of
coding.  And I'm filing bugs against the unreleased backend so that they get
fixed _before_ it is released.

Comment 9

[Wolf [:wolf]]

The fix for Bug 248242 also affects this bug, which should be fixed now also.
The entities in the URL should now be escaped, as well as html tags removed
entirely, the pages will still "look" broke in some cases, since the URLs were
not valid, but that's a seperate issue.

Marking as fixed, reopen if i'm wrong. :-)

Comment 10

[Jesse Ruderman]

Can this be made public now?

Comment 11

[Wolf [:wolf]]

Should be. I'd hope all of the Update 1.0-era security bugs (which afaik were all fixed.) are open at this point. 

Comment 12

[Yvan Boily [:ygjb][:yvan]]

Adding keywords to bugs for metrics, no action required.  Sorry about bugmail spam.