Make DH opaque. In doing so, remove some X9.42 placeholder fields, since it's impossible to set them. I switched dh_test.cc to the getters where it was easy, but OpenSSL's new setters are so tedious that I just gave it access to the internal struct. With this, there are now only two public structs (DSA and RSA) that reference CRYPTO_MUTEX. After that's removed, we can stop worrying about pthread_rwlock_t feature flags in the public headers. Update-Note: DH is now an opaque structure. Callers should use accessors instead of accessing fields. Change-Id: Ia53702f8ab58884a90d85718ee26eb03d062d234 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54625 Reviewed-by: Bob Beck <bbe@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/dh_extra/dh_asn1.c b/crypto/dh_extra/dh_asn1.c index 9d32180..de01077 100644 --- a/crypto/dh_extra/dh_asn1.c +++ b/crypto/dh_extra/dh_asn1.c
@@ -63,6 +63,7 @@ #include <openssl/err.h> #include "../bytestring/internal.h" +#include "../fipsmodule/dh/internal.h" static int parse_integer(CBS *cbs, BIGNUM **out) {
diff --git a/crypto/dh_extra/dh_test.cc b/crypto/dh_extra/dh_test.cc index 7933a8c..b88702b 100644 --- a/crypto/dh_extra/dh_test.cc +++ b/crypto/dh_extra/dh_test.cc
@@ -70,6 +70,7 @@ #include <openssl/err.h> #include <openssl/mem.h> +#include "../fipsmodule/dh/internal.h" #include "../internal.h" #include "../test/test_util.h" @@ -134,42 +135,36 @@ } printf("\np = "); - BN_print_fp(stdout, a->p); + BN_print_fp(stdout, DH_get0_p(a.get())); printf("\ng = "); - BN_print_fp(stdout, a->g); + BN_print_fp(stdout, DH_get0_g(a.get())); printf("\n"); - bssl::UniquePtr<DH> b(DH_new()); + bssl::UniquePtr<DH> b(DHparams_dup(a.get())); if (!b) { return false; } - b->p = BN_dup(a->p); - b->g = BN_dup(a->g); - if (b->p == nullptr || b->g == nullptr) { - return false; - } - if (!DH_generate_key(a.get())) { return false; } printf("pri1 = "); - BN_print_fp(stdout, a->priv_key); + BN_print_fp(stdout, DH_get0_priv_key(a.get())); printf("\npub1 = "); - BN_print_fp(stdout, a->pub_key); + BN_print_fp(stdout, DH_get0_pub_key(a.get())); printf("\n"); if (!DH_generate_key(b.get())) { return false; } printf("pri2 = "); - BN_print_fp(stdout, b->priv_key); + BN_print_fp(stdout, DH_get0_priv_key(b.get())); printf("\npub2 = "); - BN_print_fp(stdout, b->pub_key); + BN_print_fp(stdout, DH_get0_pub_key(b.get())); printf("\n"); std::vector<uint8_t> key1(DH_size(a.get())); - int ret = DH_compute_key(key1.data(), b->pub_key, a.get()); + int ret = DH_compute_key(key1.data(), DH_get0_pub_key(b.get()), a.get()); if (ret < 0) { return false; } @@ -182,7 +177,7 @@ printf("\n"); std::vector<uint8_t> key2(DH_size(b.get())); - ret = DH_compute_key(key2.data(), a->pub_key, b.get()); + ret = DH_compute_key(key2.data(), DH_get0_pub_key(a.get()), b.get()); if (ret < 0) { return false; } @@ -343,9 +338,9 @@ bssl::UniquePtr<DH> dh(DH_parse_parameters(&cbs)); if (!dh || CBS_len(&cbs) != 0 || !BIGNUMEqualsHex( - dh->p, + DH_get0_p(dh.get()), "d72034a3274fdfbf04fd246825b656d8ab2a412d740a52087c40714ed2579313") || - !BIGNUMEqualsHex(dh->g, "2") || dh->priv_length != 0) { + !BIGNUMEqualsHex(DH_get0_g(dh.get()), "2") || dh->priv_length != 0) { return false; } @@ -383,11 +378,11 @@ CBS_init(&cbs, kParamsDSA, sizeof(kParamsDSA)); dh.reset(DH_parse_parameters(&cbs)); if (!dh || CBS_len(&cbs) != 0 || - !BIGNUMEqualsHex(dh->p, + !BIGNUMEqualsHex(DH_get0_p(dh.get()), "93f3c11801e662b6d1469a2c72ea31d91810302863e2347d80caee8" "22b193c19bb42830270dddb8c03abe99cc4004d705f5203312ca467" "3451952aac11e26a55") || - !BIGNUMEqualsHex(dh->g, + !BIGNUMEqualsHex(DH_get0_g(dh.get()), "44c8105344323163d8d18c75c898533b5b4a2a0a09e7d03c5372a86" "b70419c267144fc7f0875e102ab7441e82a3d3c263309e48bb441ec" "a6a8ba1a078a77f55f") ||
diff --git a/crypto/dh_extra/params.c b/crypto/dh_extra/params.c index f50786c..0e76747 100644 --- a/crypto/dh_extra/params.c +++ b/crypto/dh_extra/params.c
@@ -57,6 +57,7 @@ #include <openssl/mem.h> #include "../fipsmodule/bn/internal.h" +#include "../fipsmodule/dh/internal.h" static BIGNUM *get_params(BIGNUM *ret, const BN_ULONG *words, size_t num_words) { @@ -452,23 +453,10 @@ return 1; } - if (!int_dh_bn_cpy(&to->q, from->q) || - !int_dh_bn_cpy(&to->j, from->j)) { + if (!int_dh_bn_cpy(&to->q, from->q)) { return 0; } - OPENSSL_free(to->seed); - to->seed = NULL; - to->seedlen = 0; - - if (from->seed) { - to->seed = OPENSSL_memdup(from->seed, from->seedlen); - if (!to->seed) { - return 0; - } - to->seedlen = from->seedlen; - } - return 1; }
diff --git a/crypto/dsa/dsa.c b/crypto/dsa/dsa.c index b8e4653..f1fc02f 100644 --- a/crypto/dsa/dsa.c +++ b/crypto/dsa/dsa.c
@@ -74,6 +74,7 @@ #include "internal.h" #include "../fipsmodule/bn/internal.h" +#include "../fipsmodule/dh/internal.h" #include "../internal.h"
diff --git a/crypto/fipsmodule/dh/check.c b/crypto/fipsmodule/dh/check.c index 5b6e03a..0c82c17 100644 --- a/crypto/fipsmodule/dh/check.c +++ b/crypto/fipsmodule/dh/check.c
@@ -58,6 +58,8 @@ #include <openssl/bn.h> +#include "internal.h" + int DH_check_pub_key(const DH *dh, const BIGNUM *pub_key, int *out_flags) { *out_flags = 0; @@ -165,9 +167,6 @@ if (!BN_is_one(t2)) { *out_flags |= DH_CHECK_INVALID_Q_VALUE; } - if (dh->j && BN_cmp(dh->j, t1)) { - *out_flags |= DH_CHECK_INVALID_J_VALUE; - } } else if (BN_is_word(dh->g, DH_GENERATOR_2)) { l = BN_mod_word(dh->p, 24); if (l == (BN_ULONG)-1) {
diff --git a/crypto/fipsmodule/dh/dh.c b/crypto/fipsmodule/dh/dh.c index 1a3c974..11dbfc2 100644 --- a/crypto/fipsmodule/dh/dh.c +++ b/crypto/fipsmodule/dh/dh.c
@@ -101,9 +101,6 @@ BN_clear_free(dh->p); BN_clear_free(dh->g); BN_clear_free(dh->q); - BN_clear_free(dh->j); - OPENSSL_free(dh->seed); - BN_clear_free(dh->counter); BN_clear_free(dh->pub_key); BN_clear_free(dh->priv_key); CRYPTO_MUTEX_cleanup(&dh->method_mont_p_lock);
diff --git a/crypto/fipsmodule/dh/internal.h b/crypto/fipsmodule/dh/internal.h index c40172d..fb525d4 100644 --- a/crypto/fipsmodule/dh/internal.h +++ b/crypto/fipsmodule/dh/internal.h
@@ -17,11 +17,31 @@ #include <openssl/base.h> +#include <openssl/thread.h> + #if defined(__cplusplus) extern "C" { #endif +struct dh_st { + BIGNUM *p; + BIGNUM *g; + BIGNUM *q; + BIGNUM *pub_key; // g^x mod p + BIGNUM *priv_key; // x + + // priv_length contains the length, in bits, of the private value. If zero, + // the private value will be the same length as |p|. + unsigned priv_length; + + CRYPTO_MUTEX method_mont_p_lock; + BN_MONT_CTX *method_mont_p; + + int flags; + CRYPTO_refcount_t references; +}; + // dh_compute_key_padded_no_self_test does the same as |DH_compute_key_padded|, // but doesn't try to run the self-test first. This is for use in the self tests // themselves, to prevent an infinite loop.
diff --git a/include/openssl/dh.h b/include/openssl/dh.h index 6aad778..660627d 100644 --- a/include/openssl/dh.h +++ b/include/openssl/dh.h
@@ -244,7 +244,6 @@ #define DH_CHECK_NOT_SUITABLE_GENERATOR 0x08 #define DH_CHECK_Q_NOT_PRIME 0x10 #define DH_CHECK_INVALID_Q_VALUE 0x20 -#define DH_CHECK_INVALID_J_VALUE 0x40 // These are compatibility defines. #define DH_NOT_SUITABLE_GENERATOR DH_CHECK_NOT_SUITABLE_GENERATOR @@ -330,31 +329,6 @@ DH *dh); -struct dh_st { - BIGNUM *p; - BIGNUM *g; - BIGNUM *pub_key; // g^x mod p - BIGNUM *priv_key; // x - - // priv_length contains the length, in bits, of the private value. If zero, - // the private value will be the same length as |p|. - unsigned priv_length; - - CRYPTO_MUTEX method_mont_p_lock; - BN_MONT_CTX *method_mont_p; - - // Place holders if we want to do X9.42 DH - BIGNUM *q; - BIGNUM *j; - unsigned char *seed; - int seedlen; - BIGNUM *counter; - - int flags; - CRYPTO_refcount_t references; -}; - - #if defined(__cplusplus) } // extern C