Repository navigation
Crash in Node v8.x at lib/_tls_legacy.js.onclienthello() #26428
Description
- Version: 8.15.0
- Platform: Ubuntu server.
- Subsystem: TLS
At my company we have a TCP server accepting connections and securing them with tls.createSecurePair(). When there is high load in the server, usually due to lots of I/O traffic, we start seeing exceptions generated by the Node.js runtime.
The crash is located at lib/_tls_legacy.js/onclienthello(): when the TLS connection is destroyed before the setImmediate() can run, this situation results in a TypeError: Cannot read property 'loadSession' of null.
The attached code destroy-early.js shows this issue with sample certificates from freelan:
$ node destroy-early.js
(node:3072) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.Socket instead.
Encrypted bytes 220
_tls_legacy.js:660
self.ssl.loadSession(session);
^
TypeError: Cannot read property 'loadSession' of null
at Immediate._onImmediate (_tls_legacy.js:660:16)
at runCallback (timers.js:789:20)
at tryOnImmediate (timers.js:751:5)
at processImmediate [as _immediateCallback] (timers.js:722:5)
Essentially we open a couple of secure pairs, client and server, send cleartext to the client and then destroy the socket inside a setImmediate():
setImmediate(() => serverPair.destroy());
However if the socket is destroyed right away it works:
serverPair.destroy();
Bug exists in 8.x since at least 8.9.4 up until the latest 8.15.0. It is not present in v10 since _tls_legacy.js has disappeared.
We are open to sending a pull request ourselves, essentially a one liner ensuring that self.ssl is not null before proceeding in onclienthello().
Thanks!
destroy-early.tar.gz
Activity
bnoordhuis commentedon Mar 5, 2019
Thanks for the bug report. After checking the code I think there may be more than one bug to fix in
lib/_tls_legacy.js. Session resumption withtls.createSecurePair()seems pretty broken at the moment. I'll have a look.alexfernandez commentedon Mar 5, 2019
Thanks! We were thinking of a simple fix since that file disappears in v10.x, but any deeper solutions are of course welcome.
tls: fix legacy SecurePair session resumption
tls: fix legacy SecurePair clienthello race window
bnoordhuis commentedon Mar 5, 2019
#26452 - if you have an opportunity to test it out, that would be great.
alexfernandez commentedon Mar 5, 2019
Tested #26452 with our test case
destroy-early.js, works beautifully! Thanks 👍Note: the
socket hang uperror is the expected outcome once the theTypeErroris solved.tls: fix legacy SecurePair session resumption
tls: fix legacy SecurePair clienthello race window
bnoordhuis commentedon Jul 9, 2019
#26452 was merged in March but apparently didn't auto-close this issue. I'll do it the old-fashioned way then.