Derived from Chromium bug: http://crbug.com/47390 WebSocket in a worker may send malformed handshake headers such as: Sec-WebSocket-Key1: !0 Sec-WebSocket-Key2: !0 where each key contains a space in its front (WebSocket specification explicitly forbids a leading or trailing space in WebSocket keys). There are two reasons why such a broken handshake is sent: - WebSocketHandshake has a bug to put a space in front of a key, and - Windows' rand_s() may fail due to Chromium's sandbox, and randomNumber (wtf/RandomNumber.h) does not check the failure and always returns zero.
Created attachment 59982 [details] Patch
Comment on attachment 59982 [details] Patch LGTM for WebSocketHandhshake.cpp change. I'm not sure it's ok to fallback to rand(). I believe sandbox should be fixed.
(In reply to comment #2) > (From update of attachment 59982 [details]) > LGTM for WebSocketHandhshake.cpp change. > I'm not sure it's ok to fallback to rand(). I believe sandbox should be fixed. Yes, Chromium's sandbox is the root cause. However, I think WebKit also needs to be prepared for rand_s() failure because it is explicitly stated that it may fail (http://msdn.microsoft.com/en-us/library/sxtz2fa8(v=VS.80).aspx). I may change my mind if other folks disagree on the randomNumber change, though.
I don't think it's OK for randomNumber() to sometimes return cryptographically strong numbers, and sometimes silently fall back to rand(). AFAIK, the main reason for having strong randomness in WebCore is preserving user privacy, as servers can track visitors by random parts of their requests. I'm not sure how to best address this. Maybe use arc4random, as on Darwin?
(In reply to comment #4) > I don't think it's OK for randomNumber() to sometimes return cryptographically strong numbers, and sometimes silently fall back to rand(). AFAIK, the main reason for having strong randomness in WebCore is preserving user privacy, as servers can track visitors by random parts of their requests. > > I'm not sure how to best address this. Maybe use arc4random, as on Darwin? Is arc4random available on Windows? I agree that silently falling back to a non-secure implementation is not a good way to go.
On a second thought, arc4random reads /dev/urandom, so it's probably affected by the same sandbox issues.
OK, I'm sold. I will create a patch that only fixes a WebSocket bug that inserts a space in an invalid position.
Note that we have found a way to workaround the sandbox: we need to call rand_s() at least once before entering the sandbox, otherwise it fails loading a DLL dynamically.
Created attachment 60085 [details] Patch
Comment on attachment 60085 [details] Patch LGTM
Comment on attachment 60085 [details] Patch r=me, but a stripWhiteSpace() call on the result would have looked less alarming (even if we're losing a tiny bit of randomness that way). > DEFINE_STATIC_LOCAL(String, randomChars, (randomCharacterInSecWebSocketKey)); > DEFINE_STATIC_LOCAL(String, spaceChar, (" ")); I'd have added String::insert(UChar) for this. But it's not new with this patch.
(In reply to comment #11) > (From update of attachment 60085 [details]) > r=me, but a stripWhiteSpace() call on the result would have looked less alarming (even if we're losing a tiny bit of randomness that way). The number of spaces is significant for calculating the challenge response. We should not remove them silently.
Comment on attachment 60085 [details] Patch Clearing flags on attachment: 60085 Committed r62163: <http://trac.webkit.org/changeset/62163>
All reviewed patches have been landed. Closing bug.
> The number of spaces is significant for calculating the challenge response. Yes, you are right - I didn't realize that "number" was also an output of the algorithm.