User Agent: Mozilla/5.0 (Windows NT 6.0; rv:20.0) Gecko/20100101 Firefox/20.0 Build ID: 20130409194949 Steps to reproduce: Using POST request, if the target page doesn't have charset information, Firefox inherits character encodings across domain. This is bad because it increases the potential of XSS. Please check the following page and confirm this behavior: http://l0.cm/fx_x-domain_inheritance.html Expected results: Firefox should not inherit the charset across domain.

Oops, it doesn't work. I'm fixing test case.

OK, looks good. Please check this behavior from the following steps: 1. Go to https://bug871161.bugzilla.mozilla.org/attachment.cgi?id=748444 . This page's charset is hz-gb-2312. 2. Click the button. 3. The redirected page's document.characterSet is hz-gb-2312. This means that Firefox inherits charset across domain. FYI, this behavior occurs independent of "Auto-Detect" option(View -> Character Encoding -> Auto-Detect).

Henri: Is there any spec that covers this case? I know we stopped inheriting across frames so it's probably unwanted in any case. I'm going to go with a "moderate" severity rating but that's mostly a guess.

The spec is http://www.whatwg.org/specs/web-apps/current-work/#determining-the-character-encoding We are supposed to inherit across same-origin frames, except UTF-16 is not inherited. We are not supposed to inherit across origins. Need to make sure the guess from the previous page behaves likewise. The plan is to map HZ to the replacement encoding; see bug 603740. Generally, it's more dangerous to force a HZ page that contain attacker-submitted content to be treated as non-HZ than the other way round. Anyone serving anything but attack PoCs as HZ is behaving recklessly.

Can we find someone to assign this to? It has been a few months.

I suppose that that this is mine.

the bug is here: https://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#791

This is minused for bounty because of the security rating of moderate. Please renominate if you disagree with the rating.

Sigh. There's more. We have an analogous bug with window.open().

Great! So do we want to up the severity here and take care of it in this bug or is there going to be a different master bug?

I don't see a reason to increase the severity. After all, sites that do the very basics of clueful Web authoring and declare their own encoding are not vulnerable. It's just that the amount of code that needs changing and Gecko is much larger than I had anticipated. (Working on it.)

(Waiting for the try server before requesting for review.) * The removal of the reading of the intl.charset.default pref in the markup document viewer is okay, because we read the pref in nsHTMLDocument anyway. * I didn't try to hide what the patch is trying to do in the commit message are in the test case, because it's so obvious from the code anyway. * Since sites that are practicing the minimum good authoring practice and declare their encoding are not vulnerable and since the patch needs to touch core interfaces anyway, I wrote the patch with the assumption that it will ride the trains.

Weird failures still to sort out: On mochitest-bc: ImportError: cannot import name OrderedDict TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_URLBarSetURI.js | Test timed out ... On mochitest-oth: 4725 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_privatemode_perwindowpb.xul | Test timed out. ... 8958 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/states/test_link.html | Test timed out.

(In reply to Henri Sivonen (:hsivonen) from comment #14) > Weird failures still to sort out: Somehow, I had managed to miss http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#779

(In reply to Henri Sivonen (:hsivonen) from comment #15) > Somehow, I had managed to miss > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > js#779 Awesome. That line has been there since the beginning of Firefox browser.js...

Looks like the encoding in that case comes from http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#254 Why do we try to inherit the encoding when using the context menu to open the link in a new window or tab when we don't inherit from the previous document (AFAICT) when following links normally (only when POSTing)? And why are we inheriting on POST and on window.open()? Trident, Blink and WebKit don't! (Trident doesn't even inherit into same-origin iframes!) Looks like I should just remove these misfeatures instead of making them secure.

For those who want to see it to believe it: http://hsivonen.iki.fi/test/moz/charset-inheritance/

This patch does the following: * Removes the inheritance through window.open(), because it's not worth while securing a feature that other browser engines don't have. It's better to remove the feature. ("Fun" discovery: nsIMarkupDocumentViewer::defaultCharacterSet was documented as "XXX Comment here". Finally, we know that it meant the charset of the opener.) * Removes the inheritance through context menu's Open in New Window, because it's not worth while securing a feature that other browser engines don't have. It's better to remove the feature. Besides, it doesn't make sense to inherit when opening in a new window, when we don't inherit when opening in a new tab or when navigating normally in the same window. * Removes the inheritance through form POST, because it's not worth while securing a feature that other browser engines don't have. It's better to remove the feature. * Changes the inheritance from parent iframe to snapshot the origin of the inherited charset value at the same point where the charset value is snapshotted both to make sure that there can't be a difference between the time the charset value is snapshotted and the time when the parent's origin is compared. (Trident doesn't have this sort of inheritance, but Blink and WebKit do.) * Removes dead code related to handling <meta> from HTMLContentSink. * Cleans up nsIDocShell::charset not to use char* for XPConnected strings. * Removes misplaced Unicode editorializing from IDL. Still need to make comm-central not break because of this.

annevk, FYI. Not that I expect there to be an active danger of this stuff getting added to specs at this point, but still.

Comment on attachment 805331 [details] [diff] [review] Stop inheriting where other browsers don't inherit Oops. This has some lines of code I was supposed to remove.

Mozmill is green!

SeaMonkey follow-up is bug 917230.

Aargh. I managed to attach the old patch again last time. :-(

Comment on attachment 805946 [details] [diff] [review] Stop inheriting where other browsers don't inherit, really v2 >+++ b/content/html/document/src/nsHTMLDocument.cpp > nsHTMLDocument::TryParentCharset(nsIDocShell* aDocShell, >+ if (!NodePrincipal()->Equals(parentPrincipal) || This depends on parentPrincipal being non-null when kCharsetFromCache <= parentSource. This is not an obvious dependency. Could you just null-check parentPrincipal here? Esp. because nsPrincipal::Equals(nullptr) == true (wtf?). :( >+++ b/docshell/base/nsIDocShell.idl >+ [noscript, notxpcom] void setParentCharset( >+ [noscript, notxpcom] void getParentCharset( >+ out ACString parentCharset, >+ out int32_t parentCharsetSource, >+ out nsIPrincipal parentCharsetPrincipal); These should probably be nostdcall too. Or you need the impls to be NS_IMETHODIMP_(void). >+++ b/parser/nsCharsetSource.h >+#define kCharsetFromHintPrevDoc 6 Where is this still used? Can it go away? r=me with those fixed.

(In reply to Boris Zbarsky [:bz] from comment #27) > Comment on attachment 805946 [details] [diff] [review] > Stop inheriting where other browsers don't inherit, really v2 > > >+++ b/content/html/document/src/nsHTMLDocument.cpp > > nsHTMLDocument::TryParentCharset(nsIDocShell* aDocShell, > >+ if (!NodePrincipal()->Equals(parentPrincipal) || > > This depends on parentPrincipal being non-null when kCharsetFromCache <= > parentSource. This is not an obvious dependency. Could you just null-check > parentPrincipal here? Esp. because nsPrincipal::Equals(nullptr) == true > (wtf?). :( It looks to me nsPrincipal::Equals(nullptr) is false. (I checked before omitting a null check at the caller.) What am I missing: nsIPrincipal.idl: > %{C++ > inline bool Equals(nsIPrincipal* aOther) { > bool equal = false; > return NS_SUCCEEDED(Equals(aOther, &equal)) && equal; > } nsPrincipal.cpp: > NS_IMETHODIMP > nsPrincipal::Equals(nsIPrincipal *aOther, bool *aResult) > { > *aResult = false; > > if (!aOther) { > NS_WARNING("Need a principal to compare this to!"); > return NS_OK; > } > >+++ b/docshell/base/nsIDocShell.idl > >+ [noscript, notxpcom] void setParentCharset( > >+ [noscript, notxpcom] void getParentCharset( > >+ out ACString parentCharset, > >+ out int32_t parentCharsetSource, > >+ out nsIPrincipal parentCharsetPrincipal); > > These should probably be nostdcall too. Or you need the impls to be > NS_IMETHODIMP_(void). Thanks. Shows that I didn't try to build this on Windows... :-( > >+++ b/parser/nsCharsetSource.h > >+#define kCharsetFromHintPrevDoc 6 > > Where is this still used? Can it go away? This is still used for document.open(). Even if it's useless, it's not a security problem, so I'd rather not deal with it in this bug. I haven't investigated if it's useless now.

> It looks to me nsPrincipal::Equals(nullptr) is false. Ah, right. I misread it. Too much dealing with methods that have actual return values. :(

Attaching a Mozilla-central patch with the review comments addressed. Since the comm-central patch needs land first, waiting for review for that part before requesting approval for landing.

Comment on attachment 805943 [details] [diff] [review] comm-central part, v2 I'm not going to get to look at this in detail before sometime in the second half of next week. Neil, can you take a look to help keep this moving? Thanks.

Comment on attachment 805943 [details] [diff] [review] comm-central part, v2 >+ // See https://bugzilla.mozilla.org/show_bug.cgi?id=917230 -- hsivonen >+// content.defaultCharacterset = getMarkupDocumentViewer().defaultCharacterSet; Please change this to use the forced character set as discussed in that bug.

(In reply to neil@parkwaycc.co.uk from comment #32) > Comment on attachment 805943 [details] [diff] [review] > comm-central part, v2 > > >+ // See https://bugzilla.mozilla.org/show_bug.cgi?id=917230 -- hsivonen > >+// content.defaultCharacterset = getMarkupDocumentViewer().defaultCharacterSet; > Please change this to use the forced character set as discussed in that bug. Thanks. Changed accordingly.

Comment on attachment 807734 [details] [diff] [review] Stop inheriting where other browsers don't inherit, with nostdcall [Security approval request comment] How easily could an exploit be constructed based on the patch? It's totally obvious that this is about stopping charset inheritance (so obvious that I put it in the commit message so that it looks less like a security bug landing). It's not clear from the patch what encoding(s) you'd use to exploit this, but it's public knowledge that HZ and the ISO-2022 family are dangerous. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? If you are independently aware of HZ and ISO-2022 being dangerous, pretty much yes. If you don't have that piece of info, no. Which older supported branches are affected by this flaw? All. If not all supported branches, which bug introduced the flaw? Ancient flaw. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No backports. The main difficulty is that there's interaction with mailnews. Other than that, backports that wouldn't change interface IDs would look different from this patch. I think we should not backport, since this isn't a Gecko vulnerability but about mitigating a vulnerability sites might have. That is, sites that adhere to basic best practices (i.e. declare the encoding they use) are not vulnerable even without this mitigation. How likely is this patch to cause regressions; how much testing does it need? Unlikely to break the Web, since the behaviors removed are behaviors that Trident/Blink/WebKit don't have.

Comment on attachment 807734 [details] [diff] [review] Stop inheriting where other browsers don't inherit, with nostdcall sec-approval+ for trunk.

Can't land before bug 925489 clears and comm-central reopens. :-(

Given this is a security bug, and the scope of what it affects is well defined, you have a=Standard8 to land on the closed tree.

Thanks. Landed: https://hg.mozilla.org/comm-central/rev/4b9e2dcb78d3 https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba9f3b24b8a

Assuming this is all green, can we get Aurora and Beta branch patches created and nominated?

(In reply to Al Billings [:abillings] from comment #39) > Assuming this is all green, can we get Aurora and Beta branch patches > created and nominated?

(In reply to Ryan VanderMeulen [:RyanVM UTC-4][PTO 10/19 - 10/27] from comment #41) > (In reply to Al Billings [:abillings] from comment #39) > > Assuming this is all green, can we get Aurora and Beta branch patches > > created and nominated? I'll look into backporting the m-c patch. The comm-central patch applies as-is to beta and trivially rebases to aurora.

verified with Nightly build 20131024030204

Here's a backport to Aurora, but the added test case fails, because on Aurora, window.open() navigates the browsing context running mochitest instead of opening a window. Why might that be?

(In reply to Henri Sivonen (:hsivonen) from comment #44) > Created attachment 823296 [details] [diff] [review] > Backport to Aurora, test fails > > Here's a backport to Aurora, but the added test case fails, because on > Aurora, window.open() navigates the browsing context running mochitest > instead of opening a window. > > Why might that be? This seems to have been a local oddity. Previous trunk already moved to Aurora. Hence, there's only Beta left as a backport target. This patch intentionally leaves some dead code around. No point in being fancy with a single-train backport.

Comment on attachment 823998 [details] [diff] [review] Backport to beta r=me, I think. Not quite sure I wouldn't have missed something if it were here. :(

Is this worth a backport to b2g18 given that it's sec-moderate?

Comment on attachment 823998 [details] [diff] [review] Backport to beta [Approval Request Comment] Bug caused by (feature/regressing bug #): Ancient. User impact if declined: If * the attacker can add user-supplied content to a victim site AND * the victim site doesn't adhere to basic best practices and fails to declare its character encoding AND * the attacker can get a user to visit a site that the attacker controls THEN the attacker might in some circumstances be able to execute JS in the context of the victim site in the user's browser. Testing completed (on m-c, etc.): Testing has completed on m-c, but what was tested is not the exact same patch as this backport. Risk to taking this patch (and alternatives if risky): There might have been a mistake in the backporting that wasn't exposed by the try server or review. String or IDL/UUID changes made by this patch: None. (In reply to Boris Zbarsky [:bz] from comment #46) > r=me, I think. Thanks. (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #47) > Is this worth a backport to b2g18 given that it's sec-moderate? Well, I wouldn't have backported even to beta on my own initiative. See comment 34.

Comment on attachment 816569 [details] [diff] [review] comm-central part, v3 [Approval Request Comment] If the mozilla-beta patch is approved, this patch needs to land on comm-beta first.

(In reply to Henri Sivonen (:hsivonen) from comment #48) > Well, I wouldn't have backported even to beta on my own initiative. See > comment 34. OK, thanks :)

Why Beta but not Aurora?

Doh, misread flags. Nevermind.

https://hg.mozilla.org/releases/mozilla-beta/rev/cf4f55f7ed5e https://hg.mozilla.org/releases/comm-beta/rev/4585547772a0 Thanks everyone. Especially thanks to the reporter for noticing this.

Verified in FF26, 2013-11-20.

Bah, by a combination of this bug and bug 382074, view source in Thunderbird started ignoring the selected character set.