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>
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>
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>
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>
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>
(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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
This is mostly to confirm the STACK_OF(ASN1_TYPE) was created the right
number of times.
Change-Id: I30c32f91cb6091e63bfcaebb0fe966270e503d93
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46984
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
b571e77773 changed these fd numbers, but that interacts poorly with
cross-version tests. Instead, remove the assumptions StartProcess() was
making about the relationship between the two sets of fds.
Change-Id: If8fe62e4d20d22776e79e05e82cb5920cbb545ec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47044
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The X509_ATTRIBUTE structure includes a hack to tolerate malformed
attributes that encode the value directly instead of a set of values.
This form is never created by OpenSSL and shouldn't be needed any more.
(Imported from upstream's e20b57270dece66ce2c68aeb5d14dd6d9f3c5d68.)
This also changes X509_ATTRIBUTE_set1_data slightly. Previously,
set1_data would override whatever was previously in the X509_ATTRIBUTE,
but leak memory. Now set1_data appends to the set. (PKCS#10 attributes
use SET OF ANY as value.) It's unclear to me if this was intentional on
upstream's part. (The attrtype == 0 case only makes sense in the old
behavior.) Since there is no other way to create a two-element SET and
upstream has long since released this behavior, I left it matching
upstream.
Update-Note: Given OpenSSL hasn't accepted these for five years, it's
unlikely anything depends on it. If something breaks, we can revert this
and revisit. No one calls X509_ATTRIBUTE_set1_data on a non-empty
X509_ATTRIBUTE, so the behavior change there should be safe.
Change-Id: Ic03c793b7d42784072ec0d9a7b6424aecc738632
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46947
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
x509_req.c changes imported from upstream's
9b0a453190efc9b14cc04e74ce2e8e35af45fb39.
Update-Note: Direct accesses of X509_ATTRIBUTE should be replaced with
one of the accessors. I couldn't find any direct accesses, so hopefully
this is fine.
Change-Id: I7eab6375d5dcf366ef72e5ce059f3558c947f35b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46946
Reviewed-by: Adam Langley <agl@google.com>
I stopped short of documenting the add1_attr_by functions because the
type parameter is a bit of a mess. It appears to be several enums put
together. To that end, I've updated the documentation on
V_ASN1_MAX_UNIVERSAL to note that we also need to avoid MBSTRING_FLAG.
As a preview of what I'm putting off to later, see
X509_ATTRIBUTE_set1_data for how the type parameter is used. set1_data
is extra fun because PKCS#10 attributes are set-valued. Plus there's
upstream's e20b57270dece66ce2c68aeb5d14dd6d9f3c5d68, which we should
import first.
Change-Id: I3453a0b224e42c6e22828c7d332ee133e09e6173
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46945
Reviewed-by: Adam Langley <agl@google.com>
Update-Note: Direct accesses of X509_PUBKEY should be replaced with one
of the accessors. I believe all callers have been fixed at this point.
Change-Id: Ib325782867478fb548da1bf5ef0023cf989f125b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46944
Reviewed-by: Adam Langley <agl@google.com>
The ASN1_BOOLEAN representation is a mess. ASN1_BOOLEAN is an int
and if non-negative (negative values mean omitted or default), gets cast
to uint8_t and encoded as the value. This means callers are simply
expected to know true is 0xff, not 1. Fix this by only encoding 0 or
0xff.
This also fixes a bug where values like 0x100 are interpreted as true
(e.g. in the tasn_enc.c logic to handle default values), but encoded as
false because the cast only looks at the least significant byte.
This CL does not change the parsing behavior, which is to allow any BER
encoding and preserve the value in the in-memory representation (though
we should tighten that). However the BER encode will no longer be
preserved when re-encoding.
Update-Note: Callers setting ASN1_BOOLEANs to a positive value other
than 0xff will now encode 0xff. This probably fixes a bug, but if anyone
was attaching significance to incorrectly-encoded booleans, that will
break.
Change-Id: I5bb53e068d5900daca07299a27c0551e78ffa91d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46924
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We have a few too many X.509 parsers.
Bug: chromium:1199744
Change-Id: Ib6f6b7bf6059ed542c334a5ca5a2d3928aae3bef
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46904
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
See go/handshake-hints (internal).
CL originally by Bin Wu <wub@google.com>. I just reworked the tests and
tidied it up a bit. This is the start of a replacement for the split
handshakes API. For now, only TLS 1.3 is supported. It starts with an
initial set of hints, but we can add more later. (In particular, we
should probably apply the remote handshaker's extension order to avoid
needing to capability protect such changes.)
Change-Id: I7b6a6dfaa84c6c6e3436d2a4026c3652b8a79f0f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46535
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
They still need to be Python-2-compatible until I figure out how to
switch the version used in the CI.
I've left out make_curve25519_tables.py because it's some bytes vs
unicode mess I don't care to figure out. We should just rewrite that in
Go which should also be much faster anyway.
Change-Id: I4446641815315a84c2979b1be1e1949f88cbacf8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46884
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
It's defined as GCM without a plaintext input, so this is trivial.
Change-Id: Id430e998447e489c2bf2dd5c6541877870104ade
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46844
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Also generate a corpus to unblock the Chromium roll. The build tools
expect there to be a corresponding directory somewhere.
Bug: 275
Change-Id: I7a061ba6625ec57c10b0ae17e68b6b0159c539d4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46826
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
'corpora' seems to be more common than 'corpuses' in Chromium code
search, including in libFuzzer's source itself.
Change-Id: I6489b57a4608f47274c4400aac135cbfb991953a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46825
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The gclient-managed files usually go in .gitignore. I think without it,
we have to redownload it all the time on the bots? Though this also
makes my git status cleaner.
Change-Id: Ic9bac6796bd3dcdea5039bcac39e853d9f12906b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46824
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
See also 86a90dc749af91f8a7b8da6628c9ffca2bae3009 from upstream. This
differs from upstream's which treats {NULL, 2} as a valid way to spell
the empty list. (I think this is a mistake and have asked them about
it.)
Upstream's CL also, for them, newly makes the empty list disable ALPN,
when previously they'd disable it but misread it as a malloc failure.
For us, we'd already fixed the misreading due to our switch to
bssl::Array and bssl::Span, but the documentation was odd. This CL
preserves that behavior, but updates the documentation and writes a
test.
Update-Note: SSL_CTX_set_alpn_protos and SSL_set_alpn_protos will now
reject invalud inputs. Previously, they would accept them, but silently
send an invalid ALPN extension which the server would almost certainly
error on.
Change-Id: Id5830b2d8c3a5cee4712878fe92ee350c4914367
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46804
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This also fixes a minor bug (that doesn't matter because we don't
implement DTLS 1.3). init_message must be paired with finish_message to
correctly handle the DTLS header.
Change-Id: I4b65c82d4b691d5b77d9e20513983145098d6f8f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46785
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We currently determine whether we need HelloRetryRequest at the same
time as resolving key share machinery. That is a little too late for
early data negotiation, so we end up accepting early data and then
clearing it later on in the function. This works but is easy to mess up,
given the preceding CL. There's also some ALPS logic that got this
wrong, but I believe it didn't result in any incorrect behavior.
Instead, this pulls secret computation out of the key_share helper
function, which now just finds the matching key share. We then check
early whether we need HRR, before deciding on early data.
Change-Id: I108865da08addfefed4a7db73c60e11cf4335093
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46765
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
TLS 1.3 servers should only skip early data if the client offered it.
Our HRR codepath didn't quite get this right. This CL is the minimal fix
for this issue, but I think we should rearrange this logic slightly
rather than deciding to do 0-RTT and then changing our mind. The next CL
will do that.
(This bug does not have any interoperability consequences. When
configured to skip early data, we're happy to vacuously skip over zero
early data records. We were just less strict than we should be.)
Change-Id: Ida42134b92b4df708b2bb959c536580bec454165
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46764
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: Ib2356f1a6e6ef8bfd5b5469eae9d1bc43dd40895
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46724
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Rather than computing kVarianceBlocks, which is hard to reason about,
use a sha1_final_with_secret_suffix abstraction. This lets us separate
reasoning in bytes about the minimum and maximum values of |data_size|
and the interaction with HMAC, separately from the core constant-time
SHA-1 update.
It's also faster. I'm guessing it's the more accurate block counts.
Before:
Did 866000 AES-128-CBC-SHA1 (16 bytes) open operations in 2000697us (6.9 MB/sec)
Did 616000 AES-128-CBC-SHA1 (256 bytes) open operations in 2001403us (78.8 MB/sec)
Did 432000 AES-128-CBC-SHA1 (1350 bytes) open operations in 2003898us (291.0 MB/sec)
Did 148000 AES-128-CBC-SHA1 (8192 bytes) open operations in 2006042us (604.4 MB/sec)
Did 83000 AES-128-CBC-SHA1 (16384 bytes) open operations in 2010885us (676.3 MB/sec)
After:
Did 2089000 AES-128-CBC-SHA1 (16 bytes) open operations in 2000049us (16.7 MB/sec) [+141.3%]
Did 851000 AES-128-CBC-SHA1 (256 bytes) open operations in 2000034us (108.9 MB/sec) [+38.2%]
Did 553000 AES-128-CBC-SHA1 (1350 bytes) open operations in 2002169us (372.9 MB/sec) [+28.1%]
Did 178000 AES-128-CBC-SHA1 (8192 bytes) open operations in 2008596us (726.0 MB/sec) [+20.1%]
Did 98000 AES-128-CBC-SHA1 (16384 bytes) open operations in 2001509us (802.2 MB/sec) [+18.6%]
Confirmed with valgrind tooling that this is still constant-time. In
doing so, I ran into a new nuisance with GCC. In loops where we run
constant_time_lt with a counter value, GCC sometimes offsets the loop
counter by the secret. It cancels it out before dereferencing memory,
etc., but valgrind does not know that x + uninit - uninit = x and gets
upset. I've worked around this with a barrier for now.
Change-Id: Ieff8d2cad1b56c07999002e67ce4e6d6aa59e0d3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46686
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This CL adds an initial implementation of the ECH server, with pieces of
the client in BoGo as necessary for testing. In particular, the server
supports ClientHelloInner compression with ech_outer_extensions. When
ECH decryption fails, it can send retry_configs back to the client.
This server passes the "ech-accept" and "ech-reject" test cases in
tls-interop-runner[0] when tested against both the cloudflare-go and nss
clients. For reproducibility, I started with the main branch at commit
707604c262d8bcf3e944ed1d5a675077304732ce and updated the endpoint's
script to pass the server's ECHConfig and private key to the boringssl
tool.
Follow-up CLs will update HPKE to the latest draft and catch us up to
draft-10.
[0]: https://github.com/xvzcf/tls-interop-runner
Bug: 275
Change-Id: I49be35af46d1fd5dd9c62252f07d0bae179381ab
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45285
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We currently construct finishedHash fairly late, after we've resolved
HelloRetryRequest. As a result, we need to defer some of the transcript
operations across a large chunk of code.
This is a remnant of earlier iterations of TLS 1.3, when
HelloRetryRequest didn't tell us the cipher suite yet. Now the cipher
suite is known earlier and we can construct the finishedHash object
immediately. In doing so, move HRR handling inside doTLS13Handshake().
This keeps more of TLS 1.3 bits together and allows us to maintain the
HRR bits of the handshake closer to the rest of HRR processing. This
will be useful for ECH which complicates this part of the process with
an inner and outer ClientHello. Finally, this adds a missing check that
the HRR and SH cipher suites match.
Change-Id: Iec149eb5c648973325b190f8a0622c9196bf3a29
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46630
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Adam Langley
Steven Valdez
Piotr Sikora
Gurleen Grewal
Dan McArdle