Mosca crashes when connecting with TLS on MQTT Port

[psorowka]

By accident we have found critical behavior in Mosca:

when connecting with a TLS connection on the plain mqtt port, the whole broker simply crashes with error:

buffer.js:582
    throw new RangeError('Trying to access beyond buffer length');
          ^
RangeError: Trying to access beyond buffer length
    at checkOffset (buffer.js:582:11)
    at Buffer.readUInt8 (buffer.js:588:5)
    at BufferList.(anonymous function) [as readUInt8] (/home/psorowka/dev/cybus/other/mosca/node_modules/mqtt-connection/node_modules/mqtt-packet/node_modules/bl/bl.js:210:58)
    at Parser._parseConnect (/home/psorowka/dev/cybus/other/mosca/node_modules/mqtt-connection/node_modules/mqtt-packet/parser.js:176:33)
    at Parser._parsePayload (/home/psorowka/dev/cybus/other/mosca/node_modules/mqtt-connection/node_modules/mqtt-packet/parser.js:110:14)
    at Parser.parse (/home/psorowka/dev/cybus/other/mosca/node_modules/mqtt-connection/node_modules/mqtt-packet/parser.js:42:48)
    at DestroyableTransform.process [as _transform] (/home/psorowka/dev/cybus/other/mosca/node_modules/mqtt-connection/lib/parseStream.js:13:12)
    at DestroyableTransform.Transform._read (/home/psorowka/dev/cybus/other/mosca/node_modules/mqtt-connection/node_modules/through2/node_modules/readable-stream/lib/_stream_transform.js:184:10)
    at DestroyableTransform.Transform._write (/home/psorowka/dev/cybus/other/mosca/node_modules/mqtt-connection/node_modules/through2/node_modules/readable-stream/lib/_stream_transform.js:172:12)
    at doWrite (/home/psorowka/dev/cybus/other/mosca/node_modules/mqtt-connection/node_modules/through2/node_modules/readable-stream/lib/_stream_writable.js:237:10)

One-liner to reproduce this behavior (assuming standard mosca running on localhost):

require('mqtt').connect('mqtts://localhost:1883');

We will dig further into it and provide a PR if we have a fix. Meanwhile we wanted to share the bug in case somebody is quicker in spotting the solution.

[psorowka]

Update on this, I just realized that the behavior depends on the node version (on client side!!)

• we run mosca on node 0.10
• when setting up the client connection on node 0.10, the behavior is as expected, namely mosca keeps running and socket is hung up for the client
• when setting up the client connection on node 4, mosca crashes as described above

obviously this might be a mqttjs issue, but still shows a vulnerability of mosca allowing to crash the whole broker that should be resolved.

[psorowka]

Okay I have resolved the issue. The problem was actually not from mosca but from the mqtt-packet module.

I will file a PR with a fix in the next minutes.

So what happened? The TLS connect message that arrived at Mosca looked like this:

<Buffer 16 03 01 01 01 01 00 00 fd 03 03 00 09 9a 85 fc 19 19 fa 06 a4 8c a5 53 5d 6e c1 01 23 be 68 00 85 b9 0c 29 8b 24 87 30 09 e6 e4 00 00 80 c0 2f c0 2b ... >

Note the 16 at the start, this suggests that we have a regular CONNECT packet. The next byte 3 suggests a remaining length of the packet of 3 bytes which is obviously not true, but the implementation of the parser just checks if at least 3 bytes remain in the buffer before it starts processing the payload.

The parseConnect function of the parser now checks the next two bytes 01 01 which would describe the protocol identifier string length. Usually this would be 4 or 6 bytes, but here we have 257 bytes. So the next 257 bytes are consumed into a string and our parsing pointer is at buffer position 259. Unfortunately, the buffer only has 260 bytes in total. That is the next parsing steps produce an out of range exception..

Why did I observe no problem in node 0.10? funny enough, in this case the initial packet looked very similar but had a length of 314 bytes and thus did not produce an overflow.

We obviously have various problems here:

• in mqtt-packet
  • the parser does not check for plausibility when a string ought to be longer than the packet
  • the parser does not check for a valid protocol identifier string at all
  • the parser does not check (at some points) if the buffer has remaining length before parsing the next bytes
  • the parser does not catch any (further, unexpected) exceptions in order to safe the upper layers from running into bad behavior

on the other hand in mosca (or mqtt-connection) also some sort of exception catch might be worth implementing so that a single falsy packet doesn't get a chance to kill the whole broker.

I concentrated on the mqtt-packet fixing for now because I would love to get some feedback on how to generally harden mosca in this respect from @mcollina

[mcollina]

This should be fixed now by reinstalling.

Regarding the try-catch, I'm against it. try-catch might leave some resources in unknown state, and will inhibit us to know about this cases and fix the condition under which a packet is thrown.

[psorowka]

I think this assessment depends extremely on the application.

I consider applications that require high availability and don't feel comfortable in having such a fragility. In such a scenario though, of course I would implement some sort of exception notification.

The only agreeable argument from my side are potential leakage situations which are indeed absolutely unwanted. As far as I know this mainly happens when try/catching asynchronous execution, which would not be the case if we had it in the parser. But I am not an expert in this point and thus will follow your opinion.

Is there anything else - except try/catch - that might improve stability here?

[mcollina]

@psorowka a try catch might leave stuff in a bad situation, possibly leaking memory, file descriptors on anything related. If any exception happens in a node application, the safest thing you can do is to shut down gracefully and restart. The only true option are domains, but they have their own problems.

Take also into account that a parsing failed issue is the first one in years.

[psorowka]

Okay than let's have a look at "shut down gracefully and restart".

Can I rely on all sockets being closed when node exits on unhandled exception? In that case something like forever may be all I need, all clients are kicked out and may reconnect.

If that is not the case I would at least need one try/catch on the very top-level mosca layer which allows me to close all connections and exit.

[psorowka]

And by the way it was not the first complete broker crash we observed, but the first we could cleanly reproduce

[mcollina]

@psorowka exactly. The best way to handle this is to stop accepting new connections, close all clients, and restart.

@psorowka bugs happens :/. Please submit even non-reproducible stacktraces for crashes.

[mcollina]

@psorowka I think I solved this once and for all: https://github.com/mqttjs/mqtt-packet/blob/master/testRandom.js. I've found a couple more cases.

[psorowka]

@mcollina thought about that exact solution as well. Have you checked that against the non-patched version? "once and for all" is a strong statement though :)

anyway, thanks for the quick merge and the good job!

[mcollina]

@psorowka if you want to tweak that script and change it to discover more, please do :).