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

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[François Marier [:francois]]

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.

Comment 2

[François Marier [:francois]]

Attachment: Manual test case

Comment 3

[Frederik Braun [:freddy]]

(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.

Comment 4

[François Marier [:francois]]

(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)."

Comment 5

[François Marier [:francois]]

Attachment: Manual test case (scripts and styles)

Comment 6

[François Marier [:francois]]

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

Comment 7

[François Marier [:francois]]

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

Comment 8

[François Marier [:francois]]

Attachment: Manual test case (scripts and styles)

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.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 10

[François Marier [:francois]]

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.

Comment 11

[Pulsebot]

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

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[François Marier [:francois]]

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

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is needed for a new feature.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/228866eb07ab