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)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
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, sec-moderate, Whiteboard: [domsecurity-active][adv-main56+][adv-esr52.4+][post-critsmash-triage])

Attachments

(1 file)

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)
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.
Flags: needinfo?(dveditz)
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
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.