Change-Id: I5fc029ceddfa60b2ccc97c138b94c1826f6d75fa
Reviewed-on: https://boringssl-review.googlesource.com/25844
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>
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>
If a caller is in the process on constructing an arbitrary |EC_GROUP|,
and they try to create an |EC_POINT| to set as the generator which is
invalid, we would previously crash.
Change-Id: Ida91354257a02bd56ac29ba3104c9782b8d70f6b
Reviewed-on: https://boringssl-review.googlesource.com/25764
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 allows a BIGNUM consumer to avoid messing around with bn->d and
bn->top/width.
Bug: 232
Change-Id: I134cf412fef24eb404ff66c84831b4591d921a17
Reviewed-on: https://boringssl-review.googlesource.com/25484
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>
The loop and the outermost special-cases are basically the same.
Change-Id: I5e3ca60ad9a04efa66b479eebf8c3637a11cdceb
Reviewed-on: https://boringssl-review.googlesource.com/25406
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>
Same mistake as bn_mul_recursive.
Change-Id: I2374d37e5da61c82ccb1ad79da55597fa3f10640
Reviewed-on: https://boringssl-review.googlesource.com/25405
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>
This is to be used in constant-time RSA CRT.
Bug: 233
Change-Id: Ibade5792324dc6aba38cab6971d255d41fb5eb91
Reviewed-on: https://boringssl-review.googlesource.com/25286
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>
Use the now constant-time modular arithmetic functions.
Bug: 236
Change-Id: I4567d67bfe62ca82ec295f2233d1a6c9b131e5d2
Reviewed-on: https://boringssl-review.googlesource.com/25285
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>
Functions that deserialize from bytes and Montgomery multiplication have
no reason to minimize their inputs.
Bug: 232
Change-Id: I121cc9b388033d684057b9df4ad0c08364849f58
Reviewed-on: https://boringssl-review.googlesource.com/25258
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>
These actually work as-is, but BN_bn2hex allocates more memory than
necessary, and we may as well skip the unnecessary words where we can.
Also add a test for this.
Bug: 232
Change-Id: Ie271fe9f3901d00dd5c3d7d63c1776de81a10ec7
Reviewed-on: https://boringssl-review.googlesource.com/25304
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 order (and later the field) are used to size stack-allocated fixed
width word arrays. They're also entirely public, so this is fine.
Bug: 232
Change-Id: Ie98869cdbbdfea92dcad64a300f7e0b47bef6bf2
Reviewed-on: https://boringssl-review.googlesource.com/25256
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>
(Just happened to see these as I went by.)
Change-Id: I348b163e6986bfca8b58e56885c35a813efe28f6
Reviewed-on: https://boringssl-review.googlesource.com/25725
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>
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>
These empty states aren't any use to either caller or implementor.
Change-Id: If0b748afeeb79e4a1386182e61c5b5ecf838de62
Reviewed-on: https://boringssl-review.googlesource.com/25254
Reviewed-by: Adam Langley <agl@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>
One less to worry about.
Bug: 232
Change-Id: Ib7d38e18fee02590088d76363e17f774cfefa59b
Reviewed-on: https://boringssl-review.googlesource.com/25252
Reviewed-by: Adam Langley <agl@google.com>
Saves a bit of work, and we get a width sanity-check.
Bug: 232
Change-Id: I1c6bc376c9d8aaf60a078fdc39f35b6f44a688c6
Reviewed-on: https://boringssl-review.googlesource.com/25251
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 matches bn_mod_mul_montgomery_small and removes a bit of
unnecessary stuttering.
Change-Id: Ife249c6e8754aef23c144dbfdea5daaf7ed9f48a
Reviewed-on: https://boringssl-review.googlesource.com/25248
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>
Having it in base.h pollutes the global namespace a bit and, in
particular, causes clang to give unhelpful suggestions in consuming
projects.
Change-Id: I6ca1a88bdd1701f0c49192a0df56ac0953c7067c
Reviewed-on: https://boringssl-review.googlesource.com/25464
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>
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>
Change-Id: I7932258890b0b2226ff6841af45926e1b11979ba
Reviewed-on: https://boringssl-review.googlesource.com/24844
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>
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>
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>
Some non-FIPS consumers exclude bcm.c and build each fragment file
separately. This means non-FIPS code cannot live in bcm.c.
https://boringssl-review.googlesource.com/25044 made the self-test
function exist outside of FIPS code, so it needed to be moved into is
own file.
To avoid confusing generate_build_files.py, this can't be named
self_test.c, so I went with self_check.c.
Change-Id: I337b39b158bc50d6ca0a8ad1b6e15eb851095e1e
Reviewed-on: https://boringssl-review.googlesource.com/25124
Reviewed-by: Martin Kreichgauer <martinkr@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>
Change-Id: Ib067411d4cafe1838c2dc42fc8bfd9011490f45c
Reviewed-on: https://boringssl-review.googlesource.com/25064
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 change adds |BORINGSSL_self_test|, which allows applications to run
the FIPS KAT tests on demand, even in non-FIPS builds.
Change-Id: I950b30a02ab030d5e05f2d86148beb4ee1b5929c
Reviewed-on: https://boringssl-review.googlesource.com/25044
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>
Update-Note: Token Binding can no longer be configured with the custom
extensions API. Instead, use the new built-in implementation. (The
internal repository should be all set.)
Bug: 183
Change-Id: I007523a638dc99582ebd1d177c38619fa7e1ac38
Reviewed-on: https://boringssl-review.googlesource.com/20645
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>
(Note that support for GCC 4.7 ends 2018-03-23.)
Change-Id: Ia2ac6a735c8177a2b3a13f16197ff918266bc1cb
Reviewed-on: https://boringssl-review.googlesource.com/24924
Reviewed-by: Adam Langley <agl@google.com>
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>
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 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>
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>
|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>
We would set it to block_size rather than zero. This doesn't cause
problems (the code behaves correctly with either value), but it is a
tiny missed optimization.
Change-Id: Ic751352750cc7ef74aa25a6cc96da82007199941
Reviewed-on: https://boringssl-review.googlesource.com/24364
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>
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>
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>
QUIC will need to derive keys at this point. This also smooths over a
part of the server 0-RTT abstraction. Like with False Start, the SSL
object is largely in a functional state at this point.
Bug: 221
Change-Id: I4207d8cb1273a1156e728a7bff3943cc2c69e288
Reviewed-on: https://boringssl-review.googlesource.com/24224
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>
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>
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>
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 fixes compilation on aarch64 and other architectures for Android.
Change-Id: I0b09ab06858c92d07e2376e244a4626a6af5037b
Reviewed-on: https://boringssl-review.googlesource.com/23764
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>
Change-Id: Id8b69bb6103dd938f4c6d0d2ec24f3d50ba5513c
Update-Note: fixes b/70034392
Reviewed-on: https://boringssl-review.googlesource.com/23744
Commit-Queue: Adam Langley <agl@google.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>
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>
We need it in both directions. Also I missed that in OBJ_obj2txt we
allowed uint64_t components, but in my new OBJ_txt2obj we only allowed
uint32_t. For consistency, upgrade that to uint64_t.
Bug: chromium:706445
Change-Id: I38cfeea8ff64b9acf7998e552727c6c3b2cc600f
Reviewed-on: https://boringssl-review.googlesource.com/23544
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>
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>
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>
wpa_supplicant appear to be using these.
Change-Id: I1f220cae69162901bcd9452e8daf67379c5e276c
Reviewed-on: https://boringssl-review.googlesource.com/23324
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 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>
Change-Id: Id12ab478b6ba441fb1b6f4c2f9479384fc3fbdb6
Reviewed-on: https://boringssl-review.googlesource.com/23144
Commit-Queue: David Benjamin <davidben@google.com>
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>
These use the square and multiply functions added earlier.
Change-Id: I723834f9a227a9983b752504a2d7ce0223c43d24
Reviewed-on: https://boringssl-review.googlesource.com/23070
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>
Also replace a pointless call to bn_mul_words with a memset.
Change-Id: Ief30ddab0e84864561b73fe2776bd0477931cf7f
Reviewed-on: https://boringssl-review.googlesource.com/23066
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>
We don't currently have test coverage for the order_mont bits (or lack
thereof) for custom curves.
Change-Id: I865d547c783226a5a3d3d203e10b0e59bad36984
Reviewed-on: https://boringssl-review.googlesource.com/23064
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 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>
Change-Id: I482093000ee2e4ba371c78b4f7f8e8b121e71640
Reviewed-on: https://boringssl-review.googlesource.com/22886
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
We capitalize things Go-style.
Change-Id: Id002efb8a85e4e1886164421bba059d9ca425964
Reviewed-on: https://boringssl-review.googlesource.com/22885
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
BN_generate_dsa_nonce will never generate a zero value of k.
Change-Id: I06964b815bc82aa678ffbc80664f9d788cf3851d
Reviewed-on: https://boringssl-review.googlesource.com/22884
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>
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>
These functions don't appear to do any stack manipulation thus all they
need are start/end directives in order for the correct CFI tables to be
emitted.
Change-Id: I4c94a9446030d363fa4bcb7c8975c689df3d21dc
Reviewed-on: https://boringssl-review.googlesource.com/22765
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>
Change-Id: Id70cfc78c8d103117d4c2195206b023a5d51edc3
Reviewed-on: https://boringssl-review.googlesource.com/22764
Commit-Queue: Adam Langley <agl@google.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>
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>
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>
It always returns one, so just void it.
Change-Id: I8733cc3d6b20185e782cf0291e9c0dc57712bb63
Reviewed-on: https://boringssl-review.googlesource.com/22564
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>
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>
This partially reverts commit 38636aba74.
Some build on Android seems to break now. I'm not really sure what the
situation is, but if the weird common symbols are still there (can we
remove them?), they probably ought to have the right flags.
Change-Id: Ief589d763d16b995ac6be536505acf7596a87b30
Reviewed-on: https://boringssl-review.googlesource.com/22404
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>
Those EXPECTs should be ASSERTs to ensure bn is not null.
Change-Id: Icb54c242ffbde5f8eaa67f19f214c9eef13705ea
Reviewed-on: https://boringssl-review.googlesource.com/22366
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>
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>
1 << 31 is technically an undefined shift. It should be 1u << 31 to shut
UBSan up. I've also converted the others for consistency.
Change-Id: I1c6fe282f55c7032cea39f5ff1035a7711155f02
Reviewed-on: https://boringssl-review.googlesource.com/22344
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>
(Credit to libFuzzer for finding this.)
Change-Id: I0353d686d883703d39145c5bdd1e56368a587a35
Reviewed-on: https://boringssl-review.googlesource.com/22324
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>
Also switch them to accepting a u16 length prefix. We appear not to have
any such tests right now, but RSA-2048 would involve modulus well larger
and primes just a hair larger than a u8 length prefix alows.
Change-Id: Icce8f1d976e159b945302fbba732e72913c7b724
Reviewed-on: https://boringssl-review.googlesource.com/22284
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 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 reverts commit f6942f0d22.
Reason for revert: This doesn't actually work in clang-cl. I
forgot we didn't have the clang-cl try bots enabled! :-( I
believe __asm__ is still okay, but I'll try it by hand
tomorrow.
Original change's description:
> Use uint128_t and __asm__ in clang-cl.
>
> clang-cl does not define __GNUC__ but is still a functioning clang. We
> should be able to use our uint128_t and __asm__ code in it on Windows.
>
> Change-Id: I67310ee68baa0c0c947b2441c265b019ef12af7e
> Reviewed-on: https://boringssl-review.googlesource.com/22184
> 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>
TBR=agl@google.com,davidben@google.com
Change-Id: I5c7e0391cd9c2e8cc0dfde37e174edaf5d17db22
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://boringssl-review.googlesource.com/22224
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>
clang-cl does not define __GNUC__ but is still a functioning clang. We
should be able to use our uint128_t and __asm__ code in it on Windows.
Change-Id: I67310ee68baa0c0c947b2441c265b019ef12af7e
Reviewed-on: https://boringssl-review.googlesource.com/22184
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>
There is also no need to make the struct public. Also tidy up includes a
bit.
Change-Id: I188848dfd8f9ed42925b2c55da8dc4751c29f146
Reviewed-on: https://boringssl-review.googlesource.com/22126
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>
Some of the complaints seem a bit questionable or their replacements
problematic, but not using strcat, strcpy, and strncpy is easy and
safer.
Change-Id: I64faf24b4f39d1ea410e883f026350094975a9b5
Reviewed-on: https://boringssl-review.googlesource.com/22125
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>
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>
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>
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>
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>
Our build logic needed to revised and and clang implements more warnings
than MSVC, so GTest needed more fixes.
Bug: 200
Change-Id: I84c5dd0c51079dd9c990e08dbea7f9022a7d6842
Reviewed-on: https://boringssl-review.googlesource.com/21204
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>
Right now, compiling with the stock gcc on debian, cmake is compiling
with -Wall which gives an error because -Wunused-value.
The gcc version is gcc (Debian 4.7.2-5) 4.7.2.
Change-Id: Iafd4cc14a22fe788d4c7bdb05202fd856f0c6395
Reviewed-on: https://boringssl-review.googlesource.com/21144
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>
ERR_FLAGS_STRING is meaningless and we can use a bitfield for the mark
bit.
Change-Id: I6f677b55b11316147512171629196c651cb33ca9
Reviewed-on: https://boringssl-review.googlesource.com/21084
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>
sha1-altivec.c is not sensitive to OPENSSL_NO_ASM, so sha1.c needs to
disable the generic implementation accordingly.
Bug: 204
Change-Id: Ic655f8b76907f07da33afa863d1b24d62d42e23a
Reviewed-on: https://boringssl-review.googlesource.com/21064
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>
Cut down on the number of cases we need to worry about here. In
particular, it would be useful for the handshake to be able to replay an
error.
Change-Id: I2345faaff5503ede1324a5599e680de83f4b106e
Reviewed-on: https://boringssl-review.googlesource.com/21004
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>
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:
bcc92d75df
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>
In case the XCode install is at, say "/Applications/Xcode 9.app". This
won't work if the path contains quotes, but it doesn't appear CMake
itself makes any effort to handle that right.
Change-Id: Ifecf6147d44ffdae8c2692b2d6c94bfafd8d7714
Reviewed-on: https://boringssl-review.googlesource.com/20944
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>
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>
Chromium's OCSP code needs the OIDs and we already have them on hand.
Change-Id: Icab012ba4ae15ce029cbfe3ed93f89470137e7f6
Reviewed-on: https://boringssl-review.googlesource.com/20724
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 haven't supported MSVC 2013 for a while (we may even be able to drop
2015 in not too long). There is also no need to pull in stdalign.h in
C++. alignof and alignas are keywords.
Change-Id: Ib31d8166282592bcb9e1c543e57758ff55746404
Reviewed-on: https://boringssl-review.googlesource.com/20704
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>
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>
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>
We don't get up to 16-byte alignment without additional work like
https://boringssl-review.googlesource.com/20204. This just makes UBSan
unhappy at us.
Change-Id: I55d9cb5b40e5177c3c7aac7828c1d22f2bfda9a6
Reviewed-on: https://boringssl-review.googlesource.com/20464
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>
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>
Thes are remnants of some old setup.
Change-Id: I09151fda9419fbe7514f2f609f70284965694bfa
Reviewed-on: https://boringssl-review.googlesource.com/20365
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 keep Chromium building.
Bug: chromium:765754
Change-Id: I312f747e27e53590a948305f80abc240bfd2063c
Reviewed-on: https://boringssl-review.googlesource.com/20344
Reviewed-by: Aaron Green <aarongreen@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Fuchsia needed to rename Magenta to Zircon. Several syscalls and status
codes changed as a result.
Change-Id: I64b5ae4537ccfb0a318452fed34040a2e8f5012e
Reviewed-on: https://boringssl-review.googlesource.com/20324
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>
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>
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>
Allocations by |OPENSSL_malloc| are prefixed with their length.
|OPENSSL_free| zeros the allocation before calling free(), eliminating
the need for a separate call to |OPENSSL_cleanse| for sensitive data.
This change will be followed up by the cleanup in
https://boringssl-review.googlesource.com/c/boringssl/+/19824.
Change-Id: Ie272f07e9248d7d78af9aea81dacec0fdb7484c4
Reviewed-on: https://boringssl-review.googlesource.com/19544
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>
Rather than clear them, even on failure, detect if an individual test
failed and dump the error queue there. We already do this at the GTest
level in ErrorTestEventListener, but that is too coarse-grained for the
file tests.
Change-Id: I3437626dcf3ec43f6fddd98153b0af73dbdcce84
Reviewed-on: https://boringssl-review.googlesource.com/19966
Reviewed-by: Steven Valdez <svaldez@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>
We have no tests for encryption right now, and evp_tests.txt needs to
force RSA-PSS to have salt length 0, even though other salt values are
more common. This also lets us test the salt length -2 silliness.
Change-Id: I30f52d36c38732c9b63a02c66ada1d08488417d4
Reviewed-on: https://boringssl-review.googlesource.com/19965
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>
It was pointed out that we have no test coverage of this. Fix this. Test
vector generated using Go's implementation.
Change-Id: Iddbc50d3b422e853f8afd50117492f4666a47373
Reviewed-on: https://boringssl-review.googlesource.com/19964
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>
linux/random.h is not really needed if FIPS mode is not enabled. Note
that use of the getrandom syscall is unaffected by this header.
Fixes commit bc7daec4d8
Change-Id: Ia367aeffb3f2802ba97fd1507de0b718d9ac2c55
Reviewed-on: https://boringssl-review.googlesource.com/19644
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>
