1582857 - (CVE-2019-11762) DOM methods called with cross-origin this object don't handle document.domain consideration correctly

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), defect)

Description

[Kris Maglione [:kmag]]

We should be using the same security checks when we try to read a property on a cross-origin object, e.g., crossOriginWindow.open and when we try to call a same-origin native function with a cross-origin this object, e.g., window.open.call(crossOriginWindow). For the most part, they do behave the same, but when the objects are same-site with a document.domain difference, the latter succeeds, while the former throws a security exception.

Comment 1

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Security regression, sort of.

So the specific testcase here is, starting with A and B same-origin windows and then changing document.domain in B but not A:

  A.alert.call(B);

Per spec this should throw an exception, as follows: https://heycam.github.io/webidl/#dfn-create-operation-function step 2.1.2.2 calls into https://html.spec.whatwg.org/multipage#integration-with-idl:perform-a-security-check which for Window and Location objects checks IsPlatformObjectSameOrigin before allowing access. Similar for https://heycam.github.io/webidl/#dfn-attribute-getter step 1.1.2.2 and https://heycam.github.io/webidl/#dfn-attribute-setter step 4.5.2. We don't end up doing this check when A and B are same-compartment right now (we do do it if they are in different compartments).

This is a regression from bug 1514050, as far as I can tell.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Performance testcase

Patch coming up, but here are some numbers for this performance testcase, all in ns per property get:

• Firefox without patch, 5 runs:

  • Top line: 155 156 162 161 154
  • Bottom line: 17 17 18 18 18

• Firefox with patch, 5 runs:

  • Top line: 186 185 185 187 190
  • Bottom line: 19 18 17 18 18

For comparison, on the same hardware,

• Chrome, 5 runs:

  • Top line: 312 326 306 317 309
  • Bottom line: 220 221 230 238 226

• Safari, 5 runs:

  • Top line: 30 34 39 32 33
  • Bottom line: 31 31 44 32 31

The top line is accessing a DOM property on a window different from the one the script is running from; the botttom line is accessing it on the same window.

I am pretty sure that for the bottom line we end up on Ion fast-paths which bypass the extra code I am adding to deal with this bug, which is why there is no performance change there. For the top line, we do have a noticeable (20%) regression, but we're still a lot faster than Chrome and still a lot slower than Safari.

The profile for the post-patch case is at https://perfht.ml/2V8Y22l and shows 6.4% under IsCrossOriginAccessibleObject, 2.3% under IsPlatformObjectSameOrigin, and probably some of the 6% of time spent in GenericGetter is the inlined IsCrossCompartmentWrapper check. This is kinda sorta consistent with the observed ~20% slowdown: the new code I added should be no more than 16% of the total runtime of the testcase, just eyeballing the numbers. If someone can think of a sane faster way to do this, I'm all ears. But it seems like there are more wins to be had by moving more of this to the JIT somehow and avoiding the proxy overhead instead...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1582857. Fix security checks around this-unwrapping. r=peterv

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9094356 [details]
Bug 1582857. Fix security checks around this-unwrapping. r=peterv

Security Approval Request

• How easily could an exploit be constructed based on the patch?: That is a good question.

The actual security impact here is that if two pages start out same-origin, but then set document.domain differently so that they are no longer same-origin, this bug allows calling arbitrary DOM methods/getters/setters on the now-cross-origin window. You just have to grab the method/getter/setter from your own window, and .call() it, passing the other window as this.

How to actually exploit that, while not having the ability to just inject script into the other window before your document.domain values diverge, is an interesting question. And of course this only affects sites that use document.domain and depend on the no-longer-same-origin aspect of it somehow.

With all that out of the way, I think it's pretty clear based on the function naming that this has to do with security checks for "this" values of DOM methods, and that the objects involved are the ones that are normally accessible cross-origin. I suspect anyone competent would figure out that means Window and Location, and likely guess that this has to do with using those as this values for DOM calls, at which point they can pretty easily discover the problem. Whether they can then exploit it is an interesting question.

I could probably obfuscate the commit message some if I had to, but I am really not sure it buys us much given the way he code looks.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: 67 and newer
• If not all supported branches, which bug introduced the flaw?: Bug 1514050
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: I think this should not cause any regressions, but a bit of shaking out would be nice, especially because this patch does make us throw exceptions in cases where we currently don't (though other browsers do).

Comment 5

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9094356 [details]
Bug 1582857. Fix security checks around this-unwrapping. r=peterv

In discussion with Dan, we rated this a sec-moderate so this doesn't need sec-approval+ to land.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

bz, sounds like you can land this in trunk now and maybe we can aim for uplift for beta 10 if the results are good.

Comment 7

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/autoland/rev/79ac597adb58c9b4946b82a78eebe7b5aa71e19b

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/79ac597adb58

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9094356 [details]
Bug 1582857. Fix security checks around this-unwrapping. r=peterv

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Possible XSS vector, which ESR users may care about.
• User impact if declined: Possible XSS on some websites.
• Fix Landed on Version: 71
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This changes the behavior of a fairly rare case (involving document.domain and manually calling methods/getters/setters from one window on an object from another one) and changes it to match other browsers.
• String or UUID changes made by this patch:

Beta/Release Uplift Approval Request

• User impact if declined: Possible XSS on some websites.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This changes the behavior of a fairly rare case (involving document.domain and manually calling methods/getters/setters from one window on an object from another one) and changes it to match other browsers.
• String changes made/needed: None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9094356 [details]
Bug 1582857. Fix security checks around this-unwrapping. r=peterv

Fixes a sec bug, approved for 70.b11 and 68.2esr.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/110ad98bd8c7

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/750758879fc3

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt

Comment 14

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt