Sandboxed iFrame XSS - javascript URI's run with target _blank
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: emcmanus, Assigned: nika)
References
()
Details
(Keywords: csectype-sop, reporter-external, sec-high, Whiteboard: [reporter-external] [client-bounty-form][post-critsmash-triage][sec-survey][adv-main98+][adv-esr91.7+])
Attachments
(4 files, 1 obsolete file)
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr91+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr91+
|
Details | Review |
298 bytes,
text/plain
|
Details |
(Thank you Firefox developers for all your hard work! The world needs Firefox.)
I believe the following is an XSS vulnerability present in the current release channel. By specifying target="blank" you can execute javascript in the domain of a sandboxed iframe, even though allow-scripts is not specified.
This behavior is blocked by Edge, Chrome, and Safari. Demo: https://emcmanus.github.io/xss.html
Expected result:
- Sandboxed iframes should not execute JS unless
allow-scripts
is specified - Firefox should block the
javascript:
uri - Firefox behavior should match other browsers
Actual result:
- Javascript runs in the current domain, even though
allow-scripts
is not specified - Edge, Safari and Chrome block this behavior
- It does not matter whether you use src or srcdoc
I believe a related bug was patched in Firefox 79: https://www.mozilla.org/en-US/security/advisories/mfsa2020-30/#CVE-2020-15653
Here is the source of the demo:
<!DOCTYPE html>
<html>
<head>
<meta http-equiv='content-type' content='text/html; charset=UTF-8'>
</head>
<body>
<iframe sandbox="allow-same-origin allow-popups" srcdoc="<a href="javascript:alert(document.domain)" target="_blank">click me</a>"></iframe>
</body>
</html>
Updated•3 years ago
|
Comment 3•3 years ago
|
||
I started looking at the patch from bug 1521542 and came to the conclusion that this is likely fixed in ContentChild::ProvideWindowCommon()
too, but I also wanted to start with a test to ensure this is the right direction..
However, when looking at existing tests in dom/html/test. It's a bit tricky to make this work with calling ok()
(or postMessage
) on the opener, since the new, _blank
context does not seem to have an opener?!
I'll keep looking.
Updated•3 years ago
|
Comment 4•3 years ago
|
||
It's hard to tell simply by comparing to chrome behavior because they always block javascript:
links targeted at a new window regardless of sandboxing.
Things that could be OK:
- the sandbox has allow-same-origin (a running script being same-origin could be fine)
- the sandbox allows popups
Things we get wrong:
- the sandbox does NOT have
allow-popups-to-escape-sandbox
- the sandbox does NOT have
allow-scripts
Javascript urls are weird creatures and this bug might just beunique to that case, but to be sure: do we have tests that ensure popups from a sandbox are themselves properly sandboxed and, with this collection of attributes, can't run scripts?
However, when looking at existing tests in dom/html/test. It's a bit tricky to make this work with calling ok() (or postMessage) on the opener, since the new, _blank context does not seem to have an opener?!
See bug 1522083, and also
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-target
https://github.com/whatwg/html/issues/4078
You should be able to set an explicit rel="opener"
Comment 5•3 years ago
•
|
||
do we have tests that ensure popups ...
And I guess we need to make sure window.open() and target=_blank are treated the same wrt inheriting the sandbox properties.
These cases should all be covered by WPT, except the case in this bug because we're the only browser engine left that opens javascript links in a new window so no point in writing cross-browser tests for it. (Maybe Internet Explorer still does, too?). Even Brendan's own browser doesn't try to bring that behavior back -- time to throw in the towel? Would certainly stop a bunch of unique-to-Firefox exploit attempts.
Updated•3 years ago
|
Comment 6•3 years ago
|
||
Nika has looked at this, and wrote a patch. I think she's trying to figure out exactly how to test it.
Assignee | ||
Comment 7•3 years ago
|
||
Assignee | ||
Comment 8•3 years ago
|
||
Depends on D138209
Updated•3 years ago
|
![]() |
||
Comment 9•3 years ago
|
||
Landed:
https://hg.mozilla.org/integration/autoland/rev/b3a8f99f0044f5a4c7c44878d67c3de880b68340
https://hg.mozilla.org/integration/autoland/rev/1a7d29ce984cc34ea7c3932f7b11976794177647
Backed out for causing failures at test_javascript_sandboxed_popup.html:
https://hg.mozilla.org/integration/autoland/rev/80065f2457a7024113287320b483adf15bce8752
Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel&revision=1a7d29ce984cc34ea7c3932f7b11976794177647&selectedTaskRun=Q5xgKgmZROGoqISWEoL9Sg.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=367256583&repo=autoland
TEST-UNEXPECTED-FAIL | docshell/test/mochitest/test_javascript_sandboxed_popup.html | Test timed out. -
after start.
Assignee | ||
Updated•3 years ago
|
![]() |
||
Comment 10•3 years ago
|
||
Part 1: Ensure we set InitialSandboxFlags when opening or replacing toplevel BrowsingContexts, r=smaug
https://hg.mozilla.org/integration/autoland/rev/a4c83abc85ffa4eaeba778334e80f7fd07a7a62c
https://hg.mozilla.org/mozilla-central/rev/a4c83abc85ff
Part 2: Add test for sandboxing javascript uris in pop-ups, r=smaug
https://hg.mozilla.org/integration/autoland/rev/1d92ad59b6416da7c2a34e7c98d393c0e7725a8c
https://hg.mozilla.org/mozilla-central/rev/1d92ad59b641
Updated•3 years ago
|
Comment 11•3 years ago
|
||
The patch landed in nightly and beta is affected.
:nika, is this bug important enough to require an uplift?
If not please set status_beta
to wontfix
.
For more information, please visit auto_nag documentation.
Updated•3 years ago
|
Updated•3 years ago
|
Comment 12•3 years ago
|
||
We rated this "moderate" incorrectly (thinking of CSP bypasses), but sandboxing is more fundamentally and relied on more broadly on the web. raising to sec-high
Comment 13•3 years ago
|
||
This needs an uplift to beta and ESR. We're running out of betas (this might be the last week?), please do it soon.
Comment 14•3 years ago
•
|
||
Comment on attachment 9262937 [details]
Bug 1744352 - Part 1: Ensure we set InitialSandboxFlags when opening or replacing toplevel BrowsingContexts, r=smaug
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: Note, the patch applies to esr91 with --fuzz=3
- User impact if declined: Executing javascript in the domain of a sandboxed iframe, even though allow-scripts is not specified
- Fix Landed on Version: 99
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Passing around the flags from the original browsing context to the new one should be relatively safe.
Beta/Release Uplift Approval Request
- User impact if declined:
- 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):
- String changes made/needed: NA
Updated•3 years ago
|
Comment 15•3 years ago
|
||
Comment on attachment 9262937 [details]
Bug 1744352 - Part 1: Ensure we set InitialSandboxFlags when opening or replacing toplevel BrowsingContexts, r=smaug
sorry, fuzz didn't do the right thing here after all
Updated•3 years ago
|
Comment 16•3 years ago
|
||
Comment on attachment 9262937 [details]
Bug 1744352 - Part 1: Ensure we set InitialSandboxFlags when opening or replacing toplevel BrowsingContexts, r=smaug
Beta/Release Uplift Approval Request
- User impact if declined: Executing javascript in the domain of a sandboxed iframe, even though allow-scripts is not specified
(This patch applies cleanly to beta, but does not to esr)
- 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): Passing around the flags from the original browsing context to the new one should be relatively safe.
- String changes made/needed: NA
Updated•3 years ago
|
Comment 17•3 years ago
|
||
Comment 18•3 years ago
|
||
Comment on attachment 9265068 [details]
Bug 1744352 - Part 1: Ensure we set InitialSandboxFlags when opening or replacing toplevel BrowsingContexts (for esr), r=mccr8
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration:
- User impact if declined: Executing javascript in the domain of a sandboxed iframe, even though allow-scripts is not specified
- Fix Landed on Version: 99
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Passing around the flags from the original browsing context to the new one should be relatively safe.
Updated•3 years ago
|
Updated•3 years ago
|
Comment 19•3 years ago
|
||
Comment on attachment 9262937 [details]
Bug 1744352 - Part 1: Ensure we set InitialSandboxFlags when opening or replacing toplevel BrowsingContexts, r=smaug
Approved for 98 beta 9, last beta, thanks.
Updated•3 years ago
|
Comment 20•3 years ago
|
||
uplift |
Updated•3 years ago
|
Comment 21•3 years ago
|
||
Verified as fixed on Windows 10 x64, macOS 11.6 and on Ubuntu 20.04 x64.
Comparing the results on Microsoft Edge and Chrome where the new tab loaded from the demo provided in the description redirect the user to about:blank#blocked instead of an empty address bar provided on Firefox.
Is this intended? Are there any intentions to change that?
Thanks.
Updated•3 years ago
|
Comment 22•3 years ago
|
||
Maybe Olli can answer the question sooner than Nika.
Comment 23•3 years ago
|
||
As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.
Please visit this google form to reply.
Comment 24•3 years ago
|
||
We don't show blocked, no.
Could you file a followup bug to do so.
Comment 25•3 years ago
|
||
Comment on attachment 9265068 [details]
Bug 1744352 - Part 1: Ensure we set InitialSandboxFlags when opening or replacing toplevel BrowsingContexts (for esr), r=mccr8
Approved for 91.7esr.
Updated•3 years ago
|
Comment 26•3 years ago
|
||
uplift |
Comment 27•3 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #24)
We don't show blocked, no.
Could you file a followup bug to do so.
Logged bug 1756999.
Thanks.
Comment 28•3 years ago
|
||
Should we consider the new bug a security issue also?
Thanks.
Comment 29•3 years ago
|
||
That isn't a security issue, just somewhat minor UI issue, IMO.
Assignee | ||
Updated•3 years ago
|
Comment 30•3 years ago
|
||
Verified as fixed on Windows 10 x64, macOS 11.6 and on Ubuntu 20.04 x64.
Updated•3 years ago
|
Comment 31•3 years ago
|
||
Updated•3 years ago
|
Comment 32•3 years ago
|
||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•9 months ago
|
Description
•