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>
Change-Id: Ic859f19edff281334bd6975dd3c3b2931c901021
Reviewed-on: https://boringssl-review.googlesource.com/23044
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've left EVP_set_buggy_rsa_parser as a no-op stub for now, but it
shouldn't need to last very long. (Just waiting for a CL to land in a
consumer.)
Bug: chromium:735616
Change-Id: I6426588f84dd0803661a79c6636a0414f4e98855
Reviewed-on: https://boringssl-review.googlesource.com/22124
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>
ssl is all that's left. Will do that once that's at a quiet point.
Change-Id: Ia183aed5671e3b2de333def138d7f2c9296fb517
Reviewed-on: https://boringssl-review.googlesource.com/19564
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>
The EC_POINT munging is sufficiently heavy on the goto err that I went
ahead and tidied it up.
Bug: 132
Change-Id: I7a3b3b3f166e39e4559acec834dd8e1ea9ac8620
Reviewed-on: https://boringssl-review.googlesource.com/17747
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>
We only ever compute it for odd (actually, prime) modulus as part of
BN_mod_sqrt.
If we cared, we could probably drop this from most binaries. This is
used to when modular square root needs Tonelli-Shanks. Modular square
root is only used for compressed coordinates. Of our supported curves
(I'm handwaiving away EC_GROUP_new_curve_GFp here[*]), only P-224 needs
the full Tonelli-Shanks algorithm (p is 1 mod 8). That computes the
Legendre symbol a bunch to find a non-square mod p. But p is known at
compile-time, so we can just hard-code a sample non-square.
Sadly, BN_mod_sqrt has some callers outside of crypto/ec, so there's
also that. Anyway, it's also not that large of a function.
[*] Glancing through SEC 2 and Brainpool, secp224r1 is the only curve
listed in either document whose prime is not either 3 mod 4 or 5 mod 8.
Even 5 mod 8 is rare: only secp224k1. It's unlikely anyone would notice
if we broke annoying primes. Though OpenSSL does support "WTLS" curves
which has an additional 1 mod 8 case.
Change-Id: If36aa78c0d41253ec024f2d90692949515356cd1
Reviewed-on: https://boringssl-review.googlesource.com/15425
Reviewed-by: Adam Langley <agl@google.com>
Thanks to Rob Sloan for clearing out Android's uses of these functions.
I forgot we can hide these now.
BUG=97
Change-Id: I9bc7bf5ca379d3345743151e606f3e911367b4ed
Reviewed-on: https://boringssl-review.googlesource.com/15364
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Robert Sloan <varomodt@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
These are meant to make Android libcore's usage of BIGNUMs for java
BigIntegers faster and nicer (specifically, so that it doesn't need
to malloc a bunch of temporary BIGNUMs).
BUG=97
Change-Id: I5f30e14c6d8c66a9848d4935ce27d030829f6923
Reviewed-on: https://boringssl-review.googlesource.com/13387
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>
BN_FLG_CONSTTIME is a ridiculous API and easy to mess up
(CVE-2016-2178). Instead, code that needs a particular algorithm which
preserves secrecy of some arguemnt should call into that algorithm
directly.
This is never set outside the library and is finally unused within the
library! Credit for all this goes almost entirely to Brian Smith. I just
took care of the last bits.
Note there was one BN_FLG_CONSTTIME check that was still reachable, the
BN_mod_inverse in RSA key generation. However, it used the same code in
both cases for even moduli and φ(n) is even if n is not a power of two.
Traditionally, RSA keys are not powers of two, even though it would make
the modular reductions a lot easier.
When reviewing, check that I didn't remove a BN_FLG_CONSTTIME that led
to a BN_mod_exp(_mont) or BN_mod_inverse call (with the exception of the
RSA one mentioned above). They should all go to functions for the
algorithms themselves like BN_mod_exp_mont_consttime.
This CL shows the checks are a no-op for all our tests:
https://boringssl-review.googlesource.com/c/12927/
BUG=125
Change-Id: I19cbb375cc75aac202bd76b51ca098841d84f337
Reviewed-on: https://boringssl-review.googlesource.com/12926
Reviewed-by: Adam Langley <alangley@gmail.com>
Towards an eventual goal of opaquifying BoringSSL structs, we want
our consumers -- in this case, Android's libcore -- to not directly
manipulate BigNums; and it would be convenient for them if we would
perform the appropriate gymnastics to interpret little-endian byte
streams.
It also seems a priori a bit strange to have only big-endian varieties
of BN byte-conversions.
This CL provides little-endian equivalents of BN_bn2bin_padded
and BN_bin2bn.
BUG=97
Change-Id: I0e92483286def86d9bd71a46d6a967a3be50f80b
Reviewed-on: https://boringssl-review.googlesource.com/12641
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>
BUG=97
Change-Id: I4799cc99511e73af44def1d4daa36a8b4699f62d
Reviewed-on: https://boringssl-review.googlesource.com/12904
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>
Simplify the code, and in particular make |BN_div|, |BN_mod|, and
|BN_nnmod| insensitive to |BN_FLG_CONSTTIME|. This improves the
effectiveness of testing by reducing the number of branches that are
likely to go untested or less tested.
There is no performance-sensitive code that uses BN_div but doesn't
already use BN_FLG_CONSTTIME except RSA signature verification and
EC_GROUP creation. RSA signature verification, ECDH, and ECDSA
performance aren't significantly different with this change.
Change-Id: Ie34c4ce925b939150529400cc60e1f414c7676cd
Reviewed-on: https://boringssl-review.googlesource.com/9105
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
For the most part, this is with random test data which isn't
particularly good. But we'll be able to add more interesting test
vectors as they come up.
Change-Id: I9c50db7ac2c4bf978d4901000ab32e3642aea82b
Reviewed-on: https://boringssl-review.googlesource.com/12222
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>
Android currently implements this manually (see NativeBN_putULongInt) by
reaching into BIGNUM's internals. BN_ULONG is a somewhat unfortunate API
anyway as the size is platform-dependent, so add a platform-independent
way to do this.
The other things Android needs are going to need more work, but this
one's easy.
BUG=97
Change-Id: I4af4dc29f9845bdce0f0663c379b4b5d3e1dc46e
Reviewed-on: https://boringssl-review.googlesource.com/11088
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>
Now that we have the extern "C++" trick, we can just embed them in the
normal headers. Move the EVP_CIPHER_CTX deleter to cipher.h and, in
doing so, take away a little bit of boilerplate in defining deleters.
Change-Id: I4a4b8d0db5274a3607914d94e76a38996bd611ec
Reviewed-on: https://boringssl-review.googlesource.com/10804
Reviewed-by: Matt Braithwaite <mab@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Unlike the Scoped* types, bssl::UniquePtr is available to C++ users, and
offered for a large variety of types. The 'extern "C++"' trick is used
to make the C++ bits digestible to C callers that wrap header files in
'extern "C"'.
Change-Id: Ifbca4c2997d6628e33028c7d7620c72aff0f862e
Reviewed-on: https://boringssl-review.googlesource.com/10521
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>
BN_mod_inverse_odd was always being used on 64-bit platforms and was being used
for all curves with an order of 450 bits or smaller (basically, everything but
P-521). We generally don't care much about minor differences in the speed of
verifying signatures using curves other than P-256 and P-384. It is better to
always use the same algorithm.
This also allows |bn_mod_inverse_general|, |bn_mod_inverse_no_branch|, and
|BN_mod_inverse| to be dropped from programs that can somehow avoid linking in
the RSA key generation and RSA CRT recovery code.
Change-Id: I79b94bff23d2b07d5e0c704f7d44538797f8c7a0
Reviewed-on: https://boringssl-review.googlesource.com/9103
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 functions appear to try to handle negative inputs, but it isn't
clear how negative inputs are supposed to work and/or if these
functions work the way they are supposed to given negative inputs.
There seems to be no legitimate reason to pass these functions negative
inputs, so just document that negative inputs shouldn't be used. More
specifically, document that the inputs should be in the range [0, n)
where |n| is the Montgomery modulus.
Change-Id: Id8732fb89616f10e673704e6fa09d78926c402d8
Reviewed-on: https://boringssl-review.googlesource.com/9033
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>
Have |bn_correct_top| fix |bn->neg| if the input is zero so that we
don't have negative zeros lying around.
Thanks to Brian Smith for noticing.
Change-Id: I91bcadebc8e353bb29c81c4367e85853886c8e4e
Reviewed-on: https://boringssl-review.googlesource.com/9074
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>
Yo dawg I herd you like blinding so I put inversion blinding in your
RSA blinding so you can randomly mask your random mask.
This improves upon the current situation where we pretend that
|BN_mod_inverse_no_branch| is constant-time, and it avoids the need to
exert a lot of effort to make a actually-constant-time modular
inversion function just for RSA blinding.
Note that if the random number generator weren't working correctly then
the blinding of the inversion wouldn't be very effective, but in that
case the RSA blinding itself would probably be completely busted, so
we're not really losing anything by relying on blinding to blind the
blinding.
Change-Id: I771100f0ad8ed3c24e80dd859ec22463ef2a194f
Reviewed-on: https://boringssl-review.googlesource.com/8923
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 also adds a missing OPENSSL_EXPORT.
Change-Id: I6c2400246280f68f51157e959438644976b1171b
Reviewed-on: https://boringssl-review.googlesource.com/9041
Reviewed-by: Adam Langley <agl@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>
There are many cases where we need |BN_rand_range| but with a minimum
value other than 0. |BN_rand_range_ex| provides that.
Change-Id: I564326c9206bf4e20a37414bdbce16a951c148ce
Reviewed-on: https://boringssl-review.googlesource.com/8921
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 reverts commits:
8d79ed674019fdcb52348d79ed6740
Because WebRTC (at least) includes our headers in an extern "C" block,
which precludes having any C++ in them.
Change-Id: Ia849f43795a40034cbd45b22ea680b51aab28b2d
MSVC doesn't define __cplusplus as 201103 to indicate C++11 support, so
just assume that the compiler supports C++11 if _MSC_VER is defined.
Change-Id: I27f6eeefe6e8dc522470f36fab76ab36d85eebac
Reviewed-on: https://boringssl-review.googlesource.com/8734
Reviewed-by: David Benjamin <davidben@google.com>
This change scatters the contents of the two scoped_types.h files into
the headers for each of the areas of the code. The types are now in the
|bssl| namespace.
Change-Id: I802b8de68fba4786b6a0ac1bacd11d81d5842423
Reviewed-on: https://boringssl-review.googlesource.com/8731
Reviewed-by: Adam Langley <agl@google.com>
Also, update the documentation about aliasing for |BN_usub|. It might
be better to find a way to factor out the shared logic between the
tests of these functions and the tests of |BN_add| and |BN_usub|, but
doing so would end up up creating a lot of parameters due to the many
distinct strings used in the messages.
Change-Id: Ic9d714858212fc92aa6bbcc3959576fe6bbf58c3
Reviewed-on: https://boringssl-review.googlesource.com/8593
Reviewed-by: David Benjamin <davidben@google.com>
Also update the documentation for |BN_sub|.
Change-Id: I544dbfc56f22844f6ca08e9e472ec13e76baf8c4
Reviewed-on: https://boringssl-review.googlesource.com/8592
Reviewed-by: David Benjamin <davidben@google.com>
As of 67cb49d045 and the corresponding upstream
change, BN_mod_word may fail, like BN_div_word. Handle this properly and
document in bn.h. Thanks to Brian Smith for pointing this out.
Change-Id: I6d4f32dc37bcabf70847c9a8b417d55d31b3a380
Reviewed-on: https://boringssl-review.googlesource.com/8491
Reviewed-by: Adam Langley <agl@google.com>
This function is only really useful for DSA signature verification,
which is something that isn't performance-sensitive. Replace its
optimized implementation with a naïve implementation that's much
simpler.
Note that it would be simpler to use |BN_mod_mul| in the new
implementation; |BN_mod_mul_montgomery| is used instead only to be
consistent with other work being done to replace uses of non-Montgomery
modular reduction with Montgomery modular reduction.
Change-Id: If587d463b73dd997acfc5b7ada955398c99cc342
Reviewed-on: https://boringssl-review.googlesource.com/7732
Reviewed-by: David Benjamin <davidben@google.com>
|BN_mod_exp_mont_word| is only useful when the base is a single word
in length and timing side channel protection of the exponent is not
needed. That's never the case in real life.
Keep the function in the API, but removes its single-word-base
optimized implementation with a call to |BN_mod_exp_mont|.
Change-Id: Ic25f6d4f187210b681c6ee6b87038b64a5744958
Reviewed-on: https://boringssl-review.googlesource.com/7731
Reviewed-by: David Benjamin <davidben@google.com>
Previously, the verification was only done when using the CRT method,
as the CRT method has been shown to be extremely sensitive to fault
attacks. However, there's no reason to avoid doing the verification
when the non-CRT method is used (performance-sensitive applications
should always be using the CRT-capable keys).
Previously, when we detected a fault (attack) through this verification,
libcrypto would fall back to the non-CRT method and assume that the
non-CRT method would give a correct result, despite having just
detecting corruption that is likely from an attack. Instead, just give
up, like NSS does.
Previously, the code tried to handle the case where the input was not
reduced mod rsa->n. This is (was) not possible, so avoid trying to
handle that. This simplifies the equality check and lets us use
|CRYPTO_memcmp|.
Change-Id: I78d1e55520a1c8c280cae2b7256e12ff6290507d
Reviewed-on: https://boringssl-review.googlesource.com/7582
Reviewed-by: David Benjamin <davidben@google.com>
aosp-master has been updated past the point that this is necessary. Sadly, all
the other hacks still are. I'll try to get things rolling so we can ditch the
others in time.
Change-Id: If7b3aad271141fb26108a53972d2d3273f956e8d
Reviewed-on: https://boringssl-review.googlesource.com/7751
Reviewed-by: Adam Langley <agl@google.com>
This reduces the chance of double-frees.
BUG=10
Change-Id: I11a240e2ea5572effeddc05acb94db08c54a2e0b
Reviewed-on: https://boringssl-review.googlesource.com/7583
Reviewed-by: David Benjamin <davidben@google.com>
There's many ways to serialize a BIGNUM, so not including asn1 in the name is
confusing (and collides with BN_bn2cbb_padded). Since BN_asn12bn looks
ridiculous, match the parse/marshal naming scheme of other modules instead.
Change-Id: I53d22ae0537a98e223ed943e943c48cb0743cf51
Reviewed-on: https://boringssl-review.googlesource.com/6822
Reviewed-by: Adam Langley <alangley@gmail.com>
This relieves some complexity budget for adding Curve25519 to this
code.
This also adds a BN_bn2cbb_padded helper function since this seems to be a
fairly common need.
Change-Id: Ied0066fdaec9d02659abd6eb1a13f33502c9e198
Reviewed-on: https://boringssl-review.googlesource.com/6767
Reviewed-by: Adam Langley <agl@google.com>
The |ri| field was only used in |BN_MONT_CTX_set|, so make it a local
variable of that function.
Change-Id: Id8c3d44ac2e30e3961311a7b1a6731fe2c33a0eb
Reviewed-on: https://boringssl-review.googlesource.com/6526
Reviewed-by: Adam Langley <agl@google.com>
Most functions can take this in as const. Note this changes an
RSA_METHOD hook, though one I would not expect anyone to override.
Change-Id: Ib70ae65e5876b01169bdc594e465e3e3c4319a8b
Reviewed-on: https://boringssl-review.googlesource.com/6419
Reviewed-by: Adam Langley <agl@google.com>
This change fixes up several comments (many of which were spotted by
Kenny Root) and also changes doc.go to detect cases where comments don't
start with the correct word. (This is a common error.)
Since we have docs builders now, these errors will be found
automatically in the future.
Change-Id: I58c6dd4266bf3bd4ec748763c8762b1a67ae5ab3
Reviewed-on: https://boringssl-review.googlesource.com/6440
Reviewed-by: Adam Langley <agl@google.com>
BN_mod_exp_mont_consttime does not modify its |BN_MONT_CTX| so that
value should be const.
Change-Id: Ie74e48eec8061899fd056fbd99dcca2a86b02cad
Reviewed-on: https://boringssl-review.googlesource.com/6403
Reviewed-by: Adam Langley <agl@google.com>
One less exported function. Nothing ever stack-allocates them, within BoringSSL
or in consumers. This avoids the slightly odd mechanism where BN_MONT_CTX_free
might or might not free the BN_MONT_CTX itself based on a flag.
(This is also consistent with OpenSSL 1.1.x which does away with the _init
variants of both this and BIGNUM so it shouldn't be a compatibility concern
long-term either.)
Change-Id: Id885ae35a26f75686cc68a8aa971e2ea6767ba88
Reviewed-on: https://boringssl-review.googlesource.com/6350
Reviewed-by: Adam Langley <alangley@gmail.com>
