Permalink
Browse files

tls: fix off-by-one error in renegotiation check

Make CLIENT_RENEG_LIMIT inclusive instead of exclusive, i.e. a limit of 2
means the peer can renegotiate twice, not just once.

Update pummel/test-tls-ci-reneg-attack accordingly and make it less timing
sensitive (and run faster) while we're at it.
  • Loading branch information...
1 parent ae5b0e1 commit ff552ddbaa3cde93301f5227c95971ddd02682ef @bnoordhuis bnoordhuis committed Jun 18, 2012
Showing with 11 additions and 8 deletions.
  1. +2 −3 lib/tls.js
  2. +9 −5 test/pummel/test-tls-ci-reneg-attack.js
View
5 lib/tls.js
@@ -572,16 +572,15 @@ function onhandshakestart() {
debug('onhandshakestart');
var self = this, ssl = this.ssl;
- ssl.handshakes++;
- if (ssl.handshakes === 1) {
+ if (ssl.timer === null) {
function timeout() {
ssl.handshakes = 0;
ssl.timer = null;
}
ssl.timer = setTimeout(timeout, exports.CLIENT_RENEG_WINDOW * 1000);
}
- else if (ssl.handshakes >= exports.CLIENT_RENEG_LIMIT) {
+ else if (++ssl.handshakes > exports.CLIENT_RENEG_LIMIT) {
// Defer the error event to the next tick. We're being called from OpenSSL's
// state machine and OpenSSL is not re-entrant. We cannot allow the user's
// callback to destroy the connection right now, it would crash and burn.
View
14 test/pummel/test-tls-ci-reneg-attack.js
@@ -49,11 +49,14 @@ function test(next) {
key: fs.readFileSync(common.fixturesDir + '/test_key.pem')
};
+ var seenError = false;
+
var server = tls.createServer(options, function(conn) {
conn.on('error', function(err) {
console.error('Caught exception: ' + err);
assert(/TLS session renegotiation attack/.test(err));
conn.destroy();
+ seenError = true;
});
conn.pipe(conn);
});
@@ -67,16 +70,17 @@ function test(next) {
// count handshakes, start the attack after the initial handshake is done
var handshakes = 0;
+ var renegs = 0;
+
child.stderr.on('data', function(data) {
+ if (seenError) return;
handshakes += (('' + data).match(/verify return:1/g) || []).length;
if (handshakes === 2) spam();
+ renegs += (('' + data).match(/RENEGOTIATING/g) || []).length;
});
child.on('exit', function() {
- // with a renegotiation limit <= 1, we always see 4 handshake markers:
- // two for the initial handshake and another two for the attempted
- // renegotiation
- assert.equal(handshakes, 2 * Math.max(2, tls.CLIENT_RENEG_LIMIT));
+ assert.equal(renegs, tls.CLIENT_RENEG_LIMIT + 1);
server.close();
process.nextTick(next);
});
@@ -94,7 +98,7 @@ function test(next) {
function spam() {
if (closed) return;
child.stdin.write('R\n');
- setTimeout(spam, 250);
+ setTimeout(spam, 50);
}
});
}

0 comments on commit ff552dd

Please sign in to comment.