From 9a60bf3726d3d42383842b9d99a69021ed3a5642 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Fri, 31 Jan 2014 16:49:24 +0400 Subject: [PATCH] tls: fix crash in SNICallback `tls_wrap.cc` was crashing in an `Unwrap` call, when non `SecureContext` object was passed to it. Check that the passed object is a `SecureContext` instance before unwrapping it. fix #7008 --- src/env.h | 1 + src/tls_wrap.cc | 34 ++++++++++++------- test/simple/test-tls-sni-option.js | 69 ++++++++++++++++++++++++++++++-------- 3 files changed, 77 insertions(+), 27 deletions(-) diff --git a/src/env.h b/src/env.h index 26630bb..68528d0 100644 --- a/src/env.h +++ b/src/env.h @@ -120,6 +120,7 @@ namespace node { V(should_keep_alive_string, "shouldKeepAlive") \ V(size_string, "size") \ V(smalloc_p_string, "_smalloc_p") \ + V(sni_context_err_string, "Invalid SNI context") \ V(sni_context_string, "sni_context") \ V(status_code_string, "statusCode") \ V(status_message_string, "statusMessage") \ diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 92febc1..4475f7b 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -747,29 +747,37 @@ void TLSCallbacks::SetServername(const FunctionCallbackInfo& args) { int TLSCallbacks::SelectSNIContextCallback(SSL* s, int* ad, void* arg) { - HandleScope scope(node_isolate); - TLSCallbacks* p = static_cast(arg); Environment* env = p->env(); const char* servername = SSL_get_servername(s, TLSEXT_NAMETYPE_host_name); - if (servername != NULL) { - // Call the SNI callback and use its return value as context - Local object = p->object(); - Local ctx = object->Get(env->sni_context_string()); + if (servername == NULL) + return SSL_TLSEXT_ERR_OK; - if (!ctx->IsObject()) - return SSL_TLSEXT_ERR_NOACK; + HandleScope scope(env->isolate()); + // Call the SNI callback and use its return value as context + Local object = p->object(); + Local ctx = object->Get(env->sni_context_string()); - p->sni_context_.Dispose(); - p->sni_context_.Reset(node_isolate, ctx); + // Not an object, probably undefined or null + if (!ctx->IsObject()) + return SSL_TLSEXT_ERR_NOACK; - SecureContext* sc = Unwrap(ctx.As()); - InitNPN(sc, p); - SSL_set_SSL_CTX(s, sc->ctx_); + Local cons = env->secure_context_constructor_template(); + if (!cons->HasInstance(ctx)) { + // Failure: incorrect SNI context object + Local err = Exception::TypeError(env->sni_context_err_string()); + p->MakeCallback(env->onerror_string(), 1, &err); + return SSL_TLSEXT_ERR_NOACK; } + p->sni_context_.Dispose(); + p->sni_context_.Reset(node_isolate, ctx); + + SecureContext* sc = Unwrap(ctx.As()); + InitNPN(sc, p); + SSL_set_SSL_CTX(s, sc->ctx_); return SSL_TLSEXT_ERR_OK; } #endif // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB diff --git a/test/simple/test-tls-sni-option.js b/test/simple/test-tls-sni-option.js index aaf37c7..7de7dea 100644 --- a/test/simple/test-tls-sni-option.js +++ b/test/simple/test-tls-sni-option.js @@ -47,10 +47,14 @@ var serverOptions = { // Just to test asynchronous callback setTimeout(function() { - if (credentials) - callback(null, crypto.createCredentials(credentials).context); - else + if (credentials) { + if (credentials.emptyRegression) + callback(null, {}); + else + callback(null, crypto.createCredentials(credentials).context); + } else { callback(null, null); + } }, 100); } }; @@ -63,6 +67,9 @@ var SNIContexts = { 'b.example.com': { key: loadPEM('agent3-key'), cert: loadPEM('agent3-cert') + }, + 'c.another.com': { + emptyRegression: true } }; @@ -89,39 +96,73 @@ var clientsOptions = [{ ca: [loadPEM('ca1-cert')], servername: 'c.wrong.com', rejectUnauthorized: false +}, { + port: serverPort, + key: loadPEM('agent3-key'), + cert: loadPEM('agent3-cert'), + ca: [loadPEM('ca1-cert')], + servername: 'c.another.com', + rejectUnauthorized: false }]; var serverResults = [], - clientResults = []; + clientResults = [], + serverErrors = [], + clientErrors = [], + serverError, + clientError; var server = tls.createServer(serverOptions, function(c) { serverResults.push(c.servername); }); +server.on('clientError', function(err) { + serverResults.push(null); + serverError = err.message; +}); + server.listen(serverPort, startTest); function startTest() { - function connectClient(options, callback) { + function connectClient(i, callback) { + var options = clientsOptions[i]; + clientError = null; + serverError = null; + var client = tls.connect(options, function() { clientResults.push( /Hostname\/IP doesn't/.test(client.authorizationError || '')); client.destroy(); - callback(); + next(); }); - }; - connectClient(clientsOptions[0], function() { - connectClient(clientsOptions[1], function() { - connectClient(clientsOptions[2], function() { - server.close(); - }); + client.on('error', function(err) { + clientResults.push(false); + clientError = err.message; + next(); }); + + function next() { + clientErrors.push(clientError); + serverErrors.push(serverError); + + if (i === clientsOptions.length - 1) + callback(); + else + connectClient(i + 1, callback); + } + }; + + connectClient(0, function() { + server.close(); }); } process.on('exit', function() { assert.deepEqual(serverResults, ['a.example.com', 'b.example.com', - 'c.wrong.com']); - assert.deepEqual(clientResults, [true, true, false]); + 'c.wrong.com', null]); + assert.deepEqual(clientResults, [true, true, false, false]); + assert.deepEqual(clientErrors, [null, null, null, "socket hang up"]); + assert.deepEqual(serverErrors, [null, null, null, "Invalid SNI context"]); }); -- 2.7.4