1198078 - importScripts in ServiceWorkers ignores mixed content restriction

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Muneaki Nishimura]

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

Steps to reproduce:

Mixed content blocking doesn't work in importScripts() of ServiceWorkers.

Steps to Reproduce:
1. Set devtools.webconsole.filter.serviceworkers=true to see worker's log by console.
2. Access to https://alice.csrf.jp/sw/importscripts/
3. Reload again and see the console and then ServiceWorker loads an external javascript from bob.csrf.jp via http.



Actual results:

If the message "A tainted javascript is loaded from http://bob.csrf.jp/." is shown on the console then mixed content restriction is ignored and the script is loaded from http: site.



Expected results:

Mixed contents restriction should be applied to importScripts on ServiceWorkers as with Web Workers. Note that the latest Chrome does.

Comment 1

[Daniel Veditz [:dveditz]]

1) why isn't the mixed-content blocker catching this? Does that mean service worker imports aren't checking content policies at all?
2) even apart from the mixed-content blocker the SW spec says not to do this.

Comment 2

[Tanvi Vyas[:tanvi]]

I set a breakpoint in http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#509 and loaded the page.  I printed content types and always got "3" for TYPE_IMAGE.  So this doesn't look like it goes through a content policy check.  It would be good to contact someone familiar with the service worker code that can help identify what code path the script goes through.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

The code is in dom/workers/ScriptLoader.cpp.  I don't see nsMixedContextBlocker in there:

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

Is there another way to call the content blocker?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Hmm, is it because we don't enter this block without a parent document?

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp?case=true&from=ScriptLoader.cpp#126

Kyle, what do you think?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

We're intending to ship service workers for push in 42, so we should uplift anything we do here.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Ben Kelly [:bkelly] from comment #4)
> Hmm, is it because we don't enter this block without a parent document?
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.
> cpp?case=true&from=ScriptLoader.cpp#126
> 
> Kyle, what do you think?

Yeah ... that's bogus.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should test SharedWorkers and sub-worker too ...

Comment 8

[Tanvi Vyas[:tanvi]]

Who should this bug be assigned to?

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> (In reply to Ben Kelly [:bkelly] from comment #4)
> > Hmm, is it because we don't enter this block without a parent document?
> > 
> >  
> > https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.
> > cpp?case=true&from=ScriptLoader.cpp#126
> > 
> > Kyle, what do you think?
> 
> Yeah ... that's bogus.

I agree that that's bogus, however, if we don't pass a parentDoc (or any other loadingNode) to NS_CheckContentPolicy() then mixed content blocker will *always* block the load because it cannot query the docshell and bails out not allowing the load [1].

Ben, what would be the alternative here?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#624

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Why don't we just always block mixed content in (service?) workers?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> Why don't we just always block mixed content in (service?) workers?

That's certainly a possibility, but it would be a bit of hack somewhere above [1] because as I mentioned above, we can not rely on nsMixedContentBlocker itself. So we would also have to set up the console logging somewhere around here, but certainly a possibility.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp?case=true&from=ScriptLoader.cpp#126

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

I am not a 100% sure but i think we should be able to rely on aContentPolicyType to be TYPE_INTERNAL_WORKER or TYPE_INTERNAL_SHARED_WORKER [1] to identify those cases and block mixed content in such cases.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIContentPolicyBase.idl#200

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> Why don't we just always block mixed content in (service?) workers?

This is essentially what chrome does for service worker:

  https://code.google.com/p/chromium/issues/detail?id=392409#c48

It was also discussed in this spec issue:

  https://github.com/slightlyoff/ServiceWorker/issues/119

Although I can't decipher what the final spec outcome was.

Comment 14

[Tanvi Vyas[:tanvi]]

The option to override protection from mixed content has been hidden into control center and telemetry shows it is not clicked often.  So it probably won't be a huge usability issue to just block mixed content service worker requests by default.  If we have specific content types we just always want to block (i.e mixed service workers) we can detect them and deny the load before querying the docshell here:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#624

At this point, we know the requested content is not secure and we know its parent is HTTPS.  We can check for service worker specific content types and block with a message to the webconsole that insecure content attempted to load and the browser blocked it.

If a service worker requested a script/image/etc, would we be able to set TYPE_SERVICEWORKER_SUBREQUEST (like we do for plugins with TYPE_OBJECT_SUBREQUEST)?  Note this means mixed display and mixed active will be blocked.

However, it since this is in the service worker spec per comment 1, it may be better to do this in the service worker code directly.  For example, websocket requests cannot be mixed so we skip them entirely and let the websocket code take care of that:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#448

Comment 15

[Muneaki Nishimura]

As mentioned in Comment 14 WebSockets on Service Workers also ignores mixed contents restriction.
The following URL is a PoC that tries to connect to ws://mallory.csrf.jp:8888 from SW.
https://alice.csrf.jp/sw/websocket/
Firefox Nightly allows to connect but Chrome 44 raises an error.

Comment 16

[Anne (:annevk)]

So the situation from the standards perspective is that mixed content is blocked for workers, shared workers, and service workers alike. Service workers can get an exception if the request they make is associated with a window (such that mixed content UI can be displayed to the user), but we don't have to support that for v1 I think and maybe if we don't get requests for it we should just not implement it.

So not passing "parentDoc" is indeed what the standards say, except for cases in a service worker where you get a request from a window and you directly make that request without modification. But I don't think we should worry about that for now as it would actually be better if we never needed to implement that.

Does that make sense?

Comment 17

[Tanvi Vyas[:tanvi]]

Yes, so for now Mixed Content should just be blocked entirely without an unblock option.  Now the question is where the blocking should be handled - in Service Workers directly or in nsMixedContentBlocker.

If in nsMixedContentBlocker, when we don't get a docshell, we need some way to know the request is from a serviceworker (ie content type) and we could handle that situation accordingly by returning a REJECT_REQUEST decision and posting to the webconsole.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So I tried adding a TYPE_INTERNAL_SERVICE_WORKER, but nsMixedContentBlocker gets called with external types rather than internal types, so it can't actually differentiate Service Workers here ...

How do we fix that?

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #18)
> So I tried adding a TYPE_INTERNAL_SERVICE_WORKER, but nsMixedContentBlocker
> gets called with external types rather than internal types, so it can't
> actually differentiate Service Workers here ...

The problem is that content policies (because they are heavily used by addons) should only see external content policy types and not Firefox internal types. Hence we convert internal types to external types before calling content policies [1]. For Bug 1048048, I am letting some internal types through for our own CSP policy [2], to give CSP the opportunity to distinguish between preloads and regular loads. 

> How do we fix that?

Obviously one solution would be to pass some internal types to our own mixed content policy, but I think at some point this solution will get really messy. In my opinion the better option (for that specific case) is to let the ScriptLoader handle mixed content blocking. Do we really want to call all content policies if we parentDoc is nullptr anyway? Or just mixed content blocking?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentPolicy.cpp#118
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8644645&action=diff

Comment 20

[Anne (:annevk)]

What are content policies? If that includes CSP, workers can have their own CSP so that should still work.

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Anne (:annevk) from comment #20)
> What are content policies? If that includes CSP, workers can have their own
> CSP so that should still work.

Content policy is our generic content blocking infrastructure that's used by mixed content blocking, CSP, and addons like Adblock Plus.

Comment 22

[Tanvi Vyas[:tanvi]]

We have 6 internal content policies - https://etherpad.mozilla.org/ContentPolicies.

Do these loads need to be exposed to addons (i.e. do we need to give them a chance to reject them)?

Comment 23

[(no longer active)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #19)
> > How do we fix that?
> 
> Obviously one solution would be to pass some internal types to our own mixed
> content policy, but I think at some point this solution will get really
> messy. In my opinion the better option (for that specific case) is to let
> the ScriptLoader handle mixed content blocking. Do we really want to call
> all content policies if we parentDoc is nullptr anyway? Or just mixed
> content blocking?

I actually think that the solution of exposing the necessary internal types to the mixed content blocker is fine here, similar to the CSP case.  The script loader needs to talk to the generic content policy infrastructure anyway, so we shouldn't make it handle mixed content blocking separately.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #23)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #19)
> > > How do we fix that?
> > 
> > Obviously one solution would be to pass some internal types to our own mixed
> > content policy, but I think at some point this solution will get really
> > messy. In my opinion the better option (for that specific case) is to let
> > the ScriptLoader handle mixed content blocking. Do we really want to call
> > all content policies if we parentDoc is nullptr anyway? Or just mixed
> > content blocking?
> 
> I actually think that the solution of exposing the necessary internal types
> to the mixed content blocker is fine here, similar to the CSP case.  The
> script loader needs to talk to the generic content policy infrastructure
> anyway, so we shouldn't make it handle mixed content blocking separately.

+1

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: 0009-mixed-content-sw.patch

This is what I have so far, but taking it further requires exposing the internal load types to nsMixedContentBlocker.

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Want to file a new bug on the content-policy infrastructure?

Comment 27

[Tanvi Vyas[:tanvi]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #26)
> Want to file a new bug on the content-policy infrastructure?

Hey Kyle,
Why do we need a separate bug?

It looks like to take this further we need to add something similar to https://bugzilla.mozilla.org/attachment.cgi?id=8644645&action=diff#a/dom/base/nsContentPolicy.cpp_sec3 that checks isMixedContentBlocker and then if it is send externalTypeOrServiceWorker.

Then in nsMixedContentBlocker.cpp here[1], we check if TYPE_INTERNAL_SERVICE_WORKER and set the decision to reject and return.

[1]http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#623

Comment 28

[(no longer active)]

Stealing.

Comment 29

[(no longer active)]

Attachment: Part 1: Do not allow loading mixed content scripts in workers

Comment 30

[(no longer active)]

Attachment: Part 2: Add tests for mixed content blocking of scripts in workers

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8660185 [details] [diff] [review]
Part 1: Do not allow loading mixed content scripts in workers

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

Thanks Ehsan, the code in general looks fine, but please answer my 2 questions.

::: dom/security/nsMixedContentBlocker.cpp
@@ +650,5 @@
> +
> +  // Disallow mixed content loads for workers, shared workers and service
> +  // workers.
> +  if (isWorkerType && parentIsHttps && isHttpScheme) {
> +    *aDecision = REJECT_REQUEST;

Two questions:
1) What happens/(should happen) if the CSP directive 'upgrade-insecure-requests' is set? At the moment, this patch would block such a request, because we can not query the docshell because 'aParent' was null on the callsite. I assume we are fine with that for shared workers, right? I am fine with that but wanna make sure we are not missing this case, what do you think?

2) Can shared workers use any other (potentially custom) scheme than https/http? If not, then we are fine with this patch, otherwise we should check if the scheme is '!isHttp*s*Scheme)' rather than isHttpScheme.

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #31)
> Comment on attachment 8660185 [details] [diff] [review]
> Part 1: Do not allow loading mixed content scripts in workers
> 
> Review of attachment 8660185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Ehsan, the code in general looks fine, but please answer my 2
> questions.
> 
> ::: dom/security/nsMixedContentBlocker.cpp
> @@ +650,5 @@
> > +
> > +  // Disallow mixed content loads for workers, shared workers and service
> > +  // workers.
> > +  if (isWorkerType && parentIsHttps && isHttpScheme) {
> > +    *aDecision = REJECT_REQUEST;
> 
> Two questions:
> 1) What happens/(should happen) if the CSP directive
> 'upgrade-insecure-requests' is set? At the moment, this patch would block
> such a request, because we can not query the docshell because 'aParent' was
> null on the callsite. I assume we are fine with that for shared workers,
> right? I am fine with that but wanna make sure we are not missing this case,
> what do you think?
> 
> 2) Can shared workers use any other (potentially custom) scheme than
> https/http? If not, then we are fine with this patch, otherwise we should
> check if the scheme is '!isHttp*s*Scheme)' rather than isHttpScheme.

Comment 33

[(no longer active)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #31)
> ::: dom/security/nsMixedContentBlocker.cpp
> @@ +650,5 @@
> > +
> > +  // Disallow mixed content loads for workers, shared workers and service
> > +  // workers.
> > +  if (isWorkerType && parentIsHttps && isHttpScheme) {
> > +    *aDecision = REJECT_REQUEST;
> 
> Two questions:
> 1) What happens/(should happen) if the CSP directive
> 'upgrade-insecure-requests' is set? At the moment, this patch would block
> such a request, because we can not query the docshell because 'aParent' was
> null on the callsite. I assume we are fine with that for shared workers,
> right? I am fine with that but wanna make sure we are not missing this case,
> what do you think?

Yes, the request will be blocked and not upgraded.  According to comment 16, that is the behavior that we should have.

> 2) Can shared workers use any other (potentially custom) scheme than
> https/http? If not, then we are fine with this patch, otherwise we should
> check if the scheme is '!isHttp*s*Scheme)' rather than isHttpScheme.

Hmm, good point!  I will make that change.

Comment 34

[(no longer active)]

Attachment: Part 1: Do not allow loading mixed content scripts in workers

Comment 35

[(no longer active)]

Attachment: Part 2: Add tests for mixed content blocking of scripts in workers

I made two changes to the tests:

* Disabled mixed content blocking by default to ensure that they will fail without the code changes.
* Disabled them on b2g where we cannot load https content in tests.

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8660378 [details] [diff] [review]
Part 1: Do not allow loading mixed content scripts in workers

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

Thanks Ehsan.

::: dom/base/nsContentUtils.cpp
@@ +8007,5 @@
>  
> +/* static */
> +nsContentPolicyType
> +nsContentUtils::InternalContentPolicyTypeToExternalOrScript(nsContentPolicyType aType)
> +{

It doesn't really make a difference, but why have it also return TYPE_INTERNAL_SCRIPT and not make it ::InternalContentPolicyTypeToExternalOrWorker() and only return the internal worker types?

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8660379 [details] [diff] [review]
Part 2: Add tests for mixed content blocking of scripts in workers

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

Thanks Ehsan!

::: dom/workers/test/serviceworkers/mochitest.ini
@@ +253,5 @@
>  [test_fetch_event_client_postmessage.html]
>  [test_escapedSlashes.html]
>  [test_eventsource_intercept.html]
>  [test_not_intercept_plugin.html]
> +[test_importscript_mixedcontent.html]

Are you not facing the *https* problem on b2g here as well?

::: dom/workers/test/test_importScripts_mixedcontent.html
@@ +13,5 @@
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1198078">DOM Worker Threads Bug 1198078</a>
> +<iframe></iframe>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +  

nit: trailing whitespace.

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Before you land these patches give Tanvi a chance to look them over though.

Comment 39

[(no longer active)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #36)
> ::: dom/base/nsContentUtils.cpp
> @@ +8007,5 @@
> >  
> > +/* static */
> > +nsContentPolicyType
> > +nsContentUtils::InternalContentPolicyTypeToExternalOrScript(nsContentPolicyType aType)
> > +{
> 
> It doesn't really make a difference, but why have it also return
> TYPE_INTERNAL_SCRIPT and not make it
> ::InternalContentPolicyTypeToExternalOrWorker() and only return the internal
> worker types?

I find that name to be misleading, since it may cause people to use it for other different worker related stuff in the future.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #37)
> ::: dom/workers/test/serviceworkers/mochitest.ini
> @@ +253,5 @@
> >  [test_fetch_event_client_postmessage.html]
> >  [test_escapedSlashes.html]
> >  [test_eventsource_intercept.html]
> >  [test_not_intercept_plugin.html]
> > +[test_importscript_mixedcontent.html]
> 
> Are you not facing the *https* problem on b2g here as well?

These tests are disabled whole-sale on b2g!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #38)
> Before you land these patches give Tanvi a chance to look them over though.

Sure.  I'll request sec-approval in the mean time to expedite...

Comment 40

[(no longer active)]

Comment on attachment 8660378 [details] [diff] [review]
Part 1: Do not allow loading mixed content scripts in workers

[Security approval request comment]
How easily could an exploit be constructed based on the patch? The hunks in nsMixedContentBlocker.cpp are pretty revealing for someone who is familiar with this code, but this patch is also big and full of piping cruft, so I don't think the attention will necessarily be focused there.  I should also make up a better commit message...

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? See above.  I'm not planning to land the tests before the fix is shipped on the release channel.

Which older supported branches are affected by this flaw? All with dedicated/shared workers, as far as I can tell.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The backport should be reasonably straightforward, I would expect.

How likely is this patch to cause regressions; how much testing does it need? The effect of the patch is pretty well understood, and I have tested it on the try server, so hopefully the risk should not be too high.  That being said, I am not comfortable to nominate it for beta and ESR yet.

Approval Request Comment
[Feature/regressing bug #]: I think we have had this bug for a while.
[User impact if declined]: See comment 0.
[Describe test coverage new/current, TreeHerder]: Has a test and has been tested on the try server.
[Risks and why]: See above.
[String/UUID change made/needed]: None.

Comment 41

[Tanvi Vyas[:tanvi]]

I will review these today.

Comment 42

[Tanvi Vyas[:tanvi]]

Comment on attachment 8660378 [details] [diff] [review]
Part 1: Do not allow loading mixed content scripts in workers

Hi Ehsan,

Thank you for picking up this bug!  See comments below.

dom/base/nsContentUtils.h
+   * Map internal content policy types to external ones or script types:
+   *   * TYPE_INTERNAL_SCRIPT
+   *   * TYPE_INTERNAL_WORKER
+   *   * TYPE_INTERNAL_SHARED_WORKER
+   *   * TYPE_INTERNAL_SERVICE_WORKER
+   *
+   *
+   * Note: DO NOT call this function unless if you know what you're doing!
+   */
+  static nsContentPolicyType InternalContentPolicyTypeToExternalOrScript(nsContentPolicyType aType);
+

s/unless if you know/unless you know/


dom/security/nsMixedContentBlocker.cpp
+  // The content policy type that we receive may be an internal type for
+  // scripts.  

The type is internal when the request for such content is made by workers.  The content loaded itself may be a script, but may also be an image.  So perhaps this comment should say:
The content policy type that we receive may be an internal type for requests initiated by workers.


Same here:
dom/base/nsIContentPolicyBase.idl
+  /**
+   * Indicates an internal constant for scripts loaded through a service
+   * worker.
s/scripts/content

+   *
+   * This will be mapped to TYPE_SCRIPT before being passed to content policy
+   * implementations.
+   */
+  const nsContentPolicyType TYPE_INTERNAL_SERVICE_WORKER = 35;
+


dom/security/nsMixedContentBlocker.cpp
-  bool isHttpScheme = false;
-  rv = aContentLocation->SchemeIs("http", &isHttpScheme);
+  bool isHttpsScheme = false;
+  rv = aContentLocation->SchemeIs("https", &isHttpsScheme);

!isHttpsScheme != isHttpScheme.  We could have some other protocol in play.  The isHttpScheme variable was introduced for the upgrade-insecure-request CSP directive.  We only want to check for an upgrade when the content being requested is actually HTTP.

For workers, you can assume at this point that the request is mixed because the parent is https and the protocol associated with aContentLocation doesn't map to any of the secure URI flags - https://dxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp?from=nsMixedcontentblocker.cpp#517


+  // Disallow mixed content loads for workers, shared workers and service
+  // workers.
+  if (isWorkerType && parentIsHttps && !isHttpsScheme) {
+    *aDecision = REJECT_REQUEST;
+    return NS_OK;
+  }
We've already done a !parentIsHttps check above that returns, so you don't need to check it here.  All you need here is:
if (isWorkerType) {
  *aDecision = REJECT_REQUEST;
  return NS_OK;
}

You should put this right after the if(!parentIsHTTPS) part.

Comment 43

[Tanvi Vyas[:tanvi]]

Comment on attachment 8660379 [details] [diff] [review]
Part 2: Add tests for mixed content blocking of scripts in workers

Please tag these tests with "mcb".

Do you have a test where an HTTPS service worker tries to include an HTTPS script to show that that still succeeds?  Perhaps that is already covered in existing service worker tests?

Comment 44

[Tanvi Vyas[:tanvi]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #33)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #31)
> > ::: dom/security/nsMixedContentBlocker.cpp
> > @@ +650,5 @@
> > > +
> > > +  // Disallow mixed content loads for workers, shared workers and service
> > > +  // workers.
> > > +  if (isWorkerType && parentIsHttps && isHttpScheme) {
> > > +    *aDecision = REJECT_REQUEST;
> > 
> > Two questions:
> > 1) What happens/(should happen) if the CSP directive
> > 'upgrade-insecure-requests' is set? At the moment, this patch would block
> > such a request, because we can not query the docshell because 'aParent' was
> > null on the callsite. I assume we are fine with that for shared workers,
> > right? I am fine with that but wanna make sure we are not missing this case,
> > what do you think?
> 
> Yes, the request will be blocked and not upgraded.  According to comment 16,
> that is the behavior that we should have.
> 

Are there cases where the content policy check happens on the child (where the parentDoc exists) and not on the parent?  If so, then we are missing an opportunity to first upgrade if the upgrade-insecure-requests directive is set.  We will block regardless of this directive.  I think that is okay (especially for v1 as Anne points out) but I just wanted to call it out as a future enhancement.  If parentDoc is available, type isWorkerType, and the upgrade directive is set, we could try the upgrade.

Comment 45

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #42)

> dom/security/nsMixedContentBlocker.cpp
> -  bool isHttpScheme = false;
> -  rv = aContentLocation->SchemeIs("http", &isHttpScheme);
> +  bool isHttpsScheme = false;
> +  rv = aContentLocation->SchemeIs("https", &isHttpsScheme);
> 
> !isHttpsScheme != isHttpScheme.  We could have some other protocol in play. 
> The isHttpScheme variable was introduced for the upgrade-insecure-request
> CSP directive.  We only want to check for an upgrade when the content being
> requested is actually HTTP.

That is a good catch; what I meant with Comment 31 is that we keep both, the isHttpScheme for upgrade-insecure-requests but also introduce a isHttp*s*Scheme which allows checking the scheme for workers. We need to update that.

Comment 46

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8660378 [details] [diff] [review]
Part 1: Do not allow loading mixed content scripts in workers

Approval for aurora and sec-approval+ so we don't ship this issue.

Comment 47

[(no longer active)]

(In reply to Tanvi Vyas [:tanvi] from comment #43)
> Comment on attachment 8660379 [details] [diff] [review]
> Part 2: Add tests for mixed content blocking of scripts in workers
> 
> Please tag these tests with "mcb".

Hmm, what do you mean?

> Do you have a test where an HTTPS service worker tries to include an HTTPS
> script to show that that still succeeds?  Perhaps that is already covered in
> existing service worker tests?

That should already be covered by other tests in dom/workers/test/serviceworkers.

Comment 48

[(no longer active)]

(In reply to Tanvi Vyas [:tanvi] from comment #44)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #33)
> > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #31)
> > > ::: dom/security/nsMixedContentBlocker.cpp
> > > @@ +650,5 @@
> > > > +
> > > > +  // Disallow mixed content loads for workers, shared workers and service
> > > > +  // workers.
> > > > +  if (isWorkerType && parentIsHttps && isHttpScheme) {
> > > > +    *aDecision = REJECT_REQUEST;
> > > 
> > > Two questions:
> > > 1) What happens/(should happen) if the CSP directive
> > > 'upgrade-insecure-requests' is set? At the moment, this patch would block
> > > such a request, because we can not query the docshell because 'aParent' was
> > > null on the callsite. I assume we are fine with that for shared workers,
> > > right? I am fine with that but wanna make sure we are not missing this case,
> > > what do you think?
> > 
> > Yes, the request will be blocked and not upgraded.  According to comment 16,
> > that is the behavior that we should have.
> > 
> 
> Are there cases where the content policy check happens on the child (where
> the parentDoc exists) and not on the parent?

No.  Service workers (and shared workers) don't have a parent doc <https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/dom/workers/WorkerPrivate.cpp#3367>.  (Note that the code I'm pointing to runs after loading the main script for the worker, and before executing it, so importScripts() would not be processed until only after that point.)

Comment 49

[(no longer active)]

(In reply to Tanvi Vyas [:tanvi] from comment #42)
> dom/security/nsMixedContentBlocker.cpp
> +  // The content policy type that we receive may be an internal type for
> +  // scripts.  
> 
> The type is internal when the request for such content is made by workers. 

No, this is only used for loading scripts inside service workers, not for other things.

> Same here:

And same here!

Comment 50

[Tanvi Vyas[:tanvi]]

Thanks for your responses Ehsan!

(In reply to Ehsan Akhgari (don't ask for review please) from comment #47)
> (In reply to Tanvi Vyas [:tanvi] from comment #43)
> > Comment on attachment 8660379 [details] [diff] [review]
> > Part 2: Add tests for mixed content blocking of scripts in workers
> > 
> > Please tag these tests with "mcb".
> 
> Hmm, what do you mean?
> 
Now we can tag specific mochitests (https://lists.mozilla.org/pipermail/dev-platform/2015-April/009217.html) so we've created a mixed content blocker tag for all tests related to mixed content called "mcb".  To use the tag just do the following:

>diff --git a/dom/workers/test/mochitest.ini b/dom/workers/test/mochitest.ini
>index a4e216b..54eee2d 100644
>--- a/dom/workers/test/mochitest.ini
>+++ b/dom/workers/test/mochitest.ini
>@@ -145,16 +148,18 @@ skip-if = (toolkit == 'gonk' && debug) #debug-only failure
> [test_errorPropagation.html]
> skip-if = buildapp == 'b2g' # b2g(times out) b2g-debug(times out) b2g-desktop(times out)
> [test_errorwarning.html]
> skip-if = buildapp == 'b2g' # b2g(Failed to load script: errorwarning_worker.js) b2g-debug(Failed to load script: errorwarning_worker.js) b2g-desktop(Failed to load script: errorwarning_worker.js)
> [test_eventDispatch.html]
> [test_fibonacci.html]
> skip-if = buildapp == 'b2g' # b2g(Failed to load script: fibonacci_worker.js) b2g-debug(Failed to load script: fibonacci_worker.js) b2g-desktop(Failed to load script: fibonacci_worker.js)
> [test_importScripts.html]
>+[test_importScripts_mixedcontent.html]

+tags = mcb

>+skip-if = buildapp == 'b2g' # no https on b2g
> [test_instanceof.html]
> [test_json.html]
> [test_jsversion.html]
> skip-if = (toolkit == 'gonk' && debug) #debug-only failure
> [test_loadEncoding.html]
> [test_loadError.html]
> [test_location.html]
> [test_longThread.html]

Here is a similar example: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser.ini#262


(In reply to Ehsan Akhgari (don't ask for review please) from comment #49)
> (In reply to Tanvi Vyas [:tanvi] from comment #42)
> > dom/security/nsMixedContentBlocker.cpp
> > +  // The content policy type that we receive may be an internal type for
> > +  // scripts.  
> > 
> > The type is internal when the request for such content is made by workers. 
> 
> No, this is only used for loading scripts inside service workers, not for
> other things.
> 
What happens when a service worker initiates the load of an image?  What content type is used then?

Comment 51

[(no longer active)]

Attachment: Part 1: Do not allow loading mixed content scripts in workers

Comment 52

[(no longer active)]

(In reply to Tanvi Vyas [:tanvi] from comment #50)
> Thanks for your responses Ehsan!
> 
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #47)
> > (In reply to Tanvi Vyas [:tanvi] from comment #43)
> > > Comment on attachment 8660379 [details] [diff] [review]
> > > Part 2: Add tests for mixed content blocking of scripts in workers
> > > 
> > > Please tag these tests with "mcb".
> > 
> > Hmm, what do you mean?
> > 
> Now we can tag specific mochitests
> (https://lists.mozilla.org/pipermail/dev-platform/2015-April/009217.html) so
> we've created a mixed content blocker tag for all tests related to mixed
> content called "mcb".  To use the tag just do the following:

Gotcha, thanks for the explanation!

> (In reply to Ehsan Akhgari (don't ask for review please) from comment #49)
> > (In reply to Tanvi Vyas [:tanvi] from comment #42)
> > > dom/security/nsMixedContentBlocker.cpp
> > > +  // The content policy type that we receive may be an internal type for
> > > +  // scripts.  
> > > 
> > > The type is internal when the request for such content is made by workers. 
> > 
> > No, this is only used for loading scripts inside service workers, not for
> > other things.
> > 
> What happens when a service worker initiates the load of an image?  What
> content type is used then?

TYPE_IMAGE, as usual.  We don't modify the content types based on who is requesting to load them.  TYPE_INTERNAL_SERVICE_WORKER is *only* used for fetching the worker scripts (be in the main script, or something importScripts'ed.)  I hope this makes more sense now.

Comment 53

[(no longer active)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #52)
> > (In reply to Ehsan Akhgari (don't ask for review please) from comment #49)
> > > (In reply to Tanvi Vyas [:tanvi] from comment #42)
> > > > dom/security/nsMixedContentBlocker.cpp
> > > > +  // The content policy type that we receive may be an internal type for
> > > > +  // scripts.  
> > > > 
> > > > The type is internal when the request for such content is made by workers. 
> > > 
> > > No, this is only used for loading scripts inside service workers, not for
> > > other things.
> > > 
> > What happens when a service worker initiates the load of an image?  What
> > content type is used then?
> 
> TYPE_IMAGE, as usual.

Err, as I explained on IRC, it will be TYPE_XMLHTTPREQUEST if you use XHR to fetch the image (or TYPE_FETCH or whatever we use for fetch if they use fecth()).  TYPE_IMAGE will never be used.

Comment 54

[Tanvi Vyas[:tanvi]]

Something just occurred to me...
What will happen if the imported script is redirected?  mcb and csp will then be called with the external types, right?  Do we need to add this externalTypeOrScript logic there?  If we don't, will run into the docshell code in nsMixedContenetBlocker and fail.  Christoph, do you have thoughts on this?

Comment 55

[Tanvi Vyas[:tanvi]]

Comment on attachment 8661431 [details] [diff] [review]
Part 1: Do not allow loading mixed content scripts in workers

Thanks for the changes Ehsan!  Take a look at my comment about redirects above.  Two minor nits below.

>-  MOZ_ASSERT(aContentType == nsContentUtils::InternalContentPolicyTypeToExternal(aContentType),
>+  MOZ_ASSERT(aContentType == nsContentUtils::InternalContentPolicyTypeToExternalOrScript(aContentType),
>              "We should only see external content policy types here.");

Change to:
"We should only see external content policy types or internal worker types here."

>@@ -638,16 +663,17 @@ nsMixedContentBlocker::ShouldLoad(bool aHadInsecureImageRedirect,
>   // http: and ws: (for websockets). Websockets are not subject to mixed content
>   // blocking since insecure websockets are not allowed within secure pages. Hence,
>   // we only have to check against http: here. Skip mixed content blocking if the
>   // subresource load uses http: and the CSP directive 'upgrade-insecure-requests'
>   // is present on the page.
>   bool isHttpScheme = false;
>   rv = aContentLocation->SchemeIs("http", &isHttpScheme);
>   NS_ENSURE_SUCCESS(rv, rv);
>+
>   if (isHttpScheme && docShell->GetDocument()->GetUpgradeInsecureRequests()) {
>     *aDecision = ACCEPT;
>     return NS_OK;
>   }
>
>   bool rootHasSecureConnection = false;
>   bool allowMixedContent = false;
>   bool isRootDocShell = false;

Remove extra newline here.

Comment 56

[(no longer active)]

(In reply to Tanvi Vyas [:tanvi] from comment #54)
> Something just occurred to me...
> What will happen if the imported script is redirected?  mcb and csp will
> then be called with the external types, right? 

Why would that be the case?  We should not be storing the external type, so even after a redirect the logic I'm adding here should work...

Comment 57

[(no longer active)]

Hmm, but you're right.  I added a test for redirects, and it fails indeed!  Thanks for catching this.

I'll investigate why tomorrow.  It's too late tonight...

Comment 58

[(no longer active)]

Attachment: Part 2: Add tests for mixed content blocking of scripts in workers

Updated the tests to cover redirects, and also addressed Tanvi's comment.

Comment 59

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #57)
> Hmm, but you're right.  I added a test for redirects, and it fails indeed! 
> Thanks for catching this.
> 
> I'll investigate why tomorrow.  It's too late tonight...

Reason is that mixedContentBlocker implements it's own redirect handling. Once we have completely converted all callsites to use ::AsyncOpen2(), this is going to become a non issue, because the redirect channel will also be openend using AsyncOpen2() and hence mixedContentBlocker will called through the regular codepath. Until then we have to implement the same logic here [1] and query the internal contentPolicyType from the loadinfo and pass that to your newly added interalContentPolicyTypeToExternalOrScript function.

Thanks Tanvi for catching.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#294

Comment 60

[(no longer active)]

Yes, makes perfect sense.  Thanks!

Comment 61

[(no longer active)]

Attachment: Part 1: Do not allow loading mixed content scripts in workers

Comment 62

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8661768 [details] [diff] [review]
Part 1: Do not allow loading mixed content scripts in workers

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

Tanvi does not have time to review that in the upcoming two days, hence she asked my to sign off on it! r=ckerschb,tanvi

::: dom/security/nsMixedContentBlocker.cpp
@@ +290,5 @@
>      // a failure. See bug 1077201.
>      return NS_OK;
>    }
>  
> +  nsContentPolicyType contentPolicyType = loadInfo->InternalContentPolicyType();

I am really happy once we have converted all callsites to use AsyncOpen2(). Separate redirect handling is confusing. Thanks for fixing this!

@@ +647,5 @@
> +    MOZ_ASSERT(!isHttpsScheme);
> +#endif
> +    *aDecision = REJECT_REQUEST;
> +    return NS_OK;
> +  }

Thanks, that looks cleaner now.

@@ +667,5 @@
>    // is present on the page.
>    bool isHttpScheme = false;
>    rv = aContentLocation->SchemeIs("http", &isHttpScheme);
>    NS_ENSURE_SUCCESS(rv, rv);
> +

Nit: remove added empty line

Comment 63

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ca0821a9b05

Comment 64

[Carsten Book [:Tomcat]]

this should have had sec-approval before this was inital pushed to mozilla-inbound (and now via the merge automatically to m-c) or ?

Comment 65

[Carsten Book [:Tomcat]]

oh never mind it already had :)

Comment 66

[(no longer active)]

checkin-needed for a backport of comment 63 to Aurora.

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/567a6defb14b

Comment 68

[Daniel Veditz [:dveditz]]

sec-high was overstating the problem really, a well-written site wouldn't want to have mixed content, and a malicious site doesn't get a whole lot of advantage by forcing a user to load something in the clear. Allowing things in the clear does expose users to risk, but the risk generally comes from folks on the wire and not the attacker creating the page/script/worker.

Comment 69

[Tanvi Vyas[:tanvi]]

Clearing my needinfo since Christoph stepped in to help here.  Thanks!

Comment 70

[Carsten Book [:Tomcat]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #66)
> checkin-needed for a backport of comment 63 to Aurora.

clearding checkin-needed since this was done in #comment 67

Comment 71

[(no longer active)]

Since this is now opened, I'm going to land the test.

Comment 72

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5204bd4ad56c

Comment 73

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5204bd4ad56c