1761981 - (CVE-2022-29911) Firefox sandbox iframe can execute scripts without allow-scripts directive

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Trung Pham]

When an iframe with the sandbox attribute set to allow-same-origin and allow-top-navigation/allow-top-navigation-by-user-activation (the below snippet), the embedded document can execute the script in the context of the parent document when users click on a javascript URL even if the sandbox attribute does not contain the allow-scripts directive.

<iframe sandbox="allow-same-origin allow-top-navigation-by-user-activation" srcdoc="<a target='_top' href='javascript:alert(location)'>alert</a>">

While I understand that it's logical to allow the embedded document to change the top.location to a javascript URL in this case, it would break the sandbox protection and make it no more secure than not using the sandbox attribute at all (e.g the embedded document can remove the sandbox attribute or reload itself in a new iframe).

It seems Chromium and Safari don't allow this behavior. I also think that is a more reasonable approach because normal users wouldn't expect an iframe without the allow-scripts directive to execute arbitrary scripts.

I noticed the behavior when trying the live demo of a markdown library (https://github.com/markedjs/marked).
The untrusted HTML is sandboxed inside an iframe with the sandbox attribute set to "allow-same-origin allow-top-navigation-by-user-activation": https://marked.js.org/demo/?text=[click me](javascript%3Aalert(location))

The application basically does the following:

<iframe id="previewIframe" sandbox="allow-same-origin allow-top-navigation" srcdoc="<base target='_parent'>"></iframe>
<script>
        var parsedMarkdown = "<a href=javascript:alert(location)>click me</a>";
        previewIframe.addEventListener("load", function(){
                previewIframe.contentDocument.body.innerHTML = parsedMarkdown;
        });
</script>

This issue might be related to https://bugzilla.mozilla.org/show_bug.cgi?id=785310

Comment 1

[Daniel Veditz [:dveditz]]

Nika: unfortunately the fix for bug 1744352 missed this case (it's an existing window, so copying the sandbox state isn't appropriate). I bet you could target some other named frame as well, though that would only rarely be useful.

Comment 2

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

Attachment: Bug 1761981, r=smaug!

Comment 3

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

Attachment: Bug 1761981 - tests, r=smaug!

Depends on D142596

Comment 4

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

Comment on attachment 9270290 [details]
Bug 1761981, r=smaug!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch likely makes it fairly obvious to a familiar attacker what the issue might be, and the test paints a fairly clear bulls-eye on the issue. We may want to land the tests desperately.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I would expect that this patch directly applies to all branches which bug 1744352 has been uplifted to.
• How likely is this patch to cause regressions; how much testing does it need?: I think it is fairly unlikely to cause regressions, as it's a fairly limited change.

Comment 5

[Tom Ritter [:tjr]]

Comment on attachment 9270290 [details]
Bug 1761981, r=smaug!

Approved to land

Comment 6

[Tom Ritter [:tjr]]

Comment on attachment 9270291 [details]
Bug 1761981 - tests, r=smaug!

Clearing sec-approval; we can land the test on or after May 16

Comment 7

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

Comment on attachment 9270290 [details]
Bug 1761981, r=smaug!

Beta/Release Uplift Approval Request

• User impact if declined: Potential iframe sandbox tag violation
• 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): Fairly straightforward change already landed on Nightly
• String changes made/needed: N/A

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Potential iframe sandbox tag violation
• Fix Landed on Version: 101
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fairly straightforward change already landed on Nightly

Comment 8

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

Attachment: Bug 1761981 - fix unexpected-pass wpts, r=smaug!

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

r=smaug
https://hg.mozilla.org/integration/autoland/rev/3150596d4d85b114272cc68916b7fcd5220ee480
https://hg.mozilla.org/mozilla-central/rev/3150596d4d85

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

fix unexpected-pass wpts
https://hg.mozilla.org/integration/autoland/rev/6ed148583fbc65aa43dd228137b3df115666b2cc
https://hg.mozilla.org/mozilla-central/rev/6ed148583fbc

Comment 11

[Dianna Smith [:diannaS]]

Comment on attachment 9270290 [details]
Bug 1761981, r=smaug!

Approved for 100.0b3

Comment 12

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/387aa3589869
https://hg.mozilla.org/releases/mozilla-beta/rev/534ced26118c

Comment 13

[Dianna Smith [:diannaS]]

Comment on attachment 9270290 [details]
Bug 1761981, r=smaug!

Approved for 91.9esr.

Comment 14

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/d7e099f6fc71ba0170d26243dccca08fb6267815
https://hg.mozilla.org/releases/mozilla-esr91/rev/ef64358e527c39caf3f301a76d1c8ce692e34131

Comment 15

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 16

[Trung Pham]

@freddy: someone asked me about this issue because they thought it was mistakenly treated as a bug.
allow-top-navigation and allow-top-navigation-by-user-activation directives are used to allow top-level navigations so "Firefox did not properly protect against top-level navigations" seems a bit confusing. Maybe It would be better to clarify that the bug could lead to script execution in violation of the sandbox (without allow-scripts).

Comment 17

[Frederik Braun [:freddy]]

Indeed, the advisory was a bit oddly formulated. I'm not sure if and how we can change advisory texts if they are incorrect. Tom, do you know?

Comment 18

[Tom Ritter [:tjr]]

Will touch base outside the bug.

Comment 19

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 20

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

7 months ago, Tom Ritter [:tjr] placed a reminder on the bug using the whiteboard tag [reminder-test 2022-05-16] .

nika, please refer to the original comment to better understand the reason for the reminder.

Comment 21

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

I don't know why this reminder was added, redirecting the ni? back to :tjr

Comment 22

[Tom Ritter [:tjr]]

It was to land https://phabricator.services.mozilla.com/D142597

Comment 23

[:Gijs (he/him)]

(In reply to Tom Ritter [:tjr] from comment #22)

It was to land https://phabricator.services.mozilla.com/D142597

Bouncing this back because it seems to have gotten lost again - I think the test still needs to land here. :-)

Comment 24

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

Rebased and set to land.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/324defc46b66
tests, r=smaug

Comment 26

[Cosmin Sabou [:CosminS]]

Backed out for mochitest plain failures on test_top_navigation_no_script.html

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=324defc46b66e3f2c283992e92a564f89da77dae&selectedTaskRun=Mo7ICk5AQ82jaePfRJlahA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=418164345&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/89697e87849d85931ae6281d4213a9ab882eabf2