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

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

Merged
merged 4 commits into from Nov 17, 2022

Conversation

takuro-sato
Copy link
Contributor

@takuro-sato takuro-sato commented Nov 8, 2022

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
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot 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 takuro-sato marked this pull request as ready for review November 8, 2022 16:17
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
Co-authored-by: Tobias Nießen <tniessen@tnie.de>
Co-authored-by: Tobias Nießen <tniessen@tnie.de>
@takuro-sato
Copy link
Contributor Author

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

@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2022
@nodejs-github-bot

This comment was marked as outdated.

@tniessen 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
@nodejs-github-bot
Copy link
Collaborator

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

panva commented Nov 17, 2022

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

@nodejs-github-bot 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 nodejs-github-bot merged commit 405ea2a into nodejs:main Nov 17, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 405ea2a

@panva
Copy link
Member

panva commented Nov 17, 2022

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

ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
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 <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Nov 23, 2022
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 <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
@vsmteam
Copy link

vsmteam commented Dec 16, 2022

Hi there,

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

danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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 <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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 <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
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 <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
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 <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
oraluben pushed a commit to oraluben/node that referenced this pull request Mar 14, 2023
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 <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
oraluben pushed a commit to oraluben/node that referenced this pull request Mar 17, 2023
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 <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants