Allow server to limit max frame length (avoid DOS and crash)
Closed this issue · 5 comments
The Hybi parser (in faye/websocket-driver-node) reads the length of the frame and immediately allocates a Buffer of that length.
It checks to make sure that the length is no more than MAX_LENGTH, which is 2^53 - 1 (the largest precisely representable JS integer), and rejects larger frames with a 1009 error before creating the new Buffer.
There are two problems here.
First, Node buffers have a max length: 1GB (0x3fffffff, https://github.com/joyent/node/blob/master/src/smalloc.h#L40). If you parse an incoming frame with length between 1GB and MAX_LENGTH, the parser will throw (and perhaps crash your whole server). Unless we expect Node to change its Buffer implementation any time soon (or you want to change the API to do something other than put an entire frame into a Buffer), MAX_LENGTH should probably default to the max Buffer length of 1GB.
Secondly, even 1GB is a huge amount! Most public-facing production websocket servers will probably want a smaller limit on the frame size: because the hybi parser allocates the buffer just based on the size field (not based on actually reading the full data over the socket), it would be easy to DOS most servers by sending lots of frames in parallel of length 1GB.
(The hixie draft parsers are not affected as much by this issue because they don't allocate a buffer until the entire contents have been read: you actually have to send 1GB of data in order to make the server allocate 1GB, whereas with hybi you just have to send a frame header to cause the allocation.)
A user of the low-level driver class can sort of do this: while it's not documented, you can do
var driver = websocketDriver.http(...);
driver.MAX_LENGTH = 10*1000*1000; // 10 MB max frame length
But a user of the higher-level faye/faye-websocket-node can't do this without being even more hacky:
var server = new FayeWebSocket(req, socket, body);
server._driver.MAX_LENGTH = 10*1000*100; // 10 MB max frame length
It would be great if there was an actual API (an option to the WebSocket constructor?) to set this max length field.
I'm happy to send a PR that implements this, if you let me know what the API should look like!
faye/websocket-driver-node#2 implements the first fix (prevents the driver from throwing).
I do additionally think it's important to allow the user of FayeWebsocket to lower the MAX_LENGTH, though.
+1
This issue you mention about pre-allocating a buffer on reading the length header has been mitigated in faye/websocket-driver-node@d93c853. We don't allocate the buffer until we know the read queue is full enough to complete the current message.
Great! I agree that this commit should fix the underlying problem.