Three years of updating calling code are finally complete!
Update-Note: Accessing the RSA struct directly is no longer supported.
Use accessors instead.
Bug: 316, 325
Change-Id: I27b4c9899cb96f5807075b8fe351eaf72a9a9d44
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60610
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
__builtin_ia32_addcarryx_u64 is, strictly speaking, an ADX intrinsic.
GCC and newer Clang seem to actually implement it without ADX, but
Clang 7 and older will actually try to generate ADX code with it. But
since the caller is not marked target("adx"), this fails to build.
Manually add ADX and BMI2 target attributes to all these functions. The
compiler should be free to use those instructions as these functions all
call into an ADX+BMI2 assembly function anyway. (Though it doesn't do
much with this.)
Note we cannot just annotate fiat_addcarryx_u64. Clang and GCC won't
inline across incompatible targets, so if we tag fiat_addcarryx_u64, we
need to tag the callers up the chain until we're willing to stop
inlining.
Change-Id: I855bb88fea666d92997984836e664292d90df5be
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60612
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
rust-openssl uses this function when targetting OpenSSL 1.1.x.
Change-Id: Ifeb1b65be9976358f9ee636ed23c1a931e03b275
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60609
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
We had to allow this when parsing certs to remain compatible with some
misissued certificates, but there's no reason to allow it when making
new values.
Update-Note: ASN1_UTCTIME_set_string and ASN1_TIME_set_string will no
longer accept times with timezone offsets, which is forbidden by RFC
5280. These functions are used when minting new certificates, rather
than parsing them. The parsing behavior is unchanged by this CL.
Change-Id: I0860deb44a49e99ce477f8cde847d20edfd29ed9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60608
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
We added SSL_CIPHER_get_prf_nid to match the other SSL_CIPHER_get_*_nid
functions, but OpenSSL went with returning the EVP_MD instead.
rust-openssl uses this function, and all the callers of
SSL_CIPHER_get_prf_nid then call EVP_get_digestbynid anyway, so this
version is preferable.
Update-Note: This change is backwards-compatible, but we should update
the QUIC code to use this new function when OPENSSL_API_VERSION is
high enough. It has the benefit of not pulling in random other hash
functions like MD4.
Change-Id: Ied66a6f0adbd5d7d86257d9349c40a2830e3c7e8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60606
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
These bits need more work (and possibly some removal) as they're very,
very far from thread-safe, but rust-openssl relies on them being
const-correct when targetting OpenSSL 1.1.x.
Change-Id: I60531c7e90dbdbcb79c09fc440bd7c6b474172df
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60607
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This probably needs a few iterations, but fix the stuff I noticed on a
first pass:
- I don't think it's useful to say this is the BoringSSL implementation
of something. That's all implicit from this crate anyway.
- Rust seems to prefer "Returns ..." rather than "Return ..."
- Algorithm names like "hkdf" or "hmac" should be written "HKDF" or
"HMAC" when referring to the algorithms. Also BoringSSL is styled
"BoringSSL" rather than "boringssl".
- Given Rust overall doesn't try to handle allocation failure, let's
just write that we don't believe in allocation failure once in the
README rather than marking which functions do and don't panic.
Change-Id: I48501717dd0b063a2fa4106c4c140d76a7ef69a9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60546
Reviewed-by: Nabil Wadih <nwadih@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
That NULL+0 is forbidden is still an awful language bug in C (fixed in
C++), but this particular instance could have been written without
pointer arithmetic. While I'm here, tidy pointers a bit:
- No need to cast pointers to char* when we're writing to void* anyway
- BIO_C_GET_BUF_MEM_PTR is technically a strict aliasing violation. The
pointer points to a BUF_MEM*, not a char*, so we should not write to
it as a char**.
- C casts from void* freely and we've usually omitted the cast in that
case. (Though if we ever move libcrypto to C++, that'll all have to
change.)
Bug: b:286384999
Change-Id: I16d7da675d61f726f259fc9a3cc4a6fce2d6d1fd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60605
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
This fixes the generated Bazel build. Bazel is strict about having all
dependencies declared, which includes files that are #included into
other files. (It also is not particularly pleased about textual
headers and wants them declared in a separate place.)
The new fiat curve25519 assembly is currently split into a BoringSSL
half, and a more generic fiat half. For now, just move the BoringSSL
customizations directly into the fiat half. This isn't ideal, as we'd,
long term, like something where the fiat code can be made standalone.
But, to fix the build, just patch in the changes now and we can ponder
how to do this better later. (Build tools and conventions for assembly
are much less clear than C, sadly.)
Also add the .note.GNU-stack bit at the end.
Change-Id: I04aa733eabf8562dba42dee63a8fd25c86a59db9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60566
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This fixes one of the two build issues triggered by the curve25519
assembly. The new curve25519 assembly is one file that works for both
ELF and Apple targets. This is handy, but breaks the naive concatenation
logic for making the generated file lists. Fix the generation to
deduplicate first.
Convincing Starlark in Bazel to deduplicate is annoyingly difficult, so
we'll just generate both lists. This results in some duplication in the
generated files, but they're generated so this is fine. Hopefully we
can, in time, just remove the per-platform lists when everyone is
migrated over. To that end, this CL adds the combined output to GN so I
can migrate Chromium.
This CL is not sufficient to unbreak the Bazel build. The next change is
also needed.
Bug: 542
Change-Id: Ibe76ff557fc43f7b4d984ccdf298f13c20f3b50c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60565
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Also teach update_clang.py how to download Chromium's Clang on macOS. We
don't currently use it in CI, but I've patched this in enough times for
local testing, that having it seems convenient.
This picks up -fsanitize=function, though it wouldn't have caught the
qsort issue anyway. I'm guessing the libc must be built with UBSan too.
Change-Id: I8c396a10b32e69fe7013283b1bb4320bc35756b6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60547
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
OpenSSL's API constraints are such that sk_FOO_sort must take a
comparison function of type
int (*cmp)(const FOO *const *a, const FOO *const *b)
However qsort expects a comparison function of type
int (*cmp)(const void *a, const void *b)
In C, it is UB to cast a function pointer to a different type and call
it, even if the underlying calling conventions are the same. Moreover,
as qsort doesn't take a context parameter on its comparisons, we cannot
do the usual convention with closures in C.
qsort_r and qsort_s would avoid this, but they are unusable. Too many
libcs don't have them, and those that do define them inconsistently. See
https://stackoverflow.com/a/39561369
It seems this UB has finally hit a sanitizer in fxbug.dev/128274.
Irritating as it is to not even have a working sort function, I think
the easiest option is to just give up on qsort. As we did with bsearch
in https://boringssl-review.googlesource.com/c/boringssl/+/35304, just
implement an in-place heap sort ourselves.
Bug: fxbug.dev/128274
Change-Id: I9de6b4018bf635da0d0c5a680bd7811d297b0bb3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60507
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
At the public API, the comparison functions are a double pointer to
match qsort. qsort comparators are passed pointers to the list elements,
and our list elements are T*. qsort needs this indirection because it
can sort values of arbitrary size. However, our type-erased versions
have no such constraints. Since we know our all elements are pointers,
we can skip the indirection.
Change-Id: Ibb102b51a5aaf0a68a7318bf14ec8f4f9c7a3daf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60506
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We have too many build systems and need to unify the source of truth
between them.
Change-Id: If8a71f400dd478f8890ff56062210295d5e6785e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60545
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL 1.1.x renamed these functions with an OPENSSL_ prefix.
Unfortunately, rust-openssl uses these, losing type-safety, rather than
the type-safe macros. It currently expects the old, unprefixed names due
to a different bug
(https://github.com/sfackler/rust-openssl/issues/1944), but to fix that,
we'll need to align with the OpenSSL names.
To keep the current version of rust-openssl working, I've preserved the
old names that rust-openssl uses, but we should clear these out.
Bug: 499
Change-Id: I3be56a54ef503620b92ce8154fafd46b2906ae63
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60505
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>
.type seems to be needed for the unwind tester to report symbols
correctly. I assume it does other useful things. .hidden
(.private_extern on macOS) keeps it from being exported out of shared
libraries. I'm not positive what .size does, but if ELF wants to know
the size of the function, we should probably tell it that.
Change-Id: Iaa2e4f8a72e81092ec1d81ee2177504c7dc89c76
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60465
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Andres Erbsen <andreser@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Did 75000 Ed25519 key generation operations in 1007110us (74470.5 ops/sec) [+26.9%]
Did 72000 Ed25519 signing operations in 1011133us (71207.2 ops/sec) [+25.5%]
Did 78000 Curve25519 base-point multiplication operations in 1006737us (77478.0 ops/sec) [+27.5%]
Change-Id: I32ca2056f42f9b92af315d8381e1b72be69dd331
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60386
Commit-Queue: Andres Erbsen <andreser@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Update-Note: Accessing the DSA struct directly is no longer supported.
Use accessors instead.
Bug: 325
Change-Id: I73dc20f2e275d48648ca84d2db7806fe155c567d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60425
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Did 29000 Curve25519 arbitrary point multiplication operations in 1026074us (28263.1 ops/sec) [+31.2%]
Change-Id: I9c7d47a047dc68d37202b6cf40d7d12b5b4936f8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60385
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The assembly functions from OpenSSL vary in how the counter overflow
works. The aarch64 implementation uses a mix of 32-bit and 64-bit
counters. This is because, when packing a block into 64-bit
general-purpose registers, it is easier to implement a 64-bit counter
than a 32-bit one. Whereas, on 32-bit general-purpose registers, or when
using vector registers with 32-bit lanes, it is easier to implement a
32-bit counter.
Counters will never overflow with the AEAD, which sets the length limit
so it never happens. (Failing to do so will reuse a key/nonce/counter
triple.) RFC 8439 is silent on what happens on overflow, so at best one
can say it is implicitly undefined behavior.
This came about because pyca/cryptography reportedly exposed a ChaCha20
API which encouraged callers to randomize the starting counter. Wrapping
with a randomized starting counter isn't inherently wrong, though it is
pointless and goes against how the spec recommends using the initial
counter value.
Nonetheless, we would prefer our functions behave consistently across
platforms, rather than silently give ill-defined output given some
inputs. So, normalize the behavior to the wrapping version in
CRYPTO_chacha_20 by dividing up into multiple ChaCha20_ctr32 calls as
needed.
Fixed: 614
Change-Id: I191461f25753b9f6b59064c6c08cd4299085e172
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60387
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Did 59000 Ed25519 key generation operations in 1004188us (58753.9 ops/sec) [+8.3%]
Did 57000 Ed25519 signing operations in 1005649us (56679.8 ops/sec) [+7.9%]
Did 19000 Ed25519 verify operations in 1054380us (18020.1 ops/sec) [-2.0%]
Did 61000 Curve25519 base-point multiplication operations in 1007401us (60551.9 ops/sec) [+8.3%]
Did 22000 Curve25519 arbitrary point multiplication operations in 1022882us (21507.9 ops/sec) [+0.5%]
Change-Id: I14668f658b1ae99850cb0f8938f90f988d0edd0b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60107
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
BN_nnmod() can deal with the situation that the first and the second
arguments are the same, but it cannot deal with the first and the
second argument being equal. In that situation, if BN_mod(x, y, x, ctx)
results in a negative x, then the result of BN_nnmod() is zero. This
breaks the strange BN_mod_inverse(m, a, m, ctx).
Reported by Guido Vranken in
https://github.com/openssl/openssl/issues/21110
Change-Id: I8584720660f214f172b3b33716a5e3b29e8f2fd8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60365
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Andres Erbsen noticed we didn't actually have tests to catch when the
format macros were wrong.
In doing so, remove BN_DEC_FMT2. It was unused and only makes sense in
the context of the bignum-to-decimal conversion, where we no longer use
it anyway. None of these macros are exported in OpenSSL at all, so it
should be safe to remove it. (We possibly can remove the others too. I
see one use outside the library, and that use would probably be better
written differently anyway.)
Change-Id: I4ffc7f9f7dfb399ac060af3caff0778000010303
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60325
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
NGINX requires this constant if SSL_get_negotiated_group is defined.
OpenSSL returns this to indicate a named group constant it doesn't
understand, which doesn't make sense out of SSL_get_negotiated_group
because the library wouldn't negotiate a group it doesn't know about.
Nonetheless, define it for compatibility.
Fixed: 615
Change-Id: I05a6d607912bb8507be9ac6ff5fe1699b4715f6b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60326
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
All inputs are marked as secret. This is not to support a use case for
calling X25519 with a secret *point* as the input, but rather to ensure
that the choice of the point cannot influence whether the scalar is
leaked or not. Same for the initial contents of the output buffer.
This is a conservative choice and may be revised in the future.
Change-Id: I595d454a8e1fdc409912aee751bb0b3cf46f5430
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60186
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Many of our point addition functions internally check for the doubling
case and branch because the addition formulas are incomplete. This
branch is fine because the multiplication formulas are arranged to not
hit this case. However, we don't want to leak the couple of intermedate
values that determine whether to branch. Previously, we ran into this
with https://boringssl-review.googlesource.com/c/boringssl/+/36465.
This wasn't sufficient. The compiler understands if (a & b) enough to
compile into two branches. Thanks to Moritz Schneider, Nicolas Dutly,
Daniele Lain, Ivan Puddu, and Srdjan Capkun for reporting this!
Fix the leak by adding a value barrier on the final value. As we're also
intentionally leaking the result of otherwise secret data flow, I've
used the constant_time_declassify functions, which feed into our
valgrind-based constant-time validation and double as barriers.
Accordingly, I've also added some CONSTTIME_SECRET markers around the
ECDSA nonce value, so we can check with valgrind the fix worked. The
marker really should be at a lower level, at ec_random_nonzero_scalar or
further (maybe RAND_bytes?), but for now I've just marked the nonce.
To then clear valgrind, add constant_time_declassify in a few other
places, like trying to convert infinity to affine coordinates. (ECDH
deals with secret points, but it is public that the point isn't
infinity.)
Valgrind now says this code is constant-time, at least up to compilation
differences introduced by the annotations. I've also inspected the
compiler output. This seems to be fine, though neither test is quite
satisfying. Ideally we could add annotations in ways that don't
influence compiler output.
Change-Id: Idfc413a75d92514717520404a0f5424903cb4453
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60225
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Right now we use NIDs to configure the group list, but group IDs (the
TLS codepoints) to return the negotiated group. The NIDs come from
OpenSSL, while the group ID was original our API. OpenSSL has since
added SSL_get_negotiated_group, but we don't implement it.
To add Kyber to QUIC, we'll need to add an API for configuring groups to
QUICHE. Carrying over our inconsistency into QUICHE's public API would
be unfortunate, so let's use this as the time to align things.
We could either align with OpenSSL and say NIDs are now the group
representation at the public API, or we could add a parallel group ID
API. (Or we could make a whole new SSL_NAMED_GROUP object to pattern
after SSL_CIPHER, which isn't wrong, but is even more new APIs.)
Aligning with OpenSSL would be fewer APIs, but NIDs aren't a great
representation. The numbers are ad-hoc and even diverge a bit between
OpenSSL and BoringSSL. The TLS codepoints are better to export out to
callers. Also QUICHE has exported the negotiated group using the
codepoints, so the natural solution would be to use codepoints on input
too.
Thus, this CL adds SSL_CTX_set1_group_ids and SSL_set1_group_ids. It
also rearranges the API docs slightly to put the group ID ones first,
and leaves a little note about the NID representation before introducing
those.
While I'm here, I've added SSL_get_negotiated_group. NGINX seems to use
it when available, so we may as well fill in that unnecessary
compatibility hole.
Bug: chromium:1442377
Change-Id: I47ca8ae52c274133f28da9893aed7fc70f942bf8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60208
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This adds "group" versions of our codepoint-based APIs. This is mostly
because it looks weird to switch terminology randomly in the
implementation. See I7a356793d36419fc668364c912ca7b4f5c6c23a2 for
additional context.
I've not bothered renaming the bssl_shim flags. That seems a waste of
time.
Change-Id: I566ad132f5a33d99efd8cb2bb8b76d9a6038c825
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60207
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We're this awkward mix of "group" and "curve" right now. On the spec
side, this is because they used to be "curves", but then RFC 7919
renamed to "group" in an attempt to generalize FFDH and ECDH. The
negotiated FFDH stuff never really went anywhere (the way it used cipher
suite values in TLS 1.2 made it unusable), but the name change stuck.
In our implementation and API, we originally called it "curve". In
preparation for TLS 1.3, we renamed the internals to "group" to match
the spec in
https://boringssl-review.googlesource.com/c/boringssl/+/7955, but the
public API was still "curve".
Then we exported a few more things in
https://boringssl-review.googlesource.com/c/boringssl/+/8565, but I left
it at "curve" to keep the public API self-consistent.
Then we added OpenSSL's new "group" APIs in
https://boringssl-review.googlesource.com/c/boringssl/+/54306, but
didn't go as far to deprecate the old ones yet.
Now I'd like to add new APIs to clear up the weird mix of TLS codepoints
and NIDs that appear in our APIs. But our naming is a mess, so either
choice of "group" or "curve" for the new API looks weird.
In hindsight, we probably should have left it at "curve". Both terms are
equally useless for the future post-quantum KEMs, but at least "curve"
is more unique of a name than "group". But at this point, I think we're
too far through the "group" rename to really backtrack:
- Chromium says "group" in its internals
- QUICHE says "group" in its internals and public API
- Our internals say "group"
- OpenSSL has switched to "group" and deprecated "curve", so new APIs
will be based on "group"
So align with all this and say "group". This CL handles set1_curves and
set1_curves_list APIs, which already have "group" replacements from
OpenSSL. A follow-up CL will handle our APIs. This is a soft deprecation
because I don't think updating things is particularly worth the churn,
but get the old names out of the way, so new code can have a simpler API
to target.
Also rewrite the documentation for that section accordingly. I don't
think we need to talk about how it's always enabled now. That's a
reference to some very, very old OpenSSL behavior where ECDH negotiation
needed to be separately enabled.
Change-Id: I7a356793d36419fc668364c912ca7b4f5c6c23a2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60206
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Update-Note: SSL_CIPHER_get_value was our original name for the
function. OpenSSL later called it SSL_CIPHER_get_protocol_id. I believe
all external callers have since been updated to use the new function.
(If I missed a few stragglers, replace with SSL_CIPHER_get_protocol_id
to fix.)
Change-Id: I956fb49bf2d13a898eed73177493d2c8d50778ad
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60205
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Change-Id: I7458b1d7aa1736d586dc80660d59c07fa2ac1c8a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59805
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Also add some tests for X25519_public_from_private, as we apparently
weren't directly testing it with test vectors.
Change-Id: I1b73a9655323d507a8e022c62530ddd4610db4b9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60109
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CONF supports a variable expansion feature, which allows a config file
to easily grow exponentially.
2d05568a7b (upstream's
6a6213556a80ab0a9eb926a1d6023b8bf44f2afd) capped the expansion to 65536
bytes, but this still allows a small input to produce a fairly large
output. (Once we have one large value, it only takes five characters
make a new key that references it, which is an expansion factor of
around 13,000.) This, combined with the string-based extensions
machinery's many quadratic behaviors (see
b92fcfdc17), means fuzzers hit timeouts.
We have very few uses of these APIs left, and none of them use this
feature. Just remove it. While none of these super-linear behaviors are
DoS bugs per se (they should never be passed untrusted input), there's
no sense in carrying an unused feature that only frustrates the fuzzers.
Update-Note: NCONF_load and NCONF_load_bio no longer support the $foo
variable expansion syntax. If you are using these functions in your
project, remove them.
Bug: oss-fuzz:59049
Change-Id: I85832ae1d05373ee420f4fb68197786736ca8272
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60105
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
DQUOTE and FCOMMENT are remnants of a second parser in OpenSSL. OpenSSL
has CONF_type_default, which is the one we imported, and CONF_type_win32
which is a different syntax which looks to be modeled after Windows ini
files. DQUOTE and FCOMMENT only exist in this one.
Change-Id: Iffa01fcb012b0f3e7f8dbf87a01ba867bdc7bcc7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60087
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This file needs more work, but apply some small, local simplifications.
Change-Id: Ia2b93f847e67ae7738afc791acb9ce3cc6cc0342
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60086
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: I8792f118d281bc7a407dfbabe1c8b8e63f9eed9f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60085
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We now ensure STACK_OF(T) sizes and indices fit in INT_MAX, so it's safe
to cast to int.
Bug: 516
Change-Id: I33dd1de6d60a852d510b9b5c3ac70e2eacbc8905
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60066
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Although we've switched STACK_OF(T) to use size_t, OpenSSL used int
pervasively. Much of crypto/x509 and third-party callers use int
indices. As much of that is in the public API now, ensure that
STACK_OF(T) can never exceed INT_MAX elements.
Bug: 516
Change-Id: I26b8fe590655f8c3e449b749b5d0222e28c413f8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60065
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
The Close() method of the middle often wasn't getting called because
`os.Exit(0)` was used in some places. Once that's fixed, it's clear that
the queue of pending reads needed to be closed before waiting for the
reader goroutine to finish. Lastly, don't bother trying to record the
error that the reader saw: just panic the process if the modulewrapper
dies during processing.
Change-Id: Icf077cefd0ace2ef721a493f99fede6269531257
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60045
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Every time we free a type with ex_data (RSA, EC_KEY, DSA, SSL_CTX, SSL,
SSL_SESSION, X509, X509_STORE), we allocate and take a read lock. The
allocation means, if we believe in malloc failures, it is possible to
leak memory on malloc failure. The read lock causes an unnecessary bit
of contention writing to the cache line.
Instead, since we never remove ex_data entries, just thread them in a
singly-linked list. This way we only need to synchronize when to stop
iterating. Add a counter to synchronize that. (Or we could make each
'next' pointers atomic, but this seemed more straightforward.)
(I suspect this doesn't matter much, but it was shorter and we were
already allocating the funcs structures anyway.)
Bug: 570
Change-Id: Ie7ba5cc44f2b71ebd79c8971e784912d53af7f5c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60025
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
3DES has long been obsolete. It uses a small block size, making it
vulnerable to attacks at sufficiently high volumes (see
https://sweet32.info/, CVE-2016-6329). On top of this, it is slow even
without constant-time protections, making it a DoS risk for server
operators.
Since the alias "3DES" has existed in OpenSSL for a long time, keep that
one working, to reduce the risk of breaking someone who specifically
wanted 3DES enabled.
Update-Note: This CL disables TLS_RSA_WITH_3DES_EDE_CBC_SHA by default.
Specifically, it will not be included unless explicitly listed in the
cipher config, as "TLS_RSA_WITH_3DES_EDE_CBC_SHA", its legacy OpenSSL
name "DES-CBC3-SHA", or the alias "3DES". To restore it, add one of the
above to your cipher config.
Bug: 599
Change-Id: Ib94a2f149b3bfa240ef1008b9f3729a9c10368fb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59425
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
OPENSSL_C11_ATOMIC is both computed in crypto/internal.h and also
defined externally. This is a remnant of C11 atomics were an opt-in
feature.
If defined externally, this means OPENSSL_C11_ATOMIC might be defined
when built as C++. That, in turn, causes <stdatomic.h> to be included in
C++ mode. At least one of our users toolchains has a <stdatomic.h> that
is incompatible with C++. We don't get anything out of including it, so
just gate the include on !defined(__cplusplus) for now.
Things to look into as follow-up:
- Fix build files to stop defining OPENSSL_C11_ATOMIC. Prior to
https://boringssl-review.googlesource.com/c/boringssl/+/59847, it was
still serving a purpose: in server builds, if autodetection fails, we
would rather fail to build than accidentally fallback to locks.
There is no lock fallback anymore.
- Fix that toolchain so their <stdatomic.h> is C++-compatible. It's
certainly not C++23-conformant. I suspect it's also not
C++11-conformant, but I'm not positive.
Change-Id: I13bcd8380efeb87b9f9cc439fe24a743e48aec60
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59985
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
If a process calls fork(), then the child process never forks again, the
child may wish to call RAND_enable_fork_unsafe_buffering(). However,
doing so exposes a bug: we assume that, if the flag is set, we don't
need to worry about fork-safety. But it is possible that the PRNG state
was cloned from another process which does not work.
Concretely, consider a zygote process, e.g. Chromium's. A zygote process
would retain fork-safety, but pass along its PRNG state to each of its
children. If the children never fork, they might disable fork-safety,
hitting this bug. (Chromium does not call this API. This is just a
hypothetical scenario.)
Fix this by reseeding whenever the fork-safety bit changes. This fix
does not strictly depend on the atomics work, but it causes us to
unconditionally sample rand_fork_unsafe_buffering_enabled(). This no
longer causes contention because it's just another atomic load.
This only affects systems without MADV_WIPEONFORK and without fast
RDRAND. If RDRAND is fast, we're always fork-safe and MADV_WIPEONFORK
allows us to efficiently detect forks.
Cq-Include-Trybots: luci.boringssl.try:linux_clang_rel_tsan
Change-Id: I6d0c471c62c951254faf85420a7dc3f4a9d65ee0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59850
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This is intended to be run under TSan. ex_data currently works by taking
a global read lock every time an RSA, SSL, SSL_CTX, etc., is freed. We
should be able to fix that but, first, make sure we have test coverage
for the threading requirements.
Bug: 570
Cq-Include-Trybots: luci.boringssl.try:linux_clang_rel_tsan
Change-Id: I0e12907e116481d88e45191a1f15f3a51833bf6d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59865
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
David Benjamin
Andres Erbsen
Maurice Lam
Theo Buehler
Nabil Wadih
Ilya Tokar
Adam Langley
Nicky Mouha