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)

Product:
Core
Component:
DOM: Security
Version:
57 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 56+ fixed
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed

People

(Reporter: s.h.h.n.j.k, Assigned: ckerschb)

References

Details

(Keywords: regression, reporter-external, sec-moderate, Whiteboard: [domsecurity-active][adv-main56+][adv-esr52.4+][post-critsmash-triage])

Attachments

(1 file)

bug_1396320_csp_sandbox.patch
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."
Christoph, can you forward / take a look?
Group: firefox-core-security → core-security
Component: Untriaged → DOM: Security
Flags: needinfo?(ckerschb)
Product: Firefox → Core
Firefox ESR 52 works as expected, current Release (55) does not. Regressed somewhere between the two.
Flags: needinfo?(dveditz)
Keywords: regression, regressionwindow-wanted, sec-moderate
Assignee: nobody → ckerschb
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(ckerschb)
Priority: -- → P1
Whiteboard: [domsecurity-active]
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.
Blocks: CVE-2017-7803
Flags: needinfo?(dveditz)
Keywords: regressionwindow-wanted
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)
Flags: sec-bounty?
Group: core-security → dom-core-security
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+
(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'.
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
status-firefox57: affected → fixed
Flags: needinfo?(ckerschb)
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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 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+
Group: dom-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Alias: CVE-2017-7823
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main56+][adv-esr52.4+]
Flags: qe-verify-
Whiteboard: [domsecurity-active][adv-main56+][adv-esr52.4+] → [domsecurity-active][adv-main56+][adv-esr52.4+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: