1503984 - Add a pref to to return null on chrome->content subframe access

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[:Felipe Gomes (needinfo for replies!)]

(from a conversation with Nika)

It would be nice to have a pref to make privileged code access to cross-origin frames fail somehow, either throwing or returning null. By toggling this pref we can see what will fail/break in our code that needs to be fixed for Fission.

Note 1: ideally this wouldn't break parent code accessing non-e10s tabs

Note 2: I've been tying some other fission-simulation behavior to the browser.fission.simulate pref, but we can use another one for more granularity, if desired

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Another point that was mentioned by Nika in the meeting: even though this will start as a pref, the end goal is to make this the default enforced behavior, in order to prevent people from writing different codepaths for same-origin and cross-origin frames.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

I've offered to mentor Felipe through these changes after I'm back. It would be awesome to have this for the all-hands.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1503984 - Add a pref to block chrome code from accessing content subframes. r=nika

The pref dom.chrome_frame_access.enabled will default to true. When false, it will block various methods that chrome code can use to traverse subframes. The initial list is:
  iframe.contentWindow
  iframe.contentDocument
  window.top
  window.parent
  window.frames[0]
  MessageEvent.source

More blocks are likely to be added in the future.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

A couple of notes about the patch:

- I'm not sure if this is the best approach for the indexed getter. I tried to stick to what we discussed and how the other cases behaved: It doesn't change window.frames.length, but window.frames[i] will return null for the frames that are not also privileged.

A different approach would be to just make window.frames[i] * always * return null (independent of the object that would be returned), and make window.frames.length return 0.

This sounds nicer and will likely break less code (one breakage can be seen in the devtools console part of this patch that I had to fix in order to continue using devtools). The downside is that it wouldn't follow the "still allow chrome to chrome access" part.


- as far as I can tell, window.frames["named getter"] already doesn't work for chrome code trying to get a named iframe from content. I had added code to WindowNamedPropertiesHandler.cpp for that but it turned out to be unecessary.


- I named the pref to something that defaults to true, because otherwise the double negative was getting to confusing  (originally we had talked about dom.strict_chrome_frame_access.enabled = false)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82ca028b0524
Add a pref to block chrome code from accessing content subframes. r=nika

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 82ca028b0524 (Bug 1503984) for StaticPrefList failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=82ca028b05248d99b0e4638cae0c0b7d99ced797&selectedJob=217752095

Backout link: https://hg.mozilla.org/integration/autoland/rev/1ec6e2d441b1dcc104d6a8807462511f1b8eb122

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217752095&repo=autoland&lineNumber=1138

[task 2018-12-18T20:04:07.044Z] 20:04:07     INFO - web-platform-tests                                           CT,, 
[task 2018-12-18T20:04:07.045Z] 20:04:07     INFO - 
[task 2018-12-18T20:04:07.046Z] 20:04:07     INFO - Application command: /builds/worker/workspace/build/application/firefox/firefox --marionette about:blank -profile /tmp/tmpsvVdPz.mozrunner
[task 2018-12-18T20:04:07.050Z] 20:04:07     INFO - Starting runner
[task 2018-12-18T20:04:07.077Z] 20:04:07     INFO - PID 912 | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpsvVdPz.mozrunner/runtests_leaks_857.log
[task 2018-12-18T20:04:07.598Z] 20:04:07     INFO - PID 912 | Assertion failure: IsAtomic<bool>::value || NS_IsMainThread() (Non-atomic static pref 'dom.chrome_frame_access.enabled' being accessed on background thread), at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/StaticPrefList.h:295
[task 2018-12-18T20:04:58.017Z] 20:04:58     INFO - PID 912 | #01: nsGlobalWindowInner::AllowChromeFrameAccess(JSContext*, JSObject*) [dom/base/nsGlobalWindowInner.cpp:2889]
[task 2018-12-18T20:04:58.017Z] 20:04:58     INFO - 
[task 2018-12-18T20:04:58.018Z] 20:04:58     INFO - PID 912 | #02: mozilla::dom::PrefableDisablers::isEnabled(JSContext*, JS::Handle<JSObject*>) const [dom/bindings/DOMJSClass.h:132]
[task 2018-12-18T20:04:58.018Z] 20:04:58     INFO - 
[task 2018-12-18T20:04:58.019Z] 20:04:58     INFO - PID 912 | #03: bool mozilla::dom::DefinePrefable<JSPropertySpec const>(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Prefable<JSPropertySpec const> const*) [dom/bindings/BindingUtils.cpp:653]

Comment 7

[:Felipe Gomes (needinfo for replies!)]

The failure was about accessing this static pref off the main thread. Is the right thing to do to just change it to a RelaxedAtomicBool?

(The comment in PrefableDisablers [1] say this is only related to things with a "Pref" annotation, but I assume it's also true for things with a "Func" annotation, right?). I'm asking because I didn't make changes to any property with a "Pref" annotation


[1] https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/dom/bindings/DOMJSClass.h#118

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> The failure was about accessing this static pref off the main thread. Is the
> right thing to do to just change it to a RelaxedAtomicBool?
> 
> (The comment in PrefableDisablers [1] say this is only related to things
> with a "Pref" annotation, but I assume it's also true for things with a
> "Func" annotation, right?). I'm asking because I didn't make changes to any
> property with a "Pref" annotation
> 
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 13788edbabb04d004e4a1ceff41d4de68a8320a2/dom/bindings/DOMJSClass.h#118

I was initially surprised that this method was called off-main-thread at all, but I suppose you're using it as a function to disable the `source` property on `MessageEvent`. We probably don't want to disable `MessageEvent::source` if the source is not a window object, but rather a port.

I think the easiest solution here is to not disable MessageEvent's properties in this first version, and revisit what we want to do there later.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Yeah, that surprised me too. I removed the MessageEvent parts from the code and tests, reverted back to a normal `bool` StaticPref and sent it to tryserver to make sure nothing else is unexpectedly touching it outside of the main thread. Let's see how it goes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e869b2aa7a2ce24ec9c050d7cb1a8b078025d45

Comment 10

[:Felipe Gomes (needinfo for replies!)]

It was indeed the MessageEvent.source that caused that

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Nika, I just rebased the patch, but a new problem showed up due to the recent landing of bug 1353867: my patch adds the Func=".." disabler to Window.top, but since it's also marked as CrossOriginReadable, the codegen throws this error: https://searchfox.org/mozilla-central/rev/b4ebbe90ae4d0468fe6232bb4ce90699738c8125/dom/bindings/Codegen.py#4163

How should I proceed?

Comment 12

[Neha Kochar [:neha]]

Adding Peter here, to see if he can help too. He worked on the windowProxy.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

I don't know too much about that logic so shifting to bz who might know more?

Comment 14

[Boris Zbarsky [:bzbarsky]]

I think we shouldn't be doing this with IDL disablers at all, personally. Specifically, I would advocate for the following behavior, some of which the patch already implements:

1. Return a modified value from .length as needed. What that value should be is an interesting question, but it should match the behavior of the indexed getter. Returning 0 for all system access like this patch does is plausible, as long as we're clear that it will prevent walking system kids of system windows.

2. Return undefined from the indexed getter as needed. The behavior in the patch right now is inconsistent, by the way: it will return things from the indexed getter in the "system window with system child" case, but claim length == 0...

3. Return "this" from .top as needed. We already do this in various cases in the C++; add to those cases. Note that the current code in the patch probably does the wrong thing when .top is evaluated on a system window that has a non-system top: it will return that non-system top.

4. Return null from .opener as needed. Again, when a system window has a non-system opener I would argue the current code does the wrong thing.... except we already check for that case in GetOpener and return null. The new logic should just live in the same place.

5. Return "this" from .parent as needed. Again, this already happens in various cases.

6. Return null from .frameElement as needed. This patch doesn't do that.

7. Return undefined from the named getter on WindowNamedPropertiesHandler as needed. This matters only for the "system window with named kids" case, of course. This patch is blocking iteration of said kids even if they are also system, right?

8. Return null from .contentDocument as needed. The patch currently makes .contentDocument disappear on iframe elements that are in web content, but leaves it on system iframe elements framing non-system documents (so you can get at things this way that are blocked via indexed or named access on the window; I suspect this is wrong).

9. Return null from .contentWindow as needed.

You'll want to decide what to do with cross-process window proxies. One thing that might be is to decide that system code should never end up with them so you don't need to worry about them.

With bug 1363208 some of the code this patch is modifying is going away and/or moving to a different spot. I can probably merge on top if this lands first, but I'll need to be very clear on what behavior we're trying to achieve to be able to do that, especially for the indexed getter case.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Felipe, could you have a look please?

Comment 16

[:Felipe Gomes (needinfo for replies!)]

I've talked to some people about this bug, and the conclusion was that the patch that we had here, even though not ready to land, was useful for its purpose, which was to use locally and/or send to try and verify what failures we could see with it (which was done.. bugs were filed tracking bug 1505898).

Since by now we already have a pref to actually turn oop-iframes on, there's no big reason to spend more effort on emulating it.

So I'll close this bug as it served its original goal.