With mis-matched keys, node swallows openssl error on request #2308

georgesnelling · 2011-12-09T16:57:02Z

Discussion topic: https://groups.google.com/d/topic/nodejs/pBexLbwHMDE/discussion

Ok, it took some spelunking, but it looks like this was fixed between 4.11 and 4.12, possibly after the 5.x branch, but the fix did not make it back into main line.

Here's a test case. Sorry if this is a bit clumsy -- I couldn't see anything in assert that would help me check if the server wrote something to stderr, but I admit I didn't look too hard.

I'll post the necessary cert and key files to run the repro in just a minute. With those files in the same directory,

node test.js, where test.js is:

var https = require('https');
var fs = require('fs');
var port = 8043;

function simpleTest(keyFileName, cb) {

console.error("Running test with " + keyFileName);

var options = {
port: port,
key: fs.readFileSync(keyFileName),
cert: fs.readFileSync('./good.crt')
}

var server = https.createServer(options, function(req, res) {
res.shouldKeepAlive = false; // so that server.close() will work.
res.end("Received secure hello using " + req.url + "\n");
});

server.listen(port);

//
// putting keyFileName on the path simply as a convenient place to stash it for
// roundtripping. It is not used as a path
//
var req = https.request({ method: 'GET', path: '/' + keyFileName, port: port }, function(res) {

res.setEncoding('utf8');

res.on('data', function(data) {
  console.log(data);
});

res.on('end', function() {
  server.close(); // this is a one-request server
});

});

req.end();

req.on('error', function(err) {
console.error('Https.get error:');
console.error(err);
server.close();
});

server.on('close', cb); // server finished closing, call back

}

// Run the test first with the good key, then with the bad key
simpleTest('good.key', function() {
simpleTest('bad.key', function() {
console.error('Test Complete');
});
});

Here's my system info:
~ $ uname -a
Darwin gsimac.local 10.8.0 Darwin Kernel Version 10.8.0: Tue Jun 7 16:33:36 PDT 2011; root:xnu-1504.15.3~1/RELEASE_I386 i386

With various versions of node, here are the results:

Results with 0.4.11: (bad)

Running test with good.key
Received secure hello using /good.key

Running test with bad.key
Https.get error:
{ stack: [Getter/Setter],
  arguments: undefined,
  type: undefined,
  message: 'socket hang up' }
Test Complete

//
// the line that begins (node SSL) is the message from openssl saying there is a cert problem
// I think it is correct behavior for this to dump to stderr
//
Results with 0.4.12 (good)

Running test with good.key
Received secure hello using /good.key

Running test with bad.key
(node SSL) error:1408A0C1:SSL routines:SSL3_GET_CLIENT_HELLO:no shared cipher
Https.get error:
{ stack: [Getter/Setter],
  arguments: undefined,
  type: undefined,
  message: 'socket hang up' }
Test Complete

Results with 0.6.5: (bad)

Running test with good.key
Received secure hello using /good.key

Running test with bad.key
Https.get error:
{ [Error: socket hang up] code: 'ECONNRESET' }
Test Complete

Will follow in just a sec with link to files.

The text was updated successfully, but these errors were encountered:

georgesnelling · 2011-12-09T17:04:37Z

Keys, cert and test source are here: https://github.com/georgesnelling/Test/tree/master/nodejs.2308

Or grab the whole bundle here: https://github.com/georgesnelling/Test/blob/master/nodejs.2308/nodejs.2308.gz

georgesnelling · 2011-12-09T18:35:46Z

Whoops, here's the test case with better formatting for md:

/*
 * Test case for https://github.com/joyent/node/issues/2308
 * Requires the files good.key, bad.key, and good.crt from 
 * https://github.com/georgesnelling/Test/tree/master/nodejs.2308
 * to be in the same folder as this file.  
 *
 * To run:  node test.js
*/


var https = require('https');
var fs = require('fs');
var port = 8043;

function simpleTest(keyFileName, cb) {

  console.error("Running test with " + keyFileName);

  var options = {
    port: port,
    key: fs.readFileSync(keyFileName),
    cert: fs.readFileSync('./good.crt')
  }

  var server = https.createServer(options, function(req, res) {
    res.shouldKeepAlive = false;  // so that server.close() will work.
    res.end("  Received secure hello using " + req.url + "\n");
  });

  server.listen(port);

  //
  // putting keyFileName on the path simply as a convenient place to stash for 
  // roundtripping. It is not used as a path
  //
  var req = https.request({ method: 'GET', path: '/' + keyFileName, port: port }, function(res) {

    res.setEncoding('utf8');

    res.on('data', function(data) {
      console.log(data);
    });

    res.on('end', function() {
      server.close(); // this is a one-request server
    });

  });

  //
  // When request end fires on the bad.key test run, the server should dump the following
  // line (or similar) to stderr:
  //
  //    (node SSL) error:1408A0C1:SSL routines:SSL3_GET_CLIENT_HELLO:no shared cipher
  //
  // If it does not, that is the bug
  //
  req.end();

  req.on('error', function(err) {
    console.error('Https.get error:');
    console.error(err);
    server.close();
  });

  server.on('close', cb); // server finished closing, call back

}

// Run the test first with the good key, then with the bad key
simpleTest('good.key', function() {
  simpleTest('bad.key', function() {
    console.error('Test Complete');
  });
});

koichik · 2011-12-17T17:51:02Z

Can someone review koichik/node@29b1fdd?
Because the error (SSL3_GET_CLIENT_HELLO:no shared cipher) is happened on the server side, this patch introduces 'clientError' event to tls.Server like http.Server.
The result of test/simple/test-tls-invalid-key.js:

$ ./node test/simple/test-tls-invalid-key.js 
DEBUG: Server: Error: 139895985051456:error:1408A0C1:SSL routines:SSL3_GET_CLIENT_HELLO:no shared cipher:s3_srvr.c:1073:

DEBUG: Client: Error: socket hang up

@georgesnelling: Thanks for the report.

Results with 0.4.12 (good)

Actually, it is just debug print :)

koichik · 2011-12-21T19:05:42Z

@georgesnelling Thanks for the report, fixed in 07c27e0.

Fixes nodejs#2308. Fixes nodejs#2246.

* Upgrade V8 to 3.6.6.15 * Upgrade npm to 1.1.0-beta-10 (isaacs) * many doc updates (Ben Noordhuis, Jeremy Martin, koichik, Dave Irvine, Seong-Rak Choi, Shannen, Adam Malcontenti-Wilson, koichik) * nodejs#2438 segfault in node v0.6.6 * dgram, timers: fix memory leaks (Ben Noordhuis, Yoshihiro Kukuchi) * repl: fix repl.start not passing the `ignoreUndefined` arg (Damon Oehlman) * nodejs#1980: Socket.pause null reference when called on a closed Stream (koichik) * nodejs#2263: XMLHttpRequest piped in a writable file stream hang (koichik) * nodejs#2069: http resource leak (koichik) * buffer.readInt global pollution fix (Phil Sung) * timers: fix performance regression (Ben Noordhuis) * nodejs#2308, nodejs#2246: node swallows openssl error on request (koichik) * nodejs#2114: timers: remove _idleTimeout from item in .unenroll() (James Hartig) * nodejs#2379: debugger: Request backtrace w/o refs (Fedor Indutny) * simple DTrace ustack helper (Dave Pacheco) * crypto: rewrite HexDecode without snprintf (Roman Shtylman) * crypto: add SecureContext.clearOptions() method (Ben Noordhuis) * crypto: don't ignore DH init errors (Ben Noordhuis)

* V8 hash collision fix (Breaks MIPS) (Bert Belder, Erik Corry) * Upgrade V8 to 3.6.6.15 * Upgrade npm to 1.1.0-beta-10 (isaacs) * many doc updates (Ben Noordhuis, Jeremy Martin, koichik, Dave Irvine, Seong-Rak Choi, Shannen, Adam Malcontenti-Wilson, koichik) * Fix segfault in node_http_parser.cc * dgram, timers: fix memory leaks (Ben Noordhuis, Yoshihiro Kukuchi) * repl: fix repl.start not passing the `ignoreUndefined` arg (Damon Oehlman) * #1980: Socket.pause null reference when called on a closed Stream (koichik) * #2263: XMLHttpRequest piped in a writable file stream hang (koichik) * #2069: http resource leak (koichik) * buffer.readInt global pollution fix (Phil Sung) * timers: fix performance regression (Ben Noordhuis) * #2308, #2246: node swallows openssl error on request (koichik) * #2114: timers: remove _idleTimeout from item in .unenroll() (James Hartig) * #2379: debugger: Request backtrace w/o refs (Fedor Indutny) * simple DTrace ustack helper (Dave Pacheco) * crypto: rewrite HexDecode without snprintf (Roman Shtylman) * crypto: don't ignore DH init errors (Ben Noordhuis)

koichik closed this as completed in 07c27e0 Dec 21, 2011

alexkwolfe pushed a commit to alexkwolfe/node that referenced this issue Dec 23, 2011

tls: Fix node swallows openssl error on request

4cce1b8

Fixes nodejs#2308. Fixes nodejs#2246.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

With mis-matched keys, node swallows openssl error on request #2308

With mis-matched keys, node swallows openssl error on request #2308

georgesnelling commented Dec 9, 2011

georgesnelling commented Dec 9, 2011

georgesnelling commented Dec 9, 2011

koichik commented Dec 17, 2011

koichik commented Dec 21, 2011

With mis-matched keys, node swallows openssl error on request #2308

With mis-matched keys, node swallows openssl error on request #2308

Comments

georgesnelling commented Dec 9, 2011

georgesnelling commented Dec 9, 2011

georgesnelling commented Dec 9, 2011

koichik commented Dec 17, 2011

koichik commented Dec 21, 2011