refinePolicy doesn't respect other directives except default-src

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Kailas, Assigned: geekboy)

Tracking

14 Branch
mozilla17
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.1 (KHTML, like Gecko) Chrome/21.0.1180.79 Safari/537.1

Steps to reproduce:

suppose policy_1 sets, default-src 'self' script-src 'self'
policy_2 sets script-src 'self'
If I used 
csp.refinePolicy(policy_1, selfURI);
csp.refinePolicy(policy_2, selfURI);





Actual results:

it sets
 script-src 'none' 



Expected results:

The expected ourcome for script-src should be :
script-src 'self'
(Reporter)

Updated

5 years ago
Depends on: 764937
There is a bug in our refinePolicy implementation.
If we're going to fix this we should consider going ahead and implementing the spec's algorithm now rather than having to change things again later. I think we just store the policies separately in a list and then apply each one in order. This will work fine for the content loading policies and get the reporting correct, that is, a violation of policy B should not be sent to the report-uri from policy A.

This gets tricky with the inline/eval restrictions because for perf reasons the value is cached. We will eventually need a way to get the engine to reset its cached value (esp. when we support <meta> policies), although until then we might just be able to check through the policies to find the appropriate report-uris when a violation is signaled.
(Assignee)

Comment 3

5 years ago
The way I'm understanding the problem here, I don't think this is a bug.  The policy "script-src 'self'" turns into "default-src 'none'" as intended.  Then, when refine-policy happens, everything gets essentially nullified.

The old CSP spec used to require a default-src (er, "allow") directive in *all* policies when they're created.  This was intended to require policy writers to be very specific about their controls.

If we do want to make a change here, it is bug 764937, and should follow with CSP 1.0 conformance and not the default.

Kailas: can you confirm that "script-src 'self'", when parsed (due to makeExplicit()), ends up with all directives set to 'none'?
(Reporter)

Comment 4

5 years ago
Sid: The  "https://csptest.computerist.org" website set following CSP rules:

allow 'self'; img-src 'self'; script-src 'self'; options 'bogus-option'; report-uri https://unknown.computerist.org:8443/report 

If user defined policy is as follows:
default-src 'none' ; script-src 'self' ; 

Then the output of refinePolicy method
"csp.refinePolicy(websiteRules,selfURI); 
csp.refinePolicy(userRules,seflURI); "
is as follows:

default-src 'none'; script-src *; style-src 'none'; media-src 'none'; img-src 'none'; object-src 'none'; frame-src 'none'; frame-ancestors *; font-src 'none'; xhr-src 'none' 

I don't understand, why "script-src *" is set to *. Both website and user CSP policy contains "script-src 'self'". It should be either 'self' or 'none' if default-src is enforced. 

Suppose user policy is as follows:

default-src * ; script-src 'self' 

Then the outcome of refinePolicy method calls on website and user rules is as follows:

default-src https://csptest.computerist.org:443; script-src *; style-src https://csptest.computerist.org:443; media-src https://csptest.computerist.org:443; img-src https://csptest.computerist.org:443; object-src https://csptest.computerist.org:443; frame-src https://csptest.computerist.org:443; frame-ancestors *; font-src https://csptest.computerist.org:443; xhr-src https://csptest.computerist.org:443
(Assignee)

Comment 5

5 years ago
Ah, yes, that more complete policy makes sense.

Looks like when we intersect two sources we ignore whether they are 'self' or not.  That's the crux of the problem: a source knows if it is self, but intersectWith ignores that.
(Assignee)

Comment 6

5 years ago
Created attachment 652912 [details] [diff] [review]
test and patch

Hey dveditz: can you review this?  It's pretty trivial -- I just have to tweak the intersection logic to resolve "self" information in the CSPSources.
Assignee: nobody → sstamm
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #652912 - Flags: review?(dveditz)
Component: Untriaged → Security
Product: Firefox → Core

Updated

5 years ago
Attachment #652912 - Flags: review?(dveditz) → review+
(Assignee)

Comment 7

5 years ago
Thanks, jst.  Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3bc83c14073
Assignee: sstamm → nobody
Component: Security → DOM: Core & HTML
Target Milestone: --- → mozilla17

Updated

5 years ago
Assignee: nobody → sstamm
https://hg.mozilla.org/mozilla-central/rev/d3bc83c14073
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.