Merge BoringSSL through a8b1633d1c6be133b9f684cc5cdd778bfd8d564e. #1553
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
(Tweaking pending CLs now upsets the tooling so doing this as a follow-up CL instead.) Change-Id: I78efc5cb72de7b41d57769bb3958c20167de69b3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52325 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
We no longer use a weak hash for certificate comparisons. There is no need to do extra work when certificates are the same. Change-Id: I3b4b295122b289ae389bce2245b8348562700855 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52346 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
CMake versions newer than ~3.1x automatically determine the subdirectory under CMAKE_INSTALL_PREFIX using the type of the installed target. Older versions need this to be manually computed using the GNUInstallDirs library. Since we override the CMAKE_INSTALL_PREFIX default, this just controls the internal layout of the install/ directory generated underneath the boringssl checkout. Bug: 488 Change-Id: I97b02006301e463bb0cfd54acb2b27656484cc85 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52345 Reviewed-by: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Inspired by Joel Sing's work in libre. Change-Id: I17267af926b7d42472f7dae3205fda9aabdfa73d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52385 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Between the lookup table, the multiple layers of reuse of the "type" variable, it is a little hard to follow what's going on with ASN1_STRING_print_ex. Replace the lookup table with a switch-case (implicitly handles the bounds check, and we can let the compiler figure out the best spelling). Then, rather than returning a "character width", which doen't represent UTF-8, just use the already-defined MBSTRING_* constants. (These changes should be covered by the existing ASN1Test.StringPrintEx test.) Change-Id: Ie3b2557bfae0f65db969e90cd0c76bc8ade963d4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52365 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
I broke the delocate tests with 27ffcc6 because that change switched the integrity check hash function in the tested configuration to SHA-256, but didn't update the expectation files. Change-Id: I05f61eda795c833847981c5b21287fd0d2b33064 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52405 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: David Benjamin <[email protected]>
Change-Id: I7d95d53d4d99cb5b58fc05ee8240577575306b94 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52406 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: David Benjamin <[email protected]>
Change-Id: I3eab2393d4fe48c900d67240c7decf223d78c2f1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52425 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]>
3DES is no longer acceptable for FIPS validation. Change-Id: Id4f80f6201220eff28cb3597540a3b796bfcf54f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52426 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
PSS verification with RSA 1024 is still acceptable. This change adds it to our regcap. SHA-512 is omitted because that doesn't fit in 1024 bits with PSS encoding. (Unless you shrink the salt, but we don't want to deal with odd PSS configurations like that.) Change-Id: I05d4778d45c25c87e2e50cd095380ecd4f12c75c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52427 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]>
The module supports these operations, they should probably be tested with ACVP. Change-Id: I09a52281f0c6877862139b73a876c995aa9af92a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52428 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]>
I'm collecting these changes, made by `go fmt` into their own change so that they don't add noise to a meaningful change in the future. Change-Id: I0ca931816fdba60ecde43e3eaffd0339d1773e73 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52429 Reviewed-by: Bob Beck <[email protected]>
Change-Id: I45b5b4c3c1ba8f591bc4b9cb0bec17b800b75eaa Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52430 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
For now, the tests assert the existing behavior of X509_NAME_print, but there are several bugs in it. Change-Id: I9bc211a880ea48f7f756650dbe1f982bc1ec689d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52366 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
CPython and wpa_supplicant are using this nowadays. To avoid needing to tweak the ticket nonce derivation, I've just internally capped the number of tickets at 16, which should be plenty. Change-Id: Ie84c15b81a2abe8ec729992e515e0bd4cc351037 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52465 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
When introducing EC_SCALAR and EC_FELEM, I used unions as convenience for converting to and from the byte representation. However, type-punning with unions is not allowed in C++ and hard to use correctly in C. As I understand the rules, they are: - The abstract machine knows what member of union was last written to. - In C, reading from an inactive member is defined to type-pun. In C++, it is UB though some compilers promise the C behavior anyway. - However, if you read or write from a *pointer* to a union member, the strict aliasing rule applies. (A function passed two pointers of different types otherwise needs to pessimally assume they came from the same union.) That last rule means the type-punning allowance doesn't apply if you take a pointer to an inactive member, and it's common to abstract otherwise direct accesses of members via pointers. openssl/openssl#18225 is an example where similar union tricks have caused problems for OpenSSL. While we don't have that code, EC_SCALAR and EC_FELEM play similar tricks. We do get a second lifeline because our alternate view is a uint8_t, which we require to be unsigned char. Strict aliasing always allows the pointer type to be a character type, so pointer-indirected accesses of EC_SCALAR.bytes aren't necessarily UB. But if we ever write to EC_SCALAR.bytes directly (and we do), we'll switch the active arm and then pointers to EC_SCALAR.words become strict aliasing violations! This is all far too complicated to deal with. Ideally everyone would build with -fno-strict-aliasing because no real C code actually follows these rules. But we don't always control our downstream consumers' CFLAGS, so let's just avoid the union. This also avoids a pitfall if we ever move libcrypto to C++. For p224-64.c, I just converted the representations directly, which avoids worrying about the top 32 bits in p224_felem_to_generic. Most of the rest was words vs. bytes conversions and boils down to a cast (we're still dealing with a character type, at the end of the day). But I took the opportunity to extract some more "words"-based helper functions out of BIGNUM, so the casts would only be in one place. That too saves us from the top bits problem in the bytes-to-words direction. Bug: 301 Change-Id: I3285a86441daaf824a4f6862e825d463a669efdb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52505 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Change-Id: I4bab5fe860e7bf2c2bdf11552d60a2e10c55d227 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52565 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: David Benjamin <[email protected]>
Change-Id: Ib0dd73fc4b5b50d542a9b937065048ed9b0a85fd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52566 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
The manual construction of the version integer is odd. The default is already zero, and as of https://boringssl-review.googlesource.com/c/boringssl/+/51632, we've settled on the empty string as the ASN1_INTEGER representation of zero. But there don't seem to be any uses of this function, so just remove it. Update-Note: Removed seemingly unused public API. Change-Id: I75f8bcdadb8ffefb0b2da0fcb0a87a8cb6398f70 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52585 Reviewed-by: Adam Langley <[email protected]>
This aligns X509_REQ's and X509_CRL's parsers to the changes already made with X509; we reject invalid versions and check that extensions are only with the corresponding version. For now, we still allow X509v1 CRLs with an explicit version, matching certificates. (The DEFAULT question is moot for X509_REQ because CSRs always encode their version, see RFC 2986.) In addition to rejecting garbage, this allows for a more efficient representation once we stop using the table-based parser: X509 and X509_CRL can just store a small enum. X509_REQ doesn't need to store anything because the single version is information-less. Update-Note: Invalid CRL and CSR versions will no longer be accepted. X509_set_version, etc., no longer allow invalid versions. Fixed: 467 Change-Id: I33f3aec747d8060ab80e0cbb8ddf97672e07642c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52605 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
These definitions are to get access to getaddrinfo() and gmtime_r() when using glibc. This in turn conflicts with other places (which would have these things in their libc anyway) where using these feature flags turns off C11 functionality we would like to use. Bug:490 Change-Id: I66fdb7292cda788df19508d99e7303ed0d4f4bdd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52545 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: David Benjamin <[email protected]>
Bug: chromium:1205638 Change-Id: Iee6d4cbdff513f248e747fc497c4bda3efb995e5 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52645 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
https://boringssl-review.googlesource.com/c/boringssl/+/51635 switched the serial number check to ASN1_INTEGER_get_uint64, but as that function actually treats overflow as an error, it adds to the error queue and we need to clear it. See also b/231880827, though whether that is a red herring or the cause, I'm not sure. Change-Id: Ibd7e9369c3455898fa3411b7a079ce21b37c586c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52648 Commit-Queue: Adam Langley <[email protected]> Reviewed-by: Adam Langley <[email protected]>
I'm leaving libc++abi unupdated for now, because it depends on a newer libc++ than Chromium has managed to update to just yet. (It looks like Chromium is, for now, manually passing -D_LIBCPP_CONSTINIT=constinit.) Probably easier to pick that up when the dust settles. (When that happens, libc++abi will need to be built as C++20, but that's fine because this is only used on bots.) Change-Id: I2b7ae62b618b0e09863b4b38548be1b937fbe0d0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52646 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
This seems to be the preferred mechanism starting CMake 3.1, and reduces some compiler-specific bits. We can probably also retire -stdlib=libc++ on AppleClang now. https://boringssl-review.googlesource.com/21984 suggests it was needed to make C++11 work, but presumably that's the default stdlib now that the default C++ version is C++11. (If it is needed, that ought to be CMAKE_CXX_STANDARD's responsibility. I don't see any evidence of them setting it, so I'm guessing this is just no longer needed.) Change-Id: Ide1fdbd3d4b8fe37a10a7948171a2c20c9de21f9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52647 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Change-Id: I2cffb9e870785a1c49d3ae872387494632bfb8fe Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52567 Reviewed-by: David Benjamin <[email protected]>
This is cribbed, with perimssion, from AWS-LC. The FIPS service indicator[1] signals when an approved service has been completed. [1] FIPS 140-3 IG 2.4.C Change-Id: Ib40210d69b3823f4d2a500b23a1606f8d6942f81 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52568 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
We have no EVP_MDs with type NID_ecdsa_with_SHA1 (that's a remnant of the old signature algorithm EVP_MDs). Also there's no sense in calling EVP_MD_type or performing the cast five times. Change-Id: I7ea60d80059420b01341accbadf9854b4c3fd1b8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52685 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
This has no callers, and seems to be practically unusable. The only way to set an X509_CRL_METHOD is X509_CRL_set_default_method, which is not thread-safe and globally affects the CRL implementation across the application. The comment says it's to handle large CRLs, so lots of processes don't have to store the same CRL in memory. As far as I can tell, X509_CRL_METHOD cannot be used to help with this. It doesn't swap out storage of the CRL, just signature verification and lookup into it. But by the time we call into X509_CRL_METHOD, the CRL has already been downloaded and the data stored on the X509_CRL structure. (Perhaps this made more sense before the structure was made opaque?) Update-Note: APIs relating to X509_CRL_METHOD are removed. Change-Id: Ia5befa2a0e4f4416c2fb2febecad99fa31c1c6ac Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52687 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
While C allows function pointer casts, it is UB to call a function with a different type than its actual type signature. That is, even though `void f(int *)` and `void g(void *)` have the same ABI, it is UB to cast `f` to a `void(*)(void *)` and then call it through that pointer. Clang CFI will try to enforce this rule. The recent CL to call X509_print in tests revealed that all the i2? and ?2i callbacks in X509V3_EXT_METHODs were implemented with functions of the wrong type, out of some combination of missing consts and void* turned into T*. This CL fixes this. Where the function wasn't exported, or had no callers, I just fixed the function itself. Where it had extension callers, I added a wrapper function with a void* type. I'm not positive whether the wrappers are the right call. On the one hand, keeping the exported functions as-is is more type-safe and more OpenSSL-compatible. However, most (but not all) uses of these are in other code defining X509V3_EXT_METHODs themselves, so the void* signature is more correct for them too. And the functions have a type signature meant for X509V3_EXT_METHOD, complete with method pointer. I've gone with leaving the exported ones as-is for now. Probably the right answer anyway is to migrate the external callers, of either type signature. Change-Id: Ib8f2995cbd890221eaa9ac864a7e553cb6711901 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52686 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]>
If we're to have any hope of fixing EVP_CIPHER_CTX's calling convention, we need to be able to change the shape of its method table. Looking back, it looks like we exported this in https://boringssl-review.googlesource.com/4330, for OpenSSH. I don't remember exactly what OpenSSH was doing, but I see in this commit, they removed a bunch of custom EVP_CIPHERs which would definitely have required an exported EVP_CIPHER struct: openssh/openssh-portable@cdccebd That's been gone for a while now, so hopefully we can hide it again. (If a project needs a cipher not implemented by OpenSSL, it's not strictly necessarily to make a custom EVP_CIPHER. It might be convenient to reuse the abstraction, but you can always just call your own APIs directly.) Update-Note: EVP_CIPHER is now opaque. Use accessors instead. Bug: 494 Change-Id: I9344690c3cfe7d19d6ca12fb66484ced57dbe869 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52725 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
Historically, the hwaes-based EVP_CIPHERs went through a different EVP_CIPHER instance than all the others. As of https://boringssl-review.googlesource.com/c/boringssl/+/28484/, this is no longer necessary so we can avoid the _generic indirection. Change-Id: I40972ff8851ecf1a417af991b22c0674a2f34464 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52726 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
For AEADs (our only EVP_CIPH_FLAG_CUSTOM_CIPHER is GCM), EVP_Cipher is not a one-shot operation. It is a thin wrapper over the internal cipher callback in the EVP_CIPHER, complete with treating in == NULL as EVP_CipherFinal_ex. Also document that you should not do this. Also document how you feed in the AAD for an AEAD EVP_CIPHER. (Although callers really should use EVP_AEAD for a much less complex interface.) Bug: 494 Change-Id: I0beb1c88cdf0406506af2772e53e9d3f8d07172a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52727 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Change-Id: Ic6a72a9dd756b1b3d6ed13c6b57ecf611638ac46 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52026 Reviewed-by: Adam Langley <[email protected]> Reviewed-by: Loola TV <[email protected]> Commit-Queue: Bob Beck <[email protected]>
Adding the ending macros to StatementMacros tells clang-format the macro terminates a statement. Adding trailing commas in the middle keeps it from trying to bundle the curly brace with the next statement. Also add a few other trailing commas that clang-format otherwise indents awkwardly. Change-Id: I0b2ba9cf07bc775649fa1e92de3e5bb2e2b0b20b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52728 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
The original implementation uses a table generated by a Perl script, and then relies on some subset of ASN1_STRFLGS_* constants overlapping with CHARTYPE_* constants, while masking off the ones that don't align. Allocating ASN1_STRFLGS_* constants is already complex with the XN_FLAG_* interaction. Avoid the additional CHARTYPE_* interaction by just writing out what it's recognizing in code. If you ignore CHARTYPE_PRINTABLESTRING (which is unused), that table is just recognizing 9 characters anyway. Also this gets charmap.h out of the way so I can clang-format every file in here without having to constantly exclude it. Change-Id: I73f31324e4b8a815887afba459e50ed091a9f999 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52729 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
The OpenSSL style writes multiline block comments with a blank line at the top and bottom, like so: /* * Some multi-line * comment */ The script already removed the trailing blank line, but not the leading one. When we go to run this script in crypto/asn1, etc., we'll come across those comments. Change-Id: I189aec87a08607008779f883a97f2c53d24ee2da Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52730 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
clang-format gets very confused by the comments in these tables. (The comments seem to have already gotten a little messed up from upstream's reformatted.) Reformat them ahead of time. I removed the tag2str number comments as they aren't really doing much good at this point. Also remove the last entry in tag2bits because it's not actually used. ASN1_tag2bit only reads the first 31 entries. Change-Id: If50770fd79b9d6ccab5558d24b0ee3a27c81a452 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52731 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
For now, it contains a call to set the service indicator so it should live in the module. In term it would be nice to move it back out and have the service indicator set in RSA and ECDSA functions themselves once the ECDSA functions can take an indicator of the hash function used. Change-Id: I2a3c262f66b1881a96ae3e49784a0dc9fc8c4589 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52705 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Change-Id: I1e4658b6a9d57e407d04c71065d51ad8caf1d5cb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52732 Reviewed-by: Adam Langley <[email protected]> Reviewed-by: Bob Beck <[email protected]>
ECDSA_verify does a runtime check that our parser round-trips, but that should already be true. Add a fuzzer to ensure it. Change-Id: I396863b8f9ed66c6296cfb16f7197a63ae99e156 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52733 Reviewed-by: Adam Langley <[email protected]>
Best to exercise the full range of lengths, where supported. This does makes the downloads larger but the ACVP demo server seems to have mostly solved its issue of hanging when serving large files. None the less, a longer timeout is needed. Change-Id: I1b4879ded8a03b40512b4e03e2b2ce59678974f7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52745 Reviewed-by: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]>
These functions aid in meeting specific compliance goals and allows configuration of things like TLS 1.3 cipher suites, which are otherwise not configurable. Change-Id: I668afc734a19ecd4b996eaa23be73ce259b13fa2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52625 Commit-Queue: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]>
Using # has slightly ambiguous with the preprocessor. Fixed: 496 Change-Id: Ib718dde957ee541d580096128405a7edde3b1bd4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52785 Reviewed-by: Adam Langley <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1553 +/- ##
==========================================
- Coverage 92.26% 92.25% -0.01%
==========================================
Files 126 127 +1
Lines 18850 18850
Branches 196 196
==========================================
- Hits 17392 17391 -1
- Misses 1422 1423 +1
Partials 36 36
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
No description provided.