SIGSEGV When Reading From SSL Stream

[jcstanaway]

• Version: v6.10.0
• Platform: Linux 962e864e64d8 4.6.3-coreos test: don't remove empty.txt on win32 #2 SMP Mon Jul 18 06:10:39 UTC 2016 x86_64 GNU/Linux
• Subsystem: tls_wrap.cc

NodeJS is consistently raising a SIGSEGV.

PID 14892 received SIGSEGV for address: 0x44
/usr/src/app/node_modules/segfault-handler/build/Release/segfault-handler.node(+0x1a5b)[0x7f8023271a5b]
/lib/x86_64-linux-gnu/libpthread.so.0(+0xf890)[0x7f8025831890]
node(SSL_get_shutdown+0x1)[0x7d8d41]
node(_ZN4node7TLSWrap8ClearOutEv+0x16e)[0x115398e]
node(_ZN4node7TLSWrap10OnReadImplElPK8uv_buf_t14uv_handle_typePv+0xa0)[0x11540a0]
node(_ZN4node10StreamWrap6OnReadEP11uv_stream_slPK8uv_buf_t+0x88)[0x1119228]
node[0x137051f]
node[0x1370b90]
node[0x13765f8]
node(uv_run+0x154)[0x1366a94]
node(_ZN4node5StartEiPPc+0x328)[0x10d7788]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5)[0x7f8025498b45]
node[0x7a9aa8]

I can't share the code that reproduces this, but have at least partially tracked down the source of the issue. It occurs in a container that runs a test application testing various other nodejs applications in other containers.

In src/tls_wrap.cc, in TLSWrap::ClearOut(), it initially checks if ssl_ is null. If not, it continues on to read data from the stream. It enters the forever loop which calls SSL_read(). SSL_read() assumes that it is provided a non-null pointer to the SSL stream. The first time through the forever loop, this is safe. However, when this loops, it is not always true that ssl_ is not null. In my scenario, a second time through the loop, ssl_ was null and the subsequent dereference attempt in SSL_read() causes the SIGSEGV.

I've made two changes:

1. Prior to each call to SSL_read() within the forever loop, check if ssl_ is null. If so, break out of the loop. Simplying returning from here caused other issues.
2. Following the forever, there is a call to SSL_get_shutdown(). This call can't be made if ssl_ is null.

The diff for the changes is below and is against the v6.10.1-proposal branch.

diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index d1b1aec..1e0efbe 100644
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -409,6 +409,10 @@ void TLSWrap::ClearOut() {
   char out[kClearOutChunkSize];
   int read;
   for (;;) {
+    if (ssl_ == nullptr) {
+      break;
+    }
+
     read = SSL_read(ssl_, out, sizeof(out));

     if (read <= 0)
@@ -430,7 +434,7 @@ void TLSWrap::ClearOut() {
     }
   }

-  int flags = SSL_get_shutdown(ssl_);
+  int flags = (ssl_ == nullptr) ? true : SSL_get_shutdown(ssl_);
   if (!eof_ && flags & SSL_RECEIVED_SHUTDOWN) {
     eof_ = true;
     OnRead(UV_EOF, nullptr);

Neither SSL_read() or SSL_get_shutdown() check if the passed parameter is null and so when null is passed, a SIGSEGV occurs.

These two changes appear to be sufficient to avoid the SIGSEGV and my test application has been able to continuously run without issue. However, I wasn't been able to track down when/where ssl_ becomes null and if the above changes are complete & sufficient. Experts required - I'm not sufficiently confident that the changes are complete & sufficient to submit a pull request.

Note that while similar to issue #8074, it is not the same. I had applied the changes suggested by pull request #11776 (against v6.10.1-proposal), but that did not prevent the SIGSEGV.

There appears to be a lot of places where pointers are not being checked. Unfortunately, this appears all over in the OpenSSL dependency (as explicitly seen with SSL_read() and SSL_get_shutdown()).

[mscdex]

/cc @nodejs/crypto

[jBarz]

It is possible that OnRead triggers a callback that closes the socket (thus "destroySSL").

[bnoordhuis]

It's near-impenetrable spaghetti code but yes, it looks TLSWrap::OnRead() (very) indirectly calls StreamBase::EmitData(), which then invokes the .onread method on the JS object.

I'll see if I can find a way to reproduce and come up with a fix.

[bnoordhuis]

#11898

[bnoordhuis]

Fixed in 03a6c6e.