958aaf1ea1, imported from upstream, had an
off-by-one error. Reproducing the failure is fairly easy as it can't
even serialize 1. See also upstream's
099e2968ed3c7d256cda048995626664082b1b30.
Rewrite the function completely with CBB and add a basic test.
BUG=chromium:639740
Change-Id: I41a91514c4bb9e83854824ed5258ffe4e49d9491
Reviewed-on: https://boringssl-review.googlesource.com/10540
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>
Change-Id: Ie60744761f5aa434a71a998f5ca98a8f8b1c25d5
Reviewed-on: https://boringssl-review.googlesource.com/10447
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 found an earlier reference for an algorithm for the optimized
computation of n0 that is very similar to the one in the "Montgomery
Multiplication" paper cited in the comments. Add a reference to it.
Henry S. Warren, Jr. pointed out that his "Montgomery Multiplication"
paper is not a chapter of his book, but a supplement to the book.
Correct the reference to it.
Change-Id: Iadeb148c61ce646d1262ccba0207a31ebdad63e9
Reviewed-on: https://boringssl-review.googlesource.com/10480
Reviewed-by: Adam Langley <agl@google.com>
This was causing some Android breakage. The real bug is actually
entirely in Android for getting its error-handling code wrong and not
handling multiple errors. I'll fix that. (See b/30917411.)
That said, BN_R_NO_INVERSE is a perfectly legitimate reason for those
operations to fail, so ERR_R_INTERNAL_ERROR isn't really a right thing
to push in front anyway. We're usually happy enough with single-error
returns (I'm still a little skeptical of this queue idea), so let's just
leave it at that.
Change-Id: I469b6e2b5987c6baec343e2cfa52bdcb6dc42879
Reviewed-on: https://boringssl-review.googlesource.com/10483
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>
If an oversize BIGNUM is presented to BN_bn2dec() it can cause
BN_div_word() to fail and not reduce the value of 't' resulting
in OOB writes to the bn_data buffer and eventually crashing.
Fix by checking return value of BN_div_word() and checking writes
don't overflow buffer.
Thanks to Shi Lei for reporting this bug.
CVE-2016-2182
(Imported from upstream's e36f27ddb80a48e579783bc29fb3758988342b71.)
Change-Id: Ib9078921b4460952c4aa5a6b03ec39a03704bb90
Reviewed-on: https://boringssl-review.googlesource.com/10367
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This more accurately reflects the documented contract for
|BN_mod_inverse_odd|.
Change-Id: Iae98dabe3943231859eaa5e798d06ebe0231b9f1
Reviewed-on: https://boringssl-review.googlesource.com/9160
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This eliminates duplicate logic.
Change-Id: I283273ae152f3644df4384558ee4a021f8c2d454
Reviewed-on: https://boringssl-review.googlesource.com/9104
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
BN_mod_inverse_odd was always being used on 64-bit platforms and was being used
for all curves with an order of 450 bits or smaller (basically, everything but
P-521). We generally don't care much about minor differences in the speed of
verifying signatures using curves other than P-256 and P-384. It is better to
always use the same algorithm.
This also allows |bn_mod_inverse_general|, |bn_mod_inverse_no_branch|, and
|BN_mod_inverse| to be dropped from programs that can somehow avoid linking in
the RSA key generation and RSA CRT recovery code.
Change-Id: I79b94bff23d2b07d5e0c704f7d44538797f8c7a0
Reviewed-on: https://boringssl-review.googlesource.com/9103
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
The main RSA public modulus size of concern is 2048 bits.
bn_mod_inverse_odd is already used for public moduli of 2048 bits and
smaller on 64-bit platforms, so for 64-bit it is a no-op. For 32-bit
x86, this seems to slightly decrease the speed of RSA signing, but not
by a lot, and plus we don't care about RSA signing performance much on
32-bit platforms. It's better to have all platforms using the same
algorithms.
Change-Id: I869dbfc98994e36a04a535c1fe63b14a902a4f13
Reviewed-on: https://boringssl-review.googlesource.com/9102
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is a step towards exposing |bn_mod_inverse_odd| for use outside
of crypto/bn/gcd.c.
Change-Id: I2968f1e43306c03775b3573a022edd92f4e91df2
Reviewed-on: https://boringssl-review.googlesource.com/9101
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This is in preparation for factoring out the binary Euclidean
implementation (the one used for odd numbers that aren't too big) for
direct use from outside of crypto/bn/gcd.c. The goal is to make the
resultant |BN_mod_inverse_odd|'s signature similar to
|BN_mod_inverse_blinded|. Thus, the logic for reducing the final result
isn't factored out because that yet-to-be-created |BN_mod_inverse_odd|
will need to do it itself.
Change-Id: Iaecb79fb17d13c774c4fb6ade8742937780b0006
Reviewed-on: https://boringssl-review.googlesource.com/9100
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
|BN_mod_exp_mont| uses |BN_nnmod| so it seems like
|BN_mod_exp_mont_consttime| should too. Further, I created
these test vectors by doing the math by hand, and the tests
passed for |BN_mod_exp_mont| but failed for
|BN_mod_exp_mont_consttime| without this change.
Change-Id: I7cffa1375e94dd8eaee87ada78285cd67fff1bac
Reviewed-on: https://boringssl-review.googlesource.com/9032
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>
Have |bn_correct_top| fix |bn->neg| if the input is zero so that we
don't have negative zeros lying around.
Thanks to Brian Smith for noticing.
Change-Id: I91bcadebc8e353bb29c81c4367e85853886c8e4e
Reviewed-on: https://boringssl-review.googlesource.com/9074
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Negative zeros are nuts, but it will probably be a while before we've
fixed everything that can create them. Fix both to consistently print
'-0' rather than '0' so failures are easier to diagnose (BN_cmp believes
the values are different.)
Change-Id: Ic38d90601b43f66219d8f44ca085432106cf98e3
Reviewed-on: https://boringssl-review.googlesource.com/9073
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Simplify the calculation of the Montgomery constants in
|BN_MONT_CTX_set|, making the inversion constant-time. It should also
be faster by avoiding any use of the |BIGNUM| API in favor of using
only 64-bit arithmetic.
Now it's obvious how it works. /s
Change-Id: I59a1e1c3631f426fbeabd0c752e0de44bcb5fd75
Reviewed-on: https://boringssl-review.googlesource.com/9031
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We managed to mix two comment styles in the Go license headers and
copy-and-paste it throughout the project.
Change-Id: Iec1611002a795368b478e1cae0b53127782210b1
Reviewed-on: https://boringssl-review.googlesource.com/9060
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>
Yo dawg I herd you like blinding so I put inversion blinding in your
RSA blinding so you can randomly mask your random mask.
This improves upon the current situation where we pretend that
|BN_mod_inverse_no_branch| is constant-time, and it avoids the need to
exert a lot of effort to make a actually-constant-time modular
inversion function just for RSA blinding.
Note that if the random number generator weren't working correctly then
the blinding of the inversion wouldn't be very effective, but in that
case the RSA blinding itself would probably be completely busted, so
we're not really losing anything by relying on blinding to blind the
blinding.
Change-Id: I771100f0ad8ed3c24e80dd859ec22463ef2a194f
Reviewed-on: https://boringssl-review.googlesource.com/8923
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This also adds a missing OPENSSL_EXPORT.
Change-Id: I6c2400246280f68f51157e959438644976b1171b
Reviewed-on: https://boringssl-review.googlesource.com/9041
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
There are many cases where we need |BN_rand_range| but with a minimum
value other than 0. |BN_rand_range_ex| provides that.
Change-Id: I564326c9206bf4e20a37414bdbce16a951c148ce
Reviewed-on: https://boringssl-review.googlesource.com/8921
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Zero is only a valid input to or output of |BN_mod_inverse| when the
modulus is one. |BN_MONT_CTX_set| actually depends on this, so test
that this works.
Change-Id: Ic18f1fe786f668394951d4309020c6ead95e5e28
Reviewed-on: https://boringssl-review.googlesource.com/8922
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>
Some gerrit git hook says this is necessary.
Change-Id: I8a7a0a0e6732688c965b43824fe54b2db79a4919
Reviewed-on: https://boringssl-review.googlesource.com/8990
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>
Besides reducing code duplication, also move the relative location of
the check of |count|. Previously, the code was generating a random
value and then terminating the loop without using it if |count| went
to zero. Now the wasted call to |BN_rand| is not made.
Also add a note about the applicability of the special case logic for
|range| of the form |0b100...| to RSA blinding.
Change-Id: Iaa33b9529f1665ac59aefcc8b371fa32445e7578
Reviewed-on: https://boringssl-review.googlesource.com/8960
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
BN_mod_mul_montgomery has a problem where the modulus is much smaller
than one of the arguments. While bn_test.cc knows this and reduces the
inputs before testing |BN_mod_mul_montgomery|, none of the previous test
vectors actually failed without this. (Except those that passed negative
vaules.)
This change adds tests where M ≪ A and B.
Change-Id: I53b5188ea5fb5e48d0d197718ed33c644cde8477
Reviewed-on: https://boringssl-review.googlesource.com/8890
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Brian Smith <brian@briansmith.org>
Commit-Queue: David Benjamin <davidben@google.com>
This reverts commits:
8d79ed674019fdcb52348d79ed6740
Because WebRTC (at least) includes our headers in an extern "C" block,
which precludes having any C++ in them.
Change-Id: Ia849f43795a40034cbd45b22ea680b51aab28b2d
This change scatters the contents of the two scoped_types.h files into
the headers for each of the areas of the code. The types are now in the
|bssl| namespace.
Change-Id: I802b8de68fba4786b6a0ac1bacd11d81d5842423
Reviewed-on: https://boringssl-review.googlesource.com/8731
Reviewed-by: Adam Langley <agl@google.com>
We currently have the situation where the |tool| and |bssl_shim| code
includes scoped_types.h from crypto/test and ssl/test. That's weird and
shouldn't happen. Also, our C++ consumers might quite like to have
access to the scoped types.
Thus this change moves some of the template code to base.h and puts it
all in a |bssl| namespace to prepare for scattering these types into
their respective headers. In order that all the existing test code be
able to access these types, it's all moved into the same namespace.
Change-Id: I3207e29474dc5fcc344ace43119df26dae04eabb
Reviewed-on: https://boringssl-review.googlesource.com/8730
Reviewed-by: David Benjamin <davidben@google.com>
We usually put main at the end. There's now nothing interesting in the
function, so avoid having to declare every test at the top.
Change-Id: Iac469f41f0fb7d1f58d12dfbf651bf0d39f073d0
Reviewed-on: https://boringssl-review.googlesource.com/8712
Reviewed-by: David Benjamin <davidben@google.com>
That removes the last of the bc stuff.
BUG=31
Change-Id: If64c974b75c36daf14c46f07b0d9355b7cd0adcb
Reviewed-on: https://boringssl-review.googlesource.com/8711
Reviewed-by: David Benjamin <davidben@google.com>
Amazingly, this function actually has (not crypto-related) callers, despite
being pretty much useless for cryptography.
BUG=31
Change-Id: I440827380995695c7a15bbf2220a05ffb28d9335
Reviewed-on: https://boringssl-review.googlesource.com/8594
Reviewed-by: David Benjamin <davidben@google.com>
These were generated by running test_mod_exp_mont5 10 times. The values with
Montgomery representation 1 were generated separately so the test file could
preserve the comment. (Though, at 10,000 lines, no one's going to find it...)
BUG=31
Change-Id: I8e9d4d6d7b5f7d283bd259df10a1dbdc90b888cf
Reviewed-on: https://boringssl-review.googlesource.com/8611
Reviewed-by: David Benjamin <davidben@google.com>
Honestly, with this size of number, they're pretty bad test vectors.
test_mod_exp_mont5 will be imported in the next commit which should help.
This was done by taking test_mod_exp's generation, running it a few times
(since otherwise the modulus is always the same). I also ran it a few times
with the odd constraint removed since BN_mod_exp is supposed to support it,
even if it's not actually useful.
BUG=31
Change-Id: Id53953f0544123a5ea71efac534946055dd5aabc
Reviewed-on: https://boringssl-review.googlesource.com/8610
Reviewed-by: David Benjamin <davidben@google.com>
That one needs reduced inputs and the other ought to be also tested against
unreduced ones is a bit annoying. But the previous commit made sure BN_nnmod
has tests, and test_mont could stand to inherit test_mod_mul's test data (it
only had five tests originally!), so I merged them.
BUG=31
Change-Id: I1eb585b14f85f0ea01ee81537a01e07ced9f5d9a
Reviewed-on: https://boringssl-review.googlesource.com/8608
Reviewed-by: David Benjamin <davidben@google.com>
In preparation for converting test_mont and test_mod_mul to test vectors, make
test_mont less silly. We can certainly get away with doing more than five
tests. Also generate |a| and |b| anew each time. Otherwise the first BN_nmod is
destructive.
Change-Id: I944007ed7b6013a16d972cb7290ab9992c9360ce
Reviewed-on: https://boringssl-review.googlesource.com/8605
Reviewed-by: David Benjamin <davidben@google.com>
No need for the special case and such.
Change-Id: If8fbc73eda0ccbaf3fd422e97c96fee6dc10b1ab
Reviewed-on: https://boringssl-review.googlesource.com/8604
Reviewed-by: David Benjamin <davidben@google.com>
Since the format no longer is readable by bc, compare it to Go's math/big
instead.
Change-Id: I34d37aa0c29c6f4178267858cb0d3941b4266b93
Reviewed-on: https://boringssl-review.googlesource.com/8603
Reviewed-by: David Benjamin <davidben@google.com>
Also, update the documentation about aliasing for |BN_usub|. It might
be better to find a way to factor out the shared logic between the
tests of these functions and the tests of |BN_add| and |BN_usub|, but
doing so would end up up creating a lot of parameters due to the many
distinct strings used in the messages.
Change-Id: Ic9d714858212fc92aa6bbcc3959576fe6bbf58c3
Reviewed-on: https://boringssl-review.googlesource.com/8593
Reviewed-by: David Benjamin <davidben@google.com>
Also update the documentation for |BN_sub|.
Change-Id: I544dbfc56f22844f6ca08e9e472ec13e76baf8c4
Reviewed-on: https://boringssl-review.googlesource.com/8592
Reviewed-by: David Benjamin <davidben@google.com>
|BN_mod_exp_mont| should be tested the same way as the other variants,
especially since it is exported.
Change-Id: I8c05725289c0ebcce7aba7e666915c4c1a841c2b
Reviewed-on: https://boringssl-review.googlesource.com/8590
Reviewed-by: David Benjamin <davidben@google.com>
The bc ones will all get replaced later.
Change-Id: Ic1c6ee320b3a5689c7dadea3f483bd92f7e39612
Reviewed-on: https://boringssl-review.googlesource.com/8528
Reviewed-by: Adam Langley <agl@google.com>
These can all share one test type. Note test_div had a separate
division by zero test which had to be extracted.
BUG=31
Change-Id: I1de0220fba78cd7f82a5dc96adb34b79c07929e9
Reviewed-on: https://boringssl-review.googlesource.com/8527
Reviewed-by: Adam Langley <agl@google.com>
crypto/bn/bn_test.cc:404:44: error: ‘n’ may be used uninitialized in this
function [-Werror=maybe-uninitialized]
Change-Id: Id590dfee4b9ae1a4fbd0965e133310dac0d06ed3
