From 12687e5a2a6ceaebf3edd032ae6af5ae42a2cae5 Mon Sep 17 00:00:00 2001 From: Chris Conlon Date: Fri, 23 Aug 2019 16:40:12 -0600 Subject: [PATCH 1/3] internally check PKCS7 content digest against messageDigest attribute --- wolfcrypt/src/pkcs7.c | 126 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-) diff --git a/wolfcrypt/src/pkcs7.c b/wolfcrypt/src/pkcs7.c index 4c8e81ca9..52173ad1f 100644 --- a/wolfcrypt/src/pkcs7.c +++ b/wolfcrypt/src/pkcs7.c @@ -3399,6 +3399,118 @@ static int wc_PKCS7_BuildSignedDataDigest(PKCS7* pkcs7, byte* signedAttrib, } +/* Verifies CMS/PKCS7 SignedData content digest matches that which is + * included in the messageDigest signed attribute. Only called when + * signed attributes are present, otherwise original signature verification + * is done over content. + * + * pkcs7 - pointer to initialized PKCS7 struct + * hashBuf - pointer to user-provided hash buffer, used with + * wc_PKCS7_VerifySignedData_ex() + * hashBufSz - size of hashBuf, octets + * + * return 0 on success, negative on error */ +static int wc_PKCS7_VerifyContentMessageDigest(PKCS7* pkcs7, + const byte* hashBuf, + word32 hashSz) +{ + int ret = 0, innerAttribSz = 0; + word32 digestSz = 0, idx = 0; + byte* digestBuf = NULL; +#ifdef WOLFSSL_SMALL_STACK + byte* digest; +#else + byte digest[MAX_PKCS7_DIGEST_SZ]; +#endif + PKCS7DecodedAttrib* attrib; + enum wc_HashType hashType; + + /* messageDigest OID (1.2.840.113549.1.9.4) */ + const byte mdOid[] = + { 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x09, 0x04 }; + + if (pkcs7 == NULL) + return BAD_FUNC_ARG; + + if ((pkcs7->content == NULL || pkcs7->contentSz == 0) && + (hashBuf == NULL || hashSz == 0)) { + WOLFSSL_MSG("SignedData bundle has no content or hash to verify"); + return BAD_FUNC_ARG; + } + + /* lookup messageDigest attribute */ + attrib = findAttrib(pkcs7, mdOid, sizeof(mdOid)); + if (attrib == NULL) { + WOLFSSL_MSG("messageDigest attribute not in bundle, must be when " + + "signed attribs are present"); + return ASN_PARSE_E; + } + + /* advance past attrib->value ASN.1 header and length */ + if (attrib->value[idx++] != ASN_OCTET_STRING) + return ASN_PARSE_E; + + if (GetLength(attrib->value, &idx, &innerAttribSz, attrib->valueSz) < 0) + return ASN_PARSE_E; + + /* get hash type and size */ + hashType = wc_OidGetHash(pkcs7->hashOID); + if (hashType == WC_HASH_TYPE_NONE) { + WOLFSSL_MSG("Error getting hash type for PKCS7 content verification"); + return BAD_FUNC_ARG; + } + + /* build content hash if needed, or use existing hash value */ + if (hashBuf == NULL) { + +#ifdef WOLFSSL_SMALL_STACK + digest = (byte*)XMALLOC(MAX_PKCS7_DIGEST_SZ, pkcs7->heap, + DYNAMIC_TYPE_TMP_BUFFER); + if (digest == NULL) + return MEMORY_E; +#endif + XMEMSET(digest, 0, MAX_PKCS7_DIGEST_SZ); + + ret = wc_Hash(hashType, pkcs7->content, pkcs7->contentSz, digest, + MAX_PKCS7_DIGEST_SZ); + if (ret < 0) { + WOLFSSL_MSG("Error hashing PKCS7 content for verification"); +#ifdef WOLFSSL_SMALL_STACK + XFREE(digest, pkcs7->heap, DYNAMIC_TYPE_TMP_BUFFER); +#endif + return ret; + } + + digestBuf = digest; + digestSz = wc_HashGetDigestSize(hashType); + + } else { + + /* user passed in pre-computed hash */ + digestBuf = (byte*)hashBuf; + digestSz = hashSz; + } + + /* compare generated to hash in messageDigest attribute */ + if ((innerAttribSz != (int)digestSz) || + (XMEMCMP(attrib->value + idx, digestBuf, digestSz) != 0)) { + WOLFSSL_MSG("Content digest does not match messageDigest attrib value"); +#ifdef WOLFSSL_SMALL_STACK + XFREE(digest, pkcs7->heap, DYNAMIC_TYPE_TMP_BUFFER); +#endif + return SIG_VERIFY_E; + } + + if (hashBuf == NULL) { +#ifdef WOLFSSL_SMALL_STACK + XFREE(digest, pkcs7->heap, DYNAMIC_TYPE_TMP_BUFFER); +#endif + } + + return 0; +} + + /* verifies SignedData signature, over either PKCS#7 DigestInfo or * content digest. * @@ -3426,7 +3538,7 @@ static int wc_PKCS7_SignedDataVerifySignature(PKCS7* pkcs7, byte* sig, if (pkcs7 == NULL) return BAD_FUNC_ARG; - /* build hash to verify against */ + /* allocate space to build hash */ pkcs7DigestSz = MAX_PKCS7_DIGEST_SZ; #ifdef WOLFSSL_SMALL_STACK pkcs7Digest = (byte*)XMALLOC(pkcs7DigestSz, pkcs7->heap, @@ -3437,6 +3549,18 @@ static int wc_PKCS7_SignedDataVerifySignature(PKCS7* pkcs7, byte* sig, XMEMSET(pkcs7Digest, 0, pkcs7DigestSz); + /* verify signed attrib digest matches that of content */ + if (signedAttrib != NULL) { + ret = wc_PKCS7_VerifyContentMessageDigest(pkcs7, hashBuf, hashSz); + if (ret != 0) { +#ifdef WOLFSSL_SMALL_STACK + XFREE(pkcs7Digest, pkcs7->heap, DYNAMIC_TYPE_TMP_BUFFER); +#endif + return ret; + } + } + + /* build hash to verify against */ ret = wc_PKCS7_BuildSignedDataDigest(pkcs7, signedAttrib, signedAttribSz, pkcs7Digest, &pkcs7DigestSz, &plainDigest, From 61d01ab7f38cd1d7818e6e5fe563839be0196611 Mon Sep 17 00:00:00 2001 From: Chris Conlon Date: Fri, 23 Aug 2019 17:05:57 -0600 Subject: [PATCH 2/3] add unit test for PKCS7 invalid detached content --- tests/api.c | 62 +++++++++++++++++++++++++++++++++++++++---- wolfcrypt/src/pkcs7.c | 4 +-- 2 files changed, 59 insertions(+), 7 deletions(-) diff --git a/tests/api.c b/tests/api.c index c0d22566d..e1379e19b 100644 --- a/tests/api.c +++ b/tests/api.c @@ -16536,11 +16536,23 @@ static void test_wc_PKCS7_EncodeSignedData_ex(void) #if defined(HAVE_PKCS7) -static int CreatePKCS7SignedData(unsigned char* output, int outputSz) +static int CreatePKCS7SignedData(unsigned char* output, int outputSz, + byte* data, word32 dataSz, + int withAttribs, int detachedSig) { PKCS7* pkcs7; WC_RNG rng; - byte data[] = "Test data to encode."; + + static byte messageTypeOid[] = + { 0x06, 0x0a, 0x60, 0x86, 0x48, 0x01, 0x86, 0xF8, 0x45, 0x01, + 0x09, 0x02 }; + static byte messageType[] = { 0x13, 2, '1', '9' }; + + PKCS7Attrib attribs[] = + { + { messageTypeOid, sizeof(messageTypeOid), messageType, + sizeof(messageType) } + }; #ifndef NO_RSA #if defined(USE_CERT_BUFFERS_2048) @@ -16617,17 +16629,30 @@ static int CreatePKCS7SignedData(unsigned char* output, int outputSz) printf(testingFmt, "wc_PKCS7_VerifySignedData()"); pkcs7->content = data; - pkcs7->contentSz = (word32)sizeof(data); + pkcs7->contentSz = dataSz; pkcs7->privateKey = key; pkcs7->privateKeySz = (word32)sizeof(key); pkcs7->encryptOID = RSAk; pkcs7->hashOID = SHAh; pkcs7->rng = &rng; + if (withAttribs) { + /* include a signed attribute */ + pkcs7->signedAttribs = attribs; + pkcs7->signedAttribsSz = (sizeof(attribs)/sizeof(PKCS7Attrib)); + } + + if (detachedSig) { + AssertIntEQ(wc_PKCS7_SetDetached(pkcs7, 1), 0); + } AssertIntGT(wc_PKCS7_EncodeSignedData(pkcs7, output, outputSz), 0); wc_PKCS7_Free(pkcs7); AssertNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, devId)); AssertIntEQ(wc_PKCS7_InitWithCert(pkcs7, NULL, 0), 0); + if (detachedSig) { + pkcs7->content = data; + pkcs7->contentSz = dataSz; + } AssertIntEQ(wc_PKCS7_VerifySignedData(pkcs7, output, outputSz), 0); wc_PKCS7_Free(pkcs7); @@ -16646,10 +16671,14 @@ static void test_wc_PKCS7_VerifySignedData(void) PKCS7* pkcs7; byte output[FOURK_BUF]; word32 outputSz = sizeof(output); + byte data[] = "Test data to encode."; byte badOut[0]; word32 badOutSz = (word32)sizeof(badOut); + byte badContent[] = "This is different content than was signed"; - AssertIntGT((outputSz = CreatePKCS7SignedData(output, outputSz)), 0); + AssertIntGT((outputSz = CreatePKCS7SignedData(output, outputSz, data, + (word32)sizeof(data), + 0, 0)), 0); AssertNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, devId)); AssertIntEQ(wc_PKCS7_Init(pkcs7, HEAP_HINT, INVALID_DEVID), 0); @@ -16670,6 +16699,26 @@ static void test_wc_PKCS7_VerifySignedData(void) wc_PKCS7_Free(pkcs7); + /* Invalid content should error, use detached signature so we can + * easily change content */ + AssertIntGT((outputSz = CreatePKCS7SignedData(output, outputSz, data, + (word32)sizeof(data), + 1, 1)), 0); + AssertNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, devId)); + AssertIntEQ(wc_PKCS7_InitWithCert(pkcs7, NULL, 0), 0); + pkcs7->content = badContent; + pkcs7->contentSz = sizeof(badContent); + AssertIntEQ(wc_PKCS7_VerifySignedData(pkcs7, output, outputSz), SIG_VERIFY_E); + wc_PKCS7_Free(pkcs7); + + /* Test success case with detached signature and valid content */ + AssertNotNull(pkcs7 = wc_PKCS7_New(HEAP_HINT, devId)); + AssertIntEQ(wc_PKCS7_InitWithCert(pkcs7, NULL, 0), 0); + pkcs7->content = data; + pkcs7->contentSz = sizeof(data); + AssertIntEQ(wc_PKCS7_VerifySignedData(pkcs7, output, outputSz), 0); + wc_PKCS7_Free(pkcs7); + printf(resultFmt, passed); #endif } /* END test_wc_PKCS7_VerifySignedData() */ @@ -24008,8 +24057,11 @@ static void test_wolfssl_PKCS7(void) byte data[FOURK_BUF]; word32 len = sizeof(data); const byte* p = data; + byte content[] = "Test data to encode."; - AssertIntGT((len = CreatePKCS7SignedData(data, len)), 0); + AssertIntGT((len = CreatePKCS7SignedData(data, len, content, + (word32)sizeof(content), + 0, 0)), 0); AssertNull(pkcs7 = d2i_PKCS7(NULL, NULL, len)); AssertNull(pkcs7 = d2i_PKCS7(NULL, &p, 0)); diff --git a/wolfcrypt/src/pkcs7.c b/wolfcrypt/src/pkcs7.c index 52173ad1f..ebc2f4228 100644 --- a/wolfcrypt/src/pkcs7.c +++ b/wolfcrypt/src/pkcs7.c @@ -3418,7 +3418,7 @@ static int wc_PKCS7_VerifyContentMessageDigest(PKCS7* pkcs7, word32 digestSz = 0, idx = 0; byte* digestBuf = NULL; #ifdef WOLFSSL_SMALL_STACK - byte* digest; + byte* digest = NULL; #else byte digest[MAX_PKCS7_DIGEST_SZ]; #endif @@ -3441,7 +3441,7 @@ static int wc_PKCS7_VerifyContentMessageDigest(PKCS7* pkcs7, /* lookup messageDigest attribute */ attrib = findAttrib(pkcs7, mdOid, sizeof(mdOid)); if (attrib == NULL) { - WOLFSSL_MSG("messageDigest attribute not in bundle, must be when " + + WOLFSSL_MSG("messageDigest attribute not in bundle, must be when " "signed attribs are present"); return ASN_PARSE_E; } From e6252a94ce1278144e80e11e29d20bb6d4971567 Mon Sep 17 00:00:00 2001 From: Chris Conlon Date: Tue, 27 Aug 2019 14:18:23 -0600 Subject: [PATCH 3/3] check attrib->value and attrib->valueSz before use --- wolfcrypt/src/pkcs7.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/wolfcrypt/src/pkcs7.c b/wolfcrypt/src/pkcs7.c index ebc2f4228..c41d6b724 100644 --- a/wolfcrypt/src/pkcs7.c +++ b/wolfcrypt/src/pkcs7.c @@ -3447,6 +3447,9 @@ static int wc_PKCS7_VerifyContentMessageDigest(PKCS7* pkcs7, } /* advance past attrib->value ASN.1 header and length */ + if (attrib->value == NULL || attrib->valueSz == 0) + return ASN_PARSE_E; + if (attrib->value[idx++] != ASN_OCTET_STRING) return ASN_PARSE_E;