Skip to content
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

SIGSEGV When Reading From SSL Stream #11885

Closed
jcstanaway opened this issue Mar 16, 2017 · 5 comments
Closed

SIGSEGV When Reading From SSL Stream #11885

jcstanaway opened this issue Mar 16, 2017 · 5 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.

Comments

@jcstanaway
Copy link

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 mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem. labels Mar 16, 2017
@mscdex
Copy link
Contributor

mscdex commented Mar 16, 2017

/cc @nodejs/crypto

@jBarz
Copy link
Contributor

jBarz commented Mar 17, 2017

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

@bnoordhuis
Copy link
Member

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
Copy link
Member

#11898

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Mar 20, 2017
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: nodejs#11885
PR-URL: nodejs#11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@bnoordhuis
Copy link
Member

Fixed in 03a6c6e.

jungx098 pushed a commit to jungx098/node that referenced this issue Mar 21, 2017
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: nodejs#11885
PR-URL: nodejs#11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this issue Mar 21, 2017
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: nodejs#11885
PR-URL: nodejs#11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 18, 2017
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: #11885
PR-URL: #11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 18, 2017
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: #11885
PR-URL: #11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 19, 2017
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: #11885
PR-URL: #11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 19, 2017
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: #11885
PR-URL: #11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: nodejs/node#11885
PR-URL: nodejs/node#11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants