783497 - refinePolicy doesn't respect other directives except default-src

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kailas]

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'

Comment 1

[Tanvi Vyas[:tanvi]]

There is a bug in our refinePolicy implementation.

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

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'?

Comment 4

[Kailas]

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

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Attachment: 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.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Thanks, jst.  Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3bc83c14073

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d3bc83c14073