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

const crypto = require('crypto');

const cert0 = new crypto.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'
);

const cert1 = new crypto.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'
)

const cert2 = new crypto.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'
);

const privateKeyPem = '-----BEGIN PRIVATE KEY-----\n' +
'MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgo5BAQydImlcOf1LS\n' +
'7FrRfoBgHHUqevagq/1nmv3thMChRANCAAQ/oNhs1q1GJ4OOats5kaARTOPBZgzb\n' +
'MpVnfwmmLDYGOJotCe0sVAUMYBCQ4DHko9DdOH6HWPSiGAzohs4nwDZz\n' +
'-----END PRIVATE KEY-----';

const privateKey = crypto.createPrivateKey({key: privateKeyPem})
console.log(privateKey.asymmetricKeyDetails) // { namedCurve: 'prime256v1' }

crypto.createPrivateKey({key: privateKeyPem})
console.log(cert1.verify(cert2.publicKey)) // true

crypto.createPrivateKey({key: privateKeyPem})
console.log(cert0.verify(cert2.publicKey))  // false
crypto.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]>

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 resolved Hide 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 resolved Hide resolved

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

takuro-sato and 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 ca3ed36 Compare 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-actions bot 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:main Nov 17, 2022

github-actions bot 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-actions bot 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

Open

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

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

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

takuro-sato commented Nov 8, 2022 •

edited

Loading

nodejs-github-bot commented Nov 8, 2022

tniessen left a comment

tniessen left a comment

takuro-sato commented Nov 8, 2022

This comment was marked as outdated.

nodejs-github-bot commented Nov 17, 2022

panva commented Nov 17, 2022

nodejs-github-bot commented Nov 17, 2022

panva commented Nov 17, 2022

mjones-vsat commented Dec 16, 2022

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

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

Conversation

takuro-sato commented Nov 8, 2022 • edited Loading

Example code to reproduce the bug

Note

nodejs-github-bot commented Nov 8, 2022

tniessen left a comment

Choose a reason for hiding this comment

tniessen left a comment

Choose a reason for hiding this comment

takuro-sato commented Nov 8, 2022

This comment was marked as outdated.

nodejs-github-bot commented Nov 17, 2022

panva commented Nov 17, 2022

nodejs-github-bot commented Nov 17, 2022

panva commented Nov 17, 2022

mjones-vsat commented Dec 16, 2022

takuro-sato commented Nov 8, 2022 •

edited

Loading