The client downgrade detection tests were not asserting on the error (would
have caught the missing error string). Additionally, Downgrade-FalseStart-Draft
isn't testing what it's supposed to; it doesn't actually configure a draft
version or anything. Fix that and have it use ALPN rather than NPN, to match
the test above.
Change-Id: I0b759385641aa00994a912303a6f5bd65522b4bb
Reviewed-on: https://boringssl-review.googlesource.com/31204
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Imported from upstream's 0971432f6f6d8b40d797133621809bd31eb7bf4e and
7d4c97add12cfa5d4589880b09d6139c3203e2f4, but with missing tests added. Along
the way, make Bytes work with any Span<const uint8_t>-convertable type.
Change-Id: If365f981fe8a8274e12000309ffd99b1bb719842
Reviewed-on: https://boringssl-review.googlesource.com/31086
Reviewed-by: Adam Langley <agl@google.com>
Historically, OpenSSL's modular exponentiation functions tolerated negative
moduli by ignoring the sign bit. The special case for a modulus of 1 should do
the same. That said, this is ridiculous and the only reason I'm importing this
is BN_abs_is_word(1) is marginally more efficient than BN_is_one() and we
haven't gotten around to enforcing positive moduli yet.
Thanks to Guido Vranken and OSSFuzz for finding this issue and reporting to
OpenSSL.
(Imported from upstream's 235119f015e46a74040b78b10fd6e954f7f07774.)
Change-Id: I526889dfbe2356753aa1e6ecfd3aa3dc3a8cd2b8
Reviewed-on: https://boringssl-review.googlesource.com/31085
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This fixes uninitialized memory read reported by Nick Mathewson in
https://github.com/openssl/openssl/issues/6347.
It imports the memset from upstream's 2c739f72e5236a8e0c351c00047c77083dcdb77f,
but I believe that fix is incorrect and instead RC4 shouldn't be allowed in
this context. See
https://github.com/openssl/openssl/pull/6603#issuecomment-413066462 for
details.
Update-Note: Decoding a password-protected PEM block with RC4 will, rather than
derive garbage from uninitialized memory, simply fail. Trying to encode a
password-protect PEM block with an unsupported cipher will also fail, rather
than output garbage (e.g. tag-less AES-GCM).
Change-Id: Ib7e23dbf5514f0a523730926daad3c0bdb989417
Reviewed-on: https://boringssl-review.googlesource.com/31084
Reviewed-by: Adam Langley <agl@google.com>
This isn't actually shorter, but there is a bunch of slicing up of the ticket,
which Span makes a little easier to follow.
Change-Id: I7ea4dfe025641a3b88e2c9b8e34246fefc23412f
Reviewed-on: https://boringssl-review.googlesource.com/29865
Reviewed-by: Adam Langley <agl@google.com>
Works in the 1.3 and 1.2 client handshakes, not implemented on the
server for now.
Creates an SSL_CTX option to reverify the server certificate on session
resumption. Reverification only runs the client's certificate verify callback.
Adds new states to the client handshakes: state_reverify_server_certificate in
TLS 1.2, and state_server_certificate_reverify in TLS 1.3.
Adds a negative test to make sure that by default we don't verify the
certificate on resumption, and positive tests that make sure we do when the
new option is set.
Change-Id: I3a47ff3eacb3099df4db4c5bc57f7c801ceea8f1
Bug: chromium:347402
Reviewed-on: https://boringssl-review.googlesource.com/29984
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Along the way, split up the EVPTest Wycheproof tests into separate tests (they
shard better when running in parallel).
Change-Id: I5ee919f7ec7c35a7f2e0cc2af4142991a808a9db
Reviewed-on: https://boringssl-review.googlesource.com/30846
Reviewed-by: Adam Langley <agl@google.com>
Also remove some transition step for a recent format change. Together, this
removes the curve hacks in the converter, which can now be purely syntactic.
The RSA ones are still a bit all over the place in terms of sharded vs
combined, so leaving that alone for now.
Change-Id: I721d6b0de388a53a39543725e366dc5b52e83561
Reviewed-on: https://boringssl-review.googlesource.com/30845
Reviewed-by: Adam Langley <agl@google.com>
The size of an int is 4, not 2. To avoid worrying about this, add a GetVector
counterpart to GetString that handles all this. Apply this uniformly to avoid
all the pointer casts. This is less important for vector<uint8_t>, but even
then we'll now notice a 1-byte OOB read since std::string is NUL-terminated.
Also it's shorter.
Change-Id: Ie96591cb8d8d52742f5fd30d70b6af0511109585
Reviewed-on: https://boringssl-review.googlesource.com/30864
Reviewed-by: Adam Langley <agl@google.com>
We currently write a mix of "if (FOO)" and "if(FOO)". While the former looks
more like a usual language, CMake believes everything, even "if" and "else", is
just a really really funny function call (a "command").
We should pick something for consistency. Upstream CMake writes "if(FOO)", so
go with that one.
Change-Id: I67e0eb650a52670110b417312a362c9f161c8721
Reviewed-on: https://boringssl-review.googlesource.com/30807
Reviewed-by: Adam Langley <agl@google.com>
SSL_CTX_set1_sigalgs_list wants a NUL-terminated string, so we need to use
GetString to give it one.
Bug: oss-fuzz:9808
Change-Id: Id7f676aa514c36de9dea900763db3cbbf5c79a4c
Reviewed-on: https://boringssl-review.googlesource.com/30804
Reviewed-by: Adam Langley <agl@google.com>
The CFI bot is currently failing on a mysterious error message, coming from the
recent clang roll.
Called function is not the same type as the call!
call void @EVP_MD_CTX_init(%struct.env_md_ctx_st* %8), !dbg !72123
LLVM ERROR: Broken function found, compilation aborted!
Chromium actually passes -flto=thin, which seems to avoid the error, testing
locally. Why it does, I haven't the slightest clue. The offending calls to
EVP_MD_CTX_init (and EVP_MD_CTX_cleanup) are those buried in
bssl::ScopedEVP_MD_CTX. However, not all calls are problematic, only the one in
test_config.cc. What's more, if I add a call in async_bio.cc, linked into all
the same targets, the copy in test_config.cc is suddenly fine!?
Maybe there's just a bug in the LTO logic that ThinLTO avoids...
Change-Id: I5266eec75edea2a38dee8ad5591db8d65d3bdede
Reviewed-on: https://boringssl-review.googlesource.com/30805
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
MSan and TSan both require instrumenting everything. Add some machinery so we
can do this on the bots.
Change-Id: I7d2106bc852ee976455d18787d3a20a35373a9e7
Reviewed-on: https://boringssl-review.googlesource.com/30644
Reviewed-by: Adam Langley <agl@google.com>
ExpectTicketKeyChanged treats its input as an in/out parameter, but the first
time around there isn't a previous key. We could just call
SSL_CTX_get_tlsext_ticket_keys directly, but running it with the "previous"
keys as all zeros seems prudent; the ticket key rotation logic lazily
initializes keys and, were we to accidentally forget to initialize a key, the
zero key seems the most likely culprit.
Change-Id: I7167bef78e0bfcdb178195230ad84597f26d825c
Reviewed-on: https://boringssl-review.googlesource.com/30684
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
These functions can be used to configure the signature algorithms. One
of them is a string mini-languaging parsing function, which we generally
dislike because it defeats static analysis. However, some dependent
projects (in this case TensorFlow) need it and we also dislike making
people patch.
Change-Id: I13f990c896a7f7332d78b1c351357d418ade8d11
Reviewed-on: https://boringssl-review.googlesource.com/30304
Reviewed-by: Steven Valdez <svaldez@google.com>
embed_test_data.go assumes that it's working with 8KB chunks. However,
if the input file contains a '\' then the Go code thinks that it counts
as a byte, but the C compiler will probably merge it with the following
char and thus that string will be slightly too short. ASAN will detect
the out-of-bounds read when 8192 bytes are copied from the string.
Change-Id: If40ccfd39ea013bd6935fcc313cfe188fe985f67
Reviewed-on: https://boringssl-review.googlesource.com/30444
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The previous limit was |UINT_MAX|. Windows limits to 600K, but that's
already causing issues. This seems like a balance between being
completely crazy and still large enough not to have to worry for a long
time. It's still probably too large for backend systems wanting to
process arbitrary PKCS#12, but I don't think any fixed value will
satisfy all desires.
Change-Id: I01a3f78d5f2df086f8dbc0e8bacfb95153738f55
Reviewed-on: https://boringssl-review.googlesource.com/30424
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This change syncs several assembly files from upstream. The only meanful
additions are more CFI directives.
Change-Id: I6aec50b6fddbea297b79bae22cfd68d5c115220f
Reviewed-on: https://boringssl-review.googlesource.com/30364
Reviewed-by: Adam Langley <agl@google.com>
In fuzzing builds, session resumptions fail if the PRNG behaves the
same as in the initial session. Not sure of the reason, but a kick to
the PRNG fixes the problem and doesn't compromise determinism, so
... *shrug*?
Change-Id: I8181d98fdff16ae82255e9cda33ce5c4c40b5399
Reviewed-on: https://boringssl-review.googlesource.com/30284
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL 1.0.2 (and thus BoringSSL) accepts keyUsage certSign or a
Netscape CA certificate-type in lieu of basicConstraints in an
intermediate certificate (unless X509_V_FLAG_X509_STRICT) is set.
Update-Note: This change tightens the code so that basicConstraints is required for intermediate certificates when verifying chains. This was previously only enabled if X509_V_FLAG_X509_STRICT was set, but that flag also has other effects.
Change-Id: I9e41f4c567084cf30ed08f015a744959982940af
Reviewed-on: https://boringssl-review.googlesource.com/30185
Reviewed-by: Matt Braithwaite <mab@google.com>
This change adds a new flag, X509_V_FLAG_REQUIRE_CA_BASIC_CONSTRAINTS,
which causes basicConstraints with isCA to be required for intermediate
CA certificates. Without this, intermediates are also acceptable if
they're missing basicConstraints, but include either a certSign
keyUsage, or a CA Netscape certificate type.
This is a short-term change for patching. I'll undo a lot of it and make
this the default in the next change.
Change-Id: I7f42ffd76c57de3037f054108951e230c1b4e415
Reviewed-on: https://boringssl-review.googlesource.com/30184
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Matt Braithwaite <mab@google.com>
The runner currently expects split handshake tests to work is GOOS is
"linux", but that includes Android, which the shim doesn't support.
Rather than try to align these two conditions, have the runner ask the
shim whether it supports split handshakes or not.
Change-Id: I7bea0d94142c4b6ee42b8f54c67b8611da93feb3
Reviewed-on: https://boringssl-review.googlesource.com/30204
Reviewed-by: Matt Braithwaite <mab@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The new binary, called |handshaker|, allows split-handshakes to be
tested using shim and handshaker binaries built at different
revisions.
The shim now proxies traffic to the handshaker during the split
handshake. The handoff and handback steps serialize additional state
about the test being performed, and its results.
The proxy and handshaker make heavy use of Unix-isms, and so
split-handshake tests are now restricted to Linux.
Change-Id: I048f0540c3978a31b3e573e00da17caf41a8059e
Reviewed-on: https://boringssl-review.googlesource.com/29348
Reviewed-by: Adam Langley <agl@google.com>
Setting OPENSSL_NO_ASM skips enabling the “ASM” language in CMake.
However, the FIPS module fundamentally needs to build asm because
delocate works via textual assembly. Thus this combination is currently
broken with CMake.
This change ensures that support for building asm is enabled in CMake
for this combination.
Change-Id: I4516cf3a6f579ee7c72f04ac25d15785926cf125
Reviewed-on: https://boringssl-review.googlesource.com/29884
Reviewed-by: Adam Langley <agl@google.com>
This change adds a function so that an ECDH and the hashing of the
resulting 'x' coordinate can occur inside the FIPS boundary.
Change-Id: If93c20a70dc9dcbca49056f10915d3ce064f641f
Reviewed-on: https://boringssl-review.googlesource.com/30104
Reviewed-by: Adam Langley <agl@google.com>
Future versions of the Wycheproof vectors will specify the curve for a
group of tests, rather than for each test. This change works with both
the old and new style.
Change-Id: I0d9a503c8357eb4c617544e727d8f4a703c2c2b0
Reviewed-on: https://boringssl-review.googlesource.com/30084
Reviewed-by: Adam Langley <agl@google.com>
MSan works by instrumenting memory accesses in the compiler. Accesses from
uninstrumented code, such as assembly, are invisible to it. MSan will
incorrectly report reads from assembly-initialized memory as uninitialized.
To avoid confusing downstream consumers with false positives, catch this at
compile-time with a more useful error.
Update-Note: BoringSSL with MSan and assembly doesn't work, but now rather than
crashing at runtime, it will fail to build altogether. It's possible someone
was building BoringSSL with MSan and either not running it at all or just not
exercising the codepaths that break.
Bug: 252
Change-Id: I0c8b0fa3c2d1e584b3f40d532a668a8c9be06cb7
Reviewed-on: https://boringssl-review.googlesource.com/29928
Reviewed-by: Adam Langley <agl@google.com>
There were some subtleties in this one. I'm not sure if TSan covers it all, but
it's better than nothing.
Change-Id: I239e3aee2fea84caa2e48f555d08c6d89f430402
Reviewed-on: https://boringssl-review.googlesource.com/29927
Reviewed-by: Adam Langley <agl@google.com>
Otherwise, if the output BIGNUM was previously negative, we'd incorrectly give
a negative result. Thanks to Guide Vranken for reporting this issue!
Fortunately, this does not appear to come up in any existing caller. This isn't
all that surprising as negative numbers never really come up in cryptography.
Were it not for OpenSSL historically designing a calculator API, we'd just
delete the bit altogether. :-(
Bug: chromium:865924
Change-Id: I28fdc986dfaba3e38435b14ebf07453d537cc60a
Reviewed-on: https://boringssl-review.googlesource.com/29944
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
In particular, although CertificateRequest comes before Certificate and
CertificateVerify in TLS 1.3, we must not resolve the CertificateRequest until
afterwards. (This is rather annoying ordering, but does mean the
CertificateRequest is covered in the signature, which is nice to have.)
Change-Id: Iab95813de5efd674aa8e2459cfc7456b146ee754
Reviewed-on: https://boringssl-review.googlesource.com/29826
Reviewed-by: Jesse Selover <jselover@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This commit is to allow Tensorflow to build with boringssl on ppc64le
and RHEL7.5/gcc 4.8.5.
All the instructions used by linux_x86_64 also need to bet set for
linux_ppc64le
Change-Id: I4ccf8a61fe3bdd0a49944b48ce7863b97f957a85
Reviewed-on: https://boringssl-review.googlesource.com/29784
Reviewed-by: Adam Langley <agl@google.com>
Callers who use SSL_get0_certificate_types today will find an empty list
in TLS 1.3, which removed it. To provide feature parity, add an accessor
for the signature algorithms list. SSL_get_signature_algorithm_key_type
can be used to map it to a key type.
"Peer signature algorithms" was already taken in the public API by
SSL_get_peer_signature_algorithm to refer to which the peer selected, so
I named this matching SSL_CTX_set_verify_algorithm_prefs.
Change-Id: I12d411d7350e744ed9f88c610df48e0d9fc13256
Reviewed-on: https://boringssl-review.googlesource.com/29684
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Vartanian <flooey@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
They've since added new files that split up ECDH and RSA. The former especially
could be useful. A later commit will switch to those. Along the way, fix the
aes_cmac_test.json entry in the convert_wycheproof.go which got lost at some
point.
Change-Id: I9c4a2e5fc5f3e0935482f583c5466c1b64fe325e
Reviewed-on: https://boringssl-review.googlesource.com/29686
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This makes the shim code read more naturally, in that the split-
handshake special case now lives in its own file.
This helps with creating a separate binary to perform split
handshakes.
Change-Id: I7970a8f368417791d18d4d44eeb379ef4b46c960
Reviewed-on: https://boringssl-review.googlesource.com/29347
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Delocate failed with some versions of Clang that reference
OPENSSL_ia32cap_P with an orq instruction.
Change-Id: I448d291594f5f147424e6f7014a681c4201b0aee
Reviewed-on: https://boringssl-review.googlesource.com/29764
Reviewed-by: Adam Langley <alangley@gmail.com>
In f2bc5f4 davidben pointed out that this function seems unnecessary
in my desired end-state. In fact, I think it may have been
unnecessary since 56986f90. (This was easier to miss at the time,
since at the time the function was part of MoveExData(), having not
yet been factored out.)
Change-Id: Ia9b4a909c93cb595666bcf7356a9f9a085901455
Reviewed-on: https://boringssl-review.googlesource.com/29604
Commit-Queue: Matt Braithwaite <mab@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Previously we used thread-local state objects in rand.c. However, for
applications with large numbers of threads, this can lead to excessive
memory usage.
This change causes us to maintain a mutex-protected pool of state
objects where the size of the pool equals the maximum concurrency of
|RAND_bytes|. This might lead to state objects bouncing between CPUs
more often, but should help the memory usage problem.
Change-Id: Ie83763d3bc139e64ac17bf7e015ad082b2f8a81a
Reviewed-on: https://boringssl-review.googlesource.com/29565
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
While I'm here, remove the silly "tlsext_" prefix. At this point it's no
longer novel that a feature is encoded in an extension.
Change-Id: Ib5fbd2121333a213bdda0332885a8c90036ebc4d
Reviewed-on: https://boringssl-review.googlesource.com/29592
Reviewed-by: Adam Langley <agl@google.com>
This doesn't actually make use of much of C++ yet. (SSL_CTX and
SSL/SSL_CONFIG carry analogous versions of a number of fields. It's
difficult to switch them to UniquePtr separately.)
Change-Id: Ia948f539c5c90e2d8301193f719604a31be17fc4
Reviewed-on: https://boringssl-review.googlesource.com/29589
Reviewed-by: Adam Langley <agl@google.com>
This doesn't give them a destructor yet, just shifts things around. In
doing so, it reveals that we inconsistently allowed internal code, but
not external code, to call functions like bssl::SSL_CTX_set_handoff_mode
without a namespace because of ADL. External code doesn't get to do
this because it doesn't see that ssl_ctx_st has a base class in
namespace bssl.
Change-Id: I2ab3b00fff2d6369e850606eed63017e4f8cf8c4
Reviewed-on: https://boringssl-review.googlesource.com/29588
Reviewed-by: Adam Langley <agl@google.com>
It's 2018, but passing STL objects across the API boundary turns out to
still be more bother than it's worth. Since we're dropping UniquePtr in
the API anyway, go the whole way and make it a plain-C API.
Change-Id: Ic0202012e5d81afe62d71b3fb57e6a27a8f63c65
Update-note: this will need corresponding changes to the internal use of SSL_CTX_add_cert_compression_alg.
Reviewed-on: https://boringssl-review.googlesource.com/29564
Reviewed-by: David Benjamin <davidben@google.com>
This makes |TestState| and |TestConfig| accessible outside
bssl_shim.cc, as well as the functions SetupCtx() and NewSSL(), which
become methods on |TestConfig|. A whole mess of callbacks move in
order to support this change.
Along the way, some bits of global state are moved (e.g. the global
test clock) and made self-initializing.
This helps with creating a separate binary to perform split
handshakes.
Change-Id: I39b00a1819074882353f5f04ed01312916f3cccb
Reviewed-on: https://boringssl-review.googlesource.com/29345
Commit-Queue: Matt Braithwaite <mab@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Previously we'd partially attempted the ssl_st / bssl::SSLConnection
subclassing split, but that gets messy when we actually try to add a
destructor, because CRYPTO_EX_DATA's cleanup function needs an ssl_st*,
not a bssl::SSLConnection*. Downcasting is technically undefined at this
point and will likely offend some CFI-like check.
Moreover, it appears that even with today's subclassing split,
New<SSL>() emits symbols like:
W ssl_st*& std::forward<ssl_st*&>(std::remove_reference<ssl_st*&>::type&)
The compiler does not bother emitting them in optimized builds, but it
does suggest we can't really avoid claiming the ssl_st type name at the
symbol level, short of doing reinterpret_casts at all API boundaries.
And, of course, we've already long claimed it at the #include level.
So I've just left this defining directly on ssl_session_st. The cost is
we need to write some silly "bssl::" prefixes in the headers, but so it
goes. In the likely event we change our minds again, we can always
revise this.
Change-Id: Ieb429e8eaabe7c2961ef7f8d9234fb71f19a5e2a
Reviewed-on: https://boringssl-review.googlesource.com/29587
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
lh_FOO_retrieve is often called with a dummy instance of FOO that has
only a few fields filled in. This works fine for C, but a C++
SSL_SESSION with destructors is a bit more of a nuisance here.
Instead, teach LHASH to allow queries by some external key type. This
avoids stack-allocating SSL_SESSION. Along the way, fix the
make_macros.sh script.
Change-Id: Ie0b482d4ffe1027049d49db63274c7c17f9398fa
Reviewed-on: https://boringssl-review.googlesource.com/29586
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This partitions the session ID space of the internal cache by version,
which is nominally something we want, but we must check the version
externally anyway for both tickets and external session cache. That
makes this measure redundant. (Servers generate session IDs and 2^256 is
huge, so there would never accidentally be a collision.)
This cuts down on the "key" in the internal session cache, which will
simplify adding something like an lh_SSL_SESSION_retrieve_key function.
(LHASH is currently lax about keys because it can freely stack-allocate
partially-initialized structs. C++ is a bit more finicky about this.)
Change-Id: I656fd9dbf023dccb163d2e8049eff8f1f9a0e21b
Reviewed-on: https://boringssl-review.googlesource.com/29585
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
bssl::UniquePtr and FOO_up_ref do not play well together. Add a helper
to simplify this. This allows us to write things like:
foo->cert = UpRef(bar->cert);
instead of:
if (bar->cert) {
X509_up_ref(bar->cert.get());
}
foo->cert.reset(bar->cert.get());
This also plays well with PushToStack. To append something to a stack
while taking a reference, it's just:
PushToStack(certs, UpRef(cert))
Change-Id: I99ae8de22b837588a2d8ffb58f86edc1d03ed46a
Reviewed-on: https://boringssl-review.googlesource.com/29584
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
alignas in C++11 is a bit more flexible than
__attribute__((aligned(x))), and we already require C++11 in tests.
Change-Id: If61c35daa5fcaaca5119dcc6808a3e746befc170
Reviewed-on: https://boringssl-review.googlesource.com/29544
Reviewed-by: Adam Langley <agl@google.com>
The whitespace in the _STL_EXTRA_DISABLED_WARNINGS value was creating issues
for the CMake generated assembler build script called by VS.
By narrowing the build scope of this STL (and thus C++ only) variable to only C++
we avoid the problem altogether as it will not be passed to the assembler script.
Change-Id: Id422bdd991492f39acc82d52af2ea6d952deb6c6
Reviewed-on: https://boringssl-review.googlesource.com/29504
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
It's 2018. I'm not sure why I added the 32-bit ones; even the 32-bit
bots build and run on 64-bit Windows. ninja.exe in depot_tools is also a
64-bit binary. I suspect this is because some of the depot_tools bits
use --platform=win32, but that's just the sys.platform string.
Alas, I stupidly named these "win32" way back. Dealing with the rename
is probably more trouble than worth it right now since the build recipes
refer to the name. Something to deal with later. (Regardless we'll want
"win32" to point to 64-bit binaries so that try jobs can test it.)
Also add the missing nasm-win32.exe to .gitignore.
For some reason the 64-bit Yasm binary does not work on the vs2017 CQ
bots, so I've left it alone. Hopefully it should be replaced by NASM
later anyway.
Change-Id: If65ececddbc6526ceebaafbef56eddea8ece58ba
Reviewed-on: https://boringssl-review.googlesource.com/29384
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is to transition BoringSSL's Windows build from Yasm to NASM. This
change itself is a no-op for now, but a later change to the BoringSSL
recipes will add a pair of standalone builders here. Then I'll get the
change I have lying around for Chromium moving.
Bug: chromium:766721
Change-Id: I4dca1c299f93bc5c01695983fe0478490c472deb
Reviewed-on: https://boringssl-review.googlesource.com/29324
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Right now we're inconsistent about it. If the OPTIONAL container is
missing, we report an error, but if the container is empty, we happily
return nothing. The latter behavior is more convenient for emulating
OpenSSL's PKCS#7 functions.
These are our own functions, so we have some leeway here. Looking
through callers, they appear to handle this fine.
Update-Note: This is a behavior change.
Change-Id: I1321025a64df3054d380003c90e57d9eb95e610f
Reviewed-on: https://boringssl-review.googlesource.com/29364
Reviewed-by: Adam Langley <agl@google.com>
CBS_asn1_ber_to_der was a little cumbersome to use. While it, in theory,
allowed callers to consistently advance past the element, no caller
actually did so consistently. Instead they would advance if conversion
happened, and not if it was already DER. For the PKCS7_* functions, this
was even caller-exposed.
Change-Id: I658d265df899bace9ba6616cb465f19c9e6c3534
Reviewed-on: https://boringssl-review.googlesource.com/29304
Reviewed-by: Adam Langley <agl@google.com>
Previously, delocate.go couldn't handle GOT references and so |stderr|
was a problematic symbol. We can cope with them now, so write FIPS
power-on test and urandom errors to stderr rather than stdout.
Change-Id: If6d7c19ee5f22dcbd74fb01c231500c2e130e6f7
Update-note: resolves internal bug 110102292.
Reviewed-on: https://boringssl-review.googlesource.com/29244
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This change adds an AES-GCM AEAD that enforces nonce uniqueness inside
the FIPS module, like we have for TLS 1.2. While TLS 1.3 has not yet
been mentioned in the FIPS 140 IG, we expect it to be in the next ~12
months and so are preparing for that.
Change-Id: I65a7d8196b08dc0033bdde5c844a73059da13d9e
Reviewed-on: https://boringssl-review.googlesource.com/29224
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
The DSA code is deprecated and will, hopefully, be removed in the future.
Nonetheless, this is easy enough to fix. It's the analog of the work we'd
already done for ECDSA.
- Document more clearly that we don't care about the DSA code.
- Use the existing constant-time modular addition function rather than
the ad-hoc code.
- Reduce the digest to satisfy modular operations' invariants. (The
underlying algorithms could accept looser bounds, but we reduce for
simplicity.) There's no particular reason to do this in constant time,
but we have the code for it, so we may as well.
- This additionally adds a missing check that num_bits(q) is a multiple
of 8. We otherwise don't compute the right answer. Verification
already rejected all 160-, 224-, and 256-bit keys, and we only
generate DSA parameters where the length of q matches some hash
function's length, so this is unlikely to cause anyone trouble.
- Use Montgomery reduction to perform the modular multiplication. This
could be optimized to save a couple Montgomery reductions as in ECDSA,
but DSA is deprecated, so I haven't bothered optimizing this.
- The reduction from g^k (mod p) to r = g^k (mod p) (mod q) is left
in variable time, but reversing it would require a discrete log
anyway. (The corresponding ECDSA operation is much easier to make
constant-time due to Hasse's theorem, though that's actually still a
TODO. I need to finish lifting EC_FELEM up the stack.)
Thanks to Keegan Ryan from NCC Group for reporting the modular addition issue
(CVE-2018-0495). The remainder is stuff I noticed along the way.
Update-Note: See the num_bits(q) change.
Change-Id: I4f032b041e2aeb09f9737a39f178c24e6a7fa1cb
Reviewed-on: https://boringssl-review.googlesource.com/29145
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Although the original value of tmp does not matter, the selects
ultimately do bit operations on the uninitialized values and thus depend
on them behaving like *some* consistent concrete value. The C spec
appears to allow uninitialized values to resolve to trap
representations, which means this isn't quite valid..
(If I'm reading it wrong and the compiler must behave as if there were a
consistent value in there, it's probably fine, but there's no sense in
risking compiler bugs on a subtle corner of things.)
Change-Id: Id4547b0ec702414b387e906c4de55595e6214ddb
Reviewed-on: https://boringssl-review.googlesource.com/29124
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The last libssl struct is now opaque! (Promote the SSL_MAX_* constants
as folks use them pretty frequently.)
Update-Note: SSL_SESSION is now opaque. I believe everything handles
this now.
Bug: 6
Change-Id: I8cd29d16173e4370f3341c0e6f0a56e00ea188e9
Reviewed-on: https://boringssl-review.googlesource.com/28964
Reviewed-by: Adam Langley <agl@google.com>
Update-Note: This tweaks the SSL_shutdown behavior. OpenSSL's original
SSL_shutdown behavior was an incoherent mix of discarding the record and
rejecting it (it would return SSL_ERROR_SYSCALL but retrying the
operation would discard it). SSLeay appears to have intended to discard
it, so we previously "fixed" it actually discard.
However, this behavior is somewhat bizarre and means we skip over
unbounded data, which we typically try to avoid. If you are trying to
cleanly shutdown the TLS portion of your protocol, surely it is at a
point where additional data is a syntax error. I suspect I originally
did not realize that, because the discarded record did not properly
continue the loop, SSL_shutdown would appear as if it rejected the data,
and so it's unlikely anyone was relying on that behavior.
Discussion in https://github.com/openssl/openssl/pull/6340 suggests
(some of) upstream also prefers rejecting.
Change-Id: Icde419049306ed17eb06ce1a7e1ff587901166f3
Reviewed-on: https://boringssl-review.googlesource.com/28864
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
This change adds server-side support for compressed certificates.
(Although some definitions for client-side support are included in the
headers, there's no code behind them yet.)
Change-Id: I0f98abf0b782b7337ddd014c58e19e6b8cc5a3c2
Reviewed-on: https://boringssl-review.googlesource.com/27964
Reviewed-by: David Benjamin <davidben@google.com>
The STL already came up with a threading abstraction for us. If this
sticks, that also means we can more easily write tests elsewhere that
use threads. (A test that makes a bunch of TLS connections on a shared
SSL_CTX run under TSan would be nice. Likewise with some of the messy
RSA locking.)
Update-Note: This adds a dependency from crypto_test to C++11 threads.
Hopefully it doesn't cause issues.
Change-Id: I26f89f6b3b79240e516017877d06fd9a815fc315
Reviewed-on: https://boringssl-review.googlesource.com/28865
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
GN does not like multiple files in the same target that share a name, so
add a script to check for this. A follow-up changes will hook that up to
the builders, so we'll flag this in try jobs rather than when the change
trickles downstream.
Change-Id: Ic413dd9aeed6da54fc85dea07f80fe7084be9e9e
Reviewed-on: https://boringssl-review.googlesource.com/28844
Reviewed-by: Adam Langley <agl@google.com>
We have a successful TLS 1.3 deployment, in spite of non-compliant
middleboxes everywhere, so now let's get this optimization in. It would
have been nice to test with this from the beginning, but sadly we forgot
about it. Ah well. This shaves 63 bytes off the server's first flight,
and then another 21 bytes off the pair of NewSessionTickets.
So we'll more easily notice in case of anything catastrophic, tie this
behavior to draft 28.
Update-Note: This slightly tweaks our draft-28 behavior.
Change-Id: I4f176a919bf7181239d6ebb31e7870f12364e0f9
Reviewed-on: https://boringssl-review.googlesource.com/28744
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This allows consumers not to use crypto_test_data.cc (which embeds all
the test files), although they'll have to provide their own
implementation of that functionality.
Change-Id: I309d5b3bd9495137e1df788b34048794b0072f3b
Reviewed-on: https://boringssl-review.googlesource.com/28706
Reviewed-by: David Benjamin <davidben@google.com>
|alloca| is dangerous and poorly specified, according to any
description of |alloca|. It's also hard for some analysis tools to
reason about.
The code here assumed |alloca| is a macro, which isn't a valid
assumption. Depending on what which headers are included and what
toolchain is being used, |alloca| may or may not be defined as a macro,
and this might change over time if/when toolchains are updated. Or, we
might be doing static analysis and/or dynamic analysis with a different
configuration w.r.t. the availability of |alloca| than production
builds use.
Regardless, the |alloca| code path only kicked in when the inputs are
840 bits or smaller. Since the multi-prime RSA support was removed, for
interesting RSA key sizes the input will be at least 1024 bits and this
code path won't be triggered since powerbufLen will be larger than 3072
bytes in those cases. ECC inversion via Fermat's Little Theorem has its
own constant-time exponentiation so there are no cases where smaller
inputs need to be fast.
The RSAZ code avoids the |OPENSSL_malloc| for 2048-bit RSA keys.
Increasingly the RSAZ code won't be used though, since it will be
skipped over on Broadwell+ CPUs. Generalize the RSAZ stack allocation
to work for non-RSAZ code paths. In order to ensure this doesn't cause
too much stack usage on platforms where RSAZ wasn't already being used,
only do so on x86-64, which already has this large stack size
requirement due to RSAZ.
This change will make it easier to refactor |BN_mod_exp_mont_consttime|
to do that more safely and in a way that's more compatible with various
analysis tools.
This is also a step towards eliminating the |uintptr_t|-based alignment
hack.
Since this change increases the number of times |OPENSSL_free| is
skipped, I've added an explicit |OPENSSL_cleanse| to ensure the
zeroization is done. This should be done regardless of the other changes
here.
Change-Id: I8a161ce2720a26127e85fff7513f394883e50b2e
Reviewed-on: https://boringssl-review.googlesource.com/28584
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
OpenSSL staples each certificate's friendly name to the X509 with
X509_alias_set1. Mimic this. pyOpenSSL expects to find it there.
Update-Note: We actually parse some attributes now. PKCS#12 files with
malformed ones may not parse.
Change-Id: I3b78958eedf195509cd222ea4f0c884be3753770
Reviewed-on: https://boringssl-review.googlesource.com/28551
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
PKCS#12 encodes passwords as NUL-terminated UCS-2, so the empty password
is encoded as {0, 0}. Some implementations use the empty byte array for
"no password". OpenSSL considers a non-NULL password as {0, 0} and a
NULL password as {}. It then, in high-level PKCS#12 parsing code, tries
both options.
Match this behavior to appease pyOpenSSL's tests.
Change-Id: I07ef91d54454b6f2647f86b7eb9b13509b2876d3
Reviewed-on: https://boringssl-review.googlesource.com/28550
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
These are tied to OPENSSL_NO_OCSP in upstream but do not actually depend
on most of the OCSP machinery. The CRL invdate extension, in particular,
isn't associated with OCSP at all. cryptography.io gets upset if these
two extensions aren't parseable, and they're tiny.
I do not believe this actually affects anything beyond functions like
X509_get_ext_d2i. In particular, the list of NIDs for the criticality
check is elsewhere.
Change-Id: I889f6ebf4ca4b34b1d9ff15f45e05878132826a1
Reviewed-on: https://boringssl-review.googlesource.com/28549
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Rather than have plain-C functions, asm functions, and accelerated
functions, just have accelerated and non-accelerated, where the latter
are either provided by assembly or by C code.
Pertinently, this allows Aarch64 to use hardware accel for the basic
|AES_*| functions.
Change-Id: I0003c0c7a43d85a3eee8c8f37697f61a3070dd40
Reviewed-on: https://boringssl-review.googlesource.com/28385
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
cryptography.io wants RSA_R_BLOCK_TYPE_IS_NOT_02, only used by the
ancient RSA_padding_check_SSLv23 function. Define it but never emit it.
Additionally, it's rather finicky about RSA_R_TOO_LARGE* errors. We
merged them in BoringSSL because having RSA_R_TOO_LARGE,
RSA_R_TOO_LARGE_FOR_MODULUS, and RSA_R_TOO_LARGE_FOR_KEY_SIZE is a
little silly. But since we don't expect well-behaved code to condition
on error codes anyway, perhaps that wasn't worth it. Split them back
up.
Looking through OpenSSL, there is a vague semantic difference:
RSA_R_DIGEST_TOO_BIG_FOR_RSA_KEY - Specifically emitted if a digest is
too big for PKCS#1 signing with this key.
RSA_R_DATA_TOO_LARGE_FOR_KEY_SIZE - You asked me to sign or encrypt a
digest/plaintext, but it's too big for this key.
RSA_R_DATA_TOO_LARGE_FOR_MODULUS - You gave me an RSA ciphertext or
signature and it is not fully reduced modulo N.
-OR-
The padding functions produced something that isn't reduced, but I
believe this is unreachable outside of RSA_NO_PADDING.
RSA_R_DATA_TOO_LARGE - Some low-level padding function was told to copy
a digest/plaintext into some buffer, but the buffer was too small. I
think this is basically unreachable.
-OR-
You asked me to verify a PSS signature, but I didn't need to bother
because the digest/salt parameters you picked were too big.
Update-Note: This depends on cl/196566462.
Change-Id: I2e539e075eff8bfcd52ccde365e975ebcee72567
Reviewed-on: https://boringssl-review.googlesource.com/28547
Reviewed-by: Adam Langley <agl@google.com>
If the callback returns an empty ALPN, we forget we negotiated ALPN at
all (bssl::Array does not distinguish null and empty). Empty ALPN
protocols are forbidden anyway, so reject these ahead of time.
Change-Id: I42f1fc4c843bc865e23fb2a2e5d57424b569ee99
Reviewed-on: https://boringssl-review.googlesource.com/28546
Reviewed-by: Adam Langley <agl@google.com>
PyOpenSSL's tests expect all of the outputs to be distinct. OpenSSL also
tends to prefix the return values with strings like "compiler:", so do
something similar.
Change-Id: Ic411c95a276b477641ebad803ac309b3035c1b13
Reviewed-on: https://boringssl-review.googlesource.com/28544
Reviewed-by: Adam Langley <agl@google.com>
cryptography.io depends on this. Specifically, it assumes that any time
a CBC-mode cipher is defined, CMAC is also defined. This is incorrect;
CMAC also requires an irreducible polynomial to represent GF(2^b).
However, one is indeed defined for 64-bit block ciphers such as 3DES.
Import tests from CAVP to test it. I've omitted the 65536-byte inputs
because they're huge and FileTest doesn't like lines that long.
Change-Id: I35b1e4975f61c757c70616f9b372b91746fc7e4a
Reviewed-on: https://boringssl-review.googlesource.com/28466
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This was reverted a second time because it ended up always setting the
final argument to CRYPTO_gcm128_init to zero, which disabled some
acceleration of GCM on ≥Haswell. With this update, that argument will be
set to 1 if |aes_hw_*| functions are being used.
Probably this will need to be reverted too for some reason. I'm hoping
to fill the entire git short description with “Revert”.
Change-Id: Ib4a06f937d35d95affdc0b63f29f01c4a8c47d03
Reviewed-on: https://boringssl-review.googlesource.com/28484
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is so Chromium can verify the session before offering it, rather
than doing it after the handshake (at which point it's too late to punt
the session) as we do today. This should, in turn, allow us to finally
verify certificates off a callback and order it correctly relative to
CertificateRequest in TLS 1.3.
(It will also order "correctly" in TLS 1.2, but this is useless. TLS 1.2
does not bind the CertificateRequest to the certificate at the point the
client needs to act on it.)
Bug: chromium:347402
Change-Id: I0daac2868c97b820aead6c3a7e4dc30d8ba44dc4
Reviewed-on: https://boringssl-review.googlesource.com/28405
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Previously, we'd omitted OpenSSL's OCSP APIs because they depend on a
complex OCSP mechanism and encourage the the unreliable server behavior
that hampers using OCSP stapling to fix revocation today. (OCSP
responses should not be fetched on-demand on a callback. They should be
managed like other server credentials and refreshed eagerly, so
temporary CA outage does not translate to loss of OCSP.)
But most of the APIs are byte-oriented anyway, so they're easy to
support. Intentionally omit the one that takes a bunch of OCSP_RESPIDs.
The callback is benign on the client (an artifact of OpenSSL reading
OCSP and verifying certificates in the wrong order). On the server, it
encourages unreliability, but pyOpenSSL/cryptography.io depends on this.
Dcument that this is only for compatibility with legacy software.
Also tweak a few things for compatilibility. cryptography.io expects
SSL_CTX_set_read_ahead to return something, SSL_get_server_tmp_key's
signature was wrong, and cryptography.io tries to redefine
SSL_get_server_tmp_key if SSL_CTRL_GET_SERVER_TMP_KEY is missing.
Change-Id: I2f99711783456bfb7324e9ad972510be8a95e845
Reviewed-on: https://boringssl-review.googlesource.com/28404
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
PyOpenSSL calls this function these days. Tested by roundtripping with
ourselves and also manually confirming our output interoperates with
OpenSSL. (For anyone repeating this experiment, the OpenSSL
command-line tool has a bug and does not correctly output friendlyName
attributes with non-ASCII characters. I'll send them a PR to fix this
shortly.)
Between this and the UTF-8 logic earlier, the theme of this patch series
seems to be "implement in C something I last implemented in
JavaScript"...
Change-Id: I258d563498d82998c6bffc6789efeaba36fe3a5e
Reviewed-on: https://boringssl-review.googlesource.com/28328
Reviewed-by: Adam Langley <agl@google.com>
This is not very useful without PKCS12_create, which a follow-up change
will implement.
Change-Id: I355ccd22a165830911ae189871ab90a6101f42ae
Reviewed-on: https://boringssl-review.googlesource.com/28327
Reviewed-by: Adam Langley <agl@google.com>
This aligns with OpenSSL 1.1.0's behavior, which deviated from OpenSSL
1.0.2. OpenSSL 1.0.2 effectively assumed input passwords were always
Latin-1.
Update-Note: If anyone was using PKCS#12 passwords with non-ASCII
characters, this changes them from being encoding-confused to hopefully
interpretting "correctly". If this breaks anything, we can add a
fallback to PKCS12_get_key_and_certs/PKCS12_parse, but OpenSSL 1.1.0
does not have such behavior. It only implements a fallback in the
command-line tool, not the APIs.
Change-Id: I0aa92db26077b07a40f85b89f4d3e0f6b0d7be87
Reviewed-on: https://boringssl-review.googlesource.com/28326
Reviewed-by: Adam Langley <agl@google.com>
Update-Note: This changes causes BoringSSL to be stricter about handling
Unicode strings:
· Reject code points outside of Unicode
· Reject surrogate values
· Don't allow invalid UTF-8 to pass through when the source claims to
be UTF-8 already.
· Drop byte-order marks.
Previously, for example, a UniversalString could contain a large-valued
code point that would cause the UTF-8 encoder to emit invalid UTF-8.
Change-Id: I94d9db7796b70491b04494be84249907ff8fb46c
Reviewed-on: https://boringssl-review.googlesource.com/28325
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Build (and carry) issues are now resolved (as far as we know). Let's try
this again...
Measurements on a Skylake VM (so a little noisy).
Before:
Did 3135 RSA 2048 signing operations in 3015866us (1039.5 ops/sec)
Did 89000 RSA 2048 verify (same key) operations in 3007271us (29594.9 ops/sec)
Did 66000 RSA 2048 verify (fresh key) operations in 3014363us (21895.2 ops/sec)
Did 324 RSA 4096 signing operations in 3004364us (107.8 ops/sec)
Did 23126 RSA 4096 verify (same key) operations in 3003398us (7699.9 ops/sec)
Did 21312 RSA 4096 verify (fresh key) operations in 3017043us (7063.9 ops/sec)
Did 31040 ECDH P-256 operations in 3024273us (10263.6 ops/sec)
Did 91000 ECDSA P-256 signing operations in 3019740us (30135.0 ops/sec)
Did 25678 ECDSA P-256 verify operations in 3046975us (8427.4 ops/sec)
After:
Did 3640 RSA 2048 signing operations in 3035845us (1199.0 ops/sec)
Did 129000 RSA 2048 verify (same key) operations in 3003691us (42947.2 ops/sec)
Did 105000 RSA 2048 verify (fresh key) operations in 3029935us (34654.2 ops/sec)
Did 510 RSA 4096 signing operations in 3014096us (169.2 ops/sec)
Did 38000 RSA 4096 verify (same key) operations in 3092814us (12286.5 ops/sec)
Did 34221 RSA 4096 verify (fresh key) operations in 3003817us (11392.5 ops/sec)
Did 38000 ECDH P-256 operations in 3061758us (12411.2 ops/sec)
Did 116000 ECDSA P-256 signing operations in 3001637us (38645.6 ops/sec)
Did 35100 ECDSA P-256 verify operations in 3023872us (11607.6 ops/sec)
Tested with Intel SDE.
Change-Id: Ib27c0d6012d14274e331ab03f958e5a0c8b7e885
Reviewed-on: https://boringssl-review.googlesource.com/28104
Reviewed-by: Adam Langley <agl@google.com>
These will be used for the PKCS#12 code and to replace some of the
crypto/asn1 logic. So far they support the ones implemented by
crypto/asn1, which are Latin-1, UCS-2 (ASN.1 BMPStrings can't go beyond
the BMP), UTF-32 (ASN.1 UniversalString) and UTF-8.
Change-Id: I3d5c0d964cc6f97c3a0a1e352c9dd7d8cc0d87f2
Reviewed-on: https://boringssl-review.googlesource.com/28324
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This also happens to make the AES_[en|de]crypt functions use AES-NI
(where available) on Intel.
Update-Note: this substantially changes how AES-NI is triggered. Worth running bssl speed (on both k8 and ppc), before and after, to confirm that there are no regressions.
Change-Id: I5f22c1975236bbc1633c24ab60d683bca8ddd4c3
Reviewed-on: https://boringssl-review.googlesource.com/28026
Reviewed-by: David Benjamin <davidben@google.com>
gRPC builds on Debian Jessie, which has GCC 4.9.2, and builds with
-Wtype-limits, which makes it warn about code intended for 64-bit
systems when building on 32-bit systems.
We have tried to avoid these issues with Clang previously by guarding
with “sizeof(size_t) > 4”, but this version of GCC isn't smart enough to
figure that out.
Change-Id: I800ceb3891436fa7c81474ede4b8656021568357
Reviewed-on: https://boringssl-review.googlesource.com/28247
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
On Windows, just switching the socket to blocking doesn't work. Instead,
switch the stdin half of the waiter to waiting for either socket write
or stdin read, depending on whether we're in the middle of trying to
write a buffer.
Change-Id: I81414898f0491e78e6ab5b28c12148a3909ec1e0
Reviewed-on: https://boringssl-review.googlesource.com/28167
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We forgot to do this in our original implementation on general ecosystem
grounds. It's also mandated starting draft-26.
Just to avoid unnecessary turbulence, since draft-23 is doomed to die
anyway, condition this on our draft-28 implementation. (We don't support
24 through 27.)
We'd actually checked this already on the Go side, but the spec wants a
different alert.
Change-Id: I0014cda03d7129df0b48de077e45f8ae9fd16976
Reviewed-on: https://boringssl-review.googlesource.com/28124
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
If the peer sends us one record that exceeds buffer, the socket will no
longer flag as readable, because data has been consumed, but SSL_read
should still be called to drain data. bssl would instead not notice and
only surface the data later on.
This can (currently) be reproduced by sending "HEAD / HTTP/1.1" to
www.google.com.
Change-Id: I73cdbe104ba6be56fc033429999e630f0eb852d8
Reviewed-on: https://boringssl-review.googlesource.com/28166
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
While |WaitForMultipleObjects| works for both sockets and stdin, the
latter is often a line-buffered console. The |HANDLE| is considered
readable if there are any console events available, but reading blocks
until a full line is available. (In POSIX, line buffering is implemented
in the kernel via termios, which is differently concerning, but does
mean |select| works as expected.)
So that |Wait| reflects final stdin read, we spawn a stdin reader thread
that writes to an in-memory buffer and signals a |WSAEVENT| to
coordinate with the socket. This is kind of silly, but it works.
I tried just writing it to a pipe, but it appears
|WaitForMultipleObjects| does not work on pipes!
Change-Id: I2bfa323fa91aad7d2035bb1fe86ee6f54b85d811
Reviewed-on: https://boringssl-review.googlesource.com/28165
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Rather than printing the SSL_ERROR_* constants, print the actual error.
This should be a bit more understandable. Debugging this also uncovered
some other issues on Windows:
- We were mixing up C runtime and Winsock errors, which are separate in
Windows.
- The thread local implementation interferes with WSAGetLastError due to
a quirk of TlsGetValue. This could affect other Windows consumers.
(Chromium uses a custom BIO, so it isn't affected.)
- SocketSetNonBlocking also interferes with WSAGetLastError.
- Listen for FD_CLOSE along with FD_READ. Connection close does not
signal FD_READ. (The select loop only barely works on Windows anyway
due to issues with stdin and line buffering, but if we take stdin out
of the equation, FD_CLOSE can be tested.)
Change-Id: Ia8d42b5ac39ebb3045d410dd768f83a3bb88b2cb
Reviewed-on: https://boringssl-review.googlesource.com/28186
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Rather than printing the SSL_ERROR_* constants, print the actual error.
This should be a bit more understandable. Debugging this also uncovered
some other issues on Windows:
- We were mixing up C runtime and Winsock errors, which are separate in
Windows.
- The thread local implementation interferes with WSAGetLastError due to
a quirk of TlsGetValue. This could affect other Windows consumers.
(Chromium uses a custom BIO, so it isn't affected.)
- SocketSetNonBlocking also interferes with WSAGetLastError.
- Listen for FD_CLOSE along with FD_READ. Connection close does not
signal FD_READ. (The select loop only barely works on Windows anyway
due to issues with stdin and line buffering, but if we take stdin out
of the equation, FD_CLOSE can be tested.)
Change-Id: If991259915acc96606a314fbe795fe6ea1e295e8
Reviewed-on: https://boringssl-review.googlesource.com/28125
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is done by adding two new tagged data types to the shim's
transcript: one for the serialized handoff, and another for the
serialized handback.
Then, the handshake driver in |TLSFuzzer| is modified to be able to
drive a handoff+handback sequence in the same way as was done for
testing: by swapping |BIO|s into additional |SSL| objects. (If a
particular transcript does not contain a serialized handoff, this is a
no-op.)
Change-Id: Iab23e4dc27959ffd3d444adc41d40a4274e83653
Reviewed-on: https://boringssl-review.googlesource.com/27204
Commit-Queue: Matt Braithwaite <mab@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The bug, courtesy of Wycheproof, is that AES key wrap requires the input
be at least two blocks, not one. This also matches the OpenSSL behavior
of those two APIs.
Update-Note: AES_wrap_key with in_len = 8 and AES_unwrap_key with
in_len = 16 will no longer work.
Change-Id: I5fc63ebc16920c2f9fd488afe8c544e0647d7507
Reviewed-on: https://boringssl-review.googlesource.com/27925
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Montgomery multiplication post-conditions in some of code paths were
formally non-constant time. Cache access pattern was result-neutral,
but a little bit asymmetric, which might have produced a signal [if
processor reordered load and stores at run-time].
(Imported from upstream's 774ff8fed67e19d4f5f0df2f59050f2737abab2a.)
Change-Id: I77443fb79242b77e704c34d69f1de9e3162e9538
Reviewed-on: https://boringssl-review.googlesource.com/27987
Reviewed-by: Adam Langley <agl@google.com>
|set| should be evaluated to determine whether to insert/append before
it is reused as a temporary variable.
When incrementing the |set| of X509_NAME_ENTRY, the inserted entry
should not be incremented.
Thanks to Ingo Schwarze for extensive debugging and the initial
fix.
(Imported from upstream bbf27cd58337116c57a1c942153330ff83d5540a)
Change-Id: Ib45d92fc6d52d7490b01d3c475eafc42dd6ef721
Reviewed-on: https://boringssl-review.googlesource.com/28005
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
https://boringssl-review.googlesource.com/27944 inadvertently caused
SHA256 and SHA384 aliases to be rejected in
SSL_CTX_set_strict_cipher_list. While this is the desired end state, in
case the removal needs to be reverted, we should probably defer this to
post-removal cleanup.
Otherwise we might update someone's "ALL:!SHA256" cipher string to
account for the removal, and then revert the removal underneath them.
Change-Id: Id516a27a2ecefb5871485d0ae18067b5bbb536bb
Reviewed-on: https://boringssl-review.googlesource.com/28004
Reviewed-by: Adam Langley <agl@google.com>
Per Brian, x25519_ge_frombytes_vartime does not match the usual
BoringSSL return value convention, and we're slightly inconsistent about
whether to mask the last byte with 63 or 127. (It then gets ANDed with
64, so it doesn't matter which.) Use 127 to align with the curve25519
RFC. Finally, when we invert the transformation, use the same constants
inverted so that they're parallel.
Bug: 243, 244
Change-Id: I0e3aca0433ead210446c58d86b2f57526bde1eac
Reviewed-on: https://boringssl-review.googlesource.com/27984
Reviewed-by: Adam Langley <agl@google.com>
All CBC ciphers in TLS are broken and insecure. TLS 1.2 introduced
AEAD-based ciphers which avoid their many problems. It also introduced
new CBC ciphers based on HMAC-SHA256 and HMAC-SHA384 that share the same
flaws as the original HMAC-SHA1 ones. These serve no purpose. Old
clients don't support them, they have the highest overhead of all TLS
ciphers, and new clients can use AEADs anyway.
Remove them from libssl. This is the smaller, more easily reverted
portion of the removal. If it survives a week or so, we can unwind a lot
more code elsewhere in libcrypto. This removal will allow us to clear
some indirect calls from crypto/cipher_extra/tls_cbc.c, aligning with
the recommendations here:
https://github.com/HACS-workshop/spectre-mitigations/blob/master/crypto_guidelines.md#2-avoid-indirect-branches-in-constant-time-code
Update-Note: The following cipher suites are removed:
- TLS_RSA_WITH_AES_128_CBC_SHA256
- TLS_RSA_WITH_AES_256_CBC_SHA256
- TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256
- TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384
- TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256
- TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384
Change-Id: I7ade0fc1fa2464626560d156659893899aab6f77
Reviewed-on: https://boringssl-review.googlesource.com/27944
Reviewed-by: Adam Langley <agl@google.com>
Chrome needs to support renegotiation at TLS 1.2 + HTTP/1.1, but we're
free to shed the handshake configuration at TLS 1.3 or HTTP/2.
Rather than making config shedding implicitly disable renegotiation,
make the actual shedding dependent on a combination of the two settings.
If config shedding is enabled, but so is renegotiation (including
whether we are a client, etc.), leave the config around. If the
renegotiation setting gets disabled again after the handshake,
re-evaluate and shed the config then.
Bug: 123
Change-Id: Ie833f413b3f15b8f0ede617991e3fef239d4a323
Reviewed-on: https://boringssl-review.googlesource.com/27904
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Matt Braithwaite <mab@google.com>
|SSL_CONFIG| is a container for bits of configuration that are
unneeded after the handshake completes. By default it is retained for
the life of the |SSL|, but it may be shed at the caller's option by
calling SSL_set_shed_handshake_config(). This is incompatible with
renegotiation, and with SSL_clear().
|SSL_CONFIG| is reachable by |ssl->config| and by |hs->config|. The
latter is always non-NULL. To avoid null checks, I've changed the
signature of a number of functions from |SSL*| arguments to
|SSL_HANDSHAKE*| arguments.
When configuration has been shed, setters that touch |SSL_CONFIG|
return an error value if that is possible. Setters that return |void|
do nothing.
Getters that request |SSL_CONFIG| values will fail with an |assert| if
the configuration has been shed. When asserts are compiled out, they
will return an error value.
The aim of this commit is to simplify analysis of split-handshakes by
making it obvious that some bits of state have no effects beyond the
handshake. It also cuts down on memory usage.
Of note: |SSL_CTX| is still reachable after the configuration has been
shed, and a couple things need to be retained only for the sake of
post-handshake hooks. Perhaps these can be fixed in time.
Change-Id: Idf09642e0518945b81a1e9fcd7331cc9cf7cc2d6
Bug: 123
Reviewed-on: https://boringssl-review.googlesource.com/27644
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
This is prefactoring for a coming change to the shim that will write
handoff and handback messages (which are serialized SSLConnection
objects) to the transcript.
This breaks the slightly tenuous ordering between the runner and the
shim. Fix the runner to wait until the shim has exited before
appending the transcript.
Change-Id: Iae34d28ec1addfe3ec4f3c77008248fe5530687c
Reviewed-on: https://boringssl-review.googlesource.com/27184
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is slower, but constant-time. It intentionally omits the signed
digit optimization because we cannot be sure the doubling case will be
unreachable for all curves. This is a fallback generic implementation
for curves which we must support for compatibility but which are not
common or important enough to justify curve-specific work.
Before:
Did 814 ECDH P-384 operations in 1085384us (750.0 ops/sec)
Did 1430 ECDSA P-384 signing operations in 1081988us (1321.6 ops/sec)
Did 308 ECDH P-521 operations in 1057741us (291.2 ops/sec)
Did 539 ECDSA P-521 signing operations in 1049797us (513.4 ops/sec)
After:
Did 715 ECDH P-384 operations in 1080161us (661.9 ops/sec)
Did 1188 ECDSA P-384 verify operations in 1069567us (1110.7 ops/sec)
Did 275 ECDH P-521 operations in 1060503us (259.3 ops/sec)
Did 506 ECDSA P-521 signing operations in 1084739us (466.5 ops/sec)
But we're still faster than the old BIGNUM implementation. EC_FELEM
more than paid for both the loss of points_make_affine and this CL.
Bug: 239
Change-Id: I65d71a731aad16b523928ee47618822d503ea704
Reviewed-on: https://boringssl-review.googlesource.com/27708
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
EC_POINT is split into the existing public EC_POINT (where the caller is
sanity-checked about group mismatches) and the low-level EC_RAW_POINT
(which, like EC_FELEM and EC_SCALAR, assume that is your problem and is
a plain old struct). Having both EC_POINT and EC_RAW_POINT is a little
silly, but we're going to want different type signatures for functions
which return void anyway (my plan is to lift a non-BIGNUM
get_affine_coordinates up through the ECDSA and ECDH code), so I think
it's fine.
This wasn't strictly necessary, but wnaf.c is a lot tidier now. Perf is
a wash; once we get up to this layer, it's only 8 entries in the table
so not particularly interesting.
Bug: 239
Change-Id: I8ace749393d359f42649a5bb0734597bb7c07a2e
Reviewed-on: https://boringssl-review.googlesource.com/27706
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Replace them with asserts and better justify why each of the internal
cases are not reachable. Also change the loop to count up to bits+1 so
it is obvious there is no memory error. (The previous loop shape made
more sense when ec_compute_wNAF would return a variable length
schedule.)
Change-Id: I9c7df6abac4290b7a3e545e3d4aa1462108e239e
Reviewed-on: https://boringssl-review.googlesource.com/27705
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
FileTest and Wycheproof express more-or-less the same things, so I've
just written a script to mechanically convert them. Saves writing a JSON
parser.
I've also left a TODO with other files that are worth converting. Per
Thai, the webcrypto variants of the files are just a different format
and will later be consolidated, so I've ignored those. The
curve/hash-specific ECDSA files and the combined one are intended to be
the same, so I've ignored the combined one. (Just by test counts, there
are some discrepancies, but Thai says he'll fix that and we can update
when that happens.)
Change-Id: I5fcbd5cb0e1bea32964b09fb469cb43410f53c2d
Reviewed-on: https://boringssl-review.googlesource.com/27785
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This is just a pristine copy of the JSON files for now. It's not hooked
up to anything yet.
Change-Id: I608b4b0368578f159cad23950d70578ff4c23da3
Reviewed-on: https://boringssl-review.googlesource.com/27784
Reviewed-by: Adam Langley <agl@google.com>
This introduces EC_FELEM, which is analogous to EC_SCALAR. It is used
for EC_POINT's representation in the generic EC_METHOD, as well as
random operations on tuned EC_METHODs that still are implemented
genericly.
Unlike EC_SCALAR, EC_FELEM's exact representation is awkwardly specific
to the EC_METHOD, analogous to how the old values were BIGNUMs but may
or may not have been in Montgomery form. This is kind of a nuisance, but
no more than before. (If p224-64.c were easily convertable to Montgomery
form, we could say |EC_FELEM| is always in Montgomery form. If we
exposed the internal add and double implementations in each of the
curves, we could give |EC_POINT| an |EC_METHOD|-specific representation
and |EC_FELEM| is purely a |EC_GFp_mont_method| type. I'll leave this
for later.)
The generic add and doubling formulas are aligned with the formulas
proved in fiat-crypto. Those only applied to a = -3, so I've proved a
generic one in https://github.com/mit-plv/fiat-crypto/pull/356, in case
someone uses a custom curve. The new formulas are verified,
constant-time, and swap a multiply for a square. As expressed in
fiat-crypto they do use more temporaries, but this seems to be fine with
stack-allocated EC_FELEMs. (We can try to help the compiler later,
but benchamrks below suggest this isn't necessary.)
Unlike BIGNUM, EC_FELEM can be stack-allocated. It also captures the
bounds in the type system and, in particular, that the width is correct,
which will make it easier to select a point in constant-time in the
future. (Indeed the old code did not always have the correct width. Its
point formula involved halving and implemented this in variable time and
variable width.)
Before:
Did 77274 ECDH P-256 operations in 10046087us (7692.0 ops/sec)
Did 5959 ECDH P-384 operations in 10031701us (594.0 ops/sec)
Did 10815 ECDSA P-384 signing operations in 10087892us (1072.1 ops/sec)
Did 8976 ECDSA P-384 verify operations in 10071038us (891.3 ops/sec)
Did 2600 ECDH P-521 operations in 10091688us (257.6 ops/sec)
Did 4590 ECDSA P-521 signing operations in 10055195us (456.5 ops/sec)
Did 3811 ECDSA P-521 verify operations in 10003574us (381.0 ops/sec)
After:
Did 77736 ECDH P-256 operations in 10029858us (7750.5 ops/sec) [+0.8%]
Did 7519 ECDH P-384 operations in 10068076us (746.8 ops/sec) [+25.7%]
Did 13335 ECDSA P-384 signing operations in 10029962us (1329.5 ops/sec) [+24.0%]
Did 11021 ECDSA P-384 verify operations in 10088600us (1092.4 ops/sec) [+22.6%]
Did 2912 ECDH P-521 operations in 10001325us (291.2 ops/sec) [+13.0%]
Did 5150 ECDSA P-521 signing operations in 10027462us (513.6 ops/sec) [+12.5%]
Did 4264 ECDSA P-521 verify operations in 10069694us (423.4 ops/sec) [+11.1%]
This more than pays for removing points_make_affine previously and even
speeds up ECDH P-256 slightly. (The point-on-curve check uses the
generic code.)
Next is to push the stack-allocating up to ec_wNAF_mul, followed by a
constant-time single-point multiplication.
Bug: 239
Change-Id: I44a2dff7c52522e491d0f8cffff64c4ab5cd353c
Reviewed-on: https://boringssl-review.googlesource.com/27668
Reviewed-by: Adam Langley <agl@google.com>
This does not appear to actually pull its weight. The purpose of this
logic is to switch some adds to the faster add_mixed in the wNAF code,
at the cost of a rather expensive inversion. This optimization kicks in
for generic curves, so P-384 and P-521:
With:
Did 32130 ECDSA P-384 signing operations in 30077563us (1068.2 ops/sec)
Did 27456 ECDSA P-384 verify operations in 30073086us (913.0 ops/sec)
Did 14122 ECDSA P-521 signing operations in 30077407us (469.5 ops/sec)
Did 11973 ECDSA P-521 verify operations in 30037330us (398.6 ops/sec)
Without:
Did 32445 ECDSA P-384 signing operations in 30069721us (1079.0 ops/sec)
Did 27056 ECDSA P-384 verify operations in 30032303us (900.9 ops/sec)
Did 13905 ECDSA P-521 signing operations in 30000430us (463.5 ops/sec)
Did 11433 ECDSA P-521 verify operations in 30021876us (380.8 ops/sec)
For single-point multiplication, the optimization is not useful. This
makes sense as we only have one table's worth of additions to convert
but still pay for the inversion. For double-point multiplication, it is
slightly useful for P-384 and very useful for P-521. However, the next
change to stack-allocate EC_FELEMs will more than compensate for
removing it. (The immediate goal here is to simplify the EC_FELEM
story.)
Additionally, that this optimization was not useful for single-point
multiplication implies that, should we wish to recover this, a modest
8-entry pre-computed (affine) base point table should have the same
effect or better.
Update-Note: I do not believe anything was calling either of these
functions. (If necessary, we can always add no-op stubs as whether a
point is affine is not visible to external code. It previously kicked in
some optimizations, but those were removed for constant-time needs
anyway.)
Bug: 239
Change-Id: Ic9c51b001c45595cfe592274c7d5d652f4234839
Reviewed-on: https://boringssl-review.googlesource.com/27667
Reviewed-by: Adam Langley <agl@google.com>
I don't think this lock is actually needed. If the process exited by the
time we call shim.Process.Kill(), then the test ultimately finished. If
not, wait() will return that the process died by a signal.
Change-Id: I668a86583aba16fd00e0cd05071acc13059a2c42
Reviewed-on: https://boringssl-review.googlesource.com/27325
Reviewed-by: Adam Langley <agl@google.com>
As the point may be the output of some private key operation, whether Z
accidentally hit one is secret.
Bug: 239
Change-Id: I7db34cd3b5dd5ca4b96980e8993a9b4eda49eb88
Reviewed-on: https://boringssl-review.googlesource.com/27664
Reviewed-by: Adam Langley <alangley@gmail.com>
This is adapted from upstream's
eb7916960bf50f436593abe3d5f2e0592d291017.
This gives a 22% win for ECDSA signing. (Upstream cites 30-40%, but they
are unnecessarily using BN_mod_exp_mont_consttime in their generic path.
The exponent is public. I expect part of their 30-40% is just offsetting
this.)
Did 506000 ECDSA P-256 signing operations in 25044595us (20204.0 ops/sec)
Did 170506 ECDSA P-256 verify operations in 25033567us (6811.1 ops/sec)
Did 618000 ECDSA P-256 signing operations in 25031294us (24689.1 ops/sec)
Did 182240 ECDSA P-256 verify operations in 25006918us (7287.6 ops/sec)
Most of the performance win appears to be from the assembly operations
and not the addition chain. I have a CL to graft the addition chain onto
the C implementation, but it did not show measurable improvement in
ECDSA verify. ECDSA sign gets 2-4% faster, but we're more concerned
about ECDSA verify in the OPENSSL_SMALL builds.
Change-Id: Ide166f98b146c025f7f80ed7906336c16818540a
Reviewed-on: https://boringssl-review.googlesource.com/27593
Reviewed-by: Adam Langley <alangley@gmail.com>
This imports the assembly portion of
eb7916960bf50f436593abe3d5f2e0592d291017 from upstream. Note the
OPENSSL_ia32cap_P bits were tweaked to be delocate-compatible. Those
should be reviewed against the original file.
Change-Id: I19eef722225bb7928275e3d93890f80aa2f8734d
Reviewed-on: https://boringssl-review.googlesource.com/27589
Reviewed-by: Adam Langley <alangley@gmail.com>
We were still using the allocating scalar inversion for ECDSA verify
because previously it seemed to be faster. It appears to have flipped
now, though probably was always just a wash.
While I'm here, save a multiplication by swapping the inversion and
Montgomery reduction.
Did 200000 ECDSA P-256 signing operations in 10025749us (19948.6 ops/sec)
Did 66234 ECDSA P-256 verify operations in 10061123us (6583.2 ops/sec)
Did 202000 ECDSA P-256 signing operations in 10020846us (20158.0 ops/sec)
Did 68052 ECDSA P-256 verify operations in 10020592us (6791.2 ops/sec)
The actual motivation is to get rid of the unchecked EC_SCALAR function
and align sign/verify in preparation for the assembly scalar ops.
Change-Id: I1bd3a5719a67966dc8edaa43535a3864b69f76d0
Reviewed-on: https://boringssl-review.googlesource.com/27588
Reviewed-by: Adam Langley <alangley@gmail.com>
No sense in adding impossible error cases we need to handle.
Additionally, tighten them a bit and require strong bounds. (I wasn't
sure what we'd need at first and made them unnecessarily general.)
Change-Id: I21a0afde90a55be2e9a0b8d7288f595252844f5f
Reviewed-on: https://boringssl-review.googlesource.com/27586
Reviewed-by: Adam Langley <alangley@gmail.com>
This is so the *_small functions can assume somewhat more uniform
widths, to simplify their error-handling.
Change-Id: I0420cb237084b253e918c64b0c170a5dfd99ab40
Reviewed-on: https://boringssl-review.googlesource.com/27584
Reviewed-by: Adam Langley <alangley@gmail.com>
The FIPS 186-4 algorithm we use includes a limit which hits a 2^-20
failure probability, assuming my math is right. We've observed roughly
2^-23. This is a little large at scale. (See b/77854769.)
To avoid modifying the FIPS algorithm, retry the whole thing four times
to bring the failure rate down to 2^-80. Along the way, now that I have
the derivation on hand, adjust
https://boringssl-review.googlesource.com/22584 to target the same
failure probability.
Along the way, fix an issue with RSA_generate_key where, if callers
don't check for failure, there may be half a key in there.
Change-Id: I0e1da98413ebd4ffa65fb74c67a58a0e0cd570ff
Reviewed-on: https://boringssl-review.googlesource.com/27288
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
https://boringssl-review.googlesource.com/10520 and then later
https://boringssl-review.googlesource.com/25285 made BN_MONT_CTX_set
constant-time, which is necessary for RSA's mont_p and mont_q. However,
due to a typo in the benchmark, they did not correctly measure.
Split BN_MONT_CTX creation into a constant-time and variable-time one.
The constant-time one uses our current algorithm and the latter restores
the original BN_mod codepath.
Should we wish to avoid BN_mod, I have an alternate version lying
around:
First, BN_set_bit + bn_mod_lshift1_consttime as now to count up to 2*R.
Next, observe that 2*R = BN_to_montgomery(2) and R*R =
BN_to_montgomery(R) = BN_to_montgomery(2^r_bits) Also observe that
BN_mod_mul_montgomery only needs n0, not RR. Split the core of
BN_mod_exp_mont into its own function so the caller handles conversion.
Raise 2*R to the r_bits power to get 2^r_bits*R = R*R.
The advantage of that algorithm is that it is still constant-time, so we
only need one BN_MONT_CTX_new. Additionally, it avoids BN_mod which is
otherwise (almost, but the remaining links should be easy to cut) out of
the critical path for correctness. One less operation to worry about.
The disadvantage is that it is gives a 25% (RSA-2048) or 32% (RSA-4096)
slower RSA verification speed. I went with the BN_mod one for the time
being.
Before:
Did 9204 RSA 2048 signing operations in 10052053us (915.6 ops/sec)
Did 326000 RSA 2048 verify (same key) operations in 10028823us (32506.3 ops/sec)
Did 50830 RSA 2048 verify (fresh key) operations in 10033794us (5065.9 ops/sec)
Did 1269 RSA 4096 signing operations in 10019204us (126.7 ops/sec)
Did 88435 RSA 4096 verify (same key) operations in 10031129us (8816.1 ops/sec)
Did 14552 RSA 4096 verify (fresh key) operations in 10053411us (1447.5 ops/sec)
After:
Did 9150 RSA 2048 signing operations in 10022831us (912.9 ops/sec)
Did 322000 RSA 2048 verify (same key) operations in 10028604us (32108.2 ops/sec)
Did 289000 RSA 2048 verify (fresh key) operations in 10017205us (28850.4 ops/sec)
Did 1270 RSA 4096 signing operations in 10072950us (126.1 ops/sec)
Did 87480 RSA 4096 verify (same key) operations in 10036328us (8716.3 ops/sec)
Did 80730 RSA 4096 verify (fresh key) operations in 10073614us (8014.0 ops/sec)
Change-Id: Ie8916d1634ccf8513ceda458fa302f09f3e93c07
Reviewed-on: https://boringssl-review.googlesource.com/27287
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The first non-zero window (which we can condition on for public
exponents) always multiplies by one. This means we can cut out one
Montgomery multiplication. It also means we never actually need to
initialize r to one, saving another Montgomery multiplication for P-521.
This, in turn, means we don't need the bn_one_to_montgomery optimization
for the public-exponent exponentations, so we can delete
bn_one_to_montgomery_small. (The function does currently promise to
handle p = 0, but this is not actually reachable, so it can just do a
reduction on RR.)
For RSA, where we're not doing many multiplications to begin with,
saving one is noticeable.
Before:
Did 92000 RSA 2048 verify (same key) operations in 3002557us (30640.6 ops/sec)
Did 25165 RSA 4096 verify (same key) operations in 3045046us (8264.2 ops/sec)
After:
Did 100000 RSA 2048 verify (same key) operations in 3002483us (33305.8 ops/sec)
Did 26603 RSA 4096 verify (same key) operations in 3010942us (8835.4 ops/sec)
(Not looking at the fresh key number yet as that still needs to be
fixed.)
Change-Id: I81a025a68d9b0f8eb0f9c6c04ec4eedf0995a345
Reviewed-on: https://boringssl-review.googlesource.com/27286
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
It's defined to return one in Montgomery form, not a normal one.
(Not that this matters. This function is only used to Fermat's Little
Theorem. Probably it should have been less general, though we'd need to
make new test vectors first.)
Change-Id: Ia8d7588e6a413b25f01280af9aacef0192283771
Reviewed-on: https://boringssl-review.googlesource.com/27285
Reviewed-by: Adam Langley <agl@google.com>
BN_mod_exp_mont is intended to protect the base, but not the exponent.
Accordingly, it shouldn't treat a base of zero as special.
Change-Id: Ib053e8ce65ab1741973a9f9bfeff8c353567439c
Reviewed-on: https://boringssl-review.googlesource.com/27284
Reviewed-by: Adam Langley <agl@google.com>
Our technique to perform the reduction only works for balanced key
sizes. For unbalanced keys, we fall back to variable-time logic.
Instead, fall back earlier to the non-CRT codepath, which is still
secure, just slower. This also aligns with the advice here:
https://github.com/HACS-workshop/spectre-mitigations/blob/master/crypto_guidelines.md#1-do-not-conditionally-choose-between-constant-and-non-constant-time
Update-Note: This is a performance hit (some keys will run 3x slower),
but only for keys with different-sized primes. I believe the Windows
crypto APIs will not accept such keys at all. There are two scenarios to
be concerned with for RSA performance:
1. Performance of reasonably-generated keys. Keys that BoringSSL or
anyone else reasonable generates will all be balanced, so this change
does not affect them.
2. Worst-case performance for DoS purposes. This CL does not change the
worst-case performance for RSA at a given bit size. In fact, it improves
it slightly. A sufficiently unbalanced RSA key is as slow as not doing
CRT at all.
In both cases, this change does not affect performance. The affected
keys are pathologically-generated ones that were not quite pathological
enough.
Bug: 235
Change-Id: Ie298dabb549ab9108fa9374aa86ebffe8b6c6c88
Reviewed-on: https://boringssl-review.googlesource.com/27504
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
data_plus_mac_size is secret. Values derived from it cannot quite be
safely divided by md_block_size because SHA-384 ciphers prevent that
field from being constant. We know the value is a power of two, so do
the strength reduction by hand.
Change-Id: Id62ab9e646f4e21d507a7059cfe84d49bbb986e6
Reviewed-on: https://boringssl-review.googlesource.com/27505
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Chrome uses the platform certificate verifier and thus cannot reliably
expect PSS signatures to work in all configurations. Add an API for the
consumer to inform BoringSSL of this ability. We will then adjust our
advertisements accordingly.
Note that, because TLS 1.2 does not have the signature_algorithms_cert
extension, turning off TLS 1.3 and using this API will stop advertising
RSA-PSS. I believe this is the correct behavior given the semantics of
that code point.
The tests check the various combinations here, as well as checking that
the peer never sends signature_algorithms_cert identical to
signature_algorithms.
Bug: 229
Change-Id: I8c33a93efdc9252097e3899425b49548fc42a93a
Reviewed-on: https://boringssl-review.googlesource.com/27488
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Conscrypt need this function right now. They ought to be fixed up to not
need this but, in the meantime, this API is also provided by OpenSSL and
will clear one most consumer reaching into SSL_SESSION.
Bumping the API since Conscrypt often involves multi-sided stuff.
Change-Id: I665ca6b6a17ef479133c29c23fc639f278128c69
Reviewed-on: https://boringssl-review.googlesource.com/27405
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
In the handoff+handback case, bssl_shim.cc creates 3 |SSL| objects:
one to receive the ClientHello, one to receive the handoff, and a
third one to receive the handback.
Before 56986f9, only the first of these received any configuration.
Since that commit, all 3 of them receive the same configuration. That
means that the handback message no longer needs to serialize as many
things.
N.B. even before 56986f9, not all of the fields were necessary. For
example, there was no reason to serialize |conf_max_version| and
|conf_min_version| in the handback, so far as I can tell.
This commit is mechanical: it simply removes everything that doesn't
cause any tests to fail. In the long run, I'll need to carefully
check for two possibilities:
- Knobs that affect the handshake after the server's first message it
sent. These are troublesome because that portion of the handshake
may run on a different |SSL|, depending on whether the handback is
early or late.
- Getters that may be called post-handshake, and that callers may
reasonably expect to reflect the value that was used during
handshake.
(I'm not sure that either case exists!)
Change-Id: Ibf6e0be6609ad6e83ab50e69199e9b2d51e59a87
Reviewed-on: https://boringssl-review.googlesource.com/27364
Commit-Queue: Matt Braithwaite <mab@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
This separates the source lists for the crypto and ssl targets from
their headers, so the header files can be listed in the 'public'
section of the targets. This allows tighter GN checking and expresses
the build structure more cleanly.
Change-Id: Ifb20c90977d7e858734654d9a03949be19a9c43a
Reviewed-on: https://boringssl-review.googlesource.com/27344
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The last-minute TLS 1.3 change was done partly for consistency with DTLS
1.3, where authenticating the record header is less obviously pointless
than in TLS. There, reconstructing it would be messy. Instead, pass in
the record header and let SSLAEADContext decide whether or not to
assemble its own.
(While I'm here, reorder all the flags so the AD and nonce ones are
grouped together.)
Change-Id: I06e65d526b21a08019e5ca6f1b7c7e0e579e7760
Reviewed-on: https://boringssl-review.googlesource.com/27024
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
https://boringssl-review.googlesource.com/10522 didn't actually do what
it was supposed to do. In fact, it appears, not paying attention to it,
we've managed to make RSA verify slower than ECDSA verify. Oops.
Did 32000 RSA 2048 verify (same key) operations in 1016746us (31473.0 ops/sec)
Did 5525 RSA 2048 verify (fresh key) operations in 1067209us (5177.1 ops/sec)
Did 8957 ECDSA P-256 verify operations in 1078570us (8304.5 ops/sec)
The difference is in setting up the BN_MONT_CTX, either computing R^2 or n0.
I'm guessing R^2. The current algorithm needs to be constant-time, but we can
split out a variable-time one if necessary.
Change-Id: Ie064a0e464aaa803815b56a6734bc9e2becef1a7
Reviewed-on: https://boringssl-review.googlesource.com/27244
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is helpful at smaller sizes because the benefits of an unlikely hit
by trival-division are smaller.
The full set of kPrimes eliminates about 94.3% of random numbers. The
first quarter eliminates about 93.2% of them. But the little extra power
of the full set seems to be borderline for RSA 3072 and clearly positive
for RSA 4096.
Did 316 RSA 2048 key-gen operations in 30035598us (10.5 ops/sec)
min: 19423us, median: 80448us, max: 394265us
Change-Id: Iee53f721329674ae7a08fabd85b4f645c24e119d
Reviewed-on: https://boringssl-review.googlesource.com/26944
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
The generic code special-cases affine points, but this leaks
information. (Of course, the generic code also doesn't have a
constant-time multiply and other problems, but one thing at a time.)
The optimization in point doubling is not useful. Point multiplication
more-or-less never doubles an affine point. The optimization in point
addition *is* useful because the wNAF code converts the tables to
affine. Accordingly, align with the P-256 code which adds a 'mixed'
parameter.
(I haven't aligned the formally-verified point formulas themselves yet;
initial testing suggests that the large number of temporaries take a
perf hit with BIGNUM. I'll check the results in EC_FELEM, which will be
stack-allocated, to see if we still need to help the compiler out.)
Strangly, it actually got a bit faster with this change. I'm guessing
because now it doesn't need to bother with unnecessary comparisons and
maybe was kinder to the branch predictor?
Before:
Did 2201 ECDH P-384 operations in 3068341us (717.3 ops/sec)
Did 4092 ECDSA P-384 signing operations in 3076981us (1329.9 ops/sec)
Did 3503 ECDSA P-384 verify operations in 3024753us (1158.1 ops/sec)
Did 992 ECDH P-521 operations in 3017884us (328.7 ops/sec)
Did 1798 ECDSA P-521 signing operations in 3059000us (587.8 ops/sec)
Did 1581 ECDSA P-521 verify operations in 3033142us (521.2 ops/sec)
After:
Did 2310 ECDH P-384 operations in 3092648us (746.9 ops/sec)
Did 4080 ECDSA P-384 signing operations in 3044588us (1340.1 ops/sec)
Did 3520 ECDSA P-384 verify operations in 3056070us (1151.8 ops/sec)
Did 992 ECDH P-521 operations in 3012779us (329.3 ops/sec)
Did 1792 ECDSA P-521 signing operations in 3019459us (593.5 ops/sec)
Did 1600 ECDSA P-521 verify operations in 3047749us (525.0 ops/sec)
Bug: 239
Change-Id: If5d13825fc98e4c58bdd1580cf0245bf7ce93a82
Reviewed-on: https://boringssl-review.googlesource.com/27004
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This changes the contract for split handshakes such that on the
receiving side, the connection is to be driven until it returns
|SSL_ERROR_HANDBACK|, rather than until SSL_do_handshake() returns
success.
Change-Id: Idd1ebfbd943d88474d7c934f4c0ae757ff3c0f37
Reviewed-on: https://boringssl-review.googlesource.com/26864
Commit-Queue: Matt Braithwaite <mab@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The relevant result (Damgård, Landrock, and Pomerance, Average Case
Error Estimates for the Strong Probably Prime Test) is only applicable
for randomly selected candidates. It relies on there being very few odd
composites with many false witnesses.
(If testing an adversarially-selected composite, false witnesses are
bounded by ϕ(n)/4 for n != 9, so one needs about 40 iterations for a
2^-80 false positive rate.)
Change-Id: I2a063dac5f9042dcb9e6affee8d2ae575f2238a9
Reviewed-on: https://boringssl-review.googlesource.com/26972
Reviewed-by: Adam Langley <agl@google.com>
This is in preparation for representing field elements with
stack-allocated types in the generic code. While there is likely little
benefit in threading all the turned field arithmetic through all the
generic code, and the P-224 logic, in particular, does not have a tight
enough abstraction for this, the current implementations depend on
BN_div, which is not compatible with stack-allocating things and avoiding
malloc.
This also speeds things up slightly, now that benchmarks cover point
validation.
Before:
Did 82786 ECDH P-224 operations in 10024326us (8258.5 ops/sec)
After:
Did 89991 ECDH P-224 operations in 10012429us (8987.9 ops/sec)
Change-Id: I468483b49f5dc69187aebd62834365ce5caab795
Reviewed-on: https://boringssl-review.googlesource.com/26971
Reviewed-by: Adam Langley <agl@google.com>
Alas, it is reachable by way of the legacy custom curves API. Add a
basic test to ensure those codepaths work.
Change-Id: If631110045a664001133a0d07fdac4c67971a15f
Reviewed-on: https://boringssl-review.googlesource.com/26970
Reviewed-by: Adam Langley <agl@google.com>
This includes a point validation, which figures into the overall cost of
an ECDH operation. If, say, point validation is slow because it uses
generic code, we'd like it to show up in benchmarks.
(Later I'd like to replace this mess with a simple byte-oriented ECDH
API. When that happens, I'll update the benchmark accordingly.)
Change-Id: If8c33542d4b40572aac0a71ea2f658e7bc501f4b
Reviewed-on: https://boringssl-review.googlesource.com/26969
Reviewed-by: Adam Langley <agl@google.com>
ECDSA converts digests to scalars by taking the leftmost n bits, where n
is the number of bits in the group order. This does not necessarily
produce a fully-reduced scalar.
Montgomery multiplication actually tolerates this slightly looser bound,
so we did not bother with the conditional subtraction. However, this
subtraction is free compared to the multiplication, inversion, and base
point multiplication. Simplify things by keeping it fully-reduced.
Change-Id: If49dffefccc21510f40418dc52ea4da7e3ff198f
Reviewed-on: https://boringssl-review.googlesource.com/26968
Reviewed-by: Adam Langley <agl@google.com>
ECDSA's logic for converting digests to scalars sometimes produces
slightly unreduced values. Test these cases.
Change-Id: I67a5078db684ee82c286f41e71b13b57c3ee707b
Reviewed-on: https://boringssl-review.googlesource.com/26967
Reviewed-by: Adam Langley <agl@google.com>
May as well use it. Also avoid an overflow with digest_len if someone
asks to sign a truly enormous digest.
Change-Id: Ia0a53007a496f9c7cadd44b1020ec2774b310936
Reviewed-on: https://boringssl-review.googlesource.com/26966
Reviewed-by: Adam Langley <agl@google.com>
For non-custom curves, this only comes up with P-521 and, even then,
only with excessively large hashes. Still, we should have test coverage
for this.
Change-Id: Id17a6f47d59d6dd4a43a93857fd3df490f9fa965
Reviewed-on: https://boringssl-review.googlesource.com/26965
Reviewed-by: Adam Langley <agl@google.com>
We do this in four different places, with the same long comment, and I'm
about to add yet another one.
Change-Id: If28e3f87ea71020d9b07b92e8947f3848473d99d
Reviewed-on: https://boringssl-review.googlesource.com/26964
Reviewed-by: Adam Langley <agl@google.com>
RSA keygen uses this to pick primes. May as well avoid bouncing on
malloc. (The BIGNUM internally allocates, of course, but that allocation
will be absorbed by BN_CTX in RSA keygen.)
Change-Id: Ie2243a6e48b9c55f777153cbf67ba5c06688c2f1
Reviewed-on: https://boringssl-review.googlesource.com/26887
Reviewed-by: Adam Langley <agl@google.com>
With this, in 0.02% of 1024-bit primes (which is what's used with an RSA
2048 generation), we'll leak that we struggled to generate values less
than the prime. I.e. that there's a greater likelihood of zero bits
after the leading 1 bit in the prime.
But this recovers all the speed loss from making key generation
constant-time, and then some.
Did 273 RSA 2048 key-gen operations in 30023223us (9.1 ops/sec)
min: 23867us, median: 93688us, max: 421466us
Did 66 RSA 3072 key-gen operations in 30041763us (2.2 ops/sec)
min: 117044us, median: 402095us, max: 1096538us
Did 31 RSA 4096 key-gen operations in 31673405us (1.0 ops/sec)
min: 245109us, median: 769480us, max: 2659386us
Change-Id: Id82dedde35f5fbb36b278189c0685a13c7824590
Reviewed-on: https://boringssl-review.googlesource.com/26924
Reviewed-by: Adam Langley <alangley@gmail.com>
Windows CryptoAPI and Go bound public exponents at 2^32-1, so don't
generate keys which would violate that.
https://github.com/golang/go/issues/3161https://msdn.microsoft.com/en-us/library/aa387685(VS.85).aspx
BoringSSL itself also enforces a 33-bit limit.
I don't currently have plans to take much advantage of it, but the
modular inverse step and one of the GCDs in RSA key generation are
helped by small public exponents[0]. In case someone feels inspired
later, get this limit enforced now. Use 32-bits as that's a more
convenient limit, and there's no requirement to produce e=2^32+1 keys.
(Is there still a requirement to accept them?)
[0] This isn't too bad, but it's only worth it if it produces simpler or
smaller code. RSA keygen is not performance-critical.
1. Make bn_mod_u16_consttime work for uint32_t. It only barely doesn't
work. Maybe only accept 3 and 65537 and pre-compute, maybe call into
bn_div_rem_words and friends, maybe just tighten the bound a hair
longer.
2. Implement bn_div_u32_consttime by incorporating 32-bit chunks much
like bn_mod_u32_consttime.
3. Perform one normal Euclidean algorithm iteration rather than using the
binary version. u, v, B, and D are now single words, while A and C
are full-width.
4. Continue with binary Euclidean algorithm (u and v are still secret),
taking advantage of most values being small.
Update-Note: RSA_generate_key_ex will no longer generate keys with
public exponents larger than 2^32-1. Everyone uses 65537, save some
folks who use 3, so this shouldn't matter.
Change-Id: I0d28a29a30d9ff73bff282e34dd98e2b64c35c79
Reviewed-on: https://boringssl-review.googlesource.com/26365
Reviewed-by: Adam Langley <alangley@gmail.com>
We don't check it is fully reduced because different implementations use
Carmichael vs Euler totients, but if d exceeds n, something is wrong.
Note the fixed-width BIGNUM changes already fail operations with
oversized d.
Update-Note: Some blatantly invalid RSA private keys will be rejected at
RSA_check_key time. Note that most of those keys already are not
usable with BoringSSL anyway. This CL moves the failure from
sign/decrypt to RSA_check_key.
Change-Id: I468dbba74a148aa58c5994cc27f549e7ae1486a2
Reviewed-on: https://boringssl-review.googlesource.com/26374
Reviewed-by: Adam Langley <alangley@gmail.com>
Rather than recompute values the same as in key generation, where
possible, we check differently. In particular, most RSA values are
modular inverses of some value. Check each of them by multiplying and
using our naive constant-time division function.
Median of 29 RSA keygens: 0m0.218s -> 0m0.205s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: Iaca19f12c045457013def844a17bf502ed09136e
Reviewed-on: https://boringssl-review.googlesource.com/26373
Reviewed-by: Adam Langley <alangley@gmail.com>
This leaves RSA_check_key, which will be fixed in subsequent commits.
Median of 29 RSA keygens: 0m0.220s -> 0m0.209s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: I325f23fcc59302e68570908e5427b65471b799f6
Reviewed-on: https://boringssl-review.googlesource.com/26371
Reviewed-by: Adam Langley <alangley@gmail.com>
This uses the full binary GCD algorithm, where all four of A, B, C, and
D must be retained. (BN_mod_inverse_odd implements the odd number
version which only needs A and C.) It is patterned after the version
in the Handbook of Applied Cryptography, but tweaked so the coefficients
are non-negative and bounded.
Median of 29 RSA keygens: 0m0.225s -> 0m0.220s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: I6dc13524ea7c8ac1072592857880ddf141d87526
Reviewed-on: https://boringssl-review.googlesource.com/26370
Reviewed-by: Adam Langley <alangley@gmail.com>
RSA key generation requires computing a GCD (p-1 and q-1 are relatively
prime with e) and an LCM (the Carmichael totient). I haven't made BN_gcd
itself constant-time here to save having to implement
bn_lshift_secret_shift, since the two necessary operations can be served
by bn_rshift_secret_shift, already added for Rabin-Miller. However, the
guts of BN_gcd are replaced. Otherwise, the new functions are only
connected to tests for now, they'll be used in subsequent CLs.
To support LCM, there is also now a constant-time division function.
This does not replace BN_div because bn_div_consttime is some 40x slower
than BN_div. That penalty is fine for RSA keygen because that operation
is not bottlenecked on division, so we prefer simplicity over
performance.
Median of 29 RSA keygens: 0m0.212s -> 0m0.225s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: Idbfbfa6e7f5a3b8782ce227fa130417b3702cf97
Reviewed-on: https://boringssl-review.googlesource.com/26369
Reviewed-by: Adam Langley <alangley@gmail.com>
Expose the constant-time abs_sub functions from the fixed Karatsuba code
in BIGNUM form for RSA to call into. RSA key generation involves
checking if |p - q| is above some lower bound.
BN_sub internally branches on which of p or q is bigger. For any given
iteration, this is not secret---one of p or q is necessarily the larger,
and whether we happened to pick the larger or smaller first is
irrelevant. Accordingly, there is no need to perform the p/q swap at the
end in constant-time.
However, this stage of the algorithm picks p first, sticks with it, and
then computes |p - q| for various q candidates. The distribution of
comparisons leaks information about p. The leak is unlikely to be
problematic, but plug it anyway.
Median of 29 RSA keygens: 0m0.210s -> 0m0.212s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: I024b4e51b364f5ca2bcb419a0393e7be13249aec
Reviewed-on: https://boringssl-review.googlesource.com/26368
Reviewed-by: Adam Langley <alangley@gmail.com>
It costs us a malloc, but it's one less function to test and implement
in constant time, now that BN_cmp and BIGNUM are okay.
Median of 29 RSA keygens: 0m0.207s -> 0m0.210s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: Ic56f92f0dcf04da1f542290a7e8cdab8036699ed
Reviewed-on: https://boringssl-review.googlesource.com/26367
Reviewed-by: Adam Langley <alangley@gmail.com>
RSA key generation currently does the GCD check before the primality
test, in hopes of discarding things invalid by other means before
running the expensive primality check.
However, GCD is about to get a bit more expensive to clear the timing
leak, and the trial division part of primality testing is quite fast.
Thus, split that portion out via a new bn_is_obviously_composite and
call it before GCD.
Median of 29 RSA keygens: 0m0.252s -> 0m0.207s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: I3999771fb73cca16797cab9332d14c4ebeb02046
Reviewed-on: https://boringssl-review.googlesource.com/26366
Reviewed-by: Adam Langley <alangley@gmail.com>
This change follows up from e759a9cd with more extensive changes and
tests:
If a name checking function (like |X509_VERIFY_PARAM_set1_host|) fails,
it now poisons the |X509_VERIFY_PARAM| so that all verifications will
fail. This is because we have observed that some callers are not
checking the return value of these functions.
Using a length of zero for a hostname to mean |strlen| is now an error.
It also an error for email addresses and IP addresses now, and doesn't
end up trying to call |strlen| on a (binary) IP address.
Setting an email address with embedded NULs now fails. So does trying to
configure an empty hostname or email with (NULL, 0).
|X509_check_*| functions in BoringSSL don't accept zero lengths (unlike
OpenSSL). It's now tested that such calls always fail.
Change-Id: I4484176f2aae74e502a09081c7e912c85e8d090b
Update-Note: several behaviour changes. See change description.
Reviewed-on: https://boringssl-review.googlesource.com/26764
Reviewed-by: David Benjamin <davidben@google.com>
Rabin-Miller requires selecting a random number from 2 to |w|-1.
This is done by picking an N-bit number and discarding out-of-range
values. This leaks information about |w|, so apply blinding. Rather than
discard bad values, adjust them to be in range.
Though not uniformly selected, these adjusted values
are still usable as Rabin-Miller checks.
Rabin-Miller is already probabilistic, so we could reach the desired
confidence levels by just suitably increasing the iteration count.
However, to align with FIPS 186-4, we use a more pessimal analysis: we
do not count the non-uniform values towards the iteration count. As a
result, this function is more complex and has more timing risk than
necessary.
We count both total iterations and uniform ones and iterate until we've
reached at least |BN_PRIME_CHECKS_BLINDED| and |iterations|,
respectively. If the latter is large enough, it will be the limiting
factor with high probability and we won't leak information.
Note this blinding does not impact most calls when picking primes
because composites are rejected early. Only the two secret primes see
extra work. So while this does make the BNTest.PrimeChecking test take
about 2x longer to run on debug mode, RSA key generation time is fine.
Another, perhaps simpler, option here would have to run
bn_rand_range_words to the full 100 count, select an arbitrary
successful try, and declare failure of the entire keygen process (as we
do already) if all tries failed. I went with the option in this CL
because I happened to come up with it first, and because the failure
probability decreases much faster. Additionally, the option in this CL
does not affect composite numbers, while the alternate would. This gives
a smaller multiplier on our entropy draw. We also continue to use the
"wasted" work for stronger assurance on primality. FIPS' numbers are
remarkably low, considering the increase has negligible cost.
Thanks to Nathan Benjamin for helping me explore the failure rate as the
target count and blinding count change.
Now we're down to the rest of RSA keygen, which will require all the
operations we've traditionally just avoided in constant-time code!
Median of 29 RSA keygens: 0m0.169s -> 0m0.298s
(Accuracy beyond 0.1s is questionable. The runs at subsequent test- and
rename-only CLs were 0m0.217s, 0m0.245s, 0m0.244s, 0m0.247s.)
Bug: 238
Change-Id: Id6406c3020f2585b86946eb17df64ac42f30ebab
Reviewed-on: https://boringssl-review.googlesource.com/25890
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
(This is actually slightly silly as |a|'s probability distribution falls
off exponentially, but it's easy enough to do right.)
Instead, we run the loop to the end. This is still performant because we
can, as before, return early on composite numbers. Only two calls
actually run to the end. Moreover, running to the end has comparable
cost to BN_mod_exp_mont_consttime.
Median time goes from 0.140s to 0.231s. That cost some, but we're still
faster than the original implementation.
We're down to one more leak, which is that the BN_rand_range_ex call
does not hide |w1|. That one may only be solved probabilistically...
Median of 29 RSA keygens: 0m0.123s -> 0m0.145s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: I4847cb0053118c572d2dd5f855388b5199fa6ce2
Reviewed-on: https://boringssl-review.googlesource.com/25888
Reviewed-by: Adam Langley <agl@google.com>
Compilers use a variant of Barrett reduction to divide by constants,
which conveniently also avoids problematic operations on the secret
numerator. Implement the variant as described here:
http://ridiculousfish.com/blog/posts/labor-of-division-episode-i.html
Repurpose this to implement a constant-time BN_mod_word replacement.
It's even much faster! I've gone ahead and replaced the other
BN_mod_word calls on the primes table.
That should give plenty of budget for the other changes. (I am assuming
that a regression is okay, as RSA keygen is not performance-sensitive,
but that I should avoid anything too dramatic.)
Proof of correctness: https://github.com/davidben/fiat-crypto/blob/barrett/src/Arithmetic/BarrettReduction/RidiculousFish.v
Median of 29 RSA keygens: 0m0.621s -> 0m0.123s
(Accuracy beyond 0.1s is questionable, though this particular
improvement is quite solid.)
Bug: 238
Change-Id: I67fa36ffe522365b13feb503c687b20d91e72932
Reviewed-on: https://boringssl-review.googlesource.com/25887
Reviewed-by: Adam Langley <agl@google.com>
The extra details in Enhanced Rabin-Miller are only used in
RSA_check_key_fips, on the public RSA modulus, which the static linker
will drop in most of our consumers anyway. Implement normal Rabin-Miller
for RSA keygen and use Montgomery reduction so it runs in constant-time.
Note that we only need to avoid leaking information about the input if
it's a large prime. If the number ends up composite, or we find it in
our table of small primes, we can return immediately.
The leaks not addressed by this CL are:
- The difficulty of selecting |b| leaks information about |w|.
- The distribution of whether step 4.4 runs leaks information about w.
- We leak |a| (the largest power of two which divides w) everywhere.
- BN_mod_word in the trial division is not constant-time.
These will be resolved in follow-up changes.
Median of 29 RSA keygens: 0m0.521 -> 0m0.621s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: I0cf0ff22079732a0a3ababfe352bb4327e95b879
Reviewed-on: https://boringssl-review.googlesource.com/25886
Reviewed-by: Adam Langley <agl@google.com>
Probably worth having actual test vectors for these, rather than
checking our code against itself. Additionally, small negative numbers
have, in the past been valuable test vectors (see long comment in
point_add from OpenSSL's ecp_nistp521.c).
Change-Id: Ia5aa8a80eb5b6d0089c3601c5fec2364e699794d
Reviewed-on: https://boringssl-review.googlesource.com/26848
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
p224_felem_neg does not produce an output within the tight bounds
suitable for p224_felem_contract. This was found by inspection of the
code.
This only affects the final y-coordinate output of arbitrary-point
multiplication, so it is a no-op for ECDH and ECDSA.
Change-Id: I1d929458d1f21d02cd8e745d2f0f7040a6bb0627
Reviewed-on: https://boringssl-review.googlesource.com/26847
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Constructed types with a recursive definition could eventually exceed
the stack given malicious input with excessive recursion. Therefore we
limit the stack depth.
CVE-2018-0739
Credit to OSSFuzz for finding this issue.
(Imported from upstream's 9310d45087ae546e27e61ddf8f6367f29848220d.)
BoringSSL does not contain any such structures, but import this anyway
with a test.
Change-Id: I0e84578ea795134f25dae2ac8b565f3c26ef3204
Reviewed-on: https://boringssl-review.googlesource.com/26844
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Primality testing checks for small words in random places.
Median of 29 RSA keygens: 0m0.811s -> 0m0.521s
(Accuracy beyond 0.1s is questionable, and this "speed up" is certainly
noise.)
Bug: 238
Change-Id: Ie5efab7291302a42ac6e283d25da0c094d8577e7
Reviewed-on: https://boringssl-review.googlesource.com/25885
Reviewed-by: Adam Langley <agl@google.com>
There are a number of random subtractions in RSA key generation. Add a
fixed-width version.
Median of 29 RSA keygens: 0m0.859s -> 0m0.811s
(Accuracy beyond 0.1s is questionable.)
Bug: 238
Change-Id: I9fa0771b95a438fd7d2635fd77a332146ccc96d9
Reviewed-on: https://boringssl-review.googlesource.com/25884
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
BoringSSL does not generally support this quirk but, in this case, we
didn't make it a fatal error and it's instead a silent omission of
hostname checking. This doesn't affect Chrome but, in case something is
using BoringSSL and using this trick, this change makes it safe.
BUG=chromium:824799
Change-Id: If417817b997b9faa9963c09dfc95d06a5d445e0b
Reviewed-on: https://boringssl-review.googlesource.com/26724
Commit-Queue: Adam Langley <alangley@gmail.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
These are composite numbers whose composite witnesses aren't in the
first however many prime numbers, so deterministically checking small
numbers may not work.
We don't check composite witnesses deterministically but these are
probably decent tests. (Not sure how else to find composites with
scarce witnesses, but these seemed decent candidates.)
Change-Id: I23dcb7ba603a64c1f7d1e9a16942e7c29c76da51
Reviewed-on: https://boringssl-review.googlesource.com/26645
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
NSS only enables compatibility mode on the server if the client
requested it by way of the session ID. This is slightly off as a client
has no way not to request it when offering a TLS 1.2 session, but it is
in the spec.
So our tests are usable for other stacks, send a fake session ID in the
runner by default. The existing EmptySessionID-TLS13* test asserts that
BoringSSL behaves as we expect it to on empty session IDs too. The
intent is that NSS will disable that test but can otherwise leave the
rest enabled.
Change-Id: I370bf90aba1805c2f6970ceee0d29ecf199f437d
Reviewed-on: https://boringssl-review.googlesource.com/26504
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
On the other hand, the type-specific
|CBS_get_optional_asn1_octet_string| must have a valid pointer and we
should check this in the “present” case or there could be a lucking
crash in some user waiting for an expected value to be missing.
Change-Id: Ida40e069ac7f0e50967e3f6c6b3fc01e49bd8894
Reviewed-on: https://boringssl-review.googlesource.com/26564
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This clearly was supposed to be a return 1. See
https://github.com/openssl/openssl/issues/5537 for details.
(Additionally, now that our BIGNUMs may be non-minimal, this function
violates the rule that BIGNUM functions should not depend on widths. We
should use w >= bn_minimal_width(a) to retain the original behavior. But
the original behavior is nuts, so let's just fix it.)
Update-Note: BN_mask_bits no longer reports failure in some cases. These
cases were platform-dependent and not useful, and code search confirms
nothing was relying on it.
Change-Id: I31b1c2de6c5de9432c17ec3c714a5626594ee03c
Reviewed-on: https://boringssl-review.googlesource.com/26464
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
EC_KEY_copy left unset fields alone, which meant it was possible to
create an EC_KEY with mismatched private key and group. Nothing was
using EC_KEY_copy anyway, and in keeping of us generally preferring
fresh objects over object reuse, remove it. EC_KEY_dup itself can also
be made simpler by using the very setters available.
Additionally, skip copying the method table. As of
https://boringssl-review.googlesource.com/16344, we no longer copy the
ex_data, so we probably shouldn't copy the method pointers either,
aligning with RSAPrivateKey_dup.
Update-Note: If I missed anything and someone uses EC_KEY_copy, it
should be easy to port them to EC_KEY_dup.
Change-Id: Ibbdcea73345d91fa143fbe70a15bb527972693e8
Reviewed-on: https://boringssl-review.googlesource.com/26404
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Also remove the -Wtautological-constant-compare logic. I believe Clang
has since removed that problematic warning from -Wall and that check was
causing problems when we were embedded as a subproject in a project that
didn't set CMP0025.
(In that case, by the time our build file ran, the compiler had already
been detected and the damage done. This unfortunately means the next
Clang version check will hit the same issue, but let's deal with that
when we get there.)
Change-Id: Iea5f262899b74c5b84f707f4cf4ac4b3540c4acb
Reviewed-on: https://boringssl-review.googlesource.com/26375
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
On reflection, I think we'll need to note whether dummy PQ padding was
echoed on a given connection. Otherwise measurements in Chrome will be
mixed with cases where people have MITM proxies that ignored the
extension, or possibly Google frontends that haven't been updated.
Therefore this change will be used to filter latency measurements in
Chrome to only include those where the extension was echoed and we'll
measure at levels of 1 byte (for control), 400 bytes, and 1100 bytes.
This also makes it an error if the server didn't echo an extension of
the same length as was sent.
Change-Id: Ib2a0b29cfb8719a75a28f3cf96710c57d88eaa68
Reviewed-on: https://boringssl-review.googlesource.com/26284
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
In this round, Google servers will echo the extension in order to test
the latency of both parties sending a PQ key-agreement message.
The extension is sent (and echoed) for both full and resumption
handshakes. This is intended to mirror the overhead of TLS 1.3 (even
when using TLS 1.2), as a resumption in TLS 1.3 still does a fresh key
agreement.
Change-Id: I9ad163afac4fd1d916f9c7359ec32994e283abeb
Reviewed-on: https://boringssl-review.googlesource.com/26185
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Thumb2 addresses are a bit a mess, depending on whether a label is
interpreted as a function pointer value (for use with BX and BLX) or as
a program counter value (for use with PC-relative addressing). Clang's
integrated assembler mis-assembles this code. See
https://crbug.com/124610#c54 for details.
Instead, use the ADR pseudo-instruction which has clear semantics and
should be supported by every assembler that handles the OpenSSL Thumb2
code. (In other files, the ADR vs SUB conditionals are based on
__thumb2__ already. For some reason, this one is based on __APPLE__, I'm
guessing to deal with an older version of clang assembler.)
It's unclear to me which of clang or binutils is "correct" or if this is
even a well-defined notion beyond "whatever binutils does". But I will
note that https://github.com/openssl/openssl/pull/4669 suggests binutils
has also changed behavior around this before.
See also https://github.com/openssl/openssl/pull/5431 in OpenSSL.
Bug: chromium:124610
Change-Id: I5e7a0c8c0f54a3f65cc324ad599a41883675f368
Reviewed-on: https://boringssl-review.googlesource.com/26164
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
When OPENSSL_DANGEROUS_RELEASE_PTHREAD_KEY is defined during the build,
this change adds a destructor function that is called when BoringSSL is
unloaded via |dlclose| or during process exit. Using |dlclose| with
BoringSSL is not supported and will leak memory, but this change allows
some code that is already doing it to survive longer.
Change-Id: Ifc6d6aae61ed0f15d61cd3dbb4ea9f8006e43dba
Reviewed-on: https://boringssl-review.googlesource.com/25784
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Fred Gylys-Colwell <fredgc@google.com>
SSLv3_method, SSLv3_client_method, and SSLv3_server_method produce
SSL_CTXs which fail every handshake. They appear no longer necessary for
compatibility, so remove them.
SSLv3 is still accessible to callers who explicitly re-enable SSLv3 on a
TLS_method, but that will be removed completely later this year.
Meanwhile, clear out a weird hack we had here.
Update-Note: I believe there are no more callers of these functions. Any
that were were already non-functional as these methods haven't been
unable to handshake for a while now.
Change-Id: I622f785b428ab0ceab77b5a9db05b2b0df28145a
Reviewed-on: https://boringssl-review.googlesource.com/26004
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We don't advertise compressed coordinates (and point format negotiation
was deprecated in TLS 1.3), so reject them. Both Internet Explorer and
Firefox appear to reject them already.
Later I hope to add an easier to use ECDH API that acts on bytes, not
EC_POINT. This clears the way for that API to only accept uncompressed
coordinates. Compressed coordinates never got deployed over NIST curves,
for better or worse. At this point, there is no sense in changing that
as new protocols should use curve25519.
Change-Id: Id2f1be791ddcf155d596f4eb0b79351766c5cdab
Reviewed-on: https://boringssl-review.googlesource.com/26024
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This reuses wnaf.c's window scheduling, but has access to the tuned
field arithemetic and pre-computed base point table. Unlike wnaf.c, we
do not make the points affine as it's not worth it for a single table.
(We already precomputed the base point table.)
Annoyingly, 32-bit x86 gets slower by a bit, but the other platforms are
faster. My guess is that that the generic code gets to use the
bn_mul_mont assembly and the compiler, faced with the increased 32-bit
register pressure and the extremely register-poor x86, is making
bad decisions on the otherwise P-256-tuned C code. The three platforms
that see much larger gains are significantly more important than 32-bit
x86 at this point, so go with this change.
armv7a (Nexus 5X) before/after [+14.4%]:
Did 2703 ECDSA P-256 verify operations in 5034539us (536.9 ops/sec)
Did 3127 ECDSA P-256 verify operations in 5091379us (614.2 ops/sec)
aarch64 (Nexus 5X) before/after [+9.2%]:
Did 6783 ECDSA P-256 verify operations in 5031324us (1348.2 ops/sec)
Did 7410 ECDSA P-256 verify operations in 5033291us (1472.2 ops/sec)
x86 before/after [-2.7%]:
Did 8961 ECDSA P-256 verify operations in 10075901us (889.3 ops/sec)
Did 8568 ECDSA P-256 verify operations in 10003001us (856.5 ops/sec)
x86_64 before/after [+8.6%]:
Did 29808 ECDSA P-256 verify operations in 10008662us (2978.2 ops/sec)
Did 32528 ECDSA P-256 verify operations in 10057137us (3234.3 ops/sec)
Change-Id: I5fa643149f5bfbbda9533e3008baadfee9979b93
Reviewed-on: https://boringssl-review.googlesource.com/25684
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This syncs up with OpenSSL master as of
50ea9d2b3521467a11559be41dcf05ee05feabd6. The non-license non-spelling
changes are CFI bits, which were added in upstream in
b84460ad3a3e4fcb22efaa0a8365b826f4264ecf.
Change-Id: I42280985f834d5b9133eacafc8ff9dbd2f0ea59a
Reviewed-on: https://boringssl-review.googlesource.com/25704
Reviewed-by: Adam Langley <agl@google.com>
These files are otherwise up-to-date with OpenSSL master as of
50ea9d2b3521467a11559be41dcf05ee05feabd6, modulo a couple of spelling
fixes which I've imported.
I've also reverted the same-line label and instruction patch to
x86_64-mont*.pl. The new delocate parser handles that fine.
Change-Id: Ife35c671a8104c3cc2fb6c5a03127376fccc4402
Reviewed-on: https://boringssl-review.googlesource.com/25644
Reviewed-by: Adam Langley <agl@google.com>
This imports 384e6de4c7e35e37fb3d6fbeb32ddcb5eb0d3d3f and
79ca382d4762c58c4b92fceb4e202e90c71292ae from upstream.
Differences from upstream:
- We've removed a number of unused functions.
- We never imported 3ff08e1dde56747011a702a9a5aae06cfa8ae5fc, which was
to give the assembly control over the memory layout in the tables. So
our "gather" is "select" (which is implemented the same because the
memory layout never did change) and our "scatter" is in C.
Change-Id: I90d4a17da9f5f693f4dc4706887dec15f010071b
Reviewed-on: https://boringssl-review.googlesource.com/25586
Reviewed-by: Adam Langley <agl@google.com>
As of upstream's 6aa36e8e5a062e31543e7796f0351ff9628832ce, the
corresponding file in OpenSSL has both an Intel and OpenSSL copyright
blocks. To properly sync up with OpenSSL, use the OpenSSL copyright
block and our version of the Intel copyright block.
Change-Id: I4dc072a11390a54d0ce38ec0b8893e48f52638de
Reviewed-on: https://boringssl-review.googlesource.com/25585
Reviewed-by: Adam Langley <agl@google.com>
OpenSSL's RSA API is poorly designed and does not have a single place to
properly initialize the key. See
https://github.com/openssl/openssl/issues/5158.
To workaround this flaw, we must lazily instantiate pre-computed
Montgomery bits with locking. This is a ton of complexity. More
importantly, it makes it very difficult to implement RSA without side
channels. The correct in-memory representation of d, dmp1, and dmq1
depend on n, p, and q, respectively. (Those values have private
magnitudes and must be sized relative to the respective moduli.)
08805fe279 attempted to fix up the various
widths under lock, when we set up BN_MONT_CTX. However, this introduces
threading issues because other threads may access those exposed
components (RSA_get0_* also count as exposed for these purposes because
they are get0 functions), while a private key operation is in progress.
Instead, we do the following:
- There is no actual need to minimize n, p, and q, but we have minimized
copies in the BN_MONT_CTXs, so use those.
- Store additional copies of d, dmp1, and dmq1, at the cost of more
memory used. These copies have the correct width and are private,
unlike d, dmp1, and dmq1 which are sadly exposed. Fix private key
operations to use them.
- Move the frozen bit out of rsa->flags, as that too was historically
accessible without locking.
(Serialization still uses the original BIGNUMs, but the RSAPrivateKey
serialization format already inherently leaks the magnitude, so this
doesn't matter.)
Change-Id: Ia3a9b0629f8efef23abb30bfed110d247d1db42f
Reviewed-on: https://boringssl-review.googlesource.com/25824
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This is a bit easier to read than BN_less_than_consttime when we must do
>= or <=, about as much work to compute, and lots of code calls BN_cmp
on secret data. This also, by extension, makes BN_cmp_word
constant-time.
BN_equal_consttime is probably a little more efficient and is perfectly
readable, so leave that one around.
Change-Id: Id2e07fe312f01cb6fd10a1306dcbf6397990cf13
Reviewed-on: https://boringssl-review.googlesource.com/25444
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This follows similar lines as the previous cleanups and fixes the
documentation of the preconditions.
And with that, RSA private key operations, provided p and q have the
same bit length, should be constant time, as far as I know. (Though I'm
sure I've missed something.)
bn_cmp_part_words and bn_cmp_words are no longer used and deleted.
Bug: 234
Change-Id: Iceefa39f57e466c214794c69b335c4d2c81f5577
Reviewed-on: https://boringssl-review.googlesource.com/25404
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The power of two computations here were extremely confusing and one of
the comments mixed && and ||. Remove the cached k = j + j value.
Optimizing the j*8, j*8, j*2, and j*4 multiplications is the compiler's
job. If it doesn't manage it, it was only a couple shifts anyway.
With that fixed, it becomes easier to tell that rr was actaully
allocated twice as large as necessary. I suspect rr is also
incorrectly-allocated in the bn_mul_part_recursive case, but I'll wait
until I've checked that function over first. (The array size
documentation on the other bn_{mul,sqr}_recursive functions have had
mistakes before.)
Change-Id: I298400b988e3bd108d01d6a7c8a5b262ddf81feb
Reviewed-on: https://boringssl-review.googlesource.com/25364
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
I left the input length as int because the calling convention passes
these messy deltas around. This micro-optimization is almost certainly
pointless, but bn_sub_part_words is written in assembly, so I've left it
alone for now. The documented preconditions were also all completely
wrong, so I've fixed them. We actually only call them for even tighter
bounds (one of dna or dnb is 0 and the other is 0 or -1), at least
outside bn_mul_part_recursive which I still need to read through.
This leaves bn_mul_part_recursive, which is reachable for RSA keys which
are not a power of two in bit width.
The first iteration of this had an uncaught bug, so I added a few more
aggressive tests generated with:
A = 0x...
B = 0x...
# Chop off 0, 1 and > 1 word for both 32 and 64-bit.
for i in (0, 1, 2, 4):
for j in (0, 1, 2, 4):
a = A >> (32*i)
b = B >> (32*j)
p = a * b
print "Product = %x" % p
print "A = %x" % a
print "B = %x" % b
print
Bug: 234
Change-Id: I72848d992637c0390cdd3c4f81cb919393b59eb8
Reviewed-on: https://boringssl-review.googlesource.com/25344
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
We still need BN_mul and, in particular, bn_mul_recursive will either
require bn_abs_sub_words be generalized or that we add a parallel
bn_abs_sub_part_words, but start with the easy one.
While I'm here, simplify the i and j mess in here. It's patterned after
the multiplication one, but can be much simpler.
Bug: 234
Change-Id: If936099d53304f2512262a1cbffb6c28ae30ccee
Reviewed-on: https://boringssl-review.googlesource.com/25325
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
There is no more need for the "constant-time" reading beyond bn->top. We
can write the bytes out naively because RSA computations no longer call
bn_correct_top/bn_set_minimal_width.
Specifically, the final computation is a BN_mod_mul_montgomery to remove
the blinding, and that keeps the sizes correct.
Bug: 237
Change-Id: I6e90d81c323b644e179d899f411479ea16deab98
Reviewed-on: https://boringssl-review.googlesource.com/25324
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Alas, the existence of RSA keys with q > p is obnoxious, but we can
canonicalize it away. To my knowledge, the remaining leaks in RSA are:
- Key generation. This is kind of hopelessly non-constant-time but
perhaps deserves a more careful ponder. Though hopefully it does not
come in at a measurable point for practical purposes.
- Private key serialization. RSAPrivateKey inherently leaks the
magnitudes of d, dmp1, dmq1, and iqmp. This is unavoidable but
hopefully does not come in at a measurable point for practical
purposes.
- If p and q have different word widths, we currently fall back to the
variable-time BN_mod rather than Montgomery reduction at the start of
CRT. I can think of ways to apply Montgomery reduction, but it's
probably better to deny CRT to such keys, if not reject them outright.
- bn_mul_fixed and bn_sqr_fixed which affect the Montgomery
multiplication bn_mul_mont-less configurations, as well as the final
CRT multiplication. We should fix this.
Bug: 233
Change-Id: I8c2ecf8f8ec104e9f26299b66ac8cbb0cad04616
Reviewed-on: https://boringssl-review.googlesource.com/25263
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
As the EC code will ultimately want to use these in "words" form by way
of EC_FELEM, and because it's much easier, I've implement these as
low-level words-based functions that require all inputs have the same
width. The BIGNUM versions which RSA and, for now, EC calls are
implemented on top of that.
Unfortunately, doing such things in constant-time and accounting for
undersized inputs requires some scratch space, and these functions don't
take BN_CTX. So I've added internal bn_mod_*_quick_ctx functions that
take a BN_CTX and the old functions now allocate a bit unnecessarily.
RSA only needs lshift (for BN_MONT_CTX) and sub (for CRT), but the
generic EC code wants add as well.
The generic EC code isn't even remotely constant-time, and I hope to
ultimately use stack-allocated EC_FELEMs, so I've made the actual
implementations here implemented in "words", which is much simpler
anyway due to not having to take care of widths.
I've also gone ahead and switched the EC code to these functions,
largely as a test of their performance (an earlier iteration made the EC
code noticeably slower). These operations are otherwise not
performance-critical in RSA.
The conversion from BIGNUM to BIGNUM+BN_CTX should be dropped by the
static linker already, and the unused BIGNUM+BN_CTX functions will fall
off when EC_FELEM happens.
Update-Note: BN_mod_*_quick bounce on malloc a bit now, but they're not
really used externally. The one caller I found was wpa_supplicant
which bounces on malloc already. They appear to be implementing
compressed coordinates by hand? We may be able to convince them to
call EC_POINT_set_compressed_coordinates_GFp.
Bug: 233, 236
Change-Id: I2bf361e9c089e0211b97d95523dbc06f1168e12b
Reviewed-on: https://boringssl-review.googlesource.com/25261
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
With fixed-width BIGNUMs, this is no longer a concern. With this CL, I
believe we now no longer call BN_num_bits on BIGNUMs with secret
magnitude.
Of course, DSA then turns around and calls the variable-time BN_mod
immediately afterwards anyway. But the DSA is deprecated and doomed to
be removed someday anyway.
Change-Id: Iac1dab22aa51c0e7f5ca0f7f44a026a242a4eaa2
Reviewed-on: https://boringssl-review.googlesource.com/25284
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
d, dmp1, dmq1, and iqmp have private magnitudes. This is awkward because
the RSAPrivateKey serialization leaks the magnitudes. Do the best we can
and fix them up before any RSA operations.
This moves the piecemeal BN_MONT_CTX_set_locked into a common function
where we can do more complex canonicalization on the keys. Ideally this
would be done on key import, but the exposed struct (and OpenSSL 1.1.0's
bad API design) mean there is no single point in time when key import is
finished.
Also document the constraints on RSA_set0_* functions. (These
constraints aren't new. They just were never documented before.)
Update-Note: If someone tried to use an invalid RSA key where d >= n,
dmp1 >= p, dmq1 >= q, or iqmp >= p, this may break. Such keys would not
have passed RSA_check_key, but it's possible to manually assemble
keys that bypass it.
Bug: 232
Change-Id: I421f883128952f892ac0cde0d224873a625f37c5
Reviewed-on: https://boringssl-review.googlesource.com/25259
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The fallback functions still themselves leak, but I've left TODOs there.
This only affects BN_mod_mul_montgomery on platforms where we don't use
the bn_mul_mont assembly, but BN_mul additionally affects the final
multiplication in RSA CRT.
Bug: 232
Change-Id: Ia1ae16162c38e10c056b76d6b2afbed67f1a5e16
Reviewed-on: https://boringssl-review.googlesource.com/25260
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This has no behavior change, but it has a semantic one. This CL is an
assertion that all BIGNUM functions tolerate non-minimal BIGNUMs now.
Specifically:
- Functions that do not touch top/width are assumed to not care.
- Functions that do touch top/width will be changed by this CL. These
should be checked in review that they tolerate non-minimal BIGNUMs.
Subsequent CLs will start adjusting the widths that BIGNUM functions
output, to fix timing leaks.
Bug: 232
Change-Id: I3a2b41b071f2174452f8d3801bce5c78947bb8f7
Reviewed-on: https://boringssl-review.googlesource.com/25257
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Test this by re-running bn_tests.txt tests a lot. For the most part,
this was done by scattering bn_minimal_width or bn_correct_top calls as
needed. We'll incrementally tease apart the functions that need to act
on non-minimal BIGNUMs in constant-time.
BN_sqr was switched to call bn_correct_top at the end, rather than
sample bn_minimal_width, in anticipation of later splitting it into
BN_sqr (for calculators) and BN_sqr_fixed (for BN_mod_mul_montgomery).
BN_div_word also uses bn_correct_top because it calls BN_lshift so
officially shouldn't rely on BN_lshift returning something
minimal-width, though I expect we'd want to split off a BN_lshift_fixed
than change that anyway?
The shifts sample bn_minimal_width rather than bn_correct_top because
they all seem to try to be very clever around the bit width. If we need
constant-time versions of them, we can adjust them later.
Bug: 232
Change-Id: Ie17b39034a713542dbe906cf8954c0c5483c7db7
Reviewed-on: https://boringssl-review.googlesource.com/25255
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Processing off-curve points is sufficiently dangerous to worry about
code that doesn't check the return value of
|EC_POINT_set_affine_coordinates| and |EC_POINT_oct2point|. While we
have integrated on-curve checks into these functions, code that ignores
the return value will still be able to work with an invalid point
because it's already been installed in the output by the time the check
is done.
Instead, in the event of an off-curve point, set the output point to the
generator, which is certainly on the curve and hopefully safe.
Change-Id: Ibc73dceb2d8d21920e07c4f6def2c8249cb78ca0
Reviewed-on: https://boringssl-review.googlesource.com/25724
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This change allows run_cavp.go to execute tests on a connected Android
device and collect the results.
Change-Id: Ica83239c58d83907b82c591c4873a3de4ba0b3c0
Reviewed-on: https://boringssl-review.googlesource.com/25604
Reviewed-by: David Benjamin <davidben@google.com>
Checking the excess words for zero doesn't need to be in constant time,
but it's free. BN_bn2bin_padded is a little silly as read_word_padded
only exists to work around bn->top being minimal. Once non-minimal
BIGNUMs are turned on and the RSA code works right, we can simplify
BN_bn2bin_padded.
Bug: 232
Change-Id: Ib81e30ca1e5a8ea90ab3278bf4ded219bac481ac
Reviewed-on: https://boringssl-review.googlesource.com/25253
Reviewed-by: Adam Langley <agl@google.com>
Some of the CFI directives from upstream include expressions such as:
.cfi_adjust_cfa_offset 32*5+8
(Also the latest version of peg moves the go generate line to
delocate.peg.go.)
Change-Id: I21bdf9ae44f81e4eca7b3565c4581a670f621a80
Reviewed-on: https://boringssl-review.googlesource.com/25624
Reviewed-by: Adam Langley <agl@google.com>
Give a non-minimal modulus, there are two possible values of R we might
pick: 2^(BN_BITS2 * width) or 2^(BN_BITS2 * bn_minimal_width).
Potentially secret moduli would make the former attractive and things
might even work, but our only secret moduli (RSA) have public bit
widths. It's more cases to test and the usual BIGNUM invariant is that
widths do not affect numerical output.
Thus, settle on minimizing mont->N for now. With the top explicitly made
minimal, computing |lgBigR| is also a little simpler.
This CL also abstracts out the < R check in the RSA code, and implements
it in a width-agnostic way.
Bug: 232
Change-Id: I354643df30530db7866bb7820e34241d7614f3c2
Reviewed-on: https://boringssl-review.googlesource.com/25250
Reviewed-by: Adam Langley <agl@google.com>
These functions already require their inputs to be reduced mod N (or, in
some cases, bounded by R or N*R), so negative numbers are nonsense. The
code still attempted to account for them by working on the absolute
value and fiddling with the sign bit. (The output would be in range (-N,
N) instead of [0, N).)
This complicates relaxing bn_correct_top because bn_correct_top is also
used to prevent storing a negative zero. Instead, just reject negative
inputs.
Upgrade-Note: These functions are public API, so some callers may
notice. Code search suggests there is only one caller outside
BoringSSL, and it looks fine.
Bug: 232
Change-Id: Ieba3acbb36b0ff6b72b8ed2b14882ec9b88e4665
Reviewed-on: https://boringssl-review.googlesource.com/25249
Reviewed-by: Adam Langley <agl@google.com>
This cuts down on a duplicated place where we mess with bn->top. It also
also better abstracts away what determines the value of R.
(I ordered this wrong and rebasing will be annoying. Specifically, the
question is what happens if the modulus is non-minimal. In
https://boringssl-review.googlesource.com/c/boringssl/+/25250/, R will
be determined by the stored width of mont->N, so we want to use mont's
copy of the modulus. Though, one way or another, the important part is
that it's inside the Montgomery abstraction.)
Bug: 232
Change-Id: I74212e094c8a47f396b87982039e49048a130916
Reviewed-on: https://boringssl-review.googlesource.com/25247
Reviewed-by: Adam Langley <agl@google.com>
This is actually a bit more complicated (the mismatching widths cases
will never actually happen in RSA), but it's easier to think about and
removes more width-sensitive logic.
Bug: 232
Change-Id: I85fe6e706be1f7d14ffaf587958e930f47f85b3c
Reviewed-on: https://boringssl-review.googlesource.com/25246
Reviewed-by: Adam Langley <agl@google.com>
This makes it easier going to and from non-minimal BIGNUMs and words
without worrying about the widths which are ultimately to become less
friendly.
Bug: 232
Change-Id: Ia57cb29164c560b600573c27b112ad9375a86aad
Reviewed-on: https://boringssl-review.googlesource.com/25245
Reviewed-by: Adam Langley <agl@google.com>
Thanks to Andres Erbsen for extremely helpful suggestions on how finally
plug this long-standing hole!
OpenSSL BIGNUMs are currently minimal-width, which means they cannot be
constant-time. We'll need to either excise BIGNUM from RSA and EC or
somehow fix BIGNUM. EC_SCALAR and later EC_FELEM work will excise it
from EC, but RSA's BIGNUMs are more transparent. Teaching BIGNUM to
handle non-minimal word widths is probably simpler.
The main constraint is BIGNUM's large "calculator" API surface. One
could, in theory, do arbitrary math on RSA components, which means all
public functions must tolerate non-minimal inputs. This is also useful
for EC; https://boringssl-review.googlesource.com/c/boringssl/+/24445 is
silly.
As a first step, fix comparison-type functions that were assuming
minimal BIGNUMs. I've also added bn_resize_words, but it is testing-only
until the rest of the library is fixed.
bn->top is now a loose upper bound we carry around. It does not affect
numerical results, only performance and secrecy. This is a departure
from the original meaning, and compiler help in auditing everything is
nice, so the final change in this series will rename bn->top to
bn->width. Thus these new functions are named per "width", not "top".
Looking further ahead, how are output BIGNUM widths determined? There's
three notions of correctness here:
1. Do I compute the right answer for all widths?
2. Do I handle secret data in constant time?
3. Does my memory usage not balloon absurdly?
For (1), a BIGNUM function must give the same answer for all input
widths. BN_mod_add_quick may assume |a| < |m|, but |a| may still be
wider than |m| by way of leading zeres. The simplest approach is to
write code in a width-agnostic way and rely on functions to accept all
widths. Where functions need to look at bn->d, we'll a few helper
functions to smooth over funny widths.
For (2), (1) is little cumbersome. Consider constant-time modular
addition. A sane type system would guarantee input widths match. But C
is weak here, and bifurcating the internals is a lot of work. Thus, at
least for now, I do not propose we move RSA's internal computation out
of BIGNUM. (EC_SCALAR/EC_FELEM are valuable for EC because we get to
stack-allocate, curves were already specialized, and EC only has two
types with many operations on those types. None of these apply to RSA.
We've got numbers mod n, mod p, mod q, and their corresponding
exponents, each of which is used for basically one operation.)
Instead, constant-time BIGNUM functions will output non-minimal widths.
This is trivial for BN_bin2bn or modular arithmetic. But for BN_mul,
constant-time[*] would dictate r->top = a->top + b->top. A calculator
repeatedly multiplying by one would then run out of memory. Those we'll
split into a private BN_mul_fixed for crypto, leaving BN_mul for
calculators. BN_mul is just BN_mul_fixed followed by bn_correct_top.
[*] BN_mul is not constant-time for other reasons, but that will be
fixed separately.
Bug: 232
Change-Id: Ide2258ae8c09a9a41bb71d6777908d1c27917069
Reviewed-on: https://boringssl-review.googlesource.com/25244
Reviewed-by: Adam Langley <agl@google.com>
Now that we have 64-bit C code, courtesy of fiat-crypto, the tradeoff
for carrying the assembly changes:
Assembly:
Did 16000 Curve25519 base-point multiplication operations in 1059932us (15095.3 ops/sec)
Did 16000 Curve25519 arbitrary point multiplication operations in 1060023us (15094.0 ops/sec)
fiat64:
Did 39000 Curve25519 base-point multiplication operations in 1004712us (38817.1 ops/sec)
Did 14000 Curve25519 arbitrary point multiplication operations in 1006827us (13905.1 ops/sec)
The assembly is still about 9% faster than fiat64, but fiat64 gets to
use the Ed25519 tables for the base point multiplication, so overall it
is actually faster to disable the assembly:
>>> 1/(1/15094.0 + 1/15095.3)
7547.324986004976
>>> 1/(1/38817.1 + 1/13905.1)
10237.73016319501
(At the cost of touching a 30kB table.)
The assembly implementation is no longer pulling its weight. Remove it
and use the fiat code in all build configurations.
Change-Id: Id736873177d5568bb16ea06994b9fcb1af104e33
Reviewed-on: https://boringssl-review.googlesource.com/25524
Reviewed-by: Adam Langley <agl@google.com>
The private key callback may not push one of its own (it's possible to
register a custom error library and whatnot, but this is tedious). If
the callback does not push any, we report SSL_ERROR_SYSCALL. This is not
completely wrong, as "syscall" really means "I don't know, something you
gave me, probably the BIO, failed so I assume you know what happened",
but most callers just check errno. And indeed cert_cb pushes its own
error, so this probably should as well.
Update-Note: Custom private key callbacks which push an error code on
failure will report both that error followed by
SSL_R_PRIVATE_KEY_OPERATION_FAILED. Callbacks which did not push any
error will switch from SSL_ERROR_SYSCALL to SSL_ERROR_SSL with
SSL_R_PRIVATE_KEY_OPERATION_FAILED.
Change-Id: I7e90cd327fe0cbcff395470381a3591364a82c74
Reviewed-on: https://boringssl-review.googlesource.com/25544
Reviewed-by: Adam Langley <agl@google.com>
Split handshakes allows the handshaking of a TLS connection to be
performed remotely. This encompasses not just the private-key and ticket
operations – support for that was already available – but also things
such as selecting the certificates and cipher suites.
The the comment block in ssl.h for details. This is highly experimental
and will change significantly before its settled.
Change-Id: I337bdfa4c3262169e9b79dd4e70b57f0d380fcad
Reviewed-on: https://boringssl-review.googlesource.com/25387
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Mono's legacy TLS 1.0 stack, as a server, does not implement any form of
resumption, but blindly echos the ClientHello session ID in the
ServerHello for no particularly good reason.
This is invalid, but due to quirks of how our client checked session ID
equality, we only noticed on the second connection, rather than the
first. Flaky failures do no one any good, so break deterministically on
the first connection, when we realize something strange is going on.
Bug: chromium:796910
Change-Id: I1f255e915fcdffeafb80be481f6c0acb3c628846
Reviewed-on: https://boringssl-review.googlesource.com/25424
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Previously we required that the calls to TLS's AES-GCM use an
incrementing nonce. This change relaxes that requirement so that nonces
need only be strictly monotonic (i.e. values can now be skipped). This
still meets the uniqueness requirements of a nonce.
Change-Id: Ib649a58bb93bf4dc0e081de8a5971daefffe9c70
Reviewed-on: https://boringssl-review.googlesource.com/25384
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
(See also https://github.com/openssl/openssl/pull/5154.)
The exponent here is one of d, dmp1, or dmq1 for RSA. This value and its
bit length are both secret. The only public upper bound is the bit width
of the corresponding modulus (RSA n, p, and q, respectively).
Although BN_num_bits is constant-time (sort of; see bn_correct_top notes
in preceding patch), this does not fix the root problem, which is that
the windows are based on the minimal bit width, not the upper bound. We
could use BN_num_bits(m), but BN_mod_exp_mont_consttime is public API
and may be called with larger exponents. Instead, use all top*BN_BITS2
bits in the BIGNUM. This is still sensitive to the long-standing
bn_correct_top leak, but we need to fix that regardless.
This may cause us to do a handful of extra multiplications for RSA keys
which are just above a whole number of words, but that is not a standard
RSA key size.
Change-Id: I5e2f12b70c303b27c597a7e513b7bf7288f7b0e3
Reviewed-on: https://boringssl-review.googlesource.com/25185
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Our 64-bit performance was much lower than it could have been, since we
weren't using the 64-bit multipliers. Fortunately, fiat-crypto is
awesome, so this is just a matter of synthesizing new code and
integration work.
Functions without the signature fiat-crypto curly braces were written by
hand and warrant more review. (It's just redistributing some bits.)
These use the donna variants which takes (and proves) some of the
instruction scheduling from donna as that's significantly faster.
Glancing over things, I suspect but have not confirmed the gap is due to
this:
https://github.com/mit-plv/fiat-crypto/pull/295#issuecomment-356892413
Clang without OPENSSL_SMALL (ECDH omitted since that uses assembly and
is unaffected by this CL).
Before:
Did 105149 Ed25519 key generation operations in 5025208us (20924.3 ops/sec)
Did 125000 Ed25519 signing operations in 5024003us (24880.6 ops/sec)
Did 37642 Ed25519 verify operations in 5072539us (7420.7 ops/sec)
After:
Did 206000 Ed25519 key generation operations in 5020547us (41031.4 ops/sec)
Did 227000 Ed25519 signing operations in 5005232us (45352.5 ops/sec)
Did 69840 Ed25519 verify operations in 5004769us (13954.7 ops/sec)
Clang + OPENSSL_SMALL:
Before:
Did 68598 Ed25519 key generation operations in 5024629us (13652.4 ops/sec)
Did 73000 Ed25519 signing operations in 5067837us (14404.6 ops/sec)
Did 36765 Ed25519 verify operations in 5078684us (7239.1 ops/sec)
Did 74000 Curve25519 base-point multiplication operations in 5016465us (14751.4 ops/sec)
Did 45600 Curve25519 arbitrary point multiplication operations in 5034680us (9057.2 ops/sec)
After:
Did 117315 Ed25519 key generation operations in 5021860us (23360.9 ops/sec)
Did 126000 Ed25519 signing operations in 5003521us (25182.3 ops/sec)
Did 64974 Ed25519 verify operations in 5047790us (12871.8 ops/sec)
Did 134000 Curve25519 base-point multiplication operations in 5058946us (26487.7 ops/sec)
Did 86000 Curve25519 arbitrary point multiplication operations in 5050478us (17028.1 ops/sec)
GCC without OPENSSL_SMALL (ECDH omitted since that uses assembly and
is unaffected by this CL).
Before:
Did 35552 Ed25519 key generation operations in 5030756us (7066.9 ops/sec)
Did 38286 Ed25519 signing operations in 5001648us (7654.7 ops/sec)
Did 10584 Ed25519 verify operations in 5068158us (2088.3 ops/sec)
After:
Did 92158 Ed25519 key generation operations in 5024021us (18343.5 ops/sec)
Did 99000 Ed25519 signing operations in 5011908us (19753.0 ops/sec)
Did 31122 Ed25519 verify operations in 5069878us (6138.6 ops/sec)
Change-Id: Ic0c24d50b4ee2bbc408b94965e9d63319936107d
Reviewed-on: https://boringssl-review.googlesource.com/24805
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The original comment was a little confusing. Also lowercase
CTR_DRBG_update to make our usual naming for static functions.
Bug: 227
Change-Id: I381c7ba12b788452d54520b7bc3b13bba8a59f2d
Reviewed-on: https://boringssl-review.googlesource.com/25204
Reviewed-by: Adam Langley <agl@google.com>
(The BN_num_bits_word implementation was originally written by Andy
Polyakov for OpenSSL. See also
https://github.com/openssl/openssl/pull/5154.)
BN_num_bits, by way of BN_num_bits_word, currently leaks the
most-significant word of its argument via branching and memory access
pattern.
BN_num_bits is called on RSA prime factors in various places. These have
public bit lengths, but all bits beyond the high bit are secret. This
fully resolves those cases.
There are a few places where BN_num_bits is called on an input where
the bit length is also secret. The two left in BoringSSL are:
- BN_mod_exp_mont_consttime calls it on the RSA private exponent.
- The timing "fix" to add the order to k in DSA.
This does *not* fully resolve those cases as we still only look at the
top word. Today, that is guaranteed to be non-zero, but only because of
the long-standing bn_correct_top timing leak. Once that is fixed (I hope
to have patches soon), a constant-time BN_num_bits on such inputs must
count bits on each word.
Instead, those cases should not call BN_num_bits at all. The former uses
the bit width to pick windows, but it should be using the maximum bit
width. The next patch will fix this. The latter is the same "fix" we
excised from ECDSA in a838f9dc7e. That
should be excised from DSA after the bn_correct_top bug is fixed.
Thanks to Dinghao Wu, Danfeng Zhang, Shuai Wang, Pei Wang, and Xiao Liu
for reporting this issue.
Change-Id: Idc3da518cc5ec18bd8688b95f959b15300a57c14
Reviewed-on: https://boringssl-review.googlesource.com/25184
Reviewed-by: Adam Langley <agl@google.com>
The EC_POINTs are still allocated (for now), but everything else fits on
the stack nicely, which saves a lot of fiddling with cleanup and
allocations.
Change-Id: Ib8480737ecc97e6b40b2c05f217cd8d3dc82cb72
Reviewed-on: https://boringssl-review.googlesource.com/25150
Reviewed-by: Adam Langley <agl@google.com>
This is to simplify clearing unnecessary mallocs out of ec_wNAF_mul, and
perhaps to use it in tuned variable-time multiplication functions.
Change-Id: Ic390d2e8e20d0ee50f3643830a582e94baebba95
Reviewed-on: https://boringssl-review.googlesource.com/25149
Reviewed-by: Adam Langley <agl@google.com>
This cuts out another total_num-length array and simplifies things.
Leading zeros at the front of the schedule don't do anything, so it's
easier to just produce a fixed-length one. (I'm also hoping to
ultimately reuse this function in //third_party/fiat/p256.c and get the
best of both worlds for ECDSA verification; tuned field arithmetic
operations, precomputed table, and variable-time multiply.)
Change-Id: I771f4ff7dcfdc3ee0eff8d9038d6dc9a0be3d4e0
Reviewed-on: https://boringssl-review.googlesource.com/25148
Reviewed-by: Adam Langley <agl@google.com>
Note this switches from walking BN_num_bits to the full bit length of
the scalar. But that can only cause it to add a few extra zeros to the
front of the schedule, which r_is_at_infinity will skip over.
Change-Id: I91e087c9c03505566b68f75fb37dfb53db467652
Reviewed-on: https://boringssl-review.googlesource.com/25147
Reviewed-by: Adam Langley <agl@google.com>
This appears to be pointless. Before, we would have a 50% chance of
doing an inversion at each non-zero bit but the first
(r_is_at_infinity), plus a 50% chance of doing an inversion at the end.
Now we would have a 50% chance of doing an inversion at each non-zero
bit. That's the same number of coin flips.
Change-Id: I8158fd48601cb041188826d4f68ac1a31a6fbbbc
Reviewed-on: https://boringssl-review.googlesource.com/25146
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is to make it easier to add new field element representations. The
Ed25519 logic in the script is partially adapted from RFC 8032's Python
code, but I replaced the point addition logic with the naive textbook
formula since this script only cares about being obviously correct.
Change-Id: I0b90bf470993c177070fd1010ac5865fedb46c82
Reviewed-on: https://boringssl-review.googlesource.com/24745
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The optimization for wsize = 1 only kicks in for 19-bit primes. The
cases for b >= 800 and cannot happen due to EC_MAX_SCALAR_BYTES.
Change-Id: If5ca908563f027172cdf31c9a22342152fecd12f
Reviewed-on: https://boringssl-review.googlesource.com/25145
Reviewed-by: Adam Langley <agl@google.com>
Simplify things slightly. The probability of the scalar being small
enough to go down a window size is astronomically small. (2^-186 for
P-256 and 2^-84 for P-384.)
Change-Id: Ie879f0b06bcfd1e6e6e3bf3f54e0d7d6567525a4
Reviewed-on: https://boringssl-review.googlesource.com/25144
Reviewed-by: Adam Langley <agl@google.com>
fiat-crypto only generates fe_mul and fe_sq, but the original Ed25519
implementation we had also had fe_sq2 for computing 2*f^2. Previously,
we inlined a version of fe_mul.
Instead, we could implement it with fe_sq and fe_add. Performance-wise,
this seems to not regress. If anything, it makes it faster?
Before (clang, run for 10 seconds):
Did 243000 Ed25519 key generation operations in 10025910us (24237.2 ops/sec)
Did 250000 Ed25519 signing operations in 10035580us (24911.4 ops/sec)
Did 73305 Ed25519 verify operations in 10071101us (7278.7 ops/sec)
Did 184000 Curve25519 base-point multiplication operations in 10040138us (18326.4 ops/sec)
Did 186000 Curve25519 arbitrary point multiplication operations in 10052721us (18502.5 ops/sec)
After (clang, run for 10 seconds):
Did 242424 Ed25519 key generation operations in 10013117us (24210.6 ops/sec)
Did 253000 Ed25519 signing operations in 10011744us (25270.3 ops/sec)
Did 73899 Ed25519 verify operations in 10048040us (7354.6 ops/sec)
Did 194000 Curve25519 base-point multiplication operations in 10005389us (19389.6 ops/sec)
Did 195000 Curve25519 arbitrary point multiplication operations in 10028443us (19444.7 ops/sec)
Before (clang + OPENSSL_SMALL, run for 10 seconds):
Did 144000 Ed25519 key generation operations in 10019344us (14372.2 ops/sec)
Did 146000 Ed25519 signing operations in 10011653us (14583.0 ops/sec)
Did 74052 Ed25519 verify operations in 10005789us (7400.9 ops/sec)
Did 150000 Curve25519 base-point multiplication operations in 10007468us (14988.8 ops/sec)
Did 91392 Curve25519 arbitrary point multiplication operations in 10057678us (9086.8 ops/sec)
After (clang + OPENSSL_SMALL, run for 10 seconds):
Did 144000 Ed25519 key generation operations in 10066724us (14304.6 ops/sec)
Did 148000 Ed25519 signing operations in 10062043us (14708.7 ops/sec)
Did 74820 Ed25519 verify operations in 10058557us (7438.4 ops/sec)
Did 151000 Curve25519 base-point multiplication operations in 10063492us (15004.7 ops/sec)
Did 90402 Curve25519 arbitrary point multiplication operations in 10049141us (8996.0 ops/sec)
Change-Id: I31e9f61833492c3ff2dfd78e1dee5e06f43c850f
Reviewed-on: https://boringssl-review.googlesource.com/24724
Reviewed-by: Adam Langley <agl@google.com>
Because the android sdk managed by CIPD (see go/use-adjective-apis),
its deps repo ndk needs to be relocated, as well as hooked by the root
DEPS instead of recursedeps. This CL addes android ndk deps in
util/bot/DEPS.
Bug: chromium:659808
Change-Id: I270053ae56ba1caaf9c67f2240855eafd499b782
Reviewed-on: https://boringssl-review.googlesource.com/24864
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
NIAP requires that the TLS KDF be tested by CAVP so this change moves
the PRF into crypto/fipsmodule/tls and adds a test harness for it. Like
the KAS tests, this is only triggered when “-niap” is passed to
run_cavp.go.
Change-Id: Iaa4973d915853c8e367e6106d829e44fcf1b4ce5
Reviewed-on: https://boringssl-review.googlesource.com/24666
Reviewed-by: Adam Langley <agl@google.com>
This change adds support for two specific CAVP tests, in order to
meet NIAP requirements.
These tests are currently only run when “-niap” is passed to run_cavp.go
because they are not part of our FIPS validation (yet).
Change-Id: I511279651aae094702332130fac5ab64d11ddfdb
Reviewed-on: https://boringssl-review.googlesource.com/24665
Reviewed-by: Adam Langley <agl@google.com>
In order to process some NIST FAX files, we needed to implement a custom
scanner function to skip over lines that are effectively comments, but
not marked as such.
In the near future we'll need to process KAS FAX files, for which we
need not only to skip over unmarked comment lines, but also to skip some
lines of the response which the FAX doesn't include.
For this we need a more powerful callback function, which this change
provides.
Change-Id: Ibb12b97ac65b3e85317d2e97386ef1c2ea263d4b
Reviewed-on: https://boringssl-review.googlesource.com/24664
Reviewed-by: Adam Langley <agl@google.com>
This extension will be used to measure the latency impact of potentially
sending a post-quantum key share by default. At this time it's purely
measuring the impact of the client sending the key share, not the server
replying with a ciphertext.
We could use the existing padding extension for this but that extension
doesn't allow the server to echo it, so we would need a different
extension in the future anyway. Thus we just create one now.
We can assume that modern clients will be using TLS 1.3 by the time that
PQ key-exchange is established and thus the key share will be sent in
all ClientHello messages. However, since TLS 1.3 isn't quite here yet,
this extension is also sent for TLS 1.0–1.2 ClientHellos. The latency
impact should be the same either way.
Change-Id: Ie4a17551f6589b28505797e8c54cddbe3338dfe5
Reviewed-on: https://boringssl-review.googlesource.com/24585
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
The P-224 implementation was missing the optimization to avoid doing
extra work when asking for only one coordinate (ECDH and ECDSA both
involve an x-coordinate query). The P-256 implementation was missing the
optimization to do one less Montgomery reduction.
TODO - Benchmarks
Change-Id: I268d9c24737c6da9efaf1c73395b73dd97355de7
Reviewed-on: https://boringssl-review.googlesource.com/24690
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
EC_POINT_set_affine_coordinates_GFp already rejects coordinates which
are out of range. There's no need to double-check.
Change-Id: Id1685355c555dda66d2a14125cb0083342f37e53
Reviewed-on: https://boringssl-review.googlesource.com/24688
Reviewed-by: Adam Langley <agl@google.com>
p224-64.c can just write straight into the EC_POINT, as the other files
do, which saves the mess around BN_CTX. It's also more correct.
ec_point_set_Jprojective_coordinates_GFp abstracts out field_encode, but
then we would want to abstract out field_decode too when reading.
That then allows us to inline ec_point_set_Jprojective_coordinates_GFp
into ec_GFp_simple_point_set_affine_coordinates and get rid of an
unnecessary tower of helper functions. Also we can use the precomputed
value of one rather than recompute it each time.
Change-Id: I8282dc66a4a437f5a3b6a1a59cc39be4cb71ccf9
Reviewed-on: https://boringssl-review.googlesource.com/24687
Reviewed-by: Adam Langley <agl@google.com>
All the messing around with field_mul and field_sqr does the same thing
as calling EC_GROUP_get_curve_GFp. This is in preparation for ultimately
moving the field elements to an EC_FELEM type.
Where we draw the BIGNUM / EC_FELEM line determines what EC_FELEM
operations we need. Since we don't care much about the performance of
this function, leave it in BIGNUM so we don't need an EC_FELEM
BN_mod_sqrt just yet. We can push it down later if we feel so inclined.
Change-Id: Iec07240d40828df6b7a29fd1f430e3b390d5f506
Reviewed-on: https://boringssl-review.googlesource.com/24686
Reviewed-by: Adam Langley <agl@google.com>
The language of RFC 5246 is "A certificate has expired or is not
currently valid", which sounds to me like |certificate_expired| should
pertain to any case where the current time is outside the
certificate's validity period.
Along the way, group the |unknown_ca| errors together.
Change-Id: I92c1fe3fc898283d0c7207625de36662cd0f784e
Reviewed-on: https://boringssl-review.googlesource.com/24624
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The Chromium certificate verifier ends up encoding a SET OF when
canonicalizing X.509 names. Requiring the caller canonicalize a SET OF
is complicated enough that we should probably sort it for folks. (We
really need to get this name canonicalization insanity out of X.509...)
This would remove the extra level of indirection in Chromium
net/cert/internal/verify_name_match.cc CBB usage.
Note this is not quite the same order as SET, but SET is kind of
useless. Since it's encoding heterogeneous values, it is reasonable to
require the caller just encode them in the correct order. In fact, a DER
SET is just SEQUENCE with a post-processing step on the definition to
fix the ordering of the fields. (Unless the SET contains an untagged
CHOICE, in which case the ordering is weird, but SETs are not really
used in the real world, much less SETs with untagged CHOICEs.)
Bug: 11
Change-Id: I51e7938a81529243e7514360f867330359ae4f2c
Reviewed-on: https://boringssl-review.googlesource.com/24444
Reviewed-by: Adam Langley <agl@google.com>
It requires a handful of additional intrinsics for now.
Fiat's freeze function only works on the tight bounds, so fe_isnonzero
gains an extra fe_carry. But all other calls of fe_tobytes are of tight
bounds anyway.
Change-Id: I834858cee7863c7344e456d7a7dbf4f414f04ae5
Reviewed-on: https://boringssl-review.googlesource.com/24545
Reviewed-by: Adam Langley <agl@google.com>
CMake targets are visible globally but gtest_main has boringssl-specific
behavior that isn't appropriate for general use.
This change makes it possible to use boringssl and abseil-cpp in the
same project (since abseil-cpp expects gtest_main to exist and be useful
for its own tests).
Change-Id: Icc81c11b8bb4b1e21cea7c9fa725b6c082bd5369
Reviewed-on: https://boringssl-review.googlesource.com/24604
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is a reland of https://boringssl-review.googlesource.com/2330. I
believe I've now cleared the fallout.
Android's attestion format uses some ludicrously large tag numbers:
https://developer.android.com/training/articles/security-key-attestation.html#certificate_schema
Add support for these in CBS/CBB. The public API does not change for
callers who were using the CBS_ASN1_* constants, but it is no longer the
case that tag representations match their DER encodings for small tag
numbers. When passing tags into CBS/CBB, use CBS_ASN1_* constants. When
working with DER byte arrays (most commonly test vectors), use the
numbers themselves.
Bug: 214
Update-Note: The in-memory representation of CBS/CBB tags changes.
Additionally, we now support tag numbers above 30. I believe I've now
actually cleared the fallout of the former. There is one test in
Chromium and the same test in the internal repository that needs
fixing.
Change-Id: I49b9d30df01f023c646d31156360ff69c91626a3
Reviewed-on: https://boringssl-review.googlesource.com/24404
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is to simplify
https://boringssl-review.googlesource.com/c/boringssl/+/24445/.
Setting or changing an EC_KEY's group after the public or private keys
have been configured is quite awkward w.r.t. consistency checks. It
becomes additionally messy if we mean to store private keys as
EC_SCALARs (and avoid the BIGNUM timing leak), whose size is
curve-dependent.
Instead, require that callers configure the group before setting either
half of the keypair. Additionally, reject EC_KEY_set_group calls that
change the group. This will simplify clearing one more BIGNUM timing
leak.
Update-Note: This will break code which sets the group and key in a
weird order. I checked calls of EC_KEY_new and confirmed they all
set the group first. If I missed any, let me know.
Change-Id: Ie89f90a318b31b6b98f71138e5ff3de5323bc9a6
Reviewed-on: https://boringssl-review.googlesource.com/24425
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This function maps |X509_V_ERR_*| to SSL alarm codes. It's used
internally when certs are verified with X509_verify_cert(), and is
helpful to callers who want to call that function, but who also want
to report its errors in a less implementation-dependent way.
Change-Id: I2900cce2eb631489f0947c317beafafd3ea57a75
Reviewed-on: https://boringssl-review.googlesource.com/24564
Commit-Queue: Matt Braithwaite <mab@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
|ASN1_INTEGER_set| and |BN_to_ASN1_INTEGER| disagree about how to encode
zero. OpenSSL master has aligned around the behaviour of the latter
(i.e. a single zero byte) so fix |ASN1_INTEGER_set| to do that. (This is
also the form that DER requires.)
At the same time, fix undefined behaviour when negative a |long| whose
value is |LONG_MIN|.
Change-Id: I1198de35e61a286ac6472e99152f3d22fda59044
Reviewed-on: https://boringssl-review.googlesource.com/24485
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
TLS 1.3 includes a server-random-based anti-downgrade signal, as a
workaround for TLS 1.2's ServerKeyExchange signature failing to cover
the entire handshake. However, because TLS 1.3 draft versions are each
doomed to die, we cannot deploy it until the final RFC. (Suppose a
draft-TLS-1.3 client checked the signal and spoke to a final-TLS-1.3
server. The server would correctly negotiate TLS 1.2 and send the
signal. But the client would then break. An anologous situation exists
with reversed roles.)
However, it appears that Cisco devices have non-compliant TLS 1.2
implementations[1] and copy over another server's server-random when
acting as a TLS terminator (client and server back-to-back).
Hopefully they are the only ones doing this. Implement a
measurement-only version with a different value. This sentinel must not
be enforced, but it will tell us whether enforcing it will cause
problems.
[1] https://www.ietf.org/mail-archive/web/tls/current/msg25168.html
Bug: 226
Change-Id: I976880bdb2ef26f51592b2f6b3b97664342679c8
Reviewed-on: https://boringssl-review.googlesource.com/24284
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
RSA_METHOD_FLAG_NO_CHECK is the same as our RSA_FLAG_OPAQUE. cURL uses
this to determine if it should call SSL_CTX_check_private_key.
Change-Id: Ie2953632346a31de346a4452f4eaad8435cf76e8
Reviewed-on: https://boringssl-review.googlesource.com/24245
Reviewed-by: Adam Langley <agl@google.com>
Update-Note: Some RSA_FLAG_* constants are gone. Code search says they
were unused, but they can be easily restored if this breaks anything.
Change-Id: I47f642af5af9f8d80972ca8da0a0c2bd271c20eb
Reviewed-on: https://boringssl-review.googlesource.com/24244
Reviewed-by: Adam Langley <agl@google.com>
Upgrade-Note: SSL_CTX_set_tls13_variant(tls13_experiment) on the server
should switch to SSL_CTX_set_tls13_variant(tls13_experiment2).
(Configuring any TLS 1.3 variants on the server enables all variants,
so this is a no-op. We're just retiring some old experiments.)
Change-Id: I60f0ca3f96ff84bdf59e1a282a46e51d99047462
Reviewed-on: https://boringssl-review.googlesource.com/23784
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
The first step of RSA with the CRT optimization is to reduce our input
modulo p and q. We can do this in constant-time[*] with Montgomery
reduction. When p and q are the same size, Montgomery reduction's bounds
hold. We need two rounds of it because the first round gives us an
unwanted R^-1.
This does not appear to have a measurable impact on performance. Also
add a long TODO describing how to make the rest of the function
constant-time[*] which hopefully we'll get to later. RSA blinding should
protect us from it all, but make this constant-time anyway.
Since this and the follow-up work will special-case weird keys, add a
test that we don't break those unintentionally. (Though I am not above
breaking them intentionally someday...)
Thanks to Andres Erbsen for discussions on how to do this bit properly.
[*] Ignoring the pervasive bn_correct_top problem for the moment.
Change-Id: Ide099a9db8249cb6549be99c5f8791a39692ea81
Reviewed-on: https://boringssl-review.googlesource.com/24204
Reviewed-by: Adam Langley <agl@google.com>
Originally, the only OpenSSL API to stringify errors was:
char *ERR_error_string(unsigned long e, char *buf);
This API leaves callers a choice to either be thread unsafe (buf = NULL)
or pass in a buffer with unknown size. Indeed the original
implementation was just a bunch of unchecked sprintfs with, in the buf =
NULL case, a static 256-byte buffer.
https://github.com/openssl/openssl/blob/388f2f56f213dfada0370d48cb9bcc3c7e980b32/crypto/err/err.c#L374
Then ERR_error_string was documented that the buffer must be size 120.
Nowhere in the code was 120 significant. I expect OpenSSL just made up a
number.
https://github.com/openssl/openssl/commit/388f2f56f213dfada0370d48cb9bcc3c7e980b32
Then upstream added the ERR_error_string_n API. Although the
documentation stated 120 bytes, the internal buffer was 256, so the code
actually translates ERR_error_string to ERR_error_string_n(e, buf, 256),
not ERR_error_string_n(e, buf, 120)!
https://github.com/openssl/openssl/commit/e5c84d5152c11a3dfa436041d3336a6f403baad8
So the documentation was wrong all this time! OpenSSL 1.1.0 corrected
the documentation to 256, but, alas, a lot of code used the
documentation and sized the buffer at 120. We should fix all
ERR_error_string callers to ERR_error_string_n but, in the meantime,
using 120 is probably less effort.
Note this also affects ERR_print_errors_cb right now. We don't have
function codes, so 120 bytes leaves 60 bytes for the reason code. Our
longest one, TLS_PEER_DID_NOT_RESPOND_WITH_CERTIFICATE_LIST is 46 bytes,
so it's a little tight, but, if needed, we can recover 20-ish bytes by
shrinking the library names. We can also always make ERR_print_errors_cb
use a larger buffer.
Change-Id: I472a1a802f2e6281cc7515d2a452208d6bac1200
Reviewed-on: https://boringssl-review.googlesource.com/24184
Reviewed-by: Adam Langley <agl@google.com>
ARMv8 kindly deprecated most of its IT instructions in Thumb mode.
These files are taken from upstream and are used on both ARMv7 and ARMv8
processors. Accordingly, silence the warnings by marking the file as
targetting ARMv7. In other files, they were accidentally silenced anyway
by way of the existing .arch lines.
This can be reproduced by building with the new NDK and passing
-DCMAKE_ASM_FLAGS=-march=armv8-a. Some of our downstream code ends up
passing that to the assembly.
Note this change does not attempt to arrange for ARMv8-A/T32 to get
code which honors the constraints. It only silences the warnings and
continues to give it the same ARMv7-A/Thumb-2 code that backwards
compatibility dictates it continue to run.
Bug: chromium:575886, b/63131949
Change-Id: I24ce0b695942eaac799347922b243353b43ad7df
Reviewed-on: https://boringssl-review.googlesource.com/24166
Reviewed-by: Adam Langley <agl@google.com>
The one in the NDK works just fine. In particular, this means one can
pass -DCMAKE_ASM_FLAGS="-march=armv8-a" and test the ARMv8 assembler
warnings.
Additionally, make the workaround put the flags in the other order, so
-march is user-overridable.
Change-Id: I278ddd17ab688f83ee01f2aca4ff32307f5b0a2d
Reviewed-on: https://boringssl-review.googlesource.com/24164
Reviewed-by: Adam Langley <agl@google.com>
The third-party toolchain file doesn't actually work with newer NDKs,
and the one shipped with the NDK has fewer bugs.
Change-Id: I59e1db393f0d66b186fb71590fab14db7faa0756
Reviewed-on: https://boringssl-review.googlesource.com/24165
Reviewed-by: Adam Langley <agl@google.com>
This makes it difficult to build against the NDK's toolchain file. The
problem is __clang__ just means Clang is the frontend and implies
nothing about which assembler. When using as, it is fine. When using
clang-as on Linux, one needs a clang-as from this year.
The only places where we case about clang's integrated assembler are iOS
(where perlasm strips out .arch anyway) and build environments like
Chromium which have a regularly-updated clang. Thus we can remove this
now.
Bug: 39
Update-Note: Holler if this breaks the build. If it doesn't break the
build, you can probably remove any BORINGSSL_CLANG_SUPPORTS_DOT_ARCH
or explicit -march armv8-a+crypto lines in your BoringSSL build.
Change-Id: I21ce54b14c659830520c2f1d51c7bd13e0980c68
Reviewed-on: https://boringssl-review.googlesource.com/24124
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
It actually works fine. I just forgot one of the typedefs last time.
This gives a roughly 2x improvement on P-256 in clang-cl +
OPENSSL_SMALL, the configuration used by Chrome.
Before:
Did 1302 ECDH P-256 operations in 1015000us (1282.8 ops/sec)
Did 4250 ECDSA P-256 signing operations in 1047000us (4059.2 ops/sec)
Did 1750 ECDSA P-256 verify operations in 1094000us (1599.6 ops/sec)
After:
Did 3250 ECDH P-256 operations in 1078000us (3014.8 ops/sec)
Did 8250 ECDSA P-256 signing operations in 1016000us (8120.1 ops/sec)
Did 3250 ECDSA P-256 verify operations in 1063000us (3057.4 ops/sec)
(These were taken on a VM, so the measurements are extremely noisy, but
this sort of improvement is visible regardless.)
Alas, we do need a little extra bit of fiddling because division does
not work (crbug.com/787617).
Bug: chromium:787617
Update-Note: This removes the MSan uint128_t workaround which does not
appear to be necessary anymore.
Change-Id: I8361314608521e5bdaf0e7eeae7a02c33f55c69f
Reviewed-on: https://boringssl-review.googlesource.com/23984
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The fiat-crypto-generated code uses the Montgomery form implementation
strategy, for both 32-bit and 64-bit code.
64-bit throughput seems slower, but the difference is smaller than noise between repetitions (-2%?)
32-bit throughput has decreased significantly for ECDH (-40%). I am
attributing this to the change from varibale-time scalar multiplication
to constant-time scalar multiplication. Due to the same bottleneck,
ECDSA verification still uses the old code (otherwise there would have
been a 60% throughput decrease). On the other hand, ECDSA signing
throughput has increased slightly (+10%), perhaps due to the use of a
precomputed table of multiples of the base point.
64-bit benchmarks (Google Cloud Haswell):
with this change:
Did 9126 ECDH P-256 operations in 1009572us (9039.5 ops/sec)
Did 23000 ECDSA P-256 signing operations in 1039832us (22119.0 ops/sec)
Did 8820 ECDSA P-256 verify operations in 1024242us (8611.2 ops/sec)
master (40e8c921ca):
Did 9340 ECDH P-256 operations in 1017975us (9175.1 ops/sec)
Did 23000 ECDSA P-256 signing operations in 1039820us (22119.2 ops/sec)
Did 8688 ECDSA P-256 verify operations in 1021108us (8508.4 ops/sec)
benchmarks on ARMv7 (LG Nexus 4):
with this change:
Did 150 ECDH P-256 operations in 1029726us (145.7 ops/sec)
Did 506 ECDSA P-256 signing operations in 1065192us (475.0 ops/sec)
Did 363 ECDSA P-256 verify operations in 1033298us (351.3 ops/sec)
master (2fce1beda0):
Did 245 ECDH P-256 operations in 1017518us (240.8 ops/sec)
Did 473 ECDSA P-256 signing operations in 1086281us (435.4 ops/sec)
Did 360 ECDSA P-256 verify operations in 1003846us (358.6 ops/sec)
64-bit tables converted as follows:
import re, sys, math
p = 2**256 - 2**224 + 2**192 + 2**96 - 1
R = 2**256
def convert(t):
x0, s1, x1, s2, x2, s3, x3 = t.groups()
v = int(x0, 0) + 2**64 * (int(x1, 0) + 2**64*(int(x2,0) + 2**64*(int(x3, 0)) ))
w = v*R%p
y0 = hex(w%(2**64))
y1 = hex((w>>64)%(2**64))
y2 = hex((w>>(2*64))%(2**64))
y3 = hex((w>>(3*64))%(2**64))
ww = int(y0, 0) + 2**64 * (int(y1, 0) + 2**64*(int(y2,0) + 2**64*(int(y3, 0)) ))
if ww != v*R%p:
print(x0,x1,x2,x3)
print(hex(v))
print(y0,y1,y2,y3)
print(hex(w))
print(hex(ww))
assert 0
return '{'+y0+s1+y1+s2+y2+s3+y3+'}'
fe_re = re.compile('{'+r'(\s*,\s*)'.join(r'(\d+|0x[abcdefABCDEF0123456789]+)' for i in range(4)) + '}')
print (re.sub(fe_re, convert, sys.stdin.read()).rstrip('\n'))
32-bit tables converted from 64-bit tables
Change-Id: I52d6e5504fcb6ca2e8b0ee13727f4500c80c1799
Reviewed-on: https://boringssl-review.googlesource.com/23244
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
It's misnamed but, more importantly, doesn't do anything because the
test client isn't sending early data to begin with. We really need to
make these tests less error-prone to write. With this fix, the test
actually notices if we remove the server-side 0-RTT check.
Also remove MaxEarlyDataSize from the other server tests which
erroneously set it. Any test with sets that was likely copy-and-pasted
incorrectly.
Change-Id: Idc24bc1590e0316946022341185285418ab8c77b
Reviewed-on: https://boringssl-review.googlesource.com/23944
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Along the way, this allows us to tidy up the invariants associated with
EC_SCALAR. They were fuzzy around ec_point_mul_scalar and some
computations starting from the digest in ECDSA. The latter I've put into
the type system with EC_LOOSE_SCALAR.
As for the former, Andres points out that particular EC implementations
are only good for scalars within a certain range, otherwise you may need
extra work to avoid the doubling case. To simplify curve
implementations, we reduce them fully rather than do the looser bit size
check, so they can have the stronger precondition to work with.
Change-Id: Iff9a0404f89adf8f7f914f8e8246c9f3136453f1
Reviewed-on: https://boringssl-review.googlesource.com/23664
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The newer clang-cl is unhappy about the tautological comparison on
Windows, but the comparison itself is unnecessary anyway, since the
values will never exceed uint32_t.
I think the reason it's not firing elsewhere is because on other 64-bit
platforms, it is not tautological because long is 64-bit. On other
32-bit platforms, I'm not sure we actually have a standalone trunk clang
builder right now.
Update-Note: UTF8_getc and UTF8_putc were unexported. No one appears to
be calling them. (We're a crypto library, not a Unicode library.)
Change-Id: I0949ddea3131dca5f55d04e672c3ccf2915c41ab
Reviewed-on: https://boringssl-review.googlesource.com/23844
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Credit to OSS-Fuzz for finding this.
CVE-2017-3738
(Imported from upstream's 5630661aecbea5fe3c4740f5fea744a1f07a6253 and
77d75993651b63e872244a3256e37967bb3c3e9e.)
Confirmed with Intel SDE that the fix makes the test vector pass and
that, without the fix, the test vector does not. (Well, we knew the
latter already, since it was our test vector.)
Change-Id: I167aa3407ddab3b434bacbd18e099c55aa40ac4c
Reviewed-on: https://boringssl-review.googlesource.com/23884
Reviewed-by: Adam Langley <agl@google.com>
We check that the private key is less than the order, but we forgot the
other end.
Update-Note: It's possible some caller was relying on this, but since
that function already checked the other half of the range, I'm
expecting this to be a no-op change.
Change-Id: I4a53357d7737735b3cfbe97d379c8ca4eca5d5ac
Reviewed-on: https://boringssl-review.googlesource.com/23665
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This is a recent Clang warning, but it's far too aggressive. The earlier
unsigned long silliness was worth fixing, but it otherwise complains on
32-bit platforms with:
if (some_size_t > 0xffffffff) {
...
}
which is unreasonable as, on 64-bit platforms, this check is meaningful
and requiring the programmer add ifdefs is error-prone. This matches
Chromium in https://crbug.com/767059.
Bug: chromium:767059
Change-Id: I0bb0f3a4b60f222e9d1b3c569471fbcf5518caed
Reviewed-on: https://boringssl-review.googlesource.com/23845
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
We can probably do this globally at this point since the cipher
requirements are much more restrict than they were in the beginning.
(Firefox, in particular, has done so far a while.) For now add a flag
since some consumer wanted this.
I'll see about connecting it to a Chrome field trial after our breakage
budget is no longer reserved for TLS 1.3.
Change-Id: Ib61dd5aae2dfd48b56e79873a7f3061a7631a5f8
Reviewed-on: https://boringssl-review.googlesource.com/23725
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Rejecting values where we'd previous called BN_nnmod may have been
overly ambitious. In the long run, all the supported ECC APIs (ECDSA*,
ECDH_compute_key, and probably some additional new ECDH API) will be
using the EC_SCALAR version anyway, so this doesn't really matter.
Change-Id: I79cd4015f2d6daf213e4413caa2a497608976f93
Reviewed-on: https://boringssl-review.googlesource.com/23584
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This reverts commit 66801feb17. This
turned out to break a lot more than expected. Hopefully we can reland it
soon, but we need to fix up some consumers first.
Note due to work that went in later, this is not a trivial revert and
should be re-reviewed.
Change-Id: I6474b67cce9a8aa03f722f37ad45914b76466bea
Reviewed-on: https://boringssl-review.googlesource.com/23644
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
OBJ_txt2obj is currently implemented using BIGNUMs which is absurd. It
also depends on the giant OID table, which is undesirable. Write a new
one and expose the low-level function so Chromium can use it without the
OID table.
Bug: chromium:706445
Change-Id: I61ff750a914194f8776cb8d81ba5d3eb5eaa3c3d
Reviewed-on: https://boringssl-review.googlesource.com/23364
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
This avoids taking quadratic time to pretty-print certificates with
excessively large integer fields. Very large integers aren't any more
readable in decimal than hexadecimal anyway, and the i2s_* functions
will parse either form.
Found by libFuzzer.
Change-Id: Id586cd1b0eef8936d38ff50433ae7c819f0054f3
Reviewed-on: https://boringssl-review.googlesource.com/23424
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
DECLARE_STACK_OF adds a trailing ; so we don't need a second one added
here.
Compiling a project using boringssl which uses -Werror,-Wextra-semi I
get errors:
```
third_party/boringssl/include/openssl/stack.h:374:1: error: extra ';' outside of a function [-Werror,-Wextra-semi]
DEFINE_STACK_OF(void)
^
third_party/boringssl/include/openssl/stack.h:355:3: note: expanded from macro 'DEFINE_STACK_OF'
BORINGSSL_DEFINE_STACK_OF_IMPL(type, type *, const type *) \
^
third_party/boringssl/include/openssl/stack.h:248:25: note: expanded from macro 'BORINGSSL_DEFINE_STACK_OF_IMPL'
DECLARE_STACK_OF(name); \
^
third_party/boringssl/include/openssl/stack.h:375:1: error: extra ';' outside of a function [-Werror,-Wextra-semi]
DEFINE_SPECIAL_STACK_OF(OPENSSL_STRING)
^
third_party/boringssl/include/openssl/stack.h:369:3: note: expanded from macro 'DEFINE_SPECIAL_STACK_OF'
BORINGSSL_DEFINE_STACK_OF_IMPL(type, type, const type)
^
third_party/boringssl/include/openssl/stack.h:248:25: note: expanded from macro 'BORINGSSL_DEFINE_STACK_OF_IMPL'
DECLARE_STACK_OF(name); \
^
2 errors generated.
```
Change-Id: Icc39e2341eb76544be72d2d7d0bd29e2f1ed0bf9
Reviewed-on: https://boringssl-review.googlesource.com/23404
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Matches the OpenSSL 1.1.0 spelling, which is what we advertise in
OPENSSL_VERSION_NUMBER now. Otherwise third-party code which uses it
will, in the long term, need ifdefs. Note this will require updates to
any existing callers (there appear to only be a couple of them), but it
should be straightforward.
Change-Id: I9dd1013609abca547152728a293529055dacc239
Reviewed-on: https://boringssl-review.googlesource.com/23325
Reviewed-by: Adam Langley <agl@google.com>
This is only a hair faster than the signing change, but still something.
I kept the call to BN_mod_inverse_odd as that appears to be faster
(constant time is not a concern for verification).
Before:
Did 22855 ECDSA P-224 verify operations in 3015099us (7580.2 ops/sec)
Did 21276 ECDSA P-256 verify operations in 3083284us (6900.4 ops/sec)
Did 2635 ECDSA P-384 verify operations in 3032582us (868.9 ops/sec)
Did 1240 ECDSA P-521 verify operations in 3068631us (404.1 ops/sec)
After:
Did 23310 ECDSA P-224 verify operations in 3056226us (7627.1 ops/sec)
Did 21210 ECDSA P-256 verify operations in 3035765us (6986.7 ops/sec)
Did 2666 ECDSA P-384 verify operations in 3023592us (881.7 ops/sec)
Did 1209 ECDSA P-521 verify operations in 3054040us (395.9 ops/sec)
Change-Id: Iec995b1a959dbc83049d0f05bdc525c14a95c28e
Reviewed-on: https://boringssl-review.googlesource.com/23077
Reviewed-by: Adam Langley <agl@google.com>
Hasse's theorem implies at most one subtraction is necessary. This is
still using BIGNUM for now because field elements
(EC_POINT_get_affine_coordinates_GFp) are BIGNUMs.
This gives an additional 2% speedup for signing.
Before:
Did 16000 ECDSA P-224 signing operations in 1064799us (15026.3 ops/sec)
Did 19000 ECDSA P-256 signing operations in 1007839us (18852.2 ops/sec)
Did 1078 ECDSA P-384 signing operations in 1079413us (998.7 ops/sec)
Did 484 ECDSA P-521 signing operations in 1083616us (446.7 ops/sec)
After:
Did 16000 ECDSA P-224 signing operations in 1054918us (15167.1 ops/sec)
Did 20000 ECDSA P-256 signing operations in 1037338us (19280.1 ops/sec)
Did 1045 ECDSA P-384 signing operations in 1049073us (996.1 ops/sec)
Did 484 ECDSA P-521 signing operations in 1085492us (445.9 ops/sec)
Change-Id: I2bfe214f968eca7a8e317928c0f3daf1a14bca90
Reviewed-on: https://boringssl-review.googlesource.com/23076
Reviewed-by: Adam Langley <agl@google.com>
None of the asymmetric crypto we inherented from OpenSSL is
constant-time because of BIGNUM. BIGNUM chops leading zeros off the
front of everything, so we end up leaking information about the first
word, in theory. BIGNUM functions additionally tend to take the full
range of inputs and then call into BN_nnmod at various points.
All our secret values should be acted on in constant-time, but k in
ECDSA is a particularly sensitive value. So, ecdsa_sign_setup, in an
attempt to mitigate the BIGNUM leaks, would add a couple copies of the
order.
This does not work at all. k is used to compute two values: k^-1 and kG.
The first operation when computing k^-1 is to call BN_nnmod if k is out
of range. The entry point to our tuned constant-time curve
implementations is to call BN_nnmod if the scalar has too many bits,
which this causes. The result is both corrections are immediately undone
but cause us to do more variable-time work in the meantime.
Replace all these computations around k with the word-based functions
added in the various preceding CLs. In doing so, replace the BN_mod_mul
calls (which internally call BN_nnmod) with Montgomery reduction. We can
avoid taking k^-1 out of Montgomery form, which combines nicely with
Brian Smith's trick in 3426d10119. Along
the way, we avoid some unnecessary mallocs.
BIGNUM still affects the private key itself, as well as the EC_POINTs.
But this should hopefully be much better now. Also it's 10% faster:
Before:
Did 15000 ECDSA P-224 signing operations in 1069117us (14030.3 ops/sec)
Did 18000 ECDSA P-256 signing operations in 1053908us (17079.3 ops/sec)
Did 1078 ECDSA P-384 signing operations in 1087853us (990.9 ops/sec)
Did 473 ECDSA P-521 signing operations in 1069835us (442.1 ops/sec)
After:
Did 16000 ECDSA P-224 signing operations in 1064799us (15026.3 ops/sec)
Did 19000 ECDSA P-256 signing operations in 1007839us (18852.2 ops/sec)
Did 1078 ECDSA P-384 signing operations in 1079413us (998.7 ops/sec)
Did 484 ECDSA P-521 signing operations in 1083616us (446.7 ops/sec)
Change-Id: I2a25e90fc99dac13c0616d0ea45e125a4bd8cca1
Reviewed-on: https://boringssl-review.googlesource.com/23075
Reviewed-by: Adam Langley <agl@google.com>
Imported from upstream's a78324d95bd4568ce2c3b34bfa1d6f14cddf92ef. I
think the "regression" part of that change is some tweak to BN_usub and
I guess the bn_*_words was to compensate for it, but we may as well
import it. Apparently the loop instruction is terrible.
Before:
Did 39871000 bn_add_words operations in 1000002us (39870920.3 ops/sec)
Did 38621750 bn_sub_words operations in 1000001us (38621711.4 ops/sec)
After:
Did 64012000 bn_add_words operations in 1000007us (64011551.9 ops/sec)
Did 81792250 bn_sub_words operations in 1000002us (81792086.4 ops/sec)
loop sets no flags (even doing the comparison to zero without ZF) while
dec sets all flags but CF, so Andres and I are assuming that because
this prevents Intel from microcoding it to dec/jnz, they otherwise can't
be bothered to add more circuitry since every compiler has internalized
by now to never use loop.
Change-Id: I3927cd1c7b707841bbe9963e3d4afd7ba9bd9b36
Reviewed-on: https://boringssl-review.googlesource.com/23344
Reviewed-by: Adam Langley <agl@google.com>
These allow precomputation of k, but bypass our nonce hardening and also
make it harder to excise BIGNUM. As a bonus, ECDSATest.SignTestVectors
is now actually covering the k^-1 and r computations.
Change-Id: I4c71dae162874a88a182387ac43999be9559ddd7
Reviewed-on: https://boringssl-review.googlesource.com/23074
Reviewed-by: Adam Langley <agl@google.com>
After much procrastinating, we finally moved Chromium to the new stuff.
We can now delete this. This is a breaking change for
SSL_PRIVATE_KEY_METHOD consumers, but it should be trivial (remove some
unused fields in the struct). I've bumped BORINGSSL_API_VERSION to ease
any multi-sided changes that may be needed.
Change-Id: I9fe562590ad938bcb4fcf9af0fadeff1d48745fb
Reviewed-on: https://boringssl-review.googlesource.com/23224
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
(Imported from upstream's c29f83c05f3a3c5641c5ddf054789a29d2163bf3.)
ext was being leaked. Upstream also did some stuff around *x which
wasn't strictly necessary (usually OpenSSL only provides basic
exception safety, not strong exception safety), but ah well.
Change-Id: I52d230990b05501b4cee6deee8dcacba4a926c18
Reviewed-on: https://boringssl-review.googlesource.com/23204
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
mem.h for |OPENSSL_cleanse| and bn/internal.h for things like
|bn_less_than_words| and |bn_correct_top|.
Change-Id: I3c447a565dd9e4f18fb2ff5d59f80564b4df8cea
Reviewed-on: https://boringssl-review.googlesource.com/23164
Reviewed-by: Adam Langley <agl@google.com>
|EC_POINT_mul| is almost exclusively used with reduced scalars, with
this exception. This comes from consumers following NIST SP 800-56A
section 5.6.2.3.2. (Though all our curves have cofactor one, so this
check isn't useful.)
Add a test for this so we don't accidentally break it.
Change-Id: I42492db38a1ea03acec4febdd7945c8a3933530a
Reviewed-on: https://boringssl-review.googlesource.com/23084
Reviewed-by: Adam Langley <agl@google.com>
We were only running a random subset of TLS 1.3 tests with variants and
let a lot of bugs through as a result.
- HelloRetryRequest-EmptyCookie wasn't actually testing what we were
trying to test.
- The second HelloRetryRequest detection needs tweaks in draft-22.
- The empty HelloRetryRequest logic can't be based on non-empty
extensions in draft-22.
- We weren't sending ChangeCipherSpec correctly in HRR or testing it
right.
- Rework how runner reads ChangeCipherSpec by setting a flag which
affects the next readRecord. This cuts down a lot of cases and works
correctly if the client didn't send early data. (In that case, we
don't flush CCS until EndOfEarlyData and runner deadlocks waiting for
the ChangeCipherSpec to arrive.)
Change-Id: I559c96ea3a8b350067e391941231713c6edb2f78
Reviewed-on: https://boringssl-review.googlesource.com/23125
Reviewed-by: Steven Valdez <svaldez@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
I still need to revive the original CL, but right now I'm interested in
giving every EC_GROUP an order_mont and having different ownership of
that field between built-in and custom groups is kind of a nuisance. If
I'm going to do that anyway, better to avoid computing the entire
EC_GROUP in one go.
I'm using some manual locking rather than CRYPTO_once here so that it
behaves well in the face of malloc errors. Not that we especially care,
but it was easy to do.
This speeds up our ECDH benchmark a bit which otherwise must construct the
EC_GROUP each time (matching real world usage).
Before:
Did 7619 ECDH P-224 operations in 1003190us (7594.8 ops/sec)
Did 7518 ECDH P-256 operations in 1060844us (7086.8 ops/sec)
Did 572 ECDH P-384 operations in 1055878us (541.7 ops/sec)
Did 264 ECDH P-521 operations in 1062375us (248.5 ops/sec)
After:
Did 8415 ECDH P-224 operations in 1066695us (7888.9 ops/sec)
Did 7952 ECDH P-256 operations in 1022819us (7774.6 ops/sec)
Did 572 ECDH P-384 operations in 1055817us (541.8 ops/sec)
Did 264 ECDH P-521 operations in 1060008us (249.1 ops/sec)
Bug: 20
Change-Id: I7446cd0a69a840551dcc2dfabadde8ee1e3ff3e2
Reviewed-on: https://boringssl-review.googlesource.com/23073
Reviewed-by: Adam Langley <agl@google.com>
Later code will take advantage of these invariants. Enforcing them on
custom curves avoids making them go through a custom codepath.
Change-Id: I23cee72a90c2e4846b41e03e6be26bc3abeb4a45
Reviewed-on: https://boringssl-review.googlesource.com/23072
Reviewed-by: Adam Langley <agl@google.com>
These can be used to invert values in ECDSA. Unlike their BIGNUM
counterparts, the caller is responsible for taking values in and out of
Montgomery domain. This will save some work later on in the ECDSA
computation.
Change-Id: Ib7292900a0fdeedce6cb3e9a9123c94863659043
Reviewed-on: https://boringssl-review.googlesource.com/23071
Reviewed-by: Adam Langley <agl@google.com>
bn_from_montgomery_in_place is actually constant-time. It is, of course,
only used by non-constant-time BIGNUM callers, but that will soon be
fixed.
Change-Id: I2b2c9943dc3b8d6a4b5b19a5bc4fa9ebad532bac
Reviewed-on: https://boringssl-review.googlesource.com/23069
Reviewed-by: Adam Langley <agl@google.com>
As part of excising BIGNUM from EC scalars, we will need a "words"
version of BN_mod_mul_montgomery. That, in turn, requires BN_sqr and
BN_mul for cases where we don't have bn_mul_mont.
BN_sqr and BN_mul have a lot of logic in there, with the most complex
cases being not even remotely constant time. Fortunately, those only
apply to RSA-sized numbers, not EC-sized numbers. (With the exception, I
believe, of 32-bit P-521 which just barely exceeds the cutoff.) Imposing
a limit also makes it easier to stack-allocate temporaries (BN_CTX
serves a similar purpose in BIGNUM).
Extract bn_mul_small and bn_sqr_small and test them as part of
bn_tests.txt. Later changes will build on these.
If we end up reusing these functions for RSA in the future (though that
would require tending to the egregiously non-constant-time code in the
no-asm build), we probably want to extract a version where there is an
explicit tmp parameter as in bn_sqr_normal rather than the stack bits.
Change-Id: If414981eefe12d6664ab2f5e991a359534aa7532
Reviewed-on: https://boringssl-review.googlesource.com/23068
Reviewed-by: Adam Langley <agl@google.com>
This rewrites the internals with a "words" variant that can avoid
bn_correct_top. It still ultimately calls bn_correct_top as the calling
convention is sadly still BIGNUM, but we can lift that calling
convention out incrementally.
Performance seems to be comparable, if not faster.
Before:
Did 85000 ECDSA P-256 signing operations in 5030401us (16897.3 ops/sec)
Did 34278 ECDSA P-256 verify operations in 5048029us (6790.4 ops/sec)
After:
Did 85000 ECDSA P-256 signing operations in 5021057us (16928.7 ops/sec)
Did 34086 ECDSA P-256 verify operations in 5010416us (6803.0 ops/sec)
Change-Id: I1159746dfcc00726dc3f28396076a354556e6e7d
Reviewed-on: https://boringssl-review.googlesource.com/23065
Reviewed-by: Adam Langley <agl@google.com>
BN_from_montgomery_word doesn't have a constant memory access pattern.
Replace the pointer trick with constant_time_select_w. There is, of
course, still the bn_correct_top leak pervasive in BIGNUM itself.
I wasn't able to measure a performance on RSA operations before or after
this change, but the benchmarks would vary wildly run to run. But one
would assume the logic here is nothing compared to the actual reduction.
Change-Id: Ide761fde3a091a93679f0a803a287aa5d0d4600d
Reviewed-on: https://boringssl-review.googlesource.com/22904
Reviewed-by: Adam Langley <agl@google.com>
See https://github.com/tlswg/tls13-spec/pull/1083. We misread the
original text spec, but it turns out the original spec text required
senders have version-specific maximum send fragments. The PR fixes this
off-by-one issue. Align with the new spec text uniformly.
This is a wire format change for our existing drafts *only if* records
have padding. We don't currently send padding, so this is fine. Unpadded
records continue to be capped at 2^14 bytes of plaintext (or 2^14+1
bytes of TLSInnerPlaintext structure).
Change-Id: I01017cfd13162504bb163dd59afd74aff0896cc4
Reviewed-on: https://boringssl-review.googlesource.com/23004
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This was primarily for my own understanding, but this should hopefully
also be clearer and more amenable to using unsigned indices later.
Change-Id: I09cc3d55de0f7d9284d3b3168d8b0446274b2ab7
Reviewed-on: https://boringssl-review.googlesource.com/22889
Reviewed-by: Adam Langley <agl@google.com>
Normal shifts do the trick just fine and are less likely to tempt the
compiler into inserting a jump.
Change-Id: Iaa1da1b6f986fd447694fcde8f3525efb9eeaf11
Reviewed-on: https://boringssl-review.googlesource.com/22888
Reviewed-by: Adam Langley <agl@google.com>
Even without strict-aliasing, C does not allow casting pointers to types
that don't match their alignment. After this change, UBSan is happy with
our code at default settings but for the negative left shift language
bug.
Note: architectures without unaligned loads do not generate the same
code for memcpy and pointer casts. But even ARMv6 can perform unaligned
loads and stores (ARMv5 couldn't), so we should be okay here.
Before:
Did 11086000 AES-128-GCM (16 bytes) seal operations in 5000391us (2217026.6 ops/sec): 35.5 MB/s
Did 370000 AES-128-GCM (1350 bytes) seal operations in 5005208us (73923.0 ops/sec): 99.8 MB/s
Did 63000 AES-128-GCM (8192 bytes) seal operations in 5029958us (12525.0 ops/sec): 102.6 MB/s
Did 9894000 AES-256-GCM (16 bytes) seal operations in 5000017us (1978793.3 ops/sec): 31.7 MB/s
Did 316000 AES-256-GCM (1350 bytes) seal operations in 5005564us (63129.7 ops/sec): 85.2 MB/s
Did 54000 AES-256-GCM (8192 bytes) seal operations in 5054156us (10684.3 ops/sec): 87.5 MB/s
After:
Did 11026000 AES-128-GCM (16 bytes) seal operations in 5000197us (2205113.1 ops/sec): 35.3 MB/s
Did 370000 AES-128-GCM (1350 bytes) seal operations in 5005781us (73914.5 ops/sec): 99.8 MB/s
Did 63000 AES-128-GCM (8192 bytes) seal operations in 5032695us (12518.1 ops/sec): 102.5 MB/s
Did 9831750 AES-256-GCM (16 bytes) seal operations in 5000010us (1966346.1 ops/sec): 31.5 MB/s
Did 316000 AES-256-GCM (1350 bytes) seal operations in 5005702us (63128.0 ops/sec): 85.2 MB/s
Did 54000 AES-256-GCM (8192 bytes) seal operations in 5053642us (10685.4 ops/sec): 87.5 MB/s
(Tested with the no-asm builds; most of this code isn't reachable
otherwise.)
Change-Id: I025c365d26491abed0116b0de3b7612159e52297
Reviewed-on: https://boringssl-review.googlesource.com/22804
Reviewed-by: Adam Langley <agl@google.com>
This avoids upsetting the C compiler. UBSan is offended by the alignment
violations in those functions. The business with offset is also
undefined behavior (pointer arithmetic is supposed to stay within a
single object).
There is a small performance cost, however:
Before:
Did 6636000 ChaCha20-Poly1305 (16 bytes) seal operations in 5000475us (1327073.9 ops/sec): 21.2 MB/s
Did 832000 ChaCha20-Poly1305 (1350 bytes) seal operations in 5003481us (166284.2 ops/sec): 224.5 MB/s
Did 155000 ChaCha20-Poly1305 (8192 bytes) seal operations in 5026933us (30833.9 ops/sec): 252.6 MB/s
After:
Did 6508000 ChaCha20-Poly1305 (16 bytes) seal operations in 5000160us (1301558.4 ops/sec): 20.8 MB/s
Did 831000 ChaCha20-Poly1305 (1350 bytes) seal operations in 5002865us (166104.8 ops/sec): 224.2 MB/s
Did 155000 ChaCha20-Poly1305 (8192 bytes) seal operations in 5013204us (30918.4 ops/sec): 253.3 MB/s
(Tested with the no-asm build which disables the custom stitched mode
assembly and ends up using this one.)
Change-Id: I76d74183f1e04ad3726463a8871ee64be04ce674
Reviewed-on: https://boringssl-review.googlesource.com/22784
Reviewed-by: Adam Langley <agl@google.com>
It's not clear if it's a feature or bug, but binutils-2.29[.1]
interprets 'adr' instruction with Thumb2 code reference differently,
in a way that affects calculation of addresses of constants' tables.
(Imported from upstream's b82acc3c1a7f304c9df31841753a0fa76b5b3cda.)
Change-Id: Ia0f5233a9fcfaf18b9d1164bf1c88217c0cbb60d
Reviewed-on: https://boringssl-review.googlesource.com/22724
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Each operation was translated from fiat-crypto output using fiat-crypto
prettyprint.py. For example fe_mul is synthesized in
https://github.com/mit-plv/fiat-crypto/blob/master/src/Specific/X25519/C32/femul.v,
and shown in the last Coq-compatible form at
https://github.com/mit-plv/fiat-crypto/blob/master/src/Specific/X25519/C32/femulDisplay.log.
Benchmarks on Google Cloud's unidentified Intel Xeon with AVX2:
git checkout $VARIANT && ( cd build && rm -rf * && CC=clang CXX=clang++ cmake -GNinja -DCMAKE_TOOLCHAIN_FILE=../util/32-bit-toolchain.cmake -DCMAKE_BUILD_TYPE=Release .. && ninja && ./tool/bssl speed -filter 25519 )
this branch:
Did 11382 Ed25519 key generation operations in 1053046us (10808.6 ops/sec)
Did 11169 Ed25519 signing operations in 1038080us (10759.3 ops/sec)
Did 2925 Ed25519 verify operations in 1001346us (2921.1 ops/sec)
Did 12000 Curve25519 base-point multiplication operations in 1084851us (11061.4 ops/sec)
Did 3850 Curve25519 arbitrary point multiplication operations in 1085565us (3546.5 ops/sec)
Did 11466 Ed25519 key generation operations in 1049821us (10921.9 ops/sec)
Did 11000 Ed25519 signing operations in 1013317us (10855.4 ops/sec)
Did 3047 Ed25519 verify operations in 1043846us (2919.0 ops/sec)
Did 12000 Curve25519 base-point multiplication operations in 1068924us (11226.2 ops/sec)
Did 3850 Curve25519 arbitrary point multiplication operations in 1090598us (3530.2 ops/sec)
Did 10309 Ed25519 key generation operations in 1003320us (10274.9 ops/sec)
Did 11000 Ed25519 signing operations in 1017862us (10807.0 ops/sec)
Did 3135 Ed25519 verify operations in 1098624us (2853.6 ops/sec)
Did 9000 Curve25519 base-point multiplication operations in 1046608us (8599.2 ops/sec)
Did 3132 Curve25519 arbitrary point multiplication operations in 1038963us (3014.5 ops/sec)
master:
Did 11564 Ed25519 key generation operations in 1068762us (10820.0 ops/sec)
Did 11104 Ed25519 signing operations in 1024278us (10840.8 ops/sec)
Did 3206 Ed25519 verify operations in 1049179us (3055.7 ops/sec)
Did 12000 Curve25519 base-point multiplication operations in 1073619us (11177.1 ops/sec)
Did 3550 Curve25519 arbitrary point multiplication operations in 1000279us (3549.0 ops/sec)
andreser@linux-andreser:~/boringssl$ build/tool/bssl speed -filter 25519
Did 11760 Ed25519 key generation operations in 1072495us (10965.1 ops/sec)
Did 10800 Ed25519 signing operations in 1003486us (10762.5 ops/sec)
Did 3245 Ed25519 verify operations in 1080399us (3003.5 ops/sec)
Did 12000 Curve25519 base-point multiplication operations in 1076021us (11152.2 ops/sec)
Did 3570 Curve25519 arbitrary point multiplication operations in 1005087us (3551.9 ops/sec)
andreser@linux-andreser:~/boringssl$ build/tool/bssl speed -filter 25519
Did 11438 Ed25519 key generation operations in 1041115us (10986.3 ops/sec)
Did 11000 Ed25519 signing operations in 1012589us (10863.2 ops/sec)
Did 3312 Ed25519 verify operations in 1082834us (3058.6 ops/sec)
Did 12000 Curve25519 base-point multiplication operations in 1061318us (11306.7 ops/sec)
Did 3580 Curve25519 arbitrary point multiplication operations in 1004923us (3562.5 ops/sec)
squashed: curve25519: convert field constants to unsigned.
import re, sys, math
def weight(i):
return 2**int(math.ceil(25.5*i))
def convert(t):
limbs = [x for x in t.groups() if x.replace('-','').isdigit()]
v = sum(weight(i)*x for (i,x) in enumerate(map(int, limbs))) % (2**255-19)
limbs = [(v % weight(i+1)) // weight(i) for i in range(10)]
assert v == sum(weight(i)*x for (i,x) in enumerate(limbs))
i = 0
ret = ''
for s in t.groups():
if s.replace('-','').isdigit():
ret += str(limbs[i])
i += 1
else:
ret += s
return ret
fe_re = re.compile(r'(\s*,\s*)'.join(r'(-?\d+)' for i in range(10)))
print (re.sub(fe_re, convert, sys.stdin.read()))
Change-Id: Ibd4f7f5c38e5c4d61c9826afb406baebe2be5168
Reviewed-on: https://boringssl-review.googlesource.com/22385
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This change doesn't actually introduce any Fiat code yet. It sets up the
directory structure to make the diffs in the next change clearer.
Change-Id: I38a21fb36b18a08b0907f9d37b7ef5d7d3137ede
Reviewed-on: https://boringssl-review.googlesource.com/22624
Reviewed-by: David Benjamin <davidben@google.com>
Generating a 2048-bit RSA key with e = 3 (don't do this), the failure
rate at 5*bits iterations appears to be around 7 failures in 1000 tries.
Bump the limit up to 32*bits. This should give a failure rate of around
2 failures in 10^14 tries.
(The FIPS 186-4 algorithm is meant for saner values of e, like 65537. e
= 3 implies a restrictive GCD requirement: the primes must both be 2 mod
3.)
Change-Id: Icd373f61e2eb90df5afaff9a0fc2b2fbb6ec3f0a
Reviewed-on: https://boringssl-review.googlesource.com/22584
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Previously, the ed25519 and SPAKE implementations called field element
operations in ways that did not satisfy the preconditions about ranges
of limbs. Furthermore, replacing signed field arithmetic with unsigned field
arithmetic with similar specifications caused tests to fail. This commit
addresses this in three steps:
(1) Split fe into fe and fe_loose, tracking the bounds
(2) Insert carry operations before uses of fe_add/fe_sub/fe_neg whose
input is already within only the loose bounds
(3) Assert that each field element is within the appropriate bounds at
the beginning and end of every field operation.
Throughput diff:
Ed25519 key generation: -2%
Ed25519 signing: -2%
Ed25519 verify: -2%
X25519: roughly unchanged
Detailed benchmarks on Google Cloud's unidentified Intel Xeon with AVX2:
git checkout $VARIANT && ( cd build && rm -rf * && CC=clang CXX=clang++ cmake -GNinja -DCMAKE_TOOLCHAIN_FILE=../util/32-bit-toolchain.cmake -DCMAKE_BUILD_TYPE=Release .. && ninja && ./tool/bssl speed -filter 25519 )
this branch:
Did 11206 Ed25519 key generation operations in 1029462us (10885.3 ops/sec)
Did 11104 Ed25519 signing operations in 1035735us (10720.9 ops/sec)
Did 3278 Ed25519 verify operations in 1087969us (3013.0 ops/sec)
Did 12000 Curve25519 base-point multiplication operations in 1078962us (11121.8 ops/sec)
Did 3610 Curve25519 arbitrary point multiplication operations in 1002767us (3600.0 ops/sec)
Did 11662 Ed25519 key generation operations in 1077690us (10821.3 ops/sec)
Did 10780 Ed25519 signing operations in 1011474us (10657.7 ops/sec)
Did 3289 Ed25519 verify operations in 1083638us (3035.1 ops/sec)
Did 12000 Curve25519 base-point multiplication operations in 1087477us (11034.7 ops/sec)
Did 3610 Curve25519 arbitrary point multiplication operations in 1017023us (3549.6 ops/sec)
Did 11018 Ed25519 key generation operations in 1011606us (10891.6 ops/sec)
Did 11000 Ed25519 signing operations in 1029961us (10680.0 ops/sec)
Did 3124 Ed25519 verify operations in 1045163us (2989.0 ops/sec)
Did 12000 Curve25519 base-point multiplication operations in 1081770us (11092.9 ops/sec)
Did 3610 Curve25519 arbitrary point multiplication operations in 1014503us (3558.4 ops/sec)
master:
Did 11662 Ed25519 key generation operations in 1059449us (11007.6 ops/sec)
Did 10908 Ed25519 signing operations in 1000081us (10907.1 ops/sec)
Did 3333 Ed25519 verify operations in 1078798us (3089.5 ops/sec)
Did 12000 Curve25519 base-point multiplication operations in 1072831us (11185.4 ops/sec)
Did 3850 Curve25519 arbitrary point multiplication operations in 1075821us (3578.7 ops/sec)
Did 11102 Ed25519 key generation operations in 1017540us (10910.6 ops/sec)
Did 11000 Ed25519 signing operations in 1013279us (10855.8 ops/sec)
Did 3311 Ed25519 verify operations in 1066866us (3103.5 ops/sec)
Did 12000 Curve25519 base-point multiplication operations in 1069668us (11218.4 ops/sec)
Did 3905 Curve25519 arbitrary point multiplication operations in 1095501us (3564.6 ops/sec)
Did 11206 Ed25519 key generation operations in 1014127us (11049.9 ops/sec)
Did 10908 Ed25519 signing operations in 1015821us (10738.1 ops/sec)
Did 3344 Ed25519 verify operations in 1100592us (3038.4 ops/sec)
Did 12000 Curve25519 base-point multiplication operations in 1072847us (11185.2 ops/sec)
Did 3570 Curve25519 arbitrary point multiplication operations in 1009373us (3536.8 ops/sec)
Change-Id: Ia014386daf36c913f3ea44c5f9a420b98670e465
Reviewed-on: https://boringssl-review.googlesource.com/22104
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We end up writing these switch cases everywhere. Let consumers decompose
these a bit. The original thought was folks should write switch-cases so
they handle everything they support, but that's a pain. As long as
algorithm preferences are always configured, we can still add new
dimensions because folks won't be asked to sign algorithms that depend
on dimensions they don't understand.
Change-Id: I3dd7f067f2c55212f0201876546bc70fee032bcf
Reviewed-on: https://boringssl-review.googlesource.com/22524
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Node's default settings spell P-256 as prime256v1. This comes from
OpenSSL additionally allowing the long and short names of each curve's
NID. This works out to one additional name per curve for the ones we
support. To avoid depending on the giant OID table, this replicates the
names in libssl.
Change-Id: I456a2db6939eb6745e5a9d2f12cf6886e6265b9f
Reviewed-on: https://boringssl-review.googlesource.com/22545
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Credit to OSS-Fuzz for finding this.
CVE-2017-3736
(Imported from upstream's 668a709a8d7ea374ee72ad2d43ac72ec60a80eee and
420b88cec8c6f7c67fad07bf508dcccab094f134.)
This bug does not affect BoringSSL as we do not enable the ADX code.
Note the test vector had to be tweaked to take things in and out of
Montgomery form. (There may be something to be said for test vectors for
just BN_mod_mul_montgomery, though we'd need separate 64-bit and 32-bit
ones because R can be different.)
Change-Id: I832070731ac1c5f893f9c1746892fc4a32f023f5
Reviewed-on: https://boringssl-review.googlesource.com/22484
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Due to a copy-paste error, the call to |left_shift_3| is missing after
reducing the password scalar in SPAKE2. This means that three bits of
the password leak in Alice's message. (Two in Bob's message as the point
N happens to have order 4l, not 8l.)
The “correct” fix is to put in the missing call to |left_shift_3|, but
that would be a breaking change. In order to fix this in a unilateral
way, we add points of small order to the masking point to bring it into
prime-order subgroup.
BUG=chromium:778101
Change-Id: I440931a3df7f009b324d2a3e3af2d893a101804f
Reviewed-on: https://boringssl-review.googlesource.com/22445
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
An embedded item wasn't allocated separately on the heap, so don't
free it as if it was.
Issue discovered by Pavel Kopyl
(Imported from upstream's cdc3307d4257f4fcebbab3b2b44207e1a399da05 and
65d414434aeecd5aa86a46adbfbcb59b4344503a.)
I do not believe this is actually reachable in BoringSSL, even in the
face of malloc errors. The only field which sets ASN1_TFLG_COMBINE is in
X509_ATTRIBUTE. That field's value is X509_ATTRIBUTE_SET which cannot
fail to initialize. (It is a CHOICE whose initialization consists of
setting the selector to -1 and calling the type's callback which is
unset for this type.)
Change-Id: I29c080f8a4ddc2f3ef9c119d0d90a899d3cb78c5
Reviewed-on: https://boringssl-review.googlesource.com/22365
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Currently we only check that the underlying EC_METHODs match, which
avoids the points being in different forms, but not that the points are
on the same curves. (We fixed the APIs early on so off-curve EC_POINTs
cannot be created.)
In particular, this comes up with folks implementating Java's crypto
APIs with ECDH_compute_key. These APIs are both unfortunate and should
not be mimicked, as they allow folks to mismatch the groups on the two
multiple EC_POINTs. Instead, ECDH APIs should take the public value as a
byte string.
Thanks also to Java's poor crypto APIs, we must support custom curves,
which makes this particularly gnarly. This CL makes EC_GROUP_cmp work
with custom curves and adds an additional subtle requirement to
EC_GROUP_set_generator.
Annoyingly, this change is additionally subtle because we now have a
reference cycle to hack around.
Change-Id: I2efbc4bd5cb65fee5f66527bd6ccad6b9d5120b9
Reviewed-on: https://boringssl-review.googlesource.com/22245
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Per the comment, it's no longer necessary. macOS i386 does not exist,
but apparently iOS i386 does! We can probably just remove it altogether,
but our cmake_minimum_required is nominally 2.8, so I just put the
version check in.
Bug: 210
Change-Id: I6e0617a3f292a218b2465eee85bd4814bd0e55c7
Reviewed-on: https://boringssl-review.googlesource.com/22304
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
I really need to resurrect the CL to make them entirely static
(https://crbug.com/boringssl/20), but, in the meantime, to make
replacing the EC_METHOD pointer in EC_POINT with EC_GROUP not
*completely* insane, make them refcounted.
OpenSSL did not do this because their EC_GROUPs are mutable
(EC_GROUP_set_asn1_flag and EC_GROUP_set_point_conversion_form). Ours
are immutable but for the two-function dance around custom curves (more
of OpenSSL's habit of making their objects too complex), which is good
enough to refcount.
Change-Id: I3650993737a97da0ddcf0e5fb7a15876e724cadc
Reviewed-on: https://boringssl-review.googlesource.com/22244
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This reverts commit 75d43b5785. Chatting
with EKR, there is some reason to believe that doing this might cause
more middlebox issues. Since we're still in the middle of working
towards viable deployment in the first place, revert this.
We can experiment with this later. I should have arranged for this to be
controlled more carefully anyway.
Change-Id: I0c8bf578f9d7364e913894e1bf3c2b8123dfd770
Reviewed-on: https://boringssl-review.googlesource.com/22204
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is an OpenSSL thing to support platforms where BN_ULONG is not
actually the size it claims to be. We define BN_ULONG to uint32_t and
uint64_t which are guaranteed by C to implement arithemetic modulo 2^32
and 2^64, respectively. Thus there is no need for any of this.
Change-Id: I098cd4cc050a136b9f2c091dfbc28dd83e01f531
Reviewed-on: https://boringssl-review.googlesource.com/21784
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This does not affect TLS 1.2 (beyond Channel ID or NPN) but, in TLS 1.3,
we send several encrypted handshake messages in a row. For the server,
this means 66 wasted bytes in TLS 1.3. Since OpenSSL has otherwise used
one record per message since the beginning and unencrypted overhead is
less interesting, leave that behavior as-is for the time being. (This
isn't the most pressing use of the breakage budget.) But TLS 1.3 is new,
so get this tight from the start.
Change-Id: I64dbd590a62469d296e1f10673c14bcd0c62919a
Reviewed-on: https://boringssl-review.googlesource.com/22068
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
We enforce that servers don't send bogus ALPN values, so consumers may
assume that SSL_get0_alpn_selected won't have anything terribly weird.
To maintain that invariant in the face of folks whose ALPN preferences
change (consider a persisted session cache), we should decline to offer
0-RTT if early_alpn would have been rejected by the check anyway.
Change-Id: Ic3a9ba4041d5d4618742eb05e27033525d96ade1
Reviewed-on: https://boringssl-review.googlesource.com/22067
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Our assembly does not use the GOT to reference symbols, which means
references to visible symbols will often require a TEXTREL. This is
undesirable, so all assembly-referenced symbols should be hidden. CPU
capabilities are the only such symbols defined in C.
These symbols may be hidden by doing at least one of:
1. Build with -fvisibility=hidden
2. __attribute__((visibility("hidden"))) in C.
3. .extern + .hidden in some assembly file referencing the symbol.
We have lots of consumers and can't always rely on (1) happening. We
were doing (3) by way of d216b71f90 and
16e38b2b8f, but missed 32-bit x86 because
it doesn't cause a linker error.
Those two patches are not in upstream. Upstream instead does (3) by way
of x86cpuid.pl and friends, but we have none of these files.
Standardize on doing (2). This avoids accidentally getting TEXTRELs on
some 32-bit x86 build configurations. This also undoes
d216b71f90 and
16e38b2b8f. They are no now longer needed
and reduce the upstream diff.
Change-Id: Ib51c43fce6a7d8292533635e5d85d3c197a93644
Reviewed-on: https://boringssl-review.googlesource.com/22064
Commit-Queue: Matt Braithwaite <mab@google.com>
Reviewed-by: Matt Braithwaite <mab@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
If you specify any --target which refers to a x86_64-apple-darwin
triple, or a more specific variant derived from it, specifying
-stdlib=libc++ is required, otherwise clang falls back to libstdc++
which didn't include c++11, and fails to compile in very obscure ways
(simply failing to find any c++11 symbols).
Change-Id: I58025cea91eaa0c16d9b5831f9965889b75bbc31
Reviewed-on: https://boringssl-review.googlesource.com/21984
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
While a fairly small hook, open_close_notify is pretty weird. It
processes things at the record level and not above. Notably, this will
break if it skips past a TLS 1.3 KeyUpdate.
Instead, it can share the core part of SSL_read/SSL_peek, with slight
tweaks to post-handshake processing. Note this does require some tweaks
to that code. Notably, to retain the current semantics that SSL_shutdown
does not call funny callbacks, we suppress tickets.
Change-Id: Ia0cbd0b9f4527f1b091dd2083a5d8c7efb2bac65
Reviewed-on: https://boringssl-review.googlesource.com/21885
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Ideally we'd put this deep in the record layer, but sending alerts
currently awkwardly sets the field early, so we can't quite lock it out
this deep down.
This is mostly a sanity-check, but a later CL will fix SSL_shutdown's
post-handshake message processing, so this will help catch errors there.
Change-Id: I78e627c19547dbcdc85fb168795240d692baf031
Reviewed-on: https://boringssl-review.googlesource.com/21884
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
With this change, it should now always be the case that rr->length is
zero on entry to ssl3_read_message. This will let us detach everything
but application data from rr. This pushes some init_buf invariants down
into tls_open_record so we don't need to maintain them everywhere.
Change-Id: I206747434e0a9603eea7d19664734fd16fa2de8e
Reviewed-on: https://boringssl-review.googlesource.com/21524
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Enough were to make record processing idempotent (we either consume a
record or we don't), but some errors would cause us to keep processing
records when we should get stuck.
This leaves errors in the layer between the record bits and the
handshake. I'm hoping that will be easier to resolve once they do not
depend on BIO, at which point the checks added in this CL may move
around.
Bug: 206
Change-Id: I6b177079388820335e25947c5bd736451780ab8f
Reviewed-on: https://boringssl-review.googlesource.com/21366
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Ultimately the ssl_buffer_* code will be above SSL_PROTOCOL_METHOD, so
having the processing be analogous is simpler. This also means that DTLS
can surface errors out of dtls_open_record without the caller reading an
extra record.
Bug: 206
Change-Id: Ic1cb3a884763c8e875e1129b1cda226f72bc95b7
Reviewed-on: https://boringssl-review.googlesource.com/21364
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
This removes the last place where non-app-data hooks leave anything
uncomsumed in rrec. (There is still a place where non-app-data hooks see
a non-empty rrec an entrance. read_app_data calls into read_handshake.
That'll be fixed in a later patch in this series.)
This should not change behavior, though some error codes may change due
to some processing happening in a slightly different order.
Since we do this in a few places, this adds a BUF_MEM_append with tests.
Change-Id: I9fe1fc0103e47f90e3c9f4acfe638927aecdeff6
Reviewed-on: https://boringssl-review.googlesource.com/21345
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
RSABadValueTooLong should have the true one as a suffix, not a prefix,
so that the version check still works. Also do the padding manually to
catch a few other bad padding cases. This is sufficient coverage so that
disabling any one comparison in the padding check flags some failure.
Change-Id: Ibcad284e5ecee3e995f43101c09e4cf7694391e9
Reviewed-on: https://boringssl-review.googlesource.com/21904
Reviewed-by: Steven Valdez <svaldez@google.com>
Application records may be packed with other application data records or
with handshake records. We also were never testing CCS and handshake
being packed together. Implement this by moving the packing logic to the
bottom of BoGo's DTLS record layer.
Change-Id: Iabc14ec4ce7b99ed1f923ce9164077efe948c7a0
Reviewed-on: https://boringssl-review.googlesource.com/21844
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
d3868ac4d6f92e57376924a62e8d52f36d7a326561ec0bbd1d5681759a947134 sde-external-8.9.0-2017-08-06-lin.tar.bz2
9852d57e0d6b4509accb4f9faf862327a79c18b630aac4f35c65ce7a270a9230 strawberry-perl-5.26.1.1-32bit-portable.zip
I've left CMake alone for now because that involves building things and
there's some mess between newer CMakes trying to mess with the Android
NDK, so that needs to be tested out a bit.
Change-Id: I6beea9d4774474ac62e843c8668fcef7f6a6fc04
Reviewed-on: https://boringssl-review.googlesource.com/21824
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
These are common between TLS and DTLS so should not have the ssl3_
prefix. (TLS-only stuff should really have a tls_ prefix, but we still
have a lot of that one.)
This also fixes a stray reference to ssl3_send_client_key_exchange..
Change-Id: Ia05b360aa090ab3b5f075d5f80f133cbfe0520d4
Reviewed-on: https://boringssl-review.googlesource.com/21346
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
We still have more links to cut for ssl.h to not pull in x509.h (notably
pem.h), but this resolves some easy ones. I've kept the constants the
same just in case, but nowhere are the constants mixed up by callers or
passed from one to the other in the functions' implementations. They're
completely independent.
Change-Id: Ic0896283378b5846afd6422bfe740951ac552f0e
Reviewed-on: https://boringssl-review.googlesource.com/21704
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit 9a4876e193 broke NGINX builds with
BoringSSL due to this missing include (OpenSSL builds work fine):
src/event/ngx_event_openssl.c: In function ‘ngx_ssl_session_ticket_key_callback’:
src/event/ngx_event_openssl.c:3065:13: error: implicit declaration of function ‘HMAC_Init_ex’; did you mean ‘SHA1_Init’? [-Werror=implicit-function-declaration]
if (HMAC_Init_ex(hctx, key[0].hmac_key, size, digest, NULL) != 1) {
^~~~~~~~~~~~
Change-Id: Ie7170f05034d5fd8c85d1948b4ab9c9bb8447d13
Reviewed-on: https://boringssl-review.googlesource.com/21664
Reviewed-by: Adam Langley <agl@google.com>
GCC 7.2.0 (in Release builds) can't figure out that |type| is always
set:
../ssl/tls_record.cc: In function ‘bssl::OpenRecordResult bssl::OpenRecord(SSL*, bssl::Span<unsigned char>*, size_t*, uint8_t*, bssl::Span<unsigned char>)’:
../ssl/tls_record.cc:595:44: error: ‘type’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
if (type != SSL3_RT_APPLICATION_DATA && type != SSL3_RT_ALERT) {
cc1plus: all warnings being treated as errors
Change-Id: I1ca9683a18d89097288018f48b50991bce185da8
Reviewed-on: https://boringssl-review.googlesource.com/21724
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The only difference is whether there's an alert to send back, but we'll
need to allow an "error without alert" in several cases anyway:
1. If the server sees an HTTP request or garbage instead of a
ClientHello, it shouldn't send an alert.
2. Resurfaced errors.
Just make zero signal no alert for now. Later on, I'm thinking we might
just want to put the alert into the outgoing buffer and make it further
uniform.
This also gives us only one error state to keep track of rather than
two.
Bug: 206
Change-Id: Ia821d9f89abd2ca6010e8851220d4e070bc42fa1
Reviewed-on: https://boringssl-review.googlesource.com/21286
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
This is analogous to the Go stack's handshakeErr field. Since it's quite
common for callers to run two I/O operations in parallel[*] like
SSL_read and SSL_write (or SSL_read and SSL_do_handshake for client
0-RTT). Accordingly, the new handshake state machine jams itself up on
handshake error, but to fully work with such callers, we should also
replay the error state.
This doesn't yet catch all cases (there are some parts of the read flow
which need to be fixed). Those will be resolved in later changes.
[*] Not actually in parallel, of course, but logically in parallel on a
non-blocking socket.
Bug: 206
Change-Id: I5a4d37a258b9e3fc555b732938b0528b839650f8
Reviewed-on: https://boringssl-review.googlesource.com/21285
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
This will be useful for the SSL stack to properly resurface handshake
failures. Leave this in a private header and, along the way, hide the
various types.
(ERR_NUM_ERRORS didn't change in meaning. The old documentation was
wrong.)
Bug: 206
Change-Id: I4c6ca98d162d11ad5e17e4baf439a18fbe371018
Reviewed-on: https://boringssl-review.googlesource.com/21284
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
crypto/bio/bio_test.cc - I'm not sure where this was added for, but none
of the functions used there appear to have feature macros documented.
crypto/bio/printf.c - -std=c99 provides (v)snprintf.
crypto/lhash/lhash_test.cc - we no longer call rand_r.
crypto/mem.c - we no longer call strdup and -std=c99 provides (v)snprintf.
Apple messed up their headers and, if _POSIX_C_SOURCE is defined but
_DARWIN_C_SOURCE isn't, pthread.h no longer defines mach_port_t. They
then shipped a version of libc++ headers that is missing this fix, so
the build breaks:
https://github.com/llvm-mirror/libcxx/commit/bcc92d75df0274b9593ebd097fcae60494e3bffc
If one uses XCode, they've hacked their pthread.h to provide mach_port_t
if defined(__cplusplus), but the standalone tools appear to be old and
missing this.
We can work around this by also defining _DARWIN_C_SOURCE in C++ files
that need _POSIX_C_SOURCE, but it appears none of these files actually
need it.
Change-Id: I5df9453730696100eb22b809febeb65053701322
Reviewed-on: https://boringssl-review.googlesource.com/20964
Reviewed-by: Adam Langley <agl@google.com>
The exponent is secret, so we should be using the consttime variant. See
also upstream's f9cbf470180841966338db1f4c28d99ec4debec4.
Change-Id: I233d4223ded5b80711d7c8f906e3579c36b24cd0
Reviewed-on: https://boringssl-review.googlesource.com/20924
Reviewed-by: Adam Langley <agl@google.com>
Although we are derived from 1.0.2, we mimic 1.1.0 in some ways around
our FOO_up_ref functions and opaque libssl types. This causes some
difficulties when porting third-party code as any OPENSSL_VERSION_NUMBER
checks for 1.1.0 APIs we have will be wrong.
Moreover, adding accessors without changing OPENSSL_VERSION_NUMBER can
break external projects. It is common to implement a compatibility
version of an accessor under #ifdef as a static function. This then
conflicts with our headers if we, unlike OpenSSL 1.0.2, have this
function.
This change switches OPENSSL_VERSION_NUMBER to 1.1.0 and atomically adds
enough accessors for software with 1.1.0 support already. The hope is
this will unblock hiding SSL_CTX and SSL_SESSION, which will be
especially useful with C++-ficiation. The cost is we will hit some
growing pains as more 1.1.0 consumers enter the ecosystem and we
converge on the right set of APIs to import from upstream.
It does not remove any 1.0.2 APIs, so we will not require that all
projects support 1.1.0. The exception is APIs which changed in 1.1.0 but
did not change the function signature. Those are breaking changes.
Specifically:
- SSL_CTX_sess_set_get_cb is now const-correct.
- X509_get0_signature is now const-correct.
For C++ consumers only, this change temporarily includes an overload
hack for SSL_CTX_sess_set_get_cb that keeps the old callback working.
This is a workaround for Node not yet supporting OpenSSL 1.1.0.
The version number is set at (the as yet unreleased) 1.1.0g to denote
that this change includes https://github.com/openssl/openssl/pull/4384.
Bug: 91
Change-Id: I5eeb27448a6db4c25c244afac37f9604d9608a76
Reviewed-on: https://boringssl-review.googlesource.com/10340
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
SSL_CTX_sessions is the only think making us expose LHASH as public API
and nothing uses it. Nothing can use it anyway as it's not thread-safe.
I haven't actually removed it yet since SSL_CTX is public, but once the
types are opaque, we could trim the number of symbols ssl.h pulls in
with some work.
Relatedly, fix thread safety of SSL_CTX_sess_number.
Change-Id: I75a6c93509d462cd5ed3ce76c587f0d1e7cd0797
Reviewed-on: https://boringssl-review.googlesource.com/20804
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
An Array<T> is an owning Span<T>. It's similar to absl::FixedArray<T>
but plays well with OPENSSL_malloc and doesn't implement inlining. With
OPENSSL_cleanse folded into OPENSSL_free, we could go nuts with
UniquePtr<uint8_t>, but having the pointer and length tied together is
nice for other reasons. Notably, Array<T> plays great with Span<T>.
Also switch the other parameter to a Span.
Bug: 132
Change-Id: I4cdcf810cf2838208c8ba9fcc6215c1e369dffb8
Reviewed-on: https://boringssl-review.googlesource.com/20667
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
MSVC 2015's SFINAE implementation is broken. In particular, it seems not
to bother expanding EnableIfContainer unless we force it to by writing
::type. That means we need to use std::enable_if rather than
enable_if_t, even though it's quite wordy.
Change-Id: Ic643ab8a956991bb14af07832be80988f7735428
Reviewed-on: https://boringssl-review.googlesource.com/20764
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
First, I spelled the wildcard name constraint in many_constraints.pem
wrong. It's .test, not *.test for name constraints. (This doesn't matter
for some_names*.pem, but it does to avoid a false negative in
many_names3.pem.)
Second, the CN of certs should be a host, not "Leaf". OpenSSL 1.1.0
checks "host-like" CNs against name constraints too and "Leaf" is
host-like.
I've also made the generator deterministic and checked it in, as PEM
blobs are not reviewable.
Change-Id: I195d9846315168a792cca829aff25c986339b8f5
Reviewed-on: https://boringssl-review.googlesource.com/20584
Reviewed-by: David Benjamin <davidben@google.com>
Fixes failed compile with [-Werror=implicit-fallthrough=], which is
default on gcc-7.x on distributions like fedora.
Enabling no implicit fallthrough for more than just clang as well to
catch this going forward.
Change-Id: I6cd880dac70ec126bd7812e2d9e5ff804d32cadd
Signed-off-by: Vincent Batts <vbatts@redhat.com>
Reviewed-on: https://boringssl-review.googlesource.com/20564
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Thanks to Lennart Beringer for pointing that that malloc failures could
lead to invalid EVP_MD_CTX states. This change cleans up the code in
general so that fallible operations are all performed before mutating
objects. Thus failures should leave objects in a valid state.
Also, |ctx_size| is never zero and a hash with no context is not
sensible, so stop handling that case and simply assert that it doesn't
occur.
Change-Id: Ia60c3796dcf2f772f55e12e49431af6475f64d52
Reviewed-on: https://boringssl-review.googlesource.com/20544
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Add a simple dumb webserver that responds with the session status for
any GET request. This option is intended to be used with -loop to
generate automated responses to requests and serves two purposes: (1)
test that application data from clients can be decrypted, (2) test that
clients can decrypt data from the server and (3) early data indicator.
Change-Id: I2b8374ca7b8db4c8effab42e86b5e3139d9466e1
Reviewed-on: https://boringssl-review.googlesource.com/20305
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
I'll fully remove this once Chrome 62 hits stable, in case any bug
reports come in for Chrome 61. Meanwhile switch the default to off so
that other consumers pick up the behavior. (Should have done this sooner
and forgot.)
Bug: chromium:735616
Change-Id: Ib27c4072f228cd3b5cce283accd22732eeef46b2
Reviewed-on: https://boringssl-review.googlesource.com/20484
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Newer versions of LLVM can emit this instruction. Note that there are
two different Intel instructions, both called “movsd”. The old one is an
auto-incrementing move that doesn't take any arguments. That's not the
one that is targetted in this change.
Change-Id: Id0c96e0c7fe0f6e4feb8a72b5bc0fa40878225b9
Reviewed-on: https://boringssl-review.googlesource.com/20425
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
crypto/asn1 routinely switches between int and long without overflow
checks. Fortunately, it funnels everything into a common entrypoint, so
we can uniformly bound all inputs to something which comfortably fits in
an int.
Change-Id: I340674c6b07820309dc5891024498878c82e225b
Reviewed-on: https://boringssl-review.googlesource.com/20366
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
base.h pulls in all the forward declarations, so this isn't needed. We
should also remove bio.h and buf.h, but cURL seems to depend on those.
Code search suggests this one is okay though.
case:yes content:\bHMAC content:openssl/ssl.h -content:openssl/hmac.h
Change-Id: Id91686bd134649245855025940bc17f82823c734
Reviewed-on: https://boringssl-review.googlesource.com/20364
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Windows provides _aligned_malloc, so we could provide an
|OPENSSL_aligned_malloc| in the future. However, since we're still
trying to get the zeroisation change landed everywhere, a self-contained
change seems easier until that has settled down.
Change-Id: I47bbd811a7fa1758f3c0a8a766a1058523949b7f
Reviewed-on: https://boringssl-review.googlesource.com/20204
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The Java client implementation of the 3SHAKE mitigation incorrectly
rejects initial handshakes when all of the following are true:
1. The ClientHello offered a session.
2. The session was successfully resumed previously.
3. The server declines the session.
4. The server sends a certificate with a different SAN list than in the
previous session.
(Note the 3SHAKE mitigation is to reject certificates changes on
renegotiation, while Java's logic applies to initial handshakes as
well.)
The end result is long-lived Java clients break on some certificate
rotations. Fingerprint Java clients and decline all offered sessions.
This avoids (2) while still introducing new sessions to clear any
existing problematic sessions.
See also b/65323005.
Change-Id: Ib2b84c69b5ecba285ffb8c4d03de5626838d794e
Reviewed-on: https://boringssl-review.googlesource.com/20184
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This guards against the name constraints check consuming large amounts
of CPU time when certificates in the presented chain contain an
excessive number of names (specifically subject email names or subject
alternative DNS names) and/or name constraints.
Name constraints checking compares the names presented in a certificate
against the name constraints included in a certificate higher up in the
chain using two nested for loops.
Move the name constraints check so that it happens after signature
verification so peers cannot exploit this using a chain with invalid
signatures. Also impose a hard limit on the number of name constraints
check loop iterations to further mitigate the issue.
Thanks to NCC for finding this issue.
Change-Id: I112ba76fe75d1579c45291042e448850b830cbb7
Reviewed-on: https://boringssl-review.googlesource.com/19164
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
c2i_ASN1_BIT_STRING takes length as a long but uses it as an int. Check bounds
before doing so. Previously, excessively large inputs to the function could
write a single byte outside the target buffer. (This is unreachable as
asn1_ex_c2i already uses int for the length.)
Thanks to NCC for finding this issue.
Change-Id: I7ae42214ca620d4159fa01c942153717a7647c65
Reviewed-on: https://boringssl-review.googlesource.com/19204
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Right now we report the per-connection value during the handshake and
the per-session value after the handshake. This also trims our tickets
slightly by removing a largely unused field from SSL_SESSION.
Putting it on SSL_HANDSHAKE would be better, but sadly a number of
bindings-type APIs expose it after the handshake.
Change-Id: I6a1383f95da9b1b141b9d6adadc05ee1e458a326
Reviewed-on: https://boringssl-review.googlesource.com/20064
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We do not expose EVP_PKEY_CTX_ctrl, so we can freely change the
semantics of EVP_PKEY_CTRL_RSA_OAEP_LABEL. That means we can pass in an
actual size_t rather than an int.
Not that anyone is actually going to exceed an INT_MAX-length RSA-OAEP
label.
Change-Id: Ifc4eb296ff9088c8815f4f8cd88100a407e4d969
Reviewed-on: https://boringssl-review.googlesource.com/19984
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
BoringSSL has more flexibility in changing things than many other library projects because we have a reasonable idea of who our users are. Still, breaking changes require some care. We depend on tight feedback loops with our consumers so that we can learn about mistakes and fix them. For that to work, updating BoringSSL must be smooth.
Ultimately, the strategy for each breaking change is decided on a case-by-case basis. This document provides guidelines and techniques to help with a smooth transition.
## Breakage risk
Traditionally, breaking changes are defined in terms of API or ABI surface. Exposed symbols and type signatures cannot change, etc. But this is a poor approximation of the true impact. Removing an API may not a breaking change if no one is using it. Conversely, [Hyrum's Law](http://www.hyrumslaw.com/) applies. Fixing a bug may be a breaking change for some consumer which was depending on that bug.
Thus, we do not think about whether a change is formally a breaking change, but about the *risk* of it breaking someone.
Some changes, such as internal cleanups or bug-fixes, are low risk and do not need special measures. Any problems can be handled when the affected consumer updates BoringSSL and notices.
Other changes, such as removing an API, forbidding some edge case, or adjusting some behavior, are more likely to break things. To help the consumer triage any resulting failures, include some text in the commit message, prefixed by `Update-Note: `. This can include what this change may break and instructions on how to fix the issue.
## Code Search
The vast majority of BoringSSL consumers are conveniently indexed in various Code Search instances. This can predict the impact of a risky change and identify code to fix ahead of time. The document “How to Code Search” in the (Google-only) [go/boringssl-folder](https://goto.google.com/boringssl-folder) includes notes on this.
## Evaluate a change's cost
If some change has high cost (from having to fix consumers) and relatively little benefit to BoringSSL, it may not be worth the trouble. For instance, it is likely not worth removing a small compatibility function in the corner of the library that is easily dropped by the static linker.
Conversely, a change that leads to a major improvement to all BoringSSL consumers, at the cost of fixing one or two consumers, is typically worth it.
## Fixing consumers
If code search reveals call sites that are definitely going to break, prefer to handle these before making the change. While unexpected breakage is always possible, we generally consider it the responsibility of the developer or group making a change to handle impact of that change. Teams are generally unhappy to be surprised by new migration work but happy to have migration work done for them.
In most cases, this is straightforward:
1. Add the replacement API.
2. As the replacement API enters each consuming repository, migrate callers to it.
3. Remove the original API once all consumers have been migrated.
The removal should still include an `Update-Note` tag, in case some were missed.
In some cases, this kind of staged approach is not feasible: perhaps the same code cannot simultaneously work before and after the change, or perhaps there are too many different versions in play. For instance, [Conscrypt](https://github.com/google/conscrypt) feeds into three different repositories. The GitHub repository consumes BoringSSL's `master` branch directly. It is pushed into Android, where it consumes Android's `external/boringssl`. Yet another copy is pushed into the internal repository, where it consumes that copy of BoringSSL. As each of these Conscrypts are updated independently from their corresponding BoringSSLs, Conscrypt upstream cannot rely on a new BoringSSL API until it is present in all copies of BoringSSL its downstreams rely on.
In that case, a multi-sided change may be more appropriate:
1. Upload the breaking change to Gerrit, but do not submit it yet. Increment the `BORINGSSL_API_VERSION` symbol.
2. Update the consuming repository with `#if BORINGSSL_API_VERSION < N` preprocessor logic. Leave a comment to remove this later, linking to your BoringSSL change.
3. When the `BORINGSSL_API_VERSION` check has propagated to relevant copies of the consuming repository, submit the BoringSSL change.
4. When the BoringSSL change has propagated to relevant copies of BoringSSL, remove the staging logic from the consumer.
Finally, in some cases, the consumer's change may be committed atomically with the BoringSSL update. This can only be done for code which only consumes one instance of BoringSSL (so the Conscrypt example above is not eligible). Check with that project's maintainer first or, better, be that project's maintainer.
If more complex changes are needed in some consumer, communicate with the relevant maintainers to plan the transition.
## Fail early, fail closed
When breaking changes do occur, they should fail as early and as detectably as possible.
Ideally, problematic consumers fail to compile. Prefer to remove functions completely over leaving an always failing stub function. Sometimes this is not possible due to other consumers, particularly bindings libraries. Alternatively, if a stub function can be reasonably justified as still satisfying the API constraints, consider adding one to improve compatibility. For example, BoringSSL has many no-op stubs corresponding to OpenSSL's many initialization functions.
If some parameter now must be `NULL`, change the type to an opaque struct pointer. Consumers passing non-`NULL` pointers will then fail to compile.
If breaking the compile is not feasible, break at runtime, in the hope that consumers have some amount of test coverage. When doing so, try to fail on the common case. In particular, do not rely on consumers adequately testing or even checking for failure cases. One strategy is to bring the object into a “poison” state: if an illegal operation occurs, set a flag to fail all subsequent ones.
In other functions, it may be appropriate to simply call `abort()`.
## Unexpected breakage
While we try to avoid breaking things, sometimes things unexpectedly break. Depending on the impact, we may fix the consumer, make a small fix to BoringSSL, or revert the change to either try again later or revise the approach.
If we do not ultimately fix the consumer, add a test in BoringSSL to capture the unexpected API contract, so future regressions are caught quickly.
## Canary changes and bake time
When planning a large project that depends on a breaking change, prefer to make the breaking change first—before committing larger changes. Or, when changing toolchain or language requirements, add a small instance of the dependency somewhere first then wait a couple of weeks for the change to appear in consumers. This ensures that reverting the change is still feasible if necessary.
While we rely on a tight feedback loop with our consumers, there are a few consumers which update less frequently. For extremely risky changes, such as introducing C++ to a target, it may be prudent to wait much longer.
## Third-party code
In many cases, we are interested in changing behavior which came from OpenSSL. OpenSSL's API surface is huge, but only a small subset is actually used. So we can and occasionally do change these behaviors. This is more complex than changing BoringSSL-only behavior due to third-party code.
We use BoringSSL with many third-party projects that normally use OpenSSL. Generally, we consider this our burden to make this work and do not encourage external projects to depend on BoringSSL. While we can and do maintain patches for this as necessary, it has overhead and so the cost of breaking third-party code is higher.
We lean fairly strongly towards making changes to BoringSSL over patching third-party code, unless the third-party change fixes a security problem.
Additionally, changing an OpenSSL API will not only affect third-party code we use today, but also any third-party code we use in the future. Thus Code Search is less useful as an absolute predictor, and the various other considerations in this document are more important.
If the patch to support a BoringSSL change can be generally useful to the third-party project, send it upstream. For instance, it may use the APIs better, clean up code, or help support newer versions of OpenSSL. In general, we try to target compatibility with “most” “well-behaved” OpenSSL consumers.
Finally, if some particular OpenSSL API or pattern is problematic to BoringSSL, it is likely problematic to OpenSSL too. Consider filing a bug with them to suggest a change, either in new code going forward or for the next API break. OpenSSL's release cycles and feedback loops are much longer than BoringSSL's, so this is usually not immediately useful, but it keeps the ecosystem moving in the right direction.
@@ -31,3 +31,4 @@ There are other files in this directory which might be helpful:
* include/openssl: public headers with API documentation in comments. Also [available online](https://commondatastorage.googleapis.com/chromium-boringssl-docs/headers.html).
* [FUZZING.md](/FUZZING.md): information about fuzzing BoringSSL.
* [CONTRIBUTING.md](/CONTRIBUTING.md): how to contribute to BoringSSL.
* [BREAKING-CHANGES.md](/BREAKING-CHANGES.md): notes on potentially-breaking changes.
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Adam Langley
David Benjamin
Steven Valdez
Joshua Liebow-Feeser
Adam Langley
Tom Bridgwater
Jesse Selover
Matthew Braithwaite
Michał Janiszewski
Ryan Tseng
Daniel Hirche
William D. Irons
Guillaume Egles
Adam Barth
Alessandro Ghedini
Loo Rong Jie
Adam Vartanian
Brian Smith
Martin Kreichgauer
James Robinson
Andrii Shyshkalov
sphawk
Fred Gylys-Colwell
Aaron Green
Nick Harper
Shenghua Zhang
Frederik Kriewitz
Gabriel Redner
Steven Valdez
Marek Gilbert
Andres Erbsen
Michał Janiszewski
Daniel Wagner-Hall
Param Reddy
Dan Willemsen
Bruce Dawson
Robert Sloan
Vincent Batts
Peter Wu