scep: require transactionID and senderNonce before dispatch

[yosuke-wolfssl]

Problem

The SCEP server validated only messageType before dispatch.
wolfcert_scep_parse_pki_message() leaves transactionID / senderNonce NULL when the attribute is absent rather than failing, and build_signed_attribs() silently omits any NULL field. A CMS-valid PKCSReq/RenewalReq omitting either therefore reached issuance, and the server signed and returned a CertRep missing transactionID and/or recipientNonce instead of refusing.

RFC 8894 §3.2.1 makes both mandatory in every pkiMessage; §3.2.1.1 and §3.2.1.5 require the response to echo them. Closes f-8042. Also removes a zero-length-transactionID collision in the pending queue and a NULL-to-memcmp() in pending_find().

Fix (src/scep/scep_server.c)

handle_pki_op() requires both attributes before decrypting or dispatching:

Request	Response
absent/empty transactionID	HTTP 400
absent/empty senderNonce	CertRep, pkiStatus 2, failInfo 2 (badRequest)

• The split is deliberate. With no transactionID there is nothing to echo, so no conforming CertRep exists — answering with one would reproduce the defect. This matches the file's existing convention: plain 400 for failures found before mt is known, signed FAILURE CertRep once tid/snonce are in hand.
• len == 0 matters. An empty attribute on the wire yields a non-NULL zero-length buffer, which a NULL-only check would miss.
• Guards precede wolfcert_scep_deenvelop(), so a rejected request costs no RSA private-key operation.

Tests (tests/integration/test_scep_roundtrip.c)

raw_http_status() is generalized to raw_http_req() (method, content type, binary body, returns the response body); a GET wrapper leaves the existing call sites unchanged. check_required_attrs() POSTs four hand-built pkiMessages:

Round	Expect
no transactionID	HTTP 400
zero-length transactionID	HTTP 400
no senderNonce	pkiStatus 2, failInfo 2, empty envelope, no recipientNonce
both present (control)	pkiStatus 0, envelope present, recipientNonce echoes

Verification

• Clean build under both CMake and autoconf, no warnings.
• 26/26 ctest pass; 26/26 clean under ASan + UBSan.
• Negative control per round: the three malformed rounds all fail against the unfixed server, each returning HTTP 200 — i.e. issuance. The control passes both fixed and unfixed, proving the harness reaches the issuance path.

[Copilot]

Pull request overview

This PR enforces mandatory SCEP transactionID and senderNonce attributes before dispatch and adds regression coverage.

Changes:

• Rejects missing or empty required attributes.
• Adds raw POST handling and malformed-request tests.
• Covers valid and invalid SCEP message cases.

Review findings:

• src/scep/scep_server.c:798 — wrap protocol errors to update thread-local diagnostics (moderate, 2 votes).
• tests/integration/test_scep_roundtrip.c:125 — handle short socket writes (moderate, 2 votes).

Reviewed changes

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

File	Summary
src/scep/scep_server.c	Validates required SCEP attributes before processing.
tests/integration/test_scep_roundtrip.c	Adds raw HTTP handling and required-attribute tests.

Suppressed comments (2)

src/scep/scep_server.c:802

• The new snonce_len == 0 path is not exercised: the added malformed round at check_required_attrs() omits sender_nonce entirely, while only transactionID gets a dedicated zero-length case. Please add an analogous non-NULL sender_nonce with length 0 and assert the same signed FAILURE response, otherwise the empty-senderNonce regression remains untested.

    if (snonce == NULL || snonce_len == 0) {

tests/integration/test_scep_roundtrip.c:780

• The new check also rejects a non-NULL senderNonce with snonce_len == 0, but this round only exercises an absent senderNonce (a.sender_nonce == NULL). A regression that drops the length check would still pass the suite; add a hand-built empty OCTET STRING senderNonce round and assert the same failure CertRep/no-recipientNonce behavior.

        else if (i == 2) {                  /* no senderNonce */
            a.transaction_id = tid;   a.transaction_id_len = sizeof(tid);
        }

---

💡 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 #22

Scan targets checked: wolfcert-bugs, wolfcert-src

No new issues found in the changed files. ✅