scep: send a CertRep from every pkiMessage rejection path

[yosuke-wolfssl]

Problem

handle_pki_op() answered three malformed-message cases with a bare
text/plain HTTP 400 instead of a pkiMessage: a missing messageType
attribute, a pkcsPKIEnvelope that fails to decrypt for messageType
19/17, and an unrecognized messageType. All three occur after
wolfcert_scep_parse_pki_message() has verified the CMS signature and
recovered the transactionID and senderNonce, so RFC 8894 section 3.2.1
requires a signed CertRep with pkiStatus FAILURE and a failInfo. A
conforming SCEP client maps any non-200 to a generic transport error and
never surfaces the failInfo. Closes f-8033.

Fix (src/scep/scep_server.c)

New send_pki_failure() builds the CertRep from the transactionID and
senderNonce already in scope, and falls back to an HTTP 400 only when the
request carried no transactionID to echo.

Branch	Was	Now
mt == NULL	400 Bad Message	CertRep pkiStatus 2 / failInfo 2
de-envelop failure (19/17)	400 Cannot Decrypt	CertRep pkiStatus 2 / failInfo 2
unrecognized messageType	400 Bad Message	CertRep pkiStatus 2 / failInfo 2

Each clears keep_alive first, so the emitted Connection: header
matches the socket teardown that the non-OK return triggers. The four
pre-existing pkiStatus 2 sites in handle_enroll() and
handle_get_cert_initial() now share the same helper, which lets
handle_get_cert_initial() drop its signer-certificate parameters.

badRequest is used throughout: wolfcert_scep_deenvelop() cannot
distinguish an unsupported cipher from a corrupt or misaddressed
envelope, so the generic code is the honest one.

Tests

check_malformed_dispatch() in tests/integration/test_scep_roundtrip.c
POSTs three hand-built signed pkiMessages — no messageType, 19 over a
payload that is not an EnvelopedData, and 99 over a real envelope — and
requires each reply to parse as a CertRep echoing the transactionID and
senderNonce, with pkiStatus 2, failInfo 2, and no enveloped
messageData.

Verification

• Clean build under CMake and autoconf, no warnings.
• 26/26 ctest pass; ASan + UBSan clean.
• Negative control: reverting each branch individually to send_text()
  fails the new test.

[Copilot]

Pull request overview

Updates the SCEP server to return signed CertRep failure responses for malformed authenticated messages and adds integration tests.

Changes:

• Centralizes failure CertRep generation.
• Covers missing, undecryptable, and unknown message types.
• Adds malformed-message round-trip coverage.

Reviewed changes

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

File	Summary
src/scep/scep_server.c	Implements shared signed failure responses. Nit (3 votes): clear keep_alive before the fallback response when no transaction ID is available.
tests/integration/test_scep_roundtrip.c	Tests malformed SCEP dispatch responses.

Suppressed comments (1)

src/scep/scep_server.c:664

• The parser does not require a senderNonce, so a signed request can reach this helper with a valid transactionID but snonce == NULL/zero length. This still emits an HTTP 200 CertRep without recipientNonce (the builder omits it when the pointer is NULL), which violates RFC 8894 §3.2.1 and cannot be matched by a client. Treat a missing senderNonce like a missing transactionID and use the HTTP 400 fallback, or reject it before building the CertRep.

    if (tid == NULL || tid_len == 0) {
        send_text(s, fd, 400, "Bad Request", "text/plain", "");
        return WOLFCERT_ERR_PROTOCOL;
    }

---

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

Scan targets checked: wolfcert-bugs, wolfcert-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #23

Scan targets checked: wolfcert-bugs, wolfcert-src

No new issues found in the changed files. ✅

[Frauschi]

The refactor half of this is good - one send_pki_failure() for the four existing pkiStatus 2 sites, and handle_get_cert_initial() losing its signer-certificate parameters. My concern is the other half: converting the mt == NULL, de-envelop-failure and unknown-messageType branches from 400 to a signed CertRep.

Two things to settle first.

1. RFC 8894 doesn't require it. 3.2 makes transactionID, messageType and a fresh senderNonce mandatory in every message, so a message missing one isn't a pkiMessage at all; 3.3's "request is granted / request is rejected -> CertRep" language is about a request the CA can reason about. On malformed input the RFC is silent - there is no MUST here in either direction.

2. On the mt == NULL path the CertRep can't conform anyway. recipientNonce is copied from the request's senderNonce, and the parser leaves senderNonce NULL both when the peer omitted it and when it sent one we declined to decode (scep_msg.c:718 continues past any attribute failing its length checks). send_pki_failure() guards tid but passes snonce through, so we emit a CertRep with no recipientNonce - which our own client rejects unconditionally (scep_client.c:882). The failInfo never reaches the caller, which is the outcome this change exists to prevent.

The reference implementations all split it the other way. smallstep/scep - the protocol library behind both micromdm/scep and step-ca, the two peers our interop tests drive - rejects a missing transactionID, a missing or unknown messageType and a missing senderNonce inside ParsePKIMessage, before any dispatch, and its Fail() hard-wires RecipientNonce(msg.SenderNonce) so it structurally cannot build one without a senderNonce. Both servers turn a parse or decrypt failure into an HTTP error and keep the signed FAILURE CertRep for decisions on a well-formed, decrypted message. OpenXPKI draws the same line. There is a cost argument too: these branches sit before deenvelop on an unauthenticated request, so each malformed POST buys an RNG init and a CA private-key signature, and #22 deliberately keeps its guards ahead of the RSA decrypt for that reason.

This also collides with #22 directly. #22 inserts its transactionID/senderNonce guards at old line 792, between the mt == NULL block here and the deenvelop call - a textual conflict, and its guards land after this PR's mt == NULL branch, so they don't cover it and its snonce guard doesn't fix the missing recipientNonce above. My comment on #22 argued for 400 by citing :815 and :828 as the file's convention, and those are exactly the two sites this PR converts. Both positions can't stand.

Suggestion: keep the send_pki_failure() extraction and the four existing sites, drop the three new conversions, and settle the 400-vs-CertRep question once across both PRs. I'm open to being argued out of it on the de-envelop branch specifically, since tid and snonce genuinely are in hand there - though the badAlg/badRequest comment below is a reason it's awkward as a CertRep too.

The rest is per-site: one build-config break in the new test, and some smaller things.

[Frauschi]

This is the only unguarded AES-128 use in the file. SCEP_SRV_ENC_OID (scep_server.c:52-57) falls back to DES3b when WOLFSSL_AES_128/HAVE_AES_CBC are absent, and every other AES-CBC use here sits behind WOLFCERT_TEST_HAVE_CIPHER_OVERRIDE (:266) or an equivalent guard. On such a build wolfcert_scep_envelop() fails before the loop, all four rounds are skipped, and the REQUIRE() in main() takes the whole integration test down even though the server is correct.

Only round 2 needs a decryptable envelope and the server de-envelops whatever OID arrives, so a file-level TEST_SCEP_ENC_OID chosen the same way the server chooses SCEP_SRV_ENC_OID covers it.

[Frauschi]

The three branches do the same thing but spell the bookkeeping three ways: mt == NULL and the unknown-messageType branch assign into rc and then patch WOLFCERT_OK up to WOLFCERT_ERR_PROTOCOL, while this one routes through send_rc. Only the middle one has a reason to differ, since it has a meaningful rc to preserve.

And here it does the opposite of what send_rc is for: on a failing send, rc = send_rc (:820) overwrites the de-envelop error with the send error. Worth using send_rc in all three and deriving rc once per branch, so the one intentional difference is the only visible one.

[Frauschi]

The helper clears keep_alive on its 400 fallback but leaves it to the caller on the CertRep path, and the callers are split: handle_pki_op (:802, :816, :832) and handle_enroll (:688, :696) set it, while handle_enroll's queue-full and handle_get_cert_initial's badCertId branches deliberately do not. Every combination is coherent today, but split ownership means a future caller that returns non-OK without the assignment emits Connection: keep-alive on a socket we then close.

The parse-failure branch five lines up (:796) already does exactly that, as do issue_and_reply()'s "Bad CSR" 400 and the 500 sites. All pre-existing, but this PR introduces the invariant and leaves its siblings out of it. Setting keep_alive = 0 inside send_text() for status >= 400 would make it unforgettable. Note also that nothing asserts the header - WolfCertHttpResponse exposes none - so a branch answering with a CertRep and keep_alive still 1 would pass the suite.

[Frauschi]

We support exactly one content-encryption OID (SCEP_SRV_ENC_OID, :31-37), so a peer using AES-256-CBC, or 3DES against a NO_DES3 build, lands here. That is badAlg (unrecognised or unsupported algorithm), not badRequest (transaction not permitted or supported), and the difference is actionable: badAlg tells the client to re-run GetCACaps and retry with another cipher, badRequest tells it to give up.

wc_PKCS7_DecodeEnvelopedData() does separate ALGO_ID_E/UNSUPPORTED_ALGO from a generic decrypt failure, but wolfcert_scep_deenvelop() collapses both into WOLFCERT_ERR_WC. If this branch stays a CertRep, that distinction needs propagating - "badRequest is the honest one" only holds because we throw the information away one layer down.

[Frauschi]

The commit message says the three branches use failInfo "2, 0 and 2". All three pass "2", and the PR body's table says 2 everywhere, so only the commit message is stale. Worth fixing before merge so git log doesn't describe a badAlg we never send.

[Frauschi]

The parser takes the transactionID attribute's inner value whatever its tag, so a peer can send an OCTET STRING or a UTF8String of arbitrary bytes and we echo them back inside a PrintableString - enc_printable_n() (scep_msg.c:100) does no charset check. The result isn't valid DER and a strict client decoder rejects it.

The encoder is pre-existing; what changes here is reachability. Before, these three cases got a text/plain 400 and the transactionID was never re-encoded; now it is, on a message that has passed nothing but the CMS signature check. If these paths stay as CertReps, worth validating the recovered transactionID against the PrintableString repertoire and taking the 400 fallback when it doesn't conform.

[Frauschi]

r_sn/r_snl are parsed and then freed unexamined, so nothing pins the fresh senderNonce that 3.2.1 requires on these replies. Adding r_sn != NULL && r_snl == 16 to the assertion chain covers it, and holding the nonce across iterations would also catch two failure replies reusing one. Server side is fine - send_cert_rep() does generate a fresh one.