Bug 184029 – Make it possible to call ContentSecurityPolicy::upgradeInsecureRequestIfNeeded() from non-main threads

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Dumez]

ContentSecurityPolicy::upgradeInsecureRequestIfNeeded() should be called from the main thread. It constructs a SecurityOrigin object internally, which is only safe on the main thread.

[Chris Dumez]

Thread 25 Crashed:: WebCore: Worker
0   com.apple.JavaScriptCore      	0x00000003c7b24604 WTFCrash + 36 (Assertions.cpp:271)
1   com.apple.WebCore             	0x00000003ba855674 WebCore::SecurityOrigin::SecurityOrigin(WebCore::URL const&) + 180 (SecurityOrigin.cpp:150)
2   com.apple.WebCore             	0x00000003ba85582d WebCore::SecurityOrigin::SecurityOrigin(WebCore::URL const&) + 29 (SecurityOrigin.cpp:165)
3   com.apple.WebCore             	0x00000003ba855c57 WebCore::SecurityOrigin::create(WebCore::URL const&) + 327 (SecurityOrigin.cpp:201)
4   com.apple.WebCore             	0x00000003ba8e0b4f WebCore::ContentSecurityPolicy::upgradeInsecureRequestIfNeeded(WebCore::URL&, WebCore::ContentSecurityPolicy::InsecureRequestType) const + 111 (ContentSecurityPolicy.cpp:826)
5   com.apple.WebCore             	0x00000003ba8e0ab3 WebCore::ContentSecurityPolicy::upgradeInsecureRequestIfNeeded(WebCore::ResourceRequest&, WebCore::ContentSecurityPolicy::InsecureRequestType) const + 67 (ContentSecurityPolicy.cpp:817)
6   com.apple.WebCore             	0x00000003b9454948 WebCore::FetchLoader::start(WebCore::ScriptExecutionContext&, WebCore::FetchRequest const&) + 344 (FetchLoader.cpp:102)
7   com.apple.WebCore             	0x00000003b9467b2f WebCore::FetchResponse::BodyLoader::start(WebCore::ScriptExecutionContext&, WebCore::FetchRequest const&) + 1103 (FetchResponse.cpp:335)
8   com.apple.WebCore             	0x00000003b9467436 WebCore::FetchResponse::fetch(WebCore::ScriptExecutionContext&, WebCore::FetchRequest&, WTF::Function<void (WebCore::ExceptionOr<WebCore::FetchResponse&>&&)>&&) + 662 (FetchResponse.cpp:198)
9   com.apple.WebCore             	0x00000003b9469e6f WebCore::WorkerGlobalScopeFetch::fetch(WebCore::WorkerGlobalScope&, WTF::Variant<WTF::RefPtr<WebCore::FetchRequest, WTF::DumbPtrTraits<WebCore::FetchRequest> >, WTF::String>&&, WebCore::FetchRequestInit&&, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&) + 255 (WorkerGlobalScopeFetch.cpp:50)
10  com.apple.WebCore             	0x00000003b92a8679 WebCore::jsWorkerGlobalScopePrototypeFunctionFetchBody(JSC::ExecState*, WebCore::JSWorkerGlobalScope*, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&, JSC::ThrowScope&) + 585 (JSWorkerGlobalScope.cpp:2727)
11  com.apple.WebCore             	0x00000003b92a8b38 long long WebCore::IDLOperationReturningPromise<WebCore::JSWorkerGlobalScope>::call<&(WebCore::jsWorkerGlobalScopePrototypeFunctionFetchBody(JSC::ExecState*, WebCore::JSWorkerGlobalScope*, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&, JSC::ThrowScope&)), (WebCore::PromiseExecutionScope)1, (WebCore::CastedThisErrorBehavior)2>(JSC::ExecState&, char const*)::'lambda'(JSC::ExecState&, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&)::operator()(JSC::ExecState&, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&) const + 680 (JSDOMOperationReturningPromise.h:52)
12  com.apple.WebCore             	0x00000003b92a87f0 JSC::JSValue WebCore::callPromiseFunction<(WebCore::PromiseExecutionScope)1, long long

[Chris Dumez]

ASSERTION FAILED: isMainThread()
./page/SecurityOrigin.cpp(150) : WebCore::SecurityOrigin::SecurityOrigin(const WebCore::URL &)
1   0x3b4db35fd WTFCrash
2   0x3a7ae4674 WebCore::SecurityOrigin::SecurityOrigin(WebCore::URL const&)
3   0x3a7ae482d WebCore::SecurityOrigin::SecurityOrigin(WebCore::URL const&)
4   0x3a7ae4c57 WebCore::SecurityOrigin::create(WebCore::URL const&)
5   0x3a7b6fb4f WebCore::ContentSecurityPolicy::upgradeInsecureRequestIfNeeded(WebCore::URL&, WebCore::ContentSecurityPolicy::InsecureRequestType) const
6   0x3a88089f6 WebCore::XMLHttpRequest::open(WTF::String const&, WebCore::URL const&, bool)
7   0x3a8808d69 WebCore::XMLHttpRequest::open(WTF::String const&, WTF::String const&, bool, WTF::String const&, WTF::String const&)
8   0x3a654d46a WebCore::jsXMLHttpRequestPrototypeFunctionOpen2Body(JSC::ExecState*, WebCore::JSXMLHttpRequest*, JSC::ThrowScope&)
9   0x3a654cd85 WebCore::jsXMLHttpRequestPrototypeFunctionOpenOverloadDispatcher(JSC::ExecState*, WebCore::JSXMLHttpRequest*, JSC::ThrowScope&)
10  0x3a65416be long long WebCore::IDLOperation<WebCore::JSXMLHttpRequest>::call<&(WebCore::jsXMLHttpRequestPrototypeFunctionOpenOverloadDispatcher(JSC::ExecState*, WebCore::JSXMLHttpRequest*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*)
11  0x3a654144c WebCore::jsXMLHttpRequestPrototypeFunctionOpen(JSC::ExecState*)

[Chris Dumez]

Created attachment 336560 [details]
WIP Patch

[Chris Dumez]

Created attachment 336561 [details]
WIP Patch

[Chris Dumez]

Created attachment 336563 [details]
WIP Patch

[Chris Dumez]

Created attachment 336567 [details]
Previous patch

[youenn fablet]

Another option would be to remove use of SecurityOrigin from ContentSecurityPolicy::upgradeInsecureRequestIfNeeded().
For instance, m_insecureNavigationRequestsToUpgrade could be a HashSet<SecurityOriginData>.

[Chris Dumez]

(In reply to youenn fablet from comment #7)
> Another option would be to remove use of SecurityOrigin from
> ContentSecurityPolicy::upgradeInsecureRequestIfNeeded().
> For instance, m_insecureNavigationRequestsToUpgrade could be a
> HashSet<SecurityOriginData>.

That was my initial proposal but Daniel did not like that as much.

[youenn fablet]

(In reply to Chris Dumez from comment #8)
> (In reply to youenn fablet from comment #7)
> > Another option would be to remove use of SecurityOrigin from
> > ContentSecurityPolicy::upgradeInsecureRequestIfNeeded().
> > For instance, m_insecureNavigationRequestsToUpgrade could be a
> > HashSet<SecurityOriginData>.
> 
> That was my initial proposal but Daniel did not like that as much.

Some thoughts:
- ContentSecurityPolicy seems to restrict its use of SecurityOrigin to a triple { protocol, host, port }. SecurityOrigin is (unfortunately?) much more than that. SecurityOriginData is just that triple.

- SecurityOriginData is probably more efficient to construct and compare.

- We usually try to do as much as possible in background threads so there should be some benefit in doing the reverse.

 - We are using ContentSecurityPolicy in NetworkProcess and will use it more and more as we want to move redirection handling in NetworkProcess.
There is no real SecurityOrigin in NetworkProcess since we use SecurityOriginData for IPC.
Having ContentSecurityPolicy use some form of SecurityOriginData might be more consistent. And we could envision handling redirections in a NetworkProcess background thread.

[youenn fablet]

We had some discussion with Daniel.
Here are some related points in that context:

- We should try to stop using SecurityOrigin in NetworkProcess/StorageProcess and use it only in WebProcess+MainThread.
- Using SecurityOriginData (as it is now, meaning dumb) as a replacement to SecurityOrigin is fine. That would lead me to think we should do it in that patch (smaller patch, more efficient). Daniel was not opposed to it but was not enthusiastic.
- We should probably refrain from extending SecurityOriginData to some capabilities that SecurityOrigin has, like canRequest. Efforts have been made to put all security checks in one place and that is probably a good thing. We will need to see how we do this for NetworkProcess. It has a very specific case to handle (HTTP redirections) so might be fine with a dedicated approach based on exporting SecurityOriginData plus some additional NetworkResourceLoadParameters values.

Daniel, please correct if I misinterpret your points of view.

[Chris Dumez]

I personally do not mind either way since I already wrote the more complicated patch. I'll wait for Daniel to comment.

[Chris Dumez]

Created attachment 336626 [details]
Alternative patch using SecurityOriginData

[Chris Dumez]

(In reply to Chris Dumez from comment #12)
> Created attachment 336626 [details]
> Alternative patch using SecurityOriginData

Here is the patch using SecurityOriginData instead based on Youenn's discussion with Daniel.

[youenn fablet]

Comment on attachment 336626 [details]
Alternative patch using SecurityOriginData

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

> Source/WebCore/loader/DocumentWriter.cpp:158
> +    HashSet<SecurityOriginData> insecureNavigationRequestsToUpgrade;

insecureNavigationRequestsToUpgrade seems not totally correct.
insecureNavigationRequestOriginsToUpgrade seems better, but maybe not worth the refactoring effort, since it would require changing takeNavigationRequestsToUpgrade and m_insecureNavigationRequestsToUpgrade?

> Source/WebCore/page/csp/ContentSecurityPolicy.h:167
> +    HashSet<SecurityOriginData>&& takeNavigationRequestsToUpgrade();

Why not HashSet<SecurityOriginData>?

[Chris Dumez]

Created attachment 336629 [details]
Alternative patch using SecurityOriginData

[Chris Dumez]

Comment on attachment 336629 [details]
Alternative patch using SecurityOriginData

Ok, let's go with this one for now since Youenn prefers it and Daniel did not object. We can also reconsider later it turns out the other patch was better once Youenn is done with his refactoring.

[youenn fablet]

(In reply to Chris Dumez from comment #16)
> Comment on attachment 336629 [details]
> Alternative patch using SecurityOriginData
> 
> Ok, let's go with this one for now since Youenn prefers it and Daniel did
> not object. We can also reconsider later it turns out the other patch was
> better once Youenn is done with his refactoring.

Right, we are currently not implementing request upgrade for ping loads.
The plan is to beef up NetworkLoadChecker so that it does so through its own ContentSecurityPolicy.

Ideally, we would make ContentSecurityPolicy to be SecurityOrigin free as part of making NetworkProcess SecurityOrigin free.
As I see it now, the only refactoring bit is ContentSecurityPolicy constructor which would take a SecurityOriginData instead of a SecurityOrigin since it is only using it to get protocol/host/port.

[WebKit Commit Bot]

Comment on attachment 336629 [details]
Alternative patch using SecurityOriginData

Clearing flags on attachment: 336629

Committed r230017: <https://trac.webkit.org/changeset/230017>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38935104>

[youenn fablet]

> Right, we are currently not implementing request upgrade for ping loads.
> The plan is to beef up NetworkLoadChecker so that it does so through its own
> ContentSecurityPolicy.

Filed https://bugs.webkit.org/show_bug.cgi?id=184098