Closed Bug 1761981 (CVE-2022-29911) Opened 2 years ago Closed 2 years ago

Firefox sandbox iframe can execute scripts without allow-scripts directive

Categories

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

defect

Tracking

()

RESOLVED FIXED
101 Branch
Tracking Status
firefox-esr91 100+ fixed
firefox98 --- wontfix
firefox99 --- wontfix
firefox100 + fixed
firefox101 + fixed

People

(Reporter: trungpaaa, Assigned: nika, NeedInfo)

References

()

Details

(Keywords: reporter-external, sec-high, Whiteboard: [reporter-external] [client-bounty-form][post-critsmash-triage][adv-main100+][adv-esr91.9+])

Attachments

(4 files, 1 obsolete file)

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

Flags: sec-bounty?
Group: firefox-core-security → dom-core-security
Component: Security → DOM: Security
Product: Firefox → Core

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(nika)
Keywords: sec-high
See Also: → CVE-2022-26384
Component: DOM: Security → DOM: Core & HTML
Assignee: nobody → nika
Severity: -- → S2
Priority: -- → P1

Depends on D142596

Flags: needinfo?(nika)

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.
Attachment #9270290 - Flags: sec-approval?
Attachment #9270291 - Flags: sec-approval?

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

Approved to land

Attachment #9270290 - Flags: sec-approval? → sec-approval+

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

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

Attachment #9270291 - Flags: sec-approval?
Flags: in-testsuite?

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
Attachment #9270290 - Flags: approval-mozilla-esr91?
Attachment #9270290 - Flags: approval-mozilla-beta?
Attachment #9270291 - Flags: approval-mozilla-beta?
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 101 Branch
Attachment #9270291 - Flags: approval-mozilla-beta?

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

Approved for 100.0b3

Attachment #9270290 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: sec-bounty? → sec-bounty+

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

Approved for 91.9esr.

Attachment #9270290 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Flags: qe-verify-
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage] → [reporter-external] [client-bounty-form][post-critsmash-triage][adv-main100+][adv-esr91.9+]
Alias: CVE-2022-29911

@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?

Flags: needinfo?(tom)

Will touch base outside the bug.

Flags: needinfo?(tom)
Attached file advisory.txt
Attachment #9274257 - Attachment is obsolete: true
See Also: → CVE-2022-34468
Whiteboard: [reporter-external] [client-bounty-form][post-critsmash-triage][adv-main100+][adv-esr91.9+] → [reporter-external] [client-bounty-form][post-critsmash-triage][adv-main100+][adv-esr91.9+][reminder 2022-05-16]
Whiteboard: [reporter-external] [client-bounty-form][post-critsmash-triage][adv-main100+][adv-esr91.9+][reminder 2022-05-16] → [reporter-external] [client-bounty-form][post-critsmash-triage][adv-main100+][adv-esr91.9+][reminder-test 2022-05-16]
Group: core-security-release

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.

Flags: needinfo?(nika)
Whiteboard: [reporter-external] [client-bounty-form][post-critsmash-triage][adv-main100+][adv-esr91.9+][reminder-test 2022-05-16] → [reporter-external] [client-bounty-form][post-critsmash-triage][adv-main100+][adv-esr91.9+]

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

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

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

Flags: needinfo?(nika)
Flags: needinfo?(nika)

Rebased and set to land.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: