Closed
Bug 1396320
(CVE-2017-7823)
Opened 7 years ago
Closed 7 years ago
allow-same-origin capability always granted in CSP sandbox directive
Categories
(Core :: DOM: Security, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: s.h.h.n.j.k, Assigned: ckerschb)
References
Details
(Keywords: regression, sec-moderate, Whiteboard: [domsecurity-active][adv-main56+][adv-esr52.4+][post-critsmash-triage])
Attachments
(1 file)
7.02 KB,
patch
|
dveditz
:
review+
jcristau
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36 Steps to reproduce: 1. Go to https://test.shhnjk.com/allow-scripts.php?xss=%3Csvg%20onload=alert(document.domain)%3E 2. You will see alert with document.domain Actual results: alert with document.domain Expected results: Firefox should grant opaque origin to document when CSP sandbox doesn't have allow-same-origin flag. Currently, "Content-Security-Policy: sandbox allow-scripts" is enough to execute script with origin. https://www.w3.org/TR/CSP2/#directive-sandbox "The sandbox directive allows any resource, framed or not, to ask for the same sorts of restrictions to be applied to itself."
Comment 1•7 years ago
|
||
Christoph, can you forward / take a look?
Group: firefox-core-security → core-security
Component: Untriaged → DOM: Security
Flags: needinfo?(ckerschb)
Product: Firefox → Core
Comment 2•7 years ago
|
||
Firefox ESR 52 works as expected, current Release (55) does not. Regressed somewhere between the two.
Flags: needinfo?(dveditz)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ckerschb
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(ckerschb)
Priority: -- → P1
Whiteboard: [domsecurity-active]
Comment 3•7 years ago
|
||
Clarification: ESR 52.2 works as expected. I hadn't updated yet, and when I did found 52.3 also regressed. Nightly good build was July 11, bad build was July 12. That's this set of changes https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0e41d07a703f&tochange=09a4282d1172 Looks like the fix for bug 1377426 is the culprit.
Updated•7 years ago
|
status-firefox55:
--- → wontfix
status-firefox56:
--- → affected
status-firefox57:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox56:
--- → +
tracking-firefox57:
--- → +
tracking-firefox-esr52:
--- → 56+
Assignee | ||
Comment 4•7 years ago
|
||
Rrrh, this one is really unforuntate - beats me. Dan, back when you reviewed the initial fix of Bug 1377426 [1], the world was still ok, but then I don't know what happenend - it seems like a rebasing problem to me. Unfortunately this problem caused the sandbox flags to be merged before checking if we need a new principal. It's really too bad :-( To make things worse, the added test for Bug 1377426 was using file_sandbox_5.html instead of file_sandbox_13.html, which then internally caused to use img5_bad (and friends) instead of img13_bad (and friends). It's really too frustrating. Anyway, I added a complete new test exactly testing what was reported in this bug. @Jun: Thanks for reporting - we definitely need to uplift this one. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1377426#c11
Attachment #8905050 -
Flags: review?(dveditz)
Updated•7 years ago
|
Flags: sec-bounty?
Updated•7 years ago
|
Group: core-security → dom-core-security
Comment 5•7 years ago
|
||
Comment on attachment 8905050 [details] [diff] [review] bug_1396320_csp_sandbox.patch Review of attachment 8905050 [details] [diff] [review]: ----------------------------------------------------------------- r=dveditz with comment nit. ::: dom/base/nsDocument.cpp @@ +3022,5 @@ > bool needNewNullPrincipal = > (cspSandboxFlags & SANDBOXED_ORIGIN) && !(mSandboxFlags & SANDBOXED_ORIGIN); > + > + mSandboxFlags |= cspSandboxFlags; > + I'm sorry I didn't notice the order shift between patch revisions :-( ::: dom/security/test/csp/test_sandbox_allow_scripts.html @@ +1,4 @@ > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Bug 1396320: Fix CSP sandbox regression for allow-same-origin</title> did you mean allow-scripts? This doesn't seem to test allow-same-origin. I assume we do have a test for allow-same-origin (with scripts disabled) elsewhere; if not we should add one of those, too.
Attachment #8905050 -
Flags: review?(dveditz) → review+
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #5) > did you mean allow-scripts? This doesn't seem to test allow-same-origin. I > assume we do have a test for allow-same-origin (with scripts disabled) > elsewhere; if not we should add one of those, too. Thanks, in fact we do: csp/test_sandbox.html which checks for 'allow-same-origin'.
Comment 7•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3bf241aaf148 Please request Beta and ESR52 approval on this when you get a chance. It'll need some minor mochitest manifest fix-ups, but the bulk of the patch grafts cleanly.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(ckerschb)
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8905050 [details] [diff] [review] bug_1396320_csp_sandbox.patch Approval Request Comment CSP iframe sandbox does not apply allow-scripts correctly hence grants iframe possibility for XSS. [Feature/Bug causing the regression]: Bug 1377426 - Other CSP rules ignored when specifying sandbox 'allow-scripts' [User impact if declined]: Potential risk for XSS. [Is this code covered by automated tests?]: Yes, landed within this patch. [Has the fix been verified in Nightly?]: No, I guess not yet. [Needs manual test from QE? If yes, steps to reproduce]: Never hurts, but we have automated tests. [List of other uplifts needed for the feature/fix]: xxx [Is the change risky?]: No, the problem occured because of a rebasing problem, its only a off by one line change. [Why is the change risky/not risky?]: see above. [String changes made/needed]: no [Approval Request Comment] see above. If this is not a sec:{high,crit} bug, please state case for ESR consideration: Potential risk for XSS. We should uplift that patch. User impact if declined: see above. Fix Landed on Version: 57 Risk to taking this patch (and alternatives if risky): xxx String or UUID changes made by this patch: no
Flags: needinfo?(ckerschb)
Attachment #8905050 -
Flags: approval-mozilla-esr52?
Attachment #8905050 -
Flags: approval-mozilla-beta?
Comment 9•7 years ago
|
||
Comment on attachment 8905050 [details] [diff] [review] bug_1396320_csp_sandbox.patch csp fix, let's get this in 56.0b11 and esr52.4
Attachment #8905050 -
Flags: approval-mozilla-esr52?
Attachment #8905050 -
Flags: approval-mozilla-esr52+
Attachment #8905050 -
Flags: approval-mozilla-beta?
Attachment #8905050 -
Flags: approval-mozilla-beta+
Comment 10•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/879a566e2a14
Flags: in-testsuite+
Comment 11•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/bd284765b5bc
Updated•7 years ago
|
Group: dom-core-security → core-security-release
Updated•7 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•7 years ago
|
Alias: CVE-2017-7823
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main56+][adv-esr52.4+]
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [domsecurity-active][adv-main56+][adv-esr52.4+] → [domsecurity-active][adv-main56+][adv-esr52.4+][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•