Fix DSA signature length handling in certificate verification

tests/api/test_dsa.c

@@ -1039,3 +1039,66 @@ int test_wc_DsaExportKeyRaw_individual_args(void)
 #endif /* !NO_DSA && WOLFSSL_KEY_GEN */
     return EXPECT_RESULT();
 } /* END test_wc_DsaExportKeyRaw_individual_args */
+
+/*
+ * Testing that a certificate signature field holding fewer bytes than the
+ * verifying DSA key's 2 * |q| is rejected without reading past the copy
+ * ConfirmSignature() makes of it.
+ */
+int test_wc_DsaVerify_cert_sig_narrower_than_q(void)
+{
+    EXPECT_DECLS;
+#if !defined(NO_DSA) && !defined(HAVE_SELFTEST) && !defined(NO_CERTS) && \
+    !defined(NO_SHA256) && !defined(NO_FILESYSTEM) && \
+    (defined(OPENSSL_EXTRA) || defined(WOLFSSL_SMALL_CERT_VERIFY))
+    /* v1 certificate signed with dsa-with-SHA256 whose signature field is
+     * DSA_MIN_SIG_SIZE bytes, as produced by splicing a 40 byte signature into
+     * a DSA certificate. The key it is checked against is supplied separately,
+     * so only the signature algorithm and the signature length matter here. */
+    static const byte cert[] = {
+        0x30, 0x81, 0x9f,
+          0x30, 0x65,
+            0x02, 0x01, 0x01,
+            0x30, 0x0b,
+              0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x03,
+              0x02,
+            0x30, 0x0e,
+              0x31, 0x0c, 0x30, 0x0a, 0x06, 0x03, 0x55, 0x04, 0x03, 0x0c,
+              0x03, 0x64, 0x73, 0x61,
+            0x30, 0x1e,
+              0x17, 0x0d, 0x32, 0x35, 0x30, 0x31, 0x30, 0x31, 0x30, 0x30,
+              0x30, 0x30, 0x30, 0x30, 0x5a,
+              0x17, 0x0d, 0x33, 0x35, 0x30, 0x31, 0x30, 0x31, 0x30, 0x30,
+              0x30, 0x30, 0x30, 0x30, 0x5a,
+            0x30, 0x0e,
+              0x31, 0x0c, 0x30, 0x0a, 0x06, 0x03, 0x55, 0x04, 0x03, 0x0c,
+              0x03, 0x64, 0x73, 0x61,
+            0x30, 0x13,
+              0x30, 0x0b,
+                0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x38, 0x04, 0x01,
+                0x05, 0x00,
+              0x03, 0x04, 0x00, 0x02, 0x01, 0x01,
+          0x30, 0x0b,
+            0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x03,
+            0x02,
+          0x03, 0x29, 0x00,
+            0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
+            0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
+            0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
+            0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01
+    };
+    /* 256 bit q, so the verifier reads r and s as 32 bytes each. */
+    byte   key[TWOK_BUF];
+    word32 keySz = 0;
+    XFILE  fp = XBADFILE;
+
+    ExpectTrue((fp = XFOPEN("./certs/dsa-pubkey-2048.der", "rb")) != XBADFILE);
+    ExpectIntGT(keySz = (word32)XFREAD(key, 1, sizeof(key), fp), 0);
+    if (fp != XBADFILE)
+        XFCLOSE(fp);
+
+    ExpectIntNE(wc_CheckCertSigPubKey(cert, (word32)sizeof(cert), NULL, key,
+        keySz, DSAk), 0);
+#endif
+    return EXPECT_RESULT();
+} /* END test_wc_DsaVerify_cert_sig_narrower_than_q */

tests/api/test_dsa.h

@@ -39,6 +39,7 @@ int test_wc_DsaSign_bad_digestSz(void);
 int test_wc_DsaImportParamsRaw_individual_args(void);
 int test_wc_DsaExportParamsRaw_individual_args(void);
 int test_wc_DsaExportKeyRaw_individual_args(void);
+int test_wc_DsaVerify_cert_sig_narrower_than_q(void);
 
 #define TEST_DSA_DECLS                                          \
     TEST_DECL_GROUP("dsa", test_wc_InitDsaKey),                 \
@@ -55,6 +56,7 @@ int test_wc_DsaExportKeyRaw_individual_args(void);
     TEST_DECL_GROUP("dsa", test_wc_DsaSign_bad_digestSz),       \
     TEST_DECL_GROUP("dsa", test_wc_DsaImportParamsRaw_individual_args), \
     TEST_DECL_GROUP("dsa", test_wc_DsaExportParamsRaw_individual_args), \
-    TEST_DECL_GROUP("dsa", test_wc_DsaExportKeyRaw_individual_args)
+    TEST_DECL_GROUP("dsa", test_wc_DsaExportKeyRaw_individual_args),    \
+    TEST_DECL_GROUP("dsa", test_wc_DsaVerify_cert_sig_narrower_than_q)
 
 #endif /* WOLFCRYPT_TEST_DSA_H */

tests/unit-mcdc/test_asn_cert_whitebox.c

@@ -1733,9 +1733,10 @@ static void wb_decode_dsa_asn1_sig(void)
     contentSz += wb_tlv(content + contentSz, ASN_INTEGER, &sVal, 1);
     sigSz = WB_SEQ(sig, content, contentSz);
 
-    ret = DecodeDsaAsn1Sig(sig, sigSz, sigCpy, NULL);
+    /* qSz 1: r and s are one byte each, so sigCpy holds them unpadded. */
+    ret = DecodeDsaAsn1Sig(sig, sigSz, sigCpy, 1, NULL);
     WB_CHECK(ret == 0 && sigCpy[0] == rVal && sigCpy[1] == sVal,
-            "valid r/s -> mp_to_unsigned_bin() succeeds both times "
+            "valid r/s -> mp_to_unsigned_bin_len() succeeds both times "
             "(:17294 false side)");
 }
 #else

tests/unit-mcdc/test_asn_fault_whitebox.c

@@ -109,9 +109,9 @@
  *     :24921/:24925 leading operands ARE reachable and are driven by the
  *     allocation sweep in section 24.
  *   - DecodeDsaAsn1Sig() :17348 both operands: the block is guarded by
- *     :17342, which has already rejected rSz + sSz > sigSz, and every
- *     caller passes a sigCpy of at least sigSz bytes, so both
- *     mp_to_unsigned_bin() calls write inside the buffer and cannot fail.
+ *     :17342, which has already rejected rSz > qSz || sSz > qSz, and every
+ *     caller passes a sigCpy of 2 * qSz bytes, so both
+ *     mp_to_unsigned_bin_len() calls write inside the buffer and cannot fail.
  *
  * the uncovered-condition report rows in the deep certificate chain-verification internals
  * (name-constraint enforcement, X.509 extension decoding/verification,
@@ -2567,25 +2567,25 @@ static byte* wb_read_pem_file(const char* path, long* outLen)
 
 /* ------------------------------------------------------------------------- *
  * Section 24b: ConfirmSignature() DSA signature-size dispatch (:17703).
- *   if (sigSz != DSA_160_SIG_SIZE && sigSz != DSA_256_SIG_SIZE)
- *       ret = DecodeDsaAsn1Sig(...);
- *   else
+ *   if (sigSz == 2 * qSz)
  *       XMEMCPY(sigCtx->sigCpy, sig, sigSz);
+ *   else
+ *       ret = DecodeDsaAsn1Sig(...);
  * A DSA-signed certificate carries an ASN.1 DSA-Sig-Value, so the raw-copy
- * arm (a signature that is exactly 40 or 64 bytes) is never taken from a
+ * arm (a signature that is exactly 2 * |q| bytes) is never taken from a
  * certificate parse. Calling ConfirmSignature() directly with a DSA public
- * key and three signature lengths drives all three rows. The signature bytes
+ * key and two signature lengths drives both rows. The signature bytes
  * themselves are irrelevant -- the call always ends in a verification
  * failure; the point is which arm the size dispatch picks.
  * ------------------------------------------------------------------------- */
 #if !defined(NO_DSA) && !defined(HAVE_SELFTEST) && !defined(NO_ASN_CRYPT)
 static void wb_confirm_signature_dsa_sigsz(void)
 {
-    static const word32 sizes[3] = { DSA_160_SIG_SIZE, DSA_256_SIG_SIZE, 50 };
-    static const char* names[3] = {
-        "sigSz == DSA_160_SIG_SIZE (1st operand false)",
-        "sigSz == DSA_256_SIG_SIZE (1st operand true, 2nd false)",
-        "sigSz neither (both operands true, ASN.1 decode path)"
+    /* certs/dsa2048.der has a 160-bit q, so 2 * qSz is DSA_160_SIG_SIZE. */
+    static const word32 sizes[2] = { DSA_160_SIG_SIZE, 50 };
+    static const char* names[2] = {
+        "sigSz == 2 * qSz (true, raw copy)",
+        "sigSz != 2 * qSz (false, ASN.1 decode path)"
     };
     byte* pem = NULL;
     long pemSz = 0;
@@ -2629,7 +2629,7 @@ static void wb_confirm_signature_dsa_sigsz(void)
     XMEMSET(sig, 0x5A, sizeof(sig));
     XMEMSET(tbs, 0x11, sizeof(tbs));
 
-    for (i = 0; i < 3; i++) {
+    for (i = 0; i < 2; i++) {
         SignatureCtx sigCtx;
         InitSignatureCtx(&sigCtx, NULL, INVALID_DEVID);
         ret = ConfirmSignature(&sigCtx, tbs, (word32)sizeof(tbs), pubDer,
@@ -2984,12 +2984,12 @@ static void wb_encoder_size_guards(void)
  * are two consecutive XMALLOCs, so failing from the first allocation drives
  * the 1st operand true and failing only the second drives the 2nd.
  *
- * RESIDUAL -- :17348 (`mp_to_unsigned_bin(r, sigCpy) != MP_OKAY ||
- * mp_to_unsigned_bin(s, sigCpy + rSz) != MP_OKAY`) has no reachable true
- * side: it is guarded by :17342, which has already rejected rSz + sSz >
- * sigSz, and every caller passes a sigCpy of at least sigSz bytes, so both
- * conversions write inside the buffer. mp_to_unsigned_bin() on an
- * initialised mp_int with a large enough output cannot fail.
+ * RESIDUAL -- :17348 (`mp_to_unsigned_bin_len(r, sigCpy, qSz) != MP_OKAY ||
+ * mp_to_unsigned_bin_len(s, sigCpy + qSz, qSz) != MP_OKAY`) has no reachable
+ * true side: it is guarded by :17342, which has already rejected rSz > qSz ||
+ * sSz > qSz, and every caller passes a sigCpy of 2 * qSz bytes, so both
+ * conversions write inside the buffer. mp_to_unsigned_bin_len() on an
+ * initialised mp_int that fits the requested length cannot fail.
  * ------------------------------------------------------------------------- */
 #if !defined(NO_DSA) && !defined(HAVE_SELFTEST)
 static void wb_decode_dsa_asn1_sig_alloc(void)
@@ -3002,15 +3002,16 @@ static void wb_decode_dsa_asn1_sig_alloc(void)
     WB_NOTE("DecodeDsaAsn1Sig(): r/s allocation guard [:17324]");
 
     XMEMSET(sigCpy, 0, sizeof(sigCpy));
-    ret = DecodeDsaAsn1Sig(sig, (word32)sizeof(sig), sigCpy, NULL);
+    /* qSz 1: r and s are one byte each, so sigCpy holds them unpadded. */
+    ret = DecodeDsaAsn1Sig(sig, (word32)sizeof(sig), sigCpy, 1, NULL);
     WB_CHECK(ret == 0, "well-formed DSA-Sig-Value (both operands false)");
 
     mcdc_fa_install();
     mcdc_fa_arm(1);
-    (void)DecodeDsaAsn1Sig(sig, (word32)sizeof(sig), sigCpy, NULL);
+    (void)DecodeDsaAsn1Sig(sig, (word32)sizeof(sig), sigCpy, 1, NULL);
     mcdc_fa_disarm();
     mcdc_fa_arm_only(2);
-    (void)DecodeDsaAsn1Sig(sig, (word32)sizeof(sig), sigCpy, NULL);
+    (void)DecodeDsaAsn1Sig(sig, (word32)sizeof(sig), sigCpy, 1, NULL);
     mcdc_fa_disarm();
     mcdc_fa_restore();
 }

wolfcrypt/src/asn.c

@@ -17507,9 +17507,9 @@ static int HashForSignature(const byte* buf, word32 bufSz, word32 sigOID,
 #endif /* !NO_ASN_CRYPT && !NO_HASH_WRAPPER */
 
 #if !defined(NO_DSA) && !defined(HAVE_SELFTEST)
-/* Try to parse as ASN.1 bitstring */
+/* Parse as an ASN.1 bitstring into sigCpy as r||s, each zero padded to qSz. */
 static int DecodeDsaAsn1Sig(const byte* sig, word32 sigSz, byte* sigCpy,
-    void* heap)
+    int qSz, void* heap)
 {
     int ret = 0;
     int rSz = 0, sSz = 0, mpinit = 0;
@@ -17542,14 +17542,14 @@ static int DecodeDsaAsn1Sig(const byte* sig, word32 sigSz, byte* sigCpy,
     if (ret == 0) {
         rSz = mp_unsigned_bin_size(r);
         sSz = mp_unsigned_bin_size(s);
-        if (rSz + sSz > (int)sigSz) {
+        if (rSz > qSz || sSz > qSz) {
             WOLFSSL_MSG("DSA sig size invalid");
             ret = ASN_SIG_CONFIRM_E;
         }
     }
     if (ret == 0) {
-        if (mp_to_unsigned_bin(r, sigCpy) != MP_OKAY ||
-            mp_to_unsigned_bin(s, sigCpy + rSz) != MP_OKAY) {
+        if (mp_to_unsigned_bin_len(r, sigCpy, qSz) != MP_OKAY ||
+            mp_to_unsigned_bin_len(s, sigCpy + qSz, qSz) != MP_OKAY) {
             WOLFSSL_MSG("DSA sig to unsigned bin failed!");
             ret = ASN_SIG_CONFIRM_E;
         }
@@ -17870,6 +17870,7 @@ int ConfirmSignature(SignatureCtx* sigCtx,
                 case DSAk:
                 {
                     word32 idx = 0;
+                    int qSz;
 
                     if (sigSz < DSA_MIN_SIG_SIZE) {
                         WOLFSSL_MSG("Verify Signature is too small");
@@ -17890,27 +17891,34 @@ int ConfirmSignature(SignatureCtx* sigCtx,
                         WOLFSSL_MSG("wc_InitDsaKey_h error");
                         goto exit_cs;
                     }
-                #ifndef WOLFSSL_NO_MALLOC
-                    sigCtx->sigCpy = (byte*)XMALLOC(sigSz,
-                                         sigCtx->heap, DYNAMIC_TYPE_SIGNATURE);
-                    if (sigCtx->sigCpy == NULL) {
-                        ERROR_OUT(MEMORY_E, exit_cs);
-                    }
-                #endif
                     if ((ret = wc_DsaPublicKeyDecode(key, &idx, sigCtx->key.dsa,
                                                                  keySz)) != 0) {
                         WOLFSSL_MSG("ASN Key decode error DSA");
                         WOLFSSL_ERROR_VERBOSE(ret);
                         goto exit_cs;
                     }
-                    if (sigSz != DSA_160_SIG_SIZE &&
-                        sigSz != DSA_256_SIG_SIZE) {
-                        ret = DecodeDsaAsn1Sig(sig, sigSz, sigCtx->sigCpy,
-                            sigCtx->heap);
+                    /* wc_DsaVerify() reads 2 * |q| bytes from sigCpy, so size
+                     * the copy from the key and reject any |q| the DSA code
+                     * cannot produce a signature for. */
+                    qSz = mp_unsigned_bin_size(&sigCtx->key.dsa->q);
+                    if (qSz < DSA_MIN_HALF_SIZE || qSz > DSA_MAX_HALF_SIZE) {
+                        WOLFSSL_MSG("Verify DSA key q size invalid");
+                        ERROR_OUT(ASN_SIG_CONFIRM_E, exit_cs);
                     }
-                    else {
+                #ifndef WOLFSSL_NO_MALLOC
+                    sigCtx->sigCpy = (byte*)XMALLOC((word32)(2 * qSz),
+                                         sigCtx->heap, DYNAMIC_TYPE_SIGNATURE);
+                    if (sigCtx->sigCpy == NULL) {
+                        ERROR_OUT(MEMORY_E, exit_cs);
+                    }
+                #endif
+                    if (sigSz == (word32)(2 * qSz)) {
                         XMEMCPY(sigCtx->sigCpy, sig, sigSz);
                     }
+                    else {
+                        ret = DecodeDsaAsn1Sig(sig, sigSz, sigCtx->sigCpy, qSz,
+                            sigCtx->heap);
+                    }
                     break;
                 }
             #endif /* !NO_DSA && !HAVE_SELFTEST */

wolfcrypt/src/dsa.c

@@ -1216,8 +1216,10 @@ int wc_DsaVerify_ex(const byte* digest, word32 digestSz, const byte* sig,
             break;
         }
 
+        /* sig holds r||s as 2 * |q| bytes and carries no length, so bound |q|
+         * to keep the read within DSA_MAX_SIG_SIZE. */
         qSz = mp_unsigned_bin_size(&key->q);
-        if (qSz <= 0) {
+        if (qSz <= 0 || qSz > DSA_MAX_HALF_SIZE) {
             ret = BAD_FUNC_ARG;
             break;
         }