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.
nodejs · Jun 18, 2012 · ff552dd · ff552dd
1 parent ae5b0e1
commit ff552dd
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 8 deletions.
diff --git 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
@@ -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);
     }
   });
 }