Closed Bug 1172948 Opened 9 years ago Closed 9 years ago

Prevent service workers in HTTPS iframes from being registered if the parent is HTTP

Categories

(Core :: DOM: Service Workers, defect)

Product:
Core
Component:
DOM: Service Workers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: bkelly, Assigned: ehsan.akhgari)

References

Details

Attachments

(3 files)

Part 1: Refactor the logic for checking for authenticated origins into a helper function
5.23 KB, patch
nsm
: review+
Details | Diff | Splinter Review
Part 2: Consider all non-chrome parent documents when checking the authenticity of an origin
5.39 KB, patch
nsm
: review+
Details | Diff | Splinter Review
Part 3: Add an explicit test case to ensure that authenticated origins that have a non-authenticated parent cannot register a service worker
3.81 KB, patch
nsm
: review+
Details | Diff | Splinter Review 
We should make sure we have a test that validates this behavior:

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

This seems like one of those security corner case issues.
Summary: Add test for ServiceWorker registered in https iframe attached to untrusted document → Prevent service workers in HTTPS iframes from being registered if the parent is HTTP
Assignee: nobody → ehsan
Comment on attachment 8620658 [details] [diff] [review]
Part 2: Consider all non-chrome parent documents when checking the authenticity of an origin

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +1251,5 @@
>    return authenticatedOrigin;
>  }
>  
> +static bool
> +IsFromAuthenticatedOrigin(nsIDocument* aDoc)

Could you add a comment that says this implements some of step 3 of https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-secure
We should eventually fold all of this into an nsContentUtils or similar helper.
Attachment #8620658 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8620659 [details] [diff] [review]
Part 3: Add an explicit test case to ensure that authenticated origins that have a non-authenticated parent cannot register a service worker

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

::: dom/workers/test/serviceworkers/register_https.html
@@ +13,5 @@
> +navigator.serviceWorker.register("empty.js", {scope: "register-https"})
> +  .then(reg => {
> +    ok(false, "Registration should fail");
> +    done();
> +  }, err => {

Nit: I prefer |then(...).catch(...)| instead of |then(..., ...)| due to "advanced mistake #2" here - http://pouchdb.com/2015/05/18/we-have-a-problem-with-promises.html

::: dom/workers/test/serviceworkers/test_register_https_in_http.html
@@ +4,5 @@
> +-->
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test that registering a service worker from inside an HTTPS iframe embedded in an HTTP iframe doesn't work</title>

Bug number please.

@@ +17,5 @@
> +<script class="testbody" type="text/javascript">
> +
> +  function runTest() {
> +    var iframe = document.createElement("iframe");
> +    iframe.src = "https://example.com/tests/dom/workers/test/serviceworkers/register_https.html";

Could you do some url parsing of window.location to make this truly use same-origin urls but with the scheme as https? I'm concerned that in the future if we add some other checks before the checks for this test case, we'll still pass this test but without checking the actual condition.
Attachment #8620659 - Flags: review?(nsm.nikhil) → review+
Status: NEW → ASSIGNED
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #5)
> @@ +17,5 @@
> > +<script class="testbody" type="text/javascript">
> > +
> > +  function runTest() {
> > +    var iframe = document.createElement("iframe");
> > +    iframe.src = "https://example.com/tests/dom/workers/test/serviceworkers/register_https.html";
> 
> Could you do some url parsing of window.location to make this truly use
> same-origin urls but with the scheme as https? I'm concerned that in the
> future if we add some other checks before the checks for this test case,
> we'll still pass this test but without checking the actual condition.

Hmm, do you mind giving me an example of what you mean?  As long as the child is loaded off of https and the parent is loaded off of http, I'm not sure how they can be same-origin...
Flags: needinfo?(nsm.nikhil)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #4)
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +1251,5 @@
> >    return authenticatedOrigin;
> >  }
> >  
> > +static bool
> > +IsFromAuthenticatedOrigin(nsIDocument* aDoc)
> 
> Could you add a comment that says this implements some of step 3 of
> https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-secure
> We should eventually fold all of this into an nsContentUtils or similar
> helper.

Agreed, but perhaps we should also actually implement the algorithm there, not just part of it?  I can look into doing that in a follow-up.  Do you know why we just implement this part of the algorithm here?
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> (In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #5)
> > @@ +17,5 @@
> > > +<script class="testbody" type="text/javascript">
> > > +
> > > +  function runTest() {
> > > +    var iframe = document.createElement("iframe");
> > > +    iframe.src = "https://example.com/tests/dom/workers/test/serviceworkers/register_https.html";
> > 
> > Could you do some url parsing of window.location to make this truly use
> > same-origin urls but with the scheme as https? I'm concerned that in the
> > future if we add some other checks before the checks for this test case,
> > we'll still pass this test but without checking the actual condition.
> 
> Hmm, do you mind giving me an example of what you mean?  As long as the
> child is loaded off of https and the parent is loaded off of http, I'm not
> sure how they can be same-origin...

You are right. I forgot that schemes are part of the origin too.
Flags: needinfo?(nsm.nikhil)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> (In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #4)
> > ::: dom/workers/ServiceWorkerManager.cpp
> > @@ +1251,5 @@
> > >    return authenticatedOrigin;
> > >  }
> > >  
> > > +static bool
> > > +IsFromAuthenticatedOrigin(nsIDocument* aDoc)
> > 
> > Could you add a comment that says this implements some of step 3 of
> > https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-secure
> > We should eventually fold all of this into an nsContentUtils or similar
> > helper.
> 
> Agreed, but perhaps we should also actually implement the algorithm there,
> not just part of it?  I can look into doing that in a follow-up.  Do you
> know why we just implement this part of the algorithm here?

Right now we don't really have a way to check for TLS state etc. in a nice way. See bug 1162772 which is relevant.
https://hg.mozilla.org/mozilla-central/rev/dc1cfa1c6dd8
https://hg.mozilla.org/mozilla-central/rev/fea0baf7b2b6
https://hg.mozilla.org/mozilla-central/rev/8fee3ab74df9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: