Closed Bug 1503984 Opened 3 years ago Closed 2 years ago

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


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




Fission Milestone M2
Tracking Status
firefox65 --- affected


(Reporter: Felipe, Assigned: Felipe, Mentored)


(Blocks 1 open bug)



(1 obsolete file)

(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
Priority: -- → P3
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.
I've offered to mentor Felipe through these changes after I'm back. It would be awesome to have this for the all-hands.
Assignee: nobody → felipc
Mentor: nika
Component: DOM → DOM: Core & HTML
Priority: P3 → P2
Summary: Add a pref to to return null on cross-origin frames lookups for chrome code → Add a pref to to return null on chrome->content subframe access
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:

More blocks are likely to be added in the future.
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)
Pushed by
Add a pref to block chrome code from accessing content subframes. r=nika
Backed out changeset 82ca028b0524 (Bug 1503984) for StaticPrefList failures.

Push with failures:

Backout link:

Failure log:

[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]
Flags: needinfo?(felipc)
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

Flags: needinfo?(felipc) → needinfo?(nika)
(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]
> 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.
Flags: needinfo?(nika)
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:
It was indeed the MessageEvent.source that caused that

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, but since it's also marked as CrossOriginReadable, the codegen throws this error:

How should I proceed?

Flags: needinfo?(nika)

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

Flags: needinfo?(peterv)

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

Flags: needinfo?(nika) → needinfo?(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.

Flags: needinfo?(bzbarsky)
Flags: needinfo?(peterv)
Fission Milestone: --- → M1
Whiteboard: [2/14] per comment 11, resolving issues after rebasing the patch
Whiteboard: [2/14] per comment 11, resolving issues after rebasing the patch → [2/22] patch r+, ready to land
Whiteboard: [2/22] patch r+, ready to land → [2/22] patch needs to be rewritten with different approach
Fission Milestone: M1 → M2

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?

Flags: needinfo?(felipc)

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.

Closed: 2 years ago
Flags: needinfo?(felipc)
Resolution: --- → WORKSFORME
Whiteboard: [2/22] patch needs to be rewritten with different approach
Attachment #9028166 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.