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>
Instead, use BN_mod_exp_mont_consttime of p - 2. This removes two more
call sites sensitive to BN_FLG_CONSTTIME. We're down to just that last
BN_mod_inverse modulo φ(n). (Sort of. It's actually not sensitive
because even mod inverses always hit the other codepath. Perhaps we
should just leave it alone.)
Note this comes with a slight behavior change. The BN_MONT_CTXs are
initialized a little earlier. If a caller calls RSA_generate_* and then
reaches into the struct to scrap all the fields on it, they'll get
confused. Before, they had to perform an operation on it to get
confused. This is a completely ridiculous thing to do.
Since we do this a lot, this introduces some convenience functions for
doing the Fermat's Little Theorem mod inverse and fixes a leak in the
DSA code should computing kinv hit a malloc error.
BUG=125
Change-Id: Iafcae2fc6fd379d161f015c90ff7050e2282e905
Reviewed-on: https://boringssl-review.googlesource.com/12925
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 code, and in particular make |BN_div|, |BN_mod|, and
|BN_nnmod| insensitive to |BN_FLG_CONSTTIME|. This improves the
effectiveness of testing by reducing the number of branches that are
likely to go untested or less tested.
There is no performance-sensitive code that uses BN_div but doesn't
already use BN_FLG_CONSTTIME except RSA signature verification and
EC_GROUP creation. RSA signature verification, ECDH, and ECDSA
performance aren't significantly different with this change.
Change-Id: Ie34c4ce925b939150529400cc60e1f414c7676cd
Reviewed-on: https://boringssl-review.googlesource.com/9105
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
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>
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>
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>
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>
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>
The comment in |BN_mod_inverse_ex| makes it clear that |BN_BITS2| was
intended. Besides fixing the code to match the comment, remove
the now-unused |BN_BITS| and the already-unused |BN_MASK| to prevent
future confusion of this sort.
On MSVC builds there seems to be very little difference in performance
between the two code paths according to |bssl speed|.
Change-Id: I765b7b3d464e2057b1d7952af25b6deb2724976a
Reviewed-on: https://boringssl-review.googlesource.com/6525
Reviewed-by: Adam Langley <agl@google.com>
History has shown there are bugs in not setting the error code
appropriately, which makes any decision making based on
|ERR_peek_last_error|, etc. suspect. Also, this call was interfering
with the link-time optimizer's ability to discard the implementations of
many functions in crypto/err during dead code elimination.
Change-Id: Iba9e553bf0a72a1370ceb17ff275f5a20fca31ec
Reviewed-on: https://boringssl-review.googlesource.com/5748
Reviewed-by: Adam Langley <agl@google.com>
Found while diagnosing some crashes and hangs in the malloc tests. This (and
the follow-up) get us further but does not quite let the malloc tests pass
quietly, even without valgrind. DTLS silently ignores some malloc failures
(confusion with silently dropping bad packets) which then translate to hangs.
Change-Id: Ief06a671e0973d09d2883432b89a86259e346653
Reviewed-on: https://boringssl-review.googlesource.com/3482
Reviewed-by: Adam Langley <agl@google.com>
Initial fork from f2d678e6e89b6508147086610e985d4e8416e867 (1.0.2 beta).
(This change contains substantial changes from the original and
effectively starts a new history.)
