diff --git a/benchmark/tls/throughput-s2c.js b/benchmark/tls/throughput-s2c.js index 7fb93c304b20..5d31949e4a84 100644 --- a/benchmark/tls/throughput-s2c.js +++ b/benchmark/tls/throughput-s2c.js @@ -6,6 +6,7 @@ const bench = common.createBenchmark(main, { sendchunklen: [256, 32 * 1024, 128 * 1024, 16 * 1024 * 1024], recvbuflen: [0, 64 * 1024, 1024 * 1024], recvbufgenfn: ['true', 'false'], + recordlen: [1024, 4096, 16384], }); const fixtures = require('../../test/common/fixtures'); @@ -14,7 +15,8 @@ let recvbuf; let received = 0; const tls = require('tls'); -function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn }) { +function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn, + recordlen }) { if (isFinite(recvbuflen) && recvbuflen > 0) recvbuf = Buffer.alloc(recvbuflen); @@ -74,6 +76,7 @@ function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn }) { } const server = tls.createServer(options, (socket) => { + socket.setMaxSendFragment(recordlen); socket.on('data', (buf) => { socket.on('drain', write); write(); diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index fbc5fc8c39c9..55b57a43c76e 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -880,20 +880,14 @@ void TLSWrap::ClearOut() { MarkPopErrorOnReturn mark_pop_error_on_return; char out[kClearOutChunkSize]; + int length = 0; int read; - for (;;) { - { - SSLLibraryCallScope ssl_library_call_scope(this); - read = SSL_read(ssl_.get(), out, sizeof(out)); - } - Debug(this, "Read %d bytes of cleartext output", read); - - if (read <= 0) - break; + // Emits out[0, length). Returns false if JS destroyed the SSL context. + auto emit = [&]() { char* current = out; - while (read > 0) { - int avail = read; + while (length > 0) { + int avail = length; uv_buf_t buf = EmitAlloc(avail); if (static_cast(buf.len) < avail) @@ -906,12 +900,44 @@ void TLSWrap::ClearOut() { // check that ssl_ != nullptr afterwards. if (ssl_ == nullptr) { Debug(this, "Returning from read loop, ssl_ == nullptr"); - return; + return false; } - read -= avail; + length -= avail; current += avail; } + return true; + }; + + for (;;) { + { + SSLLibraryCallScope ssl_library_call_scope(this); + read = SSL_read(ssl_.get(), out + length, kClearOutChunkSize - length); + } + Debug(this, "Read %d bytes of cleartext output", read); + + if (read <= 0) break; + length += read; + + // SSL_read() returns at most one record. If the next record is already + // buffered, decrypt it into the same chunk instead of emitting one read + // per record. This is limited to application data records that are too + // long to be empty: other record types can trigger callbacks, e.g. for + // renegotiation, that have to run after the preceding data has been + // emitted, and SSL_read() skips over empty records to whatever follows. + // With read-ahead disabled, enc_in_ is at a record boundary after a + // successful read. The header need not be followed by a complete record: + // SSL_read() can return WANT_READ, and the path below emits the batch. + size_t pending; + const unsigned char* next = reinterpret_cast( + NodeBIO::FromBIO(enc_in_)->Peek(&pending)); + if (length < kClearOutChunkSize && pending >= SSL3_RT_HEADER_LENGTH && + next[0] == SSL3_RT_APPLICATION_DATA && + (next[3] << 8 | next[4]) > SSL3_RT_MAX_ENCRYPTED_OVERHEAD) { + continue; + } + + if (!emit()) return; } // We need to check whether an error occurred or the connection was @@ -919,13 +945,15 @@ void TLSWrap::ClearOut() { // See node#1642 and SSL_read(3SSL) for details. SSL_get_error must be // called immediately after SSL_read, without calling into JS, which may // change OpenSSL's error queue, modify ssl_, or even destroy ssl_ - // altogether. + // altogether. Data that was read before the failure is emitted once the + // error has been captured, before it is reported. if (read <= 0) { HandleScope handle_scope(env()->isolate()); Local error; int err = SSL_get_error(ssl_.get(), read); switch (err) { case SSL_ERROR_ZERO_RETURN: + if (!emit()) return; if (!flags_.eof) { flags_.eof = true; EmitRead(UV_EOF); @@ -975,9 +1003,15 @@ void TLSWrap::ClearOut() { break; default: + emit(); return; } + if (!emit()) { + Debug(this, "Got SSL error (%d) but not calling onerror", err); + return; + } + Debug(this, "Got SSL error (%d), calling onerror", err); // When TLS Alert are stored in wbio, // it should be flushed to socket before destroyed. diff --git a/test/parallel/test-double-tls-client.js b/test/parallel/test-double-tls-client.js index 32e62c6c5d21..37c7fb359f22 100644 --- a/test/parallel/test-double-tls-client.js +++ b/test/parallel/test-double-tls-client.js @@ -23,8 +23,15 @@ const server = tls.createServer({ }) .listen(client) .on('secureConnection', common.mustCall((serverTlsSock) => { - serverTlsSock.on('data', common.mustCallAtLeast((chunk) => { - assert.strictEqual(chunk.toString(), HEAD); + serverTlsSock.once('data', common.mustCall((chunk) => { + assert.strictEqual(chunk.toString('latin1', 0, HEAD.length), HEAD); + // The start of the inner handshake can arrive in the same chunk as + // HEAD. Put it back so that the inner TLSSocket, which reads whatever + // is buffered on the socket it wraps, receives it. + if (chunk.length > HEAD.length) { + serverTlsSock.pause(); + serverTlsSock.unshift(chunk.subarray(HEAD.length)); + } subserver.emit('connection', serverTlsSock); })); })); diff --git a/test/parallel/test-tls-coalesce-records.js b/test/parallel/test-tls-coalesce-records.js new file mode 100644 index 000000000000..9eb6289c73ab --- /dev/null +++ b/test/parallel/test-tls-coalesce-records.js @@ -0,0 +1,130 @@ +'use strict'; +// TLS records that are already buffered are emitted as one chunk rather than +// one chunk per record. The end of the stream, clean or not, must still be +// reported after the data that precedes it. + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const { isBoringSSL } = require('../common/crypto'); +const assert = require('assert'); +const net = require('net'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +const RECORD_SIZE = 512; +const RECORDS = 256; +const payload = Buffer.alloc(RECORD_SIZE * RECORDS); +for (let i = 0; i < RECORDS; i++) + payload.fill(i, i * RECORD_SIZE, (i + 1) * RECORD_SIZE); + +const serverOptions = { + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), +}; +const clientOptions = { + ca: fixtures.readKey('ca1-cert.pem'), + servername: 'agent1', +}; + +function sendPayload(socket) { + assert(socket.setMaxSendFragment(RECORD_SIZE)); + socket.end(payload); +} + +// Small records are coalesced, and 'end' follows all of the data. +{ + const server = tls.createServer(serverOptions, common.mustCall(sendPayload)); + server.listen(0, common.mustCall(() => { + const chunks = []; + const client = tls.connect({ ...clientOptions, port: server.address().port }); + client.on('data', (chunk) => chunks.push(chunk)); + client.on('end', common.mustCall(() => { + assert.deepStrictEqual(Buffer.concat(chunks), payload); + assert(chunks.length < RECORDS, `${chunks.length} chunks`); + assert(chunks.every((chunk) => chunk.length <= 16 * 1024)); + server.close(); + })); + })); +} + +// An onread buffer smaller than a record still receives every byte. +{ + const server = tls.createServer(serverOptions, common.mustCall(sendPayload)); + server.listen(0, common.mustCall(() => { + const chunks = []; + const client = tls.connect({ + ...clientOptions, + port: server.address().port, + onread: { + buffer: Buffer.alloc(100), + callback(nread, buf) { + chunks.push(Buffer.from(buf.subarray(0, nread))); + }, + }, + }); + client.on('end', common.mustCall(() => { + assert.deepStrictEqual(Buffer.concat(chunks), payload); + server.close(); + })); + })); +} + +// A bad record that arrives together with data is reported after the data. +{ + // Small enough to arrive in one read and to fit into one batch. + const data = payload.subarray(0, 8 * RECORD_SIZE); + const server = tls.createServer(serverOptions, common.mustCall((socket) => { + socket.once('data', common.mustCall(() => { + assert(socket.setMaxSendFragment(RECORD_SIZE)); + socket.end(data); + })); + })); + server.listen(0, common.mustCall(() => { + // Passes the handshake through, then holds the response until the server + // ends and forwards it in one write, with the last data record (the one + // before close_notify) corrupted. + let holding = false; + const held = []; + const proxy = net.createServer(common.mustCall((socket) => { + const upstream = net.connect(server.address().port); + socket.pipe(upstream); + socket.on('error', () => {}); + upstream.on('data', (chunk) => { + if (holding) + held.push(chunk); + else + socket.write(chunk); + }); + upstream.on('end', common.mustCall(() => { + const response = Buffer.concat(held); + let closeNotify = 0; + for (let i = 0; i < response.length; i += 5 + response.readUInt16BE(i + 3)) + closeNotify = i; + response[closeNotify - 1] ^= 1; + socket.end(response); + })); + })); + proxy.listen(0, common.mustCall(() => { + const chunks = []; + const client = tls.connect({ + ...clientOptions, + port: proxy.address().port, + }, common.mustCall(() => { + holding = true; + client.write('go'); + })); + client.on('data', (chunk) => chunks.push(chunk)); + client.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, isBoringSSL ? + 'ERR_SSL_BAD_DECRYPT' : + 'ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC'); + assert.deepStrictEqual(Buffer.concat(chunks), + data.subarray(0, -RECORD_SIZE)); + proxy.close(); + server.close(); + })); + })); + })); +} diff --git a/test/parallel/test-tls-coalesce-renegotiation.js b/test/parallel/test-tls-coalesce-renegotiation.js new file mode 100644 index 000000000000..1fd9326a88be --- /dev/null +++ b/test/parallel/test-tls-coalesce-renegotiation.js @@ -0,0 +1,82 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const { isBoringSSL } = require('../common/crypto'); +if (isBoringSSL) + common.skip('BoringSSL does not support renegotiation'); + +const assert = require('assert'); +const net = require('net'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +// Deliver application data and a TLS 1.2 ClientHello in one transport write. +// The data handler must run before renegotiation starts, so it can disable it. +const payload = Buffer.from('disable renegotiation'); +let holding = false; +let client; + +const server = tls.createServer({ + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), + maxVersion: 'TLSv1.2', +}, common.mustCall((socket) => { + socket.on('data', common.mustCall((data) => { + assert.deepStrictEqual(data, payload); + socket.disableRenegotiation(); + })); + socket.on('secure', common.mustNotCall('renegotiation completed')); + socket.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ERR_TLS_RENEGOTIATION_DISABLED'); + client.destroy(); + socket.destroy(); + server.close(); + proxy.close(); + })); +})); + +const proxy = net.createServer(common.mustCall((socket) => { + const upstream = net.connect(server.address().port); + upstream.pipe(socket); + socket.on('end', () => upstream.end()); + socket.on('close', () => upstream.destroy()); + const held = []; + let pending = Buffer.alloc(0); + socket.on('data', (data) => { + if (!holding) { + upstream.write(data); + return; + } + held.push(data); + pending = Buffer.concat([pending, data]); + while (pending.length >= 5) { + const length = pending.readUInt16BE(3); + if (pending.length < 5 + length) + break; + if (pending[0] === 22) { // handshake + holding = false; + upstream.write(Buffer.concat(held)); + return; + } + pending = pending.subarray(5 + length); + } + }); +})); + +server.listen(0, common.mustCall(() => { + proxy.listen(0, common.mustCall(() => { + client = tls.connect({ + port: proxy.address().port, + ca: fixtures.readKey('ca1-cert.pem'), + servername: 'agent1', + maxVersion: 'TLSv1.2', + }, common.mustCall(() => { + holding = true; + client.write(payload, common.mustCall(() => { + assert(client.renegotiate({}, + common.mustNotCall('renegotiation completed'))); + })); + })); + })); +})); diff --git a/test/parallel/test-tls-max-send-fragment.js b/test/parallel/test-tls-max-send-fragment.js index d389cf50d3d7..1dc34c53481e 100644 --- a/test/parallel/test-tls-max-send-fragment.js +++ b/test/parallel/test-tls-max-send-fragment.js @@ -29,20 +29,26 @@ if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); +const net = require('net'); const tls = require('tls'); const buf = Buffer.allocUnsafe(10000); let received = 0; const maxChunk = 768; +// Largest application_data record the server sent, as seen on the wire. +let maxRecord = 0; const invalidArgumentError = { name: 'TypeError', code: 'ERR_INVALID_ARG_TYPE' }; +// TLS 1.2, so that application_data records are not confused with the +// encrypted handshake records of TLS 1.3. const server = tls.createServer({ key: fixtures.readKey('agent1-key.pem'), - cert: fixtures.readKey('agent1-cert.pem') + cert: fixtures.readKey('agent1-cert.pem'), + maxVersion: 'TLSv1.2', }, common.mustCall((c) => { // No size is passed. @@ -76,20 +82,46 @@ const server = tls.createServer({ assert(c.setMaxSendFragment(maxChunk)); c.end(buf); -})).listen(0, common.mustCall(function() { - const c = tls.connect(this.address().port, { - rejectUnauthorized: false - }, common.mustCall(function() { - c.on('data', common.mustCallAtLeast((chunk) => { - assert(chunk.length <= maxChunk); - received += chunk.length; - })); +})).listen(0, common.mustCall(() => { + // Records that arrive together are emitted as one 'data' chunk, so the + // fragment size is checked on the wire: a proxy parses the TLS record + // headers of everything the server sends. + const proxy = net.createServer(common.mustCall((socket) => { + const upstream = net.connect(server.address().port); + socket.pipe(upstream); + let pending = Buffer.alloc(0); + upstream.on('data', (data) => { + socket.write(data); + pending = Buffer.concat([pending, data]); + while (pending.length >= 5) { + const length = pending.readUInt16BE(3); + if (pending.length < 5 + length) + break; + if (pending[0] === 23) // application_data + maxRecord = Math.max(maxRecord, length); + pending = pending.subarray(5 + length); + } + }); + upstream.on('end', () => socket.end()); + })).listen(0, common.mustCall(() => { + const c = tls.connect(proxy.address().port, { + rejectUnauthorized: false + }, common.mustCall(() => { + c.on('data', common.mustCallAtLeast((chunk) => { + received += chunk.length; + })); - // Ensure that we receive 'end' event anyway - c.on('end', common.mustCall(function() { - c.destroy(); - server.close(); - assert.strictEqual(received, buf.length); + // Ensure that we receive 'end' event anyway + c.on('end', common.mustCall(() => { + c.destroy(); + proxy.close(); + server.close(); + assert.strictEqual(received, buf.length); + // A record carries at most maxChunk bytes of data plus the cipher's + // overhead (IV, MAC or tag, padding). + assert(maxRecord > 0 && maxRecord <= maxChunk + 256, + `largest record ${maxRecord}`); + })); })); })); }));