Bug 183025 - Add release asserts for service worker fetch and postMessage events
Summary: Add release asserts for service worker fetch and postMessage events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-21 16:55 PST by youenn fablet
Modified: 2018-02-28 17:53 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.92 KB, patch)
2018-02-21 17:02 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (5.97 KB, patch)
2018-02-22 07:18 PST, youenn fablet
no flags Details | Formatted Diff | Diff
early exit alternative (6.41 KB, patch)
2018-02-22 16:34 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-02-21 16:55:20 PST
In case a page and sw with different origins can start communicating.
Comment 1 youenn fablet 2018-02-21 17:02:58 PST
Created attachment 334426 [details]
Patch
Comment 2 youenn fablet 2018-02-21 17:05:49 PST
For fetch, we need to take into account blob and data URLs frames making loads and being intercepted by the parent frame service worker.
We also need to take into account non HTTP registered service workers.

For postMessage, I used the same approach but I wonder if there are cases where we would release assert and should not.
Comment 3 youenn fablet 2018-02-21 17:50:47 PST
Comment on attachment 334426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334426&action=review

> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:188
> +    RELEASE_ASSERT(url.isEmpty() || !url.protocolIsInHTTPFamily() || !serviceWorkerURL.protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(url, serviceWorkerURL));

I can probably remove url.isEmpty()
Comment 4 Daniel Bates 2018-02-21 20:04:44 PST
Comment on attachment 334426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334426&action=review

Same origin checks should be written in terms of SecurityOrigin. It knows how to apply them correctly.

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:124
> +            RELEASE_ASSERT(!sourceClient->url().protocolIsInHTTPFamily() || !serviceWorkerGlobalScope.url().protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(serviceWorkerGlobalScope.url(), sourceClient->url()));
> +

This does not seem correct. Can we write this in terms of SecurityOrigin?

>> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:188
>> +    String origin = request.httpOrigin();
>> +    URL url { URL(), origin.isEmpty() ? referrer : origin };
>> +    URL serviceWorkerURL = serviceWorkerThreadProxy->scriptURL();
>> +    RELEASE_ASSERT(url.isEmpty() || !url.protocolIsInHTTPFamily() || !serviceWorkerURL.protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(url, serviceWorkerURL));
> 
> I can probably remove url.isEmpty()

How did you come to the decision to make use of the HTTP Origin header or fall back to referrer? Can we write this in terms of SecurityOrigin.
Comment 5 youenn fablet 2018-02-21 20:42:01 PST
> Same origin checks should be written in terms of SecurityOrigin. It knows
> how to apply them correctly.

We do not try to enforce same origin checks.
We try enforcing service worker partitioning and controlling checks.
For instance, a data URL iframe will be controlled by the service worker controlling the parent of the iframe.

We could add a SecurityOrigin method for that purpose but this seems unnecessary to me.
Note that these checks are in case something really bad happens.

> > Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:124
> > +            RELEASE_ASSERT(!sourceClient->url().protocolIsInHTTPFamily() || !serviceWorkerGlobalScope.url().protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(serviceWorkerGlobalScope.url(), sourceClient->url()));
> > +
> 
> This does not seem correct. Can we write this in terms of SecurityOrigin?

I don't see why this is incorrect.
It is true that if we had more information, we could make tighter checks.
But we try to limit the number of changes here.

We could replace protocolHostAndPortAreEqual(...) by something like serviceWorkerGlobalScope.topOrigin().canRequest(...).
This does not look like an improvement to me though since what we are really trying to check is protocol/host/port equality.

> >> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:188
> >> +    String origin = request.httpOrigin();
> >> +    URL url { URL(), origin.isEmpty() ? referrer : origin };
> >> +    URL serviceWorkerURL = serviceWorkerThreadProxy->scriptURL();
> >> +    RELEASE_ASSERT(url.isEmpty() || !url.protocolIsInHTTPFamily() || !serviceWorkerURL.protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(url, serviceWorkerURL));
> > 
> > I can probably remove url.isEmpty()
> 
> How did you come to the decision to make use of the HTTP Origin header or
> fall back to referrer? Can we write this in terms of SecurityOrigin.

We do not have any notion of the origin of the frame triggering the load at that point.
We could pipe it through IPC but this is a best effort patch and we try to limit the number of changes.
Hence why using Origin/Referrer.
Comment 6 youenn fablet 2018-02-21 21:01:59 PST
<rdar://problem/37765052>
Comment 7 Daniel Bates 2018-02-22 00:42:29 PST
(In reply to youenn fablet from comment #5)
> > Same origin checks should be written in terms of SecurityOrigin. It knows
> > how to apply them correctly.
> 
> We do not try to enforce same origin checks.
> We try enforcing service worker partitioning and controlling checks.
> For instance, a data URL iframe will be controlled by the service worker
> controlling the parent of the iframe.
> 
> We could add a SecurityOrigin method for that purpose but this seems
> unnecessary to me.
> Note that these checks are in case something really bad happens.
> 
> > > Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:124
> > > +            RELEASE_ASSERT(!sourceClient->url().protocolIsInHTTPFamily() || !serviceWorkerGlobalScope.url().protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(serviceWorkerGlobalScope.url(), sourceClient->url()));
> > > +
> > 
> > This does not seem correct. Can we write this in terms of SecurityOrigin?
> 
> I don't see why this is incorrect.
> It is true that if we had more information, we could make tighter checks.
> But we try to limit the number of changes here.

OK.

> 
> We could replace protocolHostAndPortAreEqual(...) by something like
> serviceWorkerGlobalScope.topOrigin().canRequest(...).
> This does not look like an improvement to me though since what we are really
> trying to check is protocol/host/port equality.
> 
> > >> Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:188
> > >> +    String origin = request.httpOrigin();
> > >> +    URL url { URL(), origin.isEmpty() ? referrer : origin };
> > >> +    URL serviceWorkerURL = serviceWorkerThreadProxy->scriptURL();
> > >> +    RELEASE_ASSERT(url.isEmpty() || !url.protocolIsInHTTPFamily() || !serviceWorkerURL.protocolIsInHTTPFamily() || protocolHostAndPortAreEqual(url, serviceWorkerURL));
> > > 
> > > I can probably remove url.isEmpty()
> > 
> > How did you come to the decision to make use of the HTTP Origin header or
> > fall back to referrer? Can we write this in terms of SecurityOrigin.
> 
> We do not have any notion of the origin of the frame triggering the load at
> that point.
> We could pipe it through IPC but this is a best effort patch and we try to
> limit the number of changes.
> Hence why using Origin/Referrer.

OK.
Comment 8 Daniel Bates 2018-02-22 00:44:53 PST
Comment on attachment 334426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334426&action=review

> Source/WebCore/ChangeLog:5
> +

Please add radar before landing.

> Source/WebCore/ChangeLog:8
> +        No change of behavior.

This is OK as-is. Typically we put such a remark after the description.

> Source/WebKit/ChangeLog:5
> +

Please add radar before landing.
Comment 9 youenn fablet 2018-02-22 07:18:14 PST
Created attachment 334450 [details]
Patch for landing
Comment 10 Chris Dumez 2018-02-22 08:56:00 PST
Comment on attachment 334450 [details]
Patch for landing

Clearing flags on attachment: 334450

Committed r228919: <https://trac.webkit.org/changeset/228919>
Comment 11 Chris Dumez 2018-02-22 08:56:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 youenn fablet 2018-02-22 11:08:18 PST
We need to distinguish navigation from subresource loads as well.
Comment 13 youenn fablet 2018-02-22 16:34:58 PST
Reopening to attach new patch.
Comment 14 youenn fablet 2018-02-22 16:34:59 PST
Created attachment 334483 [details]
early exit alternative
Comment 15 youenn fablet 2018-02-28 17:53:58 PST
Resolved as fixed.
If we san crashes in trunk Safari, we can always reopen.