Bug 183025 – Add release asserts for service worker fetch and postMessage events

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

In case a page and sw with different origins can start communicating.

[youenn fablet]

Created attachment 334426 [details]
Patch

[youenn fablet]

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.

[youenn fablet]

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()

[Daniel Bates]

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.

[youenn fablet]

> 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.

[youenn fablet]

<rdar://problem/37765052>

[Daniel Bates]

(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.

[Daniel Bates]

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.

[youenn fablet]

Created attachment 334450 [details]
Patch for landing

[Chris Dumez]

Comment on attachment 334450 [details]
Patch for landing

Clearing flags on attachment: 334450

Committed r228919: <https://trac.webkit.org/changeset/228919>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

We need to distinguish navigation from subresource loads as well.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 334483 [details]
early exit alternative

[youenn fablet]

Resolved as fixed.
If we san crashes in trunk Safari, we can always reopen.