Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls.createSecureContext results in an abort #56375

Closed
zyscoder opened this issue Dec 27, 2024 · 2 comments · Fixed by #56554
Closed

tls.createSecureContext results in an abort #56375

zyscoder opened this issue Dec 27, 2024 · 2 comments · Fixed by #56554
Labels
confirmed-bug Issues with confirmed bugs. tls Issues and PRs related to the tls subsystem.

Comments

@zyscoder
Copy link

Version

v22.11.0

Platform

Linux u24vm 6.8.0-50-generic #51-Ubuntu SMP PREEMPT_DYNAMIC Sat Nov  9 17:58:29 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

tls

What steps will reproduce the bug?

Setup a node instance,

» node

and run the following javascript code.

tls = require('tls');
tls.createSecureContext({clientCertEngine:'x'});

Then the node instance occurs an abort.

How often does it reproduce? Is there a required condition?

This abort can always be triggered following the steps above.

What is the expected behavior? Why is that the expected behavior?

If any error occurs, an exception or similar error-reporting stuff should be thrown, caught, and handled correctly. There is no reason to abort the whole node process.

What do you see instead?

» node                                                 
Welcome to Node.js v22.11.0.
Type ".help" for more information.
> tls = require('tls');
{
  CLIENT_RENEG_LIMIT: 3,
  CLIENT_RENEG_WINDOW: 600,
  DEFAULT_CIPHERS: 'TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA256:ECDHE-RSA-AES256-SHA384:DHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA256:HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!PSK:!SRP:!CAMELLIA',
  DEFAULT_ECDH_CURVE: 'auto',
  DEFAULT_MIN_VERSION: 'TLSv1.2',
  DEFAULT_MAX_VERSION: 'TLSv1.3',
  getCiphers: [Function (anonymous)],
  rootCertificates: [Getter],
  convertALPNProtocols: [Function: convertALPNProtocols],
  checkServerIdentity: [Function: checkServerIdentity],
  createSecureContext: [Function: createSecureContext],
  SecureContext: [Function: SecureContext],
  TLSSocket: [Function: TLSSocket],
  Server: [Function: Server],
  createServer: [Function: createServer],
  connect: [Function: connect],
  createSecurePair: [Function: deprecated]
}
> tls.createSecureContext({clientCertEngine:'x'});
[1]    305195 segmentation fault (core dumped)  node

Additional information

No response

@lpinca lpinca added the tls Issues and PRs related to the tls subsystem. label Dec 27, 2024
@joyeecheung
Copy link
Member

joyeecheung commented Jan 10, 2025

It is a regression from #53803 cc @jasnell because it comes from a misunderstanding about how std::vector/LocalVector should be used (if it's going to be populated later, the size should not be passed into the constructor, or it will only append the new elements to a bunch of empty handles, instead of to an empty vector)

@joyeecheung
Copy link
Member

Actually #53803 also broke error reporting, because it did something like if (!...IsNothing()) to check for exceptions.

@joyeecheung joyeecheung added the confirmed-bug Issues with confirmed bugs. label Jan 10, 2025
nodejs-github-bot pushed a commit that referenced this issue Jan 12, 2025
The ncrypto move introduced regressions in
cryptoErrorListToException() by passing in the size of the
vector unnecessarily into the vector constructor and then use
push_back() (which would result in a crash on dereferencing empty
handles during later iteration) and having incorrect logic for
checking the presence of an exception. This patch fixes it.

PR-URL: #56554
Fixes: #56375
Refs: #53803
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this issue Jan 13, 2025
The ncrypto move introduced regressions in
cryptoErrorListToException() by passing in the size of the
vector unnecessarily into the vector constructor and then use
push_back() (which would result in a crash on dereferencing empty
handles during later iteration) and having incorrect logic for
checking the presence of an exception. This patch fixes it.

PR-URL: #56554
Fixes: #56375
Refs: #53803
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Jan 13, 2025
The ncrypto move introduced regressions in
cryptoErrorListToException() by passing in the size of the
vector unnecessarily into the vector constructor and then use
push_back() (which would result in a crash on dereferencing empty
handles during later iteration) and having incorrect logic for
checking the presence of an exception. This patch fixes it.

PR-URL: nodejs#56554
Fixes: nodejs#56375
Refs: nodejs#53803
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants