Discussion: easy to trigger 'error' events on tls server connections, DoS vector || caveat emptor?

[rvagg]

This is something that's been floating around in email amongst the io.js TC for a little while now and we don't have a clear agreement on how we want to approach this or if we even want to do anything about it at all. So this is now open for public discussion and we'll be interested to hear feedback from @nodejs/collaborators and the new @nodejs/tsc.

While investigating #1595, @shigeki came across an interesting case where it's trivial to kill a tls server (not https) that's not well crafted.

server.js

var tls = require('tls');

var options = {
  key: "-----BEGIN RSA PRIVATE KEY-----\nMIICXAIBAAKBgQDKLvYmbfsgnZvQ+ndYK6F9qT0o2yrr+rC3g+hGwz3Lpfzc8UqY\nFdsMOhRsr0cPiGSlDnO+/RW4crYDIYUU3PuW+KMM3IgD7QHEaSxjoILiqS03eTJv\nUJQvwsrGuOb6iB7aAAeuOX0tMRXqBRDdNgH4lNiQ17VY6J8CsbcwEA/ssQIDAQAB\nAoGAEwNVjImdKxUHOSDEplr8BcgrgzMRFz3s7mUOK9Fid0s1u0VJoYG0prKzhwD4\nqsJFzKvOQLCmIUFQUn0NecfKNN5yJ8kijGTBqgW+TiTSZjGpja0x8gwrUQw/ji11\nS+CT1rtVlhztlkM1MzitaEjmmhu2/06TUkpgC2SuDvv9ya0CQQDWm+WjlmPXoAME\nQPyOGL/zxbAFIYM2wLBZ6JgHCwnz0FI6a+sfUcOshD9x2QIYDHT3Wcf68BhYefC2\n9dg6oRsHAkEA8S2RAhl65OeEhhhPY86TBn9bpBmX+zKHMgcs3dQTOjjgSFuGhKT8\nklcppULdeM7M1XwAKuC26wQ5HrVk3+j0hwJAXJwPv8UxNqZ1RsuocMVbaB3B9KTe\nk60ZLONDX56y38ThBxb08qH9F6BGJKHp1mjhvK/ArZgYWW3do4Z5letZ1wJBAOUk\n8QB4qw3vdjddw5hFfeWPfcdlVcQiFteOf69nvrjzrwywgRdoFl0IGZZd+ES+31j6\njsIubTzP72Fg4S3ojOMCQHTOx5F/hXoKmGcLkABAQfaFdZ0jnRAAX46FKqFv/BHA\nxtApaVJwxPOJbFUsTrOxlDLtG0jha4aOVMxv9uuRTeo=\n-----END RSA PRIVATE KEY-----",
  cert: "-----BEGIN CERTIFICATE-----\nMIIBsTCCARygAwIBAgIDAQABMAsGCSqGSIb3DQEBCzATMREwDwYDVQQDFgh0ZXN0\nLmNvbTAeFw03MDAxMDEwMDAwMDBaFw0yNTA1MDMxMjQ2MjhaMBMxETAPBgNVBAMW\nCHRlc3QuY29tMIGdMAsGCSqGSIb3DQEBAQOBjQAwgYkCgYEAyi72Jm37IJ2b0Pp3\nWCuhfak9KNsq6/qwt4PoRsM9y6X83PFKmBXbDDoUbK9HD4hkpQ5zvv0VuHK2AyGF\nFNz7lvijDNyIA+0BxGksY6CC4qktN3kyb1CUL8LKxrjm+oge2gAHrjl9LTEV6gUQ\n3TYB+JTYkNe1WOifArG3MBAP7LECAwEAAaMZMBcwFQYDVR0RBA4wDIIKKi50ZXN0\nLmNvbTALBgkqhkiG9w0BAQsDgYEAOtZx1MOYqgJHLJ+bsN7M/Aq/6x3RRhFxrbqP\nEFO4IzQIhKBdSXCBWVEZ4zquuHGUErBR8a9vPzKElRw0mUgXiAtRuMe8KpqEplhf\nM7pIm57Z1OhsIqUllYTdTXO7GP7NC3bvLKSffnnu2IRVuC3GSt8V4yJXbV9gO07U\nWyy7EXQ=\n-----END CERTIFICATE-----"
};

tls.createServer(options, function(clientSocket) {
}).listen(1443);

client.js

var net = require('net');
var tls = require('tls');
var socket = net.connect(1443);
var client = tls.connect({
 socket: socket,
 rejectUnauthorized: false
}, function() {
  socket.write('a'); // note the use of `socket` not `client`
  client.end();
});

i.e. by writing directly to the socket rather than the encrypted channel the client can force an 'error' event on the server connection (clientSocket, not the server itself).

Of course, if you simply handle the 'error' event on clientSocket then you catch it:

tls.createServer(options, function(clientSocket) {
  clientSocket.on('error', function() {})
}).listen(1443);

Some think that this may be classifiable as a DoS vulnerability, while others question whether this should even be considered a bug.

I think it comes down to how you view TLS as a "protocol". Is it like HTTP in that it sits on top of TCP and is a parsed and managed protocol that provides a higher level of abstraction to the user? Or is it closer to TCP in that it's a raw protocol and can be easily abused and you don't get many guarantees about protocol handling? The HTTP handling in io.js/Node.js provides a nice encapsulation of per-connection parsing, the client can do something wrong and move outside of the bounds of the protocol and it's just disconnected without that bubbling up to the server code. However with the current TLS API a client that isn't talking within the bounds of the protocol will cause an error that bubbles all the way up and if 'error' event is not handled on each connection it will bring down the entire process.

With HTTP you can do this:

$ node -e "require('http').createServer(function (req, res) { console.log('connect'); res.end('done') }).listen(8080)"

and then this:

$ echo -e 'GET /\nWUT' | nc localhost 8080

doesn't get through to the handler at all because the client is misbehaving and is therefore simply disconnected.

As an aside, @shigeki did the right thing with this, he had a concern that this could be abused in the real-world and took it directly to the TC to discuss before raising it publicly. We've had some vigorous discussions about this and haven't agreed that it should be categorised as a security concern but if we had then it would have been handled discreetly and a fix pushed out with a matter of urgency as well as filling a CVE for it. So, if you find something like this, please take it to email. We'll have a proper security@ email soon enough but for now just find someone you know and trust on the list of TC members and contact them.

[seishun]

I don't understand the title. It implies the 'error' event is emitted on the server instance, when they're clearly emitted on the connection instance. Or is that a suggested change?

[evilpacket]

This is a really good question and one that I struggle with. Here are my thoughts based on having to deal with this middle ground for modules.

For modules I take this approach.

Does the code make a promise of security stated or commonly accepted. If this promise can be broken in some way then it is in fact a vulnerability.

The other side of the coin is that code can be used improperly. For those we don't issue an advisory but provide a secure usage recommendation. (the result is usually documentation is updated to inform of secure usage).

It seems to me we have historical precedence based on HTTP that we will not blow up when encountering an error parsing the protocol and situations in which we will blow up are documented appropriately. Given what is a commonly accepted promise, if it's not documented that it will blow up in this situation then commonly it will be implemented in this fashion and would be considered an attack on availability creating a denial of service condition.

Lastly I think this one should follow security protocol and at least be labeled in the change log as a security impacting change (either if documentation is updated to say that we don't promise to not blow up on protocol violations or if we fix it to not blow up in this situation). That way people know to look at their code either way and make the decisions as to how to take action.

[silverwind]

Another way of handling that error:

tlsServer.on('clientError', function (err, conn) {
    conn.destroy();
});

What if we refrain from throwing these errors and push users to use these error handlers if they are interested in socket errors?

[indutny]

The main thing that I don't want to miss is the ability to handle error myself. Whatever the resolution of this issue may be, it should still be possible to have the current behavior.

[rvagg]

@seishun: you're quite right regarding the title, I've attempted to correct it

[SomeoneWeird]

I'm leaning towards more of a caveat emptor because although this is definitely a DoS vector if handled improperly.. I think it should be well known that not handling error events will cause some to bubble up, and eventually crash stuff. There should be a large warning on the TLS doc page advising users that stuff like this needs to be handled. We could also start treating TLS stuff like HTTP, as @rvagg said - and start handing errors like this transparent to the user. I also highly agree with @evilpacket's last paragraph.

[shigeki]

@rvagg Thanks for opening this issue and writing the detailed description. I appreciate your help very much.
I've just confirmed that net.Server also has the same issue. For example, it can be reproduced by receiving TCP RST from one client. I made a test by using https://github.com/BruceJillis/TCP-RESET-Injection and net.Server had an unhandled error as

$./iojs net_server.js
Listening
connect
events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: read ECONNRESET
    at exports._errnoException (util.js:846:11)
    at TCP.onread (net.js:540:26)

So this is not specific only to tls.Server and it is just easy to reproduce in user land for tls case.

As for the tls error, at the first time I missed to be aware that clientError on tls.Serve is emitted only before TLS handshake completed. The word of clientError is associated for me to that of https.Server and I thought all TLS errors would forward to it. I worry about that someone else has the same association as me.

net/tls.Server are different from http/https.Server but I think it is good for users that their behavior can be as much as closer as long as it does not cause any harm. To confirm this, I made patches
a015a61 for adding a new clientError event on net.Server and 440bd82 for tls.Server to forward all socket and tls errors in regardless of TLS handshake state.
I think these are more kind for users rather than adding caveats to the doc.

[brendanashworth]

Speaking as a user, I would expect node.js/io.js to prevent any single user from crashing an entire application by throwing the error. It is okay to emit it on the connection / socket, but it shouldn't bubble up.

I know that is how the EventEmitter works, but I would expect it to gracefully handle an error by a third party. I think if both examples of code on nodejs.org are vulnerable to unhandled exceptions, there is an issue.

[ChALkeR]

@rvagg As for this:

tls.createServer(options, function(clientSocket) {
  clientSocket.on('error', function() {})
}).listen(1443);

The same is applicable to net. I don't think that this is an issue in io.js code, but it could be better described in the docs.
For example here from.on('error', is also required for the server not to crash:

net.createServer(function(from) {
    from.on('error', function(e) {
        console.log(e);
    });
    ...
}).listen(port);

Though a clientError event on net.Server would be helpful.

[indutny]

I guess we don't have any resolution, are we?

[Fishrock123]

I'm for shelving this and just closing it. It's been out in the wild and no-one has complained, and we can't seem to decide.

[indutny]

+1

[rvagg]

+1

[evanlucas]

Closing due to the +1's above for closing it. If that is premature, please re-open and accept my apologies :]