Instead, just fail if `rsa->e` is `NULL`.
I agree to license my contributions to each file under the
same terms given at the top of each file I changed.
C has implicit conversion of |void *| to other pointer types so these
casts are unnecessary. Clean them up to make the code easier to read
and to make it easier to find dangerous casts.
The |_ex| versions of these functions are unnecessary because when they
are used, they are always passed |NULL| for |r|, which is what the
non-|_ex| versions do. Just use the non-|_ex| versions instead and
remove the |_ex| versions.
Also, drop the unused flags mechanism.
Change-Id: Ida4cb5a2d4c89d9cd318e06f71867aea98408d0d
Reviewed-on: https://boringssl-review.googlesource.com/7110
Reviewed-by: David Benjamin <davidben@google.com>
Ultimately, it's better to invest effort in alternative forms of
protection of key material.
Calling `OPENSSL_cleanse` with a NULL pointer is not safe, but
`OPENSSL_cleanse` is often called in cleanup code, especially error-
handling code, where it is difficult to keep track of the NULLness of
things. The likelihood of getting this wrong is compounded by the fact
that, in OpenSSL upstream, calling `OPENSSL_cleanse(NULL, x)` for any
`x` is safe (a no-op). BoringSSL upstream doesn't want to change its
`OPENSSL_cleanse` to work like OpenSSL's. We don't want to worry about
the issue.
Apart from that, by inspection, it is clear that there are many places
in the code that don't call `OPENSSL_clease` where they "should". It
would be difficult to find all the places where a call to
`OPENSSL_clease` "should" be inserted. It is unlikely we'll ever get it
right. Actually, it's basically impossible to get it right using this
coding pattern. See
http://www.daemonology.net/blog/2014-09-06-zeroing-buffers-is-insufficient.html
and https://github.com/bitcoin/secp256k1/issues/185.
Besides all that, the zeroization isn't free. Especially in the case of
non-MSVC platforms, it either interferes with the optimizer or it
doesn't work. More importantly, thinking about how to make this
approach work wastes a lot of time that could be spent actually
improving the fundementals of the security of the code.
The changes to the tests were mostly undone since *ring* doesn't have
the "buggy" variants of the functions. BN_bn2cbb_padded was restored so
that the *ring* code would be easier to compare to the BoringSSL code.
There's many ways to serialize a BIGNUM, so not including asn1 in the name is
confusing (and collides with BN_bn2cbb_padded). Since BN_asn12bn looks
ridiculous, match the parse/marshal naming scheme of other modules instead.
Change-Id: I53d22ae0537a98e223ed943e943c48cb0743cf51
Reviewed-on: https://boringssl-review.googlesource.com/6822
Reviewed-by: Adam Langley <alangley@gmail.com>
After its initial assignment, |e| is immediately reassigned another
value and so the initial assignment from |BN_CTX_get| is useless. If
that were not the case, then the |BN_free(e)| at the end of the
function would be very bad.
Change-Id: Id63a172073501c8ac157db9188a22f55ee36b205
Reviewed-on: https://boringssl-review.googlesource.com/6951
Reviewed-by: David Benjamin <davidben@google.com>
|RSA_generate| creates the |RSA| struct for the key, instead of taking
it as an input like |RSA_generate_key_ex| does. This is a step towards
making |RSA| instances immutable.
Also, its exponent parameter |e| is a |uint32_t| instead of a |BIGNUM|
in order to help avoid the generation of keys that won't be accepted
by the Windows CryptoAPI.
|RSA_new_engine| only supported the case where |ENGINE| is NULL, for
backward compatibility, but now *ring* is not doing backward
compatibility in that way.
rsa_default_encrypt allowed an RSA modulus 8 times larger than the
intended maximum size due to bits vs. bytes confusion.
Further, as |rsa_default_encrypt| got this wrong while
|rsa_default_verify_raw| got it right, factor out the duplicated logic
so that such inconsistencies are less likely to occur.
BUG=576856
Change-Id: Ic842fadcbb3b140d2ba4295793457af2b62d9444
Reviewed-on: https://boringssl-review.googlesource.com/6900
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
|BN_mod_exp| will always choose its |BN_mod_exp_mont| code path anyway.
This will allow us to move |BN_mod_exp| out of libcrypto.
Don't use CACHE_PUBLIC flag to determine how to do blinding.
fix.
Originally *ring* did not merge any portion of BoringSSL 231cb82 as
*ring* does not need to support badly-formed Estonian smart card public
keys. However, instead of ignoring the entire commit, we should take
the parts that verify that |BN_cbs2unsigned| rejects them. Also, it is
a good idea to take the whitespace changes in bn_asn1.c so that it is
easier to compare *ring* and BoringSSL.
This check was fixed a while ago, but it could have been much simpler.
In the RSA key exchange, the expected size of the output is known, making the
padding check much simpler. There isn't any use in exporting the more general
RSA_message_index_PKCS1_type_2. (Without knowing the expected size, any
integrity check or swap to randomness or other mitigation is basically doomed
to fail.)
Verified with the valgrind uninitialized memory trick that we're still
constant-time.
Also update rsa.h to recommend against using the PKCS#1 v1.5 schemes.
Thanks to Ryan Sleevi for the suggestion.
Change-Id: I4328076b1d2e5e06617dd8907cdaa702635c2651
Reviewed-on: https://boringssl-review.googlesource.com/6613
Reviewed-by: Adam Langley <agl@google.com>
We should reject RSA public keys with exponents of less than 3.
This change also rejects even exponents, although the usefulness
of such a public key is somewhat questionable.
BUG=chromium:541257
Change-Id: I1499e9762ba40a7cf69155d21d55bc210cd6d273
Reviewed-on: https://boringssl-review.googlesource.com/6710
Reviewed-by: Adam Langley <agl@google.com>
This callback is never used. The one caller I've ever seen is in Android
code which isn't built with BoringSSL and it was a no-op.
It also doesn't actually make much sense. A callback cannot reasonably
assume that it sees every, say, SSL_CTX created because the index may be
registered after the first SSL_CTX is created. Nor is there any point in
an EX_DATA consumer in one file knowing about an SSL_CTX created in
completely unrelated code.
Replace all the pointers with a typedef to int*. This will ensure code
which passes NULL or 0 continues to compile while breaking code which
passes an actual function.
This simplifies some object creation functions which now needn't worry
about CRYPTO_new_ex_data failing. (Also avoids bouncing on the lock, but
it's taking a read lock, so this doesn't really matter.)
BUG=391192
Change-Id: I02893883c6fa8693682075b7b130aa538a0a1437
Reviewed-on: https://boringssl-review.googlesource.com/6625
Reviewed-by: Adam Langley <agl@google.com>
Remove the custom copy of those helpers.
Change-Id: I810c3ae8dbf7bc0654d3e9fb9900c425d36f64aa
Reviewed-on: https://boringssl-review.googlesource.com/6611
Reviewed-by: Adam Langley <agl@google.com>
* Rename |ring::rsa| to |ring::signature| in preparation for moving all
signature algorithms to the module.
* Make min/max key size explicit in the interface, similar to how ECDSA
fixes key sizes.
* Use the Input/Reader interface.
The crypto-bench results did not change in any significant way;
the difference is consistently mixed +/-. However, the code size
difference should be significant. Previously, every digest algorithm
implemented its own |init|, |update|, and |finish| functions. Now,
all of them share one implementation of each.
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>
Although those are only created by code owned by RSA_METHOD, custom RSA_METHODs
shouldn't be allowed to squat our internal fields and then change how you free
things.
Remove 'method' from their names now that they're not method-specific.
Change-Id: I9494ef9a7754ad59ac9fba7fd463b3336d826e0b
Reviewed-on: https://boringssl-review.googlesource.com/6423
Reviewed-by: Adam Langley <agl@google.com>
This restores the original semantics of the finished hook.
Change-Id: I70da393c7e66fb6e3be1e2511e08b34bb54fc0b4
Reviewed-on: https://boringssl-review.googlesource.com/6422
Reviewed-by: Adam Langley <agl@google.com>
Having a single RSA_METHOD means they all get pulled in. Notably, RSA key
generation pulls in the primality-checking code.
Change-Id: Iece480113754da090ddf87b64d8769f01e05d26c
Reviewed-on: https://boringssl-review.googlesource.com/6389
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I799e289a402612446e08f64f59e0243f164cf695
Reviewed-on: https://boringssl-review.googlesource.com/6372
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
