Node: Crash in Node v8.x at lib/_tls_legacy.js.onclienthello()

[alexfernandez]

• 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

[bnoordhuis]

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 with tls.createSecurePair() seems pretty broken at the moment. I'll have a look.

[alexfernandez]

Thanks! We were thinking of a simple fix since that file disappears in v10.x, but any deeper solutions are of course welcome.

[bnoordhuis]

26452 - if you have an opportunity to test it out, that would be great.

[alexfernandez]

Tested https://github.com/nodejs/node/pull/26452 with our test case destroy-early.js, works beautifully! Thanks :+1:

 $ node --version
v8.9.4
 $ node destroy-early.js 
(node:17638) [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)

 $ ../../node/node --version
v8.15.2-pre
 $ ../../node/node destroy-early.js 
(node:17652) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.TLSSocket instead.
Encrypted bytes 220
Client error: Error: socket hang up

Note: the socket hang up error is the expected outcome once the the TypeError is solved.

[bnoordhuis]

26452 was merged in March but apparently didn't auto-close this issue. I'll do it the old-fashioned way then.