crypto: never store pointer to conn in SSL_CTX

indutny · indutny · commit cb4ed3c78fd6 · 2014-09-18T02:31:47.000+04:00
SSL_CTX is shared between multiple connections and is not a right place to store per-connection data. fix nodejs#8348 Reviewed-By: Trevor Norris
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -120,8 +120,7 @@ X509_STORE* root_cert_store;
 template class SSLWrap<TLSCallbacks>;
 template void SSLWrap<TLSCallbacks>::AddMethods(Environment* env,
                                                 Handle<FunctionTemplate> t);
-template void SSLWrap<TLSCallbacks>::InitNPN(SecureContext* sc,
-                                             TLSCallbacks* base);
+template void SSLWrap<TLSCallbacks>::InitNPN(SecureContext* sc);
 template SSL_SESSION* SSLWrap<TLSCallbacks>::GetSessionCallback(
     SSL* s,
     unsigned char* key,
@@ -1013,26 +1012,21 @@ void SSLWrap<Base>::AddMethods(Environment* env, Handle<FunctionTemplate> t) {
 
 
 template <class Base>
-void SSLWrap<Base>::InitNPN(SecureContext* sc, Base* base) {
-  if (base->is_server()) {
+void SSLWrap<Base>::InitNPN(SecureContext* sc) {
 #ifdef OPENSSL_NPN_NEGOTIATED
-    // Server should advertise NPN protocols
-    SSL_CTX_set_next_protos_advertised_cb(sc->ctx_,
-                                          AdvertiseNextProtoCallback,
-                                          base);
+  // Server should advertise NPN protocols
+  SSL_CTX_set_next_protos_advertised_cb(sc->ctx_,
+                                        AdvertiseNextProtoCallback,
+                                        NULL);
+  // Client should select protocol from list of advertised
+  // If server supports NPN
+  SSL_CTX_set_next_proto_select_cb(sc->ctx_, SelectNextProtoCallback, NULL);
 #endif  // OPENSSL_NPN_NEGOTIATED
-  } else {
-#ifdef OPENSSL_NPN_NEGOTIATED
-    // Client should select protocol from list of advertised
-    // If server supports NPN
-    SSL_CTX_set_next_proto_select_cb(sc->ctx_, SelectNextProtoCallback, base);
-#endif  // OPENSSL_NPN_NEGOTIATED
-  }
 
 #ifdef NODE__HAVE_TLSEXT_STATUS_CB
   // OCSP stapling
   SSL_CTX_set_tlsext_status_cb(sc->ctx_, TLSExtStatusCallback);
-  SSL_CTX_set_tlsext_status_arg(sc->ctx_, base);
+  SSL_CTX_set_tlsext_status_arg(sc->ctx_, NULL);
 #endif  // NODE__HAVE_TLSEXT_STATUS_CB
 }
 
@@ -1688,7 +1682,7 @@ int SSLWrap<Base>::AdvertiseNextProtoCallback(SSL* s,
                                               const unsigned char** data,
                                               unsigned int* len,
                                               void* arg) {
-  Base* w = static_cast<Base*>(arg);
+  Base* w = static_cast<Base*>(SSL_get_app_data(s));
   Environment* env = w->env();
   HandleScope handle_scope(env->isolate());
   Context::Scope context_scope(env->context());
@@ -1714,7 +1708,7 @@ int SSLWrap<Base>::SelectNextProtoCallback(SSL* s,
                                            const unsigned char* in,
                                            unsigned int inlen,
                                            void* arg) {
-  Base* w = static_cast<Base*>(arg);
+  Base* w = static_cast<Base*>(SSL_get_app_data(s));
   Environment* env = w->env();
   HandleScope handle_scope(env->isolate());
   Context::Scope context_scope(env->context());
@@ -1806,7 +1800,7 @@ void SSLWrap<Base>::SetNPNProtocols(const FunctionCallbackInfo<Value>& args) {
 #ifdef NODE__HAVE_TLSEXT_STATUS_CB
 template <class Base>
 int SSLWrap<Base>::TLSExtStatusCallback(SSL* s, void* arg) {
-  Base* w = static_cast<Base*>(arg);
+  Base* w = static_cast<Base*>(SSL_get_app_data(s));
   Environment* env = w->env();
   HandleScope handle_scope(env->isolate());
 
@@ -2122,7 +2116,7 @@ int Connection::SelectSNIContextCallback_(SSL *s, int *ad, void* arg) {
       if (secure_context_constructor_template->HasInstance(ret)) {
         conn->sniContext_.Reset(env->isolate(), ret);
         SecureContext* sc = Unwrap<SecureContext>(ret.As<Object>());
-        InitNPN(sc, conn);
+        InitNPN(sc);
         SSL_set_SSL_CTX(s, sc->ctx_);
       } else {
         return SSL_TLSEXT_ERR_NOACK;
@@ -2158,7 +2152,7 @@ void Connection::New(const FunctionCallbackInfo<Value>& args) {
   if (is_server)
     SSL_set_info_callback(conn->ssl_, SSLInfoCallback);
 
-  InitNPN(sc, conn);
+  InitNPN(sc);
 
 #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
   if (is_server) {
diff --git a/src/node_crypto.h b/src/node_crypto.h
@@ -188,7 +188,7 @@ class SSLWrap {
   inline bool is_waiting_new_session() const { return new_session_wait_; }
 
  protected:
-  static void InitNPN(SecureContext* sc, Base* base);
+  static void InitNPN(SecureContext* sc);
   static void AddMethods(Environment* env, v8::Handle<v8::FunctionTemplate> t);
 
   static SSL_SESSION* GetSessionCallback(SSL* s,
diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
@@ -186,7 +186,7 @@ void TLSCallbacks::InitSSL() {
   }
 #endif  // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
 
-  InitNPN(sc_, this);
+  InitNPN(sc_);
 
   if (is_server()) {
     SSL_set_accept_state(ssl_);
@@ -800,7 +800,7 @@ int TLSCallbacks::SelectSNIContextCallback(SSL* s, int* ad, void* arg) {
   p->sni_context_.Reset(env->isolate(), ctx);
 
   SecureContext* sc = Unwrap<SecureContext>(ctx.As<Object>());
-  InitNPN(sc, p);
+  InitNPN(sc);
   SSL_set_SSL_CTX(s, sc->ctx_);
   return SSL_TLSEXT_ERR_OK;
 }

Original file line number	Diff line number	Diff line change
`@@ -186,7 +186,7 @@ void TLSCallbacks::InitSSL() {`
`186`	`186`	`}`
`187`	`187`	`#endif // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB`
`188`	`188`
`189`		`- InitNPN(sc_, this);`
	`189`	`+ InitNPN(sc_);`
`190`	`190`
`191`	`191`	`if (is_server()) {`
`192`	`192`	`SSL_set_accept_state(ssl_);`
`@@ -800,7 +800,7 @@ int TLSCallbacks::SelectSNIContextCallback(SSL* s, int* ad, void* arg) {`
`800`	`800`	`p->sni_context_.Reset(env->isolate(), ctx);`
`801`	`801`
`802`	`802`	`SecureContext* sc = Unwrap<SecureContext>(ctx.As<Object>());`
`803`		`- InitNPN(sc, p);`
	`803`	`+ InitNPN(sc);`
`804`	`804`	`SSL_set_SSL_CTX(s, sc->ctx_);`
`805`	`805`	`return SSL_TLSEXT_ERR_OK;`
`806`	`806`	`}`