Closed Bug 1130688 Opened 5 years ago Closed 5 years ago

Implement additional scope checking in ServiceWorker register() algorithm

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bkelly, Assigned: nsm)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
Assignee: nobody → nsm.nikhil
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.
Attachment #8575022 - Flags: review?(bkelly)
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().
Attachment #8575022 - Flags: review?(bkelly) → review+
Attachment #8575022 - Flags: review?(ehsan)
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.
Attachment #8575022 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/6bcba8fcdafd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1144280
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.