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>
In order to delay the digest of the handshake transcript and unify
around message-based signing callbacks, a copy of the transcript is kept
around until we are sure there is no certificate authentication.
This removes support for SSL_PRIVATE_KEY_METHOD as a client in SSL 3.0.
Change-Id: If8999a19ca021b4ff439319ab91e2cd2103caa64
Reviewed-on: https://boringssl-review.googlesource.com/8561
Reviewed-by: David Benjamin <davidben@google.com>
This file contains nothing but no-op functions. There's nothing to include.
Change-Id: I3a21207d6a47fab3a00c3f72011abef850ed7b27
Reviewed-on: https://boringssl-review.googlesource.com/8541
Reviewed-by: Steven Valdez <svaldez@google.com>
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
Two of these were even regression tests for a past bug. These are also
moved to the file, now with the amazing innovation that we *actually
check the regression test gave the right answer*.
BUG=31
Change-Id: I8097336ad39a2bb5c0af07dd8e1e34723b68d182
Reviewed-on: https://boringssl-review.googlesource.com/8525
Reviewed-by: Adam Langley <agl@google.com>
This adds tests for:
for i = 0 to 199:
Sum: 2^i
A: 2^i - 1
B: 1
for i = 0 to 199:
Sum: 2^200
A: 2^200 - 2^i
B: 2^i
I don't believe any of the existing tests actually stressed this,
amazingly enough.
Change-Id: I5edab6327bad45fc21c62bd47f4169f8bb745ff7
Reviewed-on: https://boringssl-review.googlesource.com/8523
Reviewed-by: Adam Langley <agl@google.com>
This took some finesse. I merged the lshift1 and rshift1 test vectors as
one counted down and the other up. The rshift1 vectors were all rounded
to even numbers, with the test handling the odd case. Finally, each run
only tested positive or negative (it wasn't re-randomized), so I added
both positive and negative versions of each test vector.
BUG=31
Change-Id: Ic7de45ab797074547c44c2e4ff8089b1feec5d57
Reviewed-on: https://boringssl-review.googlesource.com/8522
Reviewed-by: Adam Langley <agl@google.com>
MSVC 2015 seems to support it just fine.
Change-Id: I9c91c18c260031e6024480d1f57bbb334ed7118c
Reviewed-on: https://boringssl-review.googlesource.com/8501
Reviewed-by: Adam Langley <agl@google.com>
Test vectors taken from one run of bc_test with the -bc flag, along with
a handful of manual test vectors around numbers close to zero. (The
output was compared against bc to make sure it was correct.)
BUG=31
Change-Id: I9e9263ece64a877c8497716cd4713b4c3e44248c
Reviewed-on: https://boringssl-review.googlesource.com/8521
Reviewed-by: Adam Langley <agl@google.com>
Upstream added new instructions in
f4d456408d9d7bca31f34765d1a05fbd9fa55826 and
4e3d2866b6e8e7a700ea22e05840a093bfd7a4b1.
Change-Id: I835650426a0dffca2d8686d64aef99097a4bd186
Reviewed-on: https://boringssl-review.googlesource.com/8520
Reviewed-by: Adam Langley <agl@google.com>
(Imported from upstream's 67b8bf4d849a7c40d0226de4ebe2590c4cc7c1f7.)
Verified a no-op in generate_build_files.py.
Change-Id: I09648893ab5c795f3934da0b2ecbc5fd7eb068d5
Reviewed-on: https://boringssl-review.googlesource.com/8519
Reviewed-by: Adam Langley <agl@google.com>
Depending on architecture, perlasm differed on which one or both of:
perl foo.pl flavor output.S
perl foo.pl flavor > output.S
Upstream has now unified on the first form after making a number of
changes to their files (the second does not even work for their x86
files anymore). Sync those portions of our perlasm scripts with upstream
and update CMakeLists.txt and generate_build_files.py per the new
convention.
This imports various commits like this one:
184bc45f683c76531d7e065b6553ca9086564576 (this was done by taking a
diff, so I don't have the full list)
Confirmed that generate_build_files.py sees no change.
BUG=14
Change-Id: Id2fb5b8bc2a7369d077221b5df9a6947d41f50d2
Reviewed-on: https://boringssl-review.googlesource.com/8518
Reviewed-by: Adam Langley <agl@google.com>
We're not using the masm output (and upstream does not even support it).
Reduce unnecessary diff from upstream.
Change-Id: Ic0b0f804bd7ec1429b3b1f40746297b57dcfcef6
Reviewed-on: https://boringssl-review.googlesource.com/8517
Reviewed-by: Adam Langley <agl@google.com>
It was missing. Writing NewSessionTicket will need it.
Change-Id: I39de237894f2e8356bd6861da2b8a4d805dcd2d6
Reviewed-on: https://boringssl-review.googlesource.com/8439
Reviewed-by: Adam Langley <agl@google.com>
This also adds a missing check to the C half to ensure fake record types are
always correct, to keep implementations honest.
Change-Id: I1d65272e647ffa67018c721d52c639f8ba47d647
Reviewed-on: https://boringssl-review.googlesource.com/8510
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
As of 67cb49d045 and the corresponding upstream
change, BN_mod_word may fail, like BN_div_word. Handle this properly and
document in bn.h. Thanks to Brian Smith for pointing this out.
Change-Id: I6d4f32dc37bcabf70847c9a8b417d55d31b3a380
Reviewed-on: https://boringssl-review.googlesource.com/8491
Reviewed-by: Adam Langley <agl@google.com>
This function returns a tri-state -1 on error. We should check this.
Change-Id: I6fe130c11d10690923aac5ac7a6dfe3e3ff3f5e9
Reviewed-on: https://boringssl-review.googlesource.com/8490
Reviewed-by: Adam Langley <agl@google.com>
It was already nearly clean. Just one undeclared variable.
(Imported from upstream's abeae4d3251181f1cedd15e4433e79406b766155.)
Change-Id: I3b8f20034f914fc44faabf165d1553d4084c87cc
Reviewed-on: https://boringssl-review.googlesource.com/8393
Reviewed-by: Adam Langley <agl@google.com>
This functionally pulls in a number of changes from upstream, including:
4e3d2866b6e8e7a700ea22e05840a093bfd7a4b1
1eb12c437bbeb2c748291bcd23733d4a59d5d1ca
6a4ea0022c475bbc2c7ad98a6f05f6e2e850575b
c25278db8e4c21772a0cd81f7873e767cbc6d219
e0a651945cb5a70a2abd9902c0fd3e9759d35867
d405aa2ff265965c71ce7331cf0e49d634a06924
ce3d25d3e5a7e82fd59fd30dff7acc39baed8b5e
9ba96fbb2523cb12747c559c704c58bd8f9e7982
Notably, c25278db8e4c21772a0cd81f7873e767cbc6d219 makes it enable 'use strict'.
To avoid having to deal with complex conflicts, this was done by taking a diff
of our copy of the file with the point just before
c25278db8e4c21772a0cd81f7873e767cbc6d219, and reapplying the non-reverting
parts of our diff on top of upstream's current version.
Confirmed with generate_build_files.py that this makes no changes *except*
d405aa2ff265965c71ce7331cf0e49d634a06924 causes this sort of change throughout
chacha-x86_64.pl's nasm output:
@@ -1179,7 +1179,7 @@ $L$oop8x:
vpslld ymm14,ymm0,12
vpsrld ymm0,ymm0,20
vpor ymm0,ymm14,ymm0
- vbroadcasti128 ymm14,YMMWORD[r11]
+ vbroadcasti128 ymm14,XMMWORD[r11]
vpaddd ymm13,ymm13,ymm5
vpxor ymm1,ymm13,ymm1
vpslld ymm15,ymm1,12
This appears to be correct. vbroadcasti128 takes a 128-bit-wide second
argument, so it wants XMMWORD, not YMMWORD. I suppose nasm just didn't care.
(Looking at a diff-diff may be a more useful way to review this CL.)
Change-Id: I61be0d225ddf13b5f05d1369ddda84b2f322ef9d
Reviewed-on: https://boringssl-review.googlesource.com/8392
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
In order to ensure that we don't randomly interoperate with
implementations that don't mask scalars correctly, always generate
scalars with the wrong fixed bits.
Change-Id: I82536a856f034cfe4464fc545a99c21b3cff1691
Reviewed-on: https://boringssl-review.googlesource.com/8391
Reviewed-by: David Benjamin <davidben@google.com>
It's always one. We don't support other kinds of curves with this framework.
(Curve25519 uses a much simpler API.) This also allows us to remove the
check_pub_key_order logic.
Change-Id: Ic15e1ecd68662b838c76b1e0aa15c3a93200d744
Reviewed-on: https://boringssl-review.googlesource.com/8350
Reviewed-by: Adam Langley <agl@google.com>
DSA is deprecated, but get this aligned with some of the BN_FLG_CONSTTIME work
going on elsewhere.
Change-Id: I676ceab298a69362bef1b61d6f597c5c90da2ff0
Reviewed-on: https://boringssl-review.googlesource.com/8309
Reviewed-by: Adam Langley <agl@google.com>
It's a prime, so computing a constant-time mod inverse is straight-forward.
Change-Id: Ie09b84363c3d5da827989300a844c470437fd8f2
Reviewed-on: https://boringssl-review.googlesource.com/8308
Reviewed-by: Adam Langley <agl@google.com>
The case where ec_group_get_mont_data is NULL is only for arbitrary groups
which we now require to be prime order. BN_mod_exp_mont is fine with a NULL
BN_MONT_CTX. It will just compute it. Saves a bit of special-casing.
Also don't mark p-2 as BN_FLG_CONSTTIME as the exponent is public anyway.
Change-Id: Ie868576d52fc9ae5f5c9f2a4039a729151bf84c7
Reviewed-on: https://boringssl-review.googlesource.com/8307
Reviewed-by: Adam Langley <agl@google.com>
The Conscrypt revert cycled in long ago.
Change-Id: If3cdb211d7347dca88bd70bdc643f80b19a7e528
Reviewed-on: https://boringssl-review.googlesource.com/8306
Reviewed-by: Adam Langley <agl@google.com>
Make |BN_mod_inverse_ex| symmetric with |BN_mod_inverse_no_branch| in
this respect.
Change-Id: I4a5cbe685edf50e13ee1014391bc4001f5371fec
Reviewed-on: https://boringssl-review.googlesource.com/8316
Reviewed-by: David Benjamin <davidben@google.com>
The alignas in NEWPOLY_POLY told the compiler that it could assume a
certain alignment. However, values were allocated with malloc with no
specific alignment.
We could try and allocate aligned memory but the alignment doesn't have
a performance impact (on x86-64) so this is the simpler change. (Also,
Windows doesn't have |posix_memalign|. The cloest thing is
_alligned_alloc but then one has to use a special free function.)
Change-Id: I53955a88862160c02aa5436d991b1b797c3c17db
Reviewed-on: https://boringssl-review.googlesource.com/8315
Reviewed-by: David Benjamin <davidben@google.com>
There's no use doing the remaining work if we're going to fail due to
there being no inverse.
Change-Id: Ic6d7c92cbbc2f7c40c51e6be2de3802980d32543
Reviewed-on: https://boringssl-review.googlesource.com/8310
Reviewed-by: David Benjamin <davidben@google.com>
This ensures that the test is not flaky after lots of iterations.
Along the way, change newhope_test.cc to C++.
Change-Id: I4ef139444b8c8a98db53d075105eb6806f6c5fc7
Reviewed-on: https://boringssl-review.googlesource.com/8110
Reviewed-by: Adam Langley <agl@google.com>
(This change will be sent upstream. Since the legacy X.509 stack is just
kept around for compatibility, if they decide to fix it in a different
way, we may wish to revert this and apply their fix.)
Dating back to SSLeay, X509_LOOKUP_METHOD had this X509_LU_RETRY
machinery. But it's not documented and it appears to have never worked.
Problems with the existing logic:
- X509_LU_* is not sure whether it is a type enum (to be passed into
X509_LOOKUP_by_*) or a return enum (to be retained by those same
functions).
- X509_LOOKUP_by_* is not sure whether it returns 0/1 or an X509_LU_*
value. Looking at the functions themselves, one might think it's the
latter, but for X509_LOOKUP_by_subject returning both 0 and
X509_LU_FAIL. But looking at the call sites, some expect 0/1 (such as
X509_STORE_get1_certs) while others expect an X509_LU_* enum (such as
X509_STORE_CTX_get1_issuer). It is very fortunate that FAIL happens to
be 0 and X509 happens to be 1.
These functions primarily call to X509_LOOKUP_METHOD hooks. Looking
through OpenSSL itself and code checked into Google, I found no
evidence that any hooks have been implemented except for
get_by_subject in by_dir.c. We take that one as definitive and observe
it believes it returns 0/1. Notably, it returns 1 on success even if
asked for a type other than X509_LU_X509. (X509_LU_X509 = 1. Others are
different.) I found another piece of third-party software which corroborates
this worldview.
- X509_STORE_get_by_subject's handling of X509_LU_RETRY (it's the j < 0
check) is broken. It saves j into vs->current_method where it probably
meant to save i. (This bug has existed since SSLeay.)
It also returns j (supposedly X509_LU_RETRY) while all callers of
X509_STORE_get_by_subject expect it to return 0/1 by checking with !
instead of <= 0. (Note that all other codepaths return 0 and 1 so this
function did not actually believe it returned X509_LU_* most of the
time.)
This, in turn, gives us a free of uninitialized pointers in
X509_STORE_get1_certs and other functions which expect that *ret is
filled in if X509_STORE_get_by_subject returns success. GCC 4.9 with
optimizations from the Android NDK noticed this, which trigged this
saga.
(It's only reachable if any X509_LOOKUP_METHOD returned
X509_LU_RETRY.)
- Although the code which expects X509_STORE_get_by_subject return 0/1
does not date to SSLeay, the X509_STORE_get_by_subject call in
X509_STORE_CTX_get1_issuer *does* (though, at the time, it was inline
in X509_verify_cert. That code believes X509_STORE_get_by_subject
returns an X509_LU_* enum, but it doesn't work either! It believes
*ret is filled in on X509_LU_RETRY, thus freeing another uninitialized
pointer (GCC noticed this too).
Since this "retry" code has clearly never worked, from SSLeay onwards,
unwind it completely rather than attempt to fix it. No
X509_LOOKUP_METHOD can possibly have depended on it.
Matching all non-broken codepaths X509_LOOKUP_by_* now returns 0/1 and
X509_STORE_get_by_subject returns 0/1. X509_LU_* is purely a type enum
with X509_LU_{REJECT,FAIL} being legacy constants to keep old code
compiling. (Upstream is recommended to remove those values altogether
for 1.1.0.)
On the off chance any get_by_* X509_LOOKUP_METHOD implementations did
not return 0/1 (I have found no evidence anywhere of this, and I believe
it wouldn't have worked anyway), the X509_LOOKUP_by_* wrapper functions
will coerce the return values back to 0/1 before passing up to the
callers which want 0/1. This both avoids the error-prone -1/0/1 calling
convention and, more importantly, avoids problems with third-party
callers which expect a X509_LU_* return code. 0/1 collide with FAIL/X509
while -1 will collide with RETRY and might confuse things.
Change-Id: I98ecf6fa7342866b9124dc6f0b422cb9ce4a1ae7
Reviewed-on: https://boringssl-review.googlesource.com/8303
Reviewed-by: Adam Langley <agl@google.com>
