Commit Graph

26 Commits

Author SHA1 Message Date
David Benjamin
aaa39e97f4 Don't rely on BN_FLG_CONSTTIME in the DSA code.
DSA is deprecated, but get this aligned with some of the BN_FLG_CONSTTIME work
going on elsewhere.

Change-Id: I676ceab298a69362bef1b61d6f597c5c90da2ff0
Reviewed-on: https://boringssl-review.googlesource.com/8309
Reviewed-by: Adam Langley <agl@google.com>
2016-06-20 17:17:41 +00:00
David Benjamin
99c752ad52 Compute kinv in DSA with Fermat's Little Theorem.
It's a prime, so computing a constant-time mod inverse is straight-forward.

Change-Id: Ie09b84363c3d5da827989300a844c470437fd8f2
Reviewed-on: https://boringssl-review.googlesource.com/8308
Reviewed-by: Adam Langley <agl@google.com>
2016-06-20 17:16:18 +00:00
David Benjamin
26b7c35d8c Fix DSA, preserve BN_FLG_CONSTTIME
Operations in the DSA signing algorithm should run in constant time in
order to avoid side channel attacks. A flaw in the OpenSSL DSA
implementation means that a non-constant time codepath is followed for
certain operations. This has been demonstrated through a cache-timing
attack to be sufficient for an attacker to recover the private DSA key.

CVE-2016-2178

(Imported from upstream's 621eaf49a289bfac26d4cbcdb7396e796784c534 and
b7d0f2834e139a20560d64c73e2565e93715ce2b.)

We should eventually not depend on BN_FLG_CONSTTIME since it's a mess (seeing
as the original fix was wrong until we reported b7d0f2834e to them), but, for
now, go with the simplest fix.

Change-Id: I9ea15c1d1cc3a7e21ef5b591e1879ec97a179718
Reviewed-on: https://boringssl-review.googlesource.com/8172
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-07 19:29:18 +00:00
Brian Smith
d035730ac7 Make return value of |BN_MONT_CTX_set_locked| int.
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>
2016-04-18 23:19:08 +00:00
David Benjamin
2dc469e066 Remove dead header file.
There's nothing in here.

Change-Id: I3a501389e7e237b2e6907f27d2eb788a298d6c03
Reviewed-on: https://boringssl-review.googlesource.com/6877
Reviewed-by: Adam Langley <agl@google.com>
2016-02-17 01:34:15 +00:00
David Benjamin
df98a7ad3a Reimplement DSA_size without crypto/asn1.
BUG=499653

Change-Id: I16963fb198609d7fc0df6c57923cda3e13350753
Reviewed-on: https://boringssl-review.googlesource.com/6875
Reviewed-by: Adam Langley <agl@google.com>
2016-02-17 01:02:51 +00:00
David Benjamin
fda22a7573 Reimplement DSA parsing logic with crypto/asn1.
Functions which lose object reuse and need auditing:
- d2i_DSA_SIG
- d2i_DSAPublicKey
- d2i_DSAPrivateKey
- d2i_DSAparams

BUG=499653

Change-Id: I1cc2ae10e1e77eb57da3a858ac8734a95715ce4b
Reviewed-on: https://boringssl-review.googlesource.com/7022
Reviewed-by: Adam Langley <agl@google.com>
2016-02-17 00:26:01 +00:00
David Benjamin
3cadf63c68 Remove DSA write_params.
This imports upstream's ea6b07b54c1f8fc2275a121cdda071e2df7bd6c1 along
with a bugfix in 987157f6f63fa70dbeffca3c8bc62f26e9767ff2.

In an SPKI, a DSA key is only an INTEGER, with the group information in
the AlgorithmIdentifier. But a standalone DSAPublicKey is more complex
(and apparently made up by OpenSSL). OpenSSL implemented this with a
write_params boolean and making DSAPublicKey a CHOICE.

Instead, have p_dsa_asn1.c encode an INTEGER directly. d2i_DSAPublicKey
only parses the standalone form. (That code will be replaced later, but
first do this in preparation for rewriting the DSA ASN.1 code.)

Change-Id: I6fbe298d2723b9816806e9c196c724359b9ffd63
Reviewed-on: https://boringssl-review.googlesource.com/7021
Reviewed-by: Adam Langley <agl@google.com>
2016-02-16 23:54:38 +00:00
Brian Smith
5ba06897be Don't cast |OPENSSL_malloc|/|OPENSSL_realloc| result.
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.

Change-Id: I26988a672e8ed4d69c75cfbb284413999b475464
Reviewed-on: https://boringssl-review.googlesource.com/7102
Reviewed-by: David Benjamin <davidben@google.com>
2016-02-11 22:07:56 +00:00
David Benjamin
2936170d68 Fix memory leak in DSA redo case.
Found by clang scan-build.

Change-Id: I44a9d5ea165ede836c72aed8725d0bb0981b1004
Reviewed-on: https://boringssl-review.googlesource.com/6700
Reviewed-by: Adam Langley <agl@google.com>
2015-12-16 21:17:09 +00:00
David Benjamin
8a58933db0 Remove the CRYPTO_EX_new callback.
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>
2015-12-15 21:29:46 +00:00
David Benjamin
e8f783ac0d Unwind DH_METHOD and DSA_METHOD.
This will allow a static linker (with -ffunction-sections since things aren't
split into files) to drop unused parts of DH and DSA. Notably, the parameter
generation bits pull in primality-checking code.

Change-Id: I25087e4cb91bc9d0ab43bcb267c2e2c164e56b59
Reviewed-on: https://boringssl-review.googlesource.com/6388
Reviewed-by: Adam Langley <agl@google.com>
2015-11-03 22:54:36 +00:00
David Benjamin
79680ffaed Fix various malloc failure codepaths.
CRYPTO_MUTEX_init needs a CRYPTO_MUTEX_cleanup. Also a pile of problems
with x509_lu.c I noticed trying to import some upstream change.

Change-Id: I029a65cd2d30aa31f4832e8fbfe5b2ea0dbc66fe
Reviewed-on: https://boringssl-review.googlesource.com/6346
Reviewed-by: Adam Langley <alangley@gmail.com>
2015-10-26 19:41:01 +00:00
David Benjamin
3570d73bf1 Remove the func parameter to OPENSSL_PUT_ERROR.
Much of this was done automatically with
  find . -name '*.c' | xargs sed -E -i '' -e 's/(OPENSSL_PUT_ERROR\([a-zA-Z_0-9]+, )[a-zA-Z_0-9]+, ([a-zA-Z_0-9]+\);)/\1\2/'
  find . -name '*.c' | xargs sed -E -i '' -e 's/(OPENSSL_PUT_ERROR\([a-zA-Z_0-9]+, )[a-zA-Z_0-9]+,  ([a-zA-Z_0-9]+\);)/\1\2/'

BUG=468039

Change-Id: I4c75fd95dff85ab1d4a546b05e6aed1aeeb499d8
Reviewed-on: https://boringssl-review.googlesource.com/5276
Reviewed-by: Adam Langley <agl@google.com>
2015-07-16 02:02:37 +00:00
Adam Langley
0da323a8b8 Convert reference counts in crypto/
This change converts the reference counts in crypto/ to use
|CRYPTO_refcount_t|. The reference counts in |X509_PKEY| and |X509_INFO|
were never actually used and so were dropped.

Change-Id: I75d572cdac1f8c1083c482e29c9519282d7fd16c
Reviewed-on: https://boringssl-review.googlesource.com/4772
Reviewed-by: Adam Langley <agl@google.com>
2015-05-20 19:15:26 +00:00
David Benjamin
22ccc2d8f1 Remove unnecessary NULL checks, part 1.
First batch of the alphabet.

Change-Id: If4e60f4fbb69e04eb4b70aa1b2240e329251bfa5
Reviewed-on: https://boringssl-review.googlesource.com/4514
Reviewed-by: Adam Langley <agl@google.com>
2015-05-04 23:05:17 +00:00
David Benjamin
9f33fc63c6 Remove hash table lookups from ex_data.
Instead, each module defines a static CRYPTO_EX_DATA_CLASS to hold the values.
This makes CRYPTO_cleanup_all_ex_data a no-op as spreading the
CRYPTO_EX_DATA_CLASSes across modules (and across crypto and ssl) makes cleanup
slightly trickier. We can make it do something if needbe, but it's probably not
worth the trouble.

Change-Id: Ib6f6fd39a51d8ba88649f0fa29c66db540610c76
Reviewed-on: https://boringssl-review.googlesource.com/4375
Reviewed-by: Adam Langley <agl@google.com>
2015-04-15 23:59:35 +00:00
Adam Langley
683d7bd20a Convert BN_MONT_CTX to new-style locking.
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>
2015-04-14 20:10:27 +00:00
Brian Smith
054e682675 Eliminate unnecessary includes from low-level crypto modules.
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>
2015-04-13 20:49:18 +00:00
David Benjamin
c9a202fee3 Add in missing curly braces part 1.
Everything before crypto/ec.

Change-Id: Icbfab8e4ffe5cc56bf465eb57d3fdad3959a085c
Reviewed-on: https://boringssl-review.googlesource.com/3401
Reviewed-by: Adam Langley <agl@google.com>
2015-02-11 19:31:01 +00:00
Adam Langley
2b2d66d409 Remove string.h from base.h.
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>
2015-02-02 19:14:15 +00:00
David Benjamin
95e18c52f2 Fix DER checks for DSA_check_signature and add tests.
DSA_verify and DSA_check_signature didn't share a codepath, so the fix was only
applied to the former. Implement verify in terms of check_signature and add
tests for bad DER variants.

Change-Id: I6577f96b13b57fc89a5308bd8a7c2318defa7ee1
Reviewed-on: https://boringssl-review.googlesource.com/2820
Reviewed-by: Adam Langley <agl@google.com>
2015-01-12 22:29:10 +00:00
Adam Langley
ca9a538aa0 Fix various certificate fingerprint issues.
By using non-DER or invalid encodings outside the signed portion of a
certificate the fingerprint can be changed without breaking the signature.
Although no details of the signed portion of the certificate can be changed
this can cause problems with some applications: e.g. those using the
certificate fingerprint for blacklists.

1. Reject signatures with non zero unused bits.

If the BIT STRING containing the signature has non zero unused bits reject the
signature. All current signature algorithms require zero unused bits.

2. Check certificate algorithm consistency.

Check the AlgorithmIdentifier inside TBS matches the one in the certificate
signature. NB: this will result in signature failure errors for some broken
certificates.

3. Check DSA/ECDSA signatures use DER.

Reencode DSA/ECDSA signatures and compare with the original received signature.
Return an error if there is a mismatch.

This will reject various cases including garbage after signature (thanks to
Antti Karjalainen and Tuomo Untinen from the Codenomicon CROSS program for
discovering this case) and use of BER or invalid ASN.1 INTEGERs (negative or
with leading zeroes).

CVE-2014-8275

(Imported from upstream's 85cfc188c06bd046420ae70dd6e302f9efe022a9 and
4c52816d35681c0533c25fdd3abb4b7c6962302d)

Change-Id: Ic901aea8ea6457df27dc542a11c30464561e322b
Reviewed-on: https://boringssl-review.googlesource.com/2783
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
2015-01-09 19:41:59 +00:00
Adam Langley
bed8ce78f0 Add misc functions for easier porting.
Android requested that the wpa_supplicant go upstream. This change adds
some dummy functions and reinstates DSA_dup_DH in order to make the diff
smaller and easier for upstream.

Change-Id: I77ac271b8652bae5a0bbe16afde51d9096f3dfb5
Reviewed-on: https://boringssl-review.googlesource.com/1740
Reviewed-by: Adam Langley <agl@google.com>
2014-09-18 22:38:11 +00:00
Adam Langley
d4b4f085d9 Safe (EC)DSA nonces.
This change causes (EC)DSA nonces be to calculated by hashing the
message and private key along with entropy.
2014-06-20 13:17:33 -07:00
Adam Langley
95c29f3cd1 Inital import.
Initial fork from f2d678e6e89b6508147086610e985d4e8416e867 (1.0.2 beta).

(This change contains substantial changes from the original and
effectively starts a new history.)
2014-06-20 13:17:32 -07:00