Skip to content

scep: send a CertRep from every pkiMessage rejection path #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yosuke-wolfssl wants to merge 1 commit into
base: main
Choose a base branch
from
Open

scep: send a CertRep from every pkiMessage rejection path #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 44 additions & 23 deletions src/scep/scep_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,24 @@ static int issue_and_reply(WolfCertServer* s, int fd,
return rc;
}

/* Answer a rejected pkiMessage with a signed CertRep carrying pkiStatus
* FAILURE and failInfo, per RFC 8894 section 3.2.1. */
static int send_pki_failure(WolfCertServer* s, int fd,

@Frauschi Frauschi Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const uint8_t* tid, size_t tid_len,
const uint8_t* snonce, size_t snonce_len,
const char* fail_info)
{
if (tid == NULL || tid_len == 0) {
s->keep_alive = 0;
send_text(s, fd, 400, "Bad Request", "text/plain", "");
return WOLFCERT_ERR_PROTOCOL;
}

/* A FAILURE CertRep carries no messageData, hence no envelope target. */

@Frauschi Frauschi Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return send_cert_rep(s, fd, NULL, 0, NULL, 0,
tid, tid_len, snonce, snonce_len, "2", fail_info);
}

/* Handle messageType=19 (PKCSReq) or 17 (RenewalReq) freshly arrived. */
static int handle_enroll(WolfCertServer* s, int fd, const char* mt,
const WolfCertBuffer* csr,
Expand All @@ -668,18 +686,16 @@ static int handle_enroll(WolfCertServer* s, int fd, const char* mt,
csr->data, csr->len, s->heap) != WOLFCERT_OK) {
/* Report the failure as a CertRep, then close the connection. */
s->keep_alive = 0;
return send_cert_rep(s, fd, NULL, 0, env_target, env_target_len,
tid, tid_len, snonce, snonce_len,
"2", "2" /* badRequest */);
return send_pki_failure(s, fd, tid, tid_len, snonce, snonce_len,
"2" /* badRequest */);
}

if (check_challenge(csr->data, csr->len, s->cfg_challenge,
s->heap) != WOLFCERT_OK) {
/* Report the failure as a CertRep, then close the connection. */
s->keep_alive = 0;
return send_cert_rep(s, fd, NULL, 0, env_target, env_target_len,
tid, tid_len, snonce, snonce_len,
"2", "2" /* badRequest */);
return send_pki_failure(s, fd, tid, tid_len, snonce, snonce_len,
"2" /* badRequest */);
}

if (s->cfg.scep_require_approval) {
Expand All @@ -694,9 +710,8 @@ static int handle_enroll(WolfCertServer* s, int fd, const char* mt,

if (add != WOLFCERT_OK) {
/* Queue full - fail rather than silently losing requests. */
return send_cert_rep(s, fd, NULL, 0, env_target, env_target_len,
tid, tid_len, snonce, snonce_len,
"2", "2" /* badRequest */);
return send_pki_failure(s, fd, tid, tid_len, snonce, snonce_len,
"2" /* badRequest */);
}
}

Expand All @@ -716,18 +731,13 @@ static int handle_enroll(WolfCertServer* s, int fd, const char* mt,
* to be pending forever. */
static int handle_get_cert_initial(WolfCertServer* s, int fd,
const uint8_t* tid, size_t tid_len,
const uint8_t* snonce, size_t snonce_len,
const uint8_t* signer_cert, size_t signer_cert_len)
const uint8_t* snonce, size_t snonce_len)
{
ScepPriv* p = (ScepPriv*)s->priv;
ScepPending* e = pending_find(p, tid, tid_len);
if (e == NULL) {
const uint8_t* env_target = signer_cert ? signer_cert : s->ca.cert_der;
size_t env_target_len = signer_cert ? signer_cert_len : s->ca.cert_der_len;

return send_cert_rep(s, fd, NULL, 0, env_target, env_target_len,
tid, tid_len, snonce, snonce_len,
"2", "4" /* badCertId: no such transaction */);
return send_pki_failure(s, fd, tid, tid_len, snonce, snonce_len,
"4" /* badCertId: no such transaction */);
}

/* Approve on first poll. A production implementation would hold
Expand Down Expand Up @@ -778,6 +788,7 @@ static int handle_pki_op(WolfCertServer* s, int fd, const ScepRequest* req)
uint8_t* signer_cert = NULL;
size_t signer_cert_len = 0;
WolfCertBuffer csr = { 0 };
int send_rc = WOLFCERT_OK;

int rc = wolfcert_scep_parse_pki_message(req->body, req->body_len, &env,
&tid, &tid_len, &snonce, &snonce_len, &rnonce, &rnonce_len,
Expand All @@ -788,7 +799,11 @@ static int handle_pki_op(WolfCertServer* s, int fd, const ScepRequest* req)
}

if (mt == NULL) {

@Frauschi Frauschi Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

send_text(s, fd, 400, "Bad Message", "text/plain", "");
s->keep_alive = 0;
rc = send_pki_failure(s, fd, tid, tid_len, snonce, snonce_len,
"2" /* badRequest */);

@Frauschi Frauschi Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (rc == WOLFCERT_OK)
rc = WOLFCERT_ERR_PROTOCOL;
goto out;
}

@Frauschi Frauschi Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand All @@ -798,7 +813,11 @@ static int handle_pki_op(WolfCertServer* s, int fd, const ScepRequest* req)
if (rc != WOLFCERT_OK && strcmp(mt, "20") != 0) {
/* Decryption matters for 19/17 (CSR inside); for 20 the payload
* is IssuerAndSubject which the server matches by txid anyway. */
send_text(s, fd, 400, "Cannot Decrypt", "text/plain", "");
s->keep_alive = 0;
send_rc = send_pki_failure(s, fd, tid, tid_len, snonce, snonce_len,
"2" /* badRequest */);
if (send_rc != WOLFCERT_OK)
rc = send_rc;
goto out;
}

Expand All @@ -807,12 +826,14 @@ static int handle_pki_op(WolfCertServer* s, int fd, const ScepRequest* req)
tid, tid_len, snonce, snonce_len);
}
else if (strcmp(mt, "20") == 0) {
rc = handle_get_cert_initial(s, fd, tid, tid_len, snonce, snonce_len,
signer_cert, signer_cert_len);
rc = handle_get_cert_initial(s, fd, tid, tid_len, snonce, snonce_len);
}
else {
send_text(s, fd, 400, "Bad Message", "text/plain", "");
rc = WOLFCERT_ERR_PROTOCOL;
s->keep_alive = 0;
rc = send_pki_failure(s, fd, tid, tid_len, snonce, snonce_len,
"2" /* badRequest */);
if (rc == WOLFCERT_OK)
rc = WOLFCERT_ERR_PROTOCOL;
}

out:
Expand Down
117 changes: 117 additions & 0 deletions tests/integration/test_scep_roundtrip.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,119 @@ static int check_getnextca_ca_id(const uint8_t* ca_der_buf, size_t ca_der_len)
return 0;
}

/* handle_pki_op's dispatch failures answer with a signed CertRep FAILURE, not
* a bare HTTP error. The client cannot produce these messages, so POST
* hand-built ones. Owns and frees everything it makes. */
static int check_malformed_dispatch(uint16_t port, const WolfCertKeyCfg* kcfg,

@Frauschi Frauschi Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const uint8_t* ca_der_buf, size_t ca_der_len)
{
static const uint8_t junk[4] = { 0x04, 0x02, 0xAB, 0xCD };
static const char* const msg_type[4] = { NULL, "19", "99", NULL };

WolfCertCertMeta meta = { .subject_dn = "CN=scep-dispatch" };
WolfCertKey* key = NULL;
WolfCertBuffer csr = { 0 };
WolfCertBuffer kder = { 0 };
WolfCertBuffer env = { 0 };
uint8_t* signer = NULL;
size_t signer_len = 0;
uint8_t tid[16], snonce[16];
char url[160];
size_t i;
int rc;

memset(tid, 0x33, sizeof(tid));
memset(snonce, 0x44, sizeof(snonce));
snprintf(url, sizeof(url),
"http://127.0.0.1:%u/scep?operation=PKIOperation", port);

rc = wolfcert_key_generate(kcfg, &key);
if (rc == WOLFCERT_OK)
rc = wolfcert_csr_build(key, &meta, &csr);
if (rc == WOLFCERT_OK)
rc = wolfcert_key_to_der(key, &kder);
if (rc == WOLFCERT_OK)
rc = wolfcert_scep_self_signed_rsa((RsaKey*)key->impl, csr.data,
csr.len, &signer, &signer_len, NULL);
if (rc == WOLFCERT_OK)
rc = wolfcert_scep_envelop(ca_der_buf, ca_der_len, csr.data, csr.len,

@Frauschi Frauschi Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

AES128CBCb, &env, NULL);

for (i = 0; rc == WOLFCERT_OK && i < 4; ++i) {
/* The last round omits the transactionID, which no CertRep can echo. */
WolfCertScepAttrs a = {
.transaction_id = i == 3 ? NULL : tid,
.transaction_id_len = i == 3 ? 0 : sizeof(tid),
.sender_nonce = snonce, .sender_nonce_len = sizeof(snonce),
.message_type = msg_type[i],
};
/* Round 2 needs an envelope the CA can open, or it trips the
* decrypt branch first. */
const uint8_t* content = i == 2 ? env.data : junk;
size_t content_len = i == 2 ? env.len : sizeof(junk);
WolfCertBuffer msg = { 0 };
WolfCertBuffer renv = { 0 };
uint8_t *r_tid = NULL, *r_sn = NULL, *r_rn = NULL, *r_sc = NULL;
size_t r_tidl = 0, r_snl = 0, r_rnl = 0, r_scl = 0;
char *r_mt = NULL, *r_st = NULL, *r_fi = NULL;
WolfCertHttpResponse resp = { 0 };

rc = wolfcert_scep_build_pki_message(content, content_len,
signer, signer_len, kder.data, kder.len,
SHA256h, &a, &msg, NULL);
if (rc == WOLFCERT_OK) {
WolfCertHttpRequest req = {
.method = "POST",
.url = url,
.content_type = "application/x-pki-message",
.body = msg.data,
.body_len = msg.len,
};
int ok = wolfcert_http_request(&req, &resp) == WOLFCERT_OK;

if (i == 3) {
ok = ok && resp.status_code == 400 && resp.body_len == 0;
}
else {
ok = ok && resp.status_code == 200 && resp.body != NULL;

ok = ok && wolfcert_scep_parse_pki_message(resp.body,
resp.body_len, &renv, &r_tid, &r_tidl, &r_sn,
&r_snl, &r_rn, &r_rnl, &r_mt, &r_st, &r_sc,
&r_scl, &r_fi, NULL) == WOLFCERT_OK;

ok = ok && r_mt != NULL && strcmp(r_mt, "3") == 0 &&
r_st != NULL && strcmp(r_st, "2") == 0 &&
r_fi != NULL && strcmp(r_fi, "2") == 0 &&
r_tid != NULL && r_tidl == sizeof(tid) &&
memcmp(r_tid, tid, sizeof(tid)) == 0 &&
r_rn != NULL && r_rnl == sizeof(snonce) &&
memcmp(r_rn, snonce, sizeof(snonce)) == 0 &&
renv.len == 0;
}

WOLFCERT_XFREE(r_tid, NULL); WOLFCERT_XFREE(r_sn, NULL);
WOLFCERT_XFREE(r_rn, NULL); WOLFCERT_XFREE(r_sc, NULL);
WOLFCERT_XFREE(r_mt, NULL); WOLFCERT_XFREE(r_st, NULL);
WOLFCERT_XFREE(r_fi, NULL);
wolfcert_buffer_free(&renv);
wolfcert_http_response_free(&resp);
if (!ok)
rc = -1;
}

wolfcert_buffer_free(&msg);
}

WOLFCERT_XFREE(signer, NULL);
wolfcert_buffer_free(&env);
wolfcert_buffer_free(&kder);
wolfcert_buffer_free(&csr);
wolfcert_key_free(key);

return rc;
}

int main(void)
{
REQUIRE(wolfcert_init(NULL) == WOLFCERT_OK);
Expand Down Expand Up @@ -789,6 +902,10 @@ int main(void)
/* The CA identifier belongs on GetNextCACert as well (RFC 8894 4.6.1). */
REQUIRE(check_getnextca_ca_id(ca_der->buffer, ca_der->length) == 0);

REQUIRE(check_malformed_dispatch(wolfcert_server_port(s), &kcfg,
ca_der->buffer, ca_der->length)
== WOLFCERT_OK);

/* One-shot SCEP over https:// must refuse to run unverified, the same rule
* the session open applies: verify_server is the only peer-verification
* switch, so leaving it off would complete a silent anonymous handshake. */
Expand Down
Loading