There was even a bug where we returned -1 out of a function whose
callers use !.
Change-Id: Ic815ea9f013bcbca41ef84b9ffa9fb867f716aa6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58465
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Passes test vectors, and should be constant time, but is currently
not optimized and neither the API nor the standard is stable.
Change-Id: I89b90877e023a43ee7238e11b86065444ab3bdec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57845
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This is a double-pointer and both layers should be const. This matches
OpenSSL 1.1.1, so in addition to being more const-correct, we're more
OpenSSL-compatible.
Update-Note: Anything that defines a comparison function would need to
fix the type signature. I found only one external caller, Envoy, that
defines it. https://github.com/envoyproxy/envoy/pull/25051 fixes it.
(That we hadn't run into the upstream incompatibility suggests this is
just not a feature folks use outside the library much.)
Bumping BORINGSSL_API_VERSION, in case I missed any, and there's some
caller where we can't just use C++14 generic lambdas to smooth it over.
Fixed: 498
Change-Id: I8f07ff42215172aa65ad8819acf69b63d6d8e54c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56190
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
TLS 1.2 ECDHE and TLS 1.3 key shares were originally designed around
Diffie-Hellman-like primitives and use language based on that.
Post-quantum replacements do not look like Diffie-Hellman, where each
part exchanges a public key, but schemes that work differently can still
slot in without protocol changes.
We previously came up with our own Offer/Accept/Finish abstraction for
early post-quantum experiments, but the NIST constructions are all
expressed as KEMs: First, the recipient generates a keypair and sends
the public key. Then the sender encapsulates a symmetric secret and
sends the ciphertext. Finally, the recipient decapsulates the ciphertext
to get the secret.
Align our C++ and Go abstractions to this terminology. The functions are
now called Generate/Encap/Decap, and the output of Encap is called
"ciphertext", which seems to align with what most folks use. (RFC 9180
uses "enc" for "encapsulated key", but they staple a KEM to an AEAD, so
"ciphertext" would be ambiguous.)
Where variable names refer to parts of the protocol, rather than the
the underlying KEM-like construction, I've kept variable names matching
the protocol mechanism, so we still talk about "curves" and "key
shares", but, when using the post-quantum replacements, the terminology
is no longer quite accurate.
I've also not yet renamed SSLKeyShare yet, though the name is now
inaccurate. Also ideally we'd touch that up so the stateful object is
just a KEM private key, for SSLKEMKey. Though at that point, we maybe
should just add EVP_KEM and EVP_KEM_KEY APIs to libcrypto.
Change-Id: Icbcc1840c5d2dfad210ef4caad2a7c4bf8146553
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57726
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Since this was written, we've tidied up the EC code a bit:
1. While not quite yet infallible (but we should get there), the output
of EC_GROUP_new_by_curve_name no longer needs to be freed.
2. BN_CTX no longer does anything in EC code, so just pass in NULL.
We really should build a real ECDH API, but for now just improve our use
of the current thing.
Change-Id: I44f5429afec06c28372ae70148eb8de263d716f3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57626
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We've got two layers of serialization. There's the lower-level
SerializePrivateKey/DeserializePrivateKey functions that just encode a
private key assuming you already know the group, and then there's
Serialize/Create which output an INTEGER and OCTET STRING pair.
The latter is only used by handoff.cc, so move them there. This trims
the SSLKeyShare abstraction slightly.
Change-Id: I1c901d7c16b082bfe1b6acd0a1711575e7f95c05
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57625
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We call Accept as a TLS 1.2 client and a TLS 1.3 server. In the latter,
we create an SSLKeyShare object, Accept, and immediately destroy it. In
the former, we create the SSLKeyShare object a couple steps before
actually using it.
It's equivalent to create the object just before Accept, so switch to
that. This change means that hs->key_shares now only ever contains
objects in between Offer and Finish. Or, in KEM terms, it only ever
contains KEM private keys. (SSLKeyShare objects are currently a little
confused about what kind of state they contain.)
Change-Id: Idec62ac298785f784485bc9065f7647034d2a607
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57605
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
While hiding 'type' isn't such a huge deal, accessing 'pkey' without a
type check is very dangerous. The accessors are type-checked and avoid
this problem. It also gets us slightly closer to not needing to utter
CRYPTO_refcount_t in public headers, as we're currently not quite
declaring it right. And it allows us to remove another union:
https://boringssl-review.googlesource.com/c/boringssl/+/57106
This matches what upstream did in OpenSSL 1.1.0.
Update-Note: Code that reaches into the EVP_PKEY struct will no longer
compile, like in OpenSSL. I believe I've fixed all the cases. If I
missed any, the fix is to switch code to accessors. EVP_PKEY_id(pkey)
for pkey->type is the most common fix.
Change-Id: Ibe8d6b6cb8fbd141ea1cef0d02dc1ae3703e9469
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57105
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Prior to 3.12 (which we won't be requiring until July), OBJECT libraries
cannot be used with target_link_libraries. That means they cannot pick
up INTERFACE_INCLUDE_DIRECTORIES, which makes them pretty unusable in
the "modern CMake" style.
Just switch it to a static library to unbreak the build in CMake 3.10.
For some link ordering reason I don't understand, this also requires
explicitly linking boringssl_gtest to libcxx when we USE_CUSTOM_LIBCXX
is set.
Change-Id: Ia9d8351551f5da060248aa3ca73fe04473bf62aa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57345
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
This was added with the generated symbol-prefixing header. But it
seems to be sufficient for crypto to have a dependency on the
generated header, along with some of the stray bits of delocate.
It's a little unclear from CMake documentation how these are processed;
normally .o files can be built before libraries are built or linked,
only the link step depends on.
But, empirically, if A links B, and B has a dependency on C, then CMake
seems to run C before building any of A. I tested this by making a small
project where the generation step slept for three seconds and running
with enough parallelism that we'd have tripped.
Interestingly, in the Makefile output, the individual object file
targets didn't have that dependency, but the target itself did. But this
was true on both A and B, so I think that just might not work.
Also fix the dependency in the custom target. The old formulation broke
when using an absolute path to the symbols file.
Change-Id: I2053d44949f907d465da403a5ec69c191740268f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56928
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Also stick the very verbose default install directory in a variable so
we don't have to repeat it everywhere.
Change-Id: I1a6a85c4d42d3a6e766e52b2d0ecd8e81c6ed4e3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56607
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
It's unclear to me whether doing it target-by-target is an improvement
in crypto/fipsmodule, but this otherwise does seem a bit tidier. This
aligns with CMake's documentation and "modern CMake" which prefers this
pattern.
Change-Id: I36c81842bff8b36eeaaf5dd3e0695fb45f3376c9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56585
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Remove all the other ERR_R_MALLOC_FAILURES from the
codebase.
Also changes cbb to push to the error stack, to correctly
report cbb failures instead of now only reporting
malloc failures. Previously it turned all cbb failures
into a malloc failure
Bug: 564
Change-Id: Ic13208bf9d9aaa470e83b2f15782fc94946bbc7b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57046
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The ws2_32 dependency comes from BIO, which is in libcrypto. Once it's
added there, it should get inherited by anything downstream, so we don't
need to keep listing it.
We also no longer need -lrt. We tried to remove it in
https://boringssl-review.googlesource.com/4622, but had to revert it in
https://boringssl-review.googlesource.com/4894 because of clock_gettime.
clock_gettime, per the Linux man page, is now in libc, not librt, as of
glibc 2.17. THat was released December 2012, well past our five year
watermark, so clean this part of the build up.
Change-Id: Ie6a07434b0cb02fe916b32ab8c326ec33d40bcb6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56606
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Caught by running malloc failure tests on unit tests.
Bug: 563
Change-Id: Ic0167ef346a282dc8b5a26a1cedafced7fef9ed0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56927
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Rather than trying to override the actual malloc symbol, just intercept
OPENSSL_malloc and gate it on a build flag. (When we first wrote these,
OPENSSL_malloc was just an alias for malloc.)
This has several benefits:
- This is cross platform. We don't interfere with sanitizers or the
libc, or have to mess with global symbols.
- This removes the reason bssl_shim and handshaker linked
test_support_lib, so we can fix the tes_support_lib / gtest
dependency.
- If we ever reduce the scope of fallible mallocs, we'll want to
constrain the tests to only the ones that are fallible. An
interception strategy like this can do it. Hopefully that will also
take less time to run in the future.
Also fix the ssl malloc failure tests, as they haven't been working for
a while. (Malloc failure tests still take far too long to run to the end
though. My immediate motivation is less malloc failure and more to tidy
up the build.)
Bug: 563
Change-Id: I32165b8ecbebfdcfde26964e06a404762edd28e3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56925
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
and isxdigit.
All of these can be affected by locale, and although we weren't using
them directly (except for isxdigit) we instead had manual versions inline
everywhere.
While I am here add OPENSSL_fromxdigit and deduplicate a bunch of code
in hex decoders pulling out a hex value.
Change-Id: Ie75a4fba0f043208c50b0bb14174516462c89673
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56648
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
While information is contradictory on this subject, investigation
of several implementaions and Posix appears to indicate that it
is possible to change the behaviour of isdigit() with locale.
Change-Id: I6ba9ecbb5563d04d41c54dd071e86b2354483f77
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56625
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Template operator() instead of the type. This fixes converting
subclasses with bssl::UniquePtr. std::unique_ptr<T, D> can be converted
to std::unique_ptr<U, E> requires either D == E or for D to be
implicitly convertable to E, along with other conditions. (Notably T*
must be convertible to U*.)
In the real std::unique_ptr, we rely on std::default_delete<T> being
convertable to std::default_delete<U> if T* is convertible to U*. But
rather than write all the SFINAE complexity, I think it suffices to
move the template down a later. This simplifies SSLKeyShare::Create a
little.
Change-Id: I431610f3a69a72dd9def190d3554c89c2d3a4c32
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56385
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
More than one post-quantum group is now defined so it would be possible
for two PQ groups to be 1st and 2nd preferences. In that case, we
probably don't want to send two PQ initial key shares.
(Only one PQ group is _implemented_ currently, so we can't write a test
for this.)
Change-Id: I51ff118f224153e09a0c3ee8b142aebb6b340dcb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56226
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
There is no Kyber implementation in BoringSSL so these stubs assume that
you are locally patching such an implementation in.
Change-Id: I274b9a93e60f0eb74301c8d58f05c235687643e1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55930
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This aligns with upstream OpenSSL, so it's hopefully more compatible.
Code search says no one outside of the project uses this function, so
it's unlikely to break anyone.
Whether it makes things better is a bit of a wash: OBJ_dup and
OPENSSL_strdup loose a pointless wrapper. X509_NAME_dup gains one, but
hopefully that can be resolved once we solve the X509_NAME
const-correctness problem. CRYPTO_BUFFER_up_ref gains one... really
FOO_up_ref should have type const T * -> T *, but OpenSSL decided it
returns int, so we've got to cast.
Change-Id: Ifa6eaf26777ac7239db6021fc1eafcaed98e42c4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56032
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We've been setting /Wall on MSVC for a while, but /Wall in MSVC is
really "all". I.e., it's -Weverything in Clang and GCC, and includes
many warnings that are simply diagnostics. MSVC's own headers do not
promise to be clean under /Wall.
Rather, the equivalent of Clang and GCC's -Wall is /W4, which MSVC does
promise to pass. Under /Wall, every new MSVC release we've had to update
an ever-growing list of warning exceptions, to disable the
off-by-default warnings that were off for a reason. Switch to MSVC's
recommendations and just enable /W4.
From there, I've trimmed the exception list, now that we don't need to
re-disable disabled warnings. A few non-disabled warnings also no longer
need exceptions.
This should fix the build with VS2022, which was failing due to C5264.
C5264 flags a couple of instances in the library, but tons and tons in
MSVC headers and googletest. (The instances in the library are a little
goofy and reflect things that should have been 'inline constexpr', but
we can't rely on C++17 yet. Though I may go add a compat macro for
that.)
Fixed: 552
Change-Id: I9031c0d5bd4c7a4df1dc3040b38af9cfbcffc06e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56045
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
I did this because I was tired of explaining Grover's algorithm and
circuit depth, but it never large amounts of sense and it conflates any
measurements of post-quantum impact. If you want to configure a server
with a preference for 256-bit ciphers, that's still completely possible.
Change-Id: I3dc951ec724a713bb4da75c204d1105c62de8d74
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55929
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This function reports when security-critical checks on the X.509 key
usage extension would have failed, but were skipped due to the temporary
exception in SSL_set_enforce_rsa_key_usage. This function is meant to
aid deployments as they work through enabling this.
Change-Id: Ice0359879c0a6cbe55bf0cb81a63685506883123
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55465
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
That test cert expires in 2099, which is a ways off but if this code is
somehow still around by then, let's save the future some pain. With this
fixed, our test all pass at least through the year 3000, so we're
hopefully clear of timebombs.
Change-Id: Ie9dcbc4f4db70c6bcc1ae9717c6e1ee89eb4195c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55625
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Bug: b:261632678, chromium:1396479
Change-Id: I82f7ce05ece8b5c145d4394dc0d4173e357ce176
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55585
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
BIO_ctrl is one of OpenSSL's ioctl-style patterns, where they jam many
different function signatures into one type. BIO_ctrl returns long for
the sake of other operations, but many of them are only allowed to
return int.
Bug: 516
Change-Id: Ieffad1da89c60a538f142b12bdebdb950efd5c6a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55454
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
It should not be possible to make BoringSSL request unknown signature
algorithms, or the special SSL_SIGN_RSA_PKCS1_MD5_SHA1 value, in the
ClientHello or CertificateRequest.
Update-Note: This CL makes unknown values fail
SSL_set_verify_algorithm_prefs, etc. SSL_SIGN_RSA_PKCS1_MD5_SHA1 is
silently dropped from the list, rather than an error because, although
documented as incorrect, this hole in the abstraction seems to be
confusing. I think there's some code in Chromium which accidentally puts
it in the signing prefs (wrong but harmless) and I often need to explain
to folks that it doesn't belowing in verify prefs (puts it in the
ClientHello). This makes us tolerate the value by ignoring it.
This makes the previous pkey_supports_algorithm change moot because we'd
never get that far with SSL_SIGN_RSA_PKCS1_MD5_SHA1, but I think the
check, but I think the check belongs in that function too.
The test also reveals that some of our tests have been accidentally
passing zero into the preference list all this time.
Change-Id: I76d4eb98682515c3b819e0ed8d44f2d708a98975
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55446
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
SSL_SIGN_RSA_PKCS1_MD5_SHA1 does not really exist, but is a private use
value we allocated to internally represent the TLS 1.0/1.1 RSA signature
algorithm. (Unlike the TLS 1.0/1.1 ECDSA signature algorithm, which is
the same as SSL_SIGN_ECDSA_SHA1, the RSA one is a bespoke MD5+SHA1
concatenation which never appears in TLS 1.2 and up.)
Although documented that you're not to use it with
SSL_CTX_set_verify_algorithm_prefs and
SSL_CTX_set_signing_algorithm_prefs (it only exists for
SSL_PRIVATE_KEY_METHOD), there's nothing stopping a caller from passing
it in.
Were you to do so anyway, we'd get confused and sign or verify it at TLS
1.2. This CL is the first half of a fix: since we already have
pkey_supports_algorithm that checks a (version, sigalg, key) tuple, that
function should just know this is not a 1.2-compatible algorithm.
A subsequent CL will also fix those APIs to not accept invalid values
from the caller, since these invalid calls will still, e.g., dump
garbage values on the wire.
Change-Id: I119503f9742a17952ed08e5815fb3d1419fd4a12
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55445
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
ioutil has been deprecated since Go 1.16. The functions were moved to
some combination of io and os. See https://pkg.go.dev/io/ioutil.
(File-related functions went to os. Generic things went to io. Names
were kept the same except TempDir and TempFile are os.MkdirTemp and
os.CreateTemp, respectively.)
Change-Id: I031306f69e70424841df08f64fa9d90f31780928
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55186
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
We use unsigned, but we actually assume it is 32-bit for the bit-packing
strategy. But also introduce a typedef to hint that callers shouldn't
treat it as an arbitrary 32-bit integer. A typedef would also allow us
to extend to uint64_t in the future, if we ever need to.
Update-Note: Some APIs switch from unsigned * to uint32_t * out
pointers. This is only source-compatible if unsigned and uint32_t are
the exact same type. The CQ suggests this is indeed true. If they are
not, replace unsigned with CBS_ASN1_TAG to fix the build.
Bug: 525
Change-Id: I45cbe127c1aa252f5f6a169dca2e44d1e6e1d669
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54986
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
I had a rewrite of the decrepit ciphers (CAST and Blowfish) to use
CRYPTO_{load,store}_u32_be and drop the old macros, but this is probably
not worth the effort to review. Instead, just fix the type in the macro.
Bug: 516
Change-Id: I1cdecc16f6108a6235f90cf9c2198bc797c6716e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54985
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
SSL_export_keying_material can only be used when the exporter secret is
available, e.g. during False Start (TLS 1.2) and on the server when
processing 0-RTT (TLS 1.3). These conditions were special cased, but
there is at least one more case in TLS 1.3 where the exporter secret is
available. This change switches the logic for TLS 1.3 to check whether
the exporter secret has been derived and makes
SSL_export_keying_material functional if it has, instead of checking if
the handshake is in one of some number of specified states.
Allowing the availability of the exporter in TLS 1.3 on the server after
processing the client's handshake flight and sending the server Finished
is equivalent to the already-allowed case of exposing the exporter in
TLS 1.2 False Start.
Bug: b:255591447
Change-Id: Ib216fd4a676524a777aae17569161c02dd2e40ca
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55025
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Nick Harper <nharper@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
This reverts commit 64393b57e8. We'll
reland this change in January. Projects that rely on this revert should
use SSL_set_enforce_rsa_key_usage, available since 2019, to control the
security check without being reliant on the defaults.
Bug: 519
Change-Id: Icf53eae8c29f316c7df4ec1a7c16626ac3af8560
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55005
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Bug: 516
Change-Id: Iba2014da414658c08e42e0993912fa73848832d3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54945
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
SSL_generate_key_block is specific to TLS 1.2. It will output garbage in
TLS 1.3 (wrong KDF), so fail instead.
Update-Note: SSL_generate_key_block gets a new error case, but callers
that hit this were getting back useless output anyway.
Change-Id: Ib35384f902e03cd4654d25b39ca1808c4d878c3d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54705
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Update-Note: Clients will now require RSA server certificates used in
TLS 1.2 and earlier to include the keyEncipherment or digitalSignature
bit. keyEncipherment is required if using RSA key exchange.
digitalSignature is required if using ECDHE_RSA key exchange.
We already required this for each of ECDSA, TLS 1.3, and servers
verifying client certificates, so this just fills in the remaining hole.
Chrome has also enforced this for some time with publicly-trusted
certificates. For now, the SSL_set_enforce_rsa_key_usage API still
exists where we need to turn this off.
Fixed: 519
Change-Id: Ia440b00b60a224fa608702439aa120d633d81ddc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54606
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We never ended up using these, or making them work with TLS 1.3 (which
has KeyUpdates and NewSessionTickets). It'd still be nice to have an
in-place API, but for now unwind these ones until we have time to give
it another go. Supporting TLS 1.3's post-handshake messages will
probably require a slightly more involved design.
(I suspect some of the seal_scatter bits in tls_record.cc can also be
simplified with these removed, but I've left them alone here.)
Update-Note: Removed some unused, experimental APIs.
Change-Id: Iad1245fa467cc6e599d20561f5db44d236219e06
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54527
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We spend a lot of effort implementing a big-endian sequence number
update, etc., when the sequence number is just a 64-bit counter. (Or
48-bit counter in DTLS because we currently retain the epoch
separately. We can probably tidy that a bit too, but I'll leave that
for later. Right now the DTLS record layer state is a bit entwined
with the TLS one.)
Just store it as uint64_t. This should also simplify
https://boringssl-review.googlesource.com/c/boringssl/+/54325 a little.
Change-Id: I95233f924a660bc523b21496fdc9211055b75073
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54505
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The destructor is automatic but, as a bonus, it becomes size_t-clean.
Costs us 8 more bytes of per-connection memory per outgoing message,
which isn't ideal but the previous commit saved even more, and DTLS
isn't as important as TLS in that regard.
Bug: 516
Change-Id: I69f881169088a11b9f09c4dc3577c47c4b48ce60
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54467
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
The cookie is only needed in SSL_HANDSHAKE, so there's no need to retain
it for the lifetime of the connection. (SSL_HANDSHAKE is released after
the handshake completes.)
Back when DTLS1_COOKIE_LENGTH was 32, storing it inline made some sense.
Now that RFC 6347 increased the maximum to 255 bytes, just indirect it
with an Array<uint8_t>. Along the way, remove the DTLS1_COOKIE_LENGTH
checks. The new limit is the largest that fits in the length prefix, so
it's always redundant. In fact, the constant was one higher was allowed
anyway. Add some tests for the maximum length, as well as zero-length
cookies.
I considered just repurposing the plain cookie field, used in
HelloRetryRequest (as opposed to HelloVerifyRequest), as they're
mutually exclusive, even in DTLS 1.3. But, when we get to DTLS 1.3,
that'll get a little hairy because ssl_write_client_hello will need
extra checks to know whether hs->cookie is meant to go in the
ClientHello directly or in extensions.
Change-Id: I1afedc7ce31414879545701bf8fe4658657ba66f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54466
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: I5e1d37106d7df8e8aaede295e8eb74c971553fd5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54365
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
When installing a library individual destinations should be specified. This is required on Windows which has a .dll that goes in the runtime destination while the .lib ends up in the library destination.
Change-Id: I93cf51089f71c4375324270c6b1c4eadbc637477
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54147
Reviewed-by: Daniel Thornburgh <dthorn@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Node calls these. OpenSSL renamed their APIs to align with the IETF
renaming NamedCurve to NamedGroup. (Ironically, with post-quantum
ciphers, that name turns out also to be wrong and it probably should
have been a reference to KEMs.)
To avoid churn for now, I haven't marked the old ones as deprecated, or
renamed any of the internal types yet. We can see about doing that
later.
Change-Id: I5765cea8398f3836611977805bf8ae7d6efc0a70
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54306
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Among many many problems with renegotiation is it makes every API
ambiguous. Do we return the pending handshake's properties, or the most
recently completed handshake? Neither answer is unambiguously correct:
On the one hand, OpenSSL's API makes renegotiation transparent, so the
pending handshake breaks invariants. E.g., currently,
SSL_get_current_cipher and other functions can return NULL mid
renegotiation. See https://crbug.com/1010748.
On the other hand, OpenSSL's API is callback-heavy. During a handshake
callback, the application most likely wants to check the pending
parameters. Most notably, cert verify callbacks calling
SSL_get_peer_certificate.
Historically, only the pending state was available to return anyway.
We've since changed this
(https://boringssl-review.googlesource.com/8612), but we kept the public
APIs as-is. I was particularly worried about cert verify callbacks.
As of https://boringssl-review.googlesource.com/c/boringssl/+/14028/ and
https://boringssl-review.googlesource.com/c/boringssl/+/19665/, cert
verify is moot. We implement the 3-SHAKE mitigation in library, so the
peer cert cannot change, and we don't reverify the certificate at all.
With that, I think we should switch to returning the established
parameters. Chromium is the main consumer that enables renegotiation,
and it would be better off with this behavior. (Maybe we should try to
forbid other properties, like the cipher suite, from changing on
renegotiation. Unchangeable properties make this issue moot.)
This CL would break if the handshake internally used SSL_get_session,
but this is no longer true as of
https://boringssl-review.googlesource.com/c/boringssl/+/41865.
Update-Note: Some APIs will now behave differently mid-renegotation. I
think this is the safer option, but it is possible code was relying on
the other one.
Fixed: chromium:1010748
Change-Id: I42157ccd9704cde3eebf947136d47cda6754c36e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54165
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Most SSL_ERROR_* values are tracked directly with rwstate. SSL_get_error
is just reading the extra return value out from the previous call.
However, SSL_ERROR_ZERO_RETURN infers close_notify from the SSL's
shutdown state and a zero return value (EOF).
This works, but if we implement SSL_read_ex and SSL_write_ex, a zero
return value is no longer as carefully correlated with EOF. Moreover,
it's already possible to get a non-EOF zero return post-close_notify if
BIO_write returns an (arguably incorrect) return value. Instead, track
SSL_ERROR_ZERO_RETURN in rwstate explicitly.
Since rwstate is exposed as SSL_want and SSL_ERROR_ZERO_RETURN was
previously never returned there, I've made it map SSL_ERROR_ZERO_RETURN
back to SSL_ERROR_NONE. I've also added a test for BIO_write returning
zero, though the real purpose is for a subsequent SSL_write_ex
implementation to retain all the other tests we've added in here.
Update-Note: This is intended to be safe, but if anything breaks around
EOFs, this change is a likely culprit.
Bug: 507
Change-Id: Ide0807665f2e02ee695c4976dc5e99fb10502cf0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53946
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
A later CL will tighten up SSL_ERROR_ZERO_RETURN handling. In
preparation for this, test that SSL_CTX_set_quiet_shutdown can trigger
SSL_ERROR_ZERO_RETURN.
Bug: 507
Change-Id: Ib50a02c514673ad4b73540934480d54b372d9505
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53945
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
This runs through much the same code as the TLS 1.3 bits, though we use
a different hint field to avoid mixups between the fields. (Otherwise
the receiver may misinterpret a decryptPSK hint as the result of
decrypting the session_ticket extension, or vice versa. This could
happen if a ClientHello contains both a PSK and a session ticket.)
Bug: 504
Change-Id: I968bafe12120938e6e46e52536efd552b12c66a0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53805
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
David Benjamin
Sophie Schmieg
Bob Beck
Adam Langley
Adam Langley
Piotr Sikora
Nick Harper
Don