Bug 41327 – WebSocket: Malformed handshake headers in a worker due to rand_s failing

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yuta Kitamura]

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.

[Yuta Kitamura]

Created attachment 59982 [details]
Patch

[Fumitoshi Ukai]

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.

[Yuta Kitamura]

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

[Alexey Proskuryakov]

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?

[Adam Roben (:aroben)]

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

[Alexey Proskuryakov]

On a second thought, arc4random reads /dev/urandom, so it's probably affected by the same sandbox issues.

[Yuta Kitamura]

OK, I'm sold. I will create a patch that only fixes a WebSocket bug that inserts a space in an invalid position.

[Yuta Kitamura]

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.

[Yuta Kitamura]

Created attachment 60085 [details]
Patch

[Fumitoshi Ukai]

Comment on attachment 60085 [details]
Patch

LGTM

[Alexey Proskuryakov]

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.

[Yuta Kitamura]

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

[WebKit Commit Bot]

Comment on attachment 60085 [details]
Patch

Clearing flags on attachment: 60085

Committed r62163: <http://trac.webkit.org/changeset/62163>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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