From 6e260642a9d9584ae613db9df41769e87edcf508 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 23 Jan 2019 06:47:12 -0800 Subject: [PATCH 1/2] test: fix renegotiation tests in pummel With a recent OpenSSL update to core, a change in the behavior of `s_client` result in pummel test failures because they spam with renegotiation requests before the renegotiation is complete. Modify tests to reduce initial spamming. This fixes the TLS renegotiation test. While it fixes the bug in the HTTPS test too, that is still failing with the OpenSSL update for other (too-be-determined) reasons. Refs: https://github.com/nodejs/node/pull/25381#issuecomment-456822479 --- test/pummel/test-https-ci-reneg-attack.js | 6 +++++- test/pummel/test-tls-ci-reneg-attack.js | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/test/pummel/test-https-ci-reneg-attack.js b/test/pummel/test-https-ci-reneg-attack.js index 50e16192e1b1ca..6faafe5a5ee287 100644 --- a/test/pummel/test-https-ci-reneg-attack.js +++ b/test/pummel/test-https-ci-reneg-attack.js @@ -73,10 +73,14 @@ function test(next) { // Count handshakes, start the attack after the initial handshake is done let handshakes = 0; let renegs = 0; + let waitingToSpam = true; child.stderr.on('data', function(data) { handshakes += ((String(data)).match(/verify return:1/g) || []).length; - if (handshakes === 2) spam(); + if (handshakes === 2 && waitingToSpam) { + waitingToSpam = false; + spam(); + } renegs += ((String(data)).match(/RENEGOTIATING/g) || []).length; }); diff --git a/test/pummel/test-tls-ci-reneg-attack.js b/test/pummel/test-tls-ci-reneg-attack.js index 3509dcfd43f853..00d5dfed482b52 100644 --- a/test/pummel/test-tls-ci-reneg-attack.js +++ b/test/pummel/test-tls-ci-reneg-attack.js @@ -73,11 +73,15 @@ function test(next) { // Count handshakes, start the attack after the initial handshake is done let handshakes = 0; let renegs = 0; + let waitingToSpam = true; child.stderr.on('data', function(data) { if (seenError) return; handshakes += ((String(data)).match(/verify return:1/g) || []).length; - if (handshakes === 2) spam(); + if (handshakes === 2 && waitingToSpam) { + waitingToSpam = false; + spam(); + } renegs += ((String(data)).match(/RENEGOTIATING/g) || []).length; }); From 6b4010695baeb84cfc8e6edf3ba1bf447decd892 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 23 Jan 2019 21:07:16 -0800 Subject: [PATCH 2/2] test: fix test-https-ci-reneg-attack Refs: https://github.com/nodejs/node/pull/25381#issuecomment-457067137 --- test/pummel/test-https-ci-reneg-attack.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/pummel/test-https-ci-reneg-attack.js b/test/pummel/test-https-ci-reneg-attack.js index 6faafe5a5ee287..1812459042edf8 100644 --- a/test/pummel/test-https-ci-reneg-attack.js +++ b/test/pummel/test-https-ci-reneg-attack.js @@ -79,6 +79,8 @@ function test(next) { handshakes += ((String(data)).match(/verify return:1/g) || []).length; if (handshakes === 2 && waitingToSpam) { waitingToSpam = false; + // See long comment in spam() function. + child.stdin.write('GET / HTTP/1.1\n'); spam(); } renegs += ((String(data)).match(/RENEGOTIATING/g) || []).length; @@ -109,7 +111,14 @@ function test(next) { // simulate renegotiation attack function spam() { if (closed) return; - child.stdin.write('R\n'); + // `R` at the start of a line will cause renegotiation but may (in OpenSSL + // 1.1.1a, at least) also cause `s_client` to send `R` as a HTTP header, + // causing the server to send 400 Bad Request and close the connection. + // By sending a `Referer` header that starts with a capital `R`, OpenSSL + // 1.1.1.a s_client sends both a valid HTTP header *and* causes a + // renegotiation. ¯\(ツ)/¯ (But you need to send a valid HTTP verb first, + // hence the `GET` in the stderr data callback.) + child.stdin.write('Referer: fhqwhgads\n'); setTimeout(spam, 50); } });