F-8045 - Reject duplicate SCEP signed attributes - #20
Open
yosuke-wolfssl wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
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_messageand returnWOLFCERT_ERR_PROTOCOLon 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
transactionIDattributes 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.
- scep_attr_bit() maps a decoded attribute OID to one of six bits for messageType, pkiStatus, failInfo, transactionID, senderNonce and recipientNonce, and returns 0 for anything else. - wolfcert_scep_parse_pki_message() accumulates those bits in and returns WOLFCERT_ERR_PROTOCOL on a repeat; that path frees and NULLs every requested out-parameter and the envelope. - The six copy branches become a switch selecting the out-parameter, and the walk's locals move to the top of the function. - test_scep_msg gains make_dup_tid_signed() and test_duplicate_signed_attrib(), which build a SignedData carrying a messageType and two transactionID attributes, request every out-parameter, and require the parse to reject it with all of them returned NULL. Issue: F-8045
yosuke-wolfssl
force-pushed
the
fix/f_8045
branch
from
August 21, 2026 04:08
5b32b79 to
09d4ce9
Compare
wolfSSL-Fenrir-bot
approved these changes
Aug 21, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #20
Scan targets checked: wolfcert-bugs, wolfcert-src
No new issues found in the changed files. ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
wolfcert_scep_parse_pki_messagewalked 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_opparses a client-supplied body, and since the client self-signs its pkiMessage it controls the attribute SET while still satisfyingwc_PKCS7_VerifySignedData. Client:scep_finishparses 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_responsepasses 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.seenaccumulates those bits; a repeat returnsWOLFCERT_ERR_PROTOCOL. RFC 8894 defines each attribute as singular, so rejecting also removes the attribute-confusion primitive that first-wins-skip would leave behind.switchselecting the target out-parameter.Closes f-8045.
Test harness
test_duplicate_signed_attribintests/unit/test_scep_msg.cdriveswc_PKCS7_EncodeSignedDatawith amessageTypeplus twotransactionIDattributes — well-formed DER that is protocol-malformed, so it survives signature verification and reaches the walk. It requests every out-parameter and requiresWOLFCERT_ERR_PROTOCOLwith all of them returned NULL.Verification
-Werror.WOLFCERT_OK, andleaksreports the orphaned copy.messageTypeand 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_XMALLOCfails instead of returningWOLFCERT_ERR_MEMORY. Pre-existing, unrelated to this finding, and unfiled.