Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add option 'clear' mask #1988

Closed
wants to merge 2 commits into from
Closed

add option 'clear' mask #1988

wants to merge 2 commits into from

Conversation

e3dio
Copy link

@e3dio e3dio commented Dec 18, 2021

There was discussion here #1986 @ronag about disabling mask to improve client performance for cases when mask is not necessary, to stay semi-spec compliant can add option to use a clear mask to skip random number generation and skip XOR on all bits, and a server will not close the connection from a missing mask

improves client performance while spec compliant
@ronag
Copy link
Contributor

ronag commented Dec 18, 2021

XORing against 0 still mutates? No? I don’t think we can just skip the xoring.

@e3dio
Copy link
Author

e3dio commented Dec 18, 2021

A B A XOR B
0 0 0
0 1 1
1 0 1
1 1 0

@ronag
Copy link
Contributor

ronag commented Dec 18, 2021

Sorry. Momentary confusion. 😄

@lpinca
Copy link
Member

lpinca commented Dec 18, 2021

Sorry, but I'm comfortable with this. Using a fake mask is not spec compliant. It completely defats the point of masking.

@e3dio
Copy link
Author

e3dio commented Dec 18, 2021

Sorry, but I'm comfortable with this

Assuming not comfortable. It's easy enough to fork for special needs @ronag

@ronag
Copy link
Contributor

ronag commented Dec 18, 2021

Sorry, but I'm comfortable with this

Assuming not comfortable. It's easy enough to fork for special needs @ronag

Already done. Thanks!

@e3dio e3dio closed this Dec 18, 2021
@ronag
Copy link
Contributor

ronag commented Dec 18, 2021

Too bad uws and ws are on total opposite ends on this. 😞

@lpinca
Copy link
Member

lpinca commented Dec 18, 2021

We recently added the ability to skip UTF8 validation (#1928). This is similar. I understand that they might be desirable in a trusted environment but I don't like to keep adding them. It would also open the door for an option to accept unmasked frames on the server.

@e3dio
Copy link
Author

e3dio commented Dec 18, 2021

ws already has no mask option, best thing is use no mask send on client, and edit the uWS source to accept no mask option, he might accept PR if submit it, probably just as easy to add looking now https://github.com/uNetworking/uWebSockets/blob/master/src/WebSocketProtocol.h

@lpinca
Copy link
Member

lpinca commented Dec 18, 2021

For this specific use case a possible tradeoff would be to accept a user provided mask. The downside is that we can't skip masking unless we check that all bytes are zero.

@lpinca
Copy link
Member

lpinca commented Dec 18, 2021

I mean something like this:

const mask = Buffer.alloc(4);

ws.send(message, { mask });
ws.send(message, { mask });
ws.send(message, { mask });

@e3dio
Copy link
Author

e3dio commented Dec 18, 2021

A user provided mask is probably a good option, you can choose the level of masking you need. If you control all servers and network just use a 0 'clear' mask. If you want mask but don't need high-grade random numbers use Math.random() buf.writeUInt32BE(Math.random()*2**32). ws library can check for 0 'clear' mask on both send and receive to skip masking all bits when needed

image

@lpinca
Copy link
Member

lpinca commented Dec 18, 2021

FWIW ws switched from Math.random() to crypto.randomBytes() in 7253f06 as per #832.

I think the overhead is not a big problem on the client. I think I've never experienced entropy exhaustion.

@ghost
Copy link

ghost commented Dec 18, 2021

There is literally no reason to use random masks, even less so cryptographically secure masks. That only applies to web browsers running untrusted scripts. Any Node.js client should use the most simple mask, as it literally improves nothing whatsoever in terms of security to use cryptographically secure masks.

@e3dio
Copy link
Author

e3dio commented Dec 18, 2021

A Node.js server can assume to be secure, but it can send user generated input across the network, if the user knows the mask they can choose the bytes to send. The use case this PR is referring to is for sending from Server to Server across your own network, where there is zero reason for a mask at all, no reason to waste any CPU cycles on your machines generating crypto numbers and masking bits

@e3dio
Copy link
Author

e3dio commented Dec 18, 2021

Also using SSL is redundant so you can disable masking, might as well make it an option for those who want it

@ghost
Copy link

ghost commented Dec 18, 2021

Also using SSL is redundant so you can disable masking, might as well make it an option for those who want it

No. You can't just break spec. because you feel like it. Any message sent from a client without mask is invalid and will be refused.

What you can do, and what is still spec. compliant is to use a simple mask like a number that increments by 1 every time.

@e3dio
Copy link
Author

e3dio commented Dec 18, 2021

This PR uses a clear mask = 0 which allows skipping XOR all bits, or even better allow user supplied mask. Server will still be happy, but you could add a server check to skip unmask on mask = 0, save some CPU cycles

@ghost
Copy link

ghost commented Dec 18, 2021

Yes but the spec says the mask must change with every message. So to hit all the criteria of the spec you can use an incrementing 4 byte integer. That's certainly spec compliant in every single way (for non-web browsers).

@e3dio
Copy link
Author

e3dio commented Dec 18, 2021

The spec says sufficiently random numbers which means Crypto API, but the Spec is meant for web browsers, this is an option for advanced usage on non-browser, no one is forcing you to follow web browser spec exactly

@e3dio
Copy link
Author

e3dio commented Dec 18, 2021

There is nothing at all in the spec that says you MUST use cryptographically random masks. You're not really reading the spec. just making things up as you go

Have you looked at the spec at all ?

RFC 6455, page 33:

the masking key MUST be derived from a strong
source of entropy.. RFC 4086 [RFC4086] discusses what
entails a suitable source of entropy

RFC 4086 describes entropy sources as used in Crypto API.

The spec is for web browsers to safely implement websockets over cleartext public networks. We are talking about advanced usage for non-browsers over secure or private networks. Your web server will not be affected, it won't know any difference. However if you want to optimize you are welcome to check for mask = 0 to skip unmasking every byte in every message

@ghost
Copy link

ghost commented Dec 19, 2021

Btw, I found this in spec.

frame-masking-key = 4( %x00-FF )
; present only if frame-masked is 1
; 32 bits in length

So a mask of 0 is indeed allowed.

And again,

The unpredictability of the masking key is
essential to prevent authors of malicious applications from selecting
the bytes that appear on the wire
.

This goes out the window when net.Socket.write can be accessed to emit literally anything, anyways.

lpinca added a commit that referenced this pull request Dec 19, 2021
@e3dio
Copy link
Author

e3dio commented Dec 19, 2021

So a mask of 0 is indeed allowed

Might as well add a check on the server to skip unmask for that case, I don't know how much CPU it saves but it is some amount, especially if the client happens to send many of these

This goes out the window when net.Socket.write can be accessed

I don't think an attacker needs top level script access, they just need to supply the data that gets sent across the network, which a mask can prevent. But literally the only reason there is a mask is to protect broken http proxies that don't understand websocket protocol from a cache poison attack. If you use SSL there is no reason for mask, if you are on local network there is no reason for mask, if you are sending large amounts of messages I think you should have the option of sending a mask = 0, or a user supplied math.random etc

@ghost
Copy link

ghost commented Dec 19, 2021

I don't think an attacker needs top level script access

websockets/ws runs in Node.js (primarily?) so it doesn't matter if it so uses an Uranium based dedicated randomness module from the Greek gods - the fact you have net.Socket.write access entirely circumvents any security whatsoever from randomness.

@e3dio
Copy link
Author

e3dio commented Dec 19, 2021

Looks like @lpinca added a nice PR for user supplied mask #1989

lpinca added a commit that referenced this pull request Dec 19, 2021
@ronag
Copy link
Contributor

ronag commented Dec 19, 2021

websockets/ws runs in Node.js (primarily?) so it doesn't matter if it so uses an Uranium based dedicated randomness module from the Greek gods - the fact you have net.Socket.write access entirely circumvents any security whatsoever from randomness.

@alexhultman I guess running ws in a worker could improve that security part? From the client side.

lpinca added a commit that referenced this pull request Dec 19, 2021
The `generateMask` option specifies a function that can be used to
generate custom masking keys.

Refs: #1986
Refs: #1988
Refs: #1989
lpinca added a commit that referenced this pull request Dec 19, 2021
The `generateMask` option specifies a function that can be used to
generate custom masking keys.

Refs: #1986
Refs: #1988
Refs: #1989
lpinca added a commit that referenced this pull request Dec 20, 2021
The `generateMask` option specifies a function that can be used to
generate custom masking keys.

Refs: #1986
Refs: #1988
Refs: #1989
lpinca added a commit that referenced this pull request Dec 20, 2021
The `generateMask` option specifies a function that can be used to
generate custom masking keys.

Refs: #1986
Refs: #1988
Refs: #1989
@e3dio e3dio deleted the patch-1 branch December 20, 2021 17:59
lpinca added a commit that referenced this pull request Dec 20, 2021
The `generateMask` option specifies a function that can be used to
generate custom masking keys.

Refs: #1986
Refs: #1988
Refs: #1989
th37rose added a commit to th37rose/websocket_server that referenced this pull request May 24, 2022
The `generateMask` option specifies a function that can be used to
generate custom masking keys.

Refs: websockets/ws#1986
Refs: websockets/ws#1988
Refs: websockets/ws#1989
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants