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: refactor crypto subsystem and introduce WebCrypto API #35093

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Sep 7, 2020

@nodejs/tsc @nodejs/crypto ... This is a big one. It is still a work in progress. I'm opening the draft PR so that people can follow along with the work.

What does this do:

Three main things... lots of other little things

  1. It refactors the Node.js src crypto internals so that they are more maintainable and organized. The existing node_crypto.cc has grown into a massive disorganized and unmaintainable mess that very few brave touch. This breaks that functionality up across multiple files in src/crypto that are organized by purpose/algorithm.

  2. It makes a number of important fixes and improvements to the existing crypto internals. For example, previously, CryptoJob had no mechanism for tracking the memory associated with it. CryptoJob has been refactored into an AsyncWrap derived object, allowing and improving memory tracking.

  3. It introduces an experimental Web Crypto API implementation as require('crypto').webcrypto. The initial intent of this API is to be standards compliant.

  4. It introduces HKDF support for the legacy API. HKDF is required by the Web Crypto API implementation, so I decided to go ahead and add a variation for the legacy API also.

5. It introduces crypto.timingSafeEqual.bigint() for performing constant-time comparisons of bigint values. Pulled this due to some outstanding technical concerns that will need to be looked at later.

  1. It introduces the ability to use ArrayBuffer for the existing legacy crypto APIs. Previously, the legacy API was restricted to Buffer, TypedArray, and DataView objects. Because the Web Crypto API makes use of ArrayBuffer also, I decided to go ahead and extend that capability to the legacy API. (Note: this is still a work in progress that will be completed before this PR moves out of draft status)

  2. It introduces one extension to the Web Crypto API exportKey() and importKey() methods to allow converting back and forth between a Node.js KeyObject and a Web Crypto CryptoKey object. The CryptoKey is currently implemented as a wrapper around KeyObject.

My next task on this PR is to begin filling out the tests. This PR will remain in draft status until I have those ready. However, please feel free to begin reviewing the changes made so far.

This is a big PR that will take some time to review. I'm happy to jump on a zoom call with anyone to walk through it.

Outside of changes to error messages and codes (which have been made more consistent), there should be no backwards compatibility breaking changes to the existing legacy API. Any backwards breaking changes should be considered bugs.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 7, 2020
@jasnell jasnell changed the title subtle [WIP] crypto: refactor crypto subsystem and introduce WebCrypto API Sep 7, 2020
@jasnell jasnell added crypto Issues and PRs related to the crypto subsystem. experimental Issues and PRs related to experimental features. semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 7, 2020
@jasnell jasnell force-pushed the subtlecrypto branch 14 times, most recently from 409f56e to 13f1691 Compare September 9, 2020 23:26
@jasnell
Copy link
Member Author

jasnell commented Sep 9, 2020

For anyone curious about the performance difference that the SubtleCrypto APIs will have relative to the legacy apis... Here's an example comparing crypto.createHash() to subtle.digest() ...

Keep in mind, however, that this is tracking raw execution speed, and ignores factors such as event loop delay, event loop utilization and memory. The createHash variant is purely sync, while the subtle version uses the libuv threadpool. Significantly, the createHash variant is one operation at a time, while the subtle queues up n hash jobs concurrently.

crypto/webcrypto-digest.js n=1000 method="SHA-1" data=10 sync="createHash": 94,602.60002001791
crypto/webcrypto-digest.js n=1000 method="SHA-256" data=10 sync="createHash": 20,070.058150585086
crypto/webcrypto-digest.js n=1000 method="SHA-384" data=10 sync="createHash": 55,681.77686340876
crypto/webcrypto-digest.js n=1000 method="SHA-512" data=10 sync="createHash": 43,733.5794076068
crypto/webcrypto-digest.js n=1000 method="SHA-1" data=20 sync="createHash": 70,726.74488714593
crypto/webcrypto-digest.js n=1000 method="SHA-256" data=20 sync="createHash": 52,796.95289777924
crypto/webcrypto-digest.js n=1000 method="SHA-384" data=20 sync="createHash": 92,470.48873704823
crypto/webcrypto-digest.js n=1000 method="SHA-512" data=20 sync="createHash": 36,330.60193812862
crypto/webcrypto-digest.js n=1000 method="SHA-1" data=50 sync="createHash": 93,567.16391376476
crypto/webcrypto-digest.js n=1000 method="SHA-256" data=50 sync="createHash": 95,433.87100776259
crypto/webcrypto-digest.js n=1000 method="SHA-384" data=50 sync="createHash": 69,989.07610500153
crypto/webcrypto-digest.js n=1000 method="SHA-512" data=50 sync="createHash": 86,632.92891638233
crypto/webcrypto-digest.js n=1000 method="SHA-1" data=100 sync="createHash": 62,713.004720533296
crypto/webcrypto-digest.js n=1000 method="SHA-256" data=100 sync="createHash": 92,227.69542356953
crypto/webcrypto-digest.js n=1000 method="SHA-384" data=100 sync="createHash": 58,363.47758314272
crypto/webcrypto-digest.js n=1000 method="SHA-512" data=100 sync="createHash": 91,339.97478468656
crypto/webcrypto-digest.js n=1000 method="SHA-1" data=10 sync="subtle": 13,358.364593130998
crypto/webcrypto-digest.js n=1000 method="SHA-256" data=10 sync="subtle": 32,006.11342371284
crypto/webcrypto-digest.js n=1000 method="SHA-384" data=10 sync="subtle": 22,250.87690149597
crypto/webcrypto-digest.js n=1000 method="SHA-512" data=10 sync="subtle": 20,624.592264701296
crypto/webcrypto-digest.js n=1000 method="SHA-1" data=20 sync="subtle": 51,008.840087031276
crypto/webcrypto-digest.js n=1000 method="SHA-256" data=20 sync="subtle": 14,754.004273644876
crypto/webcrypto-digest.js n=1000 method="SHA-384" data=20 sync="subtle": 46,213.40955095308
crypto/webcrypto-digest.js n=1000 method="SHA-512" data=20 sync="subtle": 40,880.32676789835
crypto/webcrypto-digest.js n=1000 method="SHA-1" data=50 sync="subtle": 22,610.031492156264
crypto/webcrypto-digest.js n=1000 method="SHA-256" data=50 sync="subtle": 33,920.458018446894
crypto/webcrypto-digest.js n=1000 method="SHA-384" data=50 sync="subtle": 48,910.4416554189
crypto/webcrypto-digest.js n=1000 method="SHA-512" data=50 sync="subtle": 12,468.43351102472
crypto/webcrypto-digest.js n=1000 method="SHA-1" data=100 sync="subtle": 21,238.01034673388
crypto/webcrypto-digest.js n=1000 method="SHA-256" data=100 sync="subtle": 25,608.43004933131
crypto/webcrypto-digest.js n=1000 method="SHA-384" data=100 sync="subtle": 47,109.37798850117
crypto/webcrypto-digest.js n=1000 method="SHA-512" data=100 sync="subtle": 22,149.473507014736

@panva
Copy link
Member

panva commented Sep 10, 2020

@jasnell thank you for this work!

I assume the goal with the subtle interface is to 100% cover the specification, i also assume some Node.js specifics are going to be in place (e.g. passing Buffer instances, possibly also KeyObject instances to importKey?, dunno ...). I welcome the addition of built-in hkdf exposed outside of the subtle interfaces.

Altho the interface doesn't enable anything new algorithm-wise it's a great way to support modules such as jose to be isomorphic and web-compatible.

By far the biggest addition of this interface, from my point of view, is its implementation using the libuv threadpool. Being able to run both fast and slow algorithms using the same interface and not having the main thread blocked is finally here. Which brings me to obvious question - what about sign/verify/ecdh/etc operations using either key types or algorithms that are not supported by the webcrypto API specification?

Is there going to be a crypto/promises module with the existing sign/verify/publicEncrypt/privateDecrypt/createCipheriv/createDecipheriv interfaces ran using the libuv threadpool? It would be a shame if we could only get non-blocking crypto within the very limited algorithm scope of webcrypto api.

@jasnell
Copy link
Member Author

jasnell commented Sep 10, 2020

Yes, the goal is 100% compatibility with the Web Crypto API. The CryptoKey object is a wrapper around the existing KeyObject. Allowing conversion between the two using importKey and exportKey is a good idea. Buffer will be supported but is treated as if it is any other TypedArray.

Once the basic algorithms required by the WebCrypto spec are implemented, support for the broader range of algorithms we support in the legacy API will be added, starting with a few we already support such as scrypt, DH (non-ECDH), all of the ciphers, hashes, and curves reported in the getHashes, getCiphers, and getCurves APIs, and all of the keygen types supported by the existing generateKeyPair API. This will take some effort as they will be integrated into the extensible framework already provided by WebCrypto. Anything that does not fit within the WebCrypto API will be exposed directly off the crypto/promises module and not the SubtleCrypto class.

After that, I have plans for a few new algorithms we do not currently support in either API. CMAC for instance. And possibly UUID and simple HOTP/TOTP based token generation. Once the changes here are completed, it will be far easier to incorporate such additions.

@targos
Copy link
Member

targos commented Sep 10, 2020

Do you plan to integrate the WebCryptoAPI web platform tests?

@panva
Copy link
Member

panva commented Sep 10, 2020

@jasnell amazing! Happy to hear about this roadmap.

Let me know if you need assistance with testing.

@panva
Copy link
Member

panva commented Sep 10, 2020

This will take some effort as they will be integrated into the extensible framework already provided by WebCrypto

I would be very careful with extending WebCrypto with custom algorithms as the proprietary unregistered algs may clash and confuse developers thinking it's part of WebCrypto. I'd suggest to leave WebCrypto implementation inline with the current version of the spec and treat everything unregistered in WebCrypto like so 👇

Anything that does not fit within the WebCrypto API will be exposed directly off the crypto/promises module and not the SubtleCrypto class.

Ad CMAC > there's this PR that could use wrapping up. Altho from my testing it's not working as intended just yet.

@jasnell
Copy link
Member Author

jasnell commented Sep 10, 2020

@targos:

Do you plan to integrate the WebCryptoAPI web platform tests?

Absolutely, but that will likely be in a follow on PR.

@jasnell
Copy link
Member Author

jasnell commented Sep 10, 2020

@panva:

I would be very careful with extending WebCrypto with custom algorithms as the proprietary unregistered algs may clash and confuse developers thinking it's part of WebCrypto

Caution will definitely be waranted and any such extension will be in line with the extensibility guidelines of the spec (https://www.w3.org/TR/WebCryptoAPI/#extensibility). Specifically, any Node.js specific algorithms would be prefixed as such when used. For instance...

const { subtle } = require('crypto/promises');
const ec = new TextEncoder();

subtle.digest({ name: 'node.shake256', length: 100 }, ec.encode('hello'));

For well-known algorithms like Scrypt and CMAC, my intention would be to introduce those initially as node-prefixed algorithms e.g. {name: 'node-cmac' } and simultaneously open a proposal to have those added to the standard directly.

One alternative that I'm considering is exposing a separate NodeSubtleCrypto class off require('crypto/promises') that extends SubtleCrypto with the addition of the Node.js-specific extensions.

So, for instance, with this alternative, the following would fail:

const { subtle } = require('crypto/promises');
const ec = new TextEncoder();

subtle.digest({ name: 'node.shake256', length: 100 }, ec.encode('hello'));

But this would work:

const { nodeSubtle } = require('crypto/promises');
const ec = new TextEncoder();

nodeSubtle.digest({ name: 'node.shake256', length: 100 }, ec.encode('hello'));

In either case, developers will have to explicitly opt in to using the Node.js specific extensions.

@panva
Copy link
Member

panva commented Sep 10, 2020

@jasnell sounds good

codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
codebytere added a commit to electron/electron that referenced this pull request Jun 8, 2021
codebytere added a commit to electron/electron that referenced this pull request Jun 8, 2021
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
codebytere added a commit to electron/electron that referenced this pull request Jun 9, 2021
codebytere added a commit to electron/electron that referenced this pull request Jun 9, 2021
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
codebytere added a commit to electron/electron that referenced this pull request Jun 10, 2021
codebytere added a commit to electron/electron that referenced this pull request Jun 10, 2021
Before nodejs/node#35093 this include was
explicitly there but it was remove in the refactor.

Upstreamed at nodejs/node#38864.
tniessen added a commit to tniessen/node that referenced this pull request Aug 29, 2021
panva pushed a commit that referenced this pull request Sep 1, 2021
Fixes: #39936
Refs: #35093

PR-URL: #39937
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 6, 2021
Fixes: #39936
Refs: #35093

PR-URL: #39937
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 6, 2021
Fixes: #39936
Refs: #35093

PR-URL: #39937
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Commit dae283d from August 2020 introduced a call to EntropySource()
in SecretKeyGenTraits::DoKeyGen() in src/crypto/crypto_keygen.cc. There
are two problems with that:

1. It does not check the return value, it assumes EntropySource() always
   succeeds, but it can (and sometimes will) fail.

2. The random data returned byEntropySource() may not be
   cryptographically strong and therefore not suitable as keying
   material.

An example is a freshly booted system or a system without /dev/random or
getrandom(2).

EntropySource() calls out to openssl's RAND_poll() and RAND_bytes() in a
best-effort attempt to obtain random data. OpenSSL has a built-in CSPRNG
but that can fail to initialize, in which case it's possible either:

1. No random data gets written to the output buffer, i.e., the output is
   unmodified, or

2. Weak random data is written. It's theoretically possible for the
   output to be fully predictable because the CSPRNG starts from a
   predictable state.

Replace EntropySource() and CheckEntropy() with new function CSPRNG()
that enforces checking of the return value. Abort on startup when the
entropy pool fails to initialize because that makes it too easy to
compromise the security of the process.

Refs: https://hackerone.com/bugs?report_id=1690000
Refs: nodejs/node#35093
Backport-PR-URL: https://github.com/nodejs-private/node-private/pull/349
PR-URL: https://github.com/nodejs-private/node-private/pull/346
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
CVE-ID: CVE-2022-35255
tniessen added a commit to tniessen/node that referenced this pull request Jun 3, 2024
It is confusing to have both ERR_CRYPTO_SCRYPT_INVALID_PARAMETER and
ERR_CRYPTO_INVALID_SCRYPT_PARAMS. The former was the original error
code, added in 371103d, but parameter
validation gradually changed and now produces
ERR_CRYPTO_INVALID_SCRYPT_PARAMS for all parameter validation errors
coming from OpenSSL, as well as different error codes for validation
errors coming from JavaScript. The only remaining use of
ERR_CRYPTO_SCRYPT_INVALID_PARAMETER is in the validation logic that
ensures that no two synonymous options were passed. We already have an
error code for that particular case, ERR_INCOMPATIBLE_OPTION_PAIR, so
replace these last instances of ERR_CRYPTO_SCRYPT_INVALID_PARAMETER with
that error code and remove ERR_CRYPTO_SCRYPT_INVALID_PARAMETER. If there
ever is need again for such an error code, we can just use
ERR_CRYPTO_INVALID_SCRYPT_PARAMS.

Refs: nodejs#35093
Refs: nodejs#21525
Refs: nodejs#20816
nodejs-github-bot pushed a commit that referenced this pull request Jul 10, 2024
It is confusing to have both ERR_CRYPTO_SCRYPT_INVALID_PARAMETER and
ERR_CRYPTO_INVALID_SCRYPT_PARAMS. The former was the original error
code, added in 371103d, but parameter
validation gradually changed and now produces
ERR_CRYPTO_INVALID_SCRYPT_PARAMS for all parameter validation errors
coming from OpenSSL, as well as different error codes for validation
errors coming from JavaScript. The only remaining use of
ERR_CRYPTO_SCRYPT_INVALID_PARAMETER is in the validation logic that
ensures that no two synonymous options were passed. We already have an
error code for that particular case, ERR_INCOMPATIBLE_OPTION_PAIR, so
replace these last instances of ERR_CRYPTO_SCRYPT_INVALID_PARAMETER with
that error code and remove ERR_CRYPTO_SCRYPT_INVALID_PARAMETER. If there
ever is need again for such an error code, we can just use
ERR_CRYPTO_INVALID_SCRYPT_PARAMS.

Refs: #35093
Refs: #21525
Refs: #20816
PR-URL: #53305
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
It is confusing to have both ERR_CRYPTO_SCRYPT_INVALID_PARAMETER and
ERR_CRYPTO_INVALID_SCRYPT_PARAMS. The former was the original error
code, added in 371103d, but parameter
validation gradually changed and now produces
ERR_CRYPTO_INVALID_SCRYPT_PARAMS for all parameter validation errors
coming from OpenSSL, as well as different error codes for validation
errors coming from JavaScript. The only remaining use of
ERR_CRYPTO_SCRYPT_INVALID_PARAMETER is in the validation logic that
ensures that no two synonymous options were passed. We already have an
error code for that particular case, ERR_INCOMPATIBLE_OPTION_PAIR, so
replace these last instances of ERR_CRYPTO_SCRYPT_INVALID_PARAMETER with
that error code and remove ERR_CRYPTO_SCRYPT_INVALID_PARAMETER. If there
ever is need again for such an error code, we can just use
ERR_CRYPTO_INVALID_SCRYPT_PARAMS.

Refs: nodejs#35093
Refs: nodejs#21525
Refs: nodejs#20816
PR-URL: nodejs#53305
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. experimental Issues and PRs related to experimental features. lib / src Issues and PRs related to general changes in the lib or src directory. promises Issues and PRs related to ECMAScript promises. review wanted PRs that need reviews. semver-major PRs that contain breaking changes and should be released in the next major version. webcrypto
Projects
None yet
Development

Successfully merging this pull request may close these issues.