Upstream merge 2024 12 13 #2060
Merged
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
samuel40791765
force-pushed
the
upstream-merge-2024-12-13
branch
from
8f34522 to
df806b6
Compare
skmcgrail
previously approved these changes
Dec 17, 2024
This field makes no sense for static const structures. It was added early on but never used as far as I can tell. Change-Id: Ie0272c5f498ad777cb3b114589248d8b403ae457 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67047 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: Adam Langley <agl@google.com> Reviewed-by: Adam Langley <agl@google.com> (cherry picked from commit f57a11ae566ac17c1b028d79950227a33ae32fad)
As documented, the symbol prefixing mechanism is experimental and unsupported. There are several corners where we know it doesn't give the correct output. Nonetheless, this is an easy one to fix. Fixed: 707 Change-Id: I69a3e61a3198a193cb90f822218f1efbaa31fb1a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67067 Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Adam Langley <agl@google.com> (cherry picked from commit 440c51317bcbc15aec372bc78cf6fbf59d7eb435)
tb noticed that our X509_ALGOR_set_md differs from OpenSSL because we never set EVP_MD_FLAG_DIGALGID_ABSENT. That is, we include an explicit NULL parameter, while OpenSSL omits it. RFC 4055, section 2.1 says: There are two possible encodings for the AlgorithmIdentifier parameters field associated with these object identifiers. The two alternatives arise from the loss of the OPTIONAL associated with the algorithm identifier parameters when the 1988 syntax for AlgorithmIdentifier was translated into the 1997 syntax. Later the OPTIONAL was recovered via a defect report, but by then many people thought that algorithm parameters were mandatory. Because of this history some implementations encode parameters as a NULL element while others omit them entirely. The correct encoding is to omit the parameters field; however, when RSASSA-PSS and RSAES-OAEP were defined, it was done using the NULL parameters rather than absent parameters. ... To be clear, the following algorithm identifiers are used when a NULL parameter MUST be present: ... My read of this text is: 1. The correct encoding of, say, SHA-256 as an AlgorithmIdentifer *was* to omit the parameter. So if you're using it in, I dunno, CMS, you should omit it. 2. Due to a mishap, RSASSA-PSS originally said otherwise and included it. Additionally, there are some implementations that only work if you include it. 3. Once the mistake was discovered, PSS chose to preserve the mistake, rather than undo it. This means that the correct encoding of SHA-256 as an AlgorithmIdentifer is *different* depending on whether you're doing PSS or CMS. Fortunately, there are only two users of this function, one inside the library and one in Android. Both are trying to encode PSS, so the current behavior is correct. Nonetheless, we should document this. Also, because this is a huge mess, we should also add an API for specifically encoding RSA-PSS. From there, we can update Android to call that function and remove X509_ALGOR_set_md. Amusingly, RSASSA-PKCS1-v1_5 *also* differs from the "correct" encoding. RFC 8017, Appendix B.1 says: The parameters field associated with id-sha1, id-sha224, id-sha256, id-sha384, id-sha512, id-sha512/224, and id-sha512/256 should generally be omitted, but if present, it shall have a value of type NULL. This is to align with the definitions originally promulgated by NIST. For the SHA algorithms, implementations MUST accept AlgorithmIdentifier values both without parameters and with NULL parameters. Exception: When formatting the DigestInfoValue in EMSA-PKCS1-v1_5 (see Section 9.2), the parameters field associated with id-sha1, id-sha224, id-sha256, id-sha384, id-sha512, id-sha512/224, and id-sha512/256 shall have a value of type NULL. This is to maintain compatibility with existing implementations and with the numeric information values already published for EMSA-PKCS1-v1_5, which are also reflected in IEEE 1363a [IEEE1363A]. Finally, there's EVP_marshal_digest_algorithm, used in PKCS#8 and OCSP. I suspect we're doing that one wrong. I've left a TODO there to dig into that one. Bug: 710 Change-Id: I46b11f8c56442a9badd186c7f04bb366147ed98f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67088 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: Bob Beck <bbe@google.com> (cherry picked from commit ec45e104a608ba556be73a0776cfb495c6c8ae44)
We say "all tests passed" but we actually mean only the unit tests. Now the output is: Running Go tests ok boringssl.googlesource.com/boringssl/ssl/test/runner/hpke (cached) ok boringssl.googlesource.com/boringssl/util/ar (cached) ok boringssl.googlesource.com/boringssl/util/fipstools/acvp/acvptool/testmodulewrapper (cached) ok boringssl.googlesource.com/boringssl/util/fipstools/delocate (cached) Running unit tests ssl_test [shard 1/10] ... pki_test [shard 8/10] All unit tests passed! Running SSL tests 0/0/5481/5481/5481 PASS ok boringssl.googlesource.com/boringssl/ssl/test/runner 21.110s all_tests.go really should be called unit_tests.go, but renaming it will probably be too annoying. Change-Id: I7ff6684221930e19152ab3400419f4e5209aaf46 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67107 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: Bob Beck <bbe@google.com> (cherry picked from commit c38abd038c6d6f6ebbe200090821e23313c4bd9c)
These are effectively just APIs for creating Ed25519 and X25519 keys. We may want to rethink this a bit later, but for now let's just do this. Bug: 497 Change-Id: I01ae06fa86af96da993fd41611472838475bf094 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67128 Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 660973695bd20a22201e979a6e6f8c335f939cfe)
This function exists because callers sometimes write EVP_PKEY_type(EVP_PKEY_id(pkey)), which is equivalent to EVP_PKEY_base_id(pkey). In OpenSSL, all this existed so that a type parsed as EVP_PKEY_RSA2 could still be mapped to EVP_PKEY_RSA. We haven't supported this since 2015, so this purely exists as a way to check that the key type exists. In doing so, it currently pulls in the full implementation of every key type. I could replicate the list of keys, but that is one more place we have to keep things up-to-date. Instead, just make this function the identity. Looking through callers, it did not appear anyone depended on the error condition. Update-Note: EVP_PKEY_type used to return NID_undef when given a garbage key type. Given it is only ever used in concert with EVP_PKEY_id, this is unlikely to impact anyone. If it does, we can do the more tedious option. Bug: 497 Change-Id: Ibf68a07ef6906398df0fec425c869c107b8c90f4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67109 Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 8ede9514dac7cace2084d95502d4bd8ea39b08b6)
The RSA, etc., APIs have some discussion on threading expectations. We should have the same text on DH and DSA. While I'm here, const-correct DSA_SIG in some legacy DSA APIs. Change-Id: I6ad43c9347c320dc0b1c8e73850fa07c41e028ea Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67247 Reviewed-by: Theo Buehler <theorbuehler@gmail.com> Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: Bob Beck <bbe@google.com> (cherry picked from commit c5e9b4be0f2fabaac68961c0edce381703731d03)
This seems to be unnecessary. Change-Id: I0439739543d6593aadc87fc97e4ad5870616730e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67268 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit 31442d490cc487998e0fb351e854a9ff9b3ac35e)
This API allocates internally and can leave a corrupted |alg| behind. Change it to return an int so that callers can check for an error. Also fix its only caller in rsa_md_to_algor(). This is an ABI change but will not break any callers. Also add a small regress test for this API. Change-Id: I7a5d1729dcd4c7726c3d4ead3740d478231f3611 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67187 Commit-Queue: Theo Buehler <theorbuehler@gmail.com> Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> Reviewed-by: David Benjamin <davidben@google.com> (cherry picked from commit c99364a313795b2baaa40bd0683a05ae2e1cd993)
Mostly bits of DSA and RSA keygen, flagged when we make the PRNG output secret by default. There's still a ton of RSA to resolve, mostly because our constant-time bignum strategy does not interact well with valgrind when handling RSA's secret-value / public-bit-length situation. Also RSA's ASN.1 serialization is unavoidably leaky. Bug: 676 Change-Id: I08d273959065c4db6fd44180a6ac56a82f862fe8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65447 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: David Benjamin <davidben@google.com> (cherry picked from commit fce5cf02378a839174935b83b58f54aba6c2bb3e)
samuel40791765
force-pushed
the
upstream-merge-2024-12-13
branch
from
df806b6 to
dc1cc97
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2060 +/- ##
==========================================
+ Coverage 78.77% 78.78% +0.01%
==========================================
Files 598 598
Lines 103694 103735 +41
Branches 14735 14734 -1
==========================================
+ Hits 81681 81725 +44
+ Misses 21360 21358 -2
+ Partials 653 652 -1 ☔ View full report in Codecov by Sentry. |
skmcgrail
approved these changes
Dec 19, 2024
torben-hansen
approved these changes
Dec 19, 2024
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.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.