In the STL, <iterator> has a std::size for arrays. Some of these could
also just be ranged for loops. One static_assert could not use
std::size(out->whatever) because out was not a compile-time value, but
std::extent_v<decltype(out->whatever)> works instead.
Change-Id: I28007c79f5583e09167b81a34a447e205ee6dd9b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81658
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Checking for an error on the error queue is very tedious. You have to
check both library and reason code separately, which often means you
need to save the error code to a local variable.
Make an ERR_equals function. I've gone ahead and just made it public API
because even within BoringSSL, it seems to come up everywhere.
Change-Id: Ia963c9ae743ac5c6939846f8f64bbc98b495ce0b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81627
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Conscrypt relies on both this working at the EC_KEY level, but also
EVP_PKEY. Make sure this keeps working, even as we mess around with
EVP_PKEY's EC bits to separate the curves out.
While I'm here, move the common HexToBIGNUM wrapper into test_util.h.
Bug: 42290364
Change-Id: I58fa5a8487b8a499000a798040a53fc851b0a732
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81547
Reviewed-by: Lily Chen <chlily@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This code is currently only used in the p256.cc.inc (not nistz), but it
uses formulas similar to the nistz implementation. In particular, point
doubling is implemented using coordinate halving instead of multiplying
denominators by powers of two.
This change also removes the affine-addition optimization, counteracting
some of the efficiency gains but still resulting in an overall speedup
for client-side usage. This removal is intended to be temporary. The
verified affine-addition function uses a slightly different (nistz-like)
calling convention. Later work will unify the p256 and p256-nistz
files behind the new calling convention and restore the optimization
for both small and large tables.
Benchmarks on Zen 4:
../main/build-small/bssl.json
Did 14400 ECDH P-256 operations in 1000289us (14395.8 ops/sec)
Did 37000 ECDSA P-256 signing operations in 1009883us (36637.9 ops/sec)
Did 16000 ECDSA P-256 verify operations in 1003734us (15940.5 ops/sec)
build-small/bssl.json
Did 15886 ECDH P-256 operations in 1000894us (15871.8 ops/sec) [+10.3%]
Did 36000 ECDSA P-256 signing operations in 1017983us (35364.0 ops/sec) [-3.5%]
Did 18000 ECDSA P-256 verify operations in 1045215us (17221.3 ops/sec) [+8.0%]
P-256 ECDH is still ~40% slower in the small build than full build.
Estimating with double = 0.6 add, table size alone would yield ~30%.
Change-Id: Ide90296f9e14bf543df4e397f088602942c0b658
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/79787
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Andres Erbsen <andreser@google.com>
We used to pass the parent object to callbacks, but this isn't safe. The
object is in the middle of being destroyed, so the object's
invariants won't hold. We can't remove the parameters, but pass in NULL
instead.
This also has the side effect of making it possible to manage
CRYPTO_EX_DATA lifetime in its destructor, because we won't need to find
some handle back to the parent object. It also means that defensively
clearing fields in BIO destroy callbacks, while harmless, is more
obviously unnecessary.
Update-Note: CRYPTO_EX_free is no longer passed the parent object. We do
not expect any callers to be impacted. Callers that were impacted
probably had some object lifetime bug already.
Fixed: 412707574
Change-Id: Idda889898a9eca3b59e3200ba1d5b4f2e62f2d37
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/79607
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This removes the goofy sizeof/alignof assertion we've been making. That
was a strict aliasing violation. Now CRYPTO_refcount_t is just
CRYPTO_atomic_u32 without any fuss. It also means nothing should include
<openssl/thread.h> because it contains only deprecated symbols.
Bug: 412269080
Change-Id: Icbf98a31d5af2a4dadab3b20a410c10f98061ed7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/79569
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
When we have both crypto/foo/internal.h and
crypto/fipsmodule/foo/internal.h, it's important that we put FIPSMODULE
into the header guard.
At that point, we probably should just put CRYPTO in there too, and
indeed we have been putting it in a few places. Go ahead and rewrite
them all with some clever shell scripting.
Change-Id: If61586f1d49ed477023530c36e9ed74373d4d1e6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/77727
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This header used to supply CPU capability constants and things like
__ARM_MAX_ARCH__. The latter is now part of asm_base.h, automatically
included by everything, and assembly files no longer do capability
checks.
The immediate motivation was that there were a few misspelled includes
that used "openssl/arm_arch.h" instead of <openssl/arm_arch.h> (see
https://boringssl-review.googlesource.com/c/boringssl/+/76247), but
given we can just remove them anyway, may as well clear this out.
This now also means we don't pollute another prefix in the global
namespace (though external callers shouldn't have been using this header
anyway).
I've kept the header around, but a no-op, because some bindings
libraries have explicitly listed the header and it's probably not worth
the trouble to clear those out.
Bug: 42290548
Change-Id: I24f36cd48b0e3b6498ecbdc70fecc36961a8b9fe
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/76248
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
We use the standard Apache 2.0 file header, described in "APPENDIX: How
to apply the Apache License to your work."
This was primarily automated by running:
git ls-tree -r --name-only HEAD | xargs go run ./util/relicense.go
See go/boringssl-relicensing-triage for the results of triaging the
output of the tool.
As part of this, switch from taking fiat-crypto under MIT license to
Apache 2.0. (It is licensed under MIT OR Apache-2.0 OR BSD-1-Clause.)
The copyright_summary tool can also be used to confirm we didn't
accidentally drop any copyright lines:
# Run before the CL
git grep -l Copyright | xargs go run ./util/copyright_summary.go -out /tmp/old.json
# Run after the CL
git grep -l Copyright | xargs go run ./util/copyright_summary.go -compare /tmp/old.json
Bug: 364634028
Change-Id: I17c50e761e9d077a1f92e25969e50ed35e320c59
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/75852
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We can still check the public parts, which we now expect callers to
provide. (Without rsa->n, PSS does not work, and a group-less EC_KEY
tends to break horribly.) Though it's also pretty unlikely anyone is
calling these functions on such keys.
Update-Note: The filled in parts of keys backed by RSA_METHOD and
ECDSA_METHOD will now participate in RSA_check_key and EC_KEY_check_key.
Bug: 42290186
Change-Id: I3ebc952f6adb36e9ff6a6ae8413ef0ecd72ae6b6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/75147
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Except for our public headers, our files no longer need to be consumed
by C. That means inline can just be inline, and with C++17,
OPENSSL_UNUSED can be [[maybe_unused]].
Bug: 42290600
Change-Id: Ibdb309bc413660e10d075fbb71d4d1dd87101c6d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74489
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
p256-nistz-table.h was manually editted for the C++ conversion without
updating the generator. Also put the alignas marker in a slightly less
surprising location.
Change-Id: I5ac0d1852caa7b8608c856f4fe7438ea5499f872
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74488
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The referenced bug notes some ODR warnings in an unspecified
configuration. (Which I can't get AppleClang or GCC 14 to reproduce, at
least.) This is likely due to us switching to C++. The ODR rules in C++
are a little different, but also perhaps the detection is only kicking
in for C++.
Either way, structs in .cc files are generally not intended to leave
that compilation unit. (Unless it's an opaque struct listed in base.h.)
There's no such thing as `static struct` so this change wraps many
structs in .cc files in `namespace {`. There are also lots of structs in
test file. Those are less concerning, but test files should mostly
entirely be in a namespace so this change does that where possible for
test files containing structs.
Bug: 384186552
Change-Id: I6ccf715fbcdc3ea6260b5d5d05f305182b1a9450
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74407
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Following the guidance in
https://opensource.google/documentation/reference/releasing/authors,
start maintaining an AUTHORS file.
Update all existing Google copyright lines to 'The BoringSSL Authors'
per the document. This CL also changes the styling to match the new
guidance: removed the '(c)' and the comma.
All other existing copyright lines are left unmodified. Going forward,
our preference will be that new contributions to BoringSSL use 'The
BoringSSL Authors', optionally adding to the AUTHORS file if the
contributor desires.
To avoid being presumptuous, this CL does *not* proactively list every
past contributor in the BoringSSL half of the AUTHORS file. Past
contributors are welcome to send us a patch to be added, or request that
we add you. (Listed or not, the commit log continues to be a more
accurate record, and any existing non-Google copyright lines were left
unmodified.)
The OpenSSL half of the AUTHORS file is seeded with the contents of the
current OpenSSL AUTHORS file, as of writing. The current contents in the
latest revision of the 1.1.1 branch
(b372b1f76450acdfed1e2301a39810146e28b02c) and master
(d992e8729ee38b082482dc010e090bb20d1c7bd5) are identical, just formatted
in text vs Markdown.
Note when reviewing: CONTRIBUTING.md and AUTHORS contain non-mechanical
changes.
Bug: 364634028
Change-Id: I319d0ee63ec021ad85e248e8e3304b9cf9566681
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74149
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This change switches nearly all of BoringSSL to use C++. The public
functions still use the C ABI, and so code written in C can still use
BoringSSL. Also the use of the C++ standard library is minimal and no
run-time requirement for it is intended.
Change-Id: I902d2f51a3c8d6bd0dc4aabe1b192a15d7b788e8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72747
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
It used to be hand-written in assembly, so we tested it explicitly. But
it's now a thin wrapper over mul_mont. Remove the ABI tests (no longer
necessary) and convert the FromMont test vectors to MulMont.
Change-Id: I94696ac7d1f548072702a717183dec8001a47ecb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70807
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Having the files named .c but included causes a ton of problems with
build systems. Many of our build systems care about three categories of
files:
- public headers, available to downstream targets
- source files, each of which is compiled as a compilation unit
- internal headers, not available to downstream targets
There is usually a check, in Bazel called layering_check that enforces
source files only include headers that are declared somewhere
appropriate. The bcm.c fragments, under this classification, are
internal headers.
However, in both GN and Bazel, internal headers and sources
both go in the source list. They are distinguished only by file
extension. When FIPS fragments have a .c file extension, they are
misinterpreted as source files, and many things break.
Rename them. Either .h and .inc would be sufficient. Because we had to
disable Bazel's parse_headers feature, there is no difference (AFAICT)
in their handling. Also, these files actually pass the parse_headers
feature, even though they don't have an include guard. Still, the tech
of the style guide suggests that .inc is probably the better file
extension.
https://google.github.io/styleguide/cppguide.html#Self_contained_Headers
I used .c.inc rather than plain .inc so that we can easily
rename them back to .c when we solve https://crbug.com/362530616.
Note that, as .inc is not as common of a file extension, people working
on BoringSSL may need to reconfigure their editors to map .inc to C/C++.
Update-Note: Some downstream builds have been working around this by
building the fragments individually and excluding bcm.c. This change
will break those workarounds but also remove the need for it. It should
now be consistently possible to build BoringSSL without modifying the
file list.
Bug: 362664827
Change-Id: I933115c37843317a066e24a1092728c9afce35f5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70689
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This CL adjusts the libcrypto <-> BCM ECDSA interface. Previously, we
used ECDSA_do_sign and ECDSA_do_verify. This meant we have an allocated
BIGNUM-based type (ECDSA_SIG) at the boundary.
Instead use the fixed-width P1363 format at the boundary, which is nice
and straightforward. For now, I haven't exported it out of anything,
though we do have some things (Channel ID, WebCrypto) which actually
want this format, so that may be worth revisiting later.
Bug: 42290602
Change-Id: Ifbe0600fd23addc5f05141d18baad21a669ceca8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66829
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Now that we don't depend on external CRYPTO_library_init calls or the
static initializer to initialize CPU capabilities, we can drop a ton of
code.
This makes CRYPTO_library_init, and all its wrappers, into no-ops and
drops the (non-FIPS) static initializer. I've added an internal
OPENSSL_init_cpuid function for the places where the library actually
needs to initialize the CPU vector.
Note this slightly changes the default, previously
static-initializer-full build: previously, CRYPTO_library_init was a
no-op and we relied on the static initializer. Now we uniformly use
CRYPTO_once. This should be an atomic read in the steady state and
essentially free. We can restore the static initializer by default if
this ends up being a problem, but having only one mode is more
straightforward. This also avoids problems if an application calls into
BoringSSL during its own static initializer. Static initializers are not
coherently ordered.
Update-Note: The BORINGSSL_NO_STATIC_INITIALIZER build option and
CRYPTO_library_init are now unnecessary. Once updating past this
revision, those options can now be cleaned up from downstream projects.
Fixed: 40644931
Change-Id: Idc2e6ea7a73d6352e0360fd886c46d88dba3568c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69508
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We really should remove the ia32cap references from those files, but now
that we're down to two files, let's go ahead and remove the
CRYPTO_library_init requirement from our callers and close out the
initialization hole.
Notably, use of bssl-crypto in Chromium is slightly shaky without this.
Although I think, prior to this CL, we'd already gotten to benign races
being all that are possible because these two remaining spots don't
change any in-memory representations. (Unlike C/C++, benign races from
assembly are actually well-defined and truly benign.) But no sense in
relying on this when we can just fix it directly.
This CL just adds some explicit CRYPTO_library_init calls. A subsequent
one will update the docs and clean up all the remnants of our messy
initialization story.
Bug: 40644931
Change-Id: Ife288a4817b930473210f43a2680a60b040bf9a0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69507
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Strictly speaking this does not change any of the bcm/fipsmodule
code, it moves the tests out into libcrypto so that once we move
to an api boundary where bcm does not call libcrypto functions
directly, these tests still can do so.
Bug: 722
Change-Id: I9defc70a9e523e52dda2d53ab4bd155a4b44fc02
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/68787
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Mostly bits of DSA and RSA keygen, flagged when we make the PRNG output
secret by default. There's still a ton of RSA to resolve, mostly because
our constant-time bignum strategy does not interact well with valgrind
when handling RSA's secret-value / public-bit-length situation. Also
RSA's ASN.1 serialization is unavoidably leaky.
Bug: 676
Change-Id: I08d273959065c4db6fd44180a6ac56a82f862fe8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65447
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Although we usually prefer not to use special -1 returns for errors, the
public API does this across the board. Making the internal function
different doesn't do much good.
Change-Id: I6bfe8c9d989da81affeb5cb652de8d3edcbf5efa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66649
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
These were flagged when patching the PRNG to return data marked secret.
This CL doesn't include that part (I didn't run all tests, and other
tests likely require further annotations), but it does include some of
the declassifications necessary to do that later.
Broadly:
- Whenever we compute public keys from private keys, we must tell
valgrind the public key is now public. (Valgrind does not know
that cryptography works.)
- Whenever we compute signatures from private keys, we must tell
valgrind the signature is now public. (Ditto.)
- Whenever we pass a secret value but check it is fully reduced, we must
tell valgrind the comparison may be leaked. (Valgrind doesn't know
these values are always within range... that we have to check at all
is a consequence of OpenSSL's API and/or defensive coding.)
- Valgrind does not know about the randomizing properties of blinding.
We actually aim to be constant-time without RSA blinding, so that
doesn't need an annotation, but the blinded inversion step in the
process of computing the RSA blinding factor does.
Bug: 676
Change-Id: Ic3a47adddb23a61fe452b9be27b214eec2ea5235
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65367
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
(Or second, if there's a method table.)
CRYPTO_EX_free is passed the parent object, so the caller could, in
principle, inspect the object. We should pass in an object in a
self-consistent state. Also fix up the documentation. I think some bits
of a since removed CRYPTO_EX_new got jumbled up in there.
Change-Id: I316d00aee61bf544f59d4dac2efcd825e4bfa9b1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64255
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Clang seems to be undoing the constant-time code here. This is another
constant-time table lookup, so this is more of the usual problem. Found
with the valgrind-based tooling.
I didn't switch this to constant_time_conditional_memxor since we still
haven't figured out https://crbug.com/boringssl/655, but we definitely
need some kind of abstraction for this pattern.
Change-Id: Ic11873b23cde31375ac1a326ed09ac1ca53ec913
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64310
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is not in upstream OpenSSL but saves a bunch of manual overflow
checks. Note it does also introduce some zeroing of buffers, but I think
this should be fine here.
Change-Id: I0c3e65ce2d21ee9d206ccbe3075ce5291c3acb30
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63365
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
OpenSSL added a similar helper function. It's very, very common for us
to malloc something an then zero it. This saves some effort. Also
replace some more malloc + memcpy pairs with memdup.
Change-Id: I1e765c8774a0d15742827c39a1f16df9748ef247
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63345
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
With -march=haswell -DOPENSSL_SMALL=1 on cascadelake:
Did 9999 ECDH P-256 operations in 1062469us (9411.1 ops/sec) [+63.5%]
Did 25000 ECDSA P-256 signing operations in 1028302us (24311.9 ops/sec) [+48.9%]
Did 11004 ECDSA P-256 verify operations in 1072646us (10258.7 ops/sec) [+58.8%]
Same configuration measured no performance difference on haswell.
The added assembly code occupies 1352 bytes.
Change-Id: I42635b7a9bf24d942817976a5d4ce269f642251c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63185
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This replaces our dynamically creating singleton EC_GROUPs from curve
data with static EC_GROUP instances.
They're just shy of being fully static because delocate still forces us
go to through CRYPTO_once to initialize structures with pointers.
(Though, without delocate, the loader would need similar initialization
via a runtime relocation.)
This means we can now have functions like EC_group_p256(), analogous to
EVP_sha256(). These are infallible functions that return const EC_GROUP
pointers. Although there is an initial 2KiB hit to binary size (now we
precompute a few more Montgomery values), I'm hoping it'll eventually
help binaries that only use a few of the curves to drop the others. Also
it removes some locks used to initialize the static curve objects, as
well as removing an annoying error condition.
Bug: 20
Change-Id: Id051c5439f2b2fe2b09bf10964d656503ee27d9e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60931
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This allows operating systems to insist on IBT
enforcement as an exploit mitigation mechanism without
needing to make an exception for anything using a
bundled boringssl, such as chrome, mono, and qtwebengine.
Change-Id: Iac28dd3d2af177b89ffde10ae97bce23739feb94
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60625
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
David Benjamin