898190 - csp test uses invalid policy values (test failure from bug 888172 on beta 23)

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[Sid Stamm [:geekboy or :sstamm]]

When we uplifted bug 888172 to beta 23 its tests failed.  That's because of a bad CSP policy string that should never have worked in the first case.  For some reason, the test only fails in 23, not 24 or 25.

Test that fails:
content/base/test/test_CSP_bug888172.html

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fix

Comment 2

[Garrett Robinson [:grobinson]]

Comment on attachment 781301 [details] [diff] [review]
fix

Review of attachment 781301 [details] [diff] [review]:
-----------------------------------------------------------------

That should do it. Still don't know how this ever worked.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

verified the test still passes locally and pushed to central:
https://hg.mozilla.org/mozilla-central/rev/76fb8f815ac7

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 781301 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): tests for bug 888172 fail on beta 23
User impact if declined: none
Testing completed (on m-c, etc.): applied to local beta and passes, landed on m-c 
Risk to taking this patch (and alternatives if risky): zero risk, fixes a test (big win)
String or IDL/UUID changes made by this patch:

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fedcc7eb515b
https://hg.mozilla.org/releases/mozilla-beta/rev/7499e60e458b

Comment 6

[Ryan VanderMeulen [:RyanVM]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #0)
> For some reason, the test only fails in 23, not 24 or 25.

I really hope somebody's looking into what that reason is.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Yep, Garrett and I are checking it out.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Turns out that the test for bug 888172 exposed a sleeping bug in our CSP 1.0 parser implementation that the patch for bug 780978 fixed.  

http://mxr.mozilla.org/mozilla-beta/source/content/base/src/CSPUtils.jsm#893

defaultSrcDir.clone() throws when defaultSrcDir is undefined -- this leads to the CSP being rejected and reverted to "default-src *" which blocks inline scripts.  The fix is to put in a null check and create a defaultSrcDir if one is not present.

Since we uplifted a bunch of stuff but not bug 780978, the bug surfaced on beta 23 and not other builds.  Earlier builds were not affected and later ones were fixed by bug 780978.  This only surfaces with non-prefixed CSP headers, and only happens in 23 (not 22/earlier or 24/later). Since the bug itself has already been fixed in 24 and adoption of the standardized header is currently low, I think we should just let this one go.

Comment 9

[u279076]

Assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.