Merge pull request #9715 from dgarske/rsa_key_parsing

Fix for RSA private key parsing (allowing public) and RSA keygen no malloc support

Author: JacobBarthelmeh

.github/workflows/os-check.yml

@@ -70,6 +70,7 @@ jobs:
           '--enable-all --enable-certgencache',
           '--enable-sessionexport --enable-dtls --enable-dtls13',
           '--enable-sessionexport',
+          '--disable-examples CPPFLAGS=-DWOLFSSL_NO_MALLOC',
         ]
     name: make check
     if: github.repository_owner == 'wolfssl'

tests/api.c

@@ -17080,12 +17080,13 @@ static int test_wolfSSL_d2i_PrivateKeys_bio(void)
 
         RSA_free(rsa);
         rsa = NULL;
+        /* d2i_RSA_PUBKEY_bio should fail when given private key DER.
+         * wc_RsaPublicKeyDecode correctly rejects private key format. */
         ExpectIntGT(BIO_write(bio, client_key_der_2048,
                     sizeof_client_key_der_2048), 0);
-        ExpectNotNull(d2i_RSA_PUBKEY_bio(bio, &rsa));
+        ExpectNull(d2i_RSA_PUBKEY_bio(bio, &rsa));
         (void)BIO_reset(bio);
 
-        RSA_free(rsa);
         rsa = RSA_new();
         ExpectIntEQ(wolfSSL_i2d_RSAPrivateKey(rsa, NULL), 0);
 #endif /* USE_CERT_BUFFERS_2048 WOLFSSL_KEY_GEN */

tests/api/test_ossl_rsa.c

@@ -402,12 +402,11 @@ int test_wolfSSL_RSA_DER(void)
 
     for (i = 0; tbl[i].der != NULL; i++)
     {
-        /* Passing in pointer results in pointer moving. */
+        /* d2i_RSAPublicKey should fail when given private key DER.
+         * wc_RsaPublicKeyDecode correctly rejects private key format. */
         buff = tbl[i].der;
-        ExpectNotNull(d2i_RSAPublicKey(&rsa, &buff, tbl[i].sz));
-        ExpectNotNull(rsa);
-        RSA_free(rsa);
-        rsa = NULL;
+        ExpectNull(d2i_RSAPublicKey(&rsa, &buff, tbl[i].sz));
+        ExpectNull(rsa);
     }
     for (i = 0; tbl[i].der != NULL; i++)
     {

wolfcrypt/src/asn.c

@@ -11009,6 +11009,8 @@ int wc_RsaPublicKeyDecode_ex(const byte* input, word32* inOutIdx, word32 inSz,
 #ifndef WOLFSSL_ASN_TEMPLATE
     int ret = 0;
     int length = 0;
+    int firstLen = 0;
+    word32 seqEndIdx = 0;
 #if defined(OPENSSL_EXTRA) || defined(RSA_DECODE_EXTRA)
     word32 localIdx;
     byte   tag;
@@ -11066,16 +11068,19 @@ int wc_RsaPublicKeyDecode_ex(const byte* input, word32* inOutIdx, word32 inSz,
     }
 #endif /* OPENSSL_EXTRA */
 
+    /* Calculate where the sequence should end for public key validation */
+    seqEndIdx = *inOutIdx + (word32)length;
+
     /* Get modulus */
-    ret = GetASNInt(input, inOutIdx, &length, inSz);
+    ret = GetASNInt(input, inOutIdx, &firstLen, inSz);
     if (ret < 0) {
         return ASN_RSA_KEY_E;
     }
     if (nSz)
-        *nSz = (word32)length;
+        *nSz = (word32)firstLen;
     if (n)
         *n = &input[*inOutIdx];
-    *inOutIdx += (word32)length;
+    *inOutIdx += (word32)firstLen;
 
     /* Get exponent */
     ret = GetASNInt(input, inOutIdx, &length, inSz);
@@ -11088,6 +11093,18 @@ int wc_RsaPublicKeyDecode_ex(const byte* input, word32* inOutIdx, word32 inSz,
         *e = &input[*inOutIdx];
     *inOutIdx += (word32)length;
 
+    /* Detect if this is an RSA private key being passed as public key.
+     * An RSA private key has: version (small), modulus (large), exponent,
+     * followed by more integers (d, p, q, etc.).
+     * An RSA public key has: modulus (large), exponent, and nothing more.
+     * If the first integer is small (like version 0) AND there is more data
+     * remaining in the sequence, this is likely a private key. */
+    if (firstLen <= MAX_VERSION_SZ && *inOutIdx < seqEndIdx) {
+        /* First integer is small and there's more data - looks like
+         * version field of a private key, not a modulus */
+        return ASN_RSA_KEY_E;
+    }
+
     return ret;
 #else
     DECL_ASNGETDATA(dataASN, rsaPublicKeyASN_Length);
@@ -11157,6 +11174,21 @@ int wc_RsaPublicKeyDecode_ex(const byte* input, word32* inOutIdx, word32 inSz,
         }
     }
 #endif
+    if (ret == 0) {
+        /* Detect if this is an RSA private key being passed as public key.
+         * An RSA private key has: version (small int), modulus, exponent, ...
+         * An RSA public key has: modulus (large int), exponent, nothing more.
+         * If the first integer is small (like version 0) and there's more data
+         * after what we consumed, this is likely a private key. */
+        word32 nLen = dataASN[RSAPUBLICKEYASN_IDX_PUBKEY_RSA_N].data.ref.length;
+        if (nLen <= MAX_VERSION_SZ && *inOutIdx < inSz) {
+            /* Check if next byte could be an INTEGER tag - indicating more
+             * fields like in a private key structure */
+            if (input[*inOutIdx] == ASN_INTEGER) {
+                ret = ASN_RSA_KEY_E;
+            }
+        }
+    }
     if (ret == 0) {
         /* Return the buffers and lengths asked for. */
         if (n != NULL) {

wolfcrypt/src/rsa.c

@@ -628,7 +628,11 @@ int wc_FreeRsaKey(RsaKey* key)
 static int _ifc_pairwise_consistency_test(RsaKey* key, WC_RNG* rng)
 {
     static const char* msg = "Everyone gets Friday off.";
-    byte* sig;
+#ifndef WOLFSSL_NO_MALLOC
+    byte* sig = NULL;
+#else
+    byte sig[RSA_MAX_SIZE/8];
+#endif
     byte* plain;
     int ret = 0;
     word32 msgLen, plainLen, sigLen;
@@ -643,11 +647,13 @@ static int _ifc_pairwise_consistency_test(RsaKey* key, WC_RNG* rng)
 
     WOLFSSL_MSG("Doing RSA consistency test");
 
+#ifndef WOLFSSL_NO_MALLOC
     /* Sign and verify. */
     sig = (byte*)XMALLOC(sigLen, key->heap, DYNAMIC_TYPE_RSA);
     if (sig == NULL) {
         return MEMORY_E;
     }
+#endif
     XMEMSET(sig, 0, sigLen);
 #ifdef WOLFSSL_CHECK_MEM_ZERO
     wc_MemZero_Add("Pairwise CT sig", sig, sigLen);
@@ -690,7 +696,9 @@ static int _ifc_pairwise_consistency_test(RsaKey* key, WC_RNG* rng)
         ret = RSA_KEY_PAIR_E;
 
     ForceZero(sig, sigLen);
+#ifndef WOLFSSL_NO_MALLOC
     XFREE(sig, key->heap, DYNAMIC_TYPE_RSA);
+#endif
 
     return ret;
 }

wolfcrypt/test/test.c

@@ -22717,6 +22717,22 @@ static wc_test_ret_t rsa_decode_test(RsaKey* keyPub)
         goto done;
     }
 
+#ifdef USE_CERT_BUFFERS_2048
+    /* Test that public key decode rejects a private key */
+    wc_FreeRsaKey(keyPub);
+    ret = wc_InitRsaKey(keyPub, NULL);
+    if (ret != 0)
+        return WC_TEST_RET_ENC_EC(ret);
+    inOutIdx = 0;
+    ret = wc_RsaPublicKeyDecode(client_key_der_2048, &inOutIdx, keyPub,
+                                sizeof_client_key_der_2048);
+    if (ret != WC_NO_ERR_TRACE(ASN_RSA_KEY_E)) {
+        ret = WC_TEST_RET_ENC_EC(ret);
+        goto done;
+    }
+    ret = 0; /* success - public key decode correctly rejected private key */
+#endif
+
 done:
     wc_FreeRsaKey(keyPub);
     return ret;