WebSockets-07 implementation doesn't randomize all 32 bits of the mask

RESOLVED FIXED

Status

()

Core
Networking: WebSockets
--
minor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: djc, Assigned: mcmanus)

Tracking

unspecified
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox6- fixed, firefox7 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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')
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
Created attachment 536180 [details]
libpcap-websockets-valid-masks.pcal
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
(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.
(Reporter)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
(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)
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
Created attachment 536279 [details] [diff] [review]
zerobits patch 1
Assignee: nobody → mcmanus
Attachment #536279 - Flags: review?(cbiesinger)
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.
tracking-firefox6: --- → ?
tracking-firefox7: --- → ?
(Assignee)

Comment 10

6 years ago
Created attachment 536527 [details]
zerobits patch 2

patch updated as brian suggests to use PK11_GenerateRandom (via nsIRandomGenerator).
Attachment #536279 - Attachment is obsolete: true
Attachment #536279 - Flags: review?(cbiesinger)
Attachment #536527 - Flags: review?(cbiesinger)
(Assignee)

Comment 11

6 years ago
Created attachment 536588 [details] [diff] [review]
zerobits v3

interdiff is just removing what is now an out of date comment about rand()
Attachment #536527 - Attachment is obsolete: true
Attachment #536527 - Flags: review?(cbiesinger)
Attachment #536588 - Flags: review?(cbiesinger)
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!
tracking-firefox6: ? → -
tracking-firefox7: ? → ---
Comment on attachment 536588 [details] [diff] [review]
zerobits v3

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

please don't introduce trailing whitespace
Attachment #536588 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/mozilla-central/rev/17ff112d2cba
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Attachment #536588 - Flags: approval-mozilla-aurora?
Comment on attachment 536588 [details] [diff] [review]
zerobits v3

Marking + for Aurora (Firefox 6.)
Attachment #536588 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/4cd2157b1ea8
(Assignee)

Updated

6 years ago
status-firefox6: --- → fixed
status-firefox7: --- → fixed
Could you please provide a clear set of STRs in order to have this issue verified on QA side?
(Assignee)

Comment 18

6 years ago
(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.
qa-: no QA verification necessary
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.