Closed Bug 1799036 (CVE-2023-6869) Opened 2 years ago Closed 1 year ago

Contents can paint outside of an iframe by using <dialog>, position:fixed and transform

Categories

(Core :: Web Painting, defect, P1)

Product:
Core
Component:
Web Painting
Version:
Firefox 91
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
120 Branch
Tracking Flags:
Tracking Status
firefox-esr102 --- wontfix
firefox-esr115 --- wontfix
firefox106 --- wontfix
firefox107 - wontfix
firefox108 - wontfix
firefox109 - wontfix
firefox118 --- wontfix
firefox119 --- wontfix
firefox120 --- wontfix
firefox121 --- fixed

People

(Reporter: Oriol, Assigned: emilio)

References

Details

(4 keywords, Whiteboard: [adv-main121+])

Attachments

(7 files, 2 obsolete files)

testcase 1
307 bytes, text/html
Details
screenshot
3.72 KB, image/png
Details
testcase 2: possible-attack.html
7.42 KB, text/html
Details
testcase 3: possible attack, modified to repro further back by resetting `transform` and `translate` on the dialog
7.48 KB, text/html
Details
Bug 1799036 - Don't flatten away subdoc display items. r=mstange,tnikkel
48 bytes, text/x-phabricator-request
Details | Review
Bug 1799036 - Make top layer elements be fixed-pos containing blocks. r=dholbert,tnikkel
48 bytes, text/x-phabricator-request
Details | Review
advisory.txt
251 bytes, text/plain
Details
Attached file testcase 1 —

Load this testcase:

<!DOCTYPE html>
<iframe sandbox="allow-scripts" srcdoc='
  <main style="transform: scale(1)">
    <dialog>
      <div style="position: fixed; background: cyan; padding: 10px;">Painting outside iframe!</div>
    </dialog>
  </main>
  <script>document.querySelector("dialog").showModal();</script>
'></iframe>

The div is painted outside the sandboxed iframe!!!

Luckily, the parts outside the iframe don't seem to react to hit-testing, but it still seems problematic from a security perspective.

I can reproduce the problem at least since 2021-06-09 8508c35e49793fbe402ad2a706a57fabd1c2b0c4. Earlier builds instacrash any tab before I can try the testcase.

Attached image screenshot —

I imagine this could be taken advantage like in this example.
The user loads a trusted website that has legitimate content, but it also embeds something like a video from a third party.
If the third party turns evil, it can paint all the page in red, and place a scary message at the top of the page.
Then the user thinks the message comes from the trusted website, and calls the scammers.

Group: core-security → layout-core-security

(In reply to Oriol Brufau [:Oriol] from comment #0)

Earlier builds instacrash any tab before I can try the testcase.

If you're on linux, this is likely due to the fact that there's a range of Nightlies whose sandboxing code doesn't play well with current OS libraries (or something to that effect). Assuming you're running into this instacrash on mozregression, the workaround is to invoke mozregression like so, to disable the sandbox:

mozregression --pref  "security.sandbox.content.level:0"

In particular: that commit removed a transform decl on dialog:-moz-modal-dialog, which maybe we were accidentally relying on to make the dialog a container for fixed-pos elements...

Yeah -- probably not a real fix but if I add either transform: translateY(0%); or contain: paint; to that dialog:modal rule, then we seem to aggressively clip (to the bounds of the dialog itself) in the attached testcases. So the transform removal from html.css is what made these testcases start rendering "bad".

That transform styling wasn't strictly enforced, though, so I can repro it further back if I adjust the "possible attack" attachment to use <dialog style="transform: none !important">.

Attachment #9301896 - Attachment description: possible-attack.html → testcase 2: possible-attack.html
Attachment #9301891 - Attachment description: testcase → testcase 1

er, just resetting transform takes me back to bug 1651089 where we switched from translate to transform in the relevant html.css rule.

If I reset both transform and translate per this rule, I can repro further back..

Attachment #9302109 - Attachment is obsolete: true

testcase 3 repro's the issue back further than when dialog was default-enabled on Nightly.

After enabling the pref, mozregression gives me this range for testcase 3:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d6abd35b54ad39fa030217716b172fa9883bf3c8&tochange=9a2f741cef6ae4ae568a17fd3ffe52599f0a574f

That points to this commit:
https://hg.mozilla.org/mozilla-central/rev/fcfe734c80312372987f142c64747cccf95e511a
sefeng — Bug 1637307 - Push/Pop dialog to top layer when needed r=smaug,emilio

In "bad" builds just after that range, I do have to interact with the page -- e.g. mousewheel-scroll -- before the red overlay appears. But then it does appear and covers up the whole viewport.

In "good" builds before that range, the red content seems to be properly clipped to the iframe's bounds, just as it is in Chrome. (Minor difference: the dialog's clipped portions don't create scrollable overflow in the iframe in current Chrome -- i.e. no scrollbars show up there -- whereas it does create scrollable overflow in the iframe, in pre-regression "good" Firefox builds).

This does seem to be mitigated (i.e. I can't repro) if the iframe is truly cross-origin and hence out-of-process, FWIW.

e.g.
(1) if I take the srdoc=... attribute contents of testcase 3 and put them in a separate file called iframe.html and serve them over HTTP using e.g. python2 -m SimpleHTTPServer and then load the testcase as a file:/// URL (but with <iframe src="http://localhost:8000/iframe.html">), then the whole iframe ends up red, but none of the red leaks out into the outer page.

(2) If I repeat the process with both pages served over HTTP from different hosts, then I similarly don't see any red leaking out beyond the frame.

(3) If I serve both pages over http from the same host using different port numbers, then the red leaks out. (I had assumed it wouldn't since I thought port-number-differences made something cross-origin, but there's probably some subtlety I'm forgetting there... Maybe we share the same process for cross-origin hosts that only differ by port number, or something?)

Things can be same-site but not same-origin, in which case they have to be in the same process due to document domain or some such thing. That happens if they have the same host but different ports, or if some other parts of the URI don't match but some do.

Thanks, that matches what I'm seeing I think. I suppose that describes the attached testcases, which have the sandbox attribute, which IIUC means we should be treating the iframe as cross-origin. So while it's a nice bonus that this doesn't repro in an OOP iframe, but not a full mitigation since "same-process-but-still-cross-origin" scenarios do seem to be affected.

I've captured this bug in rr using the attached testcase 3 (which I believe is in the "same-process but cross-origin" category), and I've submitted it to pernosco, in case it's handy. I added a line of logging to note the moment (and PID) when we enter HTMLDialogElement::ShowModal -- it looks like this in the trace:

[Child 805041, Main Thread] WARNING: ****Now entering HTMLDialogElement::ShowModal: file /scratch/work/builds/mozilla-central/mozilla/dom/html/HTMLDialogElement.cpp:118

I'll share the pernosco URL once I've got it.

Severity: -- → S1
tracking-firefox107: --- → ?
tracking-firefox108: --- → ?
Priority: -- → P1
Keywords: sec-high

I wonder if bug 1799216 or so helps here fwiw (that fixes a clipping issue that could be relevant here).

It doesn't seem to. This does seem to properly fix it but I'm not sure why the current code doesn't quite work:

diff --git a/layout/generic/nsSubDocumentFrame.cpp b/layout/generic/nsSubDocumentFrame.cpp
index 5ebe2aab907db..fc7e6eed5a1fd 100644
--- a/layout/generic/nsSubDocumentFrame.cpp
+++ b/layout/generic/nsSubDocumentFrame.cpp
@@ -434,7 +434,8 @@ void nsSubDocumentFrame::BuildDisplayList(nsDisplayListBuilder* aBuilder,
   }
 
   DisplayListClipState::AutoSaveRestore clipState(aBuilder);
-  clipState.ClipContainingBlockDescendantsToContentBox(aBuilder, this);
+  clipState.ClipContentDescendants(GetContentRectRelativeToSelf() +
+                                   aBuilder->ToReferenceFrame(this));
 
   nsIScrollableFrame* sf = presShell->GetRootScrollFrameAsScrollable();
   bool constructZoomItem = subdocRootFrame && parentAPD != subdocAPD;
Component: Layout: Positioned → Web Painting

I'm not sure if the flattening here is worth it, probably not. If we can
flatten both the subdoc/zoom items, we might ignore the clip
incorrectly.

This simplifies the code, though see the bug comments, there might be a
better fix?

Assignee: nobody → emilio
Status: NEW → ASSIGNED
Group: layout-core-security → gfx-core-security

FYI, https://github.com/w3c/csswg-drafts/issues/8040 asks whether the dialog should be the containing block for fixed contents.
Basically the opposite of comment 5, so it would be a workaround for this problem.

Based on comment #9, this bug contains a bisection range found by mozregression. However, the Regressed by field is still not filled.

:emilio, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit auto_nag documentation.

Flags: needinfo?(emilio)
Keywords: regression
Flags: needinfo?(emilio)

(It's a bit more complicated, see above)

Priority: -- → P1

So the relevant frame structure here looks like

block (transformed)
-placeholder for dialog (true fixed pos)
-absolute list
--the "fixed" div

true fixed list
-dialog
--placeholder for the "fixed" div

Note that the placeholder for the "fixed" div is not in the containing block for its positioned frame. Our painting code is not able to handle situations like this, and it seems like it would be hard to make it work. Specifically when we are building display items for the transformed div at

https://searchfox.org/mozilla-central/rev/2f47e3dacf0d773e9c7f363cecf10cfbea490679/layout/generic/nsIFrame.cpp#3358

we clear the clip because the containing transform item will clip it's contents. So when we save the clip for the "fixed" div when we call MarkAbsoluteFramesForDisplayList there is no clip. We don't traverse into placeholders to top layer items in the normal display list building pass, instead they are handled after building the rest of the display list in the root scroll frame. At this point we follow the placeholder for the "fixed" div and we restore the empty clip and build the display items that are expecting to be inside the transform item that we built before, but they aren't inside the transform item now so they are unclipped.

Even with Emilio's patch applied we are still going to render testcases like this incorrectly (no matter how we define correctly). For example if one makes the iframe in testcase 1 scrollable the "painting outside iframe" content will not draw outside the iframe but it will appear fixed but disappear if you scroll far enough. (Safari also renders this testcases buggily).

So it seems the best course here is to get the spec updated and update our code to follow the spec so we can avoid problematic frame tree structures like this.

The other potential good thing about changing the frame tree here is that I believe (from my memory) a lot of the testcases that trip ASR related asserts involve fixed pos and dialog so that might fix those too.

Some previous similar bugs were rated sec-low (i.e. bug 1792643, bug 1735265) - is sec-high correct for this specific instance?

Flags: needinfo?(dholbert)

Yeah, I think this is essentially the same attack scenario as those other bugs (with the added comfort -- maybe also present in those bugs -- that fission provides a relatively-effictive mitigation as I indirectly noted in comment 10).

bug 1735265 comment 17 essentially describes the attack scenario here, I think, so it seems fine to apply the same security rating that we settled on there.

Flags: needinfo?(dholbert)
Keywords: sec-high → sec-low

This is the most reasonable behavior to preserve our layout invariants.

Once https://github.com/w3c/csswg-drafts/issues/8040 gets resolved we
can reconsider this.

I think at the very least we should promote fixed-pos elements inside
the top layer to the top layer, but that has its own set of
implications.

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1c7289369f60 Make top layer elements be fixed-pos containing blocks. r=dholbert
Backout by smolnar@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/77c3d7a4bf46 Backed out changeset 1c7289369f60 for causing wpt failures on /html/semantics/interactive-elements/the-dialog-element/top-layer-stacking.tentative.html CLOSED TREE
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cd7366c6b55d Make top layer elements be fixed-pos containing blocks. r=dholbert

https://hg.mozilla.org/mozilla-central/rev/cd7366c6b55d

Group: gfx-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox120: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 120 Branch

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox119 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(emilio)

There's some potential compat impact so I think this should ride the trains.

status-firefox119: fix-optional → wontfix
Flags: needinfo?(emilio)
Regressions: 1857946
Regressed by: 1858752
No longer regressed by: 1858752
Regressions: 1858752
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Regressions: 1863402

Well, yeah, but bug 1863402 also fixed this.

Status: REOPENED → RESOLVED
Closed: 1 year ago → 1 year ago
Resolution: --- → FIXED
Whiteboard: [adv-main121+]
Attached file advisory.txt (obsolete) —
Attached file advisory.txt —
Attachment #9367992 - Attachment is obsolete: true
Alias: CVE-2023-6869

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter

Group: core-security-release

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons

Keywords: reporter-external
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: