Closed Bug 1521542 (CVE-2020-15653) Opened 5 years ago Closed 4 years ago

iframe sandbox can be escaped with rel=noopener/noreferrer when "allow-popups" specified, or in general with fission

Categories

(Core :: DOM: Security, enhancement, P3)

enhancement

Tracking

()

VERIFIED FIXED
mozilla79
Tracking Status
firefox-esr68 --- wontfix
firefox-esr78 79+ verified
firefox77 --- wontfix
firefox78 --- wontfix
firefox79 + verified

People

(Reporter: annevk, Assigned: sstreich)

References

()

Details

(Keywords: csectype-priv-escalation, sec-moderate, testcase, Whiteboard: [domsecurity-active][post-critsmash-triage][adv-main79+][adv-ESR78.1+])

Attachments

(3 files)

index.html:

<iframe srcdoc="<a href=popup.html target=test rel=noreferrer>Open a popup?</a>" sandbox="allow-popups"></iframe>

popup.html:

<script> document.write("Origin: " + self.origin + "<br>Name:" + self.name); </script>

I think ideally we'd ignore rel="noreferrer noopener" and create an auxiliary browsing context. Supporting non-auxiliary browsing contexts with inherited states seems like complexity that's better avoided, but perhaps there are some implications I haven't thought through.

(Note: developer tools around sandboxing doesn't seem great either. I filed bug 1521545 on that.)

Group: core-security → dom-core-security

The code example appears to be missing rel=noreferrer noopener'.

It appears that Chrome doesn't even run the JS at all in the popup. Where as Firefox will run it in the same context. I'm also confused why when served from a file:// it doesn't open a popup.

You need to specify either noreferrer or noopener for the sandbox escape (the example has noreferrer). And yes, the client is not supposed to run JavaScript since the example doesn't specify allow-scripts. (I don't know all nuances of file:// security.)

I discovered this by working on https://github.com/whatwg/html/pull/4284 by the way. I'll continue working on a specification-side fix for this, but I'll refrain from writing tests for now and explicitly pointing out this issue, until someone is okay with me doing so.

Christoph thinks it might be a problem with serialization of the security state, which might imply worse/other things might also be forgotten.

Flags: needinfo?(ckerschb)
Summary: sandbox="allow-popups" can be escaped with rel=noopener/noreferrer → iframe sandbox can be escaped with rel=noopener/noreferrer when "allow-popups" specified

I no longer agree with my recommendation in comment 0 by the way. Given the way most of this works across browsers already, it should create a non-auxiliary top-level browsing context (i.e., one without an opener) that has sandboxing flags set.

I'm confused with what you are recommending (especially originally).

If the frame is sandboxed then we don't want it to open a new window that is not sandboxed. Even if the noreferrer means it's not an auxiliary context it should inherit sandboxing. It should be a unique origin, not the same opaque origin as the original sandboxed frame.

Given CSP sandboxing we have no problems applying sandbox properties to a top-level window.

Hey Anne, please see previous comment.

Flags: needinfo?(annevk)

I think what is stated in comment 7 is what I also state in comment 6. So we agree? (My original recommendation was when I was more hopeful about being able to keep creating a new top-level browsing context (that's not an auxiliary browsing context) "clean", without it ever having to inherit state. Turns out that's not an option and doesn't match reality anywhere.)

Flags: needinfo?(annevk)

I /think/ this data: url represents Anne's testcase (you have to toggle the pref to allow top-level data urls):

data:text/html,<iframe srcdoc="<a href=data:text/html;base64,PHNjcmlwdD4gZG9jdW1lbnQud3JpdGUoIk9yaWdpbjogIiArIHNlbGYub3JpZ2luICsgIjxicj5OYW1lOiIgKyBzZWxmLm5hbWUpOyA8L3NjcmlwdD4=%20target=test%20rel=noreferrer>Open%20a%20popup?</a>"%20sandbox="allow-popups"></iframe>

When I do that script runs in the popup window which means it's not inheriting the sandbox (it says "Origin: null" but that's because of the data: url and not the sandbox). It does not have an opener so that part works at least. Does escape the sandbox.

Priority: -- → P3
Whiteboard: [domsecurity-backlog1]

Christoph thinks it might be a problem with serialization of the security state, which might imply worse/other things might also be forgotten.

We just looked into that problem during triage. We concluded it's most likely some docshell/principal kind of bug rather than a loadinfo serialization kind of problem. Clearing ni?...

Flags: needinfo?(ckerschb)

The bug here is that nsWindowWatcher::OpenWindowInternal basically has this logic:

  1. If we plan to open a new window and have a parent, grab the parent's sandbox flags.
  2. In the noopener case, when we ask ContentChild to provide a new window, it puts it in a new process, because "dom.noopener.newprocess.enabled" is true.
  3. In the different-process case, we don't do anything with the sandbox flags and return (bug!).
  4. Set the one permitted sandboxed navigator if we have sandbox flags.
  5. Copy the sandbox flags to the new docshell if SANDBOX_PROPAGATES_TO_AUXILIARY_BROWSING_CONTEXTS is set.

In the same-process case (if I flip that "dom.noopener.newprocess.enabled" pref), things work correctly: the popup gets sandboxed.

So we really need to solve this for fission. We need to pass the sandbox flags through at least the window provider to wherever the docshell gets created. That means adding them to ProvideWindow or one of its arguments (e.g. the load state). We further need to figure out how to implement the "one permitted sandboxed navigator" bit, though it's less of a problem for noopener because noopener doesn't give the caller a thing to navigate.

Kyle, is this something that generally falls into the stuff you're working on?

Flags: needinfo?(kyle)
Summary: iframe sandbox can be escaped with rel=noopener/noreferrer when "allow-popups" specified → iframe sandbox can be escaped with rel=noopener/noreferrer when "allow-popups" specified, or in general with fission

Yeah, if this involves attaching stuff to DocShellLoadState, it's probably something I should be dealing with.

Assignee: nobody → kyle
Status: NEW → ASSIGNED
Flags: needinfo?(kyle)
Assignee: kyle → nobody
Status: ASSIGNED → NEW
Blocks: 1469429
Flags: needinfo?(nika)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #12)

The bug here is that nsWindowWatcher::OpenWindowInternal basically has this logic:

  1. If we plan to open a new window and have a parent, grab the parent's sandbox flags.
  2. In the noopener case, when we ask ContentChild to provide a new window, it puts it in a new process, because "dom.noopener.newprocess.enabled" is true.
  3. In the different-process case, we don't do anything with the sandbox flags and return (bug!).
  4. Set the one permitted sandboxed navigator if we have sandbox flags.
  5. Copy the sandbox flags to the new docshell if SANDBOX_PROPAGATES_TO_AUXILIARY_BROWSING_CONTEXTS is set.

In the same-process case (if I flip that "dom.noopener.newprocess.enabled" pref), things work correctly: the popup gets sandboxed.

I think the solution here might be to not do a "different process" load if the SANDBOX_PROPAGATES_TO_AUXILIARY_BROWSING_CONTEXTS bit is set. We don't need a different process there, and it means that we don't have to add a extra logic to the process-switching case. The behaviour is still correct without the pref set (more correct, even). The only downside is that we don't potentially cycle content processes, which is probably OK for the sandboxed popup case.

So we really need to solve this for fission. We need to pass the sandbox flags through at least the window provider to wherever the docshell gets created. That means adding them to ProvideWindow or one of its arguments (e.g. the load state). We further need to figure out how to implement the "one permitted sandboxed navigator" bit, though it's less of a problem for noopener because noopener doesn't give the caller a thing to navigate.

Fission won't need to use the InOtherProcess codepath at all, as process switches will occur during navigations, rather than needing to happen at tab creation time. because of that, this will probably generally be handled OK in the fission case.

:bzbarsky, does that sound like a reasonable fix to you? I'm curious if there are other cases where we should suppress process switching to ensure we get full popup compatibility.

Flags: needinfo?(nika) → needinfo?(bzbarsky)

We don't need a different process there

So in general, new-process for noopener is an optimization, right? The observable behavior is supposed to be the same regardless? If so, not doing new-process for the specific case when we have sandbox flags is probably fine.

The only other case I'm aware of where the "create a new process" behavior of noopener is causing us problems is bug 1577903. I haven't been able to think of others where it would be an issue so far.

Flags: needinfo?(bzbarsky)

Hey Christoph, with Cross-Origin-Opener-Policy this problem is amplified as now anything that the sandbox popups (even without noopener/noreferrer) can escape the sandbox by using that response header. html/cross-origin-opener-policy/coop-sandbox.https.html in web-platform-tests demonstrates this.

Three questions:

  1. Should this be a blocker for shipping COOP? (We'd like to ship before the end of H1.)
  2. If yes, do you know who can work on this? (Unfortunately those working on COOP don't have much experience with the sandboxing code.)
  3. If yes, is it okay to mark this as blocking bug 1613066 for tracking purposes?
Flags: needinfo?(ckerschb)

(In reply to Anne (:annevk) from comment #16)

  1. Should this be a blocker for shipping COOP? (We'd like to ship before the end of H1.)

I would think so, yes.

  1. If yes, do you know who can work on this? (Unfortunately those working on COOP don't have much experience with the sandboxing code.)

Basti will take it.

  1. If yes, is it okay to mark this as blocking bug 1613066 for tracking purposes?

I would think so, yes.

Assignee: nobody → sstreich
Status: NEW → ASSIGNED
Flags: needinfo?(ckerschb)
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]

This should be relatively easy to get working by disabling the InDifferentProcess codepath when sandboxing will propagate to auxiliary browsing contexts as I suggested in comment 14. Disabling the code in this case won't be a workaround long-term, as the entire InDifferentProcess codepath can be removed once we have full fission support.

The easiest way to do this would probably be to add another check to the code in ContentChild::ProvideWindowCommon that forces loadInDifferentProcess to be false if the SANDBOX_PROPAGATES_TO_AUXILIARY_BROWSING_CONTEXTS flag is set. (https://searchfox.org/mozilla-central/rev/9f074fab9bf905fad62e7cc32faf121195f4ba46/dom/ipc/ContentChild.cpp#884-908)

You should be able to fetch the sandbox flags in this code by doing something like this:

uint32_t parentSandboxFlags = 0;
if (Document* doc = parent->GetDocument()) {
  parentSandboxFlags = doc->GetSandboxFlags();
}

Depends on D76704

Hi Sebastian, is there anything that blocks pushing the accepted patches? Thank you!

Flags: needinfo?(sstreich)
Attachment #9151497 - Attachment description: Bug 1521542 - Disable Process Switiching for Sandboxed Contexts r=ckerschb,nika → Bug 1521542 - Disable Process Switiching for Sandboxed Contexts r=ckerschb
Attachment #9151497 - Attachment description: Bug 1521542 - Disable Process Switiching for Sandboxed Contexts r=ckerschb → Bug 1521542 - Disable Process Switching for Sandboxed Contexts r=ckerschb

Nothing blocking i am aware of, marked the patch for checkin needed :)

Flags: needinfo?(sstreich)

This is only sec-moderate and should be OK to land without approval according to your linked document, no?

Flags: needinfo?(sstreich)
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
Flags: in-testsuite?
Group: dom-core-security → core-security-release

Is this ready for a Beta approval request?

Flags: needinfo?(sstreich)

Comment on attachment 9151497 [details]
Bug 1521542 - Disable Process Switching for Sandboxed Contexts r=ckerschb

Beta/Release Uplift Approval Request

  • User impact if declined: If declined, Popups that were opened from a Sandboxed Content escape the sandbox,
    this is a threat to the users security
  • 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 patch adds a new condition onto whether we want a BrowsingContext to switch processes.
    Both decisions are well tested and used in release, so adding a new condition should not impose a big risk.
  • String changes made/needed:
Flags: needinfo?(sstreich)
Attachment #9151497 - Flags: approval-mozilla-beta?

Comment on attachment 9151497 [details]
Bug 1521542 - Disable Process Switching for Sandboxed Contexts r=ckerschb

I'd prefer to let this ride 79, as we're out of beta runway for 78.

Attachment #9151497 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Comment on attachment 9151497 [details]
Bug 1521542 - Disable Process Switching for Sandboxed Contexts r=ckerschb

Per comment 28 & 29.

Attachment #9151497 - Flags: approval-mozilla-esr78?
Flags: qe-verify+
Whiteboard: [domsecurity-active] → [domsecurity-active][post-critsmash-triage]
QA Whiteboard: [qa-triaged]

Confirmed issue with 78.0.1 esr.
Fix verified with 79.0b2 on Windows 10, macOS 10.15.5, Ubuntu 18.

Comment on attachment 9151497 [details]
Bug 1521542 - Disable Process Switching for Sandboxed Contexts r=ckerschb

Approved for 78.1esr.

Attachment #9151497 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+

Verified for ESR with the task cluster builds.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [domsecurity-active][post-critsmash-triage] → [domsecurity-active][post-critsmash-triage][adv-main79+]
Whiteboard: [domsecurity-active][post-critsmash-triage][adv-main79+] → [domsecurity-active][post-critsmash-triage][adv-main79+][adv-ESR78.1+]
Alias: CVE-2020-15653
Group: core-security-release
Regressions: 1691203
See Also: → CVE-2022-26384
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: