From 9147beccf8c5baaf4112da82bfbb22e60f09e00a Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Fri, 21 Aug 2026 11:29:44 +0900 Subject: [PATCH] scep: reject multi-valued signed attributes in a pkiMessage - wolfcert_scep_parse_pki_message() requires an attribute's value to fill its SET: voff + vlen must equal valueSz, and a mismatch takes the WOLFCERT_ERR_PROTOCOL reject path rather than skipping the attribute. - test_scep_msg gains make_signed_with_attribs(), which signs a caller-supplied PKCS7Attrib array; make_dup_tid_signed() and the new make_multi_value_tid_signed() build their attribute sets on it. - test_multi_value_signed_attrib() parses a pkiMessage whose single transactionID attribute holds two PrintableStrings and requires WOLFCERT_ERR_PROTOCOL with every out-parameter NULL. Issue: F-8045 --- src/scep/scep_msg.c | 208 ++++++++++++++++++++-------- tests/unit/test_scep_msg.c | 270 +++++++++++++++++++++++++++++++++++++ 2 files changed, 423 insertions(+), 55 deletions(-) diff --git a/src/scep/scep_msg.c b/src/scep/scep_msg.c index 615e115..2658896 100644 --- a/src/scep/scep_msg.c +++ b/src/scep/scep_msg.c @@ -601,6 +601,42 @@ WOLFCERT_TEST_VIS int wolfcert_scep_verify_next_ca_response(const uint8_t* resp_ return rc; } +/* One bit per SCEP signed attribute wolfCert reads. RFC 8894 gives each of + * them a single value, so the parser tracks which it has already seen. */ +#define SCEP_ATTR_MSG_TYPE 0x01 +#define SCEP_ATTR_PKI_STATUS 0x02 +#define SCEP_ATTR_FAIL_INFO 0x04 +#define SCEP_ATTR_TRANS_ID 0x08 +#define SCEP_ATTR_SENDER_NONCE 0x10 +#define SCEP_ATTR_RECIP_NONCE 0x20 + +static int scep_attr_bit(const PKCS7DecodedAttrib* a) +{ + if (a->oid == NULL) + return 0; + + if (a->oidSz == sizeof(OID_MSG_TYPE) && + memcmp(a->oid, OID_MSG_TYPE, a->oidSz) == 0) + return SCEP_ATTR_MSG_TYPE; + if (a->oidSz == sizeof(OID_PKI_STATUS) && + memcmp(a->oid, OID_PKI_STATUS, a->oidSz) == 0) + return SCEP_ATTR_PKI_STATUS; + if (a->oidSz == sizeof(OID_FAIL_INFO) && + memcmp(a->oid, OID_FAIL_INFO, a->oidSz) == 0) + return SCEP_ATTR_FAIL_INFO; + if (a->oidSz == sizeof(OID_TRANS_ID) && + memcmp(a->oid, OID_TRANS_ID, a->oidSz) == 0) + return SCEP_ATTR_TRANS_ID; + if (a->oidSz == sizeof(OID_SENDER_NONCE) && + memcmp(a->oid, OID_SENDER_NONCE, a->oidSz) == 0) + return SCEP_ATTR_SENDER_NONCE; + if (a->oidSz == sizeof(OID_RECIP_NONCE) && + memcmp(a->oid, OID_RECIP_NONCE, a->oidSz) == 0) + return SCEP_ATTR_RECIP_NONCE; + + return 0; +} + WOLFCERT_TEST_VIS int wolfcert_scep_parse_pki_message(const uint8_t* pki_der, size_t pki_len, WolfCertBuffer* out_envelope, uint8_t** out_transaction_id, size_t* out_tid_len, uint8_t** out_sender_nonce, size_t* out_snonce_len, @@ -608,13 +644,26 @@ WOLFCERT_TEST_VIS int wolfcert_scep_parse_pki_message(const uint8_t* pki_der, char** out_pki_status, uint8_t** out_signer_cert, size_t* out_signer_cert_len, char** out_fail_info, void* heap) { - PKCS7* p7 = wc_PKCS7_New(heap, WOLFCERT_DEVID_SOFTWARE); + PKCS7* p7; + PKCS7DecodedAttrib* a; + const byte* v; + char** out_str; + uint8_t** out_bin; + size_t* out_bin_len; + char* s; + uint8_t* b; + size_t off, vlen, voff; + int seen = 0; + int bit; + int rc; + + p7 = wc_PKCS7_New(heap, WOLFCERT_DEVID_SOFTWARE); if (p7 == NULL) return WOLFCERT_ERR_MEMORY; wc_PKCS7_AllowDegenerate(p7, 0); - int rc = wc_PKCS7_VerifySignedData(p7, (byte*)pki_der, (word32)pki_len); + rc = wc_PKCS7_VerifySignedData(p7, (byte*)pki_der, (word32)pki_len); if (rc != 0) { wc_PKCS7_Free(p7); return WOLFCERT_ERR_WC(rc, "scep", "VerifySignedData"); @@ -684,7 +733,20 @@ WOLFCERT_TEST_VIS int wolfcert_scep_parse_pki_message(const uint8_t* pki_der, if (out_fail_info) *out_fail_info = NULL; - for (PKCS7DecodedAttrib* a = p7->decodedAttrib; a != NULL; a = a->next) { + for (a = p7->decodedAttrib; a != NULL; a = a->next) { + bit = scep_attr_bit(a); + if (bit == 0) + continue; + + /* A repeated attribute is malformed: it would let the peer pick which + * copy the parser keeps and strand the copy it overwrote. */ + if ((seen & bit) != 0) { + rc = WOLFCERT_ERR(WOLFCERT_ERR_PROTOCOL, "scep", + "duplicate signed attribute in pkiMessage"); + break; + } + seen |= bit; + /* PKCS7DecodedAttrib.value can arrive in either of two shapes * depending on the wolfSSL version / producer: * (a) the outer `SET OF AttributeValue` (tag 0x31 + content), or @@ -694,8 +756,8 @@ WOLFCERT_TEST_VIS int wolfcert_scep_parse_pki_message(const uint8_t* pki_der, if (a->valueSz < 2 || a->value == NULL) continue; - const byte* v = a->value; - size_t off = 0; + v = a->value; + off = 0; if (v[0] == 0x31) { off = 2; @@ -714,8 +776,8 @@ WOLFCERT_TEST_VIS int wolfcert_scep_parse_pki_message(const uint8_t* pki_der, if (off + 2 > a->valueSz) continue; - size_t vlen = v[off + 1]; - size_t voff = off + 2; + vlen = v[off + 1]; + voff = off + 2; if (v[off + 1] == 0x81) { if (off + 3 > a->valueSz) continue; @@ -730,69 +792,105 @@ WOLFCERT_TEST_VIS int wolfcert_scep_parse_pki_message(const uint8_t* pki_der, voff = off + 4; } - if (voff + vlen > a->valueSz) - continue; + if (voff + vlen != a->valueSz) { + rc = WOLFCERT_ERR(WOLFCERT_ERR_PROTOCOL, "scep", + "multi-valued signed attribute in pkiMessage"); + break; + } off = voff; - if (a->oidSz == sizeof(OID_MSG_TYPE) && - memcmp(a->oid, OID_MSG_TYPE, a->oidSz) == 0 && out_message_type) { - char* s = (char*)WOLFCERT_XMALLOC(vlen + 1, heap); - if (s) { - memcpy(s, v + off, vlen); - s[vlen] = '\0'; - *out_message_type = s; - } - } - else if (a->oidSz == sizeof(OID_PKI_STATUS) && - memcmp(a->oid, OID_PKI_STATUS, a->oidSz) == 0 && out_pki_status) { - char* s = (char*)WOLFCERT_XMALLOC(vlen + 1, heap); - if (s) { - memcpy(s, v + off, vlen); - s[vlen] = '\0'; - *out_pki_status = s; - } + /* messageType, pkiStatus and failInfo are text; the transactionID and + * the two nonces are opaque octets carried with their length. */ + out_str = NULL; + out_bin = NULL; + out_bin_len = NULL; + + switch (bit) { + case SCEP_ATTR_MSG_TYPE: + out_str = out_message_type; + break; + case SCEP_ATTR_PKI_STATUS: + out_str = out_pki_status; + break; + case SCEP_ATTR_FAIL_INFO: + out_str = out_fail_info; + break; + case SCEP_ATTR_TRANS_ID: + out_bin = out_transaction_id; + out_bin_len = out_tid_len; + break; + case SCEP_ATTR_SENDER_NONCE: + out_bin = out_sender_nonce; + out_bin_len = out_snonce_len; + break; + case SCEP_ATTR_RECIP_NONCE: + out_bin = out_recipient_nonce; + out_bin_len = out_rnonce_len; + break; + default: + break; } - else if (a->oidSz == sizeof(OID_FAIL_INFO) && - memcmp(a->oid, OID_FAIL_INFO, a->oidSz) == 0 && out_fail_info) { - char* s = (char*)WOLFCERT_XMALLOC(vlen + 1, heap); - if (s) { + + if (out_str != NULL) { + s = (char*)WOLFCERT_XMALLOC(vlen + 1, heap); + if (s != NULL) { memcpy(s, v + off, vlen); s[vlen] = '\0'; - *out_fail_info = s; + *out_str = s; } } - else if (a->oidSz == sizeof(OID_TRANS_ID) && - memcmp(a->oid, OID_TRANS_ID, a->oidSz) == 0 && out_transaction_id) { - uint8_t* b = (uint8_t*)WOLFCERT_XMALLOC(vlen, heap); - if (b) { + else if (out_bin != NULL) { + b = (uint8_t*)WOLFCERT_XMALLOC(vlen, heap); + if (b != NULL) { memcpy(b, v + off, vlen); - *out_transaction_id = b; - *out_tid_len = vlen; + *out_bin = b; + *out_bin_len = vlen; } } - else if (a->oidSz == sizeof(OID_SENDER_NONCE) && - memcmp(a->oid, OID_SENDER_NONCE, a->oidSz) == 0 && out_sender_nonce) { - uint8_t* b = (uint8_t*)WOLFCERT_XMALLOC(vlen, heap); - if (b) { - memcpy(b, v + off, vlen); - *out_sender_nonce = b; - *out_snonce_len = vlen; - } + } + + wc_PKCS7_Free(p7); + + /* Leave nothing allocated behind on the reject path, as the early returns + * above do not either. */ + if (rc != WOLFCERT_OK) { + if (out_message_type != NULL) { + WOLFCERT_XFREE(*out_message_type, heap); + *out_message_type = NULL; } - else if (a->oidSz == sizeof(OID_RECIP_NONCE) && - memcmp(a->oid, OID_RECIP_NONCE, a->oidSz) == 0 && out_recipient_nonce) { - uint8_t* b = (uint8_t*)WOLFCERT_XMALLOC(vlen, heap); - if (b) { - memcpy(b, v + off, vlen); - *out_recipient_nonce = b; - *out_rnonce_len = vlen; - } + if (out_pki_status != NULL) { + WOLFCERT_XFREE(*out_pki_status, heap); + *out_pki_status = NULL; + } + if (out_fail_info != NULL) { + WOLFCERT_XFREE(*out_fail_info, heap); + *out_fail_info = NULL; + } + if (out_transaction_id != NULL) { + WOLFCERT_XFREE(*out_transaction_id, heap); + *out_transaction_id = NULL; + *out_tid_len = 0; } + if (out_sender_nonce != NULL) { + WOLFCERT_XFREE(*out_sender_nonce, heap); + *out_sender_nonce = NULL; + *out_snonce_len = 0; + } + if (out_recipient_nonce != NULL) { + WOLFCERT_XFREE(*out_recipient_nonce, heap); + *out_recipient_nonce = NULL; + *out_rnonce_len = 0; + } + if (out_signer_cert != NULL && out_signer_cert_len != NULL) { + WOLFCERT_XFREE(*out_signer_cert, heap); + *out_signer_cert = NULL; + *out_signer_cert_len = 0; + } + wolfcert_buffer_free(out_envelope); } - wc_PKCS7_Free(p7); - return WOLFCERT_OK; + return rc; } /* Build RFC 8894 section 3.3.2 IssuerAndSubject: diff --git a/tests/unit/test_scep_msg.c b/tests/unit/test_scep_msg.c index 9caa2f9..aa00b7b 100644 --- a/tests/unit/test_scep_msg.c +++ b/tests/unit/test_scep_msg.c @@ -1296,6 +1296,272 @@ static int test_envelop_cipher_oid(void) } #endif /* HAVE_AES_CBC && (WOLFSSL_AES_128 || WOLFSSL_AES_256) */ +static const byte scep_oid_msg_type[] = + { 0x06,0x0A,0x60,0x86,0x48,0x01,0x86,0xF8,0x45,0x01,0x09,0x02 }; +static const byte scep_oid_trans_id[] = + { 0x06,0x0A,0x60,0x86,0x48,0x01,0x86,0xF8,0x45,0x01,0x09,0x07 }; + +/* Sign fixed content with the caller's signed attributes. Taking them raw is + * what lets a test build an attribute set wolfCert itself never emits. + * Ownership of *out passes to the caller. */ +static int make_signed_with_attribs(const uint8_t* signer_cert, + size_t signer_cert_len, + const uint8_t* signer_key, + size_t signer_key_len, + PKCS7Attrib* attribs, int attribs_sz, + uint8_t** out, size_t* out_len) +{ + static const uint8_t content[3] = { 0xDE, 0xAD, 0xBE }; + PKCS7* p7 = NULL; + WC_RNG rng; + uint8_t* buf = NULL; + int have_rng = 0; + int ret = 0; + int n = 0; + + if (wc_InitRng(&rng) != 0) + return -1; + have_rng = 1; + + p7 = wc_PKCS7_New(NULL, INVALID_DEVID); + if (p7 == NULL) + ret = -1; + + if (ret == 0 && + wc_PKCS7_InitWithCert(p7, (byte*)signer_cert, (word32)signer_cert_len) != 0) + ret = -1; + + if (ret == 0) { + buf = (uint8_t*)malloc(8192); + if (buf == NULL) + ret = -1; + } + + if (ret == 0) { + p7->rng = &rng; + p7->privateKey = (byte*)signer_key; + p7->privateKeySz = (word32)signer_key_len; + p7->encryptOID = RSAk; + p7->hashOID = SHA256h; + p7->content = (byte*)content; + p7->contentSz = sizeof(content); + p7->signedAttribs = attribs; + p7->signedAttribsSz = (word32)attribs_sz; + + n = wc_PKCS7_EncodeSignedData(p7, buf, 8192); + if (n <= 0) + ret = -1; + } + + if (ret == 0) { + *out = buf; + *out_len = (size_t)n; + buf = NULL; + } + + free(buf); + if (p7 != NULL) + wc_PKCS7_Free(p7); + if (have_rng) + wc_FreeRng(&rng); + return ret; +} + +/* A messageType attribute plus two transactionID attributes whose values + * differ, each in its own Attribute SEQUENCE. */ +static int make_dup_tid_signed(const uint8_t* signer_cert, size_t signer_cert_len, + const uint8_t* signer_key, size_t signer_key_len, + uint8_t** out, size_t* out_len) +{ + static const byte msg_type[] = { 0x13, 0x02, '1', '9' }; + static const byte tid_a[] = { 0x13, 0x01, 'A' }; + static const byte tid_b[] = { 0x13, 0x01, 'B' }; + PKCS7Attrib attribs[3]; + + attribs[0].oid = scep_oid_msg_type; + attribs[0].oidSz = sizeof(scep_oid_msg_type); + attribs[0].value = msg_type; + attribs[0].valueSz = sizeof(msg_type); + attribs[1].oid = scep_oid_trans_id; + attribs[1].oidSz = sizeof(scep_oid_trans_id); + attribs[1].value = tid_a; + attribs[1].valueSz = sizeof(tid_a); + attribs[2].oid = scep_oid_trans_id; + attribs[2].oidSz = sizeof(scep_oid_trans_id); + attribs[2].value = tid_b; + attribs[2].valueSz = sizeof(tid_b); + + return make_signed_with_attribs(signer_cert, signer_cert_len, + signer_key, signer_key_len, + attribs, 3, out, out_len); +} + +/* One transactionID attribute holding two PrintableStrings. EncodeAttributes + * wraps a PKCS7Attrib's value bytes in a single SET without reading them, so + * both land inside one SET OF AttributeValue. */ +static int make_multi_value_tid_signed(const uint8_t* signer_cert, + size_t signer_cert_len, + const uint8_t* signer_key, + size_t signer_key_len, + uint8_t** out, size_t* out_len) +{ + static const byte msg_type[] = { 0x13, 0x02, '1', '9' }; + static const byte tid_two[] = { 0x13, 0x01, 'A', 0x13, 0x01, 'B' }; + PKCS7Attrib attribs[2]; + + attribs[0].oid = scep_oid_msg_type; + attribs[0].oidSz = sizeof(scep_oid_msg_type); + attribs[0].value = msg_type; + attribs[0].valueSz = sizeof(msg_type); + attribs[1].oid = scep_oid_trans_id; + attribs[1].oidSz = sizeof(scep_oid_trans_id); + attribs[1].value = tid_two; + attribs[1].valueSz = sizeof(tid_two); + + return make_signed_with_attribs(signer_cert, signer_cert_len, + signer_key, signer_key_len, + attribs, 2, out, out_len); +} + +/* RFC 8894 gives each SCEP signed attribute one value and wolfSSL's PKCS#7 + * decoder returns every copy, so a pkiMessage with two transactionID + * attributes must be rejected rather than letting the peer pick a winner. */ +static int test_duplicate_signed_attrib(void) +{ + uint8_t* ca_der = NULL; + size_t ca_len = 0; + uint8_t* ca_key = NULL; + size_t ca_key_len = 0; + uint8_t* msg = NULL; + size_t msg_len = 0; + WolfCertBuffer env = { 0 }; + uint8_t* tid = NULL; + size_t tid_len = 0; + uint8_t* snonce = NULL; + size_t snonce_len = 0; + uint8_t* rnonce = NULL; + size_t rnonce_len = 0; + char* mt = NULL; + char* ps = NULL; + char* fi = NULL; + uint8_t* signer = NULL; + size_t signer_len = 0; + int rc; + int tid_ok, snonce_ok, rnonce_ok, signer_ok, str_ok, env_ok; + + REQUIRE(make_ca(&ca_der, &ca_len, &ca_key, &ca_key_len) == 0); + REQUIRE(make_dup_tid_signed(ca_der, ca_len, ca_key, ca_key_len, + &msg, &msg_len) == 0); + + /* Every out-param is requested so the reject path has to roll back what it + * had already produced, the signer certificate and messageType included. */ + rc = wolfcert_scep_parse_pki_message(msg, msg_len, &env, + &tid, &tid_len, &snonce, &snonce_len, &rnonce, &rnonce_len, + &mt, &ps, &signer, &signer_len, &fi, NULL); + if (rc != WOLFCERT_ERR_PROTOCOL) { + fprintf(stderr, "duplicate transactionID accepted: rc=%d tid=%.*s\n", + rc, (int)tid_len, tid == NULL ? "" : (const char*)tid); + } + + /* Nothing may be left behind for the caller to leak. Capture every check, + * then free unconditionally, so a failing one cannot leak either (same + * pattern as test_non_success_has_no_envelope). */ + tid_ok = (tid == NULL && tid_len == 0); + snonce_ok = (snonce == NULL && snonce_len == 0); + rnonce_ok = (rnonce == NULL && rnonce_len == 0); + signer_ok = (signer == NULL && signer_len == 0); + str_ok = (mt == NULL && ps == NULL && fi == NULL); + env_ok = (env.data == NULL && env.len == 0); + + WOLFCERT_XFREE(tid, NULL); + WOLFCERT_XFREE(snonce, NULL); + WOLFCERT_XFREE(rnonce, NULL); + WOLFCERT_XFREE(signer, NULL); + WOLFCERT_XFREE(mt, NULL); + WOLFCERT_XFREE(ps, NULL); + WOLFCERT_XFREE(fi, NULL); + wolfcert_buffer_free(&env); + free(msg); + free(ca_der); + free(ca_key); + + REQUIRE(rc == WOLFCERT_ERR_PROTOCOL); + REQUIRE(tid_ok); + REQUIRE(snonce_ok); + REQUIRE(rnonce_ok); + REQUIRE(signer_ok); + REQUIRE(str_ok); + REQUIRE(env_ok); + return 0; +} + +/* The same ambiguity in the other encoding: one Attribute SEQUENCE whose SET + * carries two values. Nothing says which value applies, so the message is + * rejected instead of silently yielding the first. */ +static int test_multi_value_signed_attrib(void) +{ + uint8_t* ca_der = NULL; + size_t ca_len = 0; + uint8_t* ca_key = NULL; + size_t ca_key_len = 0; + uint8_t* msg = NULL; + size_t msg_len = 0; + WolfCertBuffer env = { 0 }; + uint8_t* tid = NULL; + size_t tid_len = 0; + uint8_t* snonce = NULL; + size_t snonce_len = 0; + uint8_t* rnonce = NULL; + size_t rnonce_len = 0; + char* mt = NULL; + char* ps = NULL; + char* fi = NULL; + uint8_t* signer = NULL; + size_t signer_len = 0; + int rc; + int tid_ok, snonce_ok, rnonce_ok, signer_ok, str_ok, env_ok; + + REQUIRE(make_ca(&ca_der, &ca_len, &ca_key, &ca_key_len) == 0); + REQUIRE(make_multi_value_tid_signed(ca_der, ca_len, ca_key, ca_key_len, + &msg, &msg_len) == 0); + + rc = wolfcert_scep_parse_pki_message(msg, msg_len, &env, + &tid, &tid_len, &snonce, &snonce_len, &rnonce, &rnonce_len, + &mt, &ps, &signer, &signer_len, &fi, NULL); + if (rc != WOLFCERT_ERR_PROTOCOL) { + fprintf(stderr, "multi-valued transactionID accepted: rc=%d tid=%.*s\n", + rc, (int)tid_len, tid == NULL ? "" : (const char*)tid); + } + + tid_ok = (tid == NULL && tid_len == 0); + snonce_ok = (snonce == NULL && snonce_len == 0); + rnonce_ok = (rnonce == NULL && rnonce_len == 0); + signer_ok = (signer == NULL && signer_len == 0); + str_ok = (mt == NULL && ps == NULL && fi == NULL); + env_ok = (env.data == NULL && env.len == 0); + + WOLFCERT_XFREE(tid, NULL); + WOLFCERT_XFREE(snonce, NULL); + WOLFCERT_XFREE(rnonce, NULL); + WOLFCERT_XFREE(signer, NULL); + WOLFCERT_XFREE(mt, NULL); + WOLFCERT_XFREE(ps, NULL); + WOLFCERT_XFREE(fi, NULL); + wolfcert_buffer_free(&env); + free(msg); + free(ca_der); + free(ca_key); + + REQUIRE(rc == WOLFCERT_ERR_PROTOCOL); + REQUIRE(tid_ok); + REQUIRE(snonce_ok); + REQUIRE(rnonce_ok); + REQUIRE(signer_ok); + REQUIRE(str_ok); + REQUIRE(env_ok); + return 0; +} + int main(void) { REQUIRE(test_static_mem_init() == 0); @@ -1335,6 +1601,10 @@ int main(void) return 1; if (test_signer_matches_any_bundle_cert()) return 1; + if (test_duplicate_signed_attrib()) + return 1; + if (test_multi_value_signed_attrib()) + return 1; #ifdef HAVE_ECC if (test_envelop_rejects_ecc_ra()) return 1;