Merge BoringSSL through 538b2a6cf0497cf8bb61ae726a484a3d7a34e54e #2226
+13
−16
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
These return 32-bit hashes, so they should return a platform-independent uint32_t. I've categorized X509_issuer_name_hash and friends under "convenience" functions. X509_NAME_hash and X509_NAME_hash_old are as yet unclassified. Since the hash function is only relevant to X509_LOOKUP_hash_dir, I'm thinking I'll put them with that, once that's organized. While I'm here, simplify the implementations of these functions. The hash operation itself can be made infallible and allocation-free easily. However the function itself is still fallible (and non-const, and not thread-safe) due to the cached encoding mess. X509Test.NameHash captures existing hash values, so we'd notice if this changed the output. Update-Note: This is source-compatible for C/C++, including with -Wconversion, but some bindings need a patch in cl/588632028 to be compatible. Bug: 426 Change-Id: I9bfd3f1093ab15c44d8cb2d81d53aeb3d6e49fc9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64647 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
It's 2023. We shouldn't need to be counting offsets from PC anymore. Instead, let the assembler figure this out with an ADR instruction. Additionally, since it's easy, in chacha-armv4.pl, avoid depending on the exact offset between code and data. We still depend on the code and data being close enough to fit within ADR's (very tight) bounds however. (E.g. an ADR of K256 inside sha256_block_data_order_armv8 would not work because K256 is too far away.) I have not removed the offset dependency in the SHA-2 files yet as they're a bit thorny and .Lsha256_block_data_order-K256 does not seem to work on Apple's 32-bit Arm assembler. (We probably should drop 32-bit Arm assembly on Apple platforms. It doesn't really exist anymore.) Once the armcap references are gone, that will be more straightforward. Update-Note: If 32-bit Arm assembly no longer builds, let us know and tell us what your toolchain is. Change-Id: Ie191781fed98d53c3b986b2f535132b970d79f98 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64747 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
The last iOS version that supported 32-bit was iOS 10, which I don't believe any of our consumers support anymore. (Chromium does not, neither does google/oss-policies-info.) Our iOS CI coverage comes from Chromium, so this means we don't actually have any test coverage for 32-bit iOS, only compile coverage. In addition to lacking any test coverage, 32-bit Arm assembly is more platform-dependent than one might expect, between different limitiations on patterns for PC-relative loads and lots of assembler quirks around what kinds of label expressions (which show up in PC-relative loads a lot) are allowed. Finally, since iOS in that era did not do runtime detection of features and relied on compiling a binary multiple times, the 32-bit assembly would never enable AES acceleration anyway, so it's not as impactful as on other platforms. Between all that, it's no longer worth enabling this. Disable it in target.h which, with the all the recent build simplifications, should be sufficient to disable this code. Update-Note: iOS on 32-bit Arm now disables assembly. This is unlikely to impact anyone. As far as I can tell, 32-bit Arm for iOS thoroughly does not exist anymore. Change-Id: If31208b42047377ad1b4fb0af6fee17334f18330 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64748 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
sha256_block_data_order_hw required a bit of wrestling with Arm immediate limits. PC-relative addressing in 32-bit Arm is a huge mess. I think I could have avoided the extra load with a lot of effort (convincing the assembler to evaluate a messy expression), but this is simpler and there was no measurable performance difference. Change-Id: I3fab4abc0fa24e0d689581e2c9b9faaa32bd7442 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64749 Commit-Queue: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
One of the WARNINGs in this function is unambiguously a bug. Just fix it. In doing so, add a helper for the ASN1_STRING -> ASN1_TYPE conversion and make this function easier to follow. Also document the attrtype == 0 case. I missed that one when enumerating this overcomplicated calling convention. Move that check earlier so we don't try to process the input. It's ignored anyway. Change-Id: Ia6e2dcb7c69488b6e2e58a68c3b701040ed3d4ef Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64827 Reviewed-by: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
OpenSSL 1.1.0 included d9b8b89bec4480de3a10bdaf9425db371c19145b, which a cleanup change to X509_verify_cert. This cleanup changed the semanitcs of the depth limit. Previously, the depth limit omitted the leaf but included the trust anchor. Now it omits both. We forked a little before 1.0.2, so we still had the old behavior. Now that the new behavior is well-established, switch to new one. Bump BORINGSSL_API_VERSION so callers can detect one or the other as needed. Document the new semantics. Also fix up some older docs where I implied -1 was unlimited depth. Negative numbers were actually enforced as negative numbers (which means only explicitly-trusted self-signed certs worked). Update-Note: The new semantics increase the limit by 1 compared to the old ones. Thus this change should only accept more chains than previously and be relatively safe. It also makes us more OpenSSL-compatible. Envoy will need a tweak because they unit test the boundary condition for the depth limit. Bug: 426 Fixed: 459 Change-Id: Ifaa108b8135ea3d875f2ac1f2a3b2cd8a22aa323 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64707 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
With `-mcmodel=medium`, Clang may emit loads of the GOT address. Delocate needs to handle that in order to avoid a relocation inside the module. Bug: b:316210772 Change-Id: I55eb2711860e43f68de131e28538aef9f5ce1889 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64791 Auto-Submit: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
We sometimes have decl-less comments interspersed in headers. Avoid the empty patch of blue when they get rendered. Change-Id: I6668946d07349febd6d28bbe7d4fd8d2426adca9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64927 Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
We don't want anyone using it, but we probably should render all our documented headers. I've grouped it with the rest of the legacy X.509 stack. Change-Id: If01dfd19c71598f47a40cd334b0fd7ef3e00685d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64928 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
https://boringssl-review.googlesource.com/c/boringssl/+/63946 removed it, but sadly wpa_supplicant depends on it. Change-Id: Ib3aca5269d740457ba83ca529b797bfb4a089763 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64907 Reviewed-by: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Bob Beck <[email protected]>
Change-Id: I4cb50bda726fcc0f22fcb53dca92cf297aeea169 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64929 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
The implementation does use SSSE3 (palignr, pshufb). Change-Id: I86a44c1dc505b27b87fdc3cc7f6e8115abb3dcc9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64567 Reviewed-by: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
Our documentation comments already include examples of code blocks and lists, they just don't get rendered right. We also have things that were trying to be lists but aren't. Go ahead and add support for it, and fix the handful of list-like things that didn't get rendered as lists. I took inspiration from CommonMark (https://spec.commonmark.org/0.30/) to resolve questions such as whether blank lines are needed between lists, etc., but this does not support any kind of nesting and is still far from a CommonMark parser. Aligning with CommonMark leaves the door open to pulling in a real Markdown parser if we start to need too many features. I've also borrowed the "block" terminology from CommonMark. One ambiguity of note: whether lists may interrupt paragraphs (i.e. without a blank line in between) is a little thorny. If we say no, this doesn't work: Callers should heed the following warnings: 1) Don't use the function 2) Seriously, don't use this function 3) This function is a bad idea But if we say yes, this renders wrong: This function parses an X.509 certificate (see RFC 5280) into an X509 object. We have examples of both in existing comments, though we could easily add a blank line in the former or rewrap the latter. CommonMark has a discussion on this in https://spec.commonmark.org/0.30/#lists CommonMark says yes, but with a hack that only lists starting with 1 can interrupt paragraphs. Since we're unlikely to cite RFC 1, I've matched for now, but we may want to revisit this if it gets to be a pain. I could imagine this becoming a problem: This function, on success, does some stuff and returns 1. Otherwise, it returns 0. But that looks a little weird and we usually spell out "one" and "zero". I printed all the lists we detected in existing comments, and this has not happened so far. I've also required fewer spaces than CommonMark to trigger a code block. CommonMark uses four, but four spaces plus a leading "//" and a " " is quite a lot. For now I'm not stripping the spaces after the comment marker at comment extraction time and then requiring three spaces, so two spaces relative to normal text. This is mostly to match what we've currently been doing, but we can always change it and our comments later. Change-Id: Ic61a8e93491ed96aba755aec2a5f32914bdc42ae Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64930 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
I'm not sure if this is necessary. I was playing around and this didn't look terrible. Though it will probably turn x509.h into a sea of yellow when it's ready to be rendered. Change-Id: I34b26aad8a779a3fde761558d15b64c79159892a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64931 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
wpa_supplicant also depends on that one. Change-Id: I6abb505d076eb258e6b8d68be42e624f4aedc725 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64947 Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Upstream reverted it in commit af3c895 and relanded a better version later.
…256 implementation. This was done in *ring* commit 303ca7e.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2226 +/- ##
==========================================
- Coverage 97.01% 97.00% -0.01%
==========================================
Files 165 165
Lines 20500 20500
Branches 463 463
==========================================
- Hits 19889 19887 -2
Misses 504 504
- Partials 107 109 +2 ☔ View full report in Codecov by Sentry. |
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.