Apply clean patches from deps/ncrypto back to ncrypto#9
Closed
npaun wants to merge 23 commits intonodejs:mainfrom
Closed
Apply clean patches from deps/ncrypto back to ncrypto#9npaun wants to merge 23 commits intonodejs:mainfrom
npaun wants to merge 23 commits intonodejs:mainfrom
Conversation
PR-URL: nodejs/node#55425 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
closes #57553 closes #57572 closes #57558 PR-URL: nodejs/node#57575 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#59259 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#59436 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <richard.lau@ibm.com>
PR-URL: nodejs/node#59461 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Commit 206ebeb44764d58c6a505657edab3a7a78a0b977 added an additional call to EVP_PKEY_public_check and an unconditional return from publicCheck(). This prevents the control flow from reaching the original call to either EVP_PKEY_public_check or EVP_PKEY_public_check_quick. This change restores the previous behavior, which calls EVP_PKEY_public_check_quick instead, if possible. Refs: nodejs/node#56812 PR-URL: nodejs/node#59471 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Co-authored-by: Filip Skokan <panva.ip@gmail.com> Co-authored-by: James M Snell <jasnell@gmail.com> PR-URL: nodejs/node#50353 Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
PR-URL: nodejs/node#59569 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#59537 Reviewed-By: Tobias Nießen <tniessen@tnie.de>
PR-URL: nodejs/node#59647 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#59570 Reviewed-By: James M Snell <jasnell@gmail.com>
Adds the `signatureAlgorithm` property to a X509Certificate allowing users to retrieve a string representing the algorithm used to sign the certificate. This string is defined by the OpenSSL library. Fixes: nodejs/node#59103 PR-URL: nodejs/node#59235 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
npaun
force-pushed
the
npaun/clean-patches
branch
from
7bdd7f5 to
9718666
Compare
Contributor
Author
|
Dropped the gn stuff, but the PR itself doesn't build properly so I'll have to do this differently. |
PR-URL: nodejs/node#59539 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node#59365 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Support `outputLength` option in crypto.hash() for XOF hash functions to align with the behaviour of crypto.createHash() API closes: nodejs/node#57312 Co-authored-by: Filip Skokan <panva.ip@gmail.com> PR-URL: nodejs/node#58121 Fixes: nodejs/node#57312 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: nodejs/node#58103 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
The `CipherBase` class assumes that any authentication tag will fit into `EVP_GCM_TLS_TAG_LEN` bytes, which is true because Node.js only supports GCM with AES as the blocker cipher, and the block size of AES happens to be 16 bytes, which coincidentally is also the output size of the Poly1305 construction used by ChaCha20-Poly1305 as well as the maximum size of authentication tags produced by AES in CCM or OCB mode. This commit adds a new constant `ncrypto::Cipher::MAX_AUTH_TAG_LENGTH` which is the maximum length of authentication tags produced by algorithms that Node.js supports and replaces some constants in `CipherBase` with semantically more meaningful named constants. The OpenSSL team is debating whether a constant like `MAX_AUTH_TAG_LENGTH` (`EVP_MAX_AEAD_TAG_LENGTH`) should exist at all since its value necessarily depends on the set of AEAD algorithms supported, but I do believe that, for Node.js, this is a step in the right direction. It certainly makes more sense than to use the AES-GCM tag size as defined by TLS. PR-URL: nodejs/node#57803 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Contributor
Author
|
^^^ This push is a batch of stuff that involved manual conflict resolution but was trivially obvious. e.g. just adding a function but git didn't realize for some reason and declared a conflict. |
PR-URL: nodejs/node#59491 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Start moving src/crypto functionality out to a separate dep that can be shared with other projects that need to emulate Node.js crypto behavior. PR-URL: nodejs/node#53803 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
An `std::string_view v` is a `const char* v.data()` along with an `std::size_t v.size()` that guarantees that `v.size()` contiguous elements of type `char` can be accessed relative to the pointer `v.data()`. One of the main reasons behind the existence of `std::string_view` is the ability to operate on `char` sequences without requiring null termination, which otherwise often requires expensive copies of strings to be made. As a consequence, it is generally incorrect to assume that `v.data()` points to a null-terminated sequence of `char`, and the only way to obtain a null-terminated string from an `std::string_view` is to make a copy. It is not even possible to check if the sequence pointed to by `v.data()` is null-terminated because the null character would be at position `v.data() + v.size()`, which is outside of the range that `v` guarantees safe access to. (A default-constructed `std::string_view` even sets its own data pointer to a `nullptr`, which is fine because it only needs to guarantee safe access to zero elements, i.e., to no elements). In `deps/ncrypto` and `src/crypto`, there are various APIs that consume `std::string_view v` arguments but then ignore `v.size()` and treat `v.data()` as a C-style string of type `const char*`. However, that is not what call sites would expect from functions that explicitly ask for `std::string_view` arguments, since it makes assumptions beyond the guarantees provided by `std::string_view` and leads to undefined behavior unless the given view either contains an embedded null character or the `char` at address `v.data() + v.size()` is a null character. This is not a reasonable assumption for `std::string_view` in general, and it also defeats the purpose of `std::string_view` for the most part since, when `v.size()` is being ignored, it is essentially just a `const char*`. Constructing an `std::string_view` from a `const char*` is also not "free" but requires computing the length of the C-style string (unless the length can be computed at compile time, e.g., because the value is just a string literal). Repeated conversion between `const char*` as used by OpenSSL and `std::string_view` as used by ncrypto thus incurs the additional overhead of computing the length of the string whenever an `std::string_view` is constructed from a `const char*`. (This seems negligible compared to the safety argument though.) Similarly, returning a `const char*` pointer to a C-style string as an `std::string_view` has two downsides: the function must compute the length of the string in order to construct the view, and the caller can no longer assume that the return value is null-terminated and thus cannot pass the returned view to functions that require their arguments to be null terminated. (And, for the reasons explained above, the caller also cannot check if the value is null-terminated without potentially invoking undefined behavior.) C++20 unfortunately does not have a type similar to Rust's `CStr` or GSL `czstring`. Therefore, this commit changes many occurrences of `std::string_view` back to `const char*`, which is conventional for null-terminated C-style strings and does not require computing the length of strings. There are _a lot_ of occurrences of `std::string_view` in ncrypto and for each one, we need to evaluate if it is safe and a good abstraction. I tried to do so, but I might have changed too few or too many, so please feel free to give feedback on individual occurrences. PR-URL: nodejs/node#57816 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>
* Use ncrypto APIs where appropriate * Remove obsolete no-longer used functions * Improve error handling a bit * move secure heap handling to ncrypto To simplify handling of boringssl/openssl, move secure heap impl to ncrypto. Overall the reduces the complexity of the code in crypto_util by eliminating additional ifdef branches. * simplify CryptoErrorStore::ToException a bit * simplify error handling in crypto_common * move curve utility methods to ncrypto * verify that released DataPointers aren't on secure heap The ByteSource does not currently know how to free a DataPointer allocated on the secure heap, so just verify. DataPointers on the secure heap are not something that users can allocate on their own. Their use is rare. Eventually ByteSource is going to be refactored around ncrypto APIs so these additional checks should be temporary. * simplify some ifdefs that are covered by ncrypto * cleanup some obsolete includes in crypto_util PR-URL: nodejs/node#57323 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
An eventual goal for ncrypto is to completely abstract away details of working directly with openssl in order to make it easier to work with multiple different openssl/boringssl versions. As part of that we want to move away from direct reliance on specific openssl APIs in the runtime and instead go through the ncrypto abstractions. Not only does this help other runtimes trying to be compatible with Node.js, but it helps Node.js also by reducing the complexity of the crypto code in Node.js itself. PR-URL: nodejs/node#57300 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
npaun
force-pushed
the
npaun/clean-patches
branch
from
ef347b2 to
9ddbb75
Compare
npaun
force-pushed
the
npaun/clean-patches
branch
from
9ddbb75 to
fa2b751
Compare
Contributor
Author
|
This version was too much of a mess |
ndossche
added a commit
to ndossche/ncrypto
that referenced
this pull request
Feb 16, 2026
This function can theoretically return -1, which would then be converted
to a truthy value. If this happens, then this can cause issues at the
use sites. E.g. for the test-crypto-cipheriv-decipheriv test in Node
this can cause a buffer overflow when we test with an injected error:
```
==714700==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x502000032b98 at pc 0x558a7790bb98 bp 0x7ffdcdd5ab10 sp 0x7ffdcdd5a2c8
READ of size 12 at 0x502000032b98 thread T0
#0 0x558a7790bb97 in memcpy (/work/node/out/Debug/node+0x1b0bb97) (BuildId: adb5b2c9018fdb0733966a1a971077d03aca6d5f)
nodejs#1 0x7efe5386f90b (/lib/x86_64-linux-gnu/libcrypto.so.3+0x45f90b) (BuildId: aa3dafdd9b54db25d7c9f5335b73ca5fcb293b7f)
nodejs#2 0x7efe536312c2 in EVP_CipherInit_ex (/lib/x86_64-linux-gnu/libcrypto.so.3+0x2212c2) (BuildId: aa3dafdd9b54db25d7c9f5335b73ca5fcb293b7f)
nodejs#3 0x558a7d3e7785 in ncrypto::CipherCtxPointer::init(ncrypto::Cipher const&, bool, unsigned char const*, unsigned char const*) /work/node/out/../deps/ncrypto/ncrypto.cc:3328:10
nodejs#4 0x558a78512a1b in node::crypto::CipherBase::CommonInit(char const*, ncrypto::Cipher const&, unsigned char const*, int, unsigned char const*, int, unsigned int) /work/node/out/../src/crypto/crypto_cipher.cc:366:13
nodejs#5 0x558a785125dd in node::crypto::CipherBase::InitIv(char const*, node::crypto::ByteSource const&, node::crypto::ArrayBufferOrViewContents<unsigned char> const&, unsigned int) /work/node/out/../src/crypto/crypto_cipher.cc:409:3
nodejs#6 0x558a7850f5f4 in node::crypto::CipherBase::New(v8::FunctionCallbackInfo<v8::Value> const&) /work/node/out/../src/crypto/crypto_cipher.cc:328:11
nodejs#7 0x558a788ee605 in v8::internal::FunctionCallbackArguments::CallOrConstruct(v8::internal::Tagged<v8::internal::FunctionTemplateInfo>, bool) /work/node/out/../deps/v8/src/api/api-arguments-inl.h:93:3
nodejs#8 0x558a788ec3ba in v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::DirectHandle<v8::internal::HeapObject>, v8::internal::DirectHandle<v8::internal::FunctionTemplateInfo>, v8::internal::DirectHandle<v8::internal::Object>, unsigned long*, int) /work/node/out/../deps/v8/src/builtins/builtins-api.cc:104:16
nodejs#9 0x558a788e91fc in v8::internal::Builtin_Impl_HandleApiConstruct(v8::internal::BuiltinArguments, v8::internal::Isolate*) /work/node/out/../deps/v8/src/builtins/builtins-api.cc:135:3
nodejs#10 0x558a788e91fc in v8::internal::Builtin_HandleApiConstruct(int, unsigned long*, v8::internal::Isolate*) /work/node/out/../deps/v8/src/builtins/builtins-api.cc:126:1
nodejs#11 0x7efe31a951b5 (<unknown module>)
```
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR consists of all the commits between the
deps/ncryptoand thencryptorepo that apply cleanly as-is to thencryptorepo. Another PR is coming with changes that I need to manually adjust.