Closed Bug 871161 (CVE-2013-5612) Opened 11 years ago Closed 11 years ago

Potential XSS with cross-domain (cross-origin) inheritance of charset

Categories

(Core :: DOM: Core & HTML, defect)

20 Branch
x86
Windows Vista
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox25 --- wontfix
firefox26 + verified
firefox27 + verified
firefox-esr17 --- wontfix
firefox-esr24 --- wontfix
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- fixed

People

(Reporter: masatokinugawa, Assigned: hsivonen)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main26+])

Attachments

(4 files, 8 obsolete files)

138 bytes, text/html
Details
47.71 KB, patch
hsivonen
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
4.23 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
23.64 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Attached file fx_x-domain_inheritance_testcase.html (obsolete) —
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.
Attachment #748437 - Attachment mime type: text/plain → text/html
Oops, it doesn't work. I'm fixing test case.
Attachment #748437 - Attachment is obsolete: true
Attached file testcase
Attachment #748444 - Attachment mime type: text/plain → text/html
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
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.
Keywords: sec-moderate
Flags: needinfo?(hsivonen)
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.
Flags: needinfo?(hsivonen)
Can we find someone to assign this to? It has been a few months.
I suppose that that this is mine.
Assignee: nobody → hsivonen
Summary: Potential XSS with cross-domain inheritance of charset → Potential XSS with cross-domain (cross-origin) inheritance of charset
This is minused for bounty because of the security rating of moderate. Please renominate if you disagree with the rating.
Flags: sec-bounty? → sec-bounty-
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.
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.
Attachment #797792 - Attachment is obsolete: true
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.
Attachment #805331 - Attachment is obsolete: true
Attachment #805331 - Flags: review?(bzbarsky)
Attached patch comm-central part, v2 (obsolete) — Splinter Review
Mozmill is green!
Attachment #805906 - Attachment is obsolete: true
Attachment #805943 - Flags: review?(mbanner)
SeaMonkey follow-up is bug 917230.
See Also: → 917230
Aargh. I managed to attach the old patch again last time. :-(
Attachment #805909 - Attachment is obsolete: true
Attachment #805909 - Flags: review?(bzbarsky)
Attachment #805946 - Flags: review?(bzbarsky)
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.
Attachment #805946 - Flags: review?(bzbarsky) → review+
(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.
Attachment #805946 - Attachment is obsolete: true
Attachment #807734 - Flags: review+
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.
Attachment #805943 - Flags: review?(mbanner) → review?(neil)
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.
Attachment #805943 - Flags: review?(neil) → review+
(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.
Attachment #805943 - Attachment is obsolete: true
Attachment #816569 - Flags: review+
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.
Attachment #807734 - Flags: sec-approval?
Comment on attachment 807734 [details] [diff] [review]
Stop inheriting where other browsers don't inherit, with nostdcall

sec-approval+ for trunk.
Attachment #807734 - Flags: sec-approval? → sec-approval+
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.
Assuming this is all green, can we get Aurora and Beta branch patches created and nominated?
https://hg.mozilla.org/mozilla-central/rev/2ba9f3b24b8a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(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?
Flags: needinfo?(hsivonen)
(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.
Flags: needinfo?(hsivonen)
Keywords: verifyme
verified with Nightly build 20131024030204
Status: RESOLVED → VERIFIED
Attached patch Backport to Aurora, test fails (obsolete) — Splinter Review
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?
Attached patch Backport to betaSplinter Review
(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.
Attachment #823296 - Attachment is obsolete: true
Attachment #823998 - Flags: review?(bzbarsky)
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.  :(
Attachment #823998 - Flags: review?(bzbarsky) → review+
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.
Attachment #823998 - Flags: approval-mozilla-beta?
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.
Attachment #816569 - Flags: approval-mozilla-beta?
(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.
Attachment #816569 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #823998 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified in FF26, 2013-11-20.
Whiteboard: [adv-main26+]
Alias: CVE-2013-5612
Keywords: verifyme
Group: core-security
Depends on: 1046335
Depends on: 1099558
Bah, by a combination of this bug and bug 382074, view source in Thunderbird started ignoring the selected character set.
Flags: sec-bounty-hof+
You need to log in before you can comment on or make changes to this bug.