The W3C spec doesn't say anything about "make explicit" and it isn't entirely necessary since the way the 1.0 spec doesn't really benefit from "normalizing" policies. Perhaps we should remove makeExplicit and make sure the rest of the policy checking still works in conformance with CSP 1.0.

Hangs on test run by make SOLO_FILE="test_cspreports.js" -C content/base/test check-one OUTPUT: /Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/_virtualenv/bin/python -u /Users/mmoutenot/projects/mozilla-central/config/pythonpath.py \ -I/Users/mmoutenot/projects/mozilla-central/build \ -I../../../_tests/mozbase/mozinfo \ /Users/mmoutenot/projects/mozilla-central/testing/xpcshell/runxpcshelltests.py \ --symbols-path=../../../dist/crashreporter-symbols \ --build-info-json=../../../mozinfo.json \ --test-path=test_cspreports.js \ --testing-modules-dir=../../../_tests/modules \ --profile-name=firefox \ --verbose \ \ /Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/dist/bin/xpcshell \ ../../../_tests/xpcshell/content/base/test/unit TEST-INFO | profile dir is /var/folders/4h/0g6j9v1s26j2tsgkp_922qx40000gn/T/firefox/xpcshellprofile TEST-INFO | /Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/_tests/xpcshell/content/base/test/unit/test_cspreports.js | running test ... TEST-INFO | /Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/_tests/xpcshell/content/base/test/unit/test_cspreports.js | full command: ['/Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/dist/bin/xpcshell', '-g', '/Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/dist/bin', '-a', '/Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/dist/bin', '-r', '/Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/dist/bin/components/httpd.manifest', '-m', '-n', '-s', '-e', 'const _HTTPD_JS_PATH = "/Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/dist/bin/components/httpd.js";', '-e', 'const _HEAD_JS_PATH = "/Users/mmoutenot/projects/mozilla-central/testing/xpcshell/head.js";', '-e', 'const _TESTING_MODULES_DIR = "/Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/_tests/modules/";', '-f', '/Users/mmoutenot/projects/mozilla-central/testing/xpcshell/head.js', '-e', 'const _SERVER_ADDR = "localhost"', '-e', 'const _HEAD_FILES = ["/Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/_tests/xpcshell/content/base/test/unit/head_utilities.js"];', '-e', 'const _TAIL_FILES = [];', '-e', 'const _TEST_FILE = ["/Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/_tests/xpcshell/content/base/test/unit/test_cspreports.js"];', '-e', '_execute_test(); quit(0);'] TEST-INFO | /Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/_tests/xpcshell/content/base/test/unit/test_cspreports.js | current directory: '/Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/_tests/xpcshell/content/base/test/unit' TEST-INFO | /Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/_tests/xpcshell/content/base/test/unit/test_cspreports.js | environment: ['XPCOM_DEBUG_BREAK=stack-and-abort', 'XPCSHELL_TEST_PROFILE_DIR=/var/folders/4h/0g6j9v1s26j2tsgkp_922qx40000gn/T/firefox/xpcshellprofile', 'NS_TRACE_MALLOC_DISABLE_STACKS=1', 'DYLD_LIBRARY_PATH=/Users/mmoutenot/projects/mozilla-central/obj-ff-dbg/dist/bin', 'XPCOM_MEM_LEAK_LOG=/var/folders/4h/0g6j9v1s26j2tsgkp_922qx40000gn/T/firefox/xpcshellprofile/runxpcshelltests_leaks.log', 'MOZ_CRASHREPORTER_NO_REPORT=1']

Tests were stalling due to the CSPRep not being made explicit before "shouldLoad" in contentSecurityPolicy.js. I added it to test, but understand if this is not the best remedy. Let me know if I should go about this a different way.

Comment on attachment 651142 [details] [diff] [review] Second Patch, Tests Pass Review of attachment 651142 [details] [diff] [review]: ----------------------------------------------------------------- This patch doesn't apply cleanly to mozilla-central. Does it depend on another patch? ::: content/base/src/CSPUtils.jsm @@ +423,5 @@ > > // if makeExplicit fails for any reason, default to default-src 'none'. This > // includes the case where "default-src" is not present. > + var defaultSrcDir = aCSPR._directives[SD.DEFAULT_SRC]; > + if (!defaultSrcDir) { No need to create a var here, just put the whole expression inside the if test. @@ +498,3 @@ > // make sure the context is valid > + for(var dirv in this._directives) > + { Please use the same code conventions for things like for() statements as reflected in the rest of the file. @@ +498,4 @@ > // make sure the context is valid > + for(var dirv in this._directives) > + { > + if(this._directives.hasOwnProperty(dirv)) What about directives that need to fall back to default-src? I'm not sure I understand how a non-explicit policy rep will appropriately check for permits here. I recommend doing this kind of test: if (aContext in CSPRep.SRC_DIRECTIVES) { // these all inherit default-src if they're not present if (aContext in this._directives) return this._directives[aContext].permits(aURI); else if (there's a default-src directive) return this._directives[default-src].permits(aURI); else return false; // fail closed: no applicable src directive } This is the step makeExplicit made unnecessary, but if we remove it, we have to check default-src for those directives that fall back. Please write a test that checks for this behavior. For example, test these loads: script "http://other.com/foo.js" (disallowed) img "http://other.com/foo.png" (allowed) script "http://self.com/foo.js" (allowed) against this policy: "default-src 'self'; img-src other.com; This may also remove the need for makeExplicit in some of the tests. Also, for CSP 1.0 (not the pre-spec version we're currently shipping), the last else body will have to be "return true" instead of false since I believe CSP 1.0 fails open. @@ +500,5 @@ > + { > + if(this._directives.hasOwnProperty(dirv)) > + { > + dump(dirv + " is in _directives\n"); > + if(dirv === aContext) Please merge this if and previous if statement into one. ::: content/base/test/unit/test_csputils.js @@ +376,5 @@ > // check default policy "allow *" > cspr = CSPRep.fromString("allow *", URI("http://self.com:80")); > + // make explicit for tests > + makeExplicit(cspr); > + I know we talked about this, but we probably shouldn't need to do this step in the tests. If we push the effect of makeExplicit into the permits() logic (see my comment above) we probably won't need it here, and don't want it here. Please remove unnecessary whitespace from this line too.

I think I'm getting the hang of this...

Comment on attachment 652946 [details] [diff] [review] Fifth, fixed whitespace changes Review of attachment 652946 [details] [diff] [review]: ----------------------------------------------------------------- Your styling for for/if blocks is inconsistent. Please use: for (foo statement) { body stuff; } and if (foo) { blah; } else { blah; } as used in the rest of the file. ::: content/base/src/CSPUtils.jsm @@ +499,5 @@ > + for (d in dirSet) > + if(dirSet[d] === dir) > + return true; > + return false; > + } You can use "dir in dirSet" instead of defining and calling this function since dirSet is an Object. For example: if (aContext in CSPRep.SRC_DIRECTIVES) { ... @@ +504,2 @@ > // make sure the context is valid > + if (is_dir_in_set(aContext, CSPRep.SRC_DIRECTIVES)){ You can use SD instead of CSPRep.SRC_DIRECTIVES here. @@ +506,5 @@ > + //these all inherit default-src if they're not present > + if (aContext in this._directives) > + return this._directives[aContext].permits(aURI); > + else if (aContext == SD.FRAME_ANCESTORS) > + return true; Put a comment here to say why "true" is default for FRAME_ANCESTORS. @@ +510,5 @@ > + return true; > + else if (this._directives[SD.DEFAULT_SRC]) > + return this._directives[SD.DEFAULT_SRC].permits(aURI); > + else > + return false; Please remove trailing whitespace... or this whole else statement and its body. All the branches of this if return, so the last one can just fall through to the return false at the end of the function. @@ +535,5 @@ > + if(this._directives.hasOwnProperty(dirv)) > + { > + newRep._directives[dirv] = this._directives[dirv] > + .intersectWith(aCSPRep._directives[dirv]); > + } What about directives in aCSPRep, but not in this one? Merge them like a zipper: iterate through all SD and decide what to do based on which of the two policies has the given directive (one, the other, or both). I made this next note in the previous feedback round: what about default-src? How does this logic work when there's a directive in one of the policies and default-src in the other that is more restrictive? Intersection requires that any URI in a context blocked by either policy would be blocked by the new one. Consider: policy_a: default-src *; img-src foo.com policy_b: default-src bar.com The intersection of a and b should be: policy_ab: default-src bar.com; img-src 'none' since the img_src for policy_b is inferred to be "bar.com" and when intersected with "foo.com" we get "'none'". @@ +559,5 @@ > + if(this._directives.hasOwnProperty(dirv)) > + { > + newRep._directives[dirv] = this._directives[dirv] > + .intersectWith(aCSPRep._directives[dirv]); > + } See my previous comment -- same here... how does this ensure proper intersection? ::: content/base/src/contentSecurityPolicy.js @@ +481,5 @@ > // interpret the context, and then pass off to the decision structure > CSPdebug("shouldLoad location = " + aContentLocation.asciiSpec); > CSPdebug("shouldLoad content type = " + aContentType); > var cspContext = ContentSecurityPolicy._MAPPINGS[aContentType]; > + this.makeExplicit(); What is this doing here? ::: content/base/test/unit/test_csputils.js @@ +373,5 @@ > var SD = CSPRep.SRC_DIRECTIVES; > > // check default policy "allow *" > cspr = CSPRep.fromString("allow *", URI("http://self.com:80")); > + Please remove the added whitespace. @@ +395,5 @@ > // check that the other directives were auto-filled with the > // DEFAULT_SRC one. > cspr_default_val = cspr._directives[SD.DEFAULT_SRC]; > + > + //make explicit for this test Why? Write that in the comment. @@ +408,5 @@ > // result in the same set of explicit policy directives > cspr = CSPRep.fromString("default-src *", URI("http://self.com:80")); > cspr_allow = CSPRep.fromString("allow *", URI("http://self.com:80")); > + makeExplicit(cspr); > + makeExplicit(cspr_allow); Please comment here to explain why you're using these. @@ +419,5 @@ > cspr = CSPRep.fromString("script-src https://foobar.com", > URI("http://self.com")); > cspr_allow = CSPRep.fromString("default-src *;", URI("http://self.com")); > + makeExplicit(cspr); > + makeExplicit(cspr_allow); here too.

When removing this, we should make sure the default behaviors for X- header and for the standardized header are correct. Basically when makeExplicit goes away the "inherited" policies picked up by src directives should be figured at enforcement time and not parsing time (permits() will pick the default based on presence of directives and header name). This is related a bit to bug 764937.

This removes makeExplicit and changes things that depend on explicit policies, though it makes the logic for intersectWith pretty complicated. I heavily commented it (and it passes all the CSP tests), but really we should move to a new architecture soon that doesn't intersect CSP policy objects at all. Garret, Tanvi: can you take a peek and gut-check?

Comment on attachment 756042 [details] [diff] [review] proposed patch Redirecting review to imelven.

Comment on attachment 756042 [details] [diff] [review] proposed patch Review of attachment 756042 [details] [diff] [review]: ----------------------------------------------------------------- A few questions in permits and intersectWith. Thanks for the very helpful comments ! I took a look to see what test coverage we have for CSPRep.intersectWith, it's used for every call to refinePolicy to refine "default-src *" with the supplied policy in the header. ::: content/base/src/CSPUtils.jsm @@ +471,5 @@ > > + > + // the X-Content-Security-Policy syntax requires an allow or default-src > + // directive to be present. > + if (!aCSPR._directives[SD.DEFAULT_SRC]) { allow is converted to default-src by the old parser, so this seems fine @@ +688,5 @@ > cspWarn(aCSPR, CSPLocalizer.getFormatStr("couldNotProcessUnknownDirective", [dirname])); > > } // end directive: loop > > + return aCSPR; getting the default right here is bug 764937, which is ready to land with this patch @@ +765,5 @@ > return this._directives[aContext].permits(aURI); > } > } > > + // frame-ancestors is a special case; it doesn't fall back to default-src. and we will have handled it above if it was present @@ +778,5 @@ > + } > + > + // no relevant directives present -- this means for CSP 1.0 that the load > + // should be permitted (and for the old CSP, to block it). > + return this._specCompliant; default allow for CSP 1.0 is dangerous, i'd rather see us take care of this via no directives becoming default-src * (maybe this is what you did in bug 764937, i'll take a look) and then fail closed here too. @@ +785,5 @@ > /** > * Intersects with another CSPRep, deciding the subset policy > * that should be enforced, and returning a new instance. > + * This assumes that either both CSPReps are specCompliant or they are both > + * not. that agrees with our earlier assumptions :) @@ +806,5 @@ > + let dirv = DIRS[dir]; > + let thisHasDir = this._directives.hasOwnProperty(dirv), > + thatHasDir = aCSPRep._directives.hasOwnProperty(dirv), > + thisHasDefault = this._directives.hasOwnProperty(DIRS.DEFAULT_SRC), > + thatHasDefault = aCSPRep._directives.hasOwnProperty(DIRS.DEFAULT_SRC); you only need to set thisHasDefault and thatHasDefault once, not every time per loop, no ? @@ +812,5 @@ > + > + // if both specific src directives are absent, skip this (new policy will > + // rely on default-src) > + if (!this._directives.hasOwnProperty(dirv) && > + !aCSPRep._directives.hasOwnProperty(dirv)) { why don't you use thisHasDir and thatHasDir here ? @@ +850,5 @@ > + this._directives[DIRS.DEFAULT_SRC]; > + var isect2 = thatHasDir ? > + aCSPRep._directives[dirv] : > + aCSPRep._directives[DIRS.DEFAULT_SRC]; > + newRep._directives[dirv] = isect1.intersectWith(isect2); you were right, this makes intersectWith a little narly, but (after some thought) this seems alright. there's a lot of CSP tests as well ;) ::: content/base/src/contentSecurityPolicy.js @@ +560,5 @@ > + > + // The policy might not explicitly declare each source directive (so > + // the cspContext may be implicit). If so, we have to report > + // violations as appropriate: specific or the default-src directive. > + if (this._policy._directives.hasOwnProperty(cspContext)) { i guess if cspContext already is 'default-src' (which it looks like it might be for a few different content types, see http://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#69 also #99-101) we go through this branch, which seems the same as below. ::: content/base/test/unit/test_csputils.js @@ +390,5 @@ > cspr = CSPRep.fromString("default-src *", URI("http://self.com:80")); > cspr_allow = CSPRep.fromString("allow *", URI("http://self.com:80")); > > + do_check_equivalent(cspr._directives['default-src'], > + cspr_allow._directives['default-src']); we test the 1.0 parser elsewhere in test_csputils.js : 511 // check default policy "default-src *" 512 cspr = CSPRep.fromStringSpecCompliant("default-src *", URI("http://self.com:80")); 513 // "DEFAULT_SRC directive is missing when specified in 514 // fromStringSpecCompliant" 515 do_check_has_key(cspr._directives, SD.DEFAULT_SRC); 516 517 for(var x in DEFAULTS) { 518 // each of these should be equivalent to DEFAULT_SRC 519 //DEFAULTS[x] + " does not use default rule." 520 do_check_true(cspr.permits("http://bar.com", DEFAULTS[x])); 521 } 522 }); so these changes seem good to me

(In reply to Ian Melven :imelven from comment #11) > > + if (!aCSPR._directives[SD.DEFAULT_SRC]) { > > allow is converted to default-src by the old parser, so this seems fine Yep. I relied on that. > > + return aCSPR; > > getting the default right here is bug 764937, which is ready to land with > this patch Yep. The current implementation has the wrong default, and bug 764937 can land on top of this to fix the default. > > + // no relevant directives present -- this means for CSP 1.0 that the load > > + // should be permitted (and for the old CSP, to block it). > > + return this._specCompliant; > > default allow for CSP 1.0 is dangerous, i'd rather see us take care of this > via no directives becoming default-src * (maybe this is what you did in bug > 764937, i'll take a look) and then fail closed here too. Perhaps I misunderstand what you're getting at here. We're removing make-explicit which means directives don't become anything. If there's no explicit directive for the content type and there's also no default-src directive, we have to fall back to *something*. My logic here is: 1. If there's a specific content-type directive, use it. 2. If not, but there's a default-src directive, use it. 3. There aren't relevant directives, so fail closed (pre-1.0) or open (1.0) 3 works because the first two steps guarantee there's no relevant directives at all. If default-src is "inferred" as "default-src *" when not present in standardized policies, this has the same effect, right? (Note: this codepath is only followed for resource loads, not inline scripts and such). > > + let thisHasDir = this._directives.hasOwnProperty(dirv), > > + thatHasDir = aCSPRep._directives.hasOwnProperty(dirv), > > + thisHasDefault = this._directives.hasOwnProperty(DIRS.DEFAULT_SRC), > > + thatHasDefault = aCSPRep._directives.hasOwnProperty(DIRS.DEFAULT_SRC); > > you only need to set thisHasDefault and thatHasDefault once, > not every time per loop, no ? Yeah, good catch. > why don't you use thisHasDir and thatHasDir here ? ::facepalm:: yeah. That was the point. Fixed. > you were right, this makes intersectWith a little narly, but (after some > thought) this seems alright. there's a lot of CSP tests as well ;) Yep, that'll go away when we implement support for N policies in parallel and don't need the intersect logic anymore. > i guess if cspContext already is 'default-src' (which it looks like it might > be for a few different content types, see > http://mxr.mozilla.org/mozilla-central/source/content/base/src/ > contentSecurityPolicy.js#69 also #99-101) we go through this branch, which > seems the same as below. Yeah, default-src is in the list of source directives. New patch coming shortly. Do you have open concerns about this patch? You didn't really write any questions in your review, so I'm not sure if you need any more info from me.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #12) > > > > + // no relevant directives present -- this means for CSP 1.0 that the load > > > + // should be permitted (and for the old CSP, to block it). > > > + return this._specCompliant; > > > > default allow for CSP 1.0 is dangerous, i'd rather see us take care of this > > via no directives becoming default-src * (maybe this is what you did in bug > > 764937, i'll take a look) and then fail closed here too. > > Perhaps I misunderstand what you're getting at here. We're removing > make-explicit which means directives don't become anything. If there's no > explicit directive for the content type and there's also no default-src > directive, we have to fall back to *something*. My logic here is: > > 1. If there's a specific content-type directive, use it. > 2. If not, but there's a default-src directive, use it. > 3. There aren't relevant directives, so fail closed (pre-1.0) or open (1.0) what i meant is, I'd rather see 'default-src *' being explicitly set as a present directive if no directives are actually present in the header, so #3 should never happen. > 3 works because the first two steps guarantee there's no relevant directives > at all. If default-src is "inferred" as "default-src *" when not present in > standardized policies, this has the same effect, right? yes, that's what I was getting at. I would rather see us infer a default-src * for no directives in bug 764937 and then case #3 above should be an error since it should never happen. Looking at your patch in that bug : + this._directives[SD.DEFAULT_SRC] + = CSPSourceList.fromString("*", this, this._self, true); + this._directives[SD.DEFAULT_SRC]._isImplicit = true; this is exactly what you do, so I think this case should be an error. > (Note: this codepath is only followed for resource loads, not inline scripts > and such). > New patch coming shortly. Do you have open concerns about this patch? You > didn't really write any questions in your review, so I'm not sure if you > need any more info from me. I mostly was making notes to show my work and keep track of what I had looked at (and for the possible edification of other folks working in the CSP code who may read the review notes :) ). random exposition : Dan made the point today that script protection should only be active if default-src or script-src is present, due to not wanting other directives like 'sandbox' and 'frame-options' to trigger the script protection. But the spec says that no directives AT ALL be treated as default-src * which would activate script protection. That seems fine to me. r+ with the change to error out if there's somehow no default-src directive since this should never happen. This means this needs to land at the same time as 764937 (which you were already planning).

(In reply to Ian Melven :imelven from comment #13) > what i meant is, I'd rather see 'default-src *' being explicitly set as a > present directive if no directives are actually present in the header, so #3 > should never happen. I don't agree. default-src isn't required to be present in the headers, even if there are other directives. So if we implicitly put it in there, we're keeping a part of makeExplicit alive and I want to kill it with fire because if we have to remember whether default-src was actually specified or not then reporting (etc) gets more complicated. Anyway, I was trying to stay pretty close to the spec: http://www.w3.org/TR/CSP/#parse-a-csp-policy Which doesn't say anything about adding "default-src *" when parsing a policy without that directive. Anywhere we divert from the processing model in the spec, we risk incompatibility. I'm specifically thinking about policies like "script-src 'self'". > yes, that's what I was getting at. I would rather see us infer a default-src > * for no directives in bug 764937 and then case #3 above should be an error > since it should never happen. How about instead, we put a gate in earlier in the function that makes absolute sure the load context maps to a SRC_DIRECTIVE. That way we can be guaranteed that by the time we get to that last return statement, permits() is only checking something that would be subject to the default-src policy? (frame-ancestors is shortcut earlier in permits(), we could put the other check right there and a comment about other SRC_DIRECTIVEs that are not subject to default-src). > Looking at your patch in that bug : > > + this._directives[SD.DEFAULT_SRC] > + = CSPSourceList.fromString("*", this, this._self, true); > + this._directives[SD.DEFAULT_SRC]._isImplicit = true; > > this is exactly what you do, so I think this case should be an error. Aw crap, that means bitrot in that other bug since this bug is getting rid of isImplicit entirely. > r+ with the change to error out if there's somehow no default-src directive I'm not convinced this is the right thing to do. What do we get by introducing an implicit directive? (Ignore that problem with the other bug that re-introduces _isImplicit. I should land that one, then land this one on top of it to remove _isImplicit).

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #14) > Aw crap, that means bitrot in that other bug since this bug is getting rid > of isImplicit entirely. No bitrot, I was wrong. That other bug assumes we haven't yet removed makeExplicit, so it relies on implicit directives. After we remove makeExplicit, there really shouldn't be any of those.

Added a gate to double-check that aContext is a source directive (and thus should fall-back to the "implied" default-src value if it's not present) and updated for review comments.

Comment on attachment 766148 [details] [diff] [review] proposed patch Review of attachment 766148 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/CSPUtils.jsm @@ +471,5 @@ > [dirname])); > > } // end directive: loop > > + ultra nit: extra blank line ? (may have been intentional) @@ +786,5 @@ > + // if this code runs, there's probably something calling permits() that > + // shouldn't be calling permits(). > + CSPdebug("permits called with invalid load type: " + aContext); > + return false; > + } excellent, thank you.

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb9825ad154f