Fix handling of NULL sig parameter in ECDSA_sign and similar

The problem is, that it almost works to pass sig=NULL to the
ECDSA_sign, ECDSA_sign_ex and DSA_sign, to compute the necessary
space for the resulting signature.
But since the ECDSA signature is non-deterministic
(except when ECDSA_sign_setup/ECDSA_sign_ex are used)
the resulting length may be different when the API is called again.
This can easily cause random memory corruption.
Several internal APIs had the same issue, but since they are
never called with sig=NULL, it is better to make them return an
error in that case, instead of making the code more complex.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/23529)

(cherry picked from commit 1fa2bf9b1885d2e87524421fea5041d40149cffa)

diff --git a/crypto/dsa/dsa_sign.c b/crypto/dsa/dsa_sign.c
index ddfbfa18af157e65c4fb36f9e85fec5bbb4e1dee..2f963af8e123c59d205ecf086d085c45ebb5d1ea 100644 (file)
--- a/crypto/dsa/dsa_sign.c
+++ b/crypto/dsa/dsa_sign.c
@@ -156,6 +156,11 @@ int ossl_dsa_sign_int(int type, const unsigned char *dgst, int dlen,
 {
     DSA_SIG *s;
 
+    if (sig == NULL) {
+        *siglen = DSA_size(dsa);
+        return 1;
+    }
+
     /* legacy case uses the method table */
     if (dsa->libctx == NULL || dsa->meth != DSA_get_default_method())
         s = DSA_do_sign(dgst, dlen, dsa);
@@ -165,7 +170,7 @@ int ossl_dsa_sign_int(int type, const unsigned char *dgst, int dlen,
         *siglen = 0;
         return 0;
     }
-    *siglen = i2d_DSA_SIG(s, sig != NULL ? &sig : NULL);
+    *siglen = i2d_DSA_SIG(s, &sig);
     DSA_SIG_free(s);
     return 1;
 }

diff --git a/crypto/ec/ecdsa_ossl.c b/crypto/ec/ecdsa_ossl.c
index 0bf4635e2f9723a96fb6cb06b337dd8fa82df6a1..0bdf45e6e77ded4bb23f165743144c566e2d4f6c 100644 (file)
--- a/crypto/ec/ecdsa_ossl.c
+++ b/crypto/ec/ecdsa_ossl.c
@@ -70,6 +70,11 @@ int ossl_ecdsa_sign(int type, const unsigned char *dgst, int dlen,
 {
     ECDSA_SIG *s;
 
+    if (sig == NULL && (kinv == NULL || r == NULL)) {
+        *siglen = ECDSA_size(eckey);
+        return 1;
+    }
+
     s = ECDSA_do_sign_ex(dgst, dlen, kinv, r, eckey);
     if (s == NULL) {
         *siglen = 0;

diff --git a/crypto/sm2/sm2_sign.c b/crypto/sm2/sm2_sign.c
index 31b5a63b85a2f2be15c9601bd90da3c9863cb8b3..b6d41db8e320c813f3587775dd8f9a552c43e3b2 100644 (file)
--- a/crypto/sm2/sm2_sign.c
+++ b/crypto/sm2/sm2_sign.c
@@ -441,6 +441,11 @@ int ossl_sm2_internal_sign(const unsigned char *dgst, int dgstlen,
     int sigleni;
     int ret = -1;
 
+    if (sig == NULL) {
+        ERR_raise(ERR_LIB_SM2, ERR_R_PASSED_NULL_PARAMETER);
+        goto done;
+    }
+
     e = BN_bin2bn(dgst, dgstlen, NULL);
     if (e == NULL) {
        ERR_raise(ERR_LIB_SM2, ERR_R_BN_LIB);
@@ -453,7 +458,7 @@ int ossl_sm2_internal_sign(const unsigned char *dgst, int dgstlen,
         goto done;
     }
 
-    sigleni = i2d_ECDSA_SIG(s, sig != NULL ? &sig : NULL);
+    sigleni = i2d_ECDSA_SIG(s, &sig);
     if (sigleni < 0) {
        ERR_raise(ERR_LIB_SM2, ERR_R_INTERNAL_ERROR);
        goto done;

diff --git a/test/dsatest.c b/test/dsatest.c
index 5fa83020f87a22826d9dbcdea5c1b6fcf973bf23..73c6827bb0defe1189647838624c8a7b20bd1ed6 100644 (file)
--- a/test/dsatest.c
+++ b/test/dsatest.c
@@ -332,6 +332,7 @@ static int test_dsa_sig_infinite_loop(void)
     BIGNUM *p = NULL, *q = NULL, *g = NULL, *priv = NULL, *pub = NULL, *priv2 = NULL;
     BIGNUM *badq = NULL, *badpriv = NULL;
     const unsigned char msg[] = { 0x00 };
+    unsigned int signature_len0;
     unsigned int signature_len;
     unsigned char signature[64];
 
@@ -375,10 +376,13 @@ static int test_dsa_sig_infinite_loop(void)
         goto err;
 
     /* Test passing signature as NULL */
-    if (!TEST_true(DSA_sign(0, msg, sizeof(msg), NULL, &signature_len, dsa)))
+    if (!TEST_true(DSA_sign(0, msg, sizeof(msg), NULL, &signature_len0, dsa))
+        || !TEST_int_gt(signature_len0, 0))
         goto err;
 
-    if (!TEST_true(DSA_sign(0, msg, sizeof(msg), signature, &signature_len, dsa)))
+    if (!TEST_true(DSA_sign(0, msg, sizeof(msg), signature, &signature_len, dsa))
+        || !TEST_int_gt(signature_len, 0)
+        || !TEST_int_le(signature_len, signature_len0))
         goto err;
 
     /* Test using a private key of zero fails - this causes an infinite loop without the retry test */

diff --git a/test/ecdsatest.c b/test/ecdsatest.c
index 33a52eb1b5624d80d0e0d9f019f5e5744be85ca6..ded41be5bdcd002821d520886cec2b35b3612650 100644 (file)
--- a/test/ecdsatest.c
+++ b/test/ecdsatest.c
@@ -350,15 +350,39 @@ static int test_builtin_as_sm2(int n)
 static int test_ecdsa_sig_NULL(void)
 {
     int ret;
+    unsigned int siglen0;
     unsigned int siglen;
     unsigned char dgst[128] = { 0 };
     EC_KEY *eckey = NULL;
+    unsigned char *sig = NULL;
+    BIGNUM *kinv = NULL, *rp = NULL;
 
     ret = TEST_ptr(eckey = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1))
           && TEST_int_eq(EC_KEY_generate_key(eckey), 1)
-          && TEST_int_eq(ECDSA_sign(0, dgst, sizeof(dgst), NULL, &siglen, eckey), 1)
-          && TEST_int_gt(siglen, 0);
+          && TEST_int_eq(ECDSA_sign(0, dgst, sizeof(dgst), NULL, &siglen0,
+                                    eckey), 1)
+          && TEST_int_gt(siglen0, 0)
+          && TEST_ptr(sig = OPENSSL_malloc(siglen0))
+          && TEST_int_eq(ECDSA_sign(0, dgst, sizeof(dgst), sig, &siglen,
+                                    eckey), 1)
+          && TEST_int_gt(siglen, 0)
+          && TEST_int_le(siglen, siglen0)
+          && TEST_int_eq(ECDSA_verify(0, dgst, sizeof(dgst), sig, siglen,
+                                      eckey), 1)
+          && TEST_int_eq(ECDSA_sign_setup(eckey, NULL, &kinv, &rp), 1)
+          && TEST_int_eq(ECDSA_sign_ex(0, dgst, sizeof(dgst), NULL, &siglen,
+                                       kinv, rp, eckey), 1)
+          && TEST_int_gt(siglen, 0)
+          && TEST_int_le(siglen, siglen0)
+          && TEST_int_eq(ECDSA_sign_ex(0, dgst, sizeof(dgst), sig, &siglen0,
+                                       kinv, rp, eckey), 1)
+          && TEST_int_eq(siglen, siglen0)
+          && TEST_int_eq(ECDSA_verify(0, dgst, sizeof(dgst), sig, siglen,
+                                      eckey), 1);
     EC_KEY_free(eckey);
+    OPENSSL_free(sig);
+    BN_free(kinv);
+    BN_free(rp);
     return ret;
 }