1130688 - Implement additional scope checking in ServiceWorker register() algorithm

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

It appears an additional security check was added to the register() algorithm here:

  https://github.com/slightlyoff/ServiceWorker/issues/595
  https://github.com/slightlyoff/ServiceWorker/commit/f23c1d4e566847feebcfee8814132b0f8a8d56f1

This changes the default scope to be './' and checks to see if the scope is below the SW script location.

We should either implement this or verify we already do it before pref'ing on.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Implement additional scope checking in service worker registration

Ben, some of the steps in the original commit have been replaced with the Service-Worker-Allowed header. I've implemented what should happen when that header is not passed in this patch. There is another bug on file for when it is passed, which is for v2.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8575022 [details] [diff] [review]
Implement additional scope checking in service worker registration

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

Overall LGTM.  Mostly just nits.  It seems we are relying on precise formatting of the scope.  I mainly just want to be sure that's guaranteed.  r=me with that addressed.

Flag Ehsan for DOM peer sign off.

::: dom/webidl/ServiceWorkerContainer.webidl
@@ +40,5 @@
>    [Throws,Pref="dom.serviceWorkers.testing.enabled"]
>    DOMString getScopeForUrl(DOMString url);
>  };
>  
>  dictionary RegistrationOptionList {

It appears this is now called RegistrationOptions in the spec.  Can you please update while you are here?

::: dom/workers/ServiceWorkerContainer.cpp
@@ +89,5 @@
> +
> +  nsCOMPtr<nsIURI> scriptURI;
> +  aRv = NS_NewURI(getter_AddRefs(scriptURI), aScriptURL, nullptr,
> +                  window->GetDocBaseURI());
> +  if (NS_WARN_IF(aRv.Failed())) {

Per spec step 2, this should throw a TypeError if it fails.  You do this below in the !aOptions.mScope.WasPassed() case, but not here.

@@ +101,5 @@
> +  // Step 4. If none passed, parse against script's URL
> +  if (!aOptions.mScope.WasPassed()) {
> +    nsresult rv = NS_NewURI(getter_AddRefs(scopeURI), NS_LITERAL_CSTRING("./"),
> +                            nullptr, scriptURI);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {

If we parsed the scriptURI successfully above, can we not just assert this as MOZ_ALWAYS_TRUE(NS_SUCCEEDED(rv))?

::: dom/workers/ServiceWorkerManager.cpp
@@ +443,5 @@
>  };
>  
> +namespace {
> +bool
> +GetMaxScopeString(const nsACString& aScriptSpec, nsACString& aMaxScope)

I found the name here confusing.  I guess in terms of resources covered, you are getting the "max" scope.  To me, though, I think of this as the "minimum" scope string.  Its the smallest scope string allowed and it all valid scope strings must begin with this prefix.

Maybe change it to GetRequiredScopeStringPrefix() or something?

Or, maybe we can leave this logic to nsIURI completely.  See my comment below about using nsIURI's GetRelativeSpec().

@@ +449,5 @@
> +  nsCOMPtr<nsIURI> scriptURI;
> +  nsresult rv = NS_NewURI(getter_AddRefs(scriptURI), aScriptSpec,
> +                 nullptr, nullptr);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return false;

Why not return the rv value?

@@ +602,5 @@
> +      Fail(NS_ERROR_DOM_SECURITY_ERR);
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    if (!StringBeginsWith(mRegistration->mScope, maxScopeString)) {

Are we guaranteed this will be accurate for the scope string?  It seems parsing URIs and URLs has a lot of corner cases.  If so, can you just make a comment that we have guaranteed formating of mScope in Register() or something.

A possible alternative would be:

  nsAutoCString relativeScope;
  scriptURI->GetRelativeSpec(scopeURI, relativeScope);
  if (StringBeginsWith(relativeScope, NS_LITERAL_CSTRING(".."))) {
    // bad scope
  }

I think thats safe since we guarantee same-origin in Register().

Comment 3

[(no longer active)]

Comment on attachment 8575022 [details] [diff] [review]
Implement additional scope checking in service worker registration

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

r=me on the WebIDL with Ben's comment addressed.  Please let me know if you wanted review on other parts as well.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf6cf7920b22

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bcba8fcdafd

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6bcba8fcdafd