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

RESOLVED WORKSFORME

Status

()

P2
normal
RESOLVED WORKSFORME
5 months ago
3 days ago

People

(Reporter: Felipe, Assigned: Felipe, Mentored)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Fission Milestone:M2, firefox65 affected)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
(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
(Assignee)

Comment 1

5 months ago
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

5 months ago
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
(Assignee)

Updated

5 months ago
Blocks: 1505898
(Assignee)

Updated

4 months ago
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
(Assignee)

Comment 3

4 months ago
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.
(Assignee)

Comment 4

4 months ago
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)
Status: NEW → ASSIGNED

Comment 5

3 months ago
Pushed by fgomes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82ca028b0524
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: 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]
Flags: needinfo?(felipc)
(Assignee)

Comment 7

3 months ago
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
Flags: needinfo?(felipc) → needinfo?(nika)

Comment 8

3 months ago
(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.
Flags: needinfo?(nika)
(Assignee)

Comment 9

3 months ago
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
(Assignee)

Comment 10

3 months ago
It was indeed the MessageEvent.source that caused that
(Assignee)

Comment 11

2 months ago

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?

Flags: needinfo?(nika)

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

Flags: needinfo?(peterv)

Comment 13

2 months ago

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)
(Assignee)

Updated

2 months ago
Flags: needinfo?(peterv)

Updated

a month ago
Fission Milestone: --- → M1

Updated

a month ago
Whiteboard: [2/14] per comment 11, resolving issues after rebasing the patch

Updated

a month ago
Whiteboard: [2/14] per comment 11, resolving issues after rebasing the patch → [2/22] patch r+, ready to land
(Assignee)

Updated

a month ago
Whiteboard: [2/22] patch r+, ready to land → [2/22] patch needs to be rewritten with different approach

Updated

27 days ago
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)
(Assignee)

Comment 16

3 days ago

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.

Status: ASSIGNED → RESOLVED
Last Resolved: 3 days ago
Flags: needinfo?(felipc)
Resolution: --- → WORKSFORME
Whiteboard: [2/22] patch needs to be rewritten with different approach
You need to log in before you can comment on or make changes to this bug.