Closed Bug 1208629 Opened 4 years ago Closed 4 years ago

Properly support data: and blob: URIs with an integrity atribute

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- unaffected
firefox43 + fixed
firefox44 + fixed

People

(Reporter: francois, Assigned: francois)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

data: URIs are always opaque for <script> and <link> elements, so integrity checks should always fail (i.e. they aren't eligible for SRI).

blob: URIs on the other hand are considered same-origin so we should do integrity checks on them.
Duplicate of this bug: 1208875
Attached file Manual test case (obsolete) —
(In reply to François Marier [:francois] from comment #0)
> blob: URIs on the other hand are considered same-origin so we should do
> integrity checks on them.

Strictly speaking, that's not true. I.e., one could send around blob values through postMessage from originA to originB and they shouldn't be eligible. Blobs carry their own origin and this has to be checked explicitly.
(In reply to Frederik Braun [:freddyb] from comment #3)
> (In reply to François Marier [:francois] from comment #0)
> > blob: URIs on the other hand are considered same-origin so we should do
> > integrity checks on them.
> 
> Strictly speaking, that's not true. I.e., one could send around blob values
> through postMessage from originA to originB and they shouldn't be eligible.
> Blobs carry their own origin and this has to be checked explicitly.

Can you come up with a test case that illustrates this?

I'm not very familiar with blobs and my comment above was based on Anne's observation: "For blob URLs it would just work, since they're considered same-origin (unless you played around with document.domain)."
Attached file Manual test case (scripts and styles) (obsolete) —
Attachment #8666558 - Attachment is obsolete: true
Bug 1208629 - Properly support data: and blob: URIs with an integrity atribute. r?ckerschb
Attachment #8667013 - Flags: review?(mozilla)
Comment on attachment 8667013 [details]
MozReview Request: Bug 1208629 - Properly support data: and blob: URIs with an integrity atribute. r?ckerschb

Bug 1208629 - Properly support data: and blob: URIs with an integrity atribute. r?ckerschb
The latest version of my test case includes a corner case in data: URIs (CORS-enabled) that should be blocked but currently isn't neither in Chrome nor Firefox.
Attachment #8666936 - Attachment is obsolete: true
Comment on attachment 8667013 [details]
MozReview Request: Bug 1208629 - Properly support data: and blob: URIs with an integrity atribute. r?ckerschb

https://reviewboard.mozilla.org/r/20659/#review19075

Thanks Francois, looks good, just some nits!

::: dom/security/SRICheck.cpp:314
(Diff revision 2)
> +  }

Suggestion: You have overloaded Verifyintegrity, so what if you only log within VerifyIntegrityInternal? That would save you the duplicate code for logging.

::: dom/security/SRICheck.cpp:333
(Diff revision 2)
> +  NS_ENSURE_ARG_POINTER(request);

If you only log within VerifyIntegrityInternal than you can remove that NS_ENSURE_ARG_POINTER because passing a nullptr to do_QueryInterface doesn't do any harm and then just pass the channel to verifyIntegrityInternal.

::: dom/security/test/sri/iframe_style_sameorigin.html:111
(Diff revision 2)
> +

Not a big deal but that is the same execution context so you could save some duplicate code here by just having blob1 and blob2 create a link, set the integrity attribute on the link and append it and then overwrite the intrigty and append it again. But as I said, up to you.
Attachment #8667013 - Flags: review?(mozilla) → review+
Thanks for the review Christoph. As always, I appreciate your attention to details!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> Suggestion: You have overloaded Verifyintegrity, so what if you only log
> within VerifyIntegrityInternal? That would save you the duplicate code for
> logging.

That's what I started with but it became annoying because we're actually logging different information in the two cases. One shows the name from the request and the other extracts the original URI from the channel.
https://hg.mozilla.org/mozilla-central/rev/fbd07ab0e098
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8667013 [details]
MozReview Request: Bug 1208629 - Properly support data: and blob: URIs with an integrity atribute. r?ckerschb

Approval Request Comment
[Feature/regressing bug #]: subresource integrity (introduced in 43)
[User impact if declined]: developer impact = having to work-around non-compliant implentation, user impact = may cause a browser crash
[Describe test coverage new/current, TreeHerder]: added new mochitests to cover all new use cases
[Risks and why]: low since there is good test coverage
[String/UUID change made/needed]: none
Attachment #8667013 - Flags: approval-mozilla-aurora?
Depends on: 1212979
Comment on attachment 8667013 [details]
MozReview Request: Bug 1208629 - Properly support data: and blob: URIs with an integrity atribute. r?ckerschb

Approved for uplift to aurora; includes new tests, fixes a regression
Attachment #8667013 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking since this is needed for a new feature.
You need to log in before you can comment on or make changes to this bug.