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. ✅

[Frauschi]

Two comments. The senderNonce guard is the substantive one - I think it should return 400 like the transactionID guard directly above it, since no conforming CertRep exists without a senderNonce to echo. The other is a nit on the test harness. The fix itself is right, and both guards correctly land before the deenvelop.

[Frauschi]

The transactionID branch right above gets this right: with nothing to echo there is no conforming CertRep, so it answers 400. The same argument applies here. recipientNonce is copied from the senderNonce (RFC 8894 3.2.1), so with no senderNonce this CertRep cannot carry one either - send_cert_rep(..., NULL, 0, ...) leaves attrs.recipient_nonce = NULL and scep_msg.c:189 then drops the attribute. We end up signing a response that is non-conforming in exactly the way this PR exists to stop. Our own client rejects it unconditionally (scep_client.c:882, WOLFCERT_ERR_PROTOCOL), so the failInfo=badRequest never reaches the caller - it surfaces as a generic protocol error - and we have spent a CA private-key signature on an unauthenticated malformed request.

The convention argument in the description doesn't quite hold either: :815 (deenvelop failure) and :828 (unknown messageType) both have tid and snonce in hand and still return a plain 400. The split in this file is malformed request -> 400, PKI decision on a well-formed request -> signed CertRep, and this is the only send_cert_rep() call passing recipient_nonce = NULL. Can we fold this into the guard above and flip round 3 of check_required_attrs() to expect 400?

[Frauschi]

The loop breaks identically on a clean EOF, on realloc failure, and on the 5s SO_RCVTIMEO expiry, then parses a status out of whatever arrived - so a truncated response is indistinguishable from a complete one. In check_required_attrs() a transport hiccup would surface as a parse failure attributed to the round under test, which is annoying to chase. Worth tracking why the loop exited and returning -1 on the non-EOF cases. Bounds handling itself is fine: the reserve leaves room for the read plus the terminator even after a failed grow.