Closed Bug 1396320 (CVE-2017-7823) Opened 3 years ago Closed 3 years ago

allow-same-origin capability always granted in CSP sandbox directive


(Core :: DOM: Security, defect, P1)

57 Branch



Tracking Status
firefox-esr52 56+ fixed
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed


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



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


(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
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.
"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
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

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.

Attachment #8905050 - Flags: review?(dveditz)
Flags: sec-bounty?
Group: core-security → dom-core-security
Comment on attachment 8905050 [details] [diff] [review]

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 @@
> +<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'.

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.
Closed: 3 years ago
Flags: needinfo?(ckerschb)
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8905050 [details] [diff] [review]

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]:

[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]:

[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:

Risk to taking this patch (and alternatives if risky):

String or UUID changes made by this patch: 
Flags: needinfo?(ckerschb)
Attachment #8905050 - Flags: approval-mozilla-esr52?
Attachment #8905050 - Flags: approval-mozilla-beta?
Comment on attachment 8905050 [details] [diff] [review]

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.