Skip to content

scep: require transactionID and senderNonce before dispatch #22

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
+251 −25
Open

scep: require transactionID and senderNonce before dispatch #22

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
7 changes: 7 additions & 0 deletions src/scep/scep_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,6 +792,13 @@ static int handle_pki_op(WolfCertServer* s, int fd, const ScepRequest* req)
goto out;
}

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

@Frauschi Frauschi Sep 8, 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 guard is right, but it sits one branch too low. mt == NULL (:790) runs first, and in #23 that branch becomes a send_pki_failure() call that echoes the transactionID and senderNonce back in a signed CertRep. send_pki_failure() there guards only on tid, so a message carrying a transactionID but no senderNonce gets a CertRep with no recipientNonce: build_signed_attribs() drops the attribute on the NULL (scep_msg.c:189), and our own client rejects the reply at scep_client.c:882. I added a fifth round to #23's check_malformed_dispatch() (transactionID present, no senderNonce, no messageType) and got

PROBE tid-but-no-senderNonce: status=200 msgType=3 pkiStatus=2 recipientNonce=ABSENT

Merging this PR does not prevent that, because the mt == NULL branch is upstream of the guard. Nothing in #23 catches it either, since all four of its rounds pass a senderNonce.

The guard does not depend on mt, so swapping the two blocks closes it for good:

if (tid == NULL || tid_len == 0 || snonce == NULL || snonce_len == 0) { ... }

if (mt == NULL) { ... }

Then every path that echoes an attribute is downstream of the check by construction, #23 needs no senderNonce guard of its own, and the invariant lives in one place instead of two. Only one call site in #23 is upstream today, but that is an accident of where the branches happen to sit rather than something either PR guarantees.

Separately, on the description: the mutation claim does not hold. Dropping each of the four terms individually, tid_len == 0 and snonce_len == 0 each fail the suite, but tid == NULL and snonce == NULL change nothing. When an attribute is absent the parser leaves the pointer and the length both zeroed, so the two length terms carry all four rounds. The NULL terms are fine as belt and braces, the description just claims more coverage than the rounds provide.

s->keep_alive = 0;
send_text(s, fd, 400, "Bad Message", "text/plain", "");
rc = WOLFCERT_ERR_PROTOCOL;
goto out;
}

rc = wolfcert_scep_deenvelop(s->ca.cert_der, s->ca.cert_der_len,
s->ca.key_der, s->ca.key_der_len,
env.data, env.len, &csr, s->heap);
Expand Down
269 changes: 244 additions & 25 deletions tests/integration/test_scep_roundtrip.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,23 +59,46 @@ static void* server_thread(void* arg)
return NULL;
}

/* Send a raw HTTP/1.1 GET to the plain-HTTP SCEP server on the loopback port
* and return the numeric response status (or -1 on transport failure). An
* optional body (with a matching Content-Length) is sent when `body` is
* non-NULL. Used to exercise the server's malformed-GET rejection branches and
* the GET-with-body free path, which the client API cannot produce. */
static int raw_http_status(uint16_t port, const char* target, const char* body)
/* write() on a stream socket may return a short count. */
static int write_all_fd(int fd, const uint8_t* buf, size_t len)
{
size_t off = 0;

while (off < len) {
ssize_t w = write(fd, buf + off, len - off);
if (w <= 0)
return -1;
off += (size_t)w;
}

return 0;
}

/* Raw HTTP/1.1 request to the loopback SCEP server, for malformed-request
* branches the client API cannot produce. out_body, when set, is caller-freed. */
static int raw_http_req(uint16_t port, const char* method, const char* target,
const char* content_type,
const uint8_t* body, size_t body_len, int persistent,
uint8_t** out_body, size_t* out_body_len)
{
struct sockaddr_in addr;
struct timeval tv;
char req[512];
char resp[128];
size_t body_len = body != NULL ? strlen(body) : 0;
char hdr[512];
uint8_t* resp = NULL;
size_t resp_len = 0;
size_t resp_cap = 0;
const uint8_t* sep;
int eof = 0;
int fd;
int n;
ssize_t r;
int status = -1;

if (out_body != NULL && out_body_len != NULL) {
*out_body = NULL;
*out_body_len = 0;
}

fd = socket(AF_INET, SOCK_STREAM, 0);
if (fd < 0)
return -1;
Expand All @@ -94,35 +117,86 @@ static int raw_http_status(uint16_t port, const char* target, const char* body)
return -1;
}

if (body_len > 0)
n = snprintf(req, sizeof(req),
"GET %s HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n"
"Content-Length: %zu\r\n\r\n%s",
target, body_len, body);
if (body != NULL)
n = snprintf(hdr, sizeof(hdr),
"%s %s HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: %s\r\n"
"%s%s%sContent-Length: %zu\r\n\r\n",
method, target, persistent ? "keep-alive" : "close",
content_type != NULL ? "Content-Type: " : "",
content_type != NULL ? content_type : "",
content_type != NULL ? "\r\n" : "",
body_len);
else
n = snprintf(req, sizeof(req),
"GET %s HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n\r\n",
target);
if (n < 0 || (size_t)n >= sizeof(req)) {
n = snprintf(hdr, sizeof(hdr),
"%s %s HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: %s\r\n\r\n",
method, target, persistent ? "keep-alive" : "close");
if (n < 0 || (size_t)n >= sizeof(hdr)) {
close(fd);
return -1;
}
if (write(fd, req, (size_t)n) != (ssize_t)n) {
if (write_all_fd(fd, (const uint8_t*)hdr, (size_t)n) != 0 ||
(body_len > 0 && write_all_fd(fd, body, body_len) != 0)) {
close(fd);
return -1;

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

@yosuke-wolfssl yosuke-wolfssl Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. The loop now records a clean EOF and returns -1 otherwise, so a grow failure or an SO_RCVTIMEO expiry can no longer pass for a complete response:

if (r < 0)
    break;
if (r == 0) {
    eof = 1;
    break;
}
...
if (resp == NULL || !eof) {
    free(resp);
    return -1;
}

That also made a new check possible: a rejected pkiMessage sent over a keep-alive connection must still come back 400, which only holds if the server hangs up. Verified by mutation - removing both close mechanisms fails it.

}

r = read(fd, resp, sizeof(resp) - 1);
if (r > 0) {
resp[r] = '\0';
if (strncmp(resp, "HTTP/1.1 ", 9) == 0)
status = atoi(resp + 9);
/* Read to EOF so the whole response is available. A grow failure or a
* read error -- including the SO_RCVTIMEO expiry when the server holds the
* connection open -- must not pass for a complete response. */
for (;;) {
if (resp_len + 4096 + 1 > resp_cap) {
size_t want = resp_cap == 0 ? 8192 : resp_cap * 2;
uint8_t* grown = (uint8_t*)realloc(resp, want);
if (grown == NULL)
break;
resp = grown;
resp_cap = want;
}
r = read(fd, resp + resp_len, 4096);
if (r < 0)
break;
if (r == 0) {
eof = 1;
break;
}
resp_len += (size_t)r;
}

close(fd);

if (resp == NULL || !eof) {
free(resp);
return -1;
}
resp[resp_len] = '\0';

if (resp_len > 9 && memcmp(resp, "HTTP/1.1 ", 9) == 0)
status = atoi((const char*)resp + 9);

sep = (const uint8_t*)memmem(resp, resp_len, "\r\n\r\n", 4);
if (out_body != NULL && out_body_len != NULL && sep != NULL) {
size_t off = (size_t)(sep - resp) + 4;
size_t len = resp_len - off;
uint8_t* b = (uint8_t*)malloc(len + 1);
if (b != NULL) {
memcpy(b, resp + off, len);
b[len] = '\0';
*out_body = b;
*out_body_len = len;
}
}

free(resp);
return status;
}

/* GET wrapper preserving the original call sites. */
static int raw_http_status(uint16_t port, const char* target, const char* body)
{
return raw_http_req(port, "GET", target, NULL,
(const uint8_t*)body,
body != NULL ? strlen(body) : 0, 0, NULL, NULL);
}

/* Exercise the HTTP GET PKIOperation fallback (RFC 8894 section 4.1): with a
* caps set that does not advertise POSTPKIOperation the client carries the
* pkiMessage base64-encoded in a GET query and the server decodes and issues
Expand Down Expand Up @@ -724,6 +798,148 @@ static int check_getnextca_ca_id(const uint8_t* ca_der_buf, size_t ca_der_len)
return 0;
}

/* RFC 8894 section 3.2.1 requires transactionID and a fresh senderNonce in every
* pkiMessage; the client always sends both, so POST hand-built ones instead. */
static int check_required_attrs(WolfCertServer* s, const WolfCertKeyCfg* kcfg,
const uint8_t* ca_der_buf, size_t ca_der_len)
{
WolfCertCertMeta meta = { .subject_dn = "CN=scep-attrs" };
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];
size_t i;
int rc;

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,
AES128CBCb, &env, NULL);

memset(tid, 0x11, sizeof(tid));
memset(snonce, 0x22, sizeof(snonce));

/* Each round omits one required attribute; the last is the control that
* proves this raw-POST harness reaches the issuance path at all. */
for (i = 0; rc == WOLFCERT_OK && i < 5; ++i) {
WolfCertScepAttrs a = { .message_type = "19" };
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;
uint8_t* rsp = NULL;
size_t rsp_len = 0;
int st;
int ok;

if (i == 0) { /* no transactionID */
a.sender_nonce = snonce; a.sender_nonce_len = sizeof(snonce);
}
else if (i == 1) { /* zero-length transactionID */
a.transaction_id = tid; a.transaction_id_len = 0;
a.sender_nonce = snonce; a.sender_nonce_len = sizeof(snonce);
}
else if (i == 2) { /* no senderNonce */
a.transaction_id = tid; a.transaction_id_len = sizeof(tid);
}
else if (i == 3) { /* zero-length senderNonce */
a.transaction_id = tid; a.transaction_id_len = sizeof(tid);
a.sender_nonce = snonce; a.sender_nonce_len = 0;
}
else { /* control: both present */
a.transaction_id = tid; a.transaction_id_len = sizeof(tid);
a.sender_nonce = snonce; a.sender_nonce_len = sizeof(snonce);
}

rc = wolfcert_scep_build_pki_message(env.data, env.len,
signer, signer_len, kder.data, kder.len,
SHA256h, &a, &msg, NULL);
if (rc != WOLFCERT_OK)
break;

st = raw_http_req(wolfcert_server_port(s), "POST",
"/scep?operation=PKIOperation",
"application/x-pki-message",
msg.data, msg.len, 0, &rsp, &rsp_len);
wolfcert_buffer_free(&msg);

if (i < 4) {
/* Nothing to echo, so no conforming CertRep exists. */
ok = (st == 400);
}
else {
ok = (st == 200 && rsp != NULL &&
wolfcert_scep_parse_pki_message(rsp, rsp_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_tid != NULL && r_tidl == sizeof(tid) &&
memcmp(r_tid, tid, sizeof(tid)) == 0 &&
r_st != NULL && strcmp(r_st, "0") == 0 && renv.len > 0 &&
r_rn != NULL && r_rnl == sizeof(snonce) &&
memcmp(r_rn, snonce, sizeof(snonce)) == 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);
}

free(rsp);
if (!ok) {
fprintf(stderr, "FAIL %s:%d required-attrs round %zu (status %d)\n",
__FILE__, __LINE__, i, st);
rc = -1;
}
}

/* A rejected pkiMessage must close the connection even when the client
* asked to keep it. raw_http_req reports a status only once it sees EOF,
* so a server holding the socket open returns -1 here, not 400. */
if (rc == WOLFCERT_OK) {
WolfCertScepAttrs a = { .message_type = "19",
.sender_nonce = snonce,
.sender_nonce_len = sizeof(snonce) };
WolfCertBuffer msg = { 0 };

rc = wolfcert_scep_build_pki_message(env.data, env.len,
signer, signer_len, kder.data, kder.len,
SHA256h, &a, &msg, NULL);
if (rc == WOLFCERT_OK) {
if (raw_http_req(wolfcert_server_port(s), "POST",
"/scep?operation=PKIOperation",
"application/x-pki-message",
msg.data, msg.len, 1, NULL, NULL) != 400) {
fprintf(stderr, "FAIL %s:%d required-attrs kept the "
"connection open after reject\n",
__FILE__, __LINE__);
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 @@ -885,6 +1101,9 @@ int main(void)
REQUIRE(raw_http_status(wolfcert_server_port(s),
"/scep?operation=PKIOperation&message=QUJD", "XYZ") == 400); /* body freed */

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

#ifdef WOLFCERT_HAVE_ED25519
/* Ed25519 signer must be rejected cleanly (RFC 8894 requires RSA). */
WolfCertKeyCfg edcfg = { .type = WOLFCERT_KEY_ED25519, .param = 0,
Expand Down
Loading