From ff552ddbaa3cde93301f5227c95971ddd02682ef Mon Sep 17 00:00:00 2001
From: Ben Noordhuis <info@bnoordhuis.nl>
Date: Mon, 18 Jun 2012 04:05:21 +0200
Subject: [PATCH] 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.
---
 lib/tls.js                              |  5 ++---
 test/pummel/test-tls-ci-reneg-attack.js | 14 +++++++++-----
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/lib/tls.js b/lib/tls.js
index 0b3ff01748..e396b592e8 100644
--- a/lib/tls.js
+++ b/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.
diff --git a/test/pummel/test-tls-ci-reneg-attack.js b/test/pummel/test-tls-ci-reneg-attack.js
index e9748c116f..778e288ca7 100644
--- a/test/pummel/test-tls-ci-reneg-attack.js
+++ b/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);
     }
   });
 }