Closed Bug 1298505 Opened 3 years ago Closed 3 years ago

CSP: Use NS_SecurityCompareURIs instead of CheckMayLoad

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox49 --- verified
firefox50 --- verified
firefox51 --- verified

People

(Reporter: dveditz, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file)

Looking at StripURIForReporting in CSP code it calls CheckMayLoad() comparing the document principal to the about-to-be-reported URL. CheckMayLoad() should allow every same-scheme URL so I don't think we're stripping anything. Do we have reporting tests? Maybe not, hard to set up a server for that.

The correct thing would be to call Equals (or maybe Subsumes) on the principal, or just cut to the chase and call NS_SecurityCompareURIs(<url>, mSelfURI) directly.
Assignee: nobody → ckerschb
Blocks: 1291967
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]
Hey Dan,

that is a very good fix, thanks for the hint. We do have test coverage for that actually and it worked before and it works with the new patch correctly, but we rely on mSelfURI rather than on mLoadingPrincipal. The test is: dom/security/test/unit/test_csp_reports.js and the exact subtest is:

> aURI: http://blocked.test/foo.js"
> aSelfURI: http://localhost:36602/foo/self"

which then strips 'foo.js' from the uri.

I suppose we should uplift this patch to aurora and beta.
Attachment #8785551 - Flags: review?(dveditz)
Within test_csp_reports.js we also have another test, which tests: ftp://blocked.test
Summary: CSP reports may never strip details from cross-origin urls → CSP: Use NS_SecurityCompareURIs instead of CheckMayLoad
Comment on attachment 8785551 [details] [diff] [review]
bug_1298505_csp_update_stripuriforreporting.patch

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

r=dveditz, although this change isn't strictly necessary (I was confusing CheckMayLoad() with CheckLoadURI()).
Attachment #8785551 - Flags: review?(dveditz) → review+
(this change does make it easier to fix bug 1291967 because then you don't need the mLoadingPrincipal to stick around.)
(In reply to Daniel Veditz [:dveditz] from comment #4)
> (this change does make it easier to fix bug 1291967 because then you don't
> need the mLoadingPrincipal to stick around.)

In the end I suppose we could mark bug 1291967 as a dup of this one. Anyway, we should land and uplift.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f17ae97cb65e
CSP: Update StripURIForReporting to rely on NS_SecurityCompareURIs. r=dveditz
Keywords: checkin-needed
Comment on attachment 8785551 [details] [diff] [review]
bug_1298505_csp_update_stripuriforreporting.patch

Approval Request Comment
The changes within this bug fix Bug 1291967 which was a regression and caused a crash.

This change makes StripSelfURIForReporting rely mSelfURI instead of mLoadingPrincipal and hence doesn't cause the null deref of mLoadingPrincipal.

[Feature/regressing bug #]:
The bug that made the crash appear was Bug 1208946. But, the crash is not strictly tied to Bug 1208946. There are more components involved. The crash only happens on e10s and only if the page uses CSP and in addition makes use of the CSP directive 'report-uri'.

[User impact if declined]:
Pages that ship a CSP including 'report-uri' and are visited by Firefox in e10s mode exprience a browser crash.

[Describe test coverage new/current, TreeHerder]:
* Manual verification;
* The crash itself is hard to reproduce, but we have the mochitest test_csp_reports.js to make sure the feature did not regress.

[Risks and why]:
Low, because it only affects pages that use a CSP.

[String/UUID change made/needed]:
no
Attachment #8785551 - Flags: approval-mozilla-beta?
Attachment #8785551 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f17ae97cb65e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1291967
Comment on attachment 8785551 [details] [diff] [review]
bug_1298505_csp_update_stripuriforreporting.patch

Fix for a crash first reported in 49. I'm not clear when the regression happened. Andrei, can your team help to verify this in beta from this bug and the duplicate, once the fix lands? Thanks.
Flags: needinfo?(andrei.vaida)
Attachment #8785551 - Flags: approval-mozilla-beta?
Attachment #8785551 - Flags: approval-mozilla-beta+
Attachment #8785551 - Flags: approval-mozilla-aurora?
Attachment #8785551 - Flags: approval-mozilla-aurora+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10)
> Comment on attachment 8785551 [details] [diff] [review]
> bug_1298505_csp_update_stripuriforreporting.patch
> 
> Fix for a crash first reported in 49. I'm not clear when the regression
> happened. Andrei, can your team help to verify this in beta from this bug
> and the duplicate, once the fix lands? Thanks.

Added to our 49.0b9 sign off. We'll follow up with results as soon as possible, leaving the ni? in place as a reminder.
Flags: qe-verify+
This bug has no steps to reproduce, moreover I'm not able to reproduce the duplicate bug 1291967 on Nightlies 2016-08-08 and 2016-08-25 (before the fix), Win 7 x64.
The reporter already verified bug 1291967 in https://bugzilla.mozilla.org/show_bug.cgi?id=1291967#c25.
Also, there are no crashes in Socorro in the last month on builds newer than 2016-09-01.
Considering this, I think we're safe to mark this bug as VERIFIED.
Status: RESOLVED → VERIFIED
Flags: needinfo?(andrei.vaida)
You need to log in before you can comment on or make changes to this bug.