Service worker creation from <link> leads to nullptr dereference

Service worker creation from <link> leads to nullptr dereference

If a sandboxed iframe or a frame in a suborigin attempts to register a
service worker with <link rel=serviceworker>, it leads to a nullptr
deference. This is because ServiceWorkerLinkResource::process() does not
account for the possibility that NavigatorServiceWorker::serviceWorker
can return a nullptr. In this case, sandboxed iframes and suborigins are
not allowed to register service workers, so they return nullptr.

The solution is to check for nullptr and throw an appropriate error.
Tests are added to verify as well.

BUG=649239
Committed: https://crrev.com/2c0f1f71edd2ae034329ad2617e68fa89dbe536c
Cr-Commit-Position: refs/heads/master@{#420853}

[jww, 2016-09-22 05:44:35]

jww@chromium.org changed reviewers:
+ falken@chromium.org

[jww, 2016-09-22 05:44:35]

falken@, can you review this? It turns out I was sort of accounting for
suborigins not being able to be registered via <link rel=serviceworker> by
putting the restrictions for suborigins at the same place as the restrictions
for sandboxed iframes. However, there was a bug that no one had touched if you
tried to do this from within a sandboxed iframe, so it sort of didn't work :)

Hopefully this should address the latent bug for both the sandboxed iframe and
suborigin case.

[falken, 2016-09-23 01:22:33]

looks good with nits

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-dom-access.php
(right):

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-dom-access.php:7:
<title>Service Worker DOM API fails from a suborigin</title>
really nitty: I've never heard the term "SW DOM API" before. In fact we'd say
"SW doesn't have access to the DOM" to mean it can't manipulate documents. I
would just call this "Service worker JavaScript API". But I'm not so sure about
the boundary of DOM vs JS.

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-link-rel.php
(right):

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-link-rel.php:14:
link.setAttribute('href', 'resources/sw-reject-all-with-error.js');
I think this file doesn't exist? The test could be passing just because this is
a 404. Also, does it need to be sw-reject-all-with-error, that's a pretty
specific worker. I'd just use an empty js file.

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/iframe-register-link-element.html
(right):

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/iframe-register-link-element.html:4:
link.setAttribute('href', 'fetch-event-network-error-worker.js');
similarly, I'd use empty-worker.js here.

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/sandbox-iframe-register-link-element.html
(right):

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/sandbox-iframe-register-link-element.html:19:
});
Since this is serviceworker/ it should use the somewhat unorthodox style
described here:
http://www.chromium.org/blink/serviceworker/testing

Namely:
- Use async_test(function(t) { ... }, 'Sandboxed...');
- Indent function arguments like this:

window.addEventListener('message', function(e) {
    console.log('hi');
  });

[jww, 2016-09-23 04:34:47]

Thanks, Matt. Let me know if you see anything else!

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-dom-access.php
(right):

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-dom-access.php:7:
<title>Service Worker DOM API fails from a suborigin</title>
On 2016/09/23 01:22:33, falken wrote:
> really nitty: I've never heard the term "SW DOM API" before. In fact we'd say
> "SW doesn't have access to the DOM" to mean it can't manipulate documents. I
> would just call this "Service worker JavaScript API". But I'm not so sure
about
> the boundary of DOM vs JS.

Done.

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-link-rel.php
(right):

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/suborigins/suborigin-service-worker-link-rel.php:14:
link.setAttribute('href', 'resources/sw-reject-all-with-error.js');
On 2016/09/23 01:22:33, falken wrote:
> I think this file doesn't exist? The test could be passing just because this
is
> a 404. Also, does it need to be sw-reject-all-with-error, that's a pretty
> specific worker. I'd just use an empty js file.
It does exist, from a prior CL (see
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...).

But, yes, you're absolutely right. Should be simplified, so pointing it at
/resources/dummy.js

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/iframe-register-link-element.html
(right):

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/iframe-register-link-element.html:4:
link.setAttribute('href', 'fetch-event-network-error-worker.js');
On 2016/09/23 01:22:33, falken wrote:
> similarly, I'd use empty-worker.js here.

Done.

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/sandbox-iframe-register-link-element.html
(right):

https://codereview.chromium.org/2358993003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/sandbox-iframe-register-link-element.html:19:
});
On 2016/09/23 01:22:33, falken wrote:
> Since this is serviceworker/ it should use the somewhat unorthodox style
> described here:
> http://www.chromium.org/blink/serviceworker/testing
> 
> Namely:
> - Use async_test(function(t) { ... }, 'Sandboxed...');
> - Indent function arguments like this:
> 
> window.addEventListener('message', function(e) {
>     console.log('hi');
>   });

Yikes! I had no idea. I tried to update to match the style, but you'll probably
want to double check...

[falken, 2016-09-24 15:04:30]

lgtm

https://codereview.chromium.org/2358993003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/sandbox-iframe-register-link-element.html
(right):

https://codereview.chromium.org/2358993003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/sandbox-iframe-register-link-element.html:19:
});
almost! lines 9-19 should be indented an additional 2 spaces

[jww, 2016-09-24 16:25:08]

https://codereview.chromium.org/2358993003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/sandbox-iframe-register-link-element.html
(right):

https://codereview.chromium.org/2358993003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/sandbox-iframe-register-link-element.html:19:
});
On 2016/09/24 15:04:29, falken wrote:
> almost! lines 9-19 should be indented an additional 2 spaces

Dang! Thanks :-)

[jww, 2016-09-24 16:25:14]

The CQ bit was checked by jww@chromium.org

[jww, 2016-09-24 16:25:14]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2358993003/#ps60001
(title: "Formatting")

[commit-bot: I haz the power, 2016-09-24 16:25:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-24 18:28:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-24 18:28:38]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jww, 2016-09-24 22:31:38]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2016-09-24 22:31:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-24 23:35:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-24 23:37:56]

Description was changed from

==========
Service worker creation from <link> leads to nullptr dereference

If a sandboxed iframe or a frame in a suborigin attempts to register a
service worker with <link rel=serviceworker>, it leads to a nullptr
deference. This is because ServiceWorkerLinkResource::process() does not
account for the possibility that NavigatorServiceWorker::serviceWorker
can return a nullptr. In this case, sandboxed iframes and suborigins are
not allowed to register service workers, so they return nullptr.

The solution is to check for nullptr and throw an appropriate error.
Tests are added to verify as well.


BUG=649239
==========

to

==========
Service worker creation from <link> leads to nullptr dereference

If a sandboxed iframe or a frame in a suborigin attempts to register a
service worker with <link rel=serviceworker>, it leads to a nullptr
deference. This is because ServiceWorkerLinkResource::process() does not
account for the possibility that NavigatorServiceWorker::serviceWorker
can return a nullptr. In this case, sandboxed iframes and suborigins are
not allowed to register service workers, so they return nullptr.

The solution is to check for nullptr and throw an appropriate error.
Tests are added to verify as well.

BUG=649239

Committed: https://crrev.com/2c0f1f71edd2ae034329ad2617e68fa89dbe536c
Cr-Commit-Position: refs/heads/master@{#420853}
==========

[commit-bot: I haz the power, 2016-09-24 23:37:58]

Patchset 4 (id:??) landed as
https://crrev.com/2c0f1f71edd2ae034329ad2617e68fa89dbe536c
Cr-Commit-Position: refs/heads/master@{#420853}