DOM methods called with cross-origin this object don't handle document.domain consideration correctly
Categories
(Core :: DOM: Bindings (WebIDL), defect)
Tracking
()
People
(Reporter: kmag, Assigned: bzbarsky)
References
(Regression)
Details
(Keywords: csectype-sop, regression, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main70+][adv-esr68.2+])
Attachments
(3 files, 1 obsolete file)
442 bytes,
text/html
|
Details | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
262 bytes,
text/plain
|
Details |
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.
Assignee | ||
Comment 1•5 years ago
•
|
||
[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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
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...
Assignee | ||
Comment 3•5 years ago
|
||
Assignee | ||
Comment 4•5 years ago
|
||
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).
Updated•5 years ago
|
Comment 5•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 6•5 years ago
|
||
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.
Assignee | ||
Comment 7•5 years ago
|
||
Comment 8•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
uplift |
Comment 12•5 years ago
|
||
uplift |
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 13•5 years ago
|
||
Comment 14•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•3 years ago
|
Description
•