tls: OpenSSL errors with many tls socket writes

[mscdex]

Recently while trying to create a benchmark for the JS Duplex stream compatibility for tls.connect(), I discovered an issue while trying to perform > 10-ish successive writes to the tls wrapped socket. The test case to reproduce this is in #1594.

The error I get is:

Error: 139779539269504:error:1408F119:SSL routines:SSL3_GET_RECORD:decryption failed or bad record mac:../deps/openssl/openssl/ssl/s3_pkt.c:521:

I'm not sure why it's using SSL3 in the first place since I thought 2 and 3 were disabled by default. So I tried forcing TLSv1 by setting secureProtocol: 'TLSv1_method' in the tls.connect() options, which instead causes this error:

events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: socket hang up
    at TLSSocket.onHangUp (_tls_wrap.js:951:19)
    at TLSSocket.g (events.js:257:16)
    at emitNone (events.js:72:20)
    at TLSSocket.emit (events.js:163:7)
    at endReadableNT (_stream_readable.js:890:12)
    at doNTCallback2 (node.js:436:9)
    at process._tickCallback (node.js:350:17)
    at Duplex.<anonymous> (_stream_wrap.js:38:18)
    at emitOne (events.js:77:13)
    at Duplex.emit (events.js:166:7)

[mscdex]

/cc @indutny

[bnoordhuis]

I'm not sure why it's using SSL3 in the first place since I thought 2 and 3 were disabled by default.

OpenSSL uses the same code for reading and writing SSLv3 and TLSv1.x frames. SSL and TLS are largely the same wire protocol, the differences are in the supported extensions and cipher suites.

[shigeki]

Sorry for my previous blank comment. This issue comes from a tiny bug in JSStream::DoWrite where it always sends the first buffer so that its data becomes an invalid broken SSL record data. It can be fixed the following patch.

diff --git a/src/js_stream.cc b/src/js_stream.cc
index 09c4f58..6b7c406 100644
--- a/src/js_stream.cc
+++ b/src/js_stream.cc
@@ -89,7 +89,7 @@ int JSStream::DoWrite(WriteWrap* w,

   Local<Array> bufs_arr = Array::New(env()->isolate(), count);
   for (size_t i = 0; i < count; i++)
-    bufs_arr->Set(i, Buffer::New(env(), bufs[0].base, bufs[0].len));
+    bufs_arr->Set(i, Buffer::New(env(), bufs[i].base, bufs[i].len));

   Local<Value> argv[] = {
     w->object(),

@mscdex Your test of #1594 does not have server.close() and assert check. I modified your test as below. Is it good for you?

diff --git a/test/parallel/test-tls-connect-stream-writes.js b/test/parallel/test-tls-connect-stream-writes.js
index 3c9fecc..0bf1db1 100644
--- a/test/parallel/test-tls-connect-stream-writes.js
+++ b/test/parallel/test-tls-connect-stream-writes.js
@@ -1,4 +1,5 @@
-var fs = require('fs'),
+var assert = require('assert'),
+    fs = require('fs'),
     path = require('path'),
     tls = require('tls'),
     stream = require('stream'),
@@ -12,8 +13,14 @@ var cert_dir = path.resolve(__dirname, '../fixtures'),
                 cert: fs.readFileSync(cert_dir + '/test_cert.pem'),
                 ca: [ fs.readFileSync(cert_dir + '/test_ca.pem') ],
                 ciphers: 'AES256-GCM-SHA384' };
-
-server = tls.createServer(options);
+var content = 'hello world';
+var recv_bufs = [];
+var send_data = '';
+server = tls.createServer(options, function(s) {
+  s.on('data', function(c) {
+    recv_bufs.push(c);
+  });
+});
 server.listen(common.PORT, function() {
   var raw = net.connect(common.PORT);

@@ -43,8 +50,16 @@ server.listen(common.PORT, function() {
     socket: p,
     rejectUnauthorized: false
   }, function() {
-    for (var i = 0; i < 50; ++i)
-      socket.write('hello world');
+    for (var i = 0; i < 50; ++i) {
+      socket.write(content);
+      send_data += content;
+    }
     socket.end();
+    server.close();
   });
 });
+
+process.on('exit', function() {
+  var recv_data = (Buffer.concat(recv_bufs)).toString();
+  assert.strictEqual(send_data, recv_data);
+});

[mscdex]

@shigeki The test changes look fine to me.

[indutny]

@shigeki please file a PR, LGTM

[brendanashworth]

Also fixed in #1635.