diff --git a/tests/api/test_asn.c b/tests/api/test_asn.c index d6f9924e1e..8988a9c766 100644 --- a/tests/api/test_asn.c +++ b/tests/api/test_asn.c @@ -2415,6 +2415,116 @@ int test_DecodeCertExtensions_empty_certpol_trailing(void) return EXPECT_RESULT(); } +/* A constructed ASN.1 item must be exactly consumed by the items parsed out of + * it. GetASN_Items() used to only enforce that for containers still open when + * the template ran out, so a container the template walked into and back out of + * mid-way could declare a length longer than its contents and have the excess + * silently ignored. + * + * For a certificate that means TBSCertificate.validity can claim a length that + * runs past notBefore/notAfter and swallows subject, subjectPublicKeyInfo and + * the start of extensions. wolfSSL resumed parsing right after notAfter and + * accepted the certificate, while a strict parser rejects it and a lax one may + * resume after the declared end of validity and read a completely different + * subjectPublicKeyInfo. That disagreement is exploitable: a peer holding only + * the key at the real SPKI offset can satisfy an application's public-key pin + * for an unrelated key embedded in an extension. + * + * The two DERs below differ in exactly one byte: the length octet of the + * validity SEQUENCE, 0x1e (30, the correct length) versus 0x59 (89). */ +int test_ParseCert_validity_length_overrun(void) +{ + EXPECT_DECLS; + +#if !defined(NO_CERTS) && !defined(NO_ASN) && defined(HAVE_ED25519) && \ + defined(HAVE_ED25519_VERIFY) + /* Ed25519 certificate, empty issuer and subject, one unknown extension + * whose payload happens to be a second, well-formed SPKI. Well-formed: + * validity SEQUENCE at offset 43 has length 30 and holds exactly the two + * UTCTIMEs at offsets 45 and 60. */ + static const byte validCert[] = { + 0x30, 0x82, 0x01, 0x19, 0x30, 0x81, 0xcc, 0xa0, 0x03, 0x02, 0x01, 0x02, + 0x02, 0x14, 0x3c, 0x4a, 0xc9, 0xfc, 0x05, 0xa5, 0x6c, 0xaa, 0x52, 0x8a, + 0x71, 0xcb, 0xfc, 0xd4, 0xd1, 0x6e, 0x29, 0x8d, 0x6f, 0x01, 0x30, 0x05, + 0x06, 0x03, 0x2b, 0x65, 0x70, 0x30, 0x00, + /* validity SEQUENCE, correct length 30 */ + 0x30, 0x1e, 0x17, 0x0d, 0x32, + 0x36, 0x30, 0x39, 0x30, 0x32, 0x30, 0x34, 0x35, 0x34, 0x32, 0x36, 0x5a, + 0x17, 0x0d, 0x32, 0x36, 0x31, 0x30, 0x30, 0x32, 0x30, 0x34, 0x35, 0x34, + 0x32, 0x36, 0x5a, 0x30, 0x00, 0x30, 0x2a, 0x30, 0x05, 0x06, 0x03, 0x2b, + 0x65, 0x70, 0x03, 0x21, 0x00, 0x5e, 0x1b, 0x44, 0xb7, 0xe3, 0x7b, 0x3d, + 0xe7, 0x24, 0x66, 0x20, 0xaf, 0x31, 0x66, 0x61, 0xaf, 0x52, 0x68, 0xa2, + 0x62, 0xf4, 0x47, 0xe5, 0x93, 0x1c, 0x3e, 0xae, 0xab, 0xbc, 0x71, 0xca, + 0xc2, 0xa3, 0x58, 0x30, 0x56, 0x30, 0x35, 0x06, 0x03, 0x2a, 0x03, 0x04, + 0x04, 0x2e, 0x30, 0x00, 0x30, 0x2a, 0x30, 0x05, 0x06, 0x03, 0x2b, 0x65, + 0x70, 0x03, 0x21, 0x00, 0xf3, 0xf9, 0x4d, 0x74, 0x9e, 0xd9, 0x61, 0xed, + 0xfd, 0xb7, 0x9b, 0x6c, 0xff, 0x1e, 0x1c, 0xf5, 0xeb, 0x1b, 0x72, 0xcd, + 0xc5, 0x8c, 0xfc, 0x4e, 0x71, 0xcd, 0x82, 0x9c, 0x13, 0x16, 0xc4, 0xfb, + 0x30, 0x1d, 0x06, 0x03, 0x55, 0x1d, 0x0e, 0x04, 0x16, 0x04, 0x14, 0x7a, + 0x6a, 0xa2, 0xfd, 0x2e, 0xb6, 0x62, 0x22, 0x98, 0x18, 0x0b, 0x2d, 0x17, + 0xe1, 0xf3, 0x6b, 0x86, 0x01, 0x50, 0xb1, 0x30, 0x05, 0x06, 0x03, 0x2b, + 0x65, 0x70, 0x03, 0x41, 0x00, 0x0b, 0x53, 0x43, 0x87, 0x19, 0xc2, 0x2f, + 0x7d, 0x3c, 0x32, 0x8a, 0x7f, 0xfe, 0xbb, 0xd7, 0x2c, 0x27, 0xd8, 0x59, + 0x5b, 0x47, 0x3e, 0x5d, 0xd1, 0xc9, 0x63, 0x9b, 0x83, 0x74, 0x24, 0x75, + 0x3b, 0xdd, 0x8a, 0xe1, 0x59, 0xe8, 0xcd, 0xff, 0xd0, 0x09, 0xee, 0xc8, + 0x4c, 0xe2, 0x7c, 0x69, 0x49, 0xc2, 0xb3, 0xb2, 0x60, 0x7f, 0x9c, 0x6b, + 0x0e, 0x0c, 0x97, 0x64, 0xd7, 0x6e, 0x9b, 0x4e, 0x00 + }; + /* Byte-for-byte identical except offset 44: the validity SEQUENCE declares + * length 89, so it runs to offset 134 and covers subject (75), + * subjectPublicKeyInfo (77) and the head of the extensions (121). */ + static const byte overrunCert[] = { + 0x30, 0x82, 0x01, 0x19, 0x30, 0x81, 0xcc, 0xa0, 0x03, 0x02, 0x01, 0x02, + 0x02, 0x14, 0x3c, 0x4a, 0xc9, 0xfc, 0x05, 0xa5, 0x6c, 0xaa, 0x52, 0x8a, + 0x71, 0xcb, 0xfc, 0xd4, 0xd1, 0x6e, 0x29, 0x8d, 0x6f, 0x01, 0x30, 0x05, + 0x06, 0x03, 0x2b, 0x65, 0x70, 0x30, 0x00, + /* validity SEQUENCE, overrunning length 89 */ + 0x30, 0x59, 0x17, 0x0d, 0x32, + 0x36, 0x30, 0x39, 0x30, 0x32, 0x30, 0x34, 0x35, 0x34, 0x32, 0x36, 0x5a, + 0x17, 0x0d, 0x32, 0x36, 0x31, 0x30, 0x30, 0x32, 0x30, 0x34, 0x35, 0x34, + 0x32, 0x36, 0x5a, 0x30, 0x00, 0x30, 0x2a, 0x30, 0x05, 0x06, 0x03, 0x2b, + 0x65, 0x70, 0x03, 0x21, 0x00, 0x5e, 0x1b, 0x44, 0xb7, 0xe3, 0x7b, 0x3d, + 0xe7, 0x24, 0x66, 0x20, 0xaf, 0x31, 0x66, 0x61, 0xaf, 0x52, 0x68, 0xa2, + 0x62, 0xf4, 0x47, 0xe5, 0x93, 0x1c, 0x3e, 0xae, 0xab, 0xbc, 0x71, 0xca, + 0xc2, 0xa3, 0x58, 0x30, 0x56, 0x30, 0x35, 0x06, 0x03, 0x2a, 0x03, 0x04, + 0x04, 0x2e, 0x30, 0x00, 0x30, 0x2a, 0x30, 0x05, 0x06, 0x03, 0x2b, 0x65, + 0x70, 0x03, 0x21, 0x00, 0xf3, 0xf9, 0x4d, 0x74, 0x9e, 0xd9, 0x61, 0xed, + 0xfd, 0xb7, 0x9b, 0x6c, 0xff, 0x1e, 0x1c, 0xf5, 0xeb, 0x1b, 0x72, 0xcd, + 0xc5, 0x8c, 0xfc, 0x4e, 0x71, 0xcd, 0x82, 0x9c, 0x13, 0x16, 0xc4, 0xfb, + 0x30, 0x1d, 0x06, 0x03, 0x55, 0x1d, 0x0e, 0x04, 0x16, 0x04, 0x14, 0x7a, + 0x6a, 0xa2, 0xfd, 0x2e, 0xb6, 0x62, 0x22, 0x98, 0x18, 0x0b, 0x2d, 0x17, + 0xe1, 0xf3, 0x6b, 0x86, 0x01, 0x50, 0xb1, 0x30, 0x05, 0x06, 0x03, 0x2b, + 0x65, 0x70, 0x03, 0x41, 0x00, 0x0b, 0x53, 0x43, 0x87, 0x19, 0xc2, 0x2f, + 0x7d, 0x3c, 0x32, 0x8a, 0x7f, 0xfe, 0xbb, 0xd7, 0x2c, 0x27, 0xd8, 0x59, + 0x5b, 0x47, 0x3e, 0x5d, 0xd1, 0xc9, 0x63, 0x9b, 0x83, 0x74, 0x24, 0x75, + 0x3b, 0xdd, 0x8a, 0xe1, 0x59, 0xe8, 0xcd, 0xff, 0xd0, 0x09, 0xee, 0xc8, + 0x4c, 0xe2, 0x7c, 0x69, 0x49, 0xc2, 0xb3, 0xb2, 0x60, 0x7f, 0x9c, 0x6b, + 0x0e, 0x0c, 0x97, 0x64, 0xd7, 0x6e, 0x9b, 0x4e, 0x00 + }; + DecodedCert cert; + + /* The two encodings differ in the one length octet and nothing else. */ + ExpectIntEQ(sizeof(validCert), sizeof(overrunCert)); + ExpectIntEQ(validCert[44], 0x1e); + ExpectIntEQ(overrunCert[44], 0x59); + ExpectIntEQ(XMEMCMP(validCert, overrunCert, 44), 0); + ExpectIntEQ(XMEMCMP(validCert + 45, overrunCert + 45, + sizeof(validCert) - 45), 0); + + /* Sanity check: the well-formed encoding parses. */ + wc_InitDecodedCert(&cert, validCert, (word32)sizeof(validCert), NULL); + ExpectIntEQ(wc_ParseCert(&cert, CERT_TYPE, NO_VERIFY, NULL), 0); + wc_FreeDecodedCert(&cert); + + /* The overrunning validity length must be rejected. */ + wc_InitDecodedCert(&cert, overrunCert, (word32)sizeof(overrunCert), NULL); + ExpectIntEQ(wc_ParseCert(&cert, CERT_TYPE, NO_VERIFY, NULL), + WC_NO_ERR_TRACE(ASN_PARSE_E)); + wc_FreeDecodedCert(&cert); +#endif + return EXPECT_RESULT(); +} + int test_ParseCert_SM3wSM2_short_pubkey(void) { EXPECT_DECLS; diff --git a/tests/api/test_asn.h b/tests/api/test_asn.h index b4dd7c613a..7de628eb11 100644 --- a/tests/api/test_asn.h +++ b/tests/api/test_asn.h @@ -39,6 +39,7 @@ int test_DecodeCertExtensions_dup_certpol(void); int test_DecodeCertExtensions_empty_certpol(void); int test_DecodeCertExtensions_certpol_trailing_junk(void); int test_DecodeCertExtensions_empty_certpol_trailing(void); +int test_ParseCert_validity_length_overrun(void); int test_ParseCert_SM3wSM2_short_pubkey(void); int test_ParseCert_dnBufferBoundary(void); int test_wc_DecodeObjectId(void); @@ -69,6 +70,7 @@ int test_wc_AsnFeatureCoverage(void); TEST_DECL_GROUP("asn", test_DecodeCertExtensions_empty_certpol), \ TEST_DECL_GROUP("asn", test_DecodeCertExtensions_certpol_trailing_junk), \ TEST_DECL_GROUP("asn", test_DecodeCertExtensions_empty_certpol_trailing), \ + TEST_DECL_GROUP("asn", test_ParseCert_validity_length_overrun), \ TEST_DECL_GROUP("asn", test_ParseCert_SM3wSM2_short_pubkey), \ TEST_DECL_GROUP("asn", test_ParseCert_dnBufferBoundary), \ TEST_DECL_GROUP("asn", test_wc_DecodeObjectId), \ diff --git a/tests/api/test_mldsa.c b/tests/api/test_mldsa.c index c903bd0cc3..f4cae09f62 100644 --- a/tests/api/test_mldsa.c +++ b/tests/api/test_mldsa.c @@ -1471,9 +1471,12 @@ int test_mldsa_check_key(void) #if defined(WOLFSSL_HAVE_MLDSA) && \ defined(WOLFSSL_MLDSA_PUBLIC_KEY) +/* The AlgorithmIdentifier SEQUENCE holds only the algorithm OID, so its + * length covers those 11 bytes and no more - the outer SEQUENCE length is + * encoded on that basis. */ static const unsigned char ml_dsa_public_der[] = { #ifndef WOLFSSL_NO_ML_DSA_44 - 0x30, 0x82, 0x05, 0x32, 0x30, 0x0d, 0x06, 0x09, + 0x30, 0x82, 0x05, 0x32, 0x30, 0x0b, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x03, 0x11, 0x03, 0x82, 0x05, 0x21, 0x00, 0xBC, 0x5F, 0xF8, 0x10, 0xEB, 0x08, 0x90, 0x48, @@ -1641,7 +1644,7 @@ static const unsigned char ml_dsa_public_der[] = { 0xD8, 0x6D, 0xCA, 0x6B, 0xCD, 0x3D, 0x03, 0x8F, 0x9D, 0x3A, 0x7B, 0x66, 0xCB, 0xC7, 0xDF, 0x34 #elif !defined(WOLFSSL_NO_ML_DSA_65) - 0x30, 0x82, 0x07, 0xb2, 0x30, 0x0d, 0x06, 0x09, + 0x30, 0x82, 0x07, 0xb2, 0x30, 0x0b, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x03, 0x12, 0x03, 0x82, 0x07, 0xa1, 0x00, 0xD2, 0xFD, 0x03, 0xF3, 0xA1, 0xB7, 0xF6, 0x35, @@ -1889,7 +1892,7 @@ static const unsigned char ml_dsa_public_der[] = { 0x1A, 0xE7, 0x97, 0xF5, 0x6C, 0x63, 0x74, 0xBE, 0x0C, 0x79, 0x8C, 0x0C, 0xF3, 0x98, 0xF1, 0xED #else - 0x30, 0x82, 0x0a, 0x32, 0x30, 0x0d, 0x06, 0x09, + 0x30, 0x82, 0x0a, 0x32, 0x30, 0x0b, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x03, 0x13, 0x03, 0x82, 0x0a, 0x21, 0x00, 0x69, 0x24, 0xBB, 0x42, 0x57, 0xA7, 0xB9, 0xAF, diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 8e4b6c6c22..60c4b23970 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -1877,7 +1877,7 @@ int GetASN_Items(const ASNItem* asn, ASNGetData *data, int count, int complete, #endif /* Set the end index at each depth to be the length. */ - for (i=0; i= GET_ASN_MAX_DEPTH) { #ifdef WOLFSSL_DEBUG_ASN_TEMPLATE WOLFSSL_MSG("Depth in template too large"); #endif return ASN_PARSE_E; } + /* Determine the current depth by checking index against end indices. + * Don't go lower than the expected depth. Depths lower than first + * may not have an end index set yet. */ + while ((depth > asn[i].depth) && + ((depth <= asn[0].depth) || (idx == endIdx[depth]))) { + depth--; + } /* Keep track of minimum depth. */ if (depth < minDepth) { minDepth = depth; @@ -1917,10 +1922,23 @@ int GetASN_Items(const ASNItem* asn, ASNGetData *data, int count, int complete, } } - /* Check for end of data or not a choice and tag not matching. */ + /* A constructed item the data has not used up is not finished, so + * the template must not be moving out of it. */ + if (depth > asn[i].depth) { + #ifdef WOLFSSL_DEBUG_ASN_TEMPLATE + WOLFSSL_MSG_VSNPRINTF("Depth %d in template, %d in data: %d", + asn[i].depth, depth, i); + #endif + return ASN_PARSE_E; + } + + /* Check for data not reaching this depth, end of data, or not a choice + * and tag not matching. Data not this deep means the item's enclosing + * item was never entered and the item cannot be present. */ tmpW32Val = endIdx[depth]; XFENCE(); /* Prevent memory access */ - if (idx == tmpW32Val || (data[i].dataType != ASN_DATA_TYPE_CHOICE && + if ((depth < asn[i].depth) || idx == tmpW32Val || + (data[i].dataType != ASN_DATA_TYPE_CHOICE && (input[idx] & ~ASN_CONSTRUCTED) != asn[i].tag)) { if (asn[i].optional) { /* Skip over ASN.1 items underneath this optional item. */ @@ -2088,6 +2106,9 @@ int GetASN_Items(const ASNItem* asn, ASNGetData *data, int count, int complete, /* Store reference to data and length. */ data[i].data.ref.data = input + idx; data[i].data.ref.length = (word32)len; + /* Index left at the start of the content - the items that + * follow are parsed out of this one, so move into it. */ + depth++; continue; } @@ -33958,7 +33979,7 @@ static const ASNItem eccSpecifiedASN[] = { /* cofactor */ /* COFACTOR */ { 0, ASN_INTEGER, 0, 0, 1 }, /* hash */ -/* HASH_SEQ */ { 0, ASN_SEQUENCE, 0, 0, 1 }, +/* HASH_SEQ */ { 0, ASN_SEQUENCE, 1, 0, 1 }, }; enum { ECCSPECIFIEDASN_IDX_VER = 0, diff --git a/wolfcrypt/src/asn_orig.c b/wolfcrypt/src/asn_orig.c index dfcf1f6ef8..15c82c953e 100644 --- a/wolfcrypt/src/asn_orig.c +++ b/wolfcrypt/src/asn_orig.c @@ -762,6 +762,8 @@ int wc_RsaPublicKeyDecode_ex(const byte* input, word32* inOutIdx, word32 inSz, word32 seqEndIdx = inSz; #if defined(OPENSSL_EXTRA) || defined(RSA_DECODE_EXTRA) word32 localIdx; + word32 bitStrEndIdx; + int bitStrLen = 0; byte tag; #endif @@ -771,52 +773,83 @@ int wc_RsaPublicKeyDecode_ex(const byte* input, word32* inOutIdx, word32 inSz, if (GetSequence(input, inOutIdx, &length, inSz) < 0) return ASN_PARSE_E; + /* Everything parsed out of the SubjectPublicKeyInfo / RSAPublicKey + * SEQUENCE is bounded by it, not by the end of the buffer. */ + seqEndIdx = *inOutIdx + (word32)length; + if (seqEndIdx > inSz) + return ASN_PARSE_E; + #if defined(OPENSSL_EXTRA) || defined(RSA_DECODE_EXTRA) localIdx = *inOutIdx; - if (GetASNTag(input, &localIdx, &tag, inSz) < 0) + if (GetASNTag(input, &localIdx, &tag, seqEndIdx) < 0) return BUFFER_E; if (tag != ASN_INTEGER) { + word32 algEndIdx; + /* not from decoded cert, will have algo id, skip past */ - if (GetSequence(input, inOutIdx, &length, inSz) < 0) + if (GetSequence(input, inOutIdx, &length, seqEndIdx) < 0) return ASN_PARSE_E; - if (SkipObjectId(input, inOutIdx, inSz) < 0) + /* The AlgorithmIdentifier holds the algorithm OID and its parameters + * and nothing else - subjectPublicKey is a sibling of it, not part of + * it. Parse within the declared length and require it to be used up + * exactly: an over-long one would otherwise have its excess skipped + * silently, letting one encoding be read two ways. */ + algEndIdx = *inOutIdx + (word32)length; + if (algEndIdx > seqEndIdx) + return ASN_PARSE_E; + + if (SkipObjectId(input, inOutIdx, algEndIdx) < 0) return ASN_PARSE_E; /* Option NULL ASN.1 tag */ - if (*inOutIdx >= inSz) { + if (*inOutIdx >= seqEndIdx) { return BUFFER_E; } - localIdx = *inOutIdx; - if (GetASNTag(input, &localIdx, &tag, inSz) < 0) - return ASN_PARSE_E; - - if (tag == ASN_TAG_NULL) { - ret = GetASNNull(input, inOutIdx, inSz); - if (ret != 0) - return ret; - } - #ifdef WC_RSA_PSS - /* Skip RSA PSS parameters. */ - else if (tag == (ASN_SEQUENCE | ASN_CONSTRUCTED)) { - if (GetSequence(input, inOutIdx, &length, inSz) < 0) + if (*inOutIdx < algEndIdx) { + localIdx = *inOutIdx; + if (GetASNTag(input, &localIdx, &tag, algEndIdx) < 0) return ASN_PARSE_E; - *inOutIdx += length; + + if (tag == ASN_TAG_NULL) { + ret = GetASNNull(input, inOutIdx, algEndIdx); + if (ret != 0) + return ret; + } + #ifdef WC_RSA_PSS + /* Skip RSA PSS parameters. */ + else if (tag == (ASN_SEQUENCE | ASN_CONSTRUCTED)) { + if (GetSequence(input, inOutIdx, &length, algEndIdx) < 0) + return ASN_PARSE_E; + *inOutIdx += (word32)length; + } + #endif } - #endif + + if (*inOutIdx != algEndIdx) + return ASN_PARSE_E; /* should have bit tag length and seq next */ - ret = CheckBitString(input, inOutIdx, NULL, inSz, 1, NULL); + ret = CheckBitString(input, inOutIdx, &bitStrLen, seqEndIdx, 1, NULL); if (ret != 0) return ret; - if (GetSequence(input, inOutIdx, &length, inSz) < 0) + /* The subjectPublicKey BIT STRING ends the SubjectPublicKeyInfo - + * nothing may follow it - and holds the RSAPublicKey and nothing + * else. */ + bitStrEndIdx = *inOutIdx + (word32)bitStrLen; + if (bitStrEndIdx != seqEndIdx) + return ASN_PARSE_E; + + if (GetSequence(input, inOutIdx, &length, bitStrEndIdx) < 0) return ASN_PARSE_E; /* Calculate where the sequence should end for public key validation */ seqEndIdx = *inOutIdx + (word32)length; + if (seqEndIdx != bitStrEndIdx) + return ASN_PARSE_E; } #endif /* OPENSSL_EXTRA */ @@ -854,6 +887,11 @@ int wc_RsaPublicKeyDecode_ex(const byte* input, word32* inOutIdx, word32 inSz, return ASN_RSA_KEY_E; } + /* RSAPublicKey holds the modulus and the exponent and nothing else. */ + if (*inOutIdx != seqEndIdx) { + return ASN_PARSE_E; + } + return ret; } @@ -2680,7 +2718,16 @@ static int GetValidity(DecodedCert* cert, int verify, int maxIdx) badDate = ASN_BEFORE_DATE_E; /* continue parsing */ if (GetDate(cert, ASN_AFTER, verify, maxIdx) < 0) - return ASN_AFTER_DATE_E; + badDate = ASN_AFTER_DATE_E; /* continue parsing */ + + /* Validity holds notBefore and notAfter and nothing else. A length longer + * than the two dates would have the excess skipped silently, letting the + * certificate be read two ways - the subject and subjectPublicKeyInfo a + * strict parser sees are not the ones read here. Checked before any date + * error is returned: a date error is overridable by the verify callback + * and a malformed encoding must not be reclassified as one. */ + if (cert->srcIdx != (word32)maxIdx) + return ASN_PARSE_E; if (badDate != 0) return badDate; @@ -2766,8 +2813,16 @@ int wc_GetPubX509(DecodedCert* cert, int verify, int* badDate) if ( (ret = GetName(cert, ASN_ISSUER, (int)cert->sigIndex)) < 0) return ret; - if ( (ret = GetValidity(cert, verify, (int)cert->sigIndex)) < 0) + if ( (ret = GetValidity(cert, verify, (int)cert->sigIndex)) < 0) { + /* Only a date error is deferred for the caller to override. A + * malformed encoding is a failure of the certificate, not + * something a verify callback may wave through. */ + if ((ret != WC_NO_ERR_TRACE(ASN_BEFORE_DATE_E)) && + (ret != WC_NO_ERR_TRACE(ASN_AFTER_DATE_E))) { + return ret; + } *badDate = ret; + } #ifdef WOLFSSL_CERT_REQ } #endif @@ -7819,6 +7874,15 @@ int wc_EccPublicKeyDecode(const byte* input, word32* inOutIdx, int version, length; int curve_id = ECC_CURVE_DEF; word32 oidSum, localIdx; + /* Declared end of the outer SEQUENCE and of the AlgorithmIdentifier. + * Everything below is parsed within them, not within the buffer. There is + * no AlgorithmIdentifier in the private key format, so the outer SEQUENCE + * bounds the curve there. */ + word32 seqEndIdx; + word32 algEndIdx; + /* End of the SEC1 [1] public key wrapper. Without one the public key + * BIT STRING ends the outer SEQUENCE. */ + word32 pubEndIdx; byte tag, isPrivFormat = 0; if (input == NULL || inOutIdx == NULL || key == NULL || inSz == 0) @@ -7827,12 +7891,18 @@ int wc_EccPublicKeyDecode(const byte* input, word32* inOutIdx, if (GetSequence(input, inOutIdx, &length, inSz) < 0) return ASN_PARSE_E; + seqEndIdx = *inOutIdx + (word32)length; + if (seqEndIdx > inSz) + return ASN_PARSE_E; + algEndIdx = seqEndIdx; + pubEndIdx = seqEndIdx; + /* Check if ECC private key is being used and skip private portion */ - if (GetMyVersion(input, inOutIdx, &version, inSz) >= 0) { + if (GetMyVersion(input, inOutIdx, &version, seqEndIdx) >= 0) { isPrivFormat = 1; /* Type private key */ - if (*inOutIdx >= inSz) + if (*inOutIdx >= seqEndIdx) return ASN_PARSE_E; tag = input[*inOutIdx]; *inOutIdx += 1; @@ -7840,42 +7910,55 @@ int wc_EccPublicKeyDecode(const byte* input, word32* inOutIdx, return ASN_PARSE_E; /* Skip Private Key */ - if (GetLength(input, inOutIdx, &length, inSz) < 0) + if (GetLength(input, inOutIdx, &length, seqEndIdx) < 0) return ASN_PARSE_E; if (length > ECC_MAXSIZE) return BUFFER_E; *inOutIdx += (word32)length; /* Private Curve Header */ - if (*inOutIdx >= inSz) + if (*inOutIdx >= seqEndIdx) return ASN_PARSE_E; tag = input[*inOutIdx]; *inOutIdx += 1; if (tag != ECC_PREFIX_0) return ASN_ECC_KEY_E; - if (GetLength(input, inOutIdx, &length, inSz) <= 0) + if (GetLength(input, inOutIdx, &length, seqEndIdx) <= 0) + return ASN_PARSE_E; + + /* The [0] parameters wrapper holds the curve and nothing else. */ + algEndIdx = *inOutIdx + (word32)length; + if (algEndIdx > seqEndIdx) return ASN_PARSE_E; } /* Standard ECC public key */ else { - if (GetSequence(input, inOutIdx, &length, inSz) < 0) + if (GetSequence(input, inOutIdx, &length, seqEndIdx) < 0) + return ASN_PARSE_E; + + /* The AlgorithmIdentifier holds the algorithm OID and the curve + * parameters and nothing else - the public key BIT STRING is a + * sibling of it, not part of it. */ + algEndIdx = *inOutIdx + (word32)length; + if (algEndIdx > seqEndIdx) return ASN_PARSE_E; - ret = SkipObjectId(input, inOutIdx, inSz); + ret = SkipObjectId(input, inOutIdx, algEndIdx); if (ret != 0) return ret; } - if (*inOutIdx >= inSz) { + if (*inOutIdx >= seqEndIdx) { return BUFFER_E; } localIdx = *inOutIdx; - if (GetASNTag(input, &localIdx, &tag, inSz) == 0 && + if (GetASNTag(input, &localIdx, &tag, algEndIdx) == 0 && tag == (ASN_SEQUENCE | ASN_CONSTRUCTED)) { #ifdef WOLFSSL_CUSTOM_CURVES ecc_set_type* curve; int len; + int specVersion; char* point = NULL; ret = 0; @@ -7895,19 +7978,21 @@ int wc_EccPublicKeyDecode(const byte* input, word32* inOutIdx, #endif curve->id = ECC_CURVE_CUSTOM; - if (GetSequence(input, inOutIdx, &length, inSz) < 0) + if (GetSequence(input, inOutIdx, &length, algEndIdx) < 0) ret = ASN_PARSE_E; } if (ret == 0) { - GetInteger7Bit(input, inOutIdx, inSz); - if (GetSequence(input, inOutIdx, &length, inSz) < 0) + /* SpecifiedECDomain version - only version 2 and above may carry + * a hash algorithm. */ + specVersion = GetInteger7Bit(input, inOutIdx, algEndIdx); + if (GetSequence(input, inOutIdx, &length, algEndIdx) < 0) ret = ASN_PARSE_E; } if (ret == 0) { char* p = NULL; - SkipObjectId(input, inOutIdx, inSz); - ret = ASNToHexString(input, inOutIdx, &p, inSz, + SkipObjectId(input, inOutIdx, algEndIdx); + ret = ASNToHexString(input, inOutIdx, &p, algEndIdx, key->heap, DYNAMIC_TYPE_ECC_BUFFER); if (ret == 0) { #ifndef WOLFSSL_ECC_CURVE_STATIC @@ -7921,12 +8006,12 @@ int wc_EccPublicKeyDecode(const byte* input, word32* inOutIdx, if (ret == 0) { curve->size = (int)XSTRLEN(curve->prime) / 2; - if (GetSequence(input, inOutIdx, &length, inSz) < 0) + if (GetSequence(input, inOutIdx, &length, algEndIdx) < 0) ret = ASN_PARSE_E; } if (ret == 0) { char* af = NULL; - ret = ASNToHexString(input, inOutIdx, &af, inSz, + ret = ASNToHexString(input, inOutIdx, &af, algEndIdx, key->heap, DYNAMIC_TYPE_ECC_BUFFER); if (ret == 0) { #ifndef WOLFSSL_ECC_CURVE_STATIC @@ -7939,7 +8024,7 @@ int wc_EccPublicKeyDecode(const byte* input, word32* inOutIdx, } if (ret == 0) { char* bf = NULL; - ret = ASNToHexString(input, inOutIdx, &bf, inSz, + ret = ASNToHexString(input, inOutIdx, &bf, algEndIdx, key->heap, DYNAMIC_TYPE_ECC_BUFFER); if (ret == 0) { #ifndef WOLFSSL_ECC_CURVE_STATIC @@ -7952,17 +8037,17 @@ int wc_EccPublicKeyDecode(const byte* input, word32* inOutIdx, } if (ret == 0) { localIdx = *inOutIdx; - if (*inOutIdx < inSz && GetASNTag(input, &localIdx, &tag, inSz) + if (*inOutIdx < algEndIdx && GetASNTag(input, &localIdx, &tag, algEndIdx) == 0 && tag == ASN_BIT_STRING) { len = 0; - ret = GetASNHeader(input, ASN_BIT_STRING, inOutIdx, &len, inSz); + ret = GetASNHeader(input, ASN_BIT_STRING, inOutIdx, &len, algEndIdx); if (ret > 0) ret = 0; /* reset on success */ *inOutIdx += (word32)len; } } if (ret == 0) { - ret = ASNToHexString(input, inOutIdx, (char**)&point, inSz, + ret = ASNToHexString(input, inOutIdx, (char**)&point, algEndIdx, key->heap, DYNAMIC_TYPE_ECC_BUFFER); /* sanity check that point buffer is not smaller than the expected @@ -7999,7 +8084,7 @@ int wc_EccPublicKeyDecode(const byte* input, word32* inOutIdx, ((char*)curve->Gx)[curve->size * 2] = '\0'; ((char*)curve->Gy)[curve->size * 2] = '\0'; XFREE(point, key->heap, DYNAMIC_TYPE_ECC_BUFFER); - ret = ASNToHexString(input, inOutIdx, &o, inSz, + ret = ASNToHexString(input, inOutIdx, &o, algEndIdx, key->heap, DYNAMIC_TYPE_ECC_BUFFER); if (ret == 0) { #ifndef WOLFSSL_ECC_CURVE_STATIC @@ -8011,7 +8096,7 @@ int wc_EccPublicKeyDecode(const byte* input, word32* inOutIdx, } } if (ret == 0) { - curve->cofactor = GetInteger7Bit(input, inOutIdx, inSz); + curve->cofactor = GetInteger7Bit(input, inOutIdx, algEndIdx); #ifndef WOLFSSL_ECC_CURVE_STATIC curve->oid = NULL; @@ -8021,13 +8106,44 @@ int wc_EccPublicKeyDecode(const byte* input, word32* inOutIdx, curve->oidSz = 0; curve->oidSum = 0; - if (wc_ecc_set_custom_curve(key, curve) < 0) { - ret = ASN_PARSE_E; + /* Optional hash AlgorithmIdentifier - X9.62 SpecifiedECDomain + * places one after the optional cofactor. Not used here, but it + * has to be stepped over, and it has to lie inside the + * parameters. */ + if (*inOutIdx < algEndIdx) { + localIdx = *inOutIdx; + if ((GetASNTag(input, &localIdx, &tag, algEndIdx) == 0) && + (tag == (ASN_SEQUENCE | ASN_CONSTRUCTED))) { + if (specVersion < 2) { + ret = ASN_PARSE_E; + } + else if (GetSequence(input, inOutIdx, &length, + algEndIdx) < 0) { + ret = ASN_PARSE_E; + } + else { + *inOutIdx += (word32)length; + } + } } - key->deallocSet = 1; + /* The explicit parameters end the AlgorithmIdentifier. Checked + * before the curve is installed: on a malformed encoding any + * curve already on the key must stay reachable, and this one is + * freed below rather than orphaned. */ + if (ret == 0) { + if (*inOutIdx != algEndIdx) { + ret = ASN_PARSE_E; + } + else if (wc_ecc_set_custom_curve(key, curve) < 0) { + ret = ASN_PARSE_E; + } + else { + key->deallocSet = 1; - curve = NULL; + curve = NULL; + } + } } if (curve != NULL) wc_ecc_free_curve(curve, key->heap); @@ -8040,7 +8156,7 @@ int wc_EccPublicKeyDecode(const byte* input, word32* inOutIdx, } else { /* ecc params information */ - ret = GetObjectId(input, inOutIdx, &oidSum, oidIgnoreType, inSz); + ret = GetObjectId(input, inOutIdx, &oidSum, oidIgnoreType, algEndIdx); if (ret != 0) return ret; @@ -8052,23 +8168,41 @@ int wc_EccPublicKeyDecode(const byte* input, word32* inOutIdx, } } + /* The curve - named or explicit - ends the parameters holding it: the + * AlgorithmIdentifier, or the [0] wrapper of an ECPrivateKey. Anything + * left in them would be skipped silently, letting a length that reaches + * into the public key BIT STRING be read two ways. */ + if (*inOutIdx != algEndIdx) + return ASN_PARSE_E; + if (isPrivFormat) { /* Public Curve Header - skip */ - if (*inOutIdx >= inSz) + if (*inOutIdx >= seqEndIdx) return ASN_PARSE_E; tag = input[*inOutIdx]; *inOutIdx += 1; if (tag != ECC_PREFIX_1) return ASN_ECC_KEY_E; - if (GetLength(input, inOutIdx, &length, inSz) <= 0) + if (GetLength(input, inOutIdx, &length, seqEndIdx) <= 0) + return ASN_PARSE_E; + + /* The [1] wrapper holds the public key BIT STRING and nothing else, + * and is the last field of the ECPrivateKey. */ + pubEndIdx = *inOutIdx + (word32)length; + if (pubEndIdx != seqEndIdx) return ASN_PARSE_E; } /* key header */ - ret = CheckBitString(input, inOutIdx, &length, inSz, 1, NULL); + ret = CheckBitString(input, inOutIdx, &length, pubEndIdx, 1, NULL); if (ret != 0) return ret; + /* The BIT STRING ends what holds it: the [1] wrapper, or the outer + * SEQUENCE of a SubjectPublicKeyInfo. */ + if (*inOutIdx + (word32)length != pubEndIdx) + return ASN_PARSE_E; + /* This is the raw point data compressed or uncompressed. */ if (wc_ecc_import_x963_ex(input + *inOutIdx, (word32)length, key, curve_id) != 0) { diff --git a/wolfcrypt/test/test.c b/wolfcrypt/test/test.c index 68e63525e5..07609c6603 100644 --- a/wolfcrypt/test/test.c +++ b/wolfcrypt/test/test.c @@ -30148,26 +30148,34 @@ static wc_test_ret_t rsa_decode_test(RsaKey* keyPub) WOLFSSL_SMALL_STACK_STATIC const byte e[2] = { 0x00, 0x03 }; WOLFSSL_SMALL_STACK_STATIC const byte good[] = { 0x30, 0x06, 0x02, 0x01, 0x23, 0x02, 0x1, 0x03 }; + /* In SubjectPublicKeyInfo the subjectPublicKey BIT STRING is a sibling of + * the AlgorithmIdentifier SEQUENCE, not part of it, so that SEQUENCE's + * length must cover only the OID and any algorithm parameters. */ WOLFSSL_SMALL_STACK_STATIC const byte goodAlgId[] = { - 0x30, 0x18, 0x30, 0x16, + 0x30, 0x18, 0x30, 0x0b, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x03, 0x09, 0x00, 0x30, 0x06, 0x02, 0x01, 0x23, 0x02, 0x1, 0x03 }; WOLFSSL_SMALL_STACK_STATIC const byte goodAlgIdNull[] = { - 0x30, 0x1a, 0x30, 0x18, + 0x30, 0x1a, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x05, 0x00, 0x03, 0x09, 0x00, 0x30, 0x06, 0x02, 0x01, 0x23, 0x02, 0x1, 0x03 }; + /* Defect under test: the NULL algorithm parameters have a non-zero + * length. */ WOLFSSL_SMALL_STACK_STATIC const byte badAlgIdNull[] = { - 0x30, 0x1b, 0x30, 0x19, + 0x30, 0x1b, 0x30, 0x0e, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x05, 0x01, 0x00, 0x03, 0x09, 0x00, 0x30, 0x06, 0x02, 0x01, 0x23, 0x02, 0x1, 0x03 }; + /* Defect under test: an OCTET STRING where a BIT STRING is required. */ WOLFSSL_SMALL_STACK_STATIC const byte badNotBitString[] = { - 0x30, 0x18, 0x30, 0x16, + 0x30, 0x18, 0x30, 0x0b, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x04, 0x09, 0x00, 0x30, 0x06, 0x02, 0x01, 0x23, 0x02, 0x1, 0x03 }; + /* Defect under test: the BIT STRING length runs past the end of the + * data. */ WOLFSSL_SMALL_STACK_STATIC const byte badBitStringLen[] = { - 0x30, 0x18, 0x30, 0x16, + 0x30, 0x18, 0x30, 0x0b, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x03, 0x0a, 0x00, 0x30, 0x06, 0x02, 0x01, 0x23, 0x02, 0x1, 0x03}; WOLFSSL_SMALL_STACK_STATIC const byte badNoSeq[] = { @@ -30184,9 +30192,38 @@ static wc_test_ret_t rsa_decode_test(RsaKey* keyPub) WOLFSSL_SMALL_STACK_STATIC const byte badLength[] = { 0x30, 0x04, 0x02, 0x01, 0x23, 0x02, 0x1, 0x03 }; WOLFSSL_SMALL_STACK_STATIC const byte badBitStrNoZero[] = { - 0x30, 0x17, 0x30, 0x15, + 0x30, 0x17, 0x30, 0x0b, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, 0x03, 0x08, 0x30, 0x06, 0x02, 0x01, 0x23, 0x02, 0x1, 0x03 }; + /* Defect under test: the AlgorithmIdentifier SEQUENCE length runs past + * its OID and covers the subjectPublicKey BIT STRING that follows it. + * A parser resuming after the OID reads the key, one honouring the + * declared length looks for the key after it - one encoding, two + * readings. */ + WOLFSSL_SMALL_STACK_STATIC const byte badAlgIdLenLong[] = { + 0x30, 0x18, 0x30, 0x16, + 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, + 0x03, 0x09, 0x00, 0x30, 0x06, 0x02, 0x01, 0x23, 0x02, 0x1, 0x03 }; + /* Defect under test: as above, with NULL algorithm parameters. */ + WOLFSSL_SMALL_STACK_STATIC const byte badAlgIdNullLenLong[] = { + 0x30, 0x1a, 0x30, 0x18, + 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, + 0x05, 0x00, 0x03, 0x09, 0x00, 0x30, 0x06, 0x02, 0x01, 0x23, + 0x02, 0x1, 0x03 }; + /* Defect under test: an extra element inside the SubjectPublicKeyInfo + * after the subjectPublicKey BIT STRING, which must end it. */ + WOLFSSL_SMALL_STACK_STATIC const byte badSpkiTrailing[] = { + 0x30, 0x1a, 0x30, 0x0b, + 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, + 0x03, 0x09, 0x00, 0x30, 0x06, 0x02, 0x01, 0x23, 0x02, 0x1, 0x03, + 0x05, 0x00 }; + /* Defect under test: the AlgorithmIdentifier SEQUENCE has one byte left + * over after its OID and NULL parameters. */ + WOLFSSL_SMALL_STACK_STATIC const byte badAlgIdTrailing[] = { + 0x30, 0x1b, 0x30, 0x0e, + 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, + 0x05, 0x00, 0x00, + 0x03, 0x09, 0x00, 0x30, 0x06, 0x02, 0x01, 0x23, 0x02, 0x1, 0x03 }; ret = wc_InitRsaKey(keyPub, NULL); if (ret != 0) @@ -30323,16 +30360,13 @@ static wc_test_ret_t rsa_decode_test(RsaKey* keyPub) ret != WC_NO_ERR_TRACE(ASN_PARSE_E)) { ret = WC_TEST_RET_ENC_EC(ret); goto done; } - /* TODO: Shouldn't pass as the sequence length is too small. */ + /* The SEQUENCE is too small to hold both integers - the exponent lies + * outside it and must not be read from beyond its end. */ inSz = sizeof(badLength); inOutIdx = 0; ret = wc_RsaPublicKeyDecode(badLength, &inOutIdx, keyPub, inSz); -#ifndef WOLFSSL_ASN_TEMPLATE - if (ret != 0) -#else - if (ret != WC_NO_ERR_TRACE(ASN_PARSE_E)) -#endif - { + if (ret != WC_NO_ERR_TRACE(ASN_PARSE_E) && + ret != WC_NO_ERR_TRACE(ASN_RSA_KEY_E)) { ret = WC_TEST_RET_ENC_EC(ret); goto done; } @@ -30351,6 +30385,82 @@ static wc_test_ret_t rsa_decode_test(RsaKey* keyPub) ret = wc_InitRsaKey(keyPub, NULL); if (ret != 0) return WC_TEST_RET_ENC_EC(ret); + /* A constructed ASN.1 item must be exactly used up by the items parsed + * out of it. Each of these declares an AlgorithmIdentifier longer than + * its contents and must be rejected rather than have the excess + * silently skipped. */ + inSz = sizeof(badAlgIdLenLong); inOutIdx = 0; + ret = wc_RsaPublicKeyDecode(badAlgIdLenLong, &inOutIdx, keyPub, inSz); +#if defined(WOLFSSL_ASN_TEMPLATE) || defined(OPENSSL_EXTRA) || \ + defined(RSA_DECODE_EXTRA) + if (ret != WC_NO_ERR_TRACE(ASN_PARSE_E)) +#else + /* The original decoder without the decode extras never looks at the + * AlgorithmIdentifier - the SubjectPublicKeyInfo is read as a bare + * RSAPublicKey. */ + if (ret != WC_NO_ERR_TRACE(ASN_RSA_KEY_E)) +#endif + { + ret = WC_TEST_RET_ENC_EC(ret); goto done; + } + wc_FreeRsaKey(keyPub); + ret = wc_InitRsaKey(keyPub, NULL); + if (ret != 0) return WC_TEST_RET_ENC_EC(ret); + + inSz = sizeof(badAlgIdNullLenLong); inOutIdx = 0; + ret = wc_RsaPublicKeyDecode(badAlgIdNullLenLong, &inOutIdx, keyPub, inSz); +#if defined(WOLFSSL_ASN_TEMPLATE) || defined(OPENSSL_EXTRA) || \ + defined(RSA_DECODE_EXTRA) + if (ret != WC_NO_ERR_TRACE(ASN_PARSE_E)) +#else + /* The original decoder without the decode extras never looks at the + * AlgorithmIdentifier - the SubjectPublicKeyInfo is read as a bare + * RSAPublicKey. */ + if (ret != WC_NO_ERR_TRACE(ASN_RSA_KEY_E)) +#endif + { + ret = WC_TEST_RET_ENC_EC(ret); goto done; + } + wc_FreeRsaKey(keyPub); + ret = wc_InitRsaKey(keyPub, NULL); + if (ret != 0) return WC_TEST_RET_ENC_EC(ret); + + inSz = sizeof(badSpkiTrailing); inOutIdx = 0; + ret = wc_RsaPublicKeyDecode(badSpkiTrailing, &inOutIdx, keyPub, inSz); +#if defined(WOLFSSL_ASN_TEMPLATE) || defined(OPENSSL_EXTRA) || \ + defined(RSA_DECODE_EXTRA) + if (ret != WC_NO_ERR_TRACE(ASN_PARSE_E)) +#else + /* The original decoder without the decode extras never looks at the + * AlgorithmIdentifier - the SubjectPublicKeyInfo is read as a bare + * RSAPublicKey. */ + if (ret != WC_NO_ERR_TRACE(ASN_RSA_KEY_E)) +#endif + { + ret = WC_TEST_RET_ENC_EC(ret); goto done; + } + wc_FreeRsaKey(keyPub); + ret = wc_InitRsaKey(keyPub, NULL); + if (ret != 0) return WC_TEST_RET_ENC_EC(ret); + + inSz = sizeof(badAlgIdTrailing); inOutIdx = 0; + ret = wc_RsaPublicKeyDecode(badAlgIdTrailing, &inOutIdx, keyPub, inSz); +#if defined(WOLFSSL_ASN_TEMPLATE) || defined(OPENSSL_EXTRA) || \ + defined(RSA_DECODE_EXTRA) + if (ret != WC_NO_ERR_TRACE(ASN_PARSE_E)) +#else + /* The original decoder without the decode extras never looks at the + * AlgorithmIdentifier - the SubjectPublicKeyInfo is read as a bare + * RSAPublicKey. */ + if (ret != WC_NO_ERR_TRACE(ASN_RSA_KEY_E)) +#endif + { + ret = WC_TEST_RET_ENC_EC(ret); goto done; + } + wc_FreeRsaKey(keyPub); + ret = wc_InitRsaKey(keyPub, NULL); + if (ret != 0) return WC_TEST_RET_ENC_EC(ret); + /* Valid data cases. */ inSz = sizeof(good); inOutIdx = 0; ret = wc_RsaPublicKeyDecode(good, &inOutIdx, keyPub, inSz); @@ -44722,6 +44832,20 @@ static wc_test_ret_t ecc_decode_test(void) WOLFSSL_SMALL_STACK_STATIC const byte badPoint[] = { 0x30, 0x12, 0x30, 0x09, 0x06, 0x00, 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x03, 0x01, 0x07, 0x03, 0x03, 0x00, 0x04, 0x01 }; + /* This is "good" with the AlgorithmIdentifier SEQUENCE length changed + * from 0x13 to 0x57 so that it runs past the two OIDs and covers the + * public key BIT STRING that follows it. */ + WOLFSSL_SMALL_STACK_STATIC const byte badAlgIdLenLong[] = { + 0x30, 0x59, 0x30, 0x57, 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, + 0x3d, 0x02, 0x01, 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce, 0x3d, + 0x03, 0x01, 0x07, 0x03, 0x42, 0x00, 0x04, 0x55, 0xbf, 0xf4, + 0x0f, 0x44, 0x50, 0x9a, 0x3d, 0xce, 0x9b, 0xb7, 0xf0, 0xc5, + 0x4d, 0xf5, 0x70, 0x7b, 0xd4, 0xec, 0x24, 0x8e, 0x19, 0x80, + 0xec, 0x5a, 0x4c, 0xa2, 0x24, 0x03, 0x62, 0x2c, 0x9b, 0xda, + 0xef, 0xa2, 0x35, 0x12, 0x43, 0x84, 0x76, 0x16, 0xc6, 0x56, + 0x95, 0x06, 0xcc, 0x01, 0xa9, 0xbd, 0xf6, 0x75, 0x1a, 0x42, + 0xf7, 0xbd, 0xa9, 0xb2, 0x36, 0x22, 0x5f, 0xc7, 0x5d, 0x7f, + 0xb4 }; #if defined(WOLFSSL_SMALL_STACK) && !defined(WOLFSSL_NO_MALLOC) if (key == NULL) @@ -44836,6 +44960,17 @@ static wc_test_ret_t ecc_decode_test(void) goto done; } + /* A constructed ASN.1 item must be exactly used up by the items parsed + * out of it - the AlgorithmIdentifier here is longer than its two OIDs + * and the excess must not be silently skipped. */ + inSz = sizeof(badAlgIdLenLong); + inOutIdx = 0; + ret = wc_EccPublicKeyDecode(badAlgIdLenLong, &inOutIdx, key, inSz); + if (ret != WC_NO_ERR_TRACE(ASN_PARSE_E)) { + ret = WC_TEST_RET_ENC_EC(ret); + goto done; + } + inSz = sizeof(good); inOutIdx = 0; ret = wc_EccPublicKeyDecode(good, &inOutIdx, key, inSz); @@ -44891,6 +45026,42 @@ static const byte eccKeyExplicitCurve[] = { 0x38, 0x96, 0x29, 0x7d, 0x9c, 0xe1, 0x50, 0xbe, 0xac, 0xf0, 0x1d, 0x86, 0xf4, 0x2f, 0x65, 0x0b }; +/* eccKeyExplicitCurve with the AlgorithmIdentifier length raised from 0xae + * to 0xf2 so that it runs past the explicit ECParameters and covers the + * public key BIT STRING that follows it. */ +static const byte eccKeyExplicitCurveAlgIdLenLong[] = { + 0x30, 0x81, 0xf5, 0x30, 0x81, 0xf2, 0x06, 0x07, + 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x02, 0x01, 0x30, + 0x81, 0xa2, 0x02, 0x01, 0x01, 0x30, 0x2c, 0x06, + 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x01, 0x01, + 0x02, 0x21, 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe, 0xff, + 0xff, 0xfc, 0x2f, 0x30, 0x06, 0x04, 0x01, 0x00, + 0x04, 0x01, 0x07, 0x04, 0x41, 0x04, 0x79, 0xbe, + 0x66, 0x7e, 0xf9, 0xdc, 0xbb, 0xac, 0x55, 0xa0, + 0x62, 0x95, 0xce, 0x87, 0x0b, 0x07, 0x02, 0x9b, + 0xfc, 0xdb, 0x2d, 0xce, 0x28, 0xd9, 0x59, 0xf2, + 0x81, 0x5b, 0x16, 0xf8, 0x17, 0x98, 0x48, 0x3a, + 0xda, 0x77, 0x26, 0xa3, 0xc4, 0x65, 0x5d, 0xa4, + 0xfb, 0xfc, 0x0e, 0x11, 0x08, 0xa8, 0xfd, 0x17, + 0xb4, 0x48, 0xa6, 0x85, 0x54, 0x19, 0x9c, 0x47, + 0xd0, 0x8f, 0xfb, 0x10, 0xd4, 0xb8, 0x02, 0x21, + 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xfe, 0xba, 0xae, 0xdc, 0xe6, 0xaf, 0x48, 0xa0, + 0x3b, 0xbf, 0xd2, 0x5e, 0x8c, 0xd0, 0x36, 0x41, + 0x41, 0x02, 0x01, 0x01, 0x03, 0x42, 0x00, 0x04, + 0x3c, 0x4c, 0xc9, 0x5e, 0x2e, 0xa2, 0x3d, 0x49, + 0xcc, 0x5b, 0xff, 0x4f, 0xc9, 0x2e, 0x1d, 0x4a, + 0xc6, 0x21, 0xf6, 0xf3, 0xe6, 0x0b, 0x4f, 0xa9, + 0x9d, 0x74, 0x99, 0xdd, 0x97, 0xc7, 0x6e, 0xbe, + 0x14, 0x2b, 0x39, 0x9d, 0x63, 0xc7, 0x97, 0x0d, + 0x45, 0x25, 0x40, 0x30, 0x77, 0x05, 0x76, 0x88, + 0x38, 0x96, 0x29, 0x7d, 0x9c, 0xe1, 0x50, 0xbe, + 0xac, 0xf0, 0x1d, 0x86, 0xf4, 0x2f, 0x65, 0x0b +}; static wc_test_ret_t ecc_test_custom_curves(WC_RNG* rng) { @@ -44977,8 +45148,22 @@ static wc_test_ret_t ecc_test_custom_curves(WC_RNG* rng) inOutIdx = 0; ret = wc_EccPublicKeyDecode(eccKeyExplicitCurve, &inOutIdx, key, sizeof(eccKeyExplicitCurve)); - if (ret != 0) + if (ret != 0) { ret = WC_TEST_RET_ENC_EC(ret); + goto done; + } + + /* The explicit parameters must be exactly used up by the + * AlgorithmIdentifier holding them - an over-long length reaching into + * the public key BIT STRING must not have its excess skipped. */ + inOutIdx = 0; + ret = wc_EccPublicKeyDecode(eccKeyExplicitCurveAlgIdLenLong, &inOutIdx, key, + sizeof(eccKeyExplicitCurveAlgIdLenLong)); + if (ret != WC_NO_ERR_TRACE(ASN_PARSE_E)) { + ret = WC_TEST_RET_ENC_EC(ret); + goto done; + } + ret = 0; done: