SCEP gap-filling#7
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #7
Scan targets checked: wolfcert-bugs, wolfcert-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #7
Scan targets checked: wolfcert-bugs, wolfcert-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #7
Scan targets checked: wolfcert-bugs, wolfcert-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
The renewal entry points took a new_key argument that was ignored ((void)new_key): the renewed public key is already carried in the CSR and the pkiMessage is signed with current_key. Remove the dead parameter from wolfcert_scep_renewal_req and _renewal_req_ex. Also parse the RFC 8894 section 3.2.1.4 failInfo attribute out of a FAILURE CertRep and populate WolfCertScepResult.fail_info, which was previously always left at -1. wolfcert_scep_parse_pki_message gains an out_fail_info output; all non-consuming callers pass NULL. The SCEP poll roundtrip test now asserts the client surfaces failInfo=4 (badCertId) for an unknown transaction.
Add a public helper to check a GetCACert response against a fingerprint obtained out of band before it is trusted as a CA anchor. It hashes the whole DER certificate with SHA-256 (or SHA-1 / SHA-512) and compares in constant time; WOLFCERT_SCEP_FP_AUTO selects the digest from the expected length. Returns WOLFCERT_ERR_AUTH on mismatch, WOLFCERT_ERR_UNSUPPORTED when the digest is not compiled into wolfSSL. MD5 is intentionally not offered. Unit-tested in test_scep_msg for match, single-bit tamper, AUTO dispatch, and length/argument misuse.
When the passed caps show the CA does not advertise POSTPKIOperation, carry the pkiMessage base64-encoded and percent-escaped in a GET `message` query parameter instead of POSTing it. run_pki_op chooses POST (the default, and whenever caps are unknown) or GET from the caps; a GET URL longer than WOLFCERT_SCEP_MAX_GET_URL (8 KiB) is refused with WOLFCERT_ERR_UNSUPPORTED so the caller falls back to a POST-capable CA. The in-tree test server learns to accept GET PKIOperation: it decodes the message parameter and dispatches it exactly like a POST body. Its query field gets a dedicated WOLFCERT_HTTP_QUERY_SZ (8 KiB) so the encoded message fits, and the client HTTP path ceiling is raised to match. Tests: test_scep_roundtrip enrolls once over the GET path end to end; test_scep_msg unit-tests URL construction and the oversize rejection. docs/ARCHITECTURE.md and the docs/EMBEDDED.md tunables table updated.
wolfSSL now exposes wc_ConstantCompare as a public API (alongside wc_ForceZero). Replace the three hand-rolled XOR-accumulate constant-time comparisons with it: - the CA fingerprint check (scep_client.c, dropping the local ct_diff), - the challenge-password check (scep_server.c), and - the HTTP Basic-auth credential check (est_server.c). Each keeps its existing length-mismatch guard, and wc_ConstantCompare returns 0 iff equal, so behaviour is unchanged.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #7
Scan targets checked: wolfcert-bugs, wolfcert-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| REQUIRE(url != NULL); | ||
| /* Capture the assertions, then free url before the REQUIREs so a failing | ||
| * check cannot leak the heap-allocated URL. */ | ||
| prefix_ok = (strncmp(url, pfx, strlen(pfx)) == 0); |
There was a problem hiding this comment.
🔵 [Low] test_pki_get_url: weak assertion does not verify round-trip correctness of URL encoding · Weak or missing assertions
prefix_ok checks only that the URL starts with the right static string; has_message checks only that the encoded portion is non-empty. A broken url_encode that omits escaping mandatory base64 characters (+, /, =) would produce a URL that passes both checks but fails server-side decoding.
Fix: Percent-decode then base64-decode the message= portion of the built URL and assert it equals the original small buffer.
| REQUIRE(wolfcert_scep_verify_ca_fingerprint(NULL, 0, sha256, sizeof(sha256), | ||
| WOLFCERT_SCEP_FP_SHA256) == WOLFCERT_ERR_BAD_ARG); | ||
|
|
||
| #ifndef NO_SHA |
There was a problem hiding this comment.
🔵 [Low] check_ca_fingerprint: no coverage for explicitly-requested algorithm unavailable in the build · Missing edge-case coverage on a function the PR also changed
The SHA-1 and SHA-512 blocks are wholly inside #ifndef NO_SHA / #ifdef WOLFSSL_SHA512 guards, so when those algorithms are absent the default: branch of wolfcert_scep_verify_ca_fingerprint's outer switch — which returns WOLFCERT_ERR_UNSUPPORTED — is never exercised. The same gap applies to WOLFCERT_SCEP_FP_AUTO with a matching-length fingerprint when the resolved algorithm is not compiled in.
Fix: Add #if defined(NO_SHA) / #if !defined(WOLFSSL_SHA512) blocks that assert WOLFCERT_ERR_UNSUPPORTED when those algorithms are explicitly requested but absent.
2 participants
SCEP (RFC 8894) gap-filling: GET PKIOperation, CA-fingerprint bootstrap, failInfo, and hardening
Summary
This branch closes several RFC 8894 (SCEP) conformance and robustness gaps in the client and the in-tree test server, and finishes with a small cross-cutting cleanup. It adds the HTTP GET
PKIOperationfallback, an out-of-band CA-fingerprint trust-bootstrap helper, surfaces the CertRep failInfo code, and moves the server's request handling off the stack. No functional behaviour of the existing POST-based flow changes.Four self-contained commits, each green on its own:
scep: surface CertRep failInfo and drop unused renewal new_key paramscep: add wolfcert_scep_verify_ca_fingerprint trust-bootstrap helperscep: add base64 HTTP GET PKIOperation fallback (RFC 8894 4.1)scep,est: use wc_ConstantCompare for constant-time comparisonsWhat's included
1. CertRep
failInfosurfaced; renewal API cleanupfailInfoattribute from a FAILURECertRepand exposes the code (0–4) instead of discarding it, so a caller can tell why an enrollment was rejected.new_keyparameter fromwolfcert_scep_renewal_req[_ex](it was(void)new_key;— the renewed key already travels inside the CSR).2. CA-fingerprint trust-bootstrap helper
wolfcert_scep_verify_ca_fingerprint()verifies a downloaded CA/RA certificate against a fingerprint obtained out of band — the standard SCEP trust-on-first-use bootstrap.WOLFCERT_SCEP_FP_AUTOselects the algorithm from the fingerprint length (20 / 32 / 64).3. HTTP GET
PKIOperationfallback (RFC 8894 §4.1) + server hardeningPOSTPKIOperation, the client carries the base64/percent-encodedpkiMessagein a GET query, capped byWOLFCERT_SCEP_MAX_GET_URL.PKIOperationand dispatches it exactly like a POST.path/querypoint into it, dropping peak request-handling stack from ~27 KB to under 1 KB (the GET message no longer lands on the stack). Buffer sizes are unchanged; only their storage moved. The EST server and client are untouched.Content-Lengthbody no longer leaks the body buffer when the decodedpkiMessageis installed.operation=/message=are matched as whole parameters (start-of-query or after&) rather than as substrings.500, matching its sibling error branches.4.
wc_ConstantCompareadoptionwc_ConstantCompareas a public API (alongsidewc_ForceZero). Replaces the three hand-rolled XOR-accumulate constant-time comparisons (CA fingerprint, challenge password, HTTP Basic-auth) with it. Net −16 lines; behaviour is identical (returns 0 iff equal).Public API changes
wolfcert_scep_verify_ca_fingerprint()+WolfCertScepFpAlgenumfailInfocode surfaced on the response statusnew_keyparameter removed fromwolfcert_scep_renewal_req[_ex]Build requirement
This adds one implicit dependency: wolfCert now calls
wc_ConstantCompare, so the linked wolfSSL must provide it (i.e. not built withWOLFSSL_NO_CONST_CMP). This mirrors the assumption already made forwc_ForceZero, so no new configure/check_config.hgate was added.Testing
ctest), both CMake and autoconf.message=, malformed percent-escape, invalid base64) driven over a raw socket; the GET-with-body leak-prevention path; SHA-512 fingerprint (explicit + AUTO).Notes for reviewers
roundtrip; the allocation uses theheaphint, so static-memory pools are still honoured.docs/EMBEDDED.mdand theinternal.hbuffer comments were updated to reflect the SCEP request buffer moving to the heap.