crypto: add RSA-PSS params to asymmetricKeyDetails #39851
Closed
Conversation
This file contains 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
nodejs-github-bot
added
c++
tniessen
added
the
semver-minor
|
cc @nodejs/crypto I'm not so sure about the property names. On the one hand, I'd like to align them with RFC3447. On the other hand, the hash function that is used by MGF1 is not a named property in that RFC. Also, it would be great if the Input appreciated. |
|
Also, some of the older crypto APIs are inconsistent w.r.t. short vs long names (e.g.,
|
panva
reviewed
Aug 23, 2021
It does?! Wow, let's align the names and expose those via documentation as well. |
jasnell
approved these changes
Aug 23, 2021
panva
added
request-ci
github-actions
bot
removed
the
request-ci
This comment has been minimized.
This comment has been minimized.
|
I took the liberty to
|
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
panva
force-pushed
the
crypto-add-rsa-pss-key-details
branch
from
594cdcd
to
9319f2f
Compare
|
I will push the alignment of yet undocumented key gen parameters with what we expose here as a separate PR marked semver-major to be on the safe side. Edit: #39927 |
panva
added
the
author ready
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I am still unsure what the best param names are.
|
👍 |
tniessen
commented
Aug 29, 2021
This comment has been minimized.
This comment has been minimized.
panva
approved these changes
Aug 29, 2021
jasnell
approved these changes
Aug 29, 2021
panva
pushed a commit
that referenced
this pull request
Aug 29, 2021
Fixes: #39837 Refs: openssl/openssl#10568 PR-URL: #39851 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Filip Skokan <[email protected]>
|
Landed in b6b638b |
targos
pushed a commit
that referenced
this pull request
Sep 6, 2021
Fixes: #39837 Refs: openssl/openssl#10568 PR-URL: #39851 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Filip Skokan <[email protected]>
targos
pushed a commit
that referenced
this pull request
Sep 6, 2021
Fixes: #39837 Refs: openssl/openssl#10568 PR-URL: #39851 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Filip Skokan <[email protected]>
targos
added a commit
that referenced
this pull request
Sep 6, 2021
Notable changes: crypto: * (SEMVER-MINOR) add RSA-PSS params to asymmetricKeyDetails (Tobias Nießen) #39851 deps: * (SEMVER-MINOR) add corepack (Maël Nison) #39608 * (SEMVER-MINOR) update V8 to 9.3.345.16 (Michaël Zasso) #39947 module: * (SEMVER-MINOR) support pattern trailers (Guy Bedford) #39635 stream: * (SEMVER-MINOR) add stream.compose (Robert Nagy) #39029 PR-URL: #40011
targos
added a commit
that referenced
this pull request
Sep 6, 2021
Notable changes: crypto: * (SEMVER-MINOR) add RSA-PSS params to asymmetricKeyDetails (Tobias Nießen) #39851 deps: * (SEMVER-MINOR) add corepack (Maël Nison) #39608 * (SEMVER-MINOR) update V8 to 9.3.345.16 (Michaël Zasso) #39947 module: * (SEMVER-MINOR) support pattern trailers (Guy Bedford) #39635 stream: * (SEMVER-MINOR) add stream.compose (Robert Nagy) #39029 PR-URL: #40011
targos
added a commit
that referenced
this pull request
Sep 7, 2021
Notable changes: crypto: * (SEMVER-MINOR) add RSA-PSS params to asymmetricKeyDetails (Tobias Nießen) #39851 deps: * (SEMVER-MINOR) add corepack (Maël Nison) #39608 * (SEMVER-MINOR) update V8 to 9.3.345.16 (Michaël Zasso) #39947 module: * (SEMVER-MINOR) support pattern trailers (Guy Bedford) #39635 stream: * (SEMVER-MINOR) add stream.compose (Robert Nagy) #39029 PR-URL: #40011
codebytere
added a commit
to electron/electron
that referenced
this pull request
Sep 8, 2021
agl
pushed a commit
to google/boringssl
that referenced
this pull request
Sep 10, 2021
This CL adds a maskHash member to the rsa_pss_params_st struct for increased compatibility with OpenSSL: https://source.chromium.org/chromium/chromium/src/+/main:third_party/perl/c/include/openssl/rsa.h;l=282-289 Node.js recently began to make use of this member in nodejs/node#39851 and without this member Electron sees compilation errors. Change-Id: Ibd18a31605b0a715edb279a3bca4b4f05e679767 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49365 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
|
Regarding backporting this to older versions, I'm assuming this is NOT for 14.x or older? Backporting this would require bumping OpenSSL dependency to 1.1.1e and for distros that backport fixes for OpenSSL, this could be problematic. |
|
@AdamMajer both 12 and 14 are now in maintenance and won't be getting this feature (or its dependencies) backported. |
dstebila
pushed a commit
to open-quantum-safe/boringssl
that referenced
this pull request
Feb 14, 2022
* Document that SSL_PRIVATE_KEY_METHOD should configure signing prefs. Otherwise BoringSSL may select one the private key does not support. Change-Id: Ia0a57657bd6dedaa6653c23cc850bb6b6fa8f219 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48525 Reviewed-by: Adam Langley <[email protected]> * Add convenience functions to malloc EVP_HPKE_CTX and EVP_HPKE_KEY. Some callers want the value to be heap-allocated. It's a little annoying that this returns an empty value (if we only supported heap-allocated ones, I'd have merged init into new), but since we have multiple constructor functions, this is probably the least fuss. Change-Id: I42f586e39850954fb6743f8be50a7cfffa0755ba Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48526 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Check strtoul return for overflow error in GetUnsigned() Currently, GetUnsigned() calls strtoul and checks whether the resulting unsigned long int is greater than UINT_MAX. This implicitly assumes that UINT_MAX < ULONG_MAX. Problematically, `unsigned long int` and `unsigned` have the same size on Windows [0] and on 32-bit architectures. For correctness, we now check whether strtoul failed because it would overflow the unsigned long int before checking whether the value fits in an unsigned type. [0]: https://docs.microsoft.com/en-us/cpp/cpp/data-type-ranges?view=msvc-160 Change-Id: I49702febf4543bfb7991592717443e0b2adb954f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48545 Commit-Queue: Dan McArdle <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: David Benjamin <[email protected]> * Don't enable atomics in NO_THREADS configurations. In configurations without threads, we're not thread-safe anyway. Instead use the refcount_lock.c implementation which, in turn, calls into thread_none.c, so this turns into a plain refcount. This avoids a build issue on platforms which define NO_THREADS, use C11, lack C11 atomics, and are missing a __STDC_NO_ATOMICS__ definition. The platforms ought to define __STDC_NO_ATOMICS__ or implement them, but atomics are also unnecessary overheard in NO_THREADS configurations anyway. Change-Id: I927e1825dd6474d95226b93dad704594f120450a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48565 Commit-Queue: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Add 'generate-ech' command to bssl tool The tool generates three files: an ECHConfig, its corresponding private key, and the ECHConfig wrapped in an ECHConfigList. For example, the following invocation generates the files: bssl generate-ech \ -out-ech-config-list ech_config_list.data \ -out-ech-config ech_config.data \ -out-private-key ech.key \ -public-name foo.example \ -config-id 0 Now, we can pass the ECHConfig and private key into the 'server' and 'client' commands: bssl server -accept 4430 \ -ech-config ech_config.data \ -ech-key ech.key bssl client -connect localhost:4430 \ -ech-config-list ech_config_list.data Bug: 275 Change-Id: Id4342855483fb01aa956f9aff356105c4a8ca4f6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48466 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> * acvp: add HKDF support. Change-Id: I26251ce85f2cb1b441ae415b1506161a90bd3efa Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48585 Reviewed-by: David Benjamin <[email protected]> * Revert "Revert "Revert "Disable check that X.509 extensions implies v3.""" This reverts commit be9a86f459f8e785bac42abcea5d13bd4ded251e. Let's try this again. Bug: 375 Change-Id: Ie01cced8017835b2cc6d80e5e81a4508a37fbbaf Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48625 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]> * acvp: recognise another style of JSON. Some JSON files have a header, but without a URL. Thus consider a block that doesn't contain an algorithm to also be a header. Change-Id: Ic35a827843e9d0169ba8398df69c46a5baeffb44 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48605 Reviewed-by: David Benjamin <[email protected]> * Don't overread in poly_Rq_mul The polynomials have 701, 16-bit values. But poly_Rq_mul was reading 32 bytes at offset 1384 in order to get the last 18 of them. This silently worked for a long time, but when 7153013019 switched to keeping variables on the stack it was noticed by Valgrind. This change fixes the overread. Setting watchpoints at the ends of the two inputs (and one output) now shows no overreads nor overwrites. BUG=424 Change-Id: Id86c1407ffce66593541c10feee47213f4b95c5d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48645 Reviewed-by: David Benjamin <[email protected]> * generate_ech.cc: include needed headers Change-Id: I04c8bb68801aeb0938e5b038b98811ca4ffe50f0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48685 Commit-Queue: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: David Benjamin <[email protected]> * Clarify BIO_new_mum_buf's lifetime rules. It is not obvious from "It does not take ownership of |buf|" whether the function makes a copy or not. It does not make a copy (maybe it should...), so callers are obligated to manage their lifetimes. Change-Id: I7df9a5814321fd833fcb8d009d9e0318d6668dd4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48669 Reviewed-by: Adam Langley <[email protected]> * Document another batch of functions. This covers most of the ASN.1 time functions and a handful more of x509.h. Also remove some code under #if 0. I'm running out of a easy ones to do, which is probably a good thing. Change-Id: I085b1e2a54d191a7a5f18c801b3c135cfda7bd88 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48665 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]> * Remove ASN1_STRING_FLAG_MSTRING. This flag is set when an ASN1_STRING is created from a codepath that is aware it is an "mstring" (CHOICE of multiple string or string-like types). With setters like X509_set_notBefore, it is very easy to accidentally lose the flag on some field that normally has it. The only place the flag is checked is X509_time_adj_ex. X509_time_adj_ex usually transparently picks UTCTime vs GeneralizedTime, as in the X.509 CHOICE type. But if writing to an existing object AND if the object lacks the flag, it will lock to whichever type the object was previously. It is likely any caller hitting this codepath is doing so unintentionally and has a latent bug that won't trip until 2050. In fact, one of the ways callers might accidentally lose the ASN1_STRING_FLAG_MSTRING flag is by using X509_time_adj_ex! X509_time_adj_ex(NULL) does not use an mstring-aware constructor. This CL avoids needing such a notion in the first place. Looking through callers, the one place that wants the old behavior is a call site within OpenSSL, to set the producedAt field in OCSP. That field is a GeneralizedTime, rather than a UTCTime/GeneralizedTime CHOICE. We dropped that code, but I'm making a note of it to remember when filing upstream. Update-Note: ASN1_STRING_FLAG_MSTRING is no longer defined and X509_time_adj_ex now behaves more predictably. Callers that actually wanted to lock to a specific type should call ASN1_UTCTIME_adj or ASN1_GENERALIZEDTIME_adj instead. Change-Id: Ib9e1c9dbd0c694e1e69f938da3992d1ffc9bd060 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48668 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Add some tests for time_t to ASN1_TIME conversions. Change-Id: I7712f66e16b761ee23292980cff039e62d29b22f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48666 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Reject years outside 0000-9999 in ASN1_GENERALIZEDTIME_adj. They would previously output syntax errors. Change-Id: I7817a91d0c8ed8d6ac6a5a1fd9c9ed1223c5960e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48667 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Avoid double-expanding variables in CMake. CMake's language is rather fragile and unsound. For the most part, it is a shell script with more parentheses. That is, it simply expands command arguments into a list of strings and then evaluates it, complete with shell-style differences between "${FOO}" and ${FOO}. The if() command is special and internally also expands variables. That is why things like if(FOO STREQUAL "BAR") work. CMake interprets "FOO" as a variable if it can find a variable, or a string otherwise. In addition to getting very confused on typos, it means that if("${FOO}" STREQUAL "BAR") will double-expand, and it will do strange things if BAR is a variable. CMP0054 patches this (which we set by minimum version) so that if() only expands if the token was unquoted. This fixes if("${FOO}" STREQUAL "BAR"). However, if(${FOO} STREQUAL "BAR") continues to double-expand FOO. We had a mix of all three of FOO, ${FOO}, and "${FOO}". It's not clear which is the canonical spelling at this point, but CMake own files (mostly) use FOO, as do most of our lines, so I've standardized on that. It's a little unsatisfying if we typo a variable, but I suppose ${FOO} also silently ignores unset variables. Bug: 423 Change-Id: Ib6baa27f4065eed159e8fb28820b71a0c99e0db0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48705 Reviewed-by: Adam Langley <[email protected]> * Align with upstream on 'close STDOUT' lines. When upstreaming c1d8c5b0e0ff4177ec06eed58ebcfd5a75b7f231 as https://github.com/openssl/openssl/pull/10883 and then https://github.com/openssl/openssl/pull/10930, we ended up diverging slightly: in the upstream version, I ended up applying the same change to the xlate files. Upstream also suggested "error closing STDOUT: $!". Apply the same changes here. Change-Id: I8a8cbc3944432e94a8844f9f628a900edfe77b30 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48725 Reviewed-by: Adam Langley <[email protected]> * Update ghashv8-armx.pl from upstream. This syncs this file up to e7ff223a20697e5a401d2d9bb7a75e699ed46633 from upstream's OpenSSL_1_1_1-stable branch. The main change of note is the 4x loop from upstream's 7ff2fa4b9281232f0ca1db03d42a954c462ef77d, 9ee020f8dc7813db82a119058d8f57e70e7e8904, aa7bf316980259a11dcbaf6128ed86d33dc24b97, and 603ebe03529101424670051aa0c616dc6e037b28. Benchmarks on a Pixel 4a. Before: Did 14069000 AES-128-GCM (16 bytes) seal operations in 2000042us (112.5 MB/sec) Did 6768000 AES-128-GCM (256 bytes) seal operations in 2000182us (866.2 MB/sec) Did 1902000 AES-128-GCM (1350 bytes) seal operations in 2000479us (1283.5 MB/sec) Did 359000 AES-128-GCM (8192 bytes) seal operations in 2003942us (1467.6 MB/sec) Did 182000 AES-128-GCM (16384 bytes) seal operations in 2002245us (1489.3 MB/sec) Did 13388000 AES-256-GCM (16 bytes) seal operations in 2000144us (107.1 MB/sec) Did 6069000 AES-256-GCM (256 bytes) seal operations in 2000276us (776.7 MB/sec) Did 1638000 AES-256-GCM (1350 bytes) seal operations in 2001076us (1105.1 MB/sec) Did 305000 AES-256-GCM (8192 bytes) seal operations in 2000040us (1249.3 MB/sec) Did 155000 AES-256-GCM (16384 bytes) seal operations in 2009398us (1263.8 MB/sec) After: Did 13837000 AES-128-GCM (16 bytes) seal operations in 2000131us (110.7 MB/sec) [-1.7%] Did 7506000 AES-128-GCM (256 bytes) seal operations in 2000197us (960.7 MB/sec) [+10.9%] Did 2289000 AES-128-GCM (1350 bytes) seal operations in 2000734us (1544.5 MB/sec) [+20.3%] Did 443000 AES-128-GCM (8192 bytes) seal operations in 2000321us (1814.2 MB/sec) [+23.6%] Did 225000 AES-128-GCM (16384 bytes) seal operations in 2002308us (1841.1 MB/sec) [+23.6%] Did 13280000 AES-256-GCM (16 bytes) seal operations in 2000011us (106.2 MB/sec) [-0.8%] Did 6630000 AES-256-GCM (256 bytes) seal operations in 2000229us (848.5 MB/sec) [+9.2%] Did 1916000 AES-256-GCM (1350 bytes) seal operations in 2000373us (1293.1 MB/sec) [+17.0%] Did 365000 AES-256-GCM (8192 bytes) seal operations in 2001519us (1493.9 MB/sec) [+19.6%] Did 185000 AES-256-GCM (16384 bytes) seal operations in 2006588us (1510.5 MB/sec) [+19.5%] (See cl/387919990 for some notes I made in reviewing, though likely future me will find them incomprehensible anyway.) Change-Id: Id386e80143611487e07b2fbfda15d0abc54ea145 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48726 Reviewed-by: Adam Langley <[email protected]> * Document ASN1_mbstring_copy. Change-Id: Ia2cb9d969b25d1815d8157dd74125d60b138138f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48765 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Always use an ASN1_STRING_TABLE global mask of UTF8String. ASN1_STRING_set_by_NID is very complex and depends on a "global mask" for most NIDs. (Some NIDs use a single type and use STABLE_NO_MASK to disable the global mask.) Historically, it defaulted to allowing all types, but it switched to UTF8String in OpenSSL 1.0.2. Updating the global mask is not thread-safe, and it's 2021. Let's just always use UTF-8. The only callers I found set it to UTF-8 anyway (with the exception of some test script we don't use, and some code that wasn't compiled). No-op writes in the C/C++ memory model are still race conditions, so this CL fixes some bugs in those callers. Update-Note: The global mask for ASN1_STRING_set_by_NID is now always UTF-8. Callers that want another type should reconsider and, if UTF-8 is still unsuitable, just pass the actual desired type into ASN1_mbstring_copy, X509_NAME_ENTRY_set_data, etc Change-Id: I679e99c57da9a48c805460abcb3af5b2f938c93f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48766 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Unexport ub_* constants. These constants aren't suitably namespaced and, moreover, are redefined in a_strnid.c. (The constants aren't especially useful because an X509_NAME doesn't check the upper bound.) Update-Note: Removed some unnamespaced constants. Change-Id: I7d15ae731628d3665119081289947600e7f38065 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48768 Reviewed-by: Adam Langley <[email protected]> * Unexport BIT_STRING_BITNAME. This type does not appear in any public APIs. Change-Id: Ie57c7662e691ea05ff2133beda9760832ea0d0de Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48769 Reviewed-by: Adam Langley <[email protected]> * Move X509_ALGOR to x509.h. This matches OpenSSL and the name. Also accessors like X509_ALGOR_get0 are in x509.h. Change-Id: Ic7583edcf04627cbfae822df11e75eebdd9ad7aa Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48770 Reviewed-by: Adam Langley <[email protected]> * Remove OPENSSL_NO_FP_API ifdefs. We've never tested this and plenty of files depend on FILE* APIs without ifdefs. Change-Id: I8c51c043e068b30bdde1723c3810d3e890eabfca Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48771 Reviewed-by: Adam Langley <[email protected]> * Implement ASN1_STRING_print_ex_fp, etc., with file BIOs. No sense in implementing a BIO/FILE abstraction when BIO is itself a FILE abstraction. Follow-up CLs will unwind the char_io abstraction and then split the ASN1 and X509 bits of this file. Change-Id: I00aaf2fbab44abdd88252ceb5feb071ad126a0b2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48772 Reviewed-by: Adam Langley <[email protected]> * Unwind io_ch abstraction in print functions. Change-Id: Ib342ce1acf7ea4fcff012bf149cf699807ddc0fa Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48773 Reviewed-by: Adam Langley <[email protected]> * Move a_strex.c back to asn1, split X509_NAME bits out. With io_ch unwound, X509_NAME_print_ex just calls ASN1_STRING_print_ex, so we can put all the code in the right directories. We need to duplicate maybe_write, but it's a one-line function. Change-Id: Ifaa9f1a24ee609cbaa24f93eb992f7d911f1b4a0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48774 Reviewed-by: Adam Langley <[email protected]> * Move some ASN1 printing functions to crypto/asn1. For some reason, ASN1_STRING_print was not in the same file as ASN1_STRING_print_ex, but X509_print. Although it also behaves very differently from ASN1_STRING_print_ex, so that's a little interesting. Change-Id: I3f88f8943c8e36426eedafa7e350a787881d0c74 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48775 Reviewed-by: Adam Langley <[email protected]> * Document ASN.1 printing functions. ASN1_STRING_print_ex is extremely complex and attempting to implement RFC2253, so write some tests for it. Along the way, unexport CHARTYPE_*, which are internal book-keeping used in ASN1_STRING_print_ex. Change-Id: Idb27cd40fb66dc099d1fd6d039a00404608c2063 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48776 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Check i2d_ASN1_TYPE's return value in ASN1_STRING_print_ex. Also use the simpler single-call variant. Change-Id: I3834a798549f12a9dcdec6a357d2380085baf940 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48777 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Fix ASN1_STRING_print_ex with negative integers. ASN1_STRING and ASN1_TYPE type values almost line up, but not quite. Negative INTEGERs are not possible in X509_NAME (tag2bit maps INTEGER to 0), but negative ENUMERATEDs are (tag2bit maps ENUMERATED to B_ASN1_UNKNOWN). See https://crbug.com/boringssl/412 for some notes on this mess. Either way, the library will freely produce ASN1_STRING INTEGERs and ENUMERATEDs in non-MSTRING contexts, so get this case right. Change-Id: Ica537f4d683e7a6becc96e2eee3cb66e53372124 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48785 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Remove ASN1_TFLG_SET_ORDER. ASN1_TFLG_SET_ORDER was used in OpenSSL's CMS and PKCS#7 implementations, which we've removed. Fields that use it not only get the DER SET sorting but, when serialized, go back and mutate the original object to match. This is unused, so remove it. This removes one of the sources of non-const behavior in i2d functions. Bug: 407 Change-Id: I6b2bf8d11c30a41b53d14ad475c26a1a30dfd31f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48786 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Add a test for ASN1_mbstring_copy and clean up. In writing the tests, I noticed that the documentation was wrong. First, the maximum lengths are measured in codepoints, not bytes. Second, the TODO was wrong. We actually do handle this correctly, *almost*. Rather, the bug is that the function assumes |mask| contains no extraneous bits. If it does, all extraneous bits are interpreted as B_ASN1_UTF8STRING. This seems like a bug, so I've gone ahead and fixed that, with a test. Change-Id: I7ba8fa700a8e21e6d25cb7ce879dace685eecf7e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48825 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Fix negative ENUMERATED values in multi-strings. I noticed this while I was reading through the encoder. OpenSSL's ASN.1 library is very sloppy when it comes to reusing enums. It has... - Universal tag numbers. These are just tag numbers from ASN.1 - utype. These are used in the ASN1_TYPE type field, as well as the ASN1_ITEM utype fields They are the same as universal tag numbers, except non-universal types map to V_ASN1_OTHER. I believe ASN1_TYPE types and ASN1_ITEM utypes are the same, but I am not positive. - ASN1_STRING types. These are the same as utypes, except V_ASN1_OTHER appears to only be possible when embedded inside ASN1_TYPE, and negative INTEGER and ENUMERATED values get mapped to V_ASN1_NEG_INTEGER and V_ASN1_NEG_ENUMERATED. Additionally, some values like V_ASN1_OBJECT are possible in a utype but not possible in an ASN1_STRING (and will cause lots of problems if ever placed in one). - Sometimes one of these enums is augmented with V_ASN1_UNDEF and/or V_ASN1_APP_CHOOSE for extra behaviors. - Probably others I'm missing. These get mixed up all the time. asn1_ex_i2c's MSTRING path converts from ASN1_STRING type to utype and forgets to normalize V_ASN1_NEG_*. This means that negative INTEGERs and ENUMERATEDs in MSTRINGs do not get encoded right. The negative INTEGER case is unreachable (unless the caller passes the wrong ASN1_STRING to an MSTRING i2d function, but mismatching i2d functions generally does wrong things), but the negative ENUMERATED case is reachable. Fix this and add a test. Change-Id: I762d482e72ebf03fd64bba291e751ab0b51af2a9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48805 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Fix some error returns from SSL_read and SSL_write. It's a bit of a mess, but BIO-like APIs typically return -1 on error and 0 for EOF. Change-Id: Ibdcb70e1009ffebf6cc6df40804dc4a178c7199e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48845 Reviewed-by: Adam Langley <[email protected]> * Simplify built-in BIOs slightly. The free callbacks can assume their inputs are non-NULL. They're only called from BIOs of the corresponding method, which means the BIO must exist. Also new callbacks that leave everything zero-initialized are no-ops and can be omitted. This removes the weird thing where the built-in free functions were fallible. Although the int return is still necessary for compatibility with external BIOs. Change-Id: I91e2101efc7c77c703cb649df1490bc9f515f0fd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48846 Reviewed-by: Adam Langley <[email protected]> * Add Span::first() and Span::last(). absl::Span, base::span, and std::span have first() and last() methods which give prefixes and suffixes. first() just saves 5 characters, but last() is nicer to write than subspan() for suffixes. Unlike subspan(), they also do not have clipping behavior, so we're guaranteed the length is correct. The clipping behavior comes from absl::Span::subspan() and is not present in std::span or base::span. I've left it in, in case we switch to absl::Span in the future, but I imagine absl::Span will need to migrate this at some point. Change-Id: I042dd6c566b6d753ec6de9d84e8c09ac7c270267 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48905 Reviewed-by: Adam Langley <[email protected]> * Bump minimum GCC version and note impending VS2015 deprecation. GCC 6.1 was released more than five years ago, April 27, 2016. We can thus drop some bits in the CMake files. https://gcc.gnu.org/releases.html https://gcc.gnu.org/develop.html#num_scheme Also note in BUILDING.md that VS2015 will no longer be supported next year. Then we can cycle our CQ to testing VS2017 + VS2019. (We're currently not testing VS2019 at all, though so far it hasn't been an issue.) I've been running into some VS2015-only C++ issues around conversions, so once we stop testing it, I expect it'll break. Change-Id: I7a3020df2acd61d57409108aa4d99c840b5ca994 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48925 Reviewed-by: Adam Langley <[email protected]> * Guard use of sdallocx with BORINGSSL_SDALLOCX See comment in change and https://github.com/grpc/grpc/issues/25450 Update-note: consumers may wish to define BORINGSSL_SDALLOCX if using tcmalloc. Change-Id: I123fe31a6c4013f1ce0c056f82a316c71df84939 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48885 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]> * Process the TLS 1.3 cipher suite in one place. The cipher suite, like the version, is determined by the first server message, independent of whether it's ServerHello or HelloRetryRequest. We can simplify this by just processing it before we branch on which it was. Change-Id: I747f515e9e5b05a42cbed6e7844808d0fc79a30b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48906 Reviewed-by: Adam Langley <[email protected]> * runner: Test session IDs over 32 bytes. The session ID field cannot exceed 32 bytes, and we size various buffers based on this. Test that our parsers correctly handle this. Also fix the -wait-for-debugger flag. I broke it recently by removing the statusShimStarted message. Change-Id: I29bb177f29a79bb4904fb5ba3cedfb0b6b856061 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48907 Reviewed-by: Adam Langley <[email protected]> * Refer to RFCs consistently. We were a mix of "RFC1234" and "RFC 1234". Apparently there is actually an answer for this, which is with a space textually and without a space in the citation/reference tag: https://datatracker.ietf.org/doc/html/rfc7322#section-3.5 Change-Id: I0c44023163fe3a2a3ffe28cbc644d4c952dc8f1e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48965 Reviewed-by: Adam Langley <[email protected]> * Linkify RFCs in documentation. Change-Id: If42bc55c1381dc50dd1125c2780edc6cafa964cb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48966 Reviewed-by: Adam Langley <[email protected]> * Add a CBB_add_zeros helper. We fill in placeholder values of all zeros fairly often in TLS now, as workarounds for messages being constructed in the wrong order. draft-12 of ECH adds even more of these. Add a helper so we don't need to interrupt an || chain with a memset. Change-Id: Id4f9d988ee67598645a01637cc9515b475c1aec2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48909 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Do not rely on ASN1_STRING being NUL-terminated. This imports part of the fix for CVE-2021-3712, commits d9d838ddc0ed083fb4c26dd067e71aad7c65ad16, 5f54e57406ca17731b9ade3afd561d3c652e07f2, 23446958685a593d4d9434475734b99138902ed2, and bb4d2ed4091408404e18b3326e3df67848ef63d0 from upstream. The others will be imported in follow-up CLs. Change-Id: Ic35aeb3895935ee94b82a295efade32782e8d1bc Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49005 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]> * Fix i2v_GENERAL_NAME to not assume NUL terminated strings See also 174ba8048a7f2f5e1fca31cfb93b1730d9db8300 from upstream. This differs from the upstream CL in that: - We don't silently drop trailing NULs. - As a NUL-terminated C string, the empty string is a non-NULL pointer to an array containing a zero byte. Use the latter consistently. Change-Id: I99c6c4c26be5a1771c56c6ab356425f1b85be41d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49006 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Add some tests for name constraints. Change-Id: I51606bb7e4674716ffb6688b3a8e69db3f014546 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49007 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Rewrite name constraints matching with CBS. See also 8393de42498f8be75cf0353f5c9f906a43a748d2 from upstream and CBS-2021-3712. But rather than do that, I've rewritten it with CBS, so it's a bit clearer. The previous commit added tests. Change-Id: Ie52e28f07b9bf805c8730eab7be5d40cb5d558b6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49008 Reviewed-by: Adam Langley <[email protected]> * OPENSSL_strndup should not return NULL given {NULL, 0}. The NUL-terminated representation of the empty string is a non-NULL one-byte array, not NULL. This fills in the last of the empty string cases in https://boringssl-review.googlesource.com/c/boringssl/+/49006/ Change-Id: I66c09dc3223f762b708612987b26c90e41e27c4a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49009 Reviewed-by: Adam Langley <[email protected]> * Fix typo. Subsequent CLs will add some fuzzers, etc., that'll help with catching this. Change-Id: I10a8e4b2f23ffd07b124e725c1f7454e7ea6f2dd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49025 Reviewed-by: Adam Langley <[email protected]> * Fix some error-handling in i2v functions. See upstream commits: 32f3b98d1302d4c0950dc1bf94b50269b6edbd95 432f8688bb72e21939845ac7a69359ca718c6676 7bb50cbc4af78a0c8d36fdf2c141ad1330125e2f 8c74c9d1ade0fbdab5b815ddb747351b8b839641 Change-Id: Iff614260c1b1582856edb4ae7a226f2e07537698 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49045 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Run X509_print in the certificate fuzzer. Given the error handling issues in the previous CL, we'll probably be chasing down bugs in there for a while. Change-Id: I7a219e0fe2496f602d38b4bd0fcd5585ebd72cb7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49046 Reviewed-by: Adam Langley <[email protected]> * Merge in OpenSSL's X.509 corpus. Ran the following command at OpenSSL commit 18622c7625436d7f99c0f51895c4d3cea233c62e: ./build-fuzz/fuzz/cert -merge=1 -max_len=10000 fuzz/cert_corpus/ ~/openssl/fuzz/corpora/x509 Change-Id: I22c4051351138736a0fa0202c0977ca9afc6924c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49047 Reviewed-by: Adam Langley <[email protected]> * Deduplicate our three ServerHello parsers. We do this enough that it's worth extracting a common parser. And this gives a struct we can pass around. Note this moves the server extensions block parsing out of ssl_scan_serverhello_tlsext. I've also consolidated a few error conditions to tighten the code up a bit: the TLS 1.2 code distinguishes unknown from unadvertised cipher, while the TLS 1.3 code didn't. And seeing the wrong legacy version number in TLS 1.3 is really just a syntax error since it's not the version field anymore. (RFC8446 specifies the value.) Change-Id: Ia2f44ff9a3899b5a594569f1b258f2b487930496 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48908 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Make ssl_parse_extensions a little easier to use. std::initializer_list appears to work by instantiating a T[N] at the call site (which is what we were doing anyway), so I don't believe there is a runtime dependency. This also adds a way for individual entries to turn themselves off, which means we don't need to manually check for some unsolicited extensions. Change-Id: I40f79b6a0e9c005fc621f4a798fe201bfbf08411 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48910 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Avoid re-hashing the transcript multiple times. tls13_init_key_schedule calls InitHash internally, but we also call InitHash earlier at various times. On the client, we do it early to handle HelloRetryRequest and 0-RTT. ECH draft-12 will also need to do it early. Apparently we do it early on the server too. Probably tls13_init_key_schedule doesn't need to call InitHash, but for now, it is an easy check in SSLTranscript. Change-Id: I5473047c1f29bdeb60901e4e6e80979e592bd6e9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48911 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Work around yet another MSVC 2015 SFINAE bug. Although we defined a CBS -> Span<const uint8_t> conversion, MSVC 2015 keeps trying to call the Span(const Container&) constructor. It seems to not correctly SFINAE the existence of data() and size() members unless the expression is inlined into the default template argument. Change-Id: I4e88f820b78ce72ad1b014b5bae0830bc7d099d4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48945 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Benchmark RSA private key parsing. We do non-trivial work when parsing RSA private keys (RSA_check_key) and, in some consumers, this is performance-sensitive. Bug: b/192484677 Change-Id: Ic27f5f11d8bd030de77dd500a826fb2dd7c5b75d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49105 Reviewed-by: Adam Langley <[email protected]> * Make RSA_check_key more than 2x as fast. The bulk of RSA_check_key is spent in bn_div_consttime, which is a naive but constant-time long-division algorithm for the few places that divide by a secret even divisor: RSA keygen and RSA import. RSA import is somewhat performance-sensitive, so pick some low-hanging fruit: The main observation is that, in all but one call site, the bit width of the divisor is public. That means, for an N-bit divisor, we can skip the first N-1 iterations of long division because an N-1-bit remainder cannot exceed the N-bit divisor. One minor nuisance is bn_lcm_consttime, used in RSA keygen has a case that does *not* have a public bit width. Apply the optimization there would leak information. I've implemented this as an optional public lower bound on num_bits(divisor), which all but that call fills in. Before: Did 5060 RSA 2048 private key parse operations in 1058526us (4780.2 ops/sec) Did 1551 RSA 4096 private key parse operations in 1082343us (1433.0 ops/sec) After: Did 11532 RSA 2048 private key parse operations in 1084145us (10637.0 ops/sec) [+122.5%] Did 3542 RSA 4096 private key parse operations in 1036374us (3417.7 ops/sec) [+138.5%] Bug: b/192484677 Change-Id: I893ebb8886aeb8200a1a365673b56c49774221a2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49106 Reviewed-by: Adam Langley <[email protected]> * NUL is not printable. strchr is interprets the trailing NUL as part of the string, so is_printable thought NUL was allowed. Just write the code in the obvious way and let the compiler figure it out. (It seems to make a clever bitmask or something.) Update-Note: ASN1_mbstring_ncopy will no longer allow PrintableString for strings containing NUL. Change-Id: I3675191ceb44c06f0ac7b430f88272cabf392d35 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49065 Reviewed-by: Adam Langley <[email protected]> * Include SHA512-256 in EVP_get_digestbyname and EVP_MD_do_all. Change-Id: I25a1a58589ec8843da4d1955d8fec38561f13ec9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49125 Reviewed-by: Adam Langley <[email protected]> * Rewrite ASN1_PRINTABLE_type and add tests. The old loop read one byte past the length. It also stopped the loop too early on interior NUL. See also upstream's https://github.com/openssl/openssl/pull/16433, though I've opted to rewrite the function entirely rather than use their fix. Also deduplicate the PrintableString check. Change-Id: Ia8bd282047c2a2ed1d5e71a68a3947c7c108df95 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49066 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Remove V_ASN1_APP_CHOOSE. V_ASN1_APP_CHOOSE has been discouraged by OpenSSL since 2000: https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=CHANGES;h=824f421b8d331ba2a2009dbda333a57493bedb1e;hb=fb047ebc87b18bdc4cf9ddee9ee1f5ed93e56aff#l10848 Instead, upstream recommends an MBSTRING_* constant. https://www.openssl.org/docs/man1.1.1/man3/X509_NAME_add_entry_by_NID.html This function is a bit overloaded: MBSTRING_* means "Decode my input from this format and then re-encode it using whatever string type best suits the NID (usually UTF8String, but some NIDs require PrintableString)". V_ASN1_APP_CHOOSE means "This is a Latin-1 string. Without looking at the NID, pick one of PrintableString, IA5String, or T61String". The latter is almost certainly not what callers want. If they want a particular type, they can always force it by passing a particular V_ASN1_* constant. This removes the only use of ASN1_PRINTABLE_type within the library, though there is one external use still. Update-Note: V_ASN1_APP_CHOOSE is removed. I only found one use, which has been fixed. Change-Id: Id36376dd0ec68559bbbb366e2305d42be5ddac67 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49067 Reviewed-by: Adam Langley <[email protected]> * Make most of crypto/x509 opaque. This unexports X509, X509_CINF, X509_NAME_ENTRY, X509_NAME, X509_OBJECT, X509_LOOKUP_METHOD, X509_STORE, X509_LOOKUP, and X509_STORE_CTX. Note this means X509_STORE_CTX can no longer be stack-allocated. Update-Note: Patch cl/390055173 into the roll that includes this. This unexports most of the X.509 structs, aligning with OpenSSL. Use the accessor APIs instead. Bug: 425 Change-Id: I53e915bfae3b8dc4b67642279d0e54dc606f2297 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48985 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Remove SSL_set_verify_result. Follow-up from https://boringssl-review.googlesource.com/10485 that I forgot about. It's been removed from netty-tcnative. Change-Id: Ic4b97b30787962b78a69911a6e3cd28647546f59 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49145 Reviewed-by: Adam Langley <[email protected]> * Reword SSL_get0_ech_name_override documentation. Hopefully it's a little clearer that this may be called whether or not ECH is offered. (And whether or not it's a server.) Bug: 275 Change-Id: I39c8ce5758543a0cfda84652b3fc0a5b9669fd0a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49165 Reviewed-by: Matt Mueller <[email protected]> Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> * Update to draft-ietf-tls-esni-13. Later CLs will clean up the ClientHello construction a bit (draft-12 avoids computing ClientHelloOuter twice). I suspect the transcript handling on the client can also be simpler, but I'll see what's convenient after I've changed how ClientHelloOuter is constructed. Changes of note between draft-10 and draft-13: - There is now an ECH confirmation signal in both HRR and SH. We don't actually make much use of this in our client right now, but it resolves a bunch of weird issues around HRR, including edge cases if HRR applies to one ClientHello but not the other. - The confirmation signal no longer depends on key_share and PSK, so we don't have to work around a weird ordering issue. - ech_is_inner is now folded into the main encrypted_client_hello code point. This works better with some stuff around HRR. - Padding is moved from the padding extension, computed with ClientHelloInner, to something we fill in afterwards. This makes it easier to pad up the whole thing to a multiple of 32. I've accordingly updated to the latest recommended padding construction, and updated the GREASE logic to match. - ech_outer_extensions is much easier to process because the order is required to be consistent. We were doing that anyway, and now a simple linear scan works. - ClientHelloOuterAAD now uses an all zero placeholder payload of the same length. This lets us simplify the server code, but, for now, I've kept the client code the same. I'll follow this up with a CL to avoid computing ClientHelloOuter twice. - ClientHelloOuterAAD is allowed to contain a placeholder PSK. I haven't filled that in and will do it in a follow-up CL. Bug: 275 Change-Id: I7464345125c53968b2fe692f9268e392120fc2eb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48912 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Fix calculation of draft-13 ECH confirmation signal. Apparently both we and Go flipped the parameter order for HKDF-Extract relative to the HKDF spec. (The spec orders the salt before the key.) Not sure how that happened. Found doing interop testing with Stephen Farrell's implementation. https://pkg.go.dev/golang.org/x/crypto/hkdf#Extract https://datatracker.ietf.org/doc/html/rfc5869#section-2.2 https://datatracker.ietf.org/doc/html/draft-ietf-tls-esni-13#section-7.2 Bug: 275 Change-Id: I40a7d53b45cb548e93e6a7ae235e98e55dec4a7a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49185 Reviewed-by: Adam Langley <[email protected]> * Revert "Guard use of sdallocx with BORINGSSL_SDALLOCX" This reverts commit 80df7398ce52574801821ce7a76c031c35d6b882. See https://github.com/grpc/grpc/issues/25450#issuecomment-910806034 Even if we want to do this, turns out that we still need the weak symbol in order to work in important environments. Change-Id: I50b9aef0cfe7ed70bda433c3046d46f194636d54 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49205 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]> * Switch to the new, simpler WHATWG URL formulation. In light of https://groups.google.com/a/chromium.org/g/blink-dev/c/7QN5nxjwIfM/m/q9dw9MxoAwAJ, the WHATWG URL parser is now more restrictive about which strings are valid DNS names. The final component may not be numeric. Align the ECHConfig validator with this. Bug: 275 Change-Id: Iea2a3d9a7fee5bffc683da99274c54d60379be9e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49225 Reviewed-by: Adam Langley <[email protected]> * Silence a GCC false positive warning. GCC 11.2.1 reportedly warns that CTR_DRBG_init may be passed an uninitialized personalization buffer. This appears to be a false positive, because personalization_len will be zero. But it's easy enough to zero-initialize it, so silence the warning. Bug: 432 Change-Id: I20f6b74e09f19962e8cae37d45090ff3d1c0215d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49245 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]> * Update comment for ECH draft-13. Bug: 275 Change-Id: I66c0d099f9fe6172c60cbf1f512b90f3b2bbb897 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49285 Reviewed-by: Adam Langley <[email protected]> * Check for __TRUSTY__ instead of TRUSTY. Meant to do this shortly after filing the bug but forgot. Bug: 377 Change-Id: Ic5a5c167a7b6745599e3a32c4792b66ebbb2dee0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49265 Reviewed-by: Adam Langley <[email protected]> * acvptool: add hmacDRBG support Change-Id: I63ecaaaa8ec339688c586a4b2d44e4b91b910b8f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49305 Commit-Queue: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> * Correctly propagate errors in i2d functions. tasn_enc.c was missing lots of error checks and mixed up 0 and -1 returns. Document all the internal calling conventions, as best as I can tell, and fix things up. There are also error cases it forgets to check (it generally does not notice missing non-OPTIONAL fields). This CL only addresses errors it already tries to report. Subsequent CLs will add in the missing error cases. And then if it all sticks, I'm hoping we can rewrite this with CBB. Rewriting tsan_dec.c to CBS would also be good, but that will be more difficult as we need to clear out BER first. Update-Note: Some error cases which were silently misinterpreted as missing OPTIONAL elements will now cause encoding to fail. Bug: 429 Change-Id: Ibbb3eba08eb8f8f878930c9456edc8c74479aade Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49345 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Fix x509_name_ex_i2d error-handling. This function forgot to handle errors in ASN1_item_ex_i2d. It also checked x509_name_canon for ret < 0, when x509_name_canon returns a boolean. For consistency, I've switched to x509_name_encode to return a boolean as well. It doesn't actually need to return a length because it's responsible for filling in a->bytes. (This is also far from thread-safe, but I'll figure out what to do there separately.) Bug: 429 Change-Id: I1dddeab320018be4b837f95001cbeeba4e25f0a1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49346 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Check for invalid CHOICE selectors in i2d functions. This handles normal CHOICE types. A follow-up CL will handle MSTRING and ANY types. Update-Note: An invalid CHOICE object (e.g. GENERAL_NAME) will now fail when encoded, rather than be silently omitted. In particular, CHOICE objects are default-initialized by tasn_new.c in an empty -1 state. Structures containing a required CHOICE field can no longer be encoded without filling in the CHOICE. Bug: 429 Change-Id: I7011deadf518ddc344a56b07a0e268ceaae17fe0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49347 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Correctly handle invalid ASN1_OBJECTs when encoding. asn1_ex_i2c actually does have an error condition, it just wasn't being handled. 628b3c7f2fdf68519c27dc087c400ca616616f4e, imported from upstream's f3f8e72f494b36d05e0d04fe418f92b692fbb261, tried to check for OID-less ASN1_OBJECTs and return an error. But it and the upstream change didn't actually work. -1 in this function means to omit the object, so OpenSSL was silently misinterpreting the input structure. This changes the calling convention for asn1_ex_i2c to support this. It is, unfortunately, a little messy because: 1. One cannot check for object presense without walking the ASN1_ITEM/ASN1_TEMPLATE structures. You can *almost* check if *pval is NULL, but ASN1_BOOLEAN is an int with -1 to indicate an omitted optional. There are also FBOOLEAN/TBOOLEAN types that omit FALSE/TRUE for DEFAULT. Thus, without more invasive changes, asn1_ex_i2c must be able to report an omitted element. 2. While the i2d functions report an omitted element by successfully writing zero bytes, i2c only writes the contents. It thus must distinguish between an omitted element and an element with zero-length contents. 3. i2c_ASN1_INTEGER and i2c_ASN1_BIT_STRING return zero on error rather than -1. Those error paths are not actually reachable because they only check for NULL. In fact, OpenSSL has even unexported them. But I found a few callers. Rather than unwind all this and change the calling convention, I've just made it handle 0 and map to -1 for now. It's all a no-op anyway, and hopefully we can redo all this with CBB later. I've just added an output parameter for now. In writing tests, I also noticed that the hand-written i2d_ASN1_OBJECT and i2d_ASN1_BOOLEAN return the wrong value for errors, so I've fixed that. Update-Note: A default-constructed object with a required ASN1_OBJECT field can no longer be encoded without initializing the ASN1_OBJECT. Note this affects X509: the signature algorithm is an ASN1_OBJECT. Tests that try to serialize an X509_new() must fill in all required fields. (Production code is unlikely to be affected because the output was unparsable anyway, while tests sometimes wouldn't notice.) Bug: 429 Change-Id: I04417f5ad6b994cc5ccca540c8a7714b9b3af33d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49348 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Reject -1 types in ASN1_TYPE and MSTRINGs when encoding. See https://github.com/openssl/openssl/issues/16538 Update-Note: A default-constructed object with a required ANY or string-like CHOICE field cannot be encoded until the field is specified. Note this affects i2d_X509: notBefore and notAfter are string-like CHOICEs in OpenSSL. Bug: 429 Change-Id: I97d971fa588ab72be25a4c1eb7310ed330f16c4f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49349 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]> * Reject missing required fields in i2d functions. See also 006906cddda37e24a66443199444ef4476697477 from OpenSSL, though this CL uses a different strategy from upstream. Upstream makes ASN1_item_ex_i2d continue to allow optionals and checks afterwards at every non-optional call site. This CL pushes down an optional parameter and says functions cannot omit items unless explicitly allowed. I think this is a better default, though it is a larger change. Fields are only optional when they come from an ASN1_TEMPLATE with the OPTIONAL flag. Upstream's strategy misses top-level calls. This CL additionally adds checks for optional ASN1_TEMPLATEs in contexts where it doesn't make sense. Only fields of SEQUENCEs and SETs may be OPTIONAL, but the ASN1_ITEM/ASN1_TEMPLATE split doesn't quite match ASN.1 itself. ASN1_TEMPLATE is additionally responsible for explicit/implicit tagging, and SEQUENCE/SET OF. That means CHOICE arms and the occasional top-level type (ASN1_ITEM_TEMPLATE) use ASN1_TEMPLATE but will get confused if marked optional. As part of this, i2d_FOO(NULL) now returns -1 rather than "successfully" writing 0 bytes. If we want to allow NULL at the top-level, that's not too hard to arrange, but our CBB-based i2d functions do not. Update-Note: Structures with missing mandatory fields can no longer be encoded. Note that, apart from the cases already handled by preceding CLs, tasn_new.c will fill in non-NULL empty objects everywhere. The main downstream impact I've seen of this particular change is in combination with other bugs. Consider a caller that does: GENERAL_NAME *name = GENERAL_NAME_new(); name->type = GEN_DNS; name->d.dNSName = DoSomethingComplicated(...); Suppose DoSomethingComplicated() was actually fallible and returned NULL, but the caller forgot to check. They'd now construct a GENERAL_NAME with a missing field. Previously, this would silently serialize some garbage (omitted field) or empty string. Now we fail to encode, but the true error was the uncaught DoSomethingComplicated() failure. (Which likely was itself a bug.) Bug: 429 Change-Id: I37fe618761be64a619be9fdc8d416f24ecbb8c46 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49350 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Don't read it->funcs without checking it->itype. it->funcs is only an ASN1_AUX for ASN1_ITYPE_SEQUENCE and ASN1_ITYPE_CHOICE. Fortunately, the other possible types for it->funcs are larger than ASN1_AUX and we don't touch the result when we shouldn't, so this is merely a strict aliasing violation. Change-Id: I29e94249e0b137fe8df0b16254366ae6705c8784 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49351 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Remove ASN1_OP_I2D_* callbacks. These are a little odd with the ASN1_ENCODING paths. And there were some bugs previously around CHOICE types. Nothing defines them, inside or outside BoringSSL, so remove them. Change-Id: Id2954fef8ee9637f36f7511b51dc0adc2557e3ba Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49352 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Add maskHash to RSA_PSS_PARAMS for compat This CL adds a maskHash member to the rsa_pss_params_st struct for increased compatibility with OpenSSL: https://source.chromium.org/chromium/chromium/src/+/main:third_party/perl/c/include/openssl/rsa.h;l=282-289 Node.js recently began to make use of this member in https://github.com/nodejs/node/pull/39851 and without this member Electron sees compilation errors. Change-Id: Ibd18a31605b0a715edb279a3bca4b4f05e679767 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49365 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> * Fix BN_prime_checks_for_validation to align with false-positive rate. This doesn't affect RSA key generation, which uses BN_prime_checks_for_generation. Change-Id: Ibf32c0c4bc9fed369e8f8a1efea72c5bd39185a9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49426 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> * Clarify that TLS sessions are not application sessions. Having APIs named "session" and "ID" appears to be far too tempting for developers, mistaking it as some application-level notion of session. Update the documentation, in hopes of discouraging this mistake. Change-Id: Ifd9516287092371d4701114771eff6640df1bcb0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49405 Reviewed-by: Adam Langley <[email protected]> * Fix the TLS fuzzers for ECH draft-13. Replace the hardcoded ECH config, which wasn't updated for draft-13, with a call to SSL_marshal_ech_config. Bug: 275, oss-fuzz:38054 Change-Id: I10c12b22015c9c0cb90dd6185eb375153a2531f4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49445 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Refresh fuzzer corpus for ECH draft-13. Bug: 275 Change-Id: I3a89bd31b6198c9cb2e40835219fa9f248a69c9b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49446 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]> * Add FIPS counters for AES-GCM in EVP_AEAD. BUG=b/158221316 Change-Id: I42693f760aa2852902d72622e109c5d9cac2c4d9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49485 Commit-Queue: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> * Ignore SIGPIPE in the bssl tool. Bug: 435 Change-Id: I0ed94d40d04ebc26c9996dfe2b947a6e2f140a89 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49465 Reviewed-by: Adam Langley <[email protected]> * acvptool: add CS3 support. CS3 is ciphertext-stealing variant three from SP 800-38A. Change-Id: I992dc22778c91efad361f25ff65ae5966fc447c6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49505 Commit-Queue: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> * Unwind remnants of ASN1_TFLG_NDEF. The i2d functions internally take a tag/class pair of parameters. If tag is not -1, we override the tag with (tag, class). Otherwise, class is ignored. (class is inconsistently called aclass or iclass.) Historically, the remaning bits of class were repurposed to pass extra flags down the structure. These had to be preserved in all recursive calls, so the functions take apart and reassemble the two halves of aclass/iclass. The only such flag was ASN1_TFLG_NDEF, which on certain types, caused OpenSSL to encode indefinite-length encoding. We removed this in https://boringssl-review.googlesource.com/c/boringssl/+/43889. Due to these flags, if tag == -1, class should default to zero. However, X509_NAME's callbacks pass -1, -1, instead of -1, 0, effectively setting all flags. This wasn't noticed because none of the types below X509_NAME pay attention to ASN1_TFLG_NDEF. This CL does two things: First, it unwinds the remainder of the flags machinery. If we ever need flags, we should pass it as a distinct argument. Second, it fixes the X509_NAME calls and asserts that -1 is always paired with 0. Change-Id: I285a73a06ad16980617fe23d5ea7f260fc5dbf16 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49385 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Speed up constant-time base64 decoding. I was inspired to look at this again recently and noticed we could do a bit better. Instead of a tower of selects, rely on all the cases being mutually exclusive and use the ret |= mask & value formulation without loss in clarity. We do need to fixup the invalid case slightly, but since that computation is mostly independent, I'm guessing the CPU and compiler are able to schedule it effectively. Before: Did 251000 base64 decode operations in 2002569us (159.4 MB/sec) After: Did 346000 base64 decode operations in 2005426us (219.5 MB/sec) [+37.7%] Change-Id: I542167202fd4e94c93dd5a2519a97bc388072c89 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49525 Reviewed-by: Adam Langley <[email protected]> * Allow PKCS7_sign to work for signing kernel modules. Linux module signing uses PKCS#7 / CMS because everything is awful and broken. In order to make the lives of kernel developers easier, support the calling pattern that the kernel uses to sign modules. The kernel utility was written at a time when PKCS#7 was hard coded to use SHA-1 for signing in OpenSSL and it reflects this: you can only specify “sha1” on the command line, for example. As of OpenSSL 1.1.1, at least, OpenSSL uses SHA-256 and thus so does this change. Change-Id: I32b036123a0d8b272ec9e1c0130c45bf3ed0d2c7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49545 Commit-Queue: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> * aarch64: Add missing LR validation in 'vpaes_cbc_encrypt' There is an obvious bug there: upon entry to 'vpaes_cbc_encrypt' LR may get signed. However, on the 'cbc_abort' path the LR is not going to be unsigned before 'ret' is executed. Found by manual code inspection. Co-authored-by: Russ Butler <[email protected]> Change-Id: I646cdfaee28db59aafbbd412d4bb6ba022eff15b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49605 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> * Keep EVP_CIPHER/EVP_MD lookup and do_all functions in sync Node seems uncommonly sensitive to this, so let's write these functions in a way that stays in sync and test this. See also https://boringssl-review.googlesource.com/c/boringssl/+/49585 This does incur a cost across all BoringSSL consumers that use these functions: as a result of Node indiscriminately exposing every cipher, we end up pulling more and more ciphers into these getters. But that ship sailed long ago, so, instead, document that EVP_get_cipherby* should not be used by size-conscious callers. EVP_get_digestby* probably should have the same warning, but I've left it alone for now because we don't quite have the same proliferation of digests as ciphers. (Though there are things in there, like MD4, that ought to be better disconnected.) Change-Id: I61ca406c146279bd05a52bed6c57200d1619c5da Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49625 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]> * Fix CRYPTO_malloc, etc., definitions. In upstream, these functions take file and line number arguments. Update ours to match. Guessing almost no one uses these, or we'd have caught this earlier. Change-Id: Ic09f8d8274065ac02efa78e70c215b87fa765b9f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49665 Reviewed-by: Adam Langley <[email protected]> Reviewed-by: Benjamin Brittain <[email protected]> Commit-Queue: David Benjamin <[email protected]> * Add log tag for Trusty. Trusty's TLOGE macro nowadays expects TLOG_TAG to be defined as the log tag to use. Change-Id: I18121287ba51698d354323027d5382c8406f0b99 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49685 Commit-Queue: Pete Bentley <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: David Benjamin <[email protected]> * Add note to HMAC test vectors from NIST All the test vectors testing key length greater than the block length were mislabelled as key length being equal to the block length. Add a note to these test vectors indicating they are directly from the NIST tests with the misleading input intact. Change-Id: I9fe87971265ad48e9b835fccbe92306e1670b4d6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49705 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> * Switch x509_test.cc to modify the existing X509_VERIFY_PARAM. There are two ways to configure an X509_STORE_CTX after X509_STORE_CTX_init. One can either modify the already initialized X509_VERIFY_PARAM or replace it. Modifying the existing one is more common. Replacing it actually misses some defaults. (See issue #441 for details.) In preparation for actually being able to test changes to the default, switch tests to that model. In doing so, no longer need to explicitly configure the depth and can test that default. (Though we should write tests for the depth at some point.) Bug: 439, 441 Change-Id: I254a82585d70d44eb94920f604891ebfbff4af4c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49745 Commit-Queue: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]> Reviewed-by: Adam Langley <[email protected]> * Enable X509_V_FLAG_TRUSTED_FIRST by default. The OpenSSL X.509 verifier lacks a proper path builder. When there are two paths available for a certificate, we pick one without looking at expiry, etc. In scenarios like one below, X509_V_FLAG_TRUSTED_FIRST will prefer Leaf -> Intermediate -> Root1. Otherwise, we will prefer Leaf -> Intermediate -> Root1Cross -> Root2: Root2 | Root1 Root1Cross \ / Intermediate | Leaf If Root2 is expired, as with Let's Encrypt, X509_V_FLAG_TRUSTED_FIRST will find the path we want. Same if Root1Cross is expired. (Meanwhile, if Root1 is expired, TRUSTED_FIRST will break and leaving it off works. TRUSTED_FIRST does not actually select chains with validity in mind. It just changes the semi-arbitrary decision.) OpenSSL 1.1.x now defaults to X509_V_FLAG_TRUSTED_FIRST by default, so match them. Hopefully the shorter chain is more likely to be correct. Update-Note: X509_verify_cert will now build slightly different chains by default. Hopefully, this fixes more issues than it causes, but there is a risk of trusted_first breaking other scenarios. Those scenarios will also break OpenSSL 1.1.x defaults, so hopefully this is fine. Bug: 439 Change-Id: Ie624f1f7e85a9e8c283f1caf24729aef9206ea16 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49746 Reviewed-by: Adam Langley <[email protected]> Reviewed-by: Ryan Sleevi <[email protected]> * Remove X509_STORE_set0_additional_untrusted. This was added in https://boringssl-review.googlesource.com/c/boringssl/+/12980/, but does not appear to be used anymore. The corresponding function does not exist in OpenSSL. This simplifies the tests slightly, some of which were inadvertently specifying the boolean and some weren't. Change-Id: I9b956dcd9f7151910f93f377d207c88273bd9ccf Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49747 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]> * Extract common rotl/rotr functions. We have a ton of per-file rotation functions, often with generic names that do not tell you whether they are uint32_t vs uint…
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
OpenSSL has added
RSA_get0_pss_params(temporally after openssl/openssl#10217 (comment)), which allows adding RSA-PSS params to the existingasymmetricKeyDetailsproperty.Fixes: #39837
Refs: openssl/openssl#10568