New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PKCS7 certificates list is an implicit SET OF not SEQU… #13143
Conversation
This would be elligible for CLA: trivial. If you agree please amend the commit message so that it contains CLA: trivial on a separate line. |
f30beb6
to
2d40226
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. It looks like this should be backported to 1.1.1. I agree this is trivial.
Apparently we have a test that expects the incorrect output. That needs to be fixed. Does this change have potential of breaking some interoperability if some other software expects the other output? |
Re breaking software expecting the other output, the only visible difference visible would be that certificates weren't written in the order they were added to the pkcs7 object. The only thing one could imagine is that someone writes an application to read pkcs7 written by openssl, that is expecting the first certificate to be the signing certificate. Such an application written in openssl would be looking at the first cert in p7->d.sign->cert, rather than using PKCS7_get0_signers or PKCS7_cert_from_signer_info. |
The order of certificates written to a PKCS7 file is now likely to be different. That should be noted in a clear spot, including a HISTORY section of the manpage. |
@levitte Although the change looks small, won't this will create issues for existing data. |
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
@fishsoupisgood I'm not really sure what difference this PR makes in terms of encoding and how it can fix your problem. The reason is that this is implicitly tagged sequence so the type is not part of encoding. I actually took a time to generate signed data so you can compare the results below: Before the patch: https://bit.ly/39d95km Basically you won't see any difference because the type is not encoded (just the tag is encoded) so it doesn't really matter if it is specified as a sequence or set. The only difference is that the certs will be in correct order from the OpenSSL point of view. I think this is done on purpose to keep order of certificates when re-encoding which is done in the similar way in Hence I don't think this is a bug and there's anything to fix. |
The issue is very real, perhaps I can suggest you look at the example code which reproduces the issue in the linked bug report? |
Ah ok I haven't seen the linked bug. It validates the order so this makes sense. If that's the case, it might need to also change the I think I can at least answer the question that @slontis asked. As it's implicitly tagged and both SET OF and SEQUENCE OF are consctructed, then there shouldn't be any difference for decoding. At least that's how I understand it - haven't tried it. :) |
The commit message should be reformatted like this:
Github isn't very good at picking up hints from the commit subject line, which is why the links between issue and PR aren't visible in a github supported way. |
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago |
Please rebase and reformat the commit message as @levitte stated above. |
This PR is waiting for the creator to make requested changes but it has not been updated for 30 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section). |
This PR is waiting for the creator to make requested changes but it has not been updated for 61 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section). |
This PR is waiting for the creator to make requested changes but it has not been updated for 92 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section). |
Fixes openssl#13142 CLA: trivial
Since it's a trivial reformatting of the commit message, I edited it as requested and rebased the pull request branch. |
The CI failure is relevant! |
PKCS#7 stores certificates and CRLs in (implicitly-tagged) SET OF types. This means they're unordered and, in DER, must be sorted. We currently sort neither. OpenSSL upstream sorts CRLs but doesn't sort certificates. openssl/openssl#13143 reports that Microsoft has a stricter parser that checks this. This CL fixes both fields in our serializer. This does not change the parsing code, which still preserves whatever order we happened to find, but I've updated the documentation to clarify that callers should not rely on the ordering. Based on [0] and the odd order in kPKCS7NSS, I believe this aligns with NSS's behavior. Update-Note: It is no longer the case that constructing a PKCS#7 file and parsing them back out will keep the certificates and CRLs in the same order. [0] https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/net/x509_certificate_model_nss_unittest.cc;drc=c91b0c37b5ddf31cffd732c661c0c5930b0740f4;l=286 Change-Id: If776bb78476557af2c4598f1b6dc10e189adab5d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47304 Reviewed-by: Adam Langley <agl@google.com>
* Remove some BoringSSL-only X509_CINF functions. These functions are not in any released version of OpenSSL. The history is they were added to 1.0.2 beta for CT, but then removed in favor of i2d_re_X509_tbs. We forked in between the two events. I'm not sure what the reasoning was upstream's end. I'm thinking: - X509 currently only captures the serialized TBSCertificate. It might be nice to capture the whole Certificate to avoid needing a serialization in X509_cmp and make it easier to interop with other stacks. (Unclear.) That would require not exporting the X509_CINF standalone for serialization. - The modified bit means, without locking, i2d_X509 is not const or thread-safe. We *might* be able to shift the re-encoding to i2d_re_X509_tbs, which is already inherently non-const. That requires not having X509_CINF_set_modified. I'm not sure how feasible either of these are, but between that, upstream alignment, and X509_CINF otherwise being absent from public accessors, it seems worth removing. Update-Note: X509_get_cert_info, X509_CINF_set_modified, and X509_CINF_get_signature are removed. I believe all callers have been updated. Callers should use i2d_re_X509_tbs, i2d_X509_tbs, and X509_get0_tbs_sigalg instead. Change-Id: Ic1906ba383faa7903973cb498402518985dd838c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46985 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Include assembly optimizations in Bazel builds on Linux-aarch64. Signed-off-by: Piotr Sikora <piotrsikora@google.com> Change-Id: Ieb403b6651d445948abef48d7432fd248294284f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47084 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: David Benjamin <davidben@google.com> * Use a placeholder for unknown errors in ERR_*_error_string. Change-Id: I3a16fa731cfa7c92e5fec19f78ae48650921f626 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47104 Reviewed-by: Adam Langley <agl@google.com> * A couple of Aarch64 FIPS delocate fixes. Clang 12 in opt mode produces a couple of assembly patterns that were not handled by delocate. Firstly, two-digit vector indexes were just a simple omission. Fixed. Secondly, Clang puts symbol deltas in .byte directives, and bit-shifts them. The .byte directive was not considered to be a symbol-containing directive because it's too small, but it could store deltas. Additionally, bit-shifting of symbol expressions was not supported. Fixed. Change-Id: I796299821f5ac7d3639fa6243c5d9bd5342bbddf Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47064 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> * avcp: SHA-1 for ECDSA _verification_ is still supported by NIST. Change-Id: I26a643737e99ddf75af24143829df4551040f7db Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47144 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: David Benjamin <davidben@google.com> * Clarify OBJ_get0_data and OBJ_get_length. Someone asked me about this API and I realized it didn't clarify what DER representation. Change-Id: I3c53df200612dd5a8269a14dd04e7b430cd96389 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47124 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Const-correct ASN1_OBJECT_create. The implementation is a little goofy, but OBJ_dup internally makes a copy of all the data. Change-Id: I58e6804ede00100211ac112f03e26a34a2d29b5a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47125 Reviewed-by: Adam Langley <agl@google.com> * Rename X509V*_VERSION constants. Upstream ultimately preferred a different naming convention, and type-specific constants. Align with them. Update-Note: This renames some BoringSSL-specific constants that we recently added. It doesn't look like anyone's used them yet. Change-Id: I580e0872a5f09fb1c5bab9127c35f1ed852680c0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47164 Reviewed-by: Adam Langley <agl@google.com> * Use passive entropy collection everywhere. Change-Id: I40513b3947fa571d2d0b918641b9917451ced3e1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47284 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: David Benjamin <davidben@google.com> * Reference the newer ChaCha20-Poly1305 RFC. Just some errata applied, otherwise the same. https://tools.ietf.org/rfcdiff?url2=rfc8439&url1=rfc7539 Change-Id: I0cf5d50eeca7840d0ab99c54e06f1008ac423211 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47264 Reviewed-by: Adam Langley <agl@google.com> * Remove non-deterministic bits from ECDSA ACVP test. When updating the test file for SHA-1 support, I forgot to remove the non-deterministic bits (i.e. key and signature generation) from the input vectors. Change-Id: Id47f9b2cc85282f68b71aedc271d4b4b53e04c70 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47285 Reviewed-by: David Benjamin <davidben@google.com> * Document expected use of BTI and PAC macros. https://boringssl-review.googlesource.com/c/boringssl/+/42084's commit message did a good job of explaining how BTI and PAC work, but we're missing some documentation in the header on conventions. I think these are right? Bug: 409 Change-Id: I959e68d3ca076d0bdf9d1f2b5a5f0450023de4d6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47204 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Implement ECH draft 10 and update HPKE to draft 08. Bug: 275 Change-Id: I8096070386af7d2b5020875ea09bcc0c04ebc8cd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47245 Reviewed-by: Steven Valdez <svaldez@google.com> Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: Steven Valdez <svaldez@google.com> * Correctly order PKCS#7 certificates and CRLs. PKCS#7 stores certificates and CRLs in (implicitly-tagged) SET OF types. This means they're unordered and, in DER, must be sorted. We currently sort neither. OpenSSL upstream sorts CRLs but doesn't sort certificates. https://github.com/openssl/openssl/pull/13143 reports that Microsoft has a stricter parser that checks this. This CL fixes both fields in our serializer. This does not change the parsing code, which still preserves whatever order we happened to find, but I've updated the documentation to clarify that callers should not rely on the ordering. Based on [0] and the odd order in kPKCS7NSS, I believe this aligns with NSS's behavior. Update-Note: It is no longer the case that constructing a PKCS#7 file and parsing them back out will keep the certificates and CRLs in the same order. [0] https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/net/x509_certificate_model_nss_unittest.cc;drc=c91b0c37b5ddf31cffd732c661c0c5930b0740f4;l=286 Change-Id: If776bb78476557af2c4598f1b6dc10e189adab5d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47304 Reviewed-by: Adam Langley <agl@google.com> * Remove HKDF-SHA384 and HKDF-SHA512 from HPKE. We can add them if we need them, but we're only using HKDF-SHA256 in ECH. Keep the set small to encourage a common set of parameters. Bug: 410 Change-Id: I5b9ddf3daa1d0c7f35df473470998369e9882553 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47324 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> * Remove HPKE PSK mode. We can always add it back later, but nothing's using it right now. Looking at all references to draft-irtf-cfrg-hpke in the IETF tracker, there are zero uses of any of the modes beyond SetupBase. Bug: 410 Change-Id: I23deb27554d36152776417d86e7759cb2c22e4eb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47325 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Fix a memory leak with d2i_ASN1_OBJECT object reuse. (Imported from upstream's 65b88a75921533ada8b465bc8d5c0817ad927947 and 7c65179ad95d0f6f598ee82e763fce2567fe5802.) Change-Id: Id6a9604231d3cacc5e20af07e40d09e20dc9d3c0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47332 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Revise the deterministic for_test variant of HPKE's SetupBaseS. Although we only support X25519 right now, we may need to support other KEMs in the future. In the general case, a public/private keypair is less meaningful. (If something like NTRU-HRSS even goes here, I guess it'd be the entropy passed to HRSS_encap.) Instead of taking an entire keypair, just take the private key. Perhaps we call it the "seed"? Bug: 410 Change-Id: Ifd6b6ea8ea36e6eca60d303706d6d2620f8c42d4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47326 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Don't mark up the first word in a collective comment. Change-Id: I3fc0cc07d7a0a29df02601e321d5a5a9ff128bf9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47330 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Introduce EVP_HPKE_{AEAD,KDF} types. This replaces the ID-based API with one that is more static linker friendly. For ECH, it doesn't make a difference because we currently pull in all the options we've implemented. But this means other HPKE uses need not pull in everything ECH needs and vice versa. Along the way, fix an inconsistency: we prefixed all the AEAD constants with "AEAD", but not the others. Since the rest of the name already determines everything, go with the shorter version. Bug: 410 Change-Id: I56e46c13b43c97e15eeb45204cde7019dd21e250 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47327 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Switch HPKE to a three-parameter output buffer. This is a little tedious but aligns with some of our other variable-length parameters. This is in preparation for making the HPKE APIs KEM-agnostic, so we don't need to make so many variations on the HPKE functions for each KEM. (Especially if we ever need to implement SetupPSK*, SetupAuth*, or SetupAuthPSK*.) Bug: 410 Change-Id: I0625580b15358ab1f02b7835122256e8f058a779 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47328 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * acvp: move hash iterations into modulewrapper. In cases where the RPC from acvptool to modulewrapper is expensive, these iterated tests take excessive amounts of time. By moving the inner loop into the module wrapper the number of round-trips is reduced by 1000×. Change-Id: Ic047db071239492e416a08cab60d6a7e2905e8dc Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47364 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: David Benjamin <davidben@google.com> * Make X509_SIG and X509_CERT_AUX opaque. I meant to grab more interesting types this round, but I missed a few spots. We should be able to get these out of the way though. Update-Note: Direct access of these structs should be replaced by accessors. Change-Id: I43cb8f949d53754cfebef2f84be66e89d2b96f96 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47384 Reviewed-by: Adam Langley <agl@google.com> * Add SSL_can_release_private_key. Callers using private key callbacks may retain non-trivial state with a private key. In many cases, the private key is no longer necessary immediately after the first round-trip (e.g. non-HRR TLS 1.3 connections). Add a function that callers can query to drop the state a hair earlier. This is tested in two ways. First, the asserts in front of using the key, combined with existing tests, ensure we don't start reporting it too early. Second, I've added tests in ssl_test.cc to assert we report it as early as we expect to. In doing so, the number of parameters on ConnectClientAndServer() started getting tedious, so I've split that into a CreateClientAndServer() and CompleteHandshakes(). Callers that need to configure weird things or drive the handshake manually can call CreateClientAndServer() (which takes care of the BIO pair business) and continue from there. Bug: b/183734559 Change-Id: I05e1edb6d269c8468ba7cde7dc90e0856694a0ca Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47344 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Update ACVP URLs. NIST breaks these URLs so often it's unclear if it's worth including them. If they do it again it might be a signal to remove them all. However, until then, this change updates many of them. Some were deleted because the format of the anchors has been switched and all the section numbers remove, and I don't think it's worth trying to unpick all that. Change-Id: I31457c225e68ee44d383a5a148fdcc80a3430864 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47464 Commit-Queue: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: David Benjamin <davidben@google.com> * Shift the KEM dependency in HPKE up a step. This introduces an EVP_HPKE_KEM, to capture the KEM choice, and EVP_HPKE_KEY, to capture the key import (and thus avoids asking receivers to pass in the full keypair). It is a bit more wordy now, but we'll be in a better place when some non-TLS user inevitably asks for a P-256 version. Bug: 410 Change-Id: Icb9cc8b028e6d1f86e6d8adb31ebf1f975181675 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47329 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Refer to EVP_HPKE_CTX by a consistent name. It's sometimes hpke and sometimes ctx. Our other EVP_FOO_CTX types are usually called ctx, so use ctx. Bug: 410 Change-Id: Ib1c6d8018ffd8fd180b89f5be58283f3f098e44b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47404 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> * Export the HPKE implementation. Bug: 410 Change-Id: I633eab7f2d148c9158a5bb29d73e07f1f18b7105 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47331 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Fix the ech_accept comment. This was fixed in review, but we forgot to update the comment. Change-Id: If1fdd9211ff085edeb50457edf0caba5e31b6d16 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47448 Reviewed-by: Dan McArdle <dmcardle@google.com> Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> * Move session ID assignment out of ssl_get_new_session. It's kind of weird that we assign a session ID, based on whether we detect the handshake wants stateful resumption, and then erase it afterwards. Also remove the is_server parameter, which we can get from hs. Change-Id: I94ac817c63abb08a457e0e0c29f5c2d2b60aa498 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47444 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Simplify renego + resumption handling. We do not offer sessions on renegotiation. Rather than applying this at both the ClientHello and ServerHello, just drop ssl->session, which takes care of both cases. Change-Id: I5ebaedc8d9cc0fca61242ed9b85fa3449636dfec Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47445 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> * Don't use SHA256(ticket) as the signaling session ID for tickets. We've inherited some behavior from OpenSSL where, in ticket-based client sessions, we fill in a placeholder session ID of SHA256(ticket). This was done to avoid confusing other code in OpenSSL (and possibly callers?) that assumed session_id_length != 0 determined validity. Separately, TLS 1.2 session tickets are syntactically weird. The client generates a fake signaling session ID, which the server echoes on resumption. These combined meant we used the placeholder SHA256 value as this signaling ID. Since we already have code to generate random session IDs for TLS 1.3, use that instead to minimize unnecessary implementation quirks visible on the wire. This removes one of the places we still rely on the placeholders within the library. Change-Id: I0de2781da72e2bbc030505611589c853f105ce9d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47446 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> * Check for resumption identifiers in SSL_SESSION_is_resumable. This aligns with OpenSSL. In particular, we clear not_resumable as soon as the SSL_SESSION is complete, but it may not have an ID or ticket. (Due to APIs like SSL_get_session, SSL_SESSION needs to act both as a resumption handle and a bundle of connection properties.) Along the way, use the modified function in a few internal checks which, with the ssl_update_cache change, removes the last dependency within the library on the placeholder SHA256 IDs. Change-Id: Ic225109ff31ec63ec08625e9f61a20cf0d9dd648 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47447 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Add APIs to manually fill in signatures for CRLs. This adds CRL analogs to some X509 functions added in https://boringssl-review.googlesource.com/c/boringssl/+/43784. I missed that we need to support this for CRLs too. Change-Id: Id64952a1b2d33bcd057a96c80aadd97a3c3d9fb5 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47525 Reviewed-by: Adam Langley <agl@google.com> * Validate RSA public keys more consistently. https://boringssl-review.googlesource.com/c/boringssl/+/42504 aligned RSA private key checks, but I missed the public key ones. We have two different sets of RSA public key checks right now. One in the parser just checks for e = 1 and even e. The other, when using the key, checks for overly large e and n. Align the two. Now parsing RSA public keys calls RSA_check_key and the extra checks on e are added to RSA_check_key. Note RSA private key parsing already called RSA_check_key. The consequences are: First, RSA public keys with large n, large e, or n < e will be rejected at parse time. Previously, they would be parsed but all operations on them would fail. This aligns with our existing behavior for parsing private keys. Second, operations on RSA public keys with even e will fail. They already failed to parse, but it was possible to manually construct such a key. Previously, operations wouldn't explicitly fail, but they wouldn't do anything useful because even exponents are not invertible. (Encrypting would produce something undecryptable and the private key would have a hard time reliably producing signatures we'd accept.) There is no change to RSA private keys with even e. Those would already fail the (e, d) consistency check and the fault check. Third, operations on RSA public keys with e = 1 will fail. They already failed to parse, but it was possible to manually construct such a key and "verify" signatures or "encrypt" messages. However, with e = 1, those operations are no-ops. Finally, RSA private keys with e = d = 1 will be rejected at parse and use. This is the only case that affects private keys because e = d = 1 are inverses, just pointless. Uses paired with RSA public key parsing (e.g. our TLS library checks consistency with a certificate public key) are not affected. Those already rejected such keys because we rejected them in the public key parser. This CL aligns the private half. This doesn't close https://crbug.com/boringssl/316, but we won't be able to resolve that without a consistent story for what keys are valid. Update-Note: See above. Bug: 316 Change-Id: Ic27df18c4f48e5e3e57a17d6fe39399e2f8d5c68 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47524 Reviewed-by: Adam Langley <agl@google.com> * Be clearer which signing inputs are digests. We usually call the parameter 'digest', but people sometimes think they can skip the hashing for short inputs are short. I also suspect the term 'digest' is less common. Add warnings about this. There were also some cases where we called it 'in' and even 'msg'. This CL fixes those to say 'digest'. Finally, RSA_{sign,verify}_raw are documented to be building blocks of signature schemes, rather than signature schemes themselves. It's unfortunate that EVP_PKEY_sign means "sign a digest", while EVP_DigestSign means "sign, likely internally digesting it as the first step", but we're a bit stuck there. Change-Id: I4c38afff9b6196e2789cf27653fe5e5e8c68c1bf Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47504 Reviewed-by: Adam Langley <agl@google.com> * Fix some includes. My editor was being too clever. Change-Id: I7044a09de83d3530583424eb5da2183039fb0643 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47484 Reviewed-by: Adam Langley <agl@google.com> * Check hs->early_session, not ssl->session, for the early data limit. ServerHello/EncryptedExtensions/Finished is logically one atomic flight that exits the early data state, we have process each message sequentially. Until we've processed Finished, we are still in the early data state and must support writing data. Individual messages *are* processed atomically, so the interesting points are before ServerHello (already tested), after ServerHello, and after EncryptedExtensions. The TLS 1.3 handshake internally clears ssl->session when processing ServerHello, so getting the early data information from ssl->session does not work. Instead, use hs->early_session, which is what other codepaths use. I've tested this with runner rather than ssl_test, so we can test both post-SH and post-EE states. ssl_test would be more self-contained, since we can directly control the API calls, but it cannot test the post-EE state. To reduce record overhead, our production implementation packs EE and Finished into the same record, which means the handshake will process the two atomically. Instead, I've tested this in runner, with a flag to partially drive the handshake before reading early data. I've also tweaked the logic to hopefully be a little clearer. Bug: chromium:1208784 Change-Id: Ia4901042419c5324054f97743bd1aac59ebf8f24 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47485 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Make X509_REQ and X509_REQ_INFO opaque. We can unexport the X509_REQ_INFO type entirely. (NB: OpenSSL hasn't done this, but has unexported so much of X509_REQ_INFO that it is impossible to use what remains anyway.) Update-Note: Callers that reach into X509_REQ and X509_REQ_INFO must use accessors instead. Change-Id: I1eea5207b9195c8051d5e467acd63ad5f0caf89d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47564 Reviewed-by: Adam Langley <agl@google.com> * Remove draft tokbind implementation. We didn't end up deploying this. We also never implemented the final RFC, so what we do have isn't useful for someone who wishes to deploy it anyway. Update-Note: Token binding APIs are removed. Change-Id: Iecea7c3dcf9d3e2644a3b7afaf61511310b45d5f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47584 Reviewed-by: Adam Langley <agl@google.com> * Add a missing case to SSL_error_description. Change-Id: Ib8aaa2b6bfafc88cf51d2ae0f085bb87275a4306 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47585 Reviewed-by: Adam Langley <agl@google.com> * fuzz/minimise_corpora.sh: Add shebang and chmod +x The script now matches fuzz/refresh_ssl_corpora.sh. Change-Id: I0089c5091e3e21c5590a73909b05e066fefe4a34 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47604 Reviewed-by: Adam Langley <agl@google.com> * Update the ECH GREASE size selection. We misread (or maybe it changed?) the draft padding scheme. The current text does not round the whole payload to a multiple of 32, just the server name as a fallback. Switch the GREASE size selection to match. Although, we may want to change the draft here. See also https://github.com/tlswg/draft-ietf-tls-esni/issues/433 While I'm here, update some references from draft-09 to draft-10. Also make the comment less verbose. Bug: 275 Change-Id: I3c9f34159890bc3b7d71f6877f34b895bc7f9b17 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47644 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> * Const-correct SSL_get_srtp_profiles. This is part of a very deep dependency chain. I'm sniffing at making all the add_clienthello callbacks const. Between HelloVerifyRequest, HelloRetryRequest, and soon ECH, we're creating lots of ClientHellos per connection. That's probably easiest to manage if constructing a ClientHello had no side effects. Update-Note: The change to the return type isn't quite compatible, but I only found one caller of this function, which has since been fixed. (If we need to return a non-const value for compatibility, we can do that and document that the caller should not mutate the output.) Change-Id: I21f18f7438920a5b03d874fa548f054af3a42c4a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47664 Reviewed-by: Adam Langley <agl@google.com> * runner: Reject all zero client and server randoms. If we ever forget to fill it in the randoms, they'll end up all zero. Particularly at the ClientHello, that logic is getting increasingly far away from ClientHello serialization, so add a test to make sure we notice. (This will flakily fail with probability 2^-256, which is reasonably unlikely.) Change-Id: I81f32fd96dbccf377cb92198a222b557ab66976b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47665 Reviewed-by: Adam Langley <agl@google.com> * GREASE is now RFC 8701. I forgot to update the references. Change-Id: I1a746eec13afd9fd1e59ca1824b2dd0f83ff7f74 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47666 Reviewed-by: Adam Langley <agl@google.com> * Don't try to write empty early data in the tool. We'll return 0 and get confused. (Negotiating early data and not using it is plausible if, say, the client preconnects but gets a ServerHello before any request binds the socket.) Change-Id: I94d458e18c58223f73c9340cac06e5ec5f8c84a0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47684 Reviewed-by: Adam Langley <agl@google.com> * Implement fuzzer mode for ECH server. Now skipping over HPKE decryption in |ssl_client_hello_decrypt| when fuzzer mode is enabled. To improve code coverage, this fuzzer-only logic also also has the ability to simulate a failed decryption. As a result of mostly skipping the decryption, we now have to exclude "*-ECH-Server-Decline*" tests from running in fuzzer mode. These tests rely on the now-broken assumption that decryption will fail when the client used an ECHConfig unknown to the server. Bug: 275 Change-Id: I759a79c8596897cdd3d3a37e05f2973d47346ef9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47624 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: David Benjamin <davidben@google.com> * Refresh SSL corpora after adding ECH fuzzer mode. (cd build_Fuzzer ; cmake -DFUZZ=1 -GNinja .. ; autoninja) (cd build_NoFuzzer ; cmake -DFUZZ=1 -DNO_FUZZER_MODE=1 -GNinja .. ; autoninja) (cd fuzz ; ./refresh_ssl_corpora.sh ../build_Fuzzer ../build_NoFuzzer) Bug: 275 Change-Id: If47c323d07414da290bc492eda41bebc972c01af Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47724 Reviewed-by: David Benjamin <davidben@google.com> * Test ECH server with unique and repeated config IDs. Also shortens ECH variable names in runner.go. Bug: 275 Change-Id: Iaef520ae09eb94f714fbdaa4383d1456add6f113 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47744 Commit-Queue: Dan McArdle <dmcardle@google.com> Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: David Benjamin <davidben@google.com> * Don't copy client's session ID into server's session. When decrypting a ticket we would copy the client's session ID into the session and then copy the session's ID into the ServerHello (if resuming). That seems icky. Instead install the same placeholder on the server as we do on the client. Change-Id: Icb50a3be2f05e6428f1b286c8c09015f7bb4af16 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47784 Commit-Queue: Adam Langley <agl@google.com> Reviewed-by: David Benjamin <davidben@google.com> * Fix array-parameter warnings e.g. /home/peter/boringssl/crypto/curve25519/curve25519.c:503:57: error: argument 2 of type 'const uint8_t[32]' {aka 'const unsigned char[32'} with mismatched bound [-Werror=array-parameter=] 503 | int x25519_ge_frombytes_vartime(ge_p3 *h, const uint8_t s[32]) { | ~~~~~~~~~~~~~~^~~~~ In file included from /home/peter/boringssl/crypto/curve25519/curve25519.c:33: /home/peter/boringssl/crypto/curve25519/internal.h:109:58: note: previously declared as 'const uint8_t *' {aka 'const unsigned char *'} 109 | int x25519_ge_frombytes_vartime(ge_p3 *h, const uint8_t *s); | ~~~~~~~~~~~~~~~^ /home/peter/boringssl/crypto/curve25519/curve25519.c:823:57: error: argument 2 of type 'const uint8_t *' {aka 'const unsigned char *'} declared as a pointer [-Werror=array-parameter=] 823 | void x25519_ge_scalarmult_base(ge_p3 *h, const uint8_t *a) { | ~~~~~~~~~~~~~~~^ In file included from /home/peter/boringssl/crypto/curve25519/curve25519.c:33: /home/peter/boringssl/crypto/curve25519/internal.h:117:56: note: previously declared as an array 'const uint8_t[32]' {aka 'const unsigned char[32]'} 117 | void x25519_ge_scalarmult_base(ge_p3 *h, const uint8_t a[32]); | ~~~~~~~~~~~~~~^~~~~ cc1: all warnings being treated as errors Change-Id: I7e9b68fe261a94834f519057adb6ff90c0cb73cf Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47805 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> * Ensure name not null in EVP_get_cipherbyname This adds a check to EVP_get_cipherbyname which ensures that name is not null when passed to OPENSSL_strcasecmp, which cannot handle null values. OpenSSL already ensures this in their implementation of EVP_get_cipherbyname by using OBJ_NAME_get, so this improves parity. Change-Id: Icea45a5da2a7a461d2a65fbfbc84653c4f124dab Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47844 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> * Pull HASH_TRANSFORM out of md32_common.h. The macro isn't doing any work here. Change-Id: Id97dfa4b027407c5e4b3e7eb1586c3c2a2d977d8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47806 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Make md32_common.h single-included and use an unsized helper for SHA-256. Similar to https://boringssl-review.googlesource.com/c/boringssl/+/46405, SHA256_Final and SHA224_Final hit array size warnings in the new GCC. The array sizes are, strictly speaking, purely decoration, but this is a good warning so we should be clean with it on. That same change is difficult to apply to md32_common.h because md32_common.h generates the functions for us. md32_common.h is already strange in that it is multiply-included and changes behavior based on macros defined by the caller. Instead, replace it with inline functions, which are a bit more conventional and typesafe. This allows each hash function to define the function prototype. Use this to add an unsized helper for SHA-256. Bug: 402 Change-Id: I61bc30fb58c54dd40a55c9b1ebf3fb9adde5e038 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47807 Reviewed-by: Adam Langley <agl@google.com> Reviewed-by: Peter Foley <pefoley@google.com> Commit-Queue: David Benjamin <davidben@google.com> * Add compatibility impl for EVP_PKEY_get0 Node.js uses EVP_PKEY_get0, which is present in OpenSSL but which BoringSSL currently does not export. This CL adds an implementation for it, which Electron is currently floating as a patch. See https://github.com/nodejs/node/commit/6a7eb32c5bcd92f490a57f2cd12d52e1db881d17 from Node. Change-Id: I2474cacbd22882355a8037e2033739f7496b21f2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47824 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> * Cite an RFC over 9000 (draft-ietf-quic-tls is now RFC 9001). Also now that it's finalized, flip the default for SSL_set_quic_use_legacy_codepoint. Update-Note: QUIC APIs now default to the standard code point rather than the draft one. QUICHE has already been calling SSL_set_quic_use_legacy_codepoint, so this should not affect them. Once callers implementing the draft versions cycle out, we can then drop SSL_set_quic_use_legacy_codepoint altogether. I've also bumped BORINGSSL_API_VERSION in case we end up needing an ifdef. Change-Id: Id2cab66215f4ad4c1e31503d329c0febfdb4603e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47864 Reviewed-by: David Schinazi <dschinazi@google.com> Reviewed-by: Adam Langley <agl@google.com> * fix #415: Perl scripts fail when building from a path with spaces Because file names are not enclosed in quotation marks in the open call. https://bugs.chromium.org/p/boringssl/issues/detail?id=415 ``` cmake --build "C:\Projects\ Extern\Visual C++ 2015\x64 Debug\Build\BoringSSL\." [9/439] Generating rdrand-x86_64.asm FAILED: crypto/fipsmodule/rdrand-x86_64.asm cmd.exe /C "cd /D "C:\Projects\ Extern\Visual C++ 2015\x64 Debug\Build\BoringSSL\crypto\fipsmodule" && "C:\Program Files\CMake\bin\cmake.exe" -E make_directory . && C:\Perl64\bin\perl.exe "C:/Projects/ Extern/Source/BoringSSL/crypto/fipsmodule/rand/asm/rdrand-x86_64.pl" nasm rdrand-x86_64.asm" Can't open perl script "C:/Projects/": No such file or directory error closing STDOUT at C:/Projects/ Extern/Source/BoringSSL/crypto/fipsmodule/rand/asm/rdrand-x86_64.pl line 87. ninja: build stopped: subcommand failed. ``` Bug: 415 Change-Id: I83c4a460689b9adeb439425ad390322ae8b2002a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47884 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> * Remove impossible ssl->s3 null check. ssl->s3 is never null. And if it were, we'd have crashed long before. Change-Id: Idb441c3a91d8c77327a0f9a6d193a64367f347ee Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47904 Reviewed-by: Adam Langley <agl@google.com> * DTLS-SRTP is only defined for DTLS. This avoids needing to worry about the interaction with renegotiation which, in turn, means we can drop the init callback. (If we did support DTLS renegotiation, we'd probably want to forbid the parameter from changing anyway. Changing your SRTP parameters partway through will likely confuse the RTP half of the application anyway.) Change-Id: Ifef1e9479d9df296b69b0d296f6bef57b13da68e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47905 Reviewed-by: Adam Langley <agl@google.com> * Manage Channel ID handshake state better. The channel_id_valid bit is both used for whether channel_id is filled in (SSL_get_tls_channel_id), and whether this particular handshake will eventually negotiate Channel ID. The former means that, if SSL_get_tls_channel_id is called on the client, we'll return all zeros. Apparently we never fill in channel_id on the client at all. The latter means the state needs to be reset on renegotiation because we do not currently forbid renegotiation with Channel ID (we probably should...), which is the last use of the init callback for extensions. Instead, split this into a bit for the handshake and a bit for the connection. Note this means we actually do not expose or even retain whether Channel ID was used on the client. This requires a tweak to the handoff logic, but it should be compatible. The serialized ssl->s3->channel_id was always a no-op: the handback happens before the ChannelID message, except in RSA key exchange. But we forbid Channel ID in RSA key exchange anyway. Update-Note: SSL_get_tls_channel_id will no longer return all zeros during the handshake or on the client. I did not find any callers relying on this. Change-Id: Icd4b78dd3f311d1c7dfc1cae7d2b86dc7e327a99 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47906 Reviewed-by: Adam Langley <agl@google.com> * Remove the Channel ID callback. The remaining remnants of Channel ID all configure the private key ahead of time. Unwind the callback machinery, which cuts down on async points and the cases we need to test. This also unwinds some odd interaction between the callback and SSL_set_tls_channel_id_enabled: If a client uses SSL_set_tls_channel_id_enabled but doesn't set a callback, the handshake would still pause at SSL_ERROR_WANT_CHANNEL_ID_LOOKUP. This is now removed, so SSL_set_tls_channel_id_enabled only affects the server and SSL_CTX_set1_tls_channel_id only affects the client. Update-Note: SSL_CTX_set_channel_id_cb is removed. SSL_set_tls_channel_id_enabled no longer enables Channel ID as a client, only as a server. Change-Id: I89ded99ca65e1c61b1bc4e009ca0bdca0b807359 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47907 Reviewed-by: Adam Langley <agl@google.com> * Const-correct message creation hooks. Make it a little clearer they shouldn't be updating sequence numbers, enqueuing the message, etc. That's left to add_message. (ECH clients need to construct a pair of parallel ClientHellos.) Change-Id: I554a8f200d464727bc219b66931b3d0bae266f76 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47908 Reviewed-by: Adam Langley <agl@google.com> * Move ECH-related APIs to encrypted_client_hello.cc. Bug: 275 Change-Id: Ib5804ce3d0a5faff5cf26af544a4afaaf0ad2cc8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47909 Reviewed-by: Adam Langley <agl@google.com> * Reject the ECH extension in TLS 1.2 ServerHello. The ECH server extension is defined for TLS 1.3 EncryptedExtensions, not TLS 1.2 ServerHello. Bug: 275 Change-Id: Ie6e76c238075d70e6a0694ec0192df07da3457d1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47910 Reviewed-by: Adam Langley <agl@google.com> * Add SSL_ech_accepted API and ech_is_required alerts. The first thing any deployment will want to monitor is whether ECH was actually used. Also it's useful if the command-line tool can output this. (The alert is how the client signals it discarded the connection due to ECH reject.) This also disables ECH with the handoff mechanism for now. (The immediate cause being that ech_accept isn't serialized.) We'll probably need to make some decisions around the ordering here, since ECH affects where the true ClientHello is available. Bug: 275 Change-Id: Ie4559733290e653a514fcd94431090bf86bc3172 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47911 Reviewed-by: Adam Langley <agl@google.com> * Fix ECH-Server-RepeatedConfigID test. The test was not actually using a repeated config ID. Bug: 275 Change-Id: I69519fde196247abb07dceba1da1bac22188f13f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47912 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * runner: Revise ECHConfig type in preparation for client implementation An ECHConfig is like a certificate in that knowing the fields isn't sufficient. The exact byte representation is significant. (The ECHConfig is bound into the encryption.) But the ECHConfig type only has fields, so runner can only represent ECHConfigs that are the output of our serialization function. This matters less as a client testing a server because the server can only parse ECHConfigs with fields we support. But as a server testing a client, we need to see how the client reacts to extra extensions, etc. Just using []byte to represent ECHConfigs is inconvenient, so instead pattern this after x509.Certificate: you can parse one from a byte string (not currently included since we don't need it yet), or you can construct a new one from a template with the fields you want. Bug: 275 Change-Id: I6602d0780b1cef12b6c4b442999bdff7b3d7dd70 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47964 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> * runner: Make echIsInner a boolean. Having the nil vs. non-nil []byte for the sake of a couple tests with invalid payloads is tedious. Use separate fields instead. Bug: 275 Change-Id: I557d914d60ce94d68796c05162ff3dd2ab7684db Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47965 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> * runner: Parse the status_request extension more strictly. Noticed this while I was in the area. We currently use an extremely lax parse that even tolerates syntax errors. Instead use a strict parse that ensures our client only sends what we expect. Change-Id: Ifb0e1e1698489ff217db0c7a0317caa885e20759 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47966 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com> * runner: Implement ECH server for testing. This implements draft-ietf-tls-esni-10. This will be used to test the client implementation. While I'm here, I've switched the setup logic in the server tests to use the new ServerECHConfig type. I'll probably need to patch in various features later for testing, but this should be a usable starting point. Based on an initial implementation by Dan McArdle in https://boringssl-review.googlesource.com/c/boringssl/+/46786 Bug: 275 Change-Id: I69523cda70c3da2ae505bcab837fd358195fb9e9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47967 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Update build files in generated-src * Implement a handshake hint for certificate compression. While decompression is deterministic, compression is not. New revisions of the compression algorithm may start using different (hopefully smaller!) compressions. So this doesn't cause hint mismatches, add a certificate compression hint. If the shim's Certificate message matches the handshaker, we'll reuse the already compressed message. This also adds what appears to be a missing test for when the server cannot find compression algorithms in common. Change-Id: Idbedaceb20208463d8f61581ee27971c17fcd126 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48005 Reviewed-by: Adam Langley <agl@google.com> * Move the early_data_{offered,reason} logic out of extension callbacks. ECH requires that we construct two ClientHellos. That means our add_clienthello callbacks will need to be called multiple times and should be const. (They already are called multiple times for HelloRetryRequest, but we currently thread that through the callbacks a bit. With ECH, I think we need to make them pure serialization.) Bug: 275 Change-Id: I11f8195fd2ec4b8639f0a2af01a24d4974445580 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47984 Reviewed-by: Adam Langley <agl@google.com> * Release some temporaries outside of ClientHello callbacks. Also add ECH GREASE state into the mix. Clearing this isn't critical, especially now that we have an SSL_HANDSHAKE structure, but it's easy enough. Bug: 275 Change-Id: If1aa8d5c0c8fdb5af710852778ce452c507a2524 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47985 Reviewed-by: Adam Langley <agl@google.com> * Move key_share computation out of ClientHello callbacks. Like the early_data CL, this does shift a bit of logic that was previously hidden away in the callbacks. For key_share, this is probably a good move independent of ECH. The logic around HRR, etc., was a little messy. Bug: 275 Change-Id: Iafbcebdf66ce1f7957d798a98ee6b996fff24639 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47986 Reviewed-by: Adam Langley <agl@google.com> * Remove the extension init hook. This is now never used. Instead, we rely on each renegotiation creating a new handshake structure with fresh state. This simplifies things for ECH. (We probably could make an init hook work with ECH's two-ClientHello scheme by either maintaining separate state per ClientHello or calling init once for both ClientHellos. But the few uses of init were removable, so this is easier.) Bug: 275 Change-Id: Ie5e132fe072e5ea8db21ca16aa53fcd0895d8e48 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47987 Reviewed-by: Adam Langley <agl@google.com> * Initialize grease_seed on construction. This lets ssl_get_grease_value be const. The lazy thing isn't a deal-breaker (we only need idempotence, and a non-thread-safe const also works fine), but just initializing it earlier seems simpler. Bug: 275 Change-Id: Iad228ea4a9146ede9a3849f3339f7ec9e698e6eb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47988 Reviewed-by: Adam Langley <agl@google.com> * Pick up the GREASE ECH config ID from grease_seed. This avoids an unnecessary one-byte RAND_bytes call. Bug: 275 Change-Id: Idf5bfb17401441f2af7b3c784f7b5d876d005165 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47989 Reviewed-by: Adam Langley <agl@google.com> * Compute the ECH GREASE payload outside of the callbacks. This is kinda annoying and, like the grease_seed, demonstrates a shortcoming with the idea of making add_clienthello completely const. Strictly speaking, we only need idempotence. But I think this is okay: const is much easier to enforce than idempotence, and we'll likely need to do this anyway: - While not in the current draft, I expect the draft's eventual HRR resolution to retain the ECH extension, GREASE or not, on ECH reject. Things are somewhat violating RFC8446 HRR rules right now. That means we'll need to stash the ECH payload regardless. - ECH binds all the other extensions in the outer ClientHello, so computing the payload will need to move outside the callback system anyway. In some sense, all this is shifting our ClientHello output from the "immediate mode" end of the spectrum (as we usually use) to the "retained mode" end, which I suppose makes sense as this message becomes more intricate. Bug: 275 Change-Id: I9eb8cd1cde2ce264345b6ed3ee526d4eab81e911 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47990 Reviewed-by: Adam Langley <agl@google.com> * Fix documentation typo. Change-Id: I80083805a64665f46a6a4d85e1d9d52b1722264d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48025 Reviewed-by: Adam Langley <agl@google.com> * Make add_clienthello callbacks const. This is less effective than it seems because both ((const SSL_HANDSHAKE*)hs)->ssl and ((const SSL*)ssl)->s3 are both non-const pointers. I considered moving hs->ssl to hs->ssl_ and adding const-overloaded accessors, but I'd need to repeat the same with ssl->s3, at which point this seemed not worth it yet. Maybe later if we rewrite everything to more idiomatic C++. Bug: 275 Change-Id: I9912a3df205916fdf2191a3687013d717528038d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47991 Reviewed-by: Adam Langley <agl@google.com> * Replace hs->needs_psk_binder with an output parameter. May not be strictly necessary, but similarly easier to reason about when we need to interweave multiple ClientHellos. Bug: 275 Change-Id: I9f85787860f3e8ce1653331ce52343d5bf5def23 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47992 Reviewed-by: Adam Langley <agl@google.com> * Add move support to EVP_MD_CTX. We'll need to maintain two transcripts on the ECH client and then, once we know which of ClientHelloOuter or ClientHelloInner is used, overwrite the default transcript with the alternate one. Rather than indirect through a pointer, move support is easy enough. Then this can just be hs->transcript = std::move(hs->inner_transcript). Bug: 275 Change-Id: Id4b0a0a48b956cd65ce8fc3dacfd16eebe2eb778 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47993 Reviewed-by: Adam Langley <agl@google.com> * Add a note about extension callback names. For TLS 1.3, since the bulk of extensions move to EncryptedExtensions, we made the extension callbacks apply to EncryptedExtensions and pulled the few ServerHello extensions out of the callback system. This means the ServerHello naming is a little confusing. We probably should rename these callbacks, though parse_server is a bit ambiguous as to whether this is "parse the extension from the server" or "parse as a server". For now, add a comment. Change-Id: If1aa0846426de2cc8dcb2253695a8dd3285f7b76 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47994 Reviewed-by: Adam Langley <agl@google.com> * Shift some complexity out of ssl_add_clienthello_tlsext. ssl_add_clienthello_tlsext is about to get kinda messy with ECH. Move the padding and GREASE extensions into a few helpers. Bug: 275 Change-Id: I3bb702fb79dce4be68490c4a8fd889121ecdae58 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47995 Reviewed-by: Adam Langley <agl@google.com> * Move the TLS vs DTLS header length adjustment into ssl_add_clienthello_tlsext. This makes calls to ssl_add_clienthello_tlsext a hair easier. Also we only apply the [256, 511) compatibility hack to TLS, so we can just use a constant. Bug: 275 Change-Id: Ia2b5192aeef0cd8848ecfa1ea3b89a0a7382ff1a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47996 Reviewed-by: Adam Langley <agl@google.com> * Tidy up the PSK binder logic. Computing the binders on ClientHelloInner is a little interesting. While I'm in the area, tidy this up a bit. The exploded parameters may as well be an SSL_SESSION, and hash_transcript_and_truncated_client_hello can just get folded in. Change-Id: I9d3a7e0ae9f391d6b9a23b51b5d7198e15569b11 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47997 Reviewed-by: Adam Langley <agl@google.com> * Fix ext_pre_shared_key_clienthello_length calculation. If we're dropping the PSK extension due to an HRR with mismatched hash (looking back at that, we could easily have avoided that nuisance... I've filed [0] on rfc8446bis), we don't predict the length correctly. The consequence is we don't pad the second ClientHello to the desired range. Fix this and add an assert. [0] https://github.com/tlswg/tls13-spec/issues/1227 Change-Id: I47d880b6bdafa95840f7513b6b7718851f22554d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47998 Reviewed-by: Adam Langley <agl@google.com> * Don't pad the second ClientHello. While the previous CL fixed a bug in computing this padding, we don't actually need to pad the second (cleartext) ClientHello anyway. This padding is to work around bugs in old F5 and WebSphere servers, which do not speak TLS 1.3. Save a few bytes. Change-Id: I9b5d9bb1c0d880f1b1c9182667a9d3d82588c04c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47999 Reviewed-by: Adam Langley <agl@google.com> * runner: Self-check tests more accurately and earlier. We didn't correctly handle tests where the versions figure into resumeConfig and got by because the test didn't actually check the version. Run it more accurately, and check more fields. Also add a skipVersionNameCheck option for when the heuristic doesn't work. (Some of the tests specify a TLS maximum version by passing in all the -no-tls1, etc., flags for the other versions. Moreover, some of them will set no flags for a maximum of TLS 1.3. Suppress the check on those tests.) Change-Id: I72c069b1baed09e29bf502036957fe0e90edbe60 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48000 Reviewed-by: Adam Langley <agl@google.com> * Rename SSL_ECH_SERVER_CONFIG_LIST to SSL_ECH_KEYS. The old name was really long and a bit tedious to type out. Bug: 275 Change-Id: Ie24ef811f9288e619148a2bed36ca34b67af0a3a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48001 Reviewed-by: Adam Langley <agl@google.com> * Make ECH server APIs take EVP_HPKE_KEY. Previously we would extract the KEM ID from the ECHConfig and then parse the private key using the corresponding KEM type. This CL makes it take a pre-pared EVP_HPKE_KEY and checks it matches. This does require the caller pass the key type through externally, which is probably prudent? (On the other hand we are still inferring config from the rest of the ECHConfig... maybe we can add an API to extract the EVP_HPKE_KEM from a serialized ECHConfig if it becomes a problem. I could see runner or tool wanting that out of convenience.) The immediate motivation is to add APIs to programmatically construct ECHConfigs. I'm thinking we can pass a const EVP_HPKE_KEY * to specify the key, at which point it's weird for SSL_ECH_KEYS_add to look different. Bug: 275 Change-Id: I2d424323885103d3fe0a99a9012c160baa8653bd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48002 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Add a basic API to make ECHConfigs. We'll probably need to make this more complex later, but this should be a start. I had hoped this would also simplify tests, MakeECHConfig() was still needed to generate weird inputs for tests. I've instead tidied that up a bit with a params structure. Now the only hard-coded ECHConfig in tests is to check the output of the new API. Bug: 275 Change-Id: I640a224fb4b7a7d20e8a2cd7a1e75d1e3fe69936 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48003 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Add most of an ECH client implementation. Based on an initial implementation by Dan McArdle at https://boringssl-review.googlesource.com/c/boringssl/+/46784 This CL contains most of a client implementation for draft-ietf-tls-esni-10. The pieces missing so far, which will be done in follow-up CLs are: 1. While the ClientHelloInner is padded, the server Certificate message is not. I'll add that once we resolve the spec discussions on how to do that. (We were originally going to use TLS record-level padding, but that doesn't work well with QUIC.) 2. The client should check the public name is a valid DNS name before copying it into ClientHelloOuter.server_name. 3. The ClientHelloOuter handshake flow is not yet implemented. This CL can detect when the server selects ClientHelloOuter, but for now the handshake immediately fails. A follow-up CL will remove that logic and instead add the APIs and extra checks needed. Otherwise, this should be complete, including padding and compression. The main interesting point design-wise is that we run through ClientHello construction multiple times. We need to construct ClientHelloInner and ClientHelloOuter. Then each of those has slight variants: EncodedClientHelloInner is the compressed form, and ClientHelloOuterAAD just has the ECH extension erased to avoid a circular dependency. I've computed ClientHelloInner and EncodedClientHelloInner concurrently because the compression scheme requires shifting the extensions around to be contiguous. However, I've computed ClientHelloOuterAAD and ClientHelloOuter by running through the logic twice. This probably can be done better, but the next draft revises the construction anyway, so I'm thinking I'll rework it then. (In the next draft, we use a placeholder payload of the same length, so we can construct the ClientHello once and fill in the payload.) Additionally, now that we have a client available in ssl_test, this adds a threading test to confirm that SSL_CTX_set1_ech_keys is properly synchronized. (Confirmed that, if I drop the lock in SSL_CTX_set1_ech_keys, TSan notices.) Change-Id: Icaff68b595035bdcc73c468ff638e67c84239ef4 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48004 Reviewed-by: Adam Langley <agl@google.com> * Remove outdated comment in primality testing. This comment dates to SSLeay. It appears to be describing the incremental trial division strategy where they would pick a starting candidate, compute moduli by small primes, and then update by incrementing the candidate and saved moduli instead of dividing from scratch. We use a simpler rejection sampling strategy. Change-Id: If2203d616f2b1f632bcd7033ceb60a83d1b75674 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48047 Reviewed-by: Adam Langley <agl@google.com> * runner: Check the test name against the protocol being tested. This would have caught an issue with some tests I was working on. It also catches an issue with some per-message tests, so fix those. Change-Id: I6b3ad8e0db0b1a6ccac4b346dcc652b16b73e006 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48046 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Add an option to permute ClientHello extension order. Although not permitted by the TLS specification, systems sometimes ossify TLS extension order, or byte offsets of various fields. To keep the ecosystem healthy, add an API to reorder ClientHello extensions. Since ECH, HelloRetryRequest, and HelloVerifyRequest are sensitive to extension order, I've implemented this by per-connection permutation of the indices in the kExtensions structure. This ensures that all ClientHellos within a connection are consistently ordered. As follow-up work, permuting the other messages would also be nice, though any server messages would need to be incorporated in handshake hints. Change-Id: I18ce39b4df5ee376c654943f07ec26a50e0923a9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48045 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * More reliably report handshake errors through SSL_write. This CL fixes a couple of things. First, we never tested that SSL_write refuses to write application data after a fatal alert, so add some tests here. With those tests, we can revise some of this logic: Next, this removes the write_shutdown check in SSL_write and instead relies on the lower-level versions of the check in the write_app_data, etc., hooks. This improves error-reporting on handshake errors: We generally try to make SSL_do_handshake errors sticky, analogous to handshakeErr in the Go implementation. SSL_write and SSL_read both implicitly call SSL_do_handshake. Callers driving the two in parallel will naturally call SSL_do_handshake twice. Since the error effectively applies to both operations, we save and replay handshake errors (hs->error). Handshake errors typically come with sending alerts, which also sets write_shutdown so we don't try to send more data over the channel. Checking this early in SSL_write means we don't get a chance to replay the handshake error. So this CL defers it, and the test ensures we still ultimately get it right. Finally, https://crbug.com/1078515 is a particular incarnation of this. If the server enables 0-RTT and then reverts to TLS 1.2, clients need to catch the error and retry. There, deferring the SSL_write check isn't sufficient, because the can_early_write bit removes the write path's dependency on the handshake, so we don't call into SSL_do_handshake at all. For now, I've made this error path clear can_early_write. I suspect we want it to apply to all handshake errors, though it's weird because the handshake error is effectively a read error in 0-RTT. We don't currently replay record decryption failures at SSL_write, even though those also send a fatal alert and thus break all subsequent writes. Bug: chromium:1078515 Change-Id: Icdfae6a8f2e7c1b1c921068dca244795a670807f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48065 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> * Add util/fetch_ech_config_list.go I wrote this tool to make it easier to test the ECH client against real-world servers with the bssl client tool. I found that manually extracting an ECHConfigList from a raw HTTPS record is unnecessarily painful. The tool queries DNS over UDP for HTTPS records. If it finds any HTTPS records in the response, it attempts to extract an ECHConfigList from the "ech" SvcParam. It can write each extracted ECHConfigList to a file in a given directory. Once the ECH client implementation lands, the bssl client tool should have a new flag that that takes the path to an ECHConfigList file. I am using golang.org/x/net/dns/dnsmessage to parse the DNS response. I recently added the |UnknownResource| type to this library to enable callers (like us) to extract the bytes of otherwise-unsupported records (like HTTPS). I updated the dependency with `go get -u golang.org/x/net`. Although the bssl client tool knows how to resolve the address of its "-connect" parameter, it is difficult to query HTTPS records in a platform-agnostic way. If we decide the bssl client should directly query HTTPS rather than leaning on fetch_ech_config_list.go, we should look into libresolv. Specifically, the |res_query| function enables the caller to query arbitrary record types. This may open its own can of cross-platform worms; macOS and Linux typically ship with different implementations and it is not available on Windows. For more info, see `man 3 resolver`. Bug: 275 Change-Id: I705591658921f60a958164a18b68ffb697c2ea4b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44104 Reviewed-by: David Benjamin <davidben@google.com> * Revert "Add util/fetch_ech_config_list.go" This reverts commit 160a8891ae9a1d03f29aec079a67d97bc773990e. Reason for revert: This broke go.sum on CI for some reason. Will fix and reland. Original change's description: > Add util/fetch_ech_config_list.go > > I wrote this tool to make it easier to test the ECH client against > real-world servers with the bssl client tool. I found that manually > extracting an ECHConfigList from a raw HTTPS record is unnecessarily > painful. > > The tool queries DNS over UDP for HTTPS records. If it finds any HTTPS > records in the response, it attempts to extract an ECHConfigList from > the "ech" SvcParam. It can write each extracted ECHConfigList to a file > in a given directory. Once the ECH client implementation lands, the bssl > client tool should have a new flag that that takes the path to an > ECHConfigList file. > > I am using golang.org/x/net/dns/dnsmessage to parse the DNS response. I > recently added the |UnknownResource| type to this library to enable > callers (like us) to extract the bytes of otherwise-unsupported records > (like HTTPS)…
PKCS#7 stores certificates and CRLs in (implicitly-tagged) SET OF types. This means they're unordered and, in DER, must be sorted. We currently sort neither. OpenSSL upstream sorts CRLs but doesn't sort certificates. openssl/openssl#13143 reports that Microsoft has a stricter parser that checks this. This CL fixes both fields in our serializer. This does not change the parsing code, which still preserves whatever order we happened to find, but I've updated the documentation to clarify that callers should not rely on the ordering. Based on [0] and the odd order in kPKCS7NSS, I believe this aligns with NSS's behavior. Update-Note: It is no longer the case that constructing a PKCS#7 file and parsing them back out will keep the certificates and CRLs in the same order. [0] https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/net/x509_certificate_model_nss_unittest.cc;drc=c91b0c37b5ddf31cffd732c661c0c5930b0740f4;l=286 Change-Id: If776bb78476557af2c4598f1b6dc10e189adab5d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47304 Reviewed-by: Adam Langley <agl@google.com>
This PR is waiting for the creator to make requested changes but it has not been updated for 30 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section). |
This PR is waiting for the creator to make requested changes but it has not been updated for 61 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section). |
This PR has been closed. It was waiting for the creator to make requested changes but it has not been updated for 90 days. |
RFC2315 states that the list of certificates in PKCS7 signed data objects is an implicit SET OF not SEQUENCE OF as is currently in the codebase. Because of this signatures generated by openssl fail to validate on Microsoft's new stricter DER parser in wintrust.dll.