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

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.

Depends on D142596

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 on attachment 9270290 [details]
Bug 1761981, r=smaug!

Approved to land

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

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

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

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

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

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

Approved for 100.0b3

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

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

Approved for 91.9esr.

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

@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).

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?

Will touch base outside the bug.

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.

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

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

(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. :-)

Rebased and set to land.

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