crypto: clear OpenSSL error queue after calling X509_verify()#45377

takuro-sato · 2022-11-08T15:58:23Z

Prior to this commit, functions accessing the OpenSSL error queue did not work properly after x509.verify() returned false.

Example code to reproduce the bug

constcrypto=require('crypto');constcert0=newcrypto.X509Certificate('-----BEGIN CERTIFICATE-----\n'+'MIICeTCCAWGgAwIBAgIBADANBgkqhkiG9w0BAQsFADAAMB4XDTIyMTEwNzExMzU0\n'+'NloXDTIyMTEwNzExMzU0NlowADCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoC\n'+'ggEBAKXDiNM5kCg3m7mBmLGhTWDLo3rEDf7nfI77V2rS9jXg0wV1LvnxQcYFeUud\n'+'zfvZpSc07TdmSDWJhE04/9G2fk86RwnMYj9KBLCgqzjk/wnrlcB8BuFPC5ZUKm+e\n'+'OmpfwzxRsqU0dlp/C6BmZV8cuvOKbxcq7MnACmS3lOvDUIdXjkEI1LEkzQudQNQi\n'+'S2PDPRUFm5KDkLmky2MACUkUA1eGMI8GLV60fzrsMnD6/lIGgDgRjcMdtv7D0F0K\n'+'D/AbCfXv4Z6SCKgp+0nLC5PpacNWu0yk1y4ihm9BHCRrxGcMzX5ce9pUfXVt6nUA\n'+'gwzlTcMno28yb16dBd9+XdQu/p8CAwEAATANBgkqhkiG9w0BAQsFAAOCAQEAVaRJ\n'+'CSOGycfz+bOGvSkLIOGWjtRoeaX/hMwCB/b0BuAVmcyWJuPsPcghwDnNUJAlxDnB\n'+'R7F6+J9P2kv7V+B4nlB/w0YHbidlSGeyd7AENsHqwYzOaoGCcr0B8pQrocj5Dm3g\n'+'JmPGMmMSa+MJfPD5j1///plvbLzBdG8nm9nG1yzOho+4lJf0VUwDnVEcRJp1w7kF\n'+'Ys19JJo78BEnUHJMOv1vE1KINY1k6wB69BJwZoW6tDjwHcHAtWzOA6324r+2tmnK\n'+'HCKAw78IZsDjLtC13LtrqFgWXzOh6WkLGmS7W01Coi3+uZcmWwJv/8m+2VlOXgrg\n'+'iQYf67llQlRAay0wnA==\n'+'-----END CERTIFICATE-----\n');constcert1=newcrypto.X509Certificate('-----BEGIN CERTIFICATE-----\n'+'MIICeTCCAWGgAwIBAgIBADANBgkqhkiG9w0BAQsFADAAMB4XDTIyMTEwNzExMzU0\n'+'N1oXDTIyMTEwNzExMzU0N1owADCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoC\n'+'ggEBAOc1bvNUvDCUxLhgNc9oQ8WwXsVSGHWaulf93eA1m9oB3kzvoCbEURN1k/dv\n'+'QoeLhovosZqnyz4W/2o+RHwCy/PNi3FGGaFGcpMTdwEGLn8TlGcUKv32h1Her+og\n'+'XnwGqZizD8xBbqfi5WRcX3JS02EKC8BVlDQiru2eToR+T6ZiOIGRZDNYYHUGvoSz\n'+'0nFiLzFLEwuMhEJhGmXcy0JyWx4BnYwEYYnwdKSAPakYFzvePMvpYyX2Xg4eD65j\n'+'kBRtKSYxDK39rXR6BfH7UpVjsY6H3TlVXYQdkxBPfzZlCPJBMG6TJRl2IzYJU1H4\n'+'JK84mEhDGnE9fxxvxykYX5a7YYMCAwEAATANBgkqhkiG9w0BAQsFAAOCAQEAAPcQ\n'+'cA6MFAGvbrACFYKHx7rMZ/HxjEps47Dq8zapqPWaTZVCSRI3si8lH99UW0DCRlRJ\n'+'VQnW2l7Ag7edp2RLQlCoH8gzEkCXvG3CNVE+U82WnlsUA+FTTV+X4W9Va3nNe2ZU\n'+'B8SO4V0374osYlM5UaUz9VWU3ulBupufQ9wdJTEVPcfEekDXVGeJIFeiyafhIYrB\n'+'H6OA1xhS0Yf2NtEIup2vexJ3wOxXh1ZyPR190Jr2b9ePylcUU2DWSCMvnzFdCPLt\n'+'aHR8RktrScK8BVXMag2Fvvgoco0XCIoWl/kPN3I0SnvIcTHa/uWA49ijMqwM1yDc\n'+'Timu77Xm/VYec/v1qg==\n'+'-----END CERTIFICATE-----\n')constcert2=newcrypto.X509Certificate('-----BEGIN CERTIFICATE-----\n'+'MIICeTCCAWGgAwIBAgIBADANBgkqhkiG9w0BAQsFADAAMB4XDTIyMTEwNzExMzU0\n'+'N1oXDTIyMTEwNzExMzU0N1owADCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoC\n'+'ggEBAJwgfV4eP/rsHYVtuCBvahP7tWJ/Djs9V11NHuVB11zUng2NiwfT3qvRfR/G\n'+'kc+fEUB/8K9kV4kGiFwB+xOKZ/0vpiRVKINPhrDJYdehahlT5iwF3dkTqNARp9MK\n'+'Dk1z3iAa0aQXNXCwIUNhs+8GZmXHZn8AOjAcBz03alBwUaYINMQVfjERT+BOCr7E\n'+'wK793iY8knF0ZT07GKIzqRFJhueBA+VKfoXluSSa7SoAO3UwYt72k1UIdUjA5C73\n'+'I8ZnlmIAVldjpO4AZdhwcVrHth54G3SAtt/vjk3052iJXpICduPWCUCF+pZDkRSO\n'+'DkSNfzaxr40I2raP0hvveUiArBUCAwEAATANBgkqhkiG9w0BAQsFAAOCAQEAjkdh\n'+'B0fyPt2Gyt5KRoCqsy8dS3iEhT98YzULeBk+mtcJXsbH8aEBXQzqzSnYfP3LeDxK\n'+'xP87pcP+V9sruQixHGNYQqRar8jpBuAhQAX8aqgWe4rc++3RbotafYJ7ILRj3J6K\n'+'0JXB3YXxjk7vp0aO/eaodKFM3g5EnKtXHH37yDZMQ665RPglh+MoJOw4BVutLPh4\n'+'MpmTY1yxl/HbIvqv8uQw55ftjXSaRB/pCCh4IjcQ3LS9FXE4pdESwgKScy8JsiDt\n'+'z/BtAH3hG/AbVon2RWBF4d5exFl4aJj3CSNKvMlu7q/CSbDWxdCafO433QRsI6SV\n'+'fW0jkmnFsvaUpOAF0w==\n'+'-----END CERTIFICATE-----\n');constprivateKeyPem='-----BEGIN PRIVATE KEY-----\n'+'MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgo5BAQydImlcOf1LS\n'+'7FrRfoBgHHUqevagq/1nmv3thMChRANCAAQ/oNhs1q1GJ4OOats5kaARTOPBZgzb\n'+'MpVnfwmmLDYGOJotCe0sVAUMYBCQ4DHko9DdOH6HWPSiGAzohs4nwDZz\n'+'-----END PRIVATE KEY-----';constprivateKey=crypto.createPrivateKey({key: privateKeyPem})console.log(privateKey.asymmetricKeyDetails)//{namedCurve: 'prime256v1' }crypto.createPrivateKey({key: privateKeyPem})console.log(cert1.verify(cert2.publicKey))// truecrypto.createPrivateKey({key: privateKeyPem})console.log(cert0.verify(cert2.publicKey))// falsecrypto.createPrivateKey({key: privateKeyPem})// It throws and error is about RSA key even though `key` is prime256v1

output:

{namedCurve: 'prime256v1' } true false node:internal/crypto/keys:620 handle.init(kKeyTypePrivate, data, format, type, passphrase); ^ Error: error:0200008A:rsa routines::invalid padding at Object.createPrivateKey (node:internal/crypto/keys:620:12) at Object.<anonymous> (/home/takuro/Projects/dev_node/bug.js:76:8) at Module._compile (node:internal/modules/cjs/loader:1159:14) at Module._extensions..js (node:internal/modules/cjs/loader:1213:10) at Module.load (node:internal/modules/cjs/loader:1037:32) at Module._load (node:internal/modules/cjs/loader:878:12) at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:82:12) at node:internal/main/run_main_module:23:47{opensslErrorStack: [ 'error:06880006:asn1 encoding routines::EVP lib', 'error:1C880004:Provider routines::RSA lib', 'error:02000072:rsa routines::padding check failed' ], library: 'rsa routines', reason: 'invalid padding', code: 'ERR_OSSL_RSA_INVALID_PADDING' } Node.js v19.0.1

Note

I got an error when I ran make -j4 test locally, but I believe it's not related to this PR because I got the same one with the latest main and v19.0.1.

make -s jstest ninja: Entering directory `out/Release' ninja: no work to do. === release test-tls-dhe === Path: parallel/test-tls-dhe Command: out/Release/node --no-warnings /home/takuro/Projects/node/test/parallel/test-tls-dhe.js --- TIMEOUT --- [05:04|% 100|+ 3769|- 1]: Done make[1]: *** [Makefile:307: jstest] Error 1 make: *** [Makefile:333: test] Error 2

Prior to this commit, functions accessing the OpenSSL error queue did not work properly after x509.verify() returned false.

nodejs-github-bot · 2022-11-08T15:58:41Z

Review requested:

@nodejs/crypto

tniessen

As this function is currently implemented, it ignores the difference between errors and verification failure. As such, this change appears logical. (Whether the current behavior is the best choice is a separate question.)

src/crypto/crypto_x509.cc

tniessen

Thank you for your contribution.

It seems that the email address you are using to commit your changes is not associated with your GitHub account; we highly recommend that you either add the commit email address to your GitHub account (not to your public GitHub profile!) or that you re-commit your changes with an email address of your choice that is connected to your GitHub account. Note that you can add multiple email addresses to the same GitHub account.

test/parallel/test-crypto-x509.js

Co-authored-by: Tobias Nießen <[email protected]>

takuro-sato · 2022-11-08T18:48:21Z

Thanks for the recommendation. I added the email to my GitHub account.

nodejs-github-bot · 2022-11-17T13:58:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/47984/

panva · 2022-11-17T14:41:25Z

@takuro-sato Thank you for your contribution! I noticed it is your first. Congratulations 🎉

nodejs-github-bot · 2022-11-17T14:43:04Z

Landed in 405ea2a

panva · 2022-11-17T15:02:48Z

#45495 follows this up, fixing the same problem with x509.checkPrivateKey().

Prior to this commit, functions accessing the OpenSSL error queue did not work properly after x509.verify() returned false. PR-URL: #45377 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Filip Skokan <[email protected]>

Prior to this commit, functions accessing the OpenSSL error queue did not work properly after x509.verify() returned false. PR-URL: nodejs#45377 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Filip Skokan <[email protected]>

mjones-vsat · 2022-12-16T20:16:05Z

Hi there,

Is there any chance that this (as well as the other issue with private keys) may be backported to Node 18?

Prior to this commit, functions accessing the OpenSSL error queue did not work properly after x509.verify() returned false. PR-URL: #45377 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Filip Skokan <[email protected]>

Prior to this commit, functions accessing the OpenSSL error queue did not work properly after x509.verify() returned false. Backport-PR-URL: https://github.com/nodejs-private/node-private/pull/375 CVE-ID: CVE-2023-23919 PR-URL: nodejs/node#45377 Refs: https://hackerone.com/bugs?subject=nodejs&report_id=1808596 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Filip Skokan <[email protected]>

Prior to this commit, functions accessing the OpenSSL error queue did not work properly after x509.verify() returned false. Backport-PR-URL: nodejs-private/node-private#375 CVE-ID: CVE-2023-23919 PR-URL: nodejs#45377 Refs: https://hackerone.com/bugs?subject=nodejs&report_id=1808596 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Filip Skokan <[email protected]>

crypto: clear OpenSSL error queue after calling X509_verify()
b65799c
Prior to this commit, functions accessing the OpenSSL error queue did not work properly after x509.verify() returned false.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Nov 8, 2022

takuro-sato marked this pull request as ready for review November 8, 2022 16:17

tniessen reviewed Nov 8, 2022
View reviewed changes

src/crypto/crypto_x509.cc Outdated Show resolvedHide resolved

use ClearErrorOnReturn instead of ERR_clear_error()
2775ffc

tniessen reviewed Nov 8, 2022
View reviewed changes

test/parallel/test-crypto-x509.js Outdated Show resolvedHide resolved
test/parallel/test-crypto-x509.js Outdated Show resolvedHide resolved

takuro-satoand others added 2 commits November 9, 2022 03:47

improve comment
5003cc9
Co-authored-by: Tobias Nießen <[email protected]>

improve comment
f26f710
Co-authored-by: Tobias Nießen <[email protected]>

Trott force-pushed the main branch from 2d76238 to ca3ed36Compare November 12, 2022 01:49

mscdex mentioned this pull request Nov 16, 2022
Crypto: X509Certificate.checkPrivateKey() used with a non-matching key makes the next call to createPrivateKey() fail. #45485
Closed

jasnell approved these changes Nov 16, 2022
View reviewed changes

panva added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2022

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2022

This comment was marked as outdated.
Sign in to view

tniessen approved these changes Nov 17, 2022
View reviewed changes

tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Nov 17, 2022

panva approved these changes Nov 17, 2022
View reviewed changes

panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 17, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 17, 2022

nodejs-github-bot merged commit 405ea2a into nodejs:mainNov 17, 2022

github-actionsbot mentioned this pull request Nov 18, 2022
CI Reliability 2022-11-18 nodejs/reliability#432
Open
12 tasks

This was referenced Nov 19, 2022
CI Reliability 2022-11-19 nodejs/reliability#433
Open
CI Reliability 2022-11-20 nodejs/reliability#434
Open
CI Reliability 2022-11-21 nodejs/reliability#435
Open

github-actionsbot mentioned this pull request Nov 22, 2022
CI Reliability 2022-11-22 nodejs/reliability#436
Open
15 tasks

ruyadorno mentioned this pull request Nov 24, 2022
v19.2.0 proposal #45615
Merged

mjones-vsat mentioned this pull request Dec 16, 2022
Release plan - v18.x Maintenance LTS nodejs/Release#737
Closed

danielleadams mentioned this pull request Dec 30, 2022
v18.13.0 release proposal #46025
Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

crypto: clear OpenSSL error queue after calling X509_verify()#45377

crypto: clear OpenSSL error queue after calling X509_verify() #45377

Uh oh!

takuro-sato commented Nov 8, 2022•
edited
Loading

Uh oh!

nodejs-github-bot commented Nov 8, 2022

Uh oh!

tniessen left a comment

Uh oh!

Uh oh!

tniessen left a comment

Uh oh!

Uh oh!

Uh oh!

takuro-sato commented Nov 8, 2022

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Nov 17, 2022

Uh oh!

panva commented Nov 17, 2022

Uh oh!

nodejs-github-bot commented Nov 17, 2022

Uh oh!

panva commented Nov 17, 2022

Uh oh!

mjones-vsat commented Dec 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

crypto: clear OpenSSL error queue after calling X509_verify()#45377

crypto: clear OpenSSL error queue after calling X509_verify() #45377

Uh oh!

Conversation

takuro-sato commented Nov 8, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example code to reproduce the bug

Note

Uh oh!

nodejs-github-bot commented Nov 8, 2022

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

takuro-sato commented Nov 8, 2022

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Nov 17, 2022

Uh oh!

panva commented Nov 17, 2022

Uh oh!

nodejs-github-bot commented Nov 17, 2022

Uh oh!

panva commented Nov 17, 2022

Uh oh!

mjones-vsat commented Dec 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

takuro-sato commented Nov 8, 2022•
edited
Loading