Line-based code coverage tools often don't handle the `||` operator
in conditions smartly. Separating out each condition to test for helps
them report more useful results.
This worked because `OPENSSL_PUT_ERROR` is defined as an empty macro,
but if `OPENSSL_PUT_ERROR` is redefined to a non-empty value then the
result is a syntax error.
Signing with RSA isn't exposed in the API yet, but when it is, the way
`BN_BLINDING`s are managed will be different than what BoringSSL does.
Doing this now lets us remove `CRYPTO_MUTEX` completely.
Avoid all the messiness involved with static initializers by doing it
the dumb way. Static initializers are commonly banned from applications
because they add latency to application startup.
This implementation needs to be fine-tuned to account for cases where
we don't need CPUID info (e.g. hard-coded ARM target profiles, NO_ASM
builds, platforms where we have no ASM code).
Also, it is worth investigating whether the Rust implementation of
`call_once` has optimal performance, but for now we punt on that issue.
Stop using `CRYPTO_once`. Instead, require that the CSPRNG is
explicitly threaded through every function that requires a CSPRNG. The
constructor of `SystemRandom` will then initialize the file handle for
/dev/urandom as necessary.
Use the common pattern of returning early instead of |goto err;| when
there's no cleanup to do yet. Also, move the error checking of
|BN_CTX_get| failure closer to the the calls to |BN_CTX_get|. Avoid
calling |OPENSSL_cleanse| on public data. Clarify when/why |buf| is not
freed.
Change-Id: I9df833db7eb7041c5af9349c461297372b988f98
Reviewed-on: https://boringssl-review.googlesource.com/7464
Reviewed-by: David Benjamin <davidben@google.com>
The same check is already done in |RSA_verify_raw|, so |RSA_verify|
doesn't need to do it.
Also, move the |RSA_verify_raw| check earlier.
Change-Id: I15f7db0aad386c0f764bba53e77dfc46574f7635
Reviewed-on: https://boringssl-review.googlesource.com/7463
Reviewed-by: David Benjamin <davidben@google.com>
We do not need to support engine-provided verification methods.
Change-Id: Iaad8369d403082b728c831167cc386fdcabfb067
Reviewed-on: https://boringssl-review.googlesource.com/7311
Reviewed-by: David Benjamin <davidben@google.com>
Instead of building the test suites inherited from BoringSSL as
seperate executables, link them all together into one executable,
giving all their `main` functions unique names.
This allows all the tests to be run, even on platforms that don't have
traditional process spawning, and avoids the need to keep track of
directory names even on platforms that do support process spawning.
This also makes it easier to integrate new BoringSSL test suites on
Windows, because we don't need to create a new `vcxproj` file for each
one.
Having one test executable may also make code coverage easier.
The Montgomery math functions and |BN_mod_sub_quick| are
constant-time-ish and can eventually be made constant-time. The
Montgomery math should also be faster.
A lot of consumers of obj.h only want the NID values. Others didn't need
it at all. This also removes some OBJ_nid2sn and OBJ_nid2ln calls in EVP
error paths which isn't worth pulling a large table in for.
BUG=chromium:499653
Change-Id: Id6dff578f993012e35b740a13b8e4f9c2edc0744
Reviewed-on: https://boringssl-review.googlesource.com/7563
Reviewed-by: David Benjamin <davidben@google.com>
Previously, the verification was only done when using the CRT method,
as the CRT method has been shown to be extremely sensitive to fault
attacks. However, there's no reason to avoid doing the verification
when the non-CRT method is used (performance-sensitive applications
should always be using the CRT-capable keys).
Previously, when we detected a fault (attack) through this verification,
libcrypto would fall back to the non-CRT method and assume that the
non-CRT method would give a correct result, despite having just
detecting corruption that is likely from an attack. Instead, just give
up, like NSS does.
Previously, the code tried to handle the case where the input was not
reduced mod rsa->n. This is (was) not possible, so avoid trying to
handle that. This simplifies the equality check and lets us use
|CRYPTO_memcmp|.
Drop support for keys that don't have a complete set of CRT parameters.
In the future we'll probably resurrect `RSA_recovert_crt_params` to
support things like WebCrypto that need to support keys that have only
(n, e, d).
All of the key parameters are always NULL in an `RSA` returned by
rsa_new_begin so don't bother checking if they are NULL. Also, move the
construction of each parameter closer to the point at which it is
initialized.
Instead of using `BN_with_flags` (which relies on an extra allocated
`BIGNUM`), we'll make sure that all `BIGNUM`s containing secret values
have the `BN_FLG_CONSTTIME` flag assigned when they are created.
I agree to license my contributions to each file under the
same terms given at the top of each file I changed.
The documentation in |RSA_METHOD| says that the |ctx| parameter to
|mod_exp| can be NULL, however the default implementation doesn't
handle that case. That wouldn't matter since internally it is always
called with a non-NULL |ctx| and it is static, but an external
application could get a pointer to |mod_exp| by extracting it from
the default |RSA_METHOD|. That's unlikely, but making that impossible
reduces the chances that future refactorings will cause unexpected
trouble.
Change-Id: Ie0e35e9f107551a16b49c1eb91d0d3386604e594
Reviewed-on: https://boringssl-review.googlesource.com/7580
Reviewed-by: David Benjamin <davidben@google.com>
|BN_mod_mul_montgomery| has better constant-time behavior (usually)
than |BN_mod_mul| and |BN_mod_sqr| and on platforms where we have
assembly language optimizations (when |OPENSSL_BN_ASM_MONT| is set in
crypto/bn/montgomery.c) it is faster. While doing so, reorder and
rename the |BN_MONT_CTX| parameters of the blinding functions to match
the order normally used in Montgomery math functions.
As a bonus, remove a redundant copy of the RSA public modulus from the
|BN_BLINDING| structure, which reduces memory usage.
Change-Id: I70597e40246429c7964947a1dc46d0d81c7530ef
Reviewed-on: https://boringssl-review.googlesource.com/7524
Reviewed-by: David Benjamin <davidben@google.com>
Change-Id: I0effe99d244c4ccdbb0e34db6e01a59c9463cb15
Reviewed-on: https://boringssl-review.googlesource.com/7572
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Conscrypt, thanks to Java's RSAPrivateKeySpec API, must be able to use RSA keys
with only modulus and exponent. This is kind of silly and breaks the blinding
code so they, both in OpenSSL and BoringSSL, had to explicitly turn blinding
off.
Add a test for this as we're otherwise sure to break it on accident.
We may wish to avoid the silly rsa->flags modification, I'm not sure. For now,
keep the requirement in so other consumers do not accidentally rely on this.
(Also add a few missing ERR_clear_error calls. Functions which are expected to
fail should be followed by an ERR_clear_error so later unexpected failures
don't get confused.)
BUG=boringssl:12
Change-Id: I674349821f1f59292b8edd085f21dc37e8bcaa75
Reviewed-on: https://boringssl-review.googlesource.com/7560
Reviewed-by: David Benjamin <davidben@google.com>
In |bn_blinding_update| the condition |b->e != NULL| would never be
true (probably), but the test made reasoning about the correctness of
the code confusing. That confusion was amplified by the circuitous and
unusual way in which |BN_BLINDING|s are constructed. Clarify all this
by simplifying the construction of |BN_BLINDING|s, making it more like
the construction of other structures.
Also, make counter unsigned as it is no longer ever negative.
Change-Id: I6161dcfeae19a80c780ccc6762314079fca1088b
Reviewed-on: https://boringssl-review.googlesource.com/7530
Reviewed-by: David Benjamin <davidben@google.com>
