1279139 - require-sri-for should block importScript

Status: RESOLVED FIXED

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

Description

[Abdulrahman Alqabandi]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.102 Safari/537.36

Steps to reproduce:

<?php
header("Content-Security-Policy: require-sri-for script style");
?>

<script>

x=new Worker(URL.createObjectURL(new Blob(['importScripts("https://html5sec.org/test.js")'])));

</script>


Actual results:

Looks like test.js was imported


Expected results:

blocked by 'require-sri-for script ' CSP

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

This one is tricky. At the moment SRI itself only applies to scripts/styles inserted through HTML elements and does not yet support importScripts (as far as I know). Hence, I would suppose that the CSP directive enforce-sri also does not apply to importScripts. But I might be wrong.

Francois, your opinion?

[Just as a note, workers usually don't inherit the CSP from the parent document/worker except if it's a globally unique identifier, like in this case (blob:).]

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Ah, potentially the script should be blocked, see discussion on github:
https://github.com/w3c/webappsec-subresource-integrity/pull/32#issuecomment-223490845

Comment 3

[Daniel Veditz [:dveditz]]

This is an incomplete implementation of an unfinished proposed standard -- we don't have to treat this as a hidden security bug.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Daniel Veditz [:dveditz] from comment #3)
> This is an incomplete implementation of an unfinished proposed standard --
> we don't have to treat this as a hidden security bug.

In fact, we should add a pref for require-sri-for. After chatting with Francois I filed Bug 1279420 to do so.

Comment 5

[François Marier [:francois]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Ah, potentially the script should be blocked, see discussion on github:
> https://github.com/w3c/webappsec-subresource-integrity/pull/32#issuecomment-
> 223490845

I missed that discussion but I will bring it up on the WebAppSec list. I'm not convinced that's going to be great for forward-compatibility.

Comment 6

[Frederik Braun [:freddy]]

Did you bring this up on the webappsec list? What was the outcome? I couldn't find the thread.

Comment 7

[François Marier [:francois]]

(In reply to Frederik Braun [:freddyb] from comment #6)
> Did you bring this up on the webappsec list? What was the outcome? I
> couldn't find the thread.

This dropped off of my radar. Feel free to start a thread on this. We should ensure that developers adopting SRI today will not have their sites break when we enhance SRI in a later version.

Comment 8

[Frederik Braun [:freddy]]

The spec text has changed for quite a while now. require-sri-for now mandates that we block on importScript in Workers and @import in CSS. See the note in https://w3c.github.io/webappsec-subresource-integrity/#apply-algorithm-to-request

Comment 9

[Frederik Braun [:freddy]]

Abdulrahman Alqabandi, thank you for reporting this.
I've been staring at the source with Christoph and I'll implement a check somewhere here later this or next week.

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#862

Comment 10

[François Marier [:francois]]

(In reply to Frederik Braun [:freddyb] from comment #8)
> The spec text has changed for quite a while now. require-sri-for now
> mandates that we block on importScript in Workers and @import in CSS. See
> the note in
> https://w3c.github.io/webappsec-subresource-integrity/#apply-algorithm-to-
> request

This particular spec change wasn't widely discussed. I personally suspect it will make require-sri unusable for developers. I'm not sure we should change our implementation just yet.

Comment 11

[François Marier [:francois]]

(In reply to François Marier [:francois] from comment #10)
> This particular spec change wasn't widely discussed. I personally suspect it
> will make require-sri unusable for developers. I'm not sure we should change
> our implementation just yet.

The discussion was on https://github.com/w3c/webappsec-subresource-integrity/pull/32#issuecomment-223490845 and the decision was to block everything right now. Even for sources of scripts which cannot currently be annotated with integrity metadata.

This means that this feature is more-or-less unusable (and should probably remain disabled by default) until SRI is expanded to cover these extra sources of scripts.

Comment 12

[Frederik Braun [:freddy]]

And the discussion is currently being continued in the w3c webappsec mailing list. The thread starts here: https://lists.w3.org/Archives/Public/public-webappsec/2016Sep/0027.html

Comment 13

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1279139-require-sri-for-needs-to-govern-scriptlo.patch

The Worker constructor and importScripts are using the same codepath. require-sri-for should govern both.
Here's a WIP patch with an tiny open question looking for feedback in ScriptLoader.cpp line 961.

Can you take a look, Christoph?

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8791071 [details] [diff] [review]
0001-Bug-1279139-require-sri-for-needs-to-govern-scriptlo.patch

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

Not sure if you also wanted feedback on the test, but it seems you forgot to add the *.js files to the patch.

::: dom/workers/ScriptLoader.cpp
@@ +946,4 @@
>        }
>      }
>  
> +    nsCOMPtr<nsILoadInfo> nsLoadInfo = channel->GetLoadInfo();

nit: rename the variable to loadInfo. Ah, too bad that the scriptLoadInfo is also just called loadInfo. Maybe chanLoadInfo? Or keep what you have :-)

@@ +946,5 @@
>        }
>      }
>  
> +    nsCOMPtr<nsILoadInfo> nsLoadInfo = channel->GetLoadInfo();
> +    if (nsLoadInfo->GetEnforceSRI()) {

potentially the loadInfo might be null, hence do
if (loadInfo && loadInfo->GetEnforceSRI()) {...

@@ +958,5 @@
> +      nsLoadInfo->LoadingPrincipal()->GetCsp(getter_AddRefs(csp));
> +      nsAutoCString violationURISpec;
> +      if (parentDoc) {
> +        //FIXME what to do if theres no parentdoc?
> +        parentDoc->GetDocumentURI()->GetAsciiSpec(violationURISpec);

Not sure why you would need the parentDoc in the first place? The URL you want to report is loadInfo.mUrl, and I see you are already using that, right?

But here is my question, what if we have a top level document with a CSP of require-sri. The document creates a worker with no CSP (workers can have their own CSP). I think in our current implementation the script will be blocked because we store that info in the loadInfo inherited from the parent, or am I wrong?
If so, loadInfo->LoadingPrincipal()->GetCSP() returns a nullptr CSP.

Comment 15

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1279139-require-sri-for-needs-to-govern-scriptlo.patch

You were absolutely right about the parentDoc. I have also included the missing test files.

If the document's policy says require-sri then the "new Worker()" constructor requires integrity metadata. but The Worker() constructor does not yet support integrity metadata in its options parameter. ScriptLoader:LoadScript will return without loading the script.

If, however, the document's policy allows worker (i.e., does not require sri) and the Worker comes with a CSP that does require SRI, then importScripts() will go into the same codepath and |nsLoadInfo->LoadingPrincipal()->GetCsp(getter_AddRefs(csp));| should give us the correct loading principal and thus the CSP of the worker.

I still need to figure out if this is the right way to cancel the load. The extra NetworkError in the devtools seems annoying at least.

Care to take a look? :)

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8791330 [details] [diff] [review]
0001-Bug-1279139-require-sri-for-needs-to-govern-scriptlo.patch

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

So, this looks good, but potentially the better place to put that check might be OnStreamCompleteInternal(). Also with regards to the later blocking once importScripts() becomes subject to require-sri and we really have to perform a check. I agree, at the moment it's probably better to block the load as early as we can, so we don't waste any resources for downloading the file that will never be executed anyway. Might be a good idea to consult bkelly to hear his thoughts were the best place to block the load is.

Also, I don't know what:
> The extra NetworkError in the devtools seems annoying at least.
means. What additional error? Maybe you can post the exact error. Maybe that also provides a hint where the actual check should be placed in the code.

::: dom/workers/ScriptLoader.cpp
@@ +954,5 @@
> +      //  where the actual bytes are available, i.e., OnStreamComplete.
> +      MOZ_LOG(SRILogHelper::GetSriLog(), mozilla::LogLevel::Debug,
> +            ("Scriptloader::Load, SRI required but not supported in workers"));
> +      nsCOMPtr<nsIContentSecurityPolicy> csp;
> +      chanLoadInfo->LoadingPrincipal()->GetCsp(getter_AddRefs(csp));

Even though the CSP potentially could be null here and it's good that you added the |if (csp) | check, I suppose it should never be null, right? Otherwise, how would the loadInfo the the info about the EnforceSRI bit, right? I think it might be wise to add
|MOZ_ASSERT(csp, "should have a csp for the worker here")"

@@ +959,5 @@
> +      if (csp) {
> +        csp->LogViolationDetails(
> +          nsIContentSecurityPolicy::VIOLATION_TYPE_REQUIRE_SRI_FOR_SCRIPT,
> +          loadInfo.mURL,
> +          EmptyString(), 0, EmptyString(), EmptyString());

nit, bring that line up right next to |loadInfo.mURL|

@@ +962,5 @@
> +          loadInfo.mURL,
> +          EmptyString(), 0, EmptyString(), EmptyString());
> +        }
> +      rv = NS_ERROR_SRI_CORRUPT;
> +      return rv;

nit: no need to assign to rv, just do |return NS_ERROR_SRI_CORRUPT;

Comment 17

[Frederik Braun [:freddy]]

Comment on attachment 8791330 [details] [diff] [review]
0001-Bug-1279139-require-sri-for-needs-to-govern-scriptlo.patch

Hey Ben, can you take a look at the patch?
ckerschb mentioned in his feedback (previous comment) that I should ask you where to place the check for now.
(I'll start working on a patch that does the smae in OnStreamCompleteInternal in prallel)

Comment 18

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1279139-require-sri-for-onstreamcomplete.patch

This is the other version, blocking in "OnStreamCompleteInternal".
Please provide feedback / review.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8792411 [details] [diff] [review]
0001-Bug-1279139-require-sri-for-onstreamcomplete.patch

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

Looks good, but let's wait for bkelly's feedback.

::: dom/workers/ScriptLoader.cpp
@@ +1121,5 @@
> +            ("Scriptloader::Load, SRI required but not supported in workers"));
> +      nsCOMPtr<nsIContentSecurityPolicy> wcsp;
> +      chanLoadInfo->LoadingPrincipal()->GetCsp(getter_AddRefs(wcsp));
> +      MOZ_ASSERT(wcsp, "We sould have a CSP for the worker here");
> +      wcsp->LogViolationDetails(

Having MOZ_ASSERT is great but please also surround Logviolationdetails with |if (wcsp) {| to make sure we are not crashing here in a release version.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8791330 [details] [diff] [review]
0001-Bug-1279139-require-sri-for-needs-to-govern-scriptlo.patch

I guess I would prefer making this look as similar as possible to the final case where we actually implement integrity checking.  So I would recommend the OnStreamComplete version.

That being said, is there a reason not to just implement the integrity checking here?  Is there a follow-up bug for that and what is the main blocking issue there?  It seems like we should have all of the right bits available since we do integrity checking on main thread scripts.

Comment 21

[Frederik Braun [:freddy]]

There is no spec for integrity checking.
The Worker constructor and importScripts() do not accept any of the parameters.
There might be support for that in the future, but it's unknown at this point.
I'll update our implementation as the spec evolves.

Comment 22

[Frederik Braun [:freddy]]

Comment on attachment 8792411 [details] [diff] [review]
0001-Bug-1279139-require-sri-for-onstreamcomplete.patch

Please review.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8792411 [details] [diff] [review]
0001-Bug-1279139-require-sri-for-onstreamcomplete.patch

Andrea, can you take this?  I'm short on time for personal reasons for the next week or two.

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8792411 [details] [diff] [review]
0001-Bug-1279139-require-sri-for-onstreamcomplete.patch

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

::: dom/security/test/sri/iframe_require-sri-for_main.html
@@ +31,3 @@
>    window.onload = function() {
> +    if (typeof w == "object") {
> +      parent.postMessage("finish", '*');

use w.onerror instead.

::: dom/workers/ScriptLoader.cpp
@@ +1233,5 @@
>            rv = csp->GetReferrerPolicy(&rp, &hasReferrerPolicy);
>            NS_ENSURE_SUCCESS(rv, rv);
>  
> +
> +          if (hasReferrerPolicy) { //XXX file follow-up refpolicy moving out of CSP, no?

have you filed a follow up? Add here the ID.

Comment 25

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1279139-require-sri-for-needs-to-govern-scriptlo.patch

carrying over r+ from baku.
I removed the comment about the follow-up bug, since it's already closed as invalid (bug 1307366 for the record)

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24953f3dbcb1
require-sri-for needs to govern scriptloading for workers. r=baku

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/24953f3dbcb1