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

Fix infinite recursion in Websocket parsers. #983

Merged
merged 1 commit into from Aug 6, 2012

Conversation

Projects
None yet
4 participants
@GICodeWarrior
Contributor

GICodeWarrior commented Aug 6, 2012

If a client is feeding messages faster than server can handle them, infinite
recursion occurs. Basically, the "overflow" data gets added to the parser and
it immediately parses a new message.

The fix pushes the processing of the next message (in this edge case) onto the
event queue. This prevents the stack from recursing indefinitely. This also
prevents a fast client from starving other clients.

Example Client:

'use strict';

var socket = require('socket.io-client').connect('http://localhost:8765');

socket.on('connect', function() {
    for (var i = 0; i < 5000; ++i) {
        socket.send('Howdy there!');
    }
});

Example Server:

'use strict';

var io = require('socket.io');
var sio = io.listen(8765);

var connectionCount = 0;
sio.sockets.on('connection', function(socket) {
    var messageCount = 0;
    console.log('Connection count: ', ++connectionCount);
    socket.on('disconnect', function() {
        console.log('Connection count: ', --connectionCount);
    });

    socket.on('message', function() {
        if (++messageCount % 100 === 0) {
            console.log('Message count (' + socket.id + '): ', ++messageCount);
        }
    });
});

Server output before patch:

$ node test5.js 
   info  - socket.io started
   debug - client authorized
   info  - handshake authorized fM1mVGYS6_AL49NykZbJ
   debug - setting request GET /socket.io/1/websocket/fM1mVGYS6_AL49NykZbJ
   debug - set heartbeat interval for client fM1mVGYS6_AL49NykZbJ
   debug - client authorized for 
   debug - websocket writing 1::
Connection count:  1
Message count (fM1mVGYS6_AL49NykZbJ):  1000
Message count (fM1mVGYS6_AL49NykZbJ):  2000
Message count (fM1mVGYS6_AL49NykZbJ):  3000

buffer.js:281
  pool = new SlowBuffer(Buffer.poolSize);
         ^
RangeError: Maximum call stack size exceeded

Server output after patch:

$ node test5.js 
   info  - socket.io started
   debug - client authorized
   info  - handshake authorized 7sUkDfi6mPK8ZIzHkLVX
   debug - setting request GET /socket.io/1/websocket/7sUkDfi6mPK8ZIzHkLVX
   debug - set heartbeat interval for client 7sUkDfi6mPK8ZIzHkLVX
   debug - client authorized for 
   debug - websocket writing 1::
Connection count:  1
Message count (7sUkDfi6mPK8ZIzHkLVX):  1000
Message count (7sUkDfi6mPK8ZIzHkLVX):  2000
Message count (7sUkDfi6mPK8ZIzHkLVX):  3000
Message count (7sUkDfi6mPK8ZIzHkLVX):  4000
Message count (7sUkDfi6mPK8ZIzHkLVX):  5000
Fix infinite recursion in Websocket parsers.
If a client is feeding messages faster than server can handle them, infinite
recursion occurs.  Basically, the "overflow" data gets added to the parser and
it immediately parses a new message.

The fix pushes the processing of the next message (in this edge case) onto the
event queue.  This prevents the stack from recursing indefinitely.  This also
prevents a fast client from starving other clients.

rauchg added a commit that referenced this pull request Aug 6, 2012

Merge pull request #983 from GICodeWarrior/parser-recursion
Fix infinite recursion in Websocket parsers.

@rauchg rauchg merged commit 8ca8990 into socketio:master Aug 6, 2012

@amasad

This comment has been minimized.

amasad commented Aug 13, 2012

I'm sorry maybe i'm getting this wrong. but we're facing the slow buffer issue in production. did this make it into 9.10 release. or is there a work around this?

@GICodeWarrior

This comment has been minimized.

Contributor

GICodeWarrior commented Aug 13, 2012

If you are sure it is recursion in the websocket implementation, you have a few options.

The above patch fixes the apparent issue and creates other issues (see 8ca8990).

You can swap out the websocket implementation with ws or websocket.io. This will fix the apparent issue without creating new issues.

However, there is an additional concern. If you are hitting the stack limit, that means there are many messages being received in a single Socket "data" event. By iterating over all the messages received in that event, node is blocked by that one client and cannot serve others until all the messages are parsed. All the websocket libraries I looked at are vulnerable to this issue.

I implemented a patch for WebSocket-Node that additionally defers processing of subsequent messages if processing blocks for too long. However, WebSocket-Node doesn't support Safari 5 or iOS (old websocket implementations).

My socket.io fork that integrates WebSocket-Node (my branch of it with patches).
https://github.com/GICodeWarrior/socket.io

Details on my patch for WebSocket-Node.
theturtle32/WebSocket-Node#59

@amasad

This comment has been minimized.

amasad commented Aug 13, 2012

So is it some sort of DOS?
I have socket.io running on 3 servers and at the same time all three servers would start throwing errors:

buffer.js:281
  pool = new SlowBuffer(Buffer.poolSize);
         ^
RangeError: Maximum call stack size exceeded

Also I'm running it in cluster implementation using the redis store.

@GICodeWarrior

This comment has been minimized.

Contributor

GICodeWarrior commented Aug 13, 2012

The result is a DOS, yes. It could be your deployed clients or a malicious client.

@rauchg

This comment has been minimized.

Contributor

rauchg commented Aug 13, 2012

@GICodeWarrior I'm going to apply ws on the next release.

@GICodeWarrior

This comment has been minimized.

Contributor

GICodeWarrior commented Aug 13, 2012

Good to know. That should fix the apparent issue for most people.

I have patches for WebSocket-Node that fix the blocking issue as well, so I will keep to my branch for now.

@einaros

This comment has been minimized.

Contributor

einaros commented Aug 13, 2012

@GICodeWarrior Deferred processing is an easy fix in this case. I'll add that to the next ws release.

@einaros

This comment has been minimized.

Contributor

einaros commented Aug 13, 2012

That said, deferred processing cannot deal with the real impact of many small packets. A client can (if nothing else interferes) send a maximum of ~21845 payloads per single data event. Deferring alone will still enable a client to quickly fill the queue of pending things to handle. A saner approach would be to define a (configurable) maximum payload count per tcp packet to something the server can trivially handle. Any venture beyond this limit will cause the client to be disconnected as it's dead certain to be misbehaving in some way.

What say you, @guille?

@GICodeWarrior

This comment has been minimized.

Contributor

GICodeWarrior commented Aug 13, 2012

The point is to limit the amount of time spent on each client. You certainly can't parse all the messages and toss them on the queue.

I parse frames and handle messages in a loop until a time limit is exceeded. At that point, the next processing run is deferred to the end of the queue. pause()/resume() are called to throttle the Socket as the buffer list of unprocessed data exceeds a limit.

This procedure limits the amount of memory buffered by a single connection, and it limits the amount of time a connection receives before other connections have a chance.

This is the patch I linked above.
theturtle32/WebSocket-Node#59

@einaros

This comment has been minimized.

Contributor

einaros commented Aug 13, 2012

@GICodeWarrior, it goes without saying that limiting each client's time is the point, but there will still be plenty of opportunity for an attacker to pass a wild amount of payloads per TCP frame, and thus take up quite a bit of CPU time (over time) for a relatively small amount of data sent from the client. For cloud hosting, that can become expensive.

Furthermore, in your patch, you use setTimeout. From what I've read in the past, that doesn't guarantee that all prior scheduled setTimeouts (for other clients which have been deferred), or even newly incoming frames, will run before the next loop through the malicious frame.

@GICodeWarrior

This comment has been minimized.

Contributor

GICodeWarrior commented Aug 13, 2012

I think as long as the library treats clients fairly, the case of a specific client using too much CPU is a problem for the library user to handle. The library user can always disconnect the client based on the criteria of the use case.

You may be confusing setTimeout() with process.nextTick() which is actually not intended for yielding execution.

What we really want is setImmediate(), but that only recently landed in 0.9 (since process.nextTick() is even more specific now).
nodejs/node-v0.x-archive#3845

If you have any questions about that, the folks in the libuv IRC channel are very helpful.

@rauchg

This comment has been minimized.

Contributor

rauchg commented Aug 13, 2012

ws implemented in master. Please give it a shot

@amasad

This comment has been minimized.

amasad commented Aug 14, 2012

Looks like it fixes the issue. but has it's own issues:

/home/ubuntu/.codex/supervisor/node_modules/socket.    io/lib/transports/websocket.js:33
  req.wsclient.onerror = function(){
                       ^
TypeError: Cannot set property 'onerror' of undefined
    at new WebSocket (/home/ubuntu/.    codex/supervisor/node_modules/socket.    io/lib/transports/websocket.js:33:24)
    at Manager.handleClient (/home/ubuntu/.    codex/supervisor/node_modules/socket.io/lib/manager    .js:674:19)
    at Manager.handleHTTPRequest (/home/ubuntu/.    codex/supervisor/node_modules/socket.io/lib/manager    .js:642:8)
    at Manager.handleRequest (/home/ubuntu/.    codex/supervisor/node_modules/socket.io/lib/manager    .js:601:12)
    at Server.<anonymous> (/home/ubuntu/.    codex/supervisor/node_modules/socket.io/lib/manager    .js:119:10)
    at Server.EventEmitter.emit (events.js:115:20)
    at HTTPParser.parser.onIncoming (http.js:1793:12)
    at HTTPParser.parserOnHeadersComplete [as     onHeadersComplete] (http.js:111:23)
    at Socket.socket.ondata (http.js:1690:22)
    at TCP.onread (net.js:410:27)

going to give @GICodeWarrior's fork a shot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment