Bug 184068 – WebSocket::didReceiveMessage() may construct a SecurityOrigin object on a non-main thread

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Chris Dumez]

WebSocket::didReceiveMessage() may construct a SecurityOrigin object on a non-main thread, which is not safe.

[Chris Dumez]

Created attachment 336637 [details]
Patch

[Chris Dumez]

Created attachment 336641 [details]
Patch

[youenn fablet]

Comment on attachment 336641 [details]
Patch

Maybe we could assert in SecurityOriginData::toString that URL is http/https/ws/wss.

[Chris Dumez]

Comment on attachment 336641 [details]
Patch

Clearing flags on attachment: 336641

Committed r230042: <https://trac.webkit.org/changeset/230042>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38969197>

[Daniel Bates]

Comment on attachment 336641 [details]
Patch

This patch is not right. SecurityOriginData::toString() is not equivalent to SecurityOrigin::toString(). We need to stop and think before transitioning any  any more WebCore code to SecurityOriginData. It is not safe to do so.

[youenn fablet]

(In reply to Daniel Bates from comment #7)
> Comment on attachment 336641 [details]
> Patch
> 
> This patch is not right. SecurityOriginData::toString() is not equivalent to
> SecurityOrigin::toString(). We need to stop and think before transitioning
> any  any more WebCore code to SecurityOriginData. It is not safe to do so.

In that particular case, the URL is ws or wss, so I think this patch is correct as per https://url.spec.whatwg.org/#concept-url-origin.

[Daniel Bates]

(In reply to youenn fablet from comment #8)
> (In reply to Daniel Bates from comment #7)
> > Comment on attachment 336641 [details]
> > Patch
> > 
> > This patch is not right. SecurityOriginData::toString() is not equivalent to
> > SecurityOrigin::toString(). We need to stop and think before transitioning
> > any  any more WebCore code to SecurityOriginData. It is not safe to do so.
> 
> In that particular case, the URL is ws or wss, so I think this patch is
> correct as per https://url.spec.whatwg.org/#concept-url-origin.

This is not the correct section of the spec. to reference.

Take m_url to be the parsed URL corresponding to ws://example.com. Let D be the result of SecurityOriginData::fromURL(m_url) and S be the result of SecurityOrigin::create(m_url). Then D.toString() = "ws://example.com:0" != "ws://example.com" = S.toString().

Disregarding the aforementioned correctness issue, this patch is self-contradictory because it only "fixes" the usage of SecurityOrigin in WebSocket::didReceiveMessage(). It does not fix other uses of SecurityOrigin::create(m_url) in WebSocket::didReceiveBinaryData(). Regardless, I disagree with the approach of transitioning code from SecurityOrigin to SecurityOriginData in WebCore.

[Chris Dumez]

(In reply to Daniel Bates from comment #9)
> (In reply to youenn fablet from comment #8)
> > (In reply to Daniel Bates from comment #7)
> > > Comment on attachment 336641 [details]
> > > Patch
> > > 
> > > This patch is not right. SecurityOriginData::toString() is not equivalent to
> > > SecurityOrigin::toString(). We need to stop and think before transitioning
> > > any  any more WebCore code to SecurityOriginData. It is not safe to do so.
> > 
> > In that particular case, the URL is ws or wss, so I think this patch is
> > correct as per https://url.spec.whatwg.org/#concept-url-origin.
> 
> This is not the correct section of the spec. to reference.
> 
> Take m_url to be the parsed URL corresponding to ws://example.com. Let D be
> the result of SecurityOriginData::fromURL(m_url) and S be the result of
> SecurityOrigin::create(m_url). Then D.toString() = "ws://example.com:0" !=
> "ws://example.com" = S.toString().

Where is "ws://example.com:0" coming from? D.toString() would return "ws://example.com" (as of last week end).

> 
> Disregarding the aforementioned correctness issue, this patch is
> self-contradictory because it only "fixes" the usage of SecurityOrigin in
> WebSocket::didReceiveMessage(). It does not fix other uses of
> SecurityOrigin::create(m_url) in WebSocket::didReceiveBinaryData().
I am still going though the assertion failures and fixing them as I find them.

> Regardless, I disagree with the approach of transitioning code from
> SecurityOrigin to SecurityOriginData in WebCore.

[Daniel Bates]

(In reply to Chris Dumez from comment #10)
> (In reply to Daniel Bates from comment #9)
> > (In reply to youenn fablet from comment #8)
> > > (In reply to Daniel Bates from comment #7)
> > > > Comment on attachment 336641 [details]
> > > > Patch
> > > > 
> > > > This patch is not right. SecurityOriginData::toString() is not equivalent to
> > > > SecurityOrigin::toString(). We need to stop and think before transitioning
> > > > any  any more WebCore code to SecurityOriginData. It is not safe to do so.
> > > 
> > > In that particular case, the URL is ws or wss, so I think this patch is
> > > correct as per https://url.spec.whatwg.org/#concept-url-origin.
> > 
> > This is not the correct section of the spec. to reference.
> > 
> > Take m_url to be the parsed URL corresponding to ws://example.com. Let D be
> > the result of SecurityOriginData::fromURL(m_url) and S be the result of
> > SecurityOrigin::create(m_url). Then D.toString() = "ws://example.com:0" !=
> > "ws://example.com" = S.toString().
> 
> Where is "ws://example.com:0" coming from? D.toString() would return
> "ws://example.com" (as of last week end).
> 

Oops, I was looking at an old revision. I see that <https://trac.webkit.org/changeset/229954/> fixed up SecurityOriginData::toString(). Regardless, I do not like the direction of where we are going with SecurityOriginData. Both SecurityOrigin::toString() and SecurityOriginData::toString() are implementing some subset of the origin serialization algorithm.

[Chris Dumez]

(In reply to Daniel Bates from comment #11)
> (In reply to Chris Dumez from comment #10)
> > (In reply to Daniel Bates from comment #9)
> > > (In reply to youenn fablet from comment #8)
> > > > (In reply to Daniel Bates from comment #7)
> > > > > Comment on attachment 336641 [details]
> > > > > Patch
> > > > > 
> > > > > This patch is not right. SecurityOriginData::toString() is not equivalent to
> > > > > SecurityOrigin::toString(). We need to stop and think before transitioning
> > > > > any  any more WebCore code to SecurityOriginData. It is not safe to do so.
> > > > 
> > > > In that particular case, the URL is ws or wss, so I think this patch is
> > > > correct as per https://url.spec.whatwg.org/#concept-url-origin.
> > > 
> > > This is not the correct section of the spec. to reference.
> > > 
> > > Take m_url to be the parsed URL corresponding to ws://example.com. Let D be
> > > the result of SecurityOriginData::fromURL(m_url) and S be the result of
> > > SecurityOrigin::create(m_url). Then D.toString() = "ws://example.com:0" !=
> > > "ws://example.com" = S.toString().
> > 
> > Where is "ws://example.com:0" coming from? D.toString() would return
> > "ws://example.com" (as of last week end).
> > 
> 
> Oops, I was looking at an old revision. I see that
> <https://trac.webkit.org/changeset/229954/> fixed up
> SecurityOriginData::toString(). Regardless, I do not like the direction of
> where we are going with SecurityOriginData. Both SecurityOrigin::toString()
> and SecurityOriginData::toString() are implementing some subset of the
> origin serialization algorithm.

What is your alternative proposal here?

Note that there are more usage of SecurityOrigin::create() on non-main threads than we originally thought. Maybe we should just give up and make SecurityOrigin::create() safe to call from a background thread? What do you guys think?

[youenn fablet]

It seems to me that what we need is a thread safe way to serialize an origin directly from an URL. Maybe this is simpler to implement than a fully thread safe SecurityOrigin.

[Chris Dumez]

(In reply to youenn fablet from comment #13)
> It seems to me that what we need is a thread safe way to serialize an origin
> directly from an URL. Maybe this is simpler to implement than a fully thread
> safe SecurityOrigin.

We are saying the same thing, I never said anything about making SecurityOrigin (fully) thread safe. I just want to make it OK to call SecurityOrigin::create(url) from a background thread.

Similar to String. It is safe to construct a String on any thread. It is not safe to pass a string to another thread unless you isolatecopy.
We could do the same for SecurityOrigin.

[Daniel Bates]

(In reply to Chris Dumez from comment #10)
> > 
> > Disregarding the aforementioned correctness issue, this patch is
> > self-contradictory because it only "fixes" the usage of SecurityOrigin in
> > WebSocket::didReceiveMessage(). It does not fix other uses of
> > SecurityOrigin::create(m_url) in WebSocket::didReceiveBinaryData().
> I am still going though the assertion failures and fixing them as I find
> them.
> 

This is playing whack-a-mole with our security and thread safety. This does not seem like a good strategy that will lead to high confidence of correctness and quality software. This strategy assumes that we have perfect test coverage both covering all WebCore lines of code AND covering all possible test cases because we only hit an assertion failure if execution passes over the asserted code. I would hope we have test coverage to cover WebSocket::didReceiveBinaryData() being called from a background thread. I do not have the list of tests that cause assertion failures that you are working through (please file bugs instead of keeping them in your head or waiting for others to hit the assertion failures). According to <https://bugs.webkit.org/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&bug_status=RESOLVED&chfield=%5BBug%20creation%5D&chfieldfrom=2018-03-27&chfieldto=2018-03-28&email1=cdumez%40apple.com&emailreporter1=1&emailtype1=substring&list_id=3588091&query_format=advanced> I see only two bugs about assertion failures filed between yesterday and today:

(This bug) WebSocket::didReceiveMessage() may construct a SecurityOrigin object on a non-main thread
<https://bugs.webkit.org/show_bug.cgi?id=184068>

And

Thread safety issue in IDBFactory' shouldThrowSecurityException()
<https://bugs.webkit.org/show_bug.cgi?id=184064>

Instead I suggest we use the assertion failures are a guide, together with a strategy of grep'ing for all SecurityOrigin usage around the assert that failed (would have caught the issue in WebSocket::didReceiveBinaryData()), as well as grep'ing all of WebCore and auditing threaded code.

Filed bug #184125 to fix up WebSocket::didReceiveBinaryData().

[Chris Dumez]

(In reply to Daniel Bates from comment #15)
> (In reply to Chris Dumez from comment #10)
> > > 
> > > Disregarding the aforementioned correctness issue, this patch is
> > > self-contradictory because it only "fixes" the usage of SecurityOrigin in
> > > WebSocket::didReceiveMessage(). It does not fix other uses of
> > > SecurityOrigin::create(m_url) in WebSocket::didReceiveBinaryData().
> > I am still going though the assertion failures and fixing them as I find
> > them.
> > 
> 
> This is playing whack-a-mole with our security and thread safety. This does
> not seem like a good strategy that will lead to high confidence of
> correctness and quality software. This strategy assumes that we have perfect
> test coverage both covering all WebCore lines of code AND covering all
> possible test cases because we only hit an assertion failure if execution
> passes over the asserted code. I would hope we have test coverage to cover
> WebSocket::didReceiveBinaryData() being called from a background thread. I
> do not have the list of tests that cause assertion failures that you are
> working through (please file bugs instead of keeping them in your head or
> waiting for others to hit the assertion failures). According to
> <https://bugs.webkit.org/buglist.
> cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOP
> ENED&bug_status=RESOLVED&chfield=%5BBug%20creation%5D&chfieldfrom=2018-03-
> 27&chfieldto=2018-03-28&email1=cdumez%40apple.
> com&emailreporter1=1&emailtype1=substring&list_id=3588091&query_format=advanc
> ed> I see only two bugs about assertion failures filed between yesterday and
> today:
> 
> (This bug) WebSocket::didReceiveMessage() may construct a SecurityOrigin
> object on a non-main thread
> <https://bugs.webkit.org/show_bug.cgi?id=184068>
> 
> And
> 
> Thread safety issue in IDBFactory' shouldThrowSecurityException()
> <https://bugs.webkit.org/show_bug.cgi?id=184064>
> 
> Instead I suggest we use the assertion failures are a guide, together with a
> strategy of grep'ing for all SecurityOrigin usage around the assert that
> failed (would have caught the issue in WebSocket::didReceiveBinaryData()),
> as well as grep'ing all of WebCore and auditing threaded code.
> 
> Filed bug #184125 to fix up WebSocket::didReceiveBinaryData().

I have landed at least 5 patches already and still not done. SecurityOrigin is already almost safe to construct from a background thread and making it fully safe would likely be less work and avoid annoying refactorings (especially if you do not want to use SecurityOriginData in WebCore).

[youenn fablet]

Some thoughts.

From what I see, for serializing an origin from an URL, calling shouldTreatAsPotentiallyTrustworthy is not needed.
The serialization just needs to know a SecurityOriginData and a isUnique boolean.

We could have something like:
- A "String serializeURLOrigin(const URL&)" utility function.
- serializeURLOrigin and SecurityOrigin would use an underlying function taking a SecurityOriginData and a isUnique boolean.

As of the computation of the isUnique value from background threads, we could make shouldTreatAsUniqueOrigin thread safe.
It seems that http/https/ws/wss/ftp/data/blob could be special cased for efficiency.

Not sure about blobs though.
Maybe callOnMainThreadAndWait to create a temporary SecurityOrigin is good enough.

[Chris Dumez]

(In reply to youenn fablet from comment #17)
> Some thoughts.
> 
> From what I see, for serializing an origin from an URL, calling
> shouldTreatAsPotentiallyTrustworthy is not needed.
> The serialization just needs to know a SecurityOriginData and a isUnique
> boolean.
> 
> We could have something like:
> - A "String serializeURLOrigin(const URL&)" utility function.
> - serializeURLOrigin and SecurityOrigin would use an underlying function
> taking a SecurityOriginData and a isUnique boolean.
> 
> As of the computation of the isUnique value from background threads, we
> could make shouldTreatAsUniqueOrigin thread safe.
> It seems that http/https/ws/wss/ftp/data/blob could be special cased for
> efficiency.
> 
> Not sure about blobs though.
> Maybe callOnMainThreadAndWait to create a temporary SecurityOrigin is good
> enough.

this is not just about serialization. What if we need to know if an URL is cross origin synchronously from a background thread? Then we’d need canAccess() too.

[youenn fablet]

> this is not just about serialization. What if we need to know if an URL is
> cross origin synchronously from a background thread? Then we’d need
> canAccess() too.

Yeah... then it seems that making SecurityOrigin fully thread-safe seems like the safest approach.

FWIW, looking at CSP implementation, we might need to sync some SchemeRegistries to NetworkProcess. It seems natural then to try syncing all of them and make SecurityOrigin usable in NetworkProcess.

That would extend the scope of usage of SecurityOrigin, which would increase the risk for unnoticed usage of SecurityOrigin in background threads.
I think that for now it will be main thread only.

[Daniel Bates]

(In reply to Chris Dumez from comment #12)
> (In reply to Daniel Bates from comment #11)
> > (In reply to Chris Dumez from comment #10)
> > > (In reply to Daniel Bates from comment #9)
> > > > (In reply to youenn fablet from comment #8)
> > > > > (In reply to Daniel Bates from comment #7)
> > > > > > Comment on attachment 336641 [details]
> > > > > > Patch
> > > > > > 
> > > > > > This patch is not right. SecurityOriginData::toString() is not equivalent to
> > > > > > SecurityOrigin::toString(). We need to stop and think before transitioning
> > > > > > any  any more WebCore code to SecurityOriginData. It is not safe to do so.
> > > > > 
> > > > > In that particular case, the URL is ws or wss, so I think this patch is
> > > > > correct as per https://url.spec.whatwg.org/#concept-url-origin.
> > > > 
> > > > This is not the correct section of the spec. to reference.
> > > > 
> > > > Take m_url to be the parsed URL corresponding to ws://example.com. Let D be
> > > > the result of SecurityOriginData::fromURL(m_url) and S be the result of
> > > > SecurityOrigin::create(m_url). Then D.toString() = "ws://example.com:0" !=
> > > > "ws://example.com" = S.toString().
> > > 
> > > Where is "ws://example.com:0" coming from? D.toString() would return
> > > "ws://example.com" (as of last week end).
> > > 
> > 
> > Oops, I was looking at an old revision. I see that
> > <https://trac.webkit.org/changeset/229954/> fixed up
> > SecurityOriginData::toString(). Regardless, I do not like the direction of
> > where we are going with SecurityOriginData. Both SecurityOrigin::toString()
> > and SecurityOriginData::toString() are implementing some subset of the
> > origin serialization algorithm.
> 
> What is your alternative proposal here?
> 
> Note that there are more usage of SecurityOrigin::create() on non-main
> threads than we originally thought. Maybe we should just give up and make
> SecurityOrigin::create() safe to call from a background thread? What do you
> guys think?

Make SecurityOrigin thread-safe.

[Chris Dumez]

(In reply to Daniel Bates from comment #20)
> (In reply to Chris Dumez from comment #12)
> > (In reply to Daniel Bates from comment #11)
> > > (In reply to Chris Dumez from comment #10)
> > > > (In reply to Daniel Bates from comment #9)
> > > > > (In reply to youenn fablet from comment #8)
> > > > > > (In reply to Daniel Bates from comment #7)
> > > > > > > Comment on attachment 336641 [details]
> > > > > > > Patch
> > > > > > > 
> > > > > > > This patch is not right. SecurityOriginData::toString() is not equivalent to
> > > > > > > SecurityOrigin::toString(). We need to stop and think before transitioning
> > > > > > > any  any more WebCore code to SecurityOriginData. It is not safe to do so.
> > > > > > 
> > > > > > In that particular case, the URL is ws or wss, so I think this patch is
> > > > > > correct as per https://url.spec.whatwg.org/#concept-url-origin.
> > > > > 
> > > > > This is not the correct section of the spec. to reference.
> > > > > 
> > > > > Take m_url to be the parsed URL corresponding to ws://example.com. Let D be
> > > > > the result of SecurityOriginData::fromURL(m_url) and S be the result of
> > > > > SecurityOrigin::create(m_url). Then D.toString() = "ws://example.com:0" !=
> > > > > "ws://example.com" = S.toString().
> > > > 
> > > > Where is "ws://example.com:0" coming from? D.toString() would return
> > > > "ws://example.com" (as of last week end).
> > > > 
> > > 
> > > Oops, I was looking at an old revision. I see that
> > > <https://trac.webkit.org/changeset/229954/> fixed up
> > > SecurityOriginData::toString(). Regardless, I do not like the direction of
> > > where we are going with SecurityOriginData. Both SecurityOrigin::toString()
> > > and SecurityOriginData::toString() are implementing some subset of the
> > > origin serialization algorithm.
> > 
> > What is your alternative proposal here?
> > 
> > Note that there are more usage of SecurityOrigin::create() on non-main
> > threads than we originally thought. Maybe we should just give up and make
> > SecurityOrigin::create() safe to call from a background thread? What do you
> > guys think?
> 
> Make SecurityOrigin thread-safe.

Ok, then I think we are now all in agreement. Let's do this. It will be easier and less error-prone.

[Chris Dumez]

Reverted r230042 for reason:

It is no longer needed now that it is safe to construct a SecurityOrigin from an on-main thread

Committed r230305: <https://trac.webkit.org/changeset/230305>

[Chris Dumez]

No longer needed.