41327 2010-06-28 21:17:35 -0700 WebSocket: Malformed handshake headers in a worker due to rand_s failing 2010-06-30 08:47:43 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC Windows XP RESOLVED FIXED P2 Normal --- 1 yutak yutak abarth ap aroben commit-queue ukai oldest_to_newest 244067 0 yutak 2010-06-28 21:17:35 -0700 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. 244076 1 59982 yutak 2010-06-28 21:36:22 -0700 Created attachment 59982 Patch 244078 2 59982 ukai 2010-06-28 21:40:46 -0700 Comment on attachment 59982 Patch LGTM for WebSocketHandhshake.cpp change. I'm not sure it's ok to fallback to rand(). I believe sandbox should be fixed. 244085 3 yutak 2010-06-28 21:56:45 -0700 (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. 244332 4 ap 2010-06-29 09:50:41 -0700 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? 244339 5 aroben 2010-06-29 09:58:33 -0700 (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. 244350 6 ap 2010-06-29 10:17:39 -0700 On a second thought, arc4random reads /dev/urandom, so it's probably affected by the same sandbox issues. 244627 7 yutak 2010-06-29 22:37:10 -0700 OK, I'm sold. I will create a patch that only fixes a WebSocket bug that inserts a space in an invalid position. 244628 8 yutak 2010-06-29 22:39:50 -0700 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. 244629 9 60085 yutak 2010-06-29 22:43:10 -0700 Created attachment 60085 Patch 244630 10 60085 ukai 2010-06-29 22:50:19 -0700 Comment on attachment 60085 Patch LGTM 244641 11 60085 ap 2010-06-29 23:47:10 -0700 Comment on attachment 60085 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. 244643 12 yutak 2010-06-29 23:58:34 -0700 (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. 244664 13 60085 commit-queue 2010-06-30 01:00:42 -0700 Comment on attachment 60085 Patch Clearing flags on attachment: 60085 Committed r62163: <http://trac.webkit.org/changeset/62163> 244665 14 commit-queue 2010-06-30 01:00:48 -0700 All reviewed patches have been landed. Closing bug. 244792 15 ap 2010-06-30 08:47:43 -0700 > 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. 59982 2010-06-28 21:36:22 -0700 2010-06-29 22:43:05 -0700 Patch bug-41327-20100629133620.patch text/plain 3482 yutak ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCA0ZjU2ZWE0ZDU2ZGE0N2U0M2FhMGY4MjlkZDVmOWQzNDA2ZTBhOWIyLi5k NzA5MjgwZDQ0Nzk0MDM0NWE1NjgwNjk4MjE0MWZiMmVhODUxYjQ0IDEwMDY0NAotLS0gYS9KYXZh U2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0x LDMgKzEsMjAgQEAKKzIwMTAtMDYtMjggIFl1dGEgS2l0YW11cmEgIDx5dXRha0BjaHJvbWl1bS5v cmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRml4 IHJhbmRvbU51bWJlciBvbiBXaW5kb3dzLgorCisgICAgICAgIEN1cnJlbnRseSByYW5kb21OdW1i ZXIgb24gV2luZG93cyB1c2VzIHJhbmRfcygpIGlmIHBvc3NpYmxlLCBidXQgdGhpcyBmdW5jdGlv bgorICAgICAgICBtYXkgZmFpbCBpbiBzb21lIGNvbmRpdGlvbiAoZS5nLiBDaHJvbWl1bSdzIHNh bmRib3gpLiBJbiBzdWNoIGNhc2VzLCBpdCBhbHdheXMKKyAgICAgICAgcmV0dXJucyB6ZXJvIGFz IGEgcmVzdWx0LiBUaGlzIGJlaGF2aW9yIGlzIG5vdCBkZXNpcmVibGU7IHJhbmRvbU51bWJlciBz aG91bGQKKyAgICAgICAgZmFsbCBiYWNrIHRvIHVzaW5nIHJhbmQoKSBpZiByYW5kX3MoKSBoYXMg ZmFpbGVkLgorCisgICAgICAgIFdlYlNvY2tldDogTWFsZm9ybWVkIGhhbmRzaGFrZSBoZWFkZXJz IGluIGEgd29ya2VyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD00MTMyNworCisgICAgICAgICogd3RmL1JhbmRvbU51bWJlci5jcHA6CisgICAgICAgIChX VEY6OnJhbmRvbU51bWJlcik6CisKIDIwMTAtMDYtMjggIENhaW8gTWFyY2VsbyBkZSBPbGl2ZWly YSBGaWxobyAgPGNhaW8ub2xpdmVpcmFAb3BlbmJvc3NhLm9yZz4KIAogICAgICAgICBSZXZpZXdl ZCBieSBLZW5uZXRoIFJvaGRlIENocmlzdGlhbnNlbi4KZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRD b3JlL3d0Zi9SYW5kb21OdW1iZXIuY3BwIGIvSmF2YVNjcmlwdENvcmUvd3RmL1JhbmRvbU51bWJl ci5jcHAKaW5kZXggYjIwNjUyYmE2NzliNmU5ZjM2MjA2MzU3NDEzNDA4NmU0MzQwNjFmMy4uYjAx YzY0ZjJjNDEwMDcwNGNkZWZjYTcwZmE2Y2UxNjUwYzczZDMyMCAxMDA2NDQKLS0tIGEvSmF2YVNj cmlwdENvcmUvd3RmL1JhbmRvbU51bWJlci5jcHAKKysrIGIvSmF2YVNjcmlwdENvcmUvd3RmL1Jh bmRvbU51bWJlci5jcHAKQEAgLTYwLDkgKzYwLDExIEBAIGRvdWJsZSByYW5kb21OdW1iZXIoKQog ICAgIAogI2lmIENPTVBJTEVSKE1TVkMpICYmIGRlZmluZWQoX0NSVF9SQU5EX1MpCiAgICAgdWlu dDMyX3QgYml0czsKLSAgICByYW5kX3MoJmJpdHMpOwotICAgIHJldHVybiBzdGF0aWNfY2FzdDxk b3VibGU+KGJpdHMpIC8gKHN0YXRpY19jYXN0PGRvdWJsZT4oc3RkOjpudW1lcmljX2xpbWl0czx1 aW50MzJfdD46Om1heCgpKSArIDEuMCk7Ci0jZWxpZiBPUyhEQVJXSU4pCisgICAgaWYgKCFyYW5k X3MoJmJpdHMpKQorICAgICAgICByZXR1cm4gc3RhdGljX2Nhc3Q8ZG91YmxlPihiaXRzKSAvIChz dGF0aWNfY2FzdDxkb3VibGU+KHN0ZDo6bnVtZXJpY19saW1pdHM8dWludDMyX3Q+OjptYXgoKSkg KyAxLjApOworICAgIC8vIEZhbGwgdGhyb3VnaCBpZiByYW5kX3MgZmFpbHMuCisjZW5kaWYKKyNp ZiBPUyhEQVJXSU4pCiAgICAgdWludDMyX3QgYml0cyA9IGFyYzRyYW5kb20oKTsKICAgICByZXR1 cm4gc3RhdGljX2Nhc3Q8ZG91YmxlPihiaXRzKSAvIChzdGF0aWNfY2FzdDxkb3VibGU+KHN0ZDo6 bnVtZXJpY19saW1pdHM8dWludDMyX3Q+OjptYXgoKSkgKyAxLjApOwogI2VsaWYgT1MoVU5JWCkK ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg OTAwODM4NjM0OGQ3NDVlMzVhMjcxOTFlZTc5NTk3NmMxY2FiMTUwZS4uZTMxZDY2MzA0ZmYyNDVj MTQ4OTliODMxYTM2MTg0NTdhZmQxODkxMCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAxMC0wNi0yOCAgWXV0 YSBLaXRhbXVyYSAgPHl1dGFrQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXggU2VjLVdlYlNvY2tldEtleXsxLDJ9IGhlYWRl cnMuCisKKyAgICAgICAgQWNjb3JkaW5nIHRvIFdlYlNvY2tldCBzcGVjaWZpY2F0aW9uLCBhIHZh bHVlIG9mIFNlYy1XZWJTb2NrZXRLZXl7MSwyfSBoZWFkZXIKKyAgICAgICAgc2hvdWxkIG5vdCBz dGFydCBvciBlbmQgd2l0aCBhIHNwYWNlLgorCisgICAgICAgIFdlYlNvY2tldDogTWFsZm9ybWVk IGhhbmRzaGFrZSBoZWFkZXJzIGluIGEgd29ya2VyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD00MTMyNworCisgICAgICAgIE5vIG5ldyB0ZXN0cy4gQVNT RVJUIHNob3VsZCBjYXRjaCB0aGUgcHJvYmxlbS4KKworICAgICAgICAqIHdlYnNvY2tldHMvV2Vi U29ja2V0SGFuZHNoYWtlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OmdlbmVyYXRlU2VjV2ViU29j a2V0S2V5KToKKwogMjAxMC0wNi0yOCAgRGFuaWVsIEJhdGVzICA8ZGJhdGVzQHJpbS5jb20+CiAK ICAgICAgICAgVW5yZXZpZXdlZCwgYXR0ZW1wdCB0byBmaXggUXQgYm90cy4KZGlmZiAtLWdpdCBh L1dlYkNvcmUvd2Vic29ja2V0cy9XZWJTb2NrZXRIYW5kc2hha2UuY3BwIGIvV2ViQ29yZS93ZWJz b2NrZXRzL1dlYlNvY2tldEhhbmRzaGFrZS5jcHAKaW5kZXggYmExNDczMjUwMTAyM2E0ZjE0YzZk NTZhMWQwNTMzODhlOTMzMWU5ZS4uNzcxMTYwNDhhMjA5N2Q5ODlmNGMxZTlhMmFlNjM0ZTdiZjEy Yjk1MiAxMDA2NDQKLS0tIGEvV2ViQ29yZS93ZWJzb2NrZXRzL1dlYlNvY2tldEhhbmRzaGFrZS5j cHAKKysrIGIvV2ViQ29yZS93ZWJzb2NrZXRzL1dlYlNvY2tldEhhbmRzaGFrZS5jcHAKQEAgLTEw Nyw5ICsxMDcsMTEgQEAgc3RhdGljIHZvaWQgZ2VuZXJhdGVTZWNXZWJTb2NrZXRLZXkodWludDMy X3QmIG51bWJlciwgU3RyaW5nJiBrZXkpCiAgICAgfQogICAgIERFRklORV9TVEFUSUNfTE9DQUwo U3RyaW5nLCBzcGFjZUNoYXIsICgiICIpKTsKICAgICBmb3IgKHVpbnQzMl90IGkgPSAwOyBpIDwg c3BhY2U7IGkrKykgewotICAgICAgICBpbnQgcG9zID0gc3RhdGljX2Nhc3Q8aW50PihyYW5kb21O dW1iZXIoKSAqIHMubGVuZ3RoKCkgLSAxKSArIDE7CisgICAgICAgIGludCBwb3MgPSBzdGF0aWNf Y2FzdDxpbnQ+KHJhbmRvbU51bWJlcigpICogKHMubGVuZ3RoKCkgLSAxKSkgKyAxOwogICAgICAg ICBzLmluc2VydChzcGFjZUNoYXIsIHBvcyk7CiAgICAgfQorICAgIEFTU0VSVChzWzBdICE9ICcg Jyk7CisgICAgQVNTRVJUKHNbcy5sZW5ndGgoKSAtIDFdICE9ICcgJyk7CiAgICAga2V5ID0gczsK IH0KIAo= 60085 2010-06-29 22:43:10 -0700 2010-06-30 01:00:42 -0700 Patch bug-41327-20100630144308.patch text/plain 1614 yutak ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg OTgzMDZjYzVlYzc5MWJjNjg3MDU3NzVlYTY4ZTRmNmY0NzM2ODNlMi4uMjk5MGUxY2FhOTAyNWI4 YWNhMTI4NWNjODMwODNhZTU2MTU2OWUwNyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAxMC0wNi0yOSAgWXV0 YSBLaXRhbXVyYSAgPHl1dGFrQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXggU2VjLVdlYlNvY2tldEtleXsxLDJ9IGhlYWRl cnMuCisKKyAgICAgICAgQWNjb3JkaW5nIHRvIFdlYlNvY2tldCBzcGVjaWZpY2F0aW9uLCBhIHZh bHVlIG9mIFNlYy1XZWJTb2NrZXRLZXl7MSwyfSBoZWFkZXIKKyAgICAgICAgc2hvdWxkIG5vdCBz dGFydCBvciBlbmQgd2l0aCBhIHNwYWNlLgorCisgICAgICAgIFdlYlNvY2tldDogTWFsZm9ybWVk IGhhbmRzaGFrZSBoZWFkZXJzIGluIGEgd29ya2VyIGR1ZSB0byByYW5kX3MgZmFpbGluZworICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDEzMjcKKworICAg ICAgICBObyBuZXcgdGVzdHMuIEFTU0VSVCBzaG91bGQgY2F0Y2ggdGhlIHByb2JsZW0uCisKKyAg ICAgICAgKiB3ZWJzb2NrZXRzL1dlYlNvY2tldEhhbmRzaGFrZS5jcHA6CisgICAgICAgIChXZWJD b3JlOjpnZW5lcmF0ZVNlY1dlYlNvY2tldEtleSk6CisKIDIwMTAtMDYtMjkgIFpoZW55YW8gTW8g IDx6bW9AZ29vZ2xlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBEaW1pdHJpIEdsYXprb3Yu CmRpZmYgLS1naXQgYS9XZWJDb3JlL3dlYnNvY2tldHMvV2ViU29ja2V0SGFuZHNoYWtlLmNwcCBi L1dlYkNvcmUvd2Vic29ja2V0cy9XZWJTb2NrZXRIYW5kc2hha2UuY3BwCmluZGV4IGJhMTQ3MzI1 MDEwMjNhNGYxNGM2ZDU2YTFkMDUzMzg4ZTkzMzFlOWUuLjc3MTE2MDQ4YTIwOTdkOTg5ZjRjMWU5 YTJhZTYzNGU3YmYxMmI5NTIgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvd2Vic29ja2V0cy9XZWJTb2Nr ZXRIYW5kc2hha2UuY3BwCisrKyBiL1dlYkNvcmUvd2Vic29ja2V0cy9XZWJTb2NrZXRIYW5kc2hh a2UuY3BwCkBAIC0xMDcsOSArMTA3LDExIEBAIHN0YXRpYyB2b2lkIGdlbmVyYXRlU2VjV2ViU29j a2V0S2V5KHVpbnQzMl90JiBudW1iZXIsIFN0cmluZyYga2V5KQogICAgIH0KICAgICBERUZJTkVf U1RBVElDX0xPQ0FMKFN0cmluZywgc3BhY2VDaGFyLCAoIiAiKSk7CiAgICAgZm9yICh1aW50MzJf dCBpID0gMDsgaSA8IHNwYWNlOyBpKyspIHsKLSAgICAgICAgaW50IHBvcyA9IHN0YXRpY19jYXN0 PGludD4ocmFuZG9tTnVtYmVyKCkgKiBzLmxlbmd0aCgpIC0gMSkgKyAxOworICAgICAgICBpbnQg cG9zID0gc3RhdGljX2Nhc3Q8aW50PihyYW5kb21OdW1iZXIoKSAqIChzLmxlbmd0aCgpIC0gMSkp ICsgMTsKICAgICAgICAgcy5pbnNlcnQoc3BhY2VDaGFyLCBwb3MpOwogICAgIH0KKyAgICBBU1NF UlQoc1swXSAhPSAnICcpOworICAgIEFTU0VSVChzW3MubGVuZ3RoKCkgLSAxXSAhPSAnICcpOwog ICAgIGtleSA9IHM7CiB9CiAK