WebSocket::didReceiveMessage() may construct a SecurityOrigin object on a non-main thread, which is not safe.
Created attachment 336637 [details] Patch
Created attachment 336641 [details] Patch
Comment on attachment 336641 [details] Patch Maybe we could assert in SecurityOriginData::toString that URL is http/https/ws/wss.
Comment on attachment 336641 [details] Patch Clearing flags on attachment: 336641 Committed r230042: <https://trac.webkit.org/changeset/230042>
All reviewed patches have been landed. Closing bug.
<rdar://problem/38969197>
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 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.
(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.
(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.
(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.
(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?
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.
(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.
(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().
(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).
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.
(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.
> 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.
(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.
(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.
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>
No longer needed.