scep: reject multi-valued signed attributes in a pkiMessage

[yosuke-wolfssl]

Problem

wolfcert_scep_parse_pki_message walked the whole decoded signed-attribute list and, for each of the six recognised SCEP OIDs, allocated a copy and assigned it straight over the out-parameter with no check that the parameter was already populated. wolfSSL's PKCS#7 decoder does not de-duplicate attributes by OID, so a pkiMessage carrying N copies of one orphaned N-1 allocations — the caller only ever received, and freed, the last pointer. The peer also decided which copy won.

Both sides of the protocol reach it. Server: handle_pki_op parses a client-supplied body, and since the client self-signs its pkiMessage it controls the attribute SET while still satisfying wc_PKCS7_VerifySignedData. Client: scep_finish parses the response before the signer trust check, so an attacker answering a plaintext SCEP endpoint leaks memory on every attempt even though the forged CertRep is ultimately rejected. The 1 MiB body cap bounds each request, making this memory exhaustion under sustained traffic rather than an immediate crash.

The finding lists three production call sites; only two leak. wolfcert_scep_verify_next_ca_response passes NULL for every attribute out-parameter and never allocated.

Fix (src/scep/scep_msg.c)

• scep_attr_bit() maps a decoded attribute OID to one of six bits, so the OID list exists in exactly one place.
• seen accumulates those bits; a repeat returns WOLFCERT_ERR_PROTOCOL. RFC 8894 defines each attribute as singular, so rejecting also removes the attribute-confusion primitive that first-wins-skip would leave behind.
• The check runs before the DER unwrap, so a deliberately malformed copy cannot smuggle a second one past it.
• The reject path frees and NULLs every requested out-parameter plus the envelope, preserving the contract the function's existing early returns already had — callers need not free on error.
• The six near-identical copy branches collapse into a switch selecting the target out-parameter.

Closes f-8045.

Test harness

test_duplicate_signed_attrib in tests/unit/test_scep_msg.c drives wc_PKCS7_EncodeSignedData with a messageType plus two transactionID attributes — well-formed DER that is protocol-malformed, so it survives signature verification and reaches the walk. It requests every out-parameter and requires WOLFCERT_ERR_PROTOCOL with all of them returned NULL.

Verification

• Full suite 26/26; clean under -Werror.
• Negative control: the unfixed tree returns WOLFCERT_OK, and leaks reports the orphaned copy.
• Rollback branches proven live — neutering the messageType and signer-certificate frees yields exactly two orphans, 16 B and 768 B.

Not in this PR

The copy sites still leave the out-parameter NULL when WOLFCERT_XMALLOC fails instead of returning WOLFCERT_ERR_MEMORY. Pre-existing, unrelated to this finding, and unfiled.

[Copilot]

Pull request overview

This PR hardens SCEP pkiMessage parsing by rejecting duplicate signed attributes (per RFC 8894) to prevent memory leaks and attribute-confusion behavior when wolfSSL’s PKCS#7 decoder returns multiple attributes with the same OID.

Changes:

• Add per-OID “seen” tracking in wolfcert_scep_parse_pki_message and return WOLFCERT_ERR_PROTOCOL on duplicate signed attributes.
• Centralize OID→attribute mapping via scep_attr_bit() and simplify per-attribute output handling.
• Add a unit test that constructs a SignedData with duplicate transactionID attributes and asserts rejection with no leftover allocations.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/scep/scep_msg.c	Rejects duplicate SCEP signed attributes and rolls back any outputs on protocol-malformed input.
tests/unit/test_scep_msg.c	Adds a regression test that encodes a pkiMessage with duplicate transactionID to ensure it is rejected and outputs are cleared.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #20

Scan targets checked: wolfcert-bugs, wolfcert-src

No new issues found in the changed files. ✅

[Frauschi]

The duplicate-attribute fix looks right to me. One gap worth closing while you are in here: the repeated-SEQUENCE case is covered, but a single attribute carrying two values in its SET still slips through. Comment inline.

[Frauschi]

The seen bitmask catches the same OID appearing in two Attribute SEQUENCEs, but not two AttributeValues inside one attribute's SET. wc_PKCS7_ParseAttribs creates one PKCS7DecodedAttrib per SEQUENCE and copies the SET's content into value with valueSz covering the whole SET - it never iterates the values inside. So transactionID ::= SET { PrintableString "A", PrintableString "B" } yields one list node, one bit, and no collision. The unwrap below then reads the first value and the voff + vlen > a->valueSz check lets the trailing bytes through, so the message is accepted with *out_transaction_id == "A".

Not a leak, and the first-value pick is pre-existing rather than a regression - but it does mean a peer can still hand handle_pki_op and scep_finish two values for one attribute and have us act on one of them without complaint. RFC 8894 defines each of these as single-valued, so by the same argument the rest of this change makes, I'd require the value to fill the SET:

if (voff + vlen != a->valueSz) {
    rc = WOLFCERT_ERR(WOLFCERT_ERR_PROTOCOL, "scep",
                      "multi-valued signed attribute in pkiMessage");
    break;
}

That equality holds in both shapes the code handles, and it reuses the reject path you already added, so there is no extra rollback to write. Covering it in test_duplicate_signed_attrib needs a hand-encoded SignedData - wolfSSL's EncodeAttributes emits one SET per PKCS7Attrib and cannot produce the input.

[yosuke-wolfssl]

Thanks, I added — voff + vlen != a->valueSz takes the same WOLFCERT_ERR_PROTOCOL
path, so no extra rollback. Covered by test_multi_value_signed_attrib;
against the old guard it returns WOLFCERT_OK with tid == "A", as you said.

[yosuke-wolfssl]

Hi @Frauschi ,
I fixed it as you suggested.
Could you please check this again ?