Closed Bug 1575711 Opened 4 months ago Closed 4 months ago

CallerSubsumes check for |object| and |any| WebIDL parameters does not reject remote proxy objects

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

Details

(Keywords: sec-audit, Whiteboard: [adv-main70-])

Attachments

(1 file)

In bug 1575272, I have a patch that makes dom/bindings/test/test_bug1036214.htm pass with Fission enabled by changing the test to stay cross-origin but same-process when Fission is enabled. At kmag's prompting, I started looking at what happens in the cross-process case.

To do this, you need to remove the line xoObjects.push(SpecialPowers.unwrap(SpecialPowers.wrap(window[0]).document));, because that just will throw an error.

Anyways, once you do that, the test still fails because you can pass a remote window proxy object as an |any| param (and presumably as an object, too). I'm guessing that CallerSubsumes() ends up returning true.

The guts of that method is:
nsIPrincipal* objPrin = nsContentUtils::ObjectPrincipal(js::UncheckedUnwrap(aObject));
return nsContentUtils::SubjectPrincipal()->Subsumes(objPrin);

It looks like the principal for a remote window proxy ends up being the principal of the origin page, which makes sense because there's no CCW we can unwrap to end up in another compartment.

Maybe this is okay, because the remote window proxy is such a limited object that passing it into a JS-implemented API is okay, but given the tricky nature of bug 1036214 I thought it would be worth filing a bug for. I'll just hide it for now.

Peter, do you have any ideas? Are things okay as-is, or should we maybe explicitly detect and throw for these remote window proxy objects?

Flags: needinfo?(peterv)

If nothing else, I'd really like web content not to have a way to detect the difference between a cross-process window proxy and a same-process-cross-origin window proxy, so I'd really like us to treat the two the same in cases like this if only to prevent these APIs from being used for that purpose.

That's a good point. This test isn't doing anything that requires chrome privileges.

Assignee: nobody → continuation
Flags: needinfo?(peterv)

This check is used to reject cross-origin objects from being passed in
as |any| or |object| parameters to WebIDL methods. Remote object
proxies are technically same-origin, but we want to make them behave
the same as when Fission is not enabled.

Keywords: checkin-needed

Baced out for failing test_bug1036214.html:

https://hg.mozilla.org/integration/autoland/rev/03a423fae99ef6fb5f43ede34e3611813eac3c45

Push which ran failing test: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=263528796&resultStatus=testfailed%2Cbusted%2Cexception&revision=f277c58a8bcb52c8d18dbdeb63de50dd8a8973d1
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=263528796&repo=autoland

[task 2019-08-26T17:29:29.464Z] 17:29:29 INFO - TEST-PASS | dom/bindings/test/test_bug1036214.html | Threw security exception: TypeError: Permission denied to pass cross-origin object as element of argument 1 of TestInterfaceJS.anySequenceLength.
[task 2019-08-26T17:29:29.464Z] 17:29:29 ERROR - [SimpleTest.finish()] this test already called finish!
[task 2019-08-26T17:29:29.464Z] 17:29:29 INFO - Buffered messages finished
[task 2019-08-26T17:29:29.464Z] 17:29:29 INFO - TEST-UNEXPECTED-ERROR | dom/bindings/test/test_bug1036214.html | called finish() multiple times

Flags: needinfo?(continuation)

I actually saw that error locally, but I assumed it was some fluke thing, because I have no idea why finish() could be called twice. I guess I'll take a look again.

Flags: needinfo?(continuation)

Oh, right, when I copied and pasted the iframe, I accidentally included the onload twice, so setup() was running twice.

Group: dom-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.