BN_FLG_CONSTTIME is a ridiculous API and easy to mess up
(CVE-2016-2178). Instead, code that needs a particular algorithm which
preserves secrecy of some arguemnt should call into that algorithm
directly.
This is never set outside the library and is finally unused within the
library! Credit for all this goes almost entirely to Brian Smith. I just
took care of the last bits.
Note there was one BN_FLG_CONSTTIME check that was still reachable, the
BN_mod_inverse in RSA key generation. However, it used the same code in
both cases for even moduli and φ(n) is even if n is not a power of two.
Traditionally, RSA keys are not powers of two, even though it would make
the modular reductions a lot easier.
When reviewing, check that I didn't remove a BN_FLG_CONSTTIME that led
to a BN_mod_exp(_mont) or BN_mod_inverse call (with the exception of the
RSA one mentioned above). They should all go to functions for the
algorithms themselves like BN_mod_exp_mont_consttime.
This CL shows the checks are a no-op for all our tests:
https://boringssl-review.googlesource.com/c/12927/
BUG=125
Change-Id: I19cbb375cc75aac202bd76b51ca098841d84f337
Reviewed-on: https://boringssl-review.googlesource.com/12926
Reviewed-by: Adam Langley <alangley@gmail.com>
Most C standard library functions are undefined if passed NULL, even
when the corresponding length is zero. This gives them (and, in turn,
all functions which call them) surprising behavior on empty arrays.
Some compilers will miscompile code due to this rule. See also
https://www.imperialviolet.org/2016/06/26/nonnull.html
Add OPENSSL_memcpy, etc., wrappers which avoid this problem.
BUG=23
Change-Id: I95f42b23e92945af0e681264fffaf578e7f8465e
Reviewed-on: https://boringssl-review.googlesource.com/12928
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Get one step closer to removing the dependency on |BN_div| from most
programs. Also get one step closer to a constant-time implementation of
|BN_MONT_CTX_set|; we now "just" need to create a constant-time variant
of |BN_mod_lshift1_quick|.
Note that this version might actually increase the side channel signal,
since the variance in timing in |BN_div| is probably less than the variance
from the many conditional reductions in the new method.
On one Windows x64 machine, the speed of RSA verification using the new
version is not too different from the speed of the old code. However,
|BN_div| is generally slow on Windows x64 so I expect this isn't faster
on all platforms. Regardless, we generally consider ECDSA/EdDSA
signature verification performance to be adaquate and RSA signature
verification is much, much faster even with this change.
For RSA signing the performance is not a significant factor since
performance-sensitive applications will cache the |RSA| structure and
the |RSA| structure will cache the Montgomery contexts.
Change-Id: Ib14f1a35c99b8da435e190342657f6a839381a1a
Reviewed-on: https://boringssl-review.googlesource.com/10520
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>
Windows SRWLOCK requires you call different functions here. Split
them up in preparation for switching Windows from CRITICAL_SECTION.
BUG=37
Change-Id: I7b5c6a98eab9ae5bb0734b805cfa1ff334918f35
Reviewed-on: https://boringssl-review.googlesource.com/8080
Reviewed-by: Adam Langley <agl@google.com>
This reduces the chance of double-frees.
BUG=10
Change-Id: I11a240e2ea5572effeddc05acb94db08c54a2e0b
Reviewed-on: https://boringssl-review.googlesource.com/7583
Reviewed-by: David Benjamin <davidben@google.com>
In the case |BN_CTX_get| failed, the function returned without calling
|BN_CTX_end|. Fix that.
Change-Id: Ia24cba3256e2cec106b539324e9679d690048780
Reviewed-on: https://boringssl-review.googlesource.com/7592
Reviewed-by: David Benjamin <davidben@google.com>
It looks like we started reformatting that function and adding curly braces,
etc., but forget to finish it. This is corroborated by the diff. Although git
thinks I removed the EAY-style one and tweaked the #if-0'd one, I actually
clang-formatted the EAY-style one anew and deleted the #if-0'd one after
tweaking the style to match. Only difference is the alignment stuff is
uintptr_t rather than intptr_t since the old logic was using unsigned
arithmetic.
Change-Id: Ia244e4082a6b6aed3ef587d392d171382c32db33
Reviewed-on: https://boringssl-review.googlesource.com/7574
Reviewed-by: David Benjamin <davidben@google.com>
Fix casts from const to non-const where dropping the constness is
completely unnecessary. The changes to chacha_vec.c don't result in any
changes to chacha_vec_arm.S.
Change-Id: I2f10081fd0e73ff5db746347c5971f263a5221a6
Reviewed-on: https://boringssl-review.googlesource.com/6923
Reviewed-by: David Benjamin <davidben@google.com>
The |ri| field was only used in |BN_MONT_CTX_set|, so make it a local
variable of that function.
Change-Id: Id8c3d44ac2e30e3961311a7b1a6731fe2c33a0eb
Reviewed-on: https://boringssl-review.googlesource.com/6526
Reviewed-by: Adam Langley <agl@google.com>
The file armv8-mont.pl is taken from upstream. The speed ups are fairly
modest (~30%) but seem worthwhile.
Before:
Did 231 RSA 2048 signing operations in 1008671us (229.0 ops/sec)
Did 11208 RSA 2048 verify operations in 1036997us (10808.1 ops/sec)
Did 342 RSA 2048 (3 prime, e=3) signing operations in 1021545us (334.8 ops/sec)
Did 32000 RSA 2048 (3 prime, e=3) verify operations in 1016162us (31491.0 ops/sec)
Did 45 RSA 4096 signing operations in 1039805us (43.3 ops/sec)
Did 3608 RSA 4096 verify operations in 1060283us (3402.9 ops/sec)
After:
Did 300 RSA 2048 signing operations in 1009772us (297.1 ops/sec)
Did 12740 RSA 2048 verify operations in 1075413us (11846.6 ops/sec)
Did 408 RSA 2048 (3 prime, e=3) signing operations in 1016139us (401.5 ops/sec)
Did 33000 RSA 2048 (3 prime, e=3) verify operations in 1017510us (32432.1 ops/sec)
Did 52 RSA 4096 signing operations in 1067678us (48.7 ops/sec)
Did 3408 RSA 4096 verify operations in 1062863us (3206.4 ops/sec)
Change-Id: Ife74fac784067fce3668b5c87f51d481732ff855
Reviewed-on: https://boringssl-review.googlesource.com/6444
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Most functions can take this in as const. Note this changes an
RSA_METHOD hook, though one I would not expect anyone to override.
Change-Id: Ib70ae65e5876b01169bdc594e465e3e3c4319a8b
Reviewed-on: https://boringssl-review.googlesource.com/6419
Reviewed-by: Adam Langley <agl@google.com>
One less exported function. Nothing ever stack-allocates them, within BoringSSL
or in consumers. This avoids the slightly odd mechanism where BN_MONT_CTX_free
might or might not free the BN_MONT_CTX itself based on a flag.
(This is also consistent with OpenSSL 1.1.x which does away with the _init
variants of both this and BIGNUM so it shouldn't be a compatibility concern
long-term either.)
Change-Id: Id885ae35a26f75686cc68a8aa971e2ea6767ba88
Reviewed-on: https://boringssl-review.googlesource.com/6350
Reviewed-by: Adam Langley <alangley@gmail.com>
The function BN_MONT_CTX_set was assuming that the modulus was non-zero
and therefore that |mod->top| > 0. In an error situation that may not be
the case and could cause a seg fault.
This is a follow on from CVE-2015-1794.
(Imported from upstream's 512368c9ed4d53fb230000e83071eb81bf628b22.)
The CVE itself doesn't affect us as the bit strength check in the DHE logic
excludes zero.
Also add tests to bn_test for a couple of division by zero cases. (This and
BN_div.)
Change-Id: Ibd8ef98d6be48eb95110021c23cd8e278656764d
Reviewed-on: https://boringssl-review.googlesource.com/5690
Reviewed-by: Adam Langley <agl@google.com>
This introduces a per-RSA/DSA/DH lock. This is good for lock contention,
although pthread locks are depressingly bloated.
Change-Id: I07c4d1606fc35135fc141ebe6ba904a28c8f8a0c
Reviewed-on: https://boringssl-review.googlesource.com/4324
Reviewed-by: Adam Langley <agl@google.com>
Including string.h in base.h causes any file that includes a BoringSSL
header to include string.h. Generally this wouldn't be a problem,
although string.h might slow down the compile if it wasn't otherwise
needed. However, it also causes problems for ipsec-tools in Android
because OpenSSL didn't have this behaviour.
This change removes string.h from base.h and, instead, adds it to each
.c file that requires it.
Change-Id: I5968e50b0e230fd3adf9b72dd2836e6f52d6fb37
Reviewed-on: https://boringssl-review.googlesource.com/3200
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
The lazy-initialisation of BN_MONT_CTX was serialising all threads, as noted by
Daniel Sands and co at Sandia. This was to handle the case that 2 or more
threads race to lazy-init the same context, but stunted all scalability in the
case where 2 or more threads are doing unrelated things! We favour the latter
case by punishing the former. The init work gets done by each thread that finds
the context to be uninitialised, and we then lock the "set" logic after that
work is done - the winning thread's work gets used, the losing threads throw
away what they've done.
(Imported from upstream's bf43446835bfd3f9abf1898a99ae20f2285320f3)
Initial fork from f2d678e6e89b6508147086610e985d4e8416e867 (1.0.2 beta).
(This change contains substantial changes from the original and
effectively starts a new history.)
