diff --git a/tests/api/test_dsa.c b/tests/api/test_dsa.c index c816a69f095..a3ef5d1614f 100644 --- a/tests/api/test_dsa.c +++ b/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 */ diff --git a/tests/api/test_dsa.h b/tests/api/test_dsa.h index 0b65e2b03ec..cb23a49e581 100644 --- a/tests/api/test_dsa.h +++ b/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 */ diff --git a/tests/unit-mcdc/test_asn_cert_whitebox.c b/tests/unit-mcdc/test_asn_cert_whitebox.c index 9c52f9e783e..d286140480d 100644 --- a/tests/unit-mcdc/test_asn_cert_whitebox.c +++ b/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 diff --git a/tests/unit-mcdc/test_asn_fault_whitebox.c b/tests/unit-mcdc/test_asn_fault_whitebox.c index a9086aecc57..f644d161680 100644 --- a/tests/unit-mcdc/test_asn_fault_whitebox.c +++ b/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(); } diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index ba98f4b7871..38a001bae41 100644 --- a/wolfcrypt/src/asn.c +++ b/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 */ diff --git a/wolfcrypt/src/dsa.c b/wolfcrypt/src/dsa.c index b40b0b974e6..536741f21a9 100644 --- a/wolfcrypt/src/dsa.c +++ b/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; }