Various Updates #137
Open
Various Updates #137
+7,827
−4,899
Conversation
This file contains hidden or 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
We should be able to parse a variety of valid types, and also reject syntax errors in any types that we recognize. Also double-check that nothing went wrong in the translation from i2d_ASN1_TYPE to X509_ALGOR (basically the only use of ASN1_TYPE in the library). Change-Id: I825f44c35a7e8ea6374641cf5e0ea5daaf471ad7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81727 Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
EXTERNAL, EMBEDDED PDV, and CHARACTER STRING are actually constructed! They're encoded as some implicitly-tagged SEQUENCE. I couldn't be bothered to fill in valid contents thought. While I'm here, make RELATIVE-OID non-empty. That is the most likely new type for us to pay attention to here, just because we borrowed them for trust anchor IDs. (I'm not totally clear on whether the empty relative OID is a valid relative OID. Meh.) Change-Id: I193e5fb800d317cad8711a09078ac15adf80d76a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81728 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Update-Note: ASN1_TIME, DIRECTORYSTRING, and DISPLAYTEXT are no longer usable in custom macro-based ASN.1 types using <openssl/asn1t.h>. There do not seem to be any external callers that depend on this. If this sticks then, after all the built-in types are rewritten, we may be able to remove MSTRING from tasn_* altogether! Bug: 42290417 Change-Id: Ia12d8767bcada8eda75550e13bbab5a4a572382c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81729 Commit-Queue: David Benjamin <[email protected]> Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
These functions only need to pull in the algorithms they support. The new API is slightly less friendly in this context because it expects the caller to have found the end first, but it's easy enough to to write a small wrapper. Bug: 42290364 Change-Id: Ibdc44f399182cd5722700917aec6c151221f4674 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81747 Reviewed-by: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Mostly so we don't forget to const them when X509_NAME is finally fixed. Bug: 42290269 Change-Id: I78a8b31d9b846669db1ba2e98133203e5be24949 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81748 Reviewed-by: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Every time I touch this function, I forget that the algorithm update actually impacts the serialization. Change-Id: I8d484f9616d01a6ddd1ad428b01ac4bc922800ab Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81749 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Older versions of Go did not encode extension-less certificates correctly. See https://go-review.googlesource.com/c/go/+/399827 Rerun the script now that the bug is fixed. Bug: 442221114 Change-Id: I121fe3120d933a0a8fcaf1757cf9f49f014ee710 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81750 Reviewed-by: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
The extensions list in a certificate, CRL, and CRL entry is defined as: ... extensions [3] EXPLICIT Extensions OPTIONAL ... ... crlEntryExtensions Extensions OPTIONAL ... ... crlExtensions [0] EXPLICIT Extensions OPTIONAL ... Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension This means that a present but empty extensions list is actually invalid. Rather, if you have no extensions to encode, you are meant to omit the list altogether. Fix the delete_ext functions to handle this correctly. Bug: 442221114 Change-Id: I92af89d3e7120e06489359b3c6e2af499ecf5b85 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81751 Reviewed-by: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]>
This removes a bunch of .c_str() calls and is a bit tidier. Change-Id: Ib5ac52a1a6dc1522deaae354b9ad5ed8883bb1d3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81767 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Test that, signatures over unusual TBSCertificates are verified correctly. This tests that encoding is correctly round-tripped through the parser to the verifier. In principle, this should never happen because a DER parser will only accept the canonical encoding of an object. However, it is possible for encoding to not round-trip if we accept any BER inputs, or our in-memory representation does not capture the full range of abstract TBSCertificate values. X509 objects cache the encoded TBSCertificate, so all encoding variations should be captured. This test tries to exercise the cache's effects on signature verification. In reality, the cache is barely load-bearing. We now reject most non-DER inputs, and X509_NAME also saves its encoding. Still, the test ensures this remains the case. Amusingly, you're actually formally *not* supposed to do this. An X.509 signature is defined to use the DER encoding of the TBSCertificate. An ASN.1 type is independent of the particular encoding used. If you parsed a BER (or XER!) X.509 Certificate, you're meant to canonicalize the encoding back to DER to verify it. As far as I know, no one does it that way. Change-Id: I67c4932ec87d1f6fedfc8c711cf0f7837759947e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81768 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]>
DER being self-describing means we can actually programmatically find all the places to insert trailing data, even without access to the schema. This does not handle DER structures that are embedded inside OCTET STRINGs, like X.509 extensions. Those we'll need to write something else on top of this. Change-Id: Ic22a857d62417bd961cdc92cb8072bbe896bffb6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81769 Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
Not especially important, but I'm curious how the performance changes once d2i_X509 detaches from the ASN.1 templates. (Foreshadowing: individual fields don't do much, but once the whole TBSCertificate is handwritten, it helps!) Bug: 42290417 Change-Id: Icc5964faf7181aa707e9c682461f505afcefeea8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81770 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
The *_sign_ctx APIs implicitly call EVP_MD_CTX_cleanup on return, on both success and failure. Some callers rely on this to avoid a memory leak. An earlier iteration of this patch set accidentally broke this. Add a test so we catch this. Change-Id: I241b9161606f91adfbc37064304b815145703659 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81807 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
This is very minor in itself but the aim is to start setting up the patterns to rewrite the X509_CINF parser with CBS/CBB. In doing so, I'd like to directly embed some of the many child fields in X509. It saves some tiny mallocs and avoids worrying about whether the field is null. To support that, I've made the low-level parsing functions for crypto/asn1 look like "parse into" rather "parse and return new". The embed + parse-into pattern also avoids a thorny discrepancy between d2i_FOO and FOO_new: FOO_new currently fills in every required field, which is nice because it means, e.g., an X509 actually never has a null issuer. But if the parser first internally calls X509_new to construct the X509 and then fills it in, the inner X509_NAME parser will then have to throw it away and make a new one. As a result, we have this weird internal x509_new_null function. (tasn_dec.cc avoids this because internally it actually is a parse-into pattern, not a parse-new pattern. And then it just keeps allocating objects and checking for errors everywhere.) On the flip side, it means a bunch of types have to become embeddable within the library, instead of always allocated. That means adding internal init/cleanup functions. All this would be made a lot easier with C++ constructors and destructors, but there's a lot to work through there (I'm not thrilled about how we stomp ::~ssl_ctx_st() in libssl.) So, for now, I'm doing all this C-style. The immediate reason for revisiting that parser now is to thread EVP_PKEY_ALG parameters into the SPKI parser. The table-based parser is makes it very hard to add a new parameter. But I've also wanted to do this for some time anyway. Bug: 42290417 Change-Id: I628803992d5091333b9213db18736db2b9911cb0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81771 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
I had hoped to do this incrementally, but X509_ALGOR contains an ASN1_TYPE, so all types get imported at once. For now, these functions are just wired up to tasn_dec.cc, but subsequent changes will read them one-by-one. For the parse convention, I opted for a caller-supplied output variable, even though we've generally tried to avoid object reuse in the caller-exposed d2i functions. At the level of core ASN.1 types, the object reuse is hopefully manageable (and in practice I expect they'll always be in the empty state). The motivation here is to let us internally embed types into the X509, etc., structs and avoid the layers of tiny allocations. This does mean that the dispatch code inside ASN1_TYPE and the dispatch code in tasn_dec.cc is no longer shared, but that sharing wasn't valid by strict aliasing anyway. (Nothing in tasn_dec.cc is valid by strict aliasing.) Bug: 42290417 Change-Id: Idb6910e23f4fa4d01ee39ebcbf2ae09df14e87e3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81772 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
We open-coded a lot of these because we didn't have C++ templates in the beginning. Now we do and we can map calling conventions much more straightforwardly. Bug: 42290417 Change-Id: I7e5ba9e1bf2bd556bbff614b11ac15eb6e155f49 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81773 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Upstream OpenSSL has a similar function. This lets us copy X509_ALGOR values even when X509_ALGORs are embedded into their parent structs. Use it to implement X509_ALGOR_dup without going through ASN1_ITEM. Change-Id: I554a0ddb0eb2768c8bb02e52dc744d7ef267172f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81774 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
It would be tempting to move knowledge of the tag to tasn_dec.cc, so that the callbacks don't need to handle optionality, but this field cannot express all tags, and we may have to bridge a type that's a CHOICE. Instead, remove it, since we're not actually using it. (Although this macro is public, it is not possible to call it externally because we don't export ASN1_EXTERN_FUNCS.) Change-Id: I2adcc2a152b0f643e1d1a70e31510c8bdd432b3d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81775 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Having to keep flipping back and forth between the calling conventions when bridging rewritten types is tedious. This does not rewrite the core of tasn_dec.cc (although at this point much of it is already CBS-based), but it does suggest a calling convention for it. Right now, the internal calling convention is the double-pointer thing from d2i, with an extra twist that functions must return 0 for error, 1 for success, and -1 for "success but parsed nothing". This proposes a CBS in/out param to return how bytes we consumed, and a plain success/error return value. The -1 case can be modeled as successfully consuming zero bytes. For now, we still have to bridge the two inside tasn_dec.cc. Also for this CL I have not yet rewritten the messy X509_NAME parser, or the tasn_dec.cc parser. There's also a messy situation where this object sometimes is called with an existing object already there, and sometimes without one. I don't see a good way to avoid that one, but hopefully it'll become less and as important as we stop using the tasn system. Bug: 42290418 Change-Id: I3969e19ade81aedf449b4baf139fe5f5b1dd867b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81776 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
We do still need to give X509_ALGOR an ASN1_ITEM, both because the containing types have not been migrated, and because there are external callers that use <openssl/asn1t.h> and embed X509_ALGOR. https://crbug.com/42290417#comment2 has a list of those. This is a bit more boilerplate than I'd like. Some comes from just needing to define a bunch of functions. Some comes from not being able to use C++ ctors and dtors, which is a bit hard to avoid because X509_ALGOR is actually a public struct. Some is because defining an ASN1_ITEM involves a bit of code. I've hidden that last one behind a macro. (In the long run, I'd like to make the ASN1_ITEM callbacks CBS/CBB-based, but that's a bit tied up with rewriting tasn_enc.cc and tasn_dec.cc, which is in turn tied up with the X509_NAME parse callback, which depends on ASN1_item_ex_d2i and ASN1_item_ex_i2d.) Bug: 42290417 Change-Id: Ib0bfc9c942aa6bf784cf15af6b3747c9fc9a88f5 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81777 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
In doing so, embed it into the struct and save a small malloc. Bug: 42290417 Change-Id: Iaf6c679b577a27876b489d688c57c78a69d9130c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81778 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Leave some placeholders where we'll plumb in a list of supported algorithms for the saved EVP_PKEY. Bug: 42290417 Change-Id: I8a0f385afa5866465829e3bd6a7891263c714c5f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81779 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
In preparation for folding the structs together. Bug: 42290417 Change-Id: Idbad2d732ac6ea555b08ae8c352c2fb7f7c7ff31 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81780 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
In doing so, embed a whole host of types into X509. With all this, we can *finally* add extra parameters to the parser, like lists of supported algorithms or flag-protecting future changes. In the previous state, it went through a very messy reflection-based framework. Since we now control the parser entrypoint, this also means we can handle the cached encoding differently. Just to make the X509_parse_from_buffer and d2i_X509 flows more uniform, we now always save a CRYPTO_BUFFER containing the whole certificate, and then TBSCertificate marshalling finds the TBSCertificate from there. (Though it is still possible to create X509 objects without CRYPTO_BUFFERs.) That in turn means the generic ASN1_ENCODING path doesn't need to store a CRYPTO_BUFFER anymore, so all that is now unwound. If we rewrite the CSR and CRL parsers, then ASN1_ENCODING can be removed entirely. As a bonus, this new parser is significantly faster! Before: Did 1037634 Parse X.509 certificate operations in 5000690us (207498.2 ops/sec) After: Did 1318068 Parse X.509 certificate operations in 5000462us (263589.2 ops/sec) (The X509_NAME parser is still using tasn_dec, so there's still some overhead there. This also does not remove the overhead from X509's in-memory representation where lots of parts of the certificate get copied separate allocations.) Update-Note: This CL is not expected to change the external behavior of the X.509 parser, but it's a lot of code that had to get shuffled around, so if something funny happens around X.509, this is a likely culprit. Bug: 42290417 Change-Id: Ia4142f5e8d31b041176a545379c7df30e946a670 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81781 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Also test that putting an EVP_PKEY_RSA_PSS key in a certificate works... mostly. X509_get0_pubkey on the resulting object should work but does not yet. Later work will fix this. This code also should be reworked to not depend on the big OID table, but I've left that as a TODO for now. Bug: 42290364, 384818542 Change-Id: Ifd727cb06ed6c9d1501ef2ef155f4b3c66c8d488 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81782 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
This controls the algorithms that the SPKI will be parsed with. Note there is a decision point here with our API: is an X509 a holder of an EVP_PKEY, and thus parsed with algorithms in mind, or is it just an abstractly parsed certificate, and algorithms are instead passed to an X509_get_publickey_with_algorithms. This CL takes the first route, largely because upstream already went down this path in many ways: - X509_get0_pubkey expose the fact that X509's retain a cached EVP_PKEY. - Upstream OpenSSL added X509_new_ex which passes an OSSL_LIB_CTX into the X509. It's a little unfortunate that this pattern relies on object reuse in d2i_X509, but so it goes. - A caller using the same X509 to verify mutiple signatures (e.g. a root CA) might wish to hold on to the EVP_PKEY to avoid importing the key a bunch. (This probably doesn't matter too much, but if we ever add an API to precompute a table to speed up ECDSA verify...) This changes follows in those footsteps. Note this means that the same certificate bytes might produce semantically different X509 objects (with or without the SPKI in EVP_PKEY form) depending on how it was parsed. This is a little unfortunate but probably the most straightforward model given where we are. Bug: 42290364, 384818542 Change-Id: I8c0c95cb1d3221fc0b79a7749833c4f0d87646eb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81787 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
After the switch to C++, and https://boringssl-review.googlesource.com/c/boringssl/+/79569, we no longer depend on atomics being equivalently sized and aligned with their underlying types. Change-Id: I633c891a66539f7feaeab941a18172e8ac76dfb1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81827 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Not needed and deprecated [0]. This CL should be a no-op. [0]: https://chromium.googlesource.com/external/github.com/google/googletest.git/+/a05c0915074bcd1b82f232e081da9bb6c205c28d/googlemock/include/gmock/gmock-actions.h#2046 Bug: chromium:439838457 Change-Id: Ib93a80dbe2bb336a1e141422ca4a7de438e8ad4c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81907 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]> Auto-Submit: Jonathan Lee <[email protected]>
Change-Id: I3bc492831af3f4a619684a1294b04d721fbc27ab Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81927 Commit-Queue: Adam Langley <[email protected]> Reviewed-by: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]>
There is no reason for the list of supported groups for key exchange to
contain duplicates, because a group is either supported or not, and
specifying it multiple times has no additional effect. Also it is a
syntax error on the wire. Allowing duplicates in the SSL_CTX and
SSL_CONFIG members that track this list would create problems because we
will soon require other inputs (explicitly configured client key shares)
to be a subset of the supported groups. This CL requires the groups
configured via SSL_{,CTX_}set1_{groups,group_ids,groups_list} to be
unique.
Update-Note: The setters for supported groups,
SSL_{,CTX_}set1_{groups,group_ids,groups_list} will now fail if the
provided list of groups contains duplicates.
Change-Id: I67aa3bac811da6239e8f89fe501218a7a60dba11
Bug: 437414371
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81467
Auto-Submit: Lily Chen <[email protected]>
Reviewed-by: David Benjamin <[email protected]>
Commit-Queue: Lily Chen <[email protected]>
This seems to be a relatively common trick. https://crypto.stackexchange.com/a/47496 and https://bearssl.org/bigint.html#montgomery-reduction-and-multiplication were the clearest citations I could find. I modified it slightly. Normally you need n0 up to the precision of the word size you can easily multiply. We actually need double-word (64-bit) precision on 32-bit architectures, due to the x86 assembly. (32-bit x86 has 64-bit multiplication in SSE2. We currently compute a 64-bit n0 on all 32-bit platforms but just ignore the upper half on the others.) So we start by computing the 32-bit inverse, using uint32_t, then do one extra iteration in 64-bit. The recurrence can also be modified for the negative inverse, which lets us do the negation while we're still single-precision. This is also the technique described in https://en.wikipedia.org/wiki/Montgomery_modular_multiplication#Arithmetic_in_Montgomery_form as Hensel's lemma, but I'm having a hard time following the extremely generalized description there. Change-Id: I08580cdd09929cebca723570ef44068304d5ac43 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82027 Reviewed-by: Lily Chen <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
This is a bit more readable than the bit tricks, which compilers don't reliably detect anyway. Change-Id: I922976b5f5c3b0b8c3f02afd95f3d6783f6ffaf6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82047 Reviewed-by: Lily Chen <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Lily Chen <[email protected]>
It is not very difficult to get an ASN1_TYPE that is self-inconsistent, where the ASN1_TYPE-level type says one thing and the ASN1_STRING-level type says another. In particular, a not entirely unreasonable caller pattern used in Android does this when filling in X509_ALGOR parameters. The old ASN1_TYPE encoder took the ASN1_TYPE type, but the new one takes the ASN1_STRING type. Match the old behavior. We probably should also make ASN1_TYPE_set0 internally fix the type of the ASN1_STRING, but I haven't done that here. I think, either way, we should restore the old behavior of using the outer type. With the mess of exposed types and ambiguous APIs in OpenSSL, it's not too hard to manually construct something invalid here. Bug: 446993031 Change-Id: Idd5c0356769d8ff9a034222a9ffc107ce7c6f02e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82187 Reviewed-by: Lily Chen <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Although a pointless no-op, X509_set_issuer_name(cert, X509_get_issuer_name(cert)) used to work. https://boringssl-review.googlesource.com/c/boringssl/+/81894 broke this. Change-Id: I67c13ff9d4fe668f58fcf692d12131e269f3ab58 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82207 Commit-Queue: David Benjamin <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Lily Chen <[email protected]> Reviewed-by: Lily Chen <[email protected]>
Change-Id: If4c5603eada7c95154cfd5bd3156fbe972b720ba Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82147 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]>
…205 docs See fips202205::kSigAlgs. Change-Id: I1668ad9cb9207193face2a92a7782a59ae2438e1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82227 Reviewed-by: Lily Chen <[email protected]> Commit-Queue: David Benjamin <[email protected]> Commit-Queue: Lily Chen <[email protected]> Auto-Submit: David Benjamin <[email protected]>
This is practically true in the ecosystem, and even true for some of the calls within BoringSSL itself. Change-Id: I6841ec844b511ea759af64c9f507ab7a13b43225 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82247 Auto-Submit: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Change-Id: Ie07a1ad744a2eb6df3fc4d2b69d40222a74c5416 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82167 Auto-Submit: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]>
The bulk of these were fixed in https://boringssl-review.googlesource.com/c/boringssl/+/81947, but one was missed. This CL fixes the last one. Change-Id: Icb85f71aef807e0dd08b903d86365363ffa7970d Bug: 442860745 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82288 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> Auto-Submit: Lily Chen <[email protected]>
These all used to work, by virtue of us internally making a copy of the object before setting it. Now that we copy in-place, we need some extra checks. Change-Id: I5e0a7bf017cb4f4bb705a8776683b6f60bc66c4e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82287 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Lily Chen <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Change-Id: Ic2db9522098896b658d7b644f0cd2c5a85dd00d1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82309 Commit-Queue: David Benjamin <[email protected]> Commit-Queue: Adam Langley <[email protected]> Reviewed-by: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]>
This used to be tolerated by way of the general tasn_enc machinery. We don't support this, but one existing test was relying on this to force i2d_X509_NAME to fail and exercise that error path. I don't think it's actually possible, short of malloc failure, to construct an X509_NAME such that i2d_X509_NAME fails anymore. It's easier to just restore this artificial failure case than to figure out what, if anything, to do with their check. Change-Id: I49bd19e711518756c5ff20230dc4df6c56bf1977 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82310 Reviewed-by: Lily Chen <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Lily Chen <[email protected]>
Change-Id: I0799e3685e0d88848cf45b501bec6b9b9ecf43c2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82347 Commit-Queue: Adam Langley <[email protected]> Reviewed-by: David Benjamin <[email protected]> Auto-Submit: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
It is confusing to always have to explain that our API believes different notion of cipher suite ID than the IETF does. The only gap at this point is that we don't have constants defined. Define the constants for the cipher suites we implement, then deprecate all the old stuff. Switch ourselves internally to only pass the 16-bit IDs around, which avoids some confusing 0xffffs. The leading 0x03 can be pasted on at the legacy SSL_CIPHER_get_id function. The new constants were named to match the IANA names, minus the leading TLS_. This means a few constants don't match their CK values in name. There are TLS1_CK_ constants for a host of cipher suites we don't implement. I've left them alone for now, but I think we can remove them in a follow-up commit. Change-Id: Ifeb9cdceb351610fab7633c4068a8729a64ff11b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82307 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Lily Chen <[email protected]> Commit-Queue: Lily Chen <[email protected]>
QUICHE expects TLS1_CK_* to be defined in tls1.h, not ssl.h. For now, just define them in tls1.h as before. Ideally we'd define them in terms of the other constants, but we'd then either need to move SSL_CIPHER_* out of ssl.h or merge tls1.h and ssl3.h into ssl.h. The latter is probably a better direction, but is a much larger change, so start with this. This should unbreak the Chromium roll. Change-Id: Ic403bef7043028d30aa9916b403ebb3721734f32 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82387 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Lily Chen <[email protected]> Commit-Queue: Lily Chen <[email protected]>
Use the IANA identifier: https://www.iana.org/assignments/hpke/hpke.xhtml#hpke-kem-ids. It was originally proposed by https://datatracker.ietf.org/doc/draft-connolly-cfrg-hpke-mlkem/04/ which is now superceded by https://datatracker.ietf.org/doc/draft-ietf-hpke-pq/. Change-Id: Ifa387be8d347f845b398e0d7b68ad8b4649b409a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82327 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
See https://boringssl-review.googlesource.com/c/boringssl/+/82327/comment/5192b1ef_5bd31e64/ for more info. Change-Id: I1223c2b0f57ebc65b9fc5060a852a2f505a3c700 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82367 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Change-Id: Iba36dc1cf61792ba0fa733f7a15c3f925b3e8157 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82427 Reviewed-by: Lily Chen <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Lily Chen <[email protected]>
I figured that that this modulewrapper didn't need to bother exercising batch mode but the shared code implements buffering and so it has to advertise batch support so that acvptool will send flush commands. Change-Id: I5fdd973332b70692c7c0a0768994ee5708dc2aa0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82447 Reviewed-by: David Benjamin <[email protected]> Auto-Submit: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Similar to change-id: Ifa387be8d347f845b398e0d7b68ad8b4649b409a. Change-Id: Ie1e8c785440d6d5d95c03476553669694fc58615 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82428 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Most of the uses of constant_time_lt were unnecessary and can be simpler. constant_time_lt needs to perform extra operations because the inputs may use the full bit width of the input, but these values are known to be smaller. We largely only need to select by the MSB of some values. Add a constant_time_select_32 so we don't have to implicitly cast to/from int. Remove a comment about unary minus and MSVC. We do that throughout the codebase already. Finally, matching the ML-KEM implementation, remove a vectorization-impeding value barrier. This is... disappointing, but seems to be a significant performance difference. Like in ML-KEM, this was broadly okay except where we sample some value, where Clang was tempted too far into misbehaving. Apple M1 Pro, Clang: Before: Did 38824 MLDSA key generation operations in 4021430us (9654.3 ops/sec) Did 7950 MLDSA sign (randomized) operations in 4072620us (1952.1 ops/sec) Did 1163000 MLDSA parse (valid) public key operations in 4001540us (290638.1 ops/sec) Did 40320 MLDSA verify (valid signature) operations in 4024830us (10017.8 ops/sec) Did 40180 MLDSA verify (invalid signature) operations in 4009297us (10021.7 ops/sec) After: Did 48655 MLDSA key generation operations in 4020313us (12102.3 ops/sec) [+25.4%] Did 13361 MLDSA sign (randomized) operations in 4078864us (3275.7 ops/sec) [+67.8%] Did 1158000 MLDSA parse (valid) public key operations in 4000017us (289498.8 ops/sec) [-0.4%] Did 56000 MLDSA verify (valid signature) operations in 4051698us (13821.4 ops/sec) [+38.0%] Did 56000 MLDSA verify (invalid signature) operations in 4062468us (13784.7 ops/sec) [+37.5%] Intel(R) Xeon(R) Gold 6154 CPU @ 3.00GHz, GCC: Before: Did 17346 MLDSA key generation operations in 4019390us (4315.6 ops/sec) Did 3444 MLDSA sign (randomized) operations in 4066107us (847.0 ops/sec) Did 494000 MLDSA parse (valid) public key operations in 4004318us (123366.8 ops/sec) Did 16842 MLDSA verify (valid signature) operations in 4093079us (4114.8 ops/sec) Did 17220 MLDSA verify (invalid signature) operations in 4089998us (4210.3 ops/sec) After: Did 23058 MLDSA key generation operations in 4030723us (5720.6 ops/sec) [+32.6%] Did 6534 MLDSA sign (randomized) operations in 4061126us (1608.9 ops/sec) [+90.0%] Did 494000 MLDSA parse (valid) public key operations in 4002108us (123434.9 ops/sec) [+0.1%] Did 26180 MLDSA verify (valid signature) operations in 4045953us (6470.7 ops/sec) [+57.3%] Did 25800 MLDSA verify (invalid signature) operations in 4009973us (6434.0 ops/sec) [+52.8%] Intel(R) Xeon(R) Gold 6154 CPU @ 3.00GHz, Clang: Before: Did 17499 MLDSA key generation operations in 4059819us (4310.3 ops/sec) Did 3520 MLDSA sign (randomized) operations in 4070484us (864.8 ops/sec) Did 494000 MLDSA parse (valid) public key operations in 4003764us (123383.9 ops/sec) Did 16926 MLDSA verify (valid signature) operations in 4029917us (4200.1 ops/sec) Did 17220 MLDSA verify (invalid signature) operations in 4099146us (4200.9 ops/sec) After: Did 23104 MLDSA key generation operations in 4036297us (5724.1 ops/sec) [+32.8%] Did 6336 MLDSA sign (randomized) operations in 4006447us (1581.5 ops/sec) [+82.9%] Did 494000 MLDSA parse (valid) public key operations in 4005244us (123338.3 ops/sec) [-0.0%] Did 26460 MLDSA verify (valid signature) operations in 4081059us (6483.6 ops/sec) [+54.4%] Did 26120 MLDSA verify (invalid signature) operations in 4021846us (6494.5 ops/sec) [+54.6%] Change-Id: I9f010ca1dde37a306e4a207caa12ec4feb920716 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82527 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Adam Langley <[email protected]>
Change-Id: I911d6a1b8430aba57dd8ff74fd0b6fc4e9a5ea37 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82507 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: David Benjamin <[email protected]> Auto-Submit: Adam Langley <[email protected]>
Change-Id: I4cec91ceadf1be7413085f56c2f9efaa18fc8ca2 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82547 Reviewed-by: Lily Chen <[email protected]> Commit-Queue: Lily Chen <[email protected]> Auto-Submit: David Benjamin <[email protected]>
Change-Id: I3695a459ce608478c3c7d604c117410dbce61ed1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82548 Commit-Queue: Lily Chen <[email protected]> Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Lily Chen <[email protected]>
This change introduces a new mechanism for clients to provide a "hint" about which key exchange groups the server is likely to support. This is meant to be used for draft-ietf-tls-key-share-prediction. We will try to make a prediction based on the server's hint, and if a group was predicted, it is sent as the key_share, overriding any explicit key shares configured by the caller. Bug: 437414371 Change-Id: Ie98d2a63e5bfb4f10683ca4f1f77e5439b39e256 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81588 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Lily Chen <[email protected]>
Change-Id: Ia3dffb0dc0f0dd41357a52bdd1327d03a4087eee Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82587 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> Auto-Submit: Lily Chen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
@dstebila To fix the squash commit issue, I directly renamed pi-20250831 from #136 to main. This makes the tree cleaner.