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>
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>
Call |RSA_check_key| after parsing an RSA private key in order to
verify that the key is consistent. This is consistent with ECC key
parsing, which does a similar key check.
Call |RSA_check_key| after key generation mostly as a way of
double-checking the key generation was done correctly. A similar check
was not added to |EC_KEY_generate| because |EC_KEY_generate| is used
for generating ephemeral ECDH keys, and the check would be too
expensive for that use.
Change-Id: I5759d0d101c00711bbc30f81a3759f8bff01427c
Reviewed-on: https://boringssl-review.googlesource.com/7522
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>
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|.
Change-Id: I78d1e55520a1c8c280cae2b7256e12ff6290507d
Reviewed-on: https://boringssl-review.googlesource.com/7582
Reviewed-by: David Benjamin <davidben@google.com>
Require the public exponent to be available unless
|RSA_FLAG_NO_BLINDING| is set on the key. Also, document this.
If the public exponent |e| is not available, then we could compute it
from |p|, |q|, and |d|. However, there's no reasonable situation in
which we'd have |p| or |q| but not |e|; either we have all the CRT
parameters, or we have (e, d, n), or we have only (d, n). The
calculation to compute |e| exposes the private key to risk of side
channel attacks.
Also, it was particularly wasteful to compute |e| for each
|BN_BLINDING| created, instead of just once before the first
|BN_BLINDING| was created.
|BN_BLINDING| now no longer needs to contain a duplicate copy of |e|,
so it is now more space-efficient.
Note that the condition |b->e != NULL| in |bn_blinding_update| was
always true since commit cbf56a5683.
Change-Id: Ic2fd6980e0d359dcd53772a7c31bdd0267e316b4
Reviewed-on: https://boringssl-review.googlesource.com/7594
Reviewed-by: David Benjamin <davidben@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>
When |rsa->e == NULL| we cannot verify the result, so using the CRT
would leave the key too vulnerable to fault attacks.
Change-Id: I154622cf6205ba4d5fb219143db6072a787c2d1f
Reviewed-on: https://boringssl-review.googlesource.com/7581
Reviewed-by: David Benjamin <davidben@google.com>
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>
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>
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>
Simplify the code by always caching Montgomery contexts in the RSA
structure, regardless of the |RSA_FLAG_CACHE_PUBLIC| and
|RSA_FLAG_CACHE_PRIVATE| flags. Deprecate those flags.
Now that we do this no more than once per key per RSA exponent, the
private key exponents better because the initialization of the
Montgomery contexts isn't perfectly side-channel protected.
Change-Id: I4fbcfec0f2f628930bfeb811285b0ae3d103ac5e
Reviewed-on: https://boringssl-review.googlesource.com/7521
Reviewed-by: David Benjamin <davidben@google.com>
As far as I can tell, this is the last place within libcrypto where
this type of check is missing.
Change-Id: I3d09676abab8c9f6c4e87214019a382ec2ba90ee
Reviewed-on: https://boringssl-review.googlesource.com/7519
Reviewed-by: David Benjamin <davidben@google.com>
This reverts commit ba70118d8e. Reverting this
did not resolve the regression and the cause is now known.
BUG=593963
Change-Id: Ic5e24b74e8f16b01d9fdd80f267a07ef026c82cf
Reviewed-on: https://boringssl-review.googlesource.com/7454
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This reverts commit b944882f26.
Recent Chrome canaries show a visible jump in ERR_SSL_PROTOCOL_ERROR which
coincided with a DEPS roll that included this change. Speculatively revert it
to see if they go back down afterwards.
Change-Id: I067798db144c348d666985986dfb9720d1153b7a
Reviewed-on: https://boringssl-review.googlesource.com/7391
Reviewed-by: David Benjamin <davidben@google.com>
This removes a hard dependency on |BN_mod_exp|, which will allow the
linker to drop it in programs that don't use other features that
require it.
Also, remove the |mont| member of |bn_blinding_st| in favor of having
callers pass it when necssaary. The |mont| member was a weak reference,
and weak references tend to be error-prone.
Finally, reduce the scope of some parts of the blinding code to
|static|.
Change-Id: I16d8ccc2d6d950c1bb40377988daf1a377a21fe6
Reviewed-on: https://boringssl-review.googlesource.com/7111
Reviewed-by: David Benjamin <davidben@google.com>
Reduce the maximum RSA exponent size to 33 bits, regardless of modulus
size, for public key operations.
Change-Id: I88502b1033d8854696841531031298e8ad96a467
Reviewed-on: https://boringssl-review.googlesource.com/6901
Reviewed-by: Adam Langley <agl@google.com>
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>
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>
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>
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>
We were getting this because of C's defaults, but it's fragile to leave
it like this because someone may add another field at the end in the
future.
Change-Id: I8b2dcbbc7cee8062915d15101f99f5a1aae6ad87
Reviewed-on: https://boringssl-review.googlesource.com/5860
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
RSA_PADDING_NONE is actually the important one for RSA_decrypt since OAEP isn't
used much and RSA_PKCS1_PADDING is unsafe to use due to timing constraints.
(The SSL stack uses RSA_PADDING_NONE and does the padding check separately.)
Change-Id: I5f9d168e7c34796a41bf01fc1878022742b63501
Reviewed-on: https://boringssl-review.googlesource.com/5641
Reviewed-by: Adam Langley <agl@google.com>
The documentation for |BN_CTX_get| states: "Once |BN_CTX_get| has
returned NULL, all future calls will also return NULL until
|BN_CTX_end| is called." Some code takes advantage of that guarantee
by only checking the return value of the last call to |BN_CTX_get| in a
series of calls. That is correct and the most efficient way of doing
it. However, that pattern is inconsistent with most of the other uses
of |BN_CTX_get|. Also, static analysis tools like Coverity cannot
understand that pattern. This commit removes the instances of that
pattern that Coverity complained about when scanning *ring*.
Change-Id: Ie36d0223ea1caee460c7979547cf5bfd5fb16f93
Reviewed-on: https://boringssl-review.googlesource.com/5611
Reviewed-by: Adam Langley <agl@google.com>
BN_copy can fail on malloc failure. The case in crypto/rsa was causing the
malloc tests in all_tests.go to infinite loop.
Change-Id: Id5900512013fba9960444d78a8c056aa4314fb2d
Reviewed-on: https://boringssl-review.googlesource.com/5110
Reviewed-by: Adam Langley <agl@google.com>
Finish up crypto, minus the legacy modules we haven't been touching much.
Change-Id: I0e9e1999a627aed5fb14841f8a2a7d0b68398e85
Reviewed-on: https://boringssl-review.googlesource.com/4517
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>
Beyond generally eliminating unnecessary includes, eliminate as many
includes of headers that declare/define particularly error-prone
functionality like strlen, malloc, and free. crypto/err/internal.h was
added to remove the dependency on openssl/thread.h from the public
openssl/err.h header. The include of <stdlib.h> in openssl/mem.h was
retained since it defines OPENSSL_malloc and friends as macros around
the stdlib.h functions. The public x509.h, x509v3.h, and ssl.h headers
were not changed in order to minimize breakage of source compatibility
with external code.
Change-Id: I0d264b73ad0a720587774430b2ab8f8275960329
Reviewed-on: https://boringssl-review.googlesource.com/4220
Reviewed-by: Adam Langley <agl@google.com>
Now that ERR is using thread-local storage, there's very little that the
THREADID code is doing and it can be turned into stub functions.
Change-Id: I668613fec39b26c894d029b10a8173c3055f6019
Some code, sadly, tests the error and the extra error is breaking it.
Change-Id: I89eabadf5d2c5f7dd761030da33dd4c3f2ac8382
Reviewed-on: https://boringssl-review.googlesource.com/3720
Reviewed-by: David Benjamin <davidben@chromium.org>
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>
This change extracts two, common parts of RSA_decrypt and RSA_sign into
a function called |private_transform|. It also allows this to be
overridden in a method, which is convenient for opaque keys that only
expose the raw RSA transform as it means that the padding code from
BoringSSL can be easily reimplemented.
One significant change here is that short RSA ciphertexts will no longer
be accepted. I think this is correct and OpenSSL has a comment about PGP
mistakenly stripping leading zeros. However, these is the possibility
that it could break something.
Change-Id: I258c5cbbf21314cc9b6e8d2a2b898fd9a440cd40
Reviewed-on: https://boringssl-review.googlesource.com/1554
Reviewed-by: Adam Langley <agl@google.com>
It's unused with SSLv2 gone. Also, being a decryption padding check, it really
should be constant-time and isn't.
Change-Id: I96be02cb50f9bf0229b9174eccd80fa338bf8e3e
Reviewed-on: https://boringssl-review.googlesource.com/1254
Reviewed-by: Adam Langley <agl@google.com>
This is to avoid having to copy over the RSA modulus in all of Chromium's
platform-specific keys.
Change-Id: I20bf22446a5cfb633b900c3b392b7a1da81a5431
Reviewed-on: https://boringssl-review.googlesource.com/1151
Reviewed-by: Adam Langley <agl@google.com>
This change adds a new function, BN_bn2bin_padded, that attempts, as
much as possible, to serialise a BIGNUM in constant time.
This is used to avoid some timing leaks in RSA decryption.
Initial fork from f2d678e6e89b6508147086610e985d4e8416e867 (1.0.2 beta).
(This change contains substantial changes from the original and
effectively starts a new history.)
