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 function hasn't done anything since 2015, when
https://boringssl-review.googlesource.com/5792 made it a no-op.
Also move RAND_get_system_entropy_for_custom_prng under obscure
functions. It's usually not what you want.
Change-Id: Ica57552792acff3c5991e6db09c15112db09d133
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61505
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
This was, I assume, needed for cross-version tests, but we're long past
2020 now. While I'm here, switch a unique_ptr(new T) into make_unique as
we can rely on C++14 now.
Change-Id: I3f2fde90f124522c21868c15217f607b7f6d75db
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61428
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
They end up in uint64_t frequently right now just because the CBS APIs
use uint64_t, but we don't actually accept that large of an iteration
count.
Also use uint32_t instead of unsigned. This type should be sized based
on how large of an iteration count we think is reasonable, not something
platform-dependent.
Bug: 516
Change-Id: Ie5ff379af6bc65c5e4d25f4d10774bd819f08a50
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61426
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Change-Id: Ie64ed383a1f6b63a0624c8a6f64d92a33dabf56e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61485
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Also fix the comments for ERR_STATE because they were actually wrong.
Bug: 516
Change-Id: I3b352fc75e63075a9f02f33c6e23da0f821a323e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61425
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@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>
Another point indirection removed. As part of this, remove the extra
copy of one and just use generator.raw.Z.
Bug: 20
Change-Id: I066f624fe02e17082383afc15871ab2431e97b61
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60930
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Instead, rely on internal.h to either include the header or polyfill it.
On Windows, we don't (yet) require C11, so we can't rely on the header
being directly includable. Though given it took a couple months to
notice this, it's clear the non-C11 path is pretty much untested, so we
need to get rid of it.
Bug: 624
Change-Id: I86a6961c93161c3adfacd374affb8bfb2be0a542
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61445
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Bug: 287
Change-Id: Ia907c5dd7fd31e95098730673d2da1bede6d79ed
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61405
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The delocate machinery makes it annoying to have pointers in structures.
Also this is a hair more compact.
Bug: 20
Change-Id: I2bc2dd97018277b5be55fd560f4171b7b85928ff
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60929
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This changes the order of things so that by default your
system is expected to provide us with a getentropy() in
<unistd.h> for integrators that are not explicitly
supported.
We preserve the getrandom/urandom dance for Linux and Android
for now.
Linux has had getentropy() in libc's since 2017
macOS, and all the BSD's have had it for any versions we
care about.
iOS hides it from us - so we use CommonCrypto CCRandomGenerateBytes
Update-Note: Non-macOS Apple platforms now use CCRandomGenerateBytes
instead of /dev/urandom. Linux behavior remains unchanged. Platforms
which were not explicitly supported with a different codepath will also
switch from /dev/urandom to getentropy. If your platform specifically
requires /dev/urandom, let us know.know
Bug: 287
Change-Id: I0c2b8c594c473e4395379f50b0c4e6713c0a4c02
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61325
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
libpki.so moved and I forgot to upload this script.
Change-Id: I920c1bd506f7e3848a47729cb70ecf72402e0b49
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61345
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
As part of this, align the generated and standalone builds in how
crypto/test/gtest_main.cc is added. Since not all test targets
(urandom_test) include gtest_main.cc, and the generated one just
manually adds it to the file lists, just put it into the file lists
ahead of time. That way we don't need to synchronize the dependency
information with the generated build.
This also aligns the generated build with
https://boringssl-review.googlesource.com/c/boringssl/+/56567 to put
files like crypto/test/abi_test.cc and crypto/test/file_test_gtest.cc in
the test_support library.
Update-Note: If something odd happens with the test_support library in
downstream builds, this CL is probably to blame.
Bug: 542
Change-Id: I235e0ccd0432f4b380a92b265ede35eb8a6a6e36
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61288
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
CMake and the generate builds now broadly share a source of truth for
the test files.
Update-Note: In the standalone CMake build, build/crypto/crypto_test is
now build/crypto_test, etc. For now, the build still copies the outputs
to the subdirectories (it's cheap and avoids some workflow turbulence),
but I'm thinking we keep that for six months or so and then remove it.
Bug: 542
Change-Id: I8f97e1fcedea1375d48567dfd2da01a6e66ec4e8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61286
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
One less value to initialize statically. Also this simplifies EC_GROUP
initialization. While I'm here, reorder EC_GROUP to pad better.
This lets us simplify the init bits slightly. It does mean p224-64.c,
the one EC_GROUP that doesn't use Montgomery reduction, carries around a
wasted Montgomery context, but it'll make generating the tables
statically much easier. Also once the data is pre-generated, the cost is
minimal.
Bug: 20
Change-Id: Ib66e655ce5a0902ab3ed6695fcbb46aa87683885
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60928
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This reverts commit 28e4a1b838. Bazel
broke --cxxopt on Windows in
https://github.com/bazelbuild/bazel/issues/15073, which means projects
enabling, say, C++20 with --cxxopt=/std:c++20 are silently passing
/std:c++20 to our C files.
This is already a problem, but MSVC is smart enough to silently ignore
the flag when building C. However, MSVC will report an error if you then
pass /std:c++20 /std:c11 into the same command. It seems that check is
not aware of this ignoring behavior.
Ultimately, this is a Bazel bug, and one that makes the broken versions
of Bazel unsuitable for use with C. This was fixed in Bazel in
https://github.com/bazelbuild/bazel/pull/18119 and backported to the
upcoming Bazel 6.3.0 release in
https://github.com/bazelbuild/bazel/pull/18552
Temporarily revert the change. When Bazel 6.3.0 is released, we'll put
this back and require Windows users use a functioning version of Bazel.
Bug: 624
Fixed: 623
Change-Id: I68d9b2ed8751b4cf5dc7f42f8c1fbd42a97d6ca2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61365
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This removes a place where we make hard-coded assumes about test names.
Also it shards pki_test, as that test suite is large enough to benefit
from it.
Change-Id: I392254b73a2df2f022ccf13508552372c103bff7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61285
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This achieves much of the same goals as
https://boringssl-review.googlesource.com/c/boringssl/+/61245, but
reuses the existing source.cmake parser in generate_build_files.py.
There are two practical differences here:
First, CMake knows that, if it sees include(sources.cmake), it should
add sources.cmake as a dependency of build.ninja, and re-run cmake if
source.cmake changes. It does not know this for file(STRINGS). This can
be fixed with CMAKE_CONFIGURE_DEPENDS, but since we already have this,
may as well use it.
Second, sources.cmake wants paths rooted at the top-level BoringSSL
source directory, which means we want to define the targets at the top
level, rather than in subdirectories. While it will make the top-level
CMakeLists.txt larger, I think this is a good move:
- ./build/crypto_test --gtest_filter=... is just less annoying to type
than ./build/crypto/crypto_test --gtest_filter=...
- It will (eventually) mean libcrypto.a lives at build/libcrypto.a
instead of build/crypto/libcrypto.a. This means we can pass a single
-L parameter when building things against BoringSSL, without relying
on the install target.
- It aligns with the generated CMake build. I would like, long-term, to
stop maintaining multiple CMake builds, and the generated one seems to
have a better convention. And one that will be more disruptive to
others if we change it.
- Top-level-rooted paths are more convenient for generate_build_files.py
to work with, because that's what we want to output.
As I get further along in 542, I expect I'll move this once again,
possibly to some JSON file, because I'll need my new pregenerate tool to
parse it (and we'll no longer be constrained by what's easy to consume
in CMake), but use this for now.
Bug: 542
Change-Id: I1e8b894057264da5d7624a1fbb92f9e1198ea38e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61266
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
One less value to initialize statically. Instead, just check if r +
order < p. It's one additional comparison, but those have negligible
cost here.
Bug: 20
Change-Id: Iabc9c1894b58aeba45282e3360e38fe843eb7139
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60927
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This will make it easier to support other build systems rather
than relying on names of files matching a special pattern to
mean a special thing.
Bug: 1322914
Change-Id: I3d1df70e79934717275c0c331e7e5cd3cc5c99e5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61245
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
While we are at it, add a fillins/log.h and modify import_spec.json
to remove the need for patching out LOG and DVLOG (which removes
one more patch in the patch stack
Change-Id: I3b7b512490ee0e8c465734b2236b31da7d132ec7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61225
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We require MSVC 2019 now, which has a /std:c11 flag. Enable it to match
the CMake build and remove a blocker for requiring C11 unconditionally.
(This select branch is also used by clang-cl. I had meant to figure out
the @bazel_tools business as part of this, but it turns out clang-cl
works better with the MSVC flags than the GCC ones anyway. -Wall in
clang-cl is like MSVC's /Wall and actually means all warnings. Ideally
we'd still condition this on the compiler, in case anyone uses MinGW,
but we can figure that out later.)
Tested with bazelisk build :all and
bazelisk build --compiler=clang-cl :all on Windows.
Change-Id: I4559789a221071eef39f9d34929f0e9c5994119e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61127
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
While I'm here, update x/crypto and x/net to their latest versions.
byteReader is a straightforward port, except there doesn't seem to be a
convenient way to read length-prefixed bytes without manually casting
from cryptobyte.String to []byte, so I've done that.
byteBuilder is a bit more involved because it's based on closures, but
still a mechanical change.
As part of this, I switched runner's ticket format to use u24 length
prefixes instead of u32, because cryptobyte.String doesn't have u32
length prefixes. (Although, oddly, cryptobyte.Builder does.)
Fixed: 374
Change-Id: If9bea0b41fe2b8bc48f040a667753b160da469bb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61186
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
BN_MONT_CTX already has the modulus, so just use it. This is one less
value to initialize statically.
Bug: 20
Change-Id: I78f73994ab595b795e99d67851bdff3b73fc3dd6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60926
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: Ie5cb298398c1944704c74b63f51323800f6dc5b0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61065
Reviewed-by: David Benjamin <davidben@google.com>
Auto-Submit: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: Id38833b329b0d661fb18e8a75b671379effe82a6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61166
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Android have not updated their bindgen (see b/279198502), so they cannot
yet pick up inline functions automatically.
Bug: 596
Change-Id: I49d5adaaa3537ada545c9c6fce98ea2dbf2f40ae
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61165
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
This is breaking the oss-fuzz build. Changes made:
- No need make a subproject, I think. In particular, keep the minimum
CMake versions matching.
- Let's not include it in install just yet, since it's still
experimental.
- I removed the comment about public headers. The target doesn't
actually provide any public headers for the moment.
- Apparently the "modern CMake" way to set properties is per-target
rather than using the directory-wide options, so I've switched to
that.
Bug: oss-fuzz:60049
Change-Id: I511667ca8e83cb1997f82b329ed38f980574305e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61126
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
OPENSSL_WINDOWS doesn't *quite* imply that crypto/rand_extra/windows.c
is used, thanks to fuzzer mode.
The sea of ifdefs here is becoming quite a mess, so I've added
OPENSSL_RAND_* resolve the dispatch in one place. Perhaps later we
should also we can also simplify this by just including
CRYPTO_init_sysrand and CRYPTO_sysrand_if_available in all the C files.
But that'll be easier to do when Trusty's RNG is moved in tree.
While I'm here, fold some of the ifdefs in windows.c together.
Change-Id: Ic9c21c5c943a409ebb1d77f27daea1eeb9422e9d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61085
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Trying to migrate Chromium to the "link all the asm files together"
strategy broke the aarch64 Android build because some of the ifdef'd out
assembly files were missing the .note.gnu.property section for BTI. If
we add support for IBT, that'll be another one.
To fix this, introduce <openssl/asm_base.h>, which must be included at
the start of every assembly file (before the target ifdefs). This does a
couple things:
- It emits BTI and noexecstack markers into every assembly file, even
those that ifdef themselves out.
- It resolves the MSan -> OPENSSL_NO_ASM logic, so we only need to do it
once.
- It defines the same OPENSSL_X86_64, etc., defines we set elsewhere, so
we can ensure they're consistent.
This required carving files up a bit. <openssl/base.h> has a lot of
things, such that trying to guard everything in it on __ASSEMBLER__
would be tedious. Instead, I moved the target defines to a new
<openssl/target.h>. Then <openssl/asm_base.h> is the new header that
pulls in all those things.
Bug: 542
Change-Id: I1682b4d929adea72908655fa1bb15765a6b3473b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60765
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The Windows system RNG[1] lives in bcryptprimitives.dll which exports
the function ProcessPrng[2] to supply random bytes from its internal
generators. These are seeded and reseeded from the operating
system using a device connection to \\Device\CNG which is opened
when bcryptprimitives.dll is first loaded.
After this CL boringssl calls ProcessPrng() directly.
Before this CL boringssl got its system randomness (on non-UWP
desktop Windows) from calls to RtlGenRandom[3].
This function is undocumented and unsupported, but has always been
available by linking to SystemFunction036 in advadpi32.dll. In
Windows 10 and later, this export simply forwards to
cryptbase.dll!SystemFunction036 which calls ProcessPrng()
directly.
cryptbase!SystemFunction036 decompiled:
```
BOOLEAN SystemFunction036(PVOID RandomBuffer,ULONG RandomBufferLength)
{
BOOL retval;
retval = ProcessPrng(RandomBuffer,RandomBufferLength);
return retval != 0;
}
```
Loading cryptbase.dll has the side effect of opening a device handle
to \\Device\KsecDD which is not used by boringssl's random number
wrappers. Calling ProcessPrng() directly allows sandboxed programs
such as Chromium to avoid having this handle if they do not need it.
ProcessPrng() also takes a size_t length rather than a u32 length,
allowing some simplification of the calling code.
After this CL we require bcryptprimitives to be loaded before the
first call to CRYPTO_srand(). Applications using the library should
either load the module themselves or call CRYPTO_pre_sandbox_init().
Before this CL boringssl required that advapi32, cryptbase and
bcryptprimitives were all loaded so this should not represent a
breaking change.
[1] https://learn.microsoft.com/en-us/windows/win32/seccng/processprng
[2] https://download.microsoft.com/download/1/c/9/1c9813b8-089c-4fef-b2ad-ad80e79403ba/Whitepaper%20-%20The%20Windows%2010%20random%20number%20generation%20infrastructure.pdf
[3] https://docs.google.com/document/d/13n1t5ak0yofzcadQCF7Ew5TewSUkNfQ3n-IYodjeRYc/edit
Bug: chromium:74242
Change-Id: Ifb1d6ef1a4539ff6e9a2c36cc119b7700ca2be8f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60825
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Initially this leaves the canonical source in chrome, Additions
and fillins are committed directly, the chrome files are coverted
using the IMPORT script run from the pki directory for the moment.
The intention here is to continue frequent automatic conversion
(and avoid wholesale cosmetic changes in here for now) until
chrome converts to use these files in place of it's versions.
At that point these will become the definiative files, and the
IMPORT script can be tossed out.
A middle step along the way will be to change google3's verify.cc
in third_party/chromium_certificate_verifier to use this instead
of it's own extracted copy.
Status (and what is not done yet) being roughly tracked in README.md
Bug: chromium:1322914
Change-Id: Ibdb5479bc68985fa61ce6b10f98f31f6b3a7cbdf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60285
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We could just use the string literal as-is.
Change-Id: I2efe01fd9b020db1bb086001407bcf7fa8487551
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61045
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Change-Id: I824560a5bddb9183e61c663ca4cbdc4530177e66
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60945
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: Andres Erbsen <andreser@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
nanolibc is an embedded platform with no threads. To start unforking
that build, generalize some of the OPENSSL_TRUSTY defines. OpenSSL has
OPENSSL_NO_SOCK if you don't have sockets and OPENSSL_NO_POSIX_IO if you
don't have file descriptors. Those names are fine enough, so I've
borrowed them here too.
There's more to be done here, but this will clear out some of it.
Change-Id: Iaba1fafdebb46ebb8f68b7956535dd0ccaaa832f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60890
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
The shim is now passed a shim ID which allows us to dispatch.
Previously, we dispatched with a distinct TCP server port per test, but
this seems to exhaust local ports.
As with https://boringssl-review.googlesource.com/c/boringssl/+/60885,
this changes the runner/shim protocol, but not the shim/handshaker
protocol. See that CL for discussion.
Change-Id: Ia22d1bc0bcda1b1869bce4630138538c30f02668
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60888
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
That we pull a value out of a channel and put it back is pretty weird.
Also we don't need a select in accept(). It's enough to just close the
listener when we learn the child is gone. (That will cancel the
Accept call.)
Change-Id: If520d9f405fa0b1ad6e3cd23e9ba8a35ff39ba75
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60887
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
It's a bit more verbose to set up, but makes the error paths in
Connect() tidier. While I'm here, stick to Windows' actual SOCKET
type until we have to cross into BIO. It doesn't really matter
(Windows cannot use the upper half of that type without badly
breaking backwards compatibility), but it silences some 64/32
truncation warnings.
Change-Id: I7be7c2b543373a7a9fc50711131e5345d84ebb8b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60886
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The runner currently tries to listen on IPv6 and then falls back to IPv4
on error. The shim does the same. If they pick different ones, this
breaks down.
Normally, fallback happens because the system doesn't have IPv6, and
both sides will make the same decision. But if binding to IPv6 fails for
other reasons, they may mismatch. We're observing them fail due to what
seems to port exhaustion. When this happens, shim and runner don't
agree on the same address family.
Instead, just tell the shim which address to connect to. This doesn't
fix the underlying port exhaustion problem, but it does seem to fix the
flakes. Although given we are still exhausting ports and falling back to
IPv4, it doesn't truly fix it. Later CLs will address port exhaustion by
using a single server port.
This changes the runner <-> shim protocol, but hopefully in a fairly
obvious way that others using BoGo can easily follow. It shouldn't break
our cross-version tests because we keep runner and shim at the same
versio there.
To avoid needing to make an incompatible change to the shim <->
handshaker protocol, which would impact our cross-version tests, this
introduces a mechanism for the shim omit flags when talking to the
handshaker. The handshaker doesn't need to know how to connect to the
runner.
Also print the error string on Windows. Sadly this is a bit tedious.
Change-Id: Ic8bda9a854a115c206c05a659a2e34f544b844a6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60885
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We really should remove these (we only support them in private keys)
but, in the meantime, add some tests for all the curves, not just P-256.
Change-Id: I9c4c0660f082fa1701afe11f51bb157b06befd3c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60925
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This would have made debugging some cross-version test easier.
Change-Id: I7b1bc160b5acf40ec02b9ed5ac2d836e3203cf9a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60805
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
The compiler already knows that .S means assembler and should define
__ASSEMBLER__ for us. At least on the CQ, it seems this is not
necessary, and some builds are showing a warning about redefining
__ASSEMBLER__, which may be caused by this.
Change-Id: I390bfcb50cefef42df72cd61ead55159b1838771
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60785
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
RSA CRT is tiny bit messier when p < q.
https://boringssl-review.googlesource.com/25263 solved this by
normalizing to p > q. The cost was we sometimes had to compute a new
iqmp.
Modular inversion is expensive. We did it only once per key, but it's
still a performance cliff in per-key costs. When later work moves
freeze_private_key into RSA private key parsing, it will be a
performance cliff in the private key parser.
Instead, just handle p < q in the CRT function. The only difference is
needing one extra reduction before the modular subtraction. Even using
the fully general mod_montgomery function (as opposed to checking p < q,
or using bn_reduce_once when num_bits(p) == num_bits(q)) was not
measurable.
In doing so, I noticed we didn't actually have tests that exercise the
reduction step. I added one to evp_tests.txt, but it is only meaningful
when blinding is disabled. (Another cost of blinding.) When blinding is
enabled, the answers mod p and q are randomized and we hit this case
with about 1.8% probability. See comment in evp_test.txt.
I kept the optimization where we store iqmp in Montgomery form, not
because the optimization matters, but because we need to store a
corrected, fixed-width version of the value anyway, so we may as well
store it in a more convenient form.
M1 Max
Before:
Did 9048 RSA 2048 signing operations in 5033403us (1797.6 ops/sec)
Did 1500 RSA 4096 signing operations in 5009288us (299.4 ops/sec)
After:
Did 9116 RSA 2048 signing operations in 5053802us (1803.8 ops/sec) [+0.3%]
Did 1500 RSA 4096 signing operations in 5008283us (299.5 ops/sec) [+0.0%]
Intel(R) Xeon(R) Gold 6154 CPU @ 3.00GHz
Before:
Did 9282 RSA 2048 signing operations in 5019395us (1849.2 ops/sec)
Did 1302 RSA 4096 signing operations in 5055011us (257.6 ops/sec)
After:
Did 9240 RSA 2048 signing operations in 5024845us (1838.9 ops/sec) [-0.6%]
Did 1302 RSA 4096 signing operations in 5046157us (258.0 ops/sec) [+0.2%]
Bug: 316
Change-Id: Icb90c7d5f5188f9b69a6d7bcc63db13d92ec26d5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60705
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
David Benjamin
Bob Beck
Alex Gough
Andres Erbsen