Last Comment Bug 660613 - WebSockets-07 implementation doesn't randomize all 32 bits of the mask
: WebSockets-07 implementation doesn't randomize all 32 bits of the mask
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: unspecified
: x86_64 Windows 7
: -- minor (vote)
: ---
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-30 05:09 PDT by Dirkjan Ochtman (:djc)
Modified: 2011-09-22 14:30 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed
fixed


Attachments
libpcap-websockets-valid-masks.pcal (12.58 KB, application/octet-stream)
2011-05-30 14:55 PDT, Patrick McManus [:mcmanus]
no flags Details
zerobits patch 1 (3.61 KB, patch)
2011-05-31 06:11 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
zerobits patch 2 (4.98 KB, text/plain)
2011-05-31 21:58 PDT, Patrick McManus [:mcmanus]
no flags Details
zerobits v3 (6.22 KB, patch)
2011-06-01 06:28 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
blizzard: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Dirkjan Ochtman (:djc) 2011-05-30 05:09:38 PDT
It seems that masks sent by Firefox 6.0a2 (2011-05-29) always include two bytes of entropy only, violating the current draft (-07) of the spec, which states:

"When preparing a masked frame, the client MUST pick a fresh masking key uniformly at random from the set of allowed 32-bit values."

Instead, masks my server has received from Firefox include these:

MASK ('\x00', '\x00', ']', '\x03')
MASK ('\x00', '\x00', 'z', 'Z')
MASK ('\x00', '\x00', 'E', '\t')
MASK ('\x00', '\x00', '\x12', '8')
MASK ('\x00', '\x00', 'V', '\xae')
MASK ('\x00', '\x00', '\x07', '2')
MASK ('\x00', '\x00', '\x01', ' ')
MASK ('\x00', '\x00', 'u', '\x9a')
Comment 1 Patrick McManus [:mcmanus] 2011-05-30 14:53:52 PDT
Hi Dirkjan, I'm afraid you'll need to provide more information - simply testing what masks we generate on a URL of my choosing makes this bug WFM. ( works for me).

Please see the attached packet capture - for a session with http://websocketstest.com/ - there are 2 websockets sessions during which 27 messages are generated from firefox. They use the following masks, which can be verified from the packet capture:

633ABB7F, 0A03E44D, 5DAC00BC, 3C0CB093, 4CCDCACB, 657585A0, 0A47F5A4,
6848FC94, 3D354353, 0BF62B5E, 2F4B3C4A, 0EA37387, 42CC3A8D, 2A7C4A76,
571836BE, 0607BA2A, 56AA0137, 5E34DCC5, 41584888, 61F93F69, 6B8C297F,
0D80A3CF, 3464063E, 7B16AB0C, 1E7A27F5, 200E3AC6, 06487E7D


> Instead, masks my server has received from Firefox include these:

your statement is a bit ambiguous. All 32 bit values are legal, so if you have a very large data set I would expect to see such values included.. if you only have a handful then it is indeed suspicious.

From your post to the IETF wg I know you are developing your own server. I would appreciate it if you could create a packet capture of the data before it comes into your server so we can verify independent of your logging what you are seeing.

If the issue persists, it would be helpful to have a URL that it can be produced against - as my standard test URL above as you see does not do this.
Comment 2 Patrick McManus [:mcmanus] 2011-05-30 14:55:43 PDT
Created attachment 536180 [details]
libpcap-websockets-valid-masks.pcal
Comment 3 Patrick McManus [:mcmanus] 2011-05-30 15:23:58 PDT
I reran my test on windows, my capture was from linux, and received a whole lot of zeroes in my masks. I don't know why that would differ, but I'll look into it.

thanks.
Comment 4 Patrick McManus [:mcmanus] 2011-05-30 15:33:50 PDT
(In reply to comment #3)

> lot of zeroes in my masks. I don't know why that would differ, but I'll look


apparently because RAND_MAX is not always 32 bits large.
Comment 5 Dirkjan Ochtman (:djc) 2011-05-30 23:37:59 PDT
Okay, so it would be conditional on the client being on Windows? I used a Windows 7 client during testing, so that would be consistent with my experience.

And yes, the masks collected above are not a small sample from all the masks I've seen; all the 20-50 masks I saw yesterday had two null bytes.
Comment 6 Patrick McManus [:mcmanus] 2011-05-31 05:58:53 PDT
(In reply to comment #5)
> Okay, so it would be conditional on the client being on Windows? I used a
> Windows 7 client during testing, so that would be consistent with my
> experience.

It varies by platform, but basically yes. Windows illustrated the problem more strongly than Linux and while I had done interoperability tests with windows I hadn't looked at the masks being selected on that platform. Thanks for running the aurora code!

(patch forthcoming)
Comment 7 Patrick McManus [:mcmanus] 2011-05-31 06:10:00 PDT
bug impact
-------------

The bug boils down to some fixed zero bits being used in two different random values - the nonce used for the cross-origin handshake, and the obfuscation mask used on uploaded payloads. There is no impact on interoperability, and otherwise a very minor impact is experienced as those values have no cryptographic properties anyhow.

The cross origin handshake is designed for the remote server to prove that it understands the handshake algorithm by manipulating the nonce with a cat and a hash. The fact that there are a few less significant bits in that nonce isn't going to change anything.

WRT the mask, according to the threat model the mask protects against it will allow the attacker to generate runs only 2 bytes long of predictable data. The IETF working group has already decideded that runs of 2, or 3 are not a threat when they chose not to apply the mask to the websockets framing information (which is influencable by the attacker). So the valid bits of the mask provide sufficient protection.
Comment 8 Patrick McManus [:mcmanus] 2011-05-31 06:11:20 PDT
Created attachment 536279 [details] [diff] [review]
zerobits patch 1
Comment 9 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-05-31 18:50:09 PDT
Let's just use PK11_GenerateRandom, instead of implementing a new, non-secure PRNG that is used in a security context. I think it is probably overkill, but I also doubt it would be a performance problem to do so.
Comment 10 Patrick McManus [:mcmanus] 2011-05-31 21:58:23 PDT
Created attachment 536527 [details]
zerobits patch 2

patch updated as brian suggests to use PK11_GenerateRandom (via nsIRandomGenerator).
Comment 11 Patrick McManus [:mcmanus] 2011-06-01 06:28:47 PDT
Created attachment 536588 [details] [diff] [review]
zerobits v3

interdiff is just removing what is now an out of date comment about rand()
Comment 12 Christopher Blizzard (:blizzard) 2011-06-02 14:48:41 PDT
This isn't severe enough to track for Firefox 6, but if you want an approval for the patch for Aurora for Firefox 6, please ask when you're ready and landed on m-c.  Thanks!
Comment 13 Christian :Biesinger (don't email me, ping me on IRC) 2011-06-02 15:50:25 PDT
Comment on attachment 536588 [details] [diff] [review]
zerobits v3

     *(((PRUint32 *)payload) - 1) = PR_htonl(mask);
-
+    
     LOG(("WebSocketHandler:: PrimeNewOutgoingMessage() "

please don't introduce trailing whitespace
Comment 14 Patrick McManus [:mcmanus] 2011-06-03 07:52:28 PDT
http://hg.mozilla.org/mozilla-central/rev/17ff112d2cba
Comment 15 Christopher Blizzard (:blizzard) 2011-06-09 14:40:53 PDT
Comment on attachment 536588 [details] [diff] [review]
zerobits v3

Marking + for Aurora (Firefox 6.)
Comment 16 Patrick McManus [:mcmanus] 2011-06-10 07:00:05 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/4cd2157b1ea8
Comment 17 Virgil Dicu [:virgil] [QA] 2011-08-22 04:20:19 PDT
Could you please provide a clear set of STRs in order to have this issue verified on QA side?
Comment 18 Patrick McManus [:mcmanus] 2011-08-22 05:59:44 PDT
(In reply to Virgil Dicu from comment #17)
> Could you please provide a clear set of STRs in order to have this issue
> verified on QA side?


The description should be sufficient when applied the the windows platform.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 14:30:16 PDT
qa-: no QA verification necessary

Note You need to log in before you can comment on or make changes to this bug.