Fix some issues discovered when running ruby OpenSSL gem tests.

- If length param to rand bytes function is 0, do nothing and return success. - In we_dh_compute_key_int, if DH_get0_priv_key returns NULL, error out. The lack of NULL check here was causing a seg fault. - Add support for the dh_paramgen_prime_len control string in DH code. - Receiving EVP_PKEY_CTRL_DH_PARAMGEN_GENERATOR should return an error, since we don't support setting the generator. - Add a DH control command unit test. - Move EVP_PKEY_CTRL_DH_PAD definition into openssl_bc.h so that unit tests have access to it. Add comment noting which OpenSSL version introduced this macro. See ZD #14805.
wolfSSL · Sep 27, 2022 · 55399ee · 55399ee
1 parent 79f5048
commit 55399ee
Show file tree

Hide file tree

Showing 6 changed files with 155 additions and 43 deletions.
diff --git a/include/wolfengine/we_openssl_bc.h b/include/wolfengine/we_openssl_bc.h
@@ -200,6 +200,11 @@ size_t EC_POINT_point2buf(const EC_GROUP *group, const EC_POINT *point,
 #define EVP_PKEY_ECDH_KDF_X9_63     EVP_PKEY_ECDH_KDF_X9_62
 #endif
 
+#ifndef EVP_PKEY_CTRL_DH_PAD
+/* First defined in OPENSSL_VERSION_NUMBER == 0x10101001L. */
+#define EVP_PKEY_CTRL_DH_PAD (EVP_PKEY_ALG_CTRL + 16)
+#endif
+
 WOLFENGINE_LOCAL const BIGNUM *DH_get0_p(const DH *dh);
 WOLFENGINE_LOCAL const BIGNUM *DH_get0_g(const DH *dh);
 WOLFENGINE_LOCAL const BIGNUM *DH_get0_q(const DH *dh);

diff --git a/src/we_dh.c b/src/we_dh.c
@@ -31,10 +31,6 @@
  *     local one.
  */
 
-#ifndef EVP_PKEY_CTRL_DH_PAD
-#define EVP_PKEY_CTRL_DH_PAD   (EVP_PKEY_ALG_CTRL + 16)
-#endif
-
 #define DEFAULT_PRIME_LEN 1024
 
 /**
@@ -567,6 +563,7 @@ static int we_dh_compute_key_int(we_Dh *engineDh, unsigned char *secret,
     unsigned char *privBuf = NULL;
     int privLen = 0;
     unsigned int secLen = 0;
+    const BIGNUM* privBn;
 
     WOLFENGINE_ENTER(WE_LOG_KE, "we_dh_compute_key_int");
     WOLFENGINE_MSG_VERBOSE(WE_LOG_KE, "ARGS [engineDh = %p, secret = %p, "
@@ -600,7 +597,15 @@ static int we_dh_compute_key_int(we_Dh *engineDh, unsigned char *secret,
 
     if (ret == 1) {
         /* Convert our private key to a byte array. */
-        ret = we_dh_bignum_to_bin(DH_get0_priv_key(dh), &privBuf, &privLen);
+        privBn = DH_get0_priv_key(dh);
+        if (privBn == NULL) {
+            WOLFENGINE_ERROR_MSG(WE_LOG_KE, "Private key is NULL. Can't create "
+                                            "DH shared secret.");
+            ret = 0;
+        }
+        else {
+            ret = we_dh_bignum_to_bin(DH_get0_priv_key(dh), &privBuf, &privLen);
+        }
     }
 
     if (ret == 1) {
@@ -1057,6 +1062,7 @@ static int we_dh_pkey_ctrl(EVP_PKEY_CTX *ctx, int type, int num, void *ptr)
                     "setting the generator when generating DH params");
                 /* wolfCrypt doesn't allow setting the generator when generating
                  * DH params. */
+                ret = 0;
                 break;
             case EVP_PKEY_CTRL_DH_PAD:
                 dh->pad = num;
@@ -1086,7 +1092,8 @@ static int we_dh_pkey_ctrl(EVP_PKEY_CTX *ctx, int type, int num, void *ptr)
  * Extra operations for working with DH.
  * Supported operations include:
  *  - "dh_param": set the named parameters.
- *  - "pad": pad out secret to input length.
+ *  - "dh_pad": pad out secret to input length.
+ *  - "dh_paramgen_prime_len": set the length of the prime, "p."
  *
  * @param  ctx    [in]  Public key context of operation.
  * @param  type   [in]  Type of operation to perform.
@@ -1113,7 +1120,7 @@ static int we_dh_pkey_ctrl_str(EVP_PKEY_CTX *ctx, const char *type,
 
     if (ret == 1) {
         /* Set named DH parameters. */
-        if (XSTRNCMP(type, "dh_param", 9) == 0) {
+        if (XSTRCMP(type, "dh_param") == 0) {
         #ifndef HAVE_WC_DHSETNAMEDKEY
             const DhParams *params = NULL;
         #else
@@ -1122,7 +1129,7 @@ static int we_dh_pkey_ctrl_str(EVP_PKEY_CTX *ctx, const char *type,
 
         #ifdef HAVE_PUBLIC_FFDHE
         #ifdef HAVE_FFDHE_2048
-            if (XSTRNCMP(value, "ffdhe2048", 10) == 0) {
+            if (XSTRCMP(value, "ffdhe2048") == 0) {
                 WOLFENGINE_MSG(WE_LOG_KE,
                                "Setting named parameters: ffdhe2048");
             #ifndef HAVE_WC_DHSETNAMEDKEY
@@ -1134,7 +1141,7 @@ static int we_dh_pkey_ctrl_str(EVP_PKEY_CTX *ctx, const char *type,
             else
         #endif
         #ifdef HAVE_FFDHE_3072
-            if (XSTRNCMP(value, "ffdhe3072", 10) == 0) {
+            if (XSTRCMP(value, "ffdhe3072") == 0) {
                 WOLFENGINE_MSG(WE_LOG_KE,
                                "Setting named parameters: ffdhe3072");
             #ifndef HAVE_WC_DHSETNAMEDKEY
@@ -1146,7 +1153,7 @@ static int we_dh_pkey_ctrl_str(EVP_PKEY_CTX *ctx, const char *type,
             else
         #endif
         #ifdef HAVE_FFDHE_4096
-            if (XSTRNCMP(value, "ffdhe4096", 10) == 0) {
+            if (XSTRCMP(value, "ffdhe4096") == 0) {
                 WOLFENGINE_MSG(WE_LOG_KE,
                                "Setting named parameters: ffdhe4096");
             #ifndef HAVE_WC_DHSETNAMEDKEY
@@ -1180,7 +1187,8 @@ static int we_dh_pkey_ctrl_str(EVP_PKEY_CTX *ctx, const char *type,
                 rc = wc_DhSetNamedKey(&dh->key, params);
             #endif
                 if (rc != 0) {
-                     WOLFENGINE_ERROR_MSG(WE_LOG_KE, "Failed set parameters");
+                     WOLFENGINE_ERROR_MSG(WE_LOG_KE, "Failed to set "
+                        "parameters.");
                      ret = 0;
                 }
             }
@@ -1189,9 +1197,13 @@ static int we_dh_pkey_ctrl_str(EVP_PKEY_CTX *ctx, const char *type,
             }
         }
         /* Set padding requirement for secret output. */
-        else if (XSTRNCMP(type, "dh_pad", 7) == 0) {
+        else if (XSTRCMP(type, "dh_pad") == 0) {
             dh->pad = XATOI(value);
         }
+        else if (XSTRCMP(type, "dh_paramgen_prime_len") == 0) {
+            ret = we_dh_pkey_ctrl(ctx, EVP_PKEY_CTRL_DH_PARAMGEN_PRIME_LEN,
+                                  XATOI(value), NULL);
+        }
         else {
             /* Unsupported control type. */
             XSNPRINTF(errBuff, sizeof(errBuff), "Unsupported ctrl string %s",

diff --git a/src/we_random.c b/src/we_random.c
@@ -358,43 +358,45 @@ static int we_rand_bytes(unsigned char *buf, int num)
     WOLFENGINE_MSG_VERBOSE(WE_LOG_RNG, "ARGS [buf = %p, num = %d]",
                            buf, num);
 
-#ifndef WE_SINGLE_THREADED
-    rc = wc_LockMutex(we_rng_mutex);
-    if (rc != 0) {
-        WOLFENGINE_ERROR_FUNC(WE_LOG_RNG, "wc_LockMutex", rc);
-        ret = 0;
-    }
-    else
-#endif
-    {
-        /* Use global random to generator pseudo-random data. */
-        rc = wc_RNG_GenerateBlock(we_rng, buf, num);
+    if (num > 0) {
+    #ifndef WE_SINGLE_THREADED
+        rc = wc_LockMutex(we_rng_mutex);
         if (rc != 0) {
-            WOLFENGINE_ERROR_FUNC(WE_LOG_RNG, "wc_RNG_GenerateBlock", rc);
+            WOLFENGINE_ERROR_FUNC(WE_LOG_RNG, "wc_LockMutex", rc);
             ret = 0;
         }
-    #ifndef WE_STATIC_WOLFSSL
-        /* Mix global seed if RAND_add() or RAND_seed() has been called. */
-        if (ret == 1 && haveSeed) {
-            ret = we_rand_mix_seed(buf, num, we_seed, sizeof(we_seed));
-            if (ret != 1) {
-                WOLFENGINE_ERROR_MSG(WE_LOG_RNG, "we_rand_mix_seed with global "
-                                                 "seed failed");
+        else
+    #endif
+        {
+            /* Use global random to generator pseudo-random data. */
+            rc = wc_RNG_GenerateBlock(we_rng, buf, num);
+            if (rc != 0) {
+                WOLFENGINE_ERROR_FUNC(WE_LOG_RNG, "wc_RNG_GenerateBlock", rc);
+                ret = 0;
             }
-        }
-        /* Mix in weak entropy. */
-        if (ret == 1) {
-            ret = we_rand_add_weak_entropy(buf, num);
-            if (ret != 1) {
-                WOLFENGINE_ERROR_MSG(WE_LOG_RNG, "we_rand_mix_seed with "
-                    "weak entropy failed");
+        #ifndef WE_STATIC_WOLFSSL
+            /* Mix global seed if RAND_add() or RAND_seed() has been called. */
+            if (ret == 1 && haveSeed) {
+                ret = we_rand_mix_seed(buf, num, we_seed, sizeof(we_seed));
+                if (ret != 1) {
+                    WOLFENGINE_ERROR_MSG(WE_LOG_RNG, "we_rand_mix_seed with "
+                                                     "global seed failed");
+                }
             }
-        }
-    #endif /* !WE_STATIC_WOLFSSL */
+            /* Mix in weak entropy. */
+            if (ret == 1) {
+                ret = we_rand_add_weak_entropy(buf, num);
+                if (ret != 1) {
+                    WOLFENGINE_ERROR_MSG(WE_LOG_RNG, "we_rand_mix_seed with "
+                        "weak entropy failed");
+                }
+            }
+        #endif /* !WE_STATIC_WOLFSSL */
 
-    #ifndef WE_SINGLE_THREADED
-        wc_UnLockMutex(we_rng_mutex);
-    #endif
+        #ifndef WE_SINGLE_THREADED
+            wc_UnLockMutex(we_rng_mutex);
+        #endif
+        }
     }
 
     WOLFENGINE_LEAVE(WE_LOG_RNG, "we_rand_bytes", ret);

diff --git a/test/test_dh.c b/test/test_dh.c
@@ -426,6 +426,97 @@ int test_dh_pkey(ENGINE* e, void* data)
     return err;
 }
 
+int test_dh_ctrl(ENGINE* e, void* data)
+{
+    int err;
+    EVP_PKEY_CTX* ctx = NULL;
+    int primeLengths[] = {1024, 2048, 3072};
+    word32 i;
+
+    (void)data;
+
+    err = (ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_DH, e)) == NULL;
+    if (err == 0){
+        err = EVP_PKEY_paramgen_init(ctx) <= 0;
+    }
+
+    for (i = 0; err == 0 && i < sizeof(primeLengths)/sizeof(*primeLengths);
+         ++i) {
+        /* Set valid prime lengths. */
+        err = EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_DH, EVP_PKEY_OP_PARAMGEN,
+                  EVP_PKEY_CTRL_DH_PARAMGEN_PRIME_LEN, primeLengths[i],
+                  NULL) <= 0;
+    }
+    if (err == 0) {
+        /* Set an invalid prime length. */
+        err = EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_DH, EVP_PKEY_OP_PARAMGEN,
+                  EVP_PKEY_CTRL_DH_PARAMGEN_PRIME_LEN, 512, NULL) > 0;
+    }
+    if (err == 0) {
+        /* Set a valid prime length via control string. */
+        err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_paramgen_prime_len", "2048") <= 0;
+    }
+
+    if (err == 0) {
+        /* Set the generator (not supported by wolfCrypt). */
+        err = EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_DH, EVP_PKEY_OP_PARAMGEN,
+                  EVP_PKEY_CTRL_DH_PARAMGEN_GENERATOR, 2, NULL) > 0;
+    }
+
+    if (err == 0) {
+        /* Zero pad secret. */
+        err = EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_DH, EVP_PKEY_OP_PARAMGEN,
+                  EVP_PKEY_CTRL_DH_PAD, 1, NULL) <= 0;
+    }
+    if (err == 0) {
+        /* Same as above but using a control string. */
+        err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_pad", "1") <= 0;
+    }
+
+    if (err == 0) {
+        /* Set peer key for shared secret. No-op internally. */
+        err = EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_DH, EVP_PKEY_OP_PARAMGEN,
+                  EVP_PKEY_CTRL_PEER_KEY, 0, NULL) <= 0;
+    }
+
+#ifdef HAVE_PUBLIC_FFDHE
+    /* Set parameters to different named parameter sets. */
+    #ifdef HAVE_FFDHE_2048
+    if (err == 0) {
+        err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_param", "ffdhe2048") <= 0;
+    }
+    #else
+    if (err == 0) {
+        err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_param", "ffdhe2048") > 0;
+    }
+    #endif /* HAVE_FFDHE_2048 */
+
+    #ifdef HAVE_FFDHE_3072
+    if (err == 0) {
+        err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_param", "ffdhe3072") <= 0;
+    }
+    #else
+    if (err == 0) {
+        err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_param", "ffdhe3072") > 0;
+    }
+    #endif /* HAVE_FFDHE_3072 */
+
+    #ifdef HAVE_FFDHE_4096
+    if (err == 0) {
+        err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_param", "ffdhe4096") <= 0;
+    }
+    #else
+    if (err == 0) {
+        err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_param", "ffdhe4096") > 0;
+    }
+    #endif /* HAVE_FFDHE_4096 */
+#endif /* HAVE_PUBLIC_FFDHE */
+
+    EVP_PKEY_CTX_free(ctx);
+
+    return err;
+}
+
 #endif /* WE_HAVE_EVP_PKEY */
 
 #endif /* WE_HAVE_DH */
diff --git a/test/unit.c b/test/unit.c
@@ -153,6 +153,7 @@ TEST_CASE test_case[] = {
 #ifdef WE_HAVE_EVP_PKEY
     TEST_DECL(test_dh_pgen_pkey, NULL),
     TEST_DECL(test_dh_pkey, NULL),
+    TEST_DECL(test_dh_ctrl, NULL),
 #if !defined(WE_SINGLE_THREADED) && defined(_WIN32)
     TEST_DECL(test_dh_key_gen_multithreaded, NULL),
 #endif /* !WE_SINGLE_THREADED && _WIN32 */

diff --git a/test/unit.h b/test/unit.h
@@ -249,6 +249,7 @@ int test_dh(ENGINE *e, void *data);
 #ifdef WE_HAVE_EVP_PKEY
 int test_dh_pgen_pkey(ENGINE *e, void *data);
 int test_dh_pkey(ENGINE *e, void *data);
+int test_dh_ctrl(ENGINE *e, void *data);
 #if !defined(WE_SINGLE_THREADED) && defined(_WIN32)
 int test_dh_key_gen_multithreaded(ENGINE *e, void *data);
 #endif /* !WE_SINGLE_THREADED && _WIN32 */