tls: defer re-entrant calls to SSL state machine from JS · nodejs/node@d141ddb · GitHub
Skip to content

Commit d141ddb

Browse files
pimterryaduh95
authored andcommitted
tls: defer re-entrant calls to SSL state machine from JS
Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #65105 Reviewed-By: Filip Skokan <panva.ip@gmail.com>
1 parent 05f4e54 commit d141ddb

6 files changed

Lines changed: 352 additions & 6 deletions

‎src/crypto/crypto_tls.cc‎

Lines changed: 105 additions & 6 deletions

‎src/crypto/crypto_tls.h‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,23 @@ class TLSWrap : public AsyncWrap,
5454

5555
enum class UnderlyingStreamWriteStatus { kHasActive, kVacancy };
5656

57+
// The SSL library's state machine is not reentrant. Node holds this scope
58+
// across every call into it, so that JS the SSL library invokes on its own
59+
// stack is recognised and kept from re-entering.
60+
class SSLLibraryCallScope {
61+
public:
62+
explicit SSLLibraryCallScope(TLSWrap* wrap) : wrap_(wrap) {
63+
wrap_->ssl_library_call_depth_++;
64+
}
65+
~SSLLibraryCallScope() { wrap_->ssl_library_call_depth_--; }
66+
67+
SSLLibraryCallScope(const SSLLibraryCallScope&) = delete;
68+
SSLLibraryCallScope& operator=(const SSLLibraryCallScope&) = delete;
69+
70+
private:
71+
TLSWrap* wrap_;
72+
};
73+
5774
static void Initialize(v8::Local<v8::Object> target,
5875
v8::Local<v8::Value> unused,
5976
v8::Local<v8::Context> context,
@@ -194,6 +211,16 @@ class TLSWrap : public AsyncWrap,
194211
// underlying stream even if there is no clear text to read or write.
195212
void Cycle();
196213

214+
inline bool in_ssl_library_call() const {
215+
return ssl_library_call_depth_ > 0;
216+
}
217+
218+
// Setup Cycle() after a library call, to flush anything DoWrite() held back
219+
void ScheduleDeferredCycle();
220+
221+
// Flush a shutdown held back by DoShutdown(), if there is one.
222+
void FlushPendingShutdown();
223+
197224
// Implement StreamListener:
198225
// Returns buf that points into enc_in_.
199226
uv_buf_t OnStreamAlloc(size_t size) override;
@@ -296,6 +323,9 @@ class TLSWrap : public AsyncWrap,
296323
size_t write_size_ = 0;
297324
BaseObjectPtr<AsyncWrap> current_write_;
298325
BaseObjectPtr<AsyncWrap> current_empty_write_;
326+
// Set when DoShutdown() was called while the SSL library was on the stack,
327+
// and so has yet to send close_notify.
328+
BaseObjectPtr<AsyncWrap> pending_shutdown_;
299329
std::string error_;
300330

301331
// TODO(@jasnell): These state flags should be revisited.
@@ -316,6 +346,7 @@ class TLSWrap : public AsyncWrap,
316346
bool shutdown : 1;
317347
bool cert_cb_running : 1;
318348
bool eof : 1;
349+
bool deferred_cycle_scheduled : 1;
319350
bool established : 1;
320351
bool write_callback_scheduled : 1;
321352
bool has_active_write_issued_by_prev_listener : 1;
@@ -325,6 +356,9 @@ class TLSWrap : public AsyncWrap,
325356

326357
int cycle_depth_ = 0;
327358

359+
// Nesting depth of calls into the SSL library. See SSLLibraryCallScope.
360+
int ssl_library_call_depth_ = 0;
361+
328362
// SSL_set_cert_cb
329363
CertCb cert_cb_ = nullptr;
330364
void* cert_cb_arg_ = nullptr;
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
'use strict';
2+
3+
// A shutdown deferred off the SSL library's stack must not run while the
4+
// 'newSession' callback is still outstanding. Make sure it's deferred
5+
// correctly so the connection still cleanly closes.
6+
7+
const common = require('../common');
8+
9+
if (!common.hasCrypto)
10+
common.skip('missing crypto');
11+
12+
const assert = require('assert');
13+
const fixtures = require('../common/fixtures');
14+
const tls = require('tls');
15+
const { SSL_OP_NO_TICKET } = require('crypto').constants;
16+
17+
const server = tls.createServer({
18+
key: fixtures.readKey('agent1-key.pem'),
19+
cert: fixtures.readKey('agent1-cert.pem'),
20+
// The only config that consistently fires newSession on both OpenSSL &
21+
// BoringSSL is TLS v1.2 + session id resumption (tickets disabled):
22+
minVersion: 'TLSv1.2',
23+
maxVersion: 'TLSv1.2',
24+
secureOptions: SSL_OP_NO_TICKET,
25+
ALPNCallback: common.mustCall(function({ protocols }) {
26+
this.end();
27+
return protocols[0];
28+
}),
29+
});
30+
31+
// Answering asynchronously holds EncOut() while the shutdown is replayed.
32+
server.on('newSession', common.mustCall((id, data, callback) => {
33+
setImmediate(callback);
34+
}));
35+
36+
server.on('tlsClientError', common.mustNotCall());
37+
server.on('secureConnection', common.mustCall((socket) => {
38+
socket.on('error', common.mustNotCall());
39+
}));
40+
41+
server.listen(0, common.mustCall(() => {
42+
const client = tls.connect({
43+
port: server.address().port,
44+
ALPNProtocols: ['a'],
45+
rejectUnauthorized: false,
46+
}, common.mustCall(() => {
47+
assert.strictEqual(client.alpnProtocol, 'a');
48+
}));
49+
50+
client.on('end', common.mustCall());
51+
client.on('close', common.mustCall((hadError) => {
52+
assert.strictEqual(hadError, false);
53+
server.close();
54+
}));
55+
client.on('error', common.mustNotCall());
56+
}));
Lines changed: 53 additions & 0 deletions

0 commit comments

Comments
 (0)