Fenrir fixes#10
Open
Frauschi wants to merge 7 commits into
Open
Conversation
The chunked Transfer-Encoding read loop in parse_request decided the body was complete by scanning the whole accumulated buffer for the byte sequence "0\r\n" at any offset. Those bytes are not unique to the terminator: a chunk-size line whose value is a multiple of 16, such as "10\r\n" or "800\r\n", ends in them, and chunk data can contain them too. A single-buffer request masked the problem because the full body was already present when the false match fired. When a client streams a large CSR across several TCP segments, the false match stopped the loop before the remaining segments arrived. The decode pass then rejected the request as a protocol error or produced a truncated body, so enrollment failed. Walk the chunk framing positionally instead. Parse each chunk-size line, skip exactly that many payload bytes plus the trailing CRLF, and treat the message as complete only when a genuine zero-length chunk is reached at a chunk-header boundary. Keep reading until that point. Add a multi-segment regression test that delivers a well-formed chunked body in three timed segments with a multiple-of-16 chunk size, and assert the full body reaches the CSR layer. Fixes F-6876.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10
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. Findings are non-blocking.
The blocking and non-blocking session enroll variants base64-encode the CSR body with wolfcert_base64_encode_mime but left content_transfer_ encoding unset in their WolfCertHttpRequest initializers. The HTTP layer emits the Content-Transfer-Encoding header only when that field is set, so both session paths sent a base64 body with no header, contrary to RFC 7030 section 4.2.1. The one-shot enroll path already set it; the two session entry points were missed. Set content_transfer_encoding to base64 in both session initializers so they match the one-shot path. Extend the EST unit test to capture the request a session enroll puts on the wire and assert the header is present. Fixes F-6880.
csr__take_tl in the EST server and der_take_tl in the CSR-attributes parser validated a decoded DER length with "hdr + len > avail". A long-form length can be up to 0xFFFFFFFF, so on a 32-bit target that addition wraps: a length of FF FF FF FF makes hdr + len truncate to a small value that passes the check, and the parser then walks with a bogus multi-gigabyte length and reads past the end of a truncated, attacker-supplied CSR. The EST enrollment path base64-decodes the request body and reaches csr__has_attr_oid before issuance, so on a 32-bit build with csr-attribute enforcement and no Basic auth this is an unauthenticated out-of-bounds read. Switch both checks to the subtraction form already used by der_seq_len, which cannot overflow because the header size is guaranteed to be no greater than the available bytes at that point. Fixes F-6886
wolfcert_store_read_key decoded the private key PEM into a heap buffer and then released it with wolfcert_buffer_free, which is a plain free with no zeroization, leaving the plaintext key material in freed heap. The companion wolfcert_store_write_key already forces the buffer to zero before freeing it. Force the decoded PEM to zero before freeing it on the read path so both directions clear key material consistently. Fixes F-6888.
handle_enroll answered a signer/CSR key mismatch and a challenge password failure with a plain HTTP 400 or 403. By that point the server has already parsed the transaction ID, senderNonce and signer certificate from the request, so it can build a proper signed CertRep with pkiStatus FAILURE. A bare HTTP error instead is opaque to a conforming SCEP client, whose transport layer maps any non-200 to a generic HTTP error and never surfaces the SCEP failInfo. The pending queue paths in the same handler already reply with a CertRep FAILURE, so these two sites were inconsistent. Reply with send_cert_rep using pkiStatus 2 and failInfo 2 (badRequest) at both sites, reusing the transaction ID, senderNonce and envelope target already in scope. Tighten the challenge password roundtrip test to assert the client now sees a SCEP protocol failure rather than a transport level HTTP error. Fixes F-6881.
do_scep_round_trip generated the transactionID and senderNonce with an unchecked wc_InitRng_ex followed by two unchecked wc_RNG_GenerateBlock calls, and send_cert_rep did the same for its senderNonce. If RNG initialization fails, wc_RNG_GenerateBlock returns an error without writing the buffer, so the code went on to send uninitialized stack bytes on the wire as the transactionID and nonce, producing a request or response the peer cannot correctly match. The rest of the SCEP code in scep_msg.c already checks these calls and returns WOLFCERT_ERR_CRYPTO. Check the return of wc_InitRng_ex and each wc_RNG_GenerateBlock at both sites. On failure free the RNG when it was initialized, release the live envelope buffer, and return WOLFCERT_ERR_CRYPTO. Fixes F-6877 and F-6887.
has_cap scanned the entire GetCACaps response for the capability name as a substring. RFC 8894 section 3.5.2 defines the response as a newline-delimited list of exact tokens, so a server returning an unknown future token that contains a known one as a substring, such as Renewal-Extra or AESGCM, would make the client believe the server advertised Renewal or AES and possibly pick a code path the server does not support. Walk the body line by line, strip a trailing carriage return, and match a line only when it equals the capability token exactly and case-insensitively. Add a GetCACaps parsing test that serves substring-only tokens alongside one exact token and asserts only the exact token is detected. Fixes F-6882.
Frauschi
commented
Jul 21, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10
Scan targets checked: wolfcert-bugs, wolfcert-src
No new issues found in the changed files. ✅
philljj
requested changes
Jul 21, 2026
philljj
left a comment
philljj
left a comment
Contributor
There was a problem hiding this comment.
Looks good, just a few suggestions / questions.
| .accept = "application/pkcs7-mime", | ||
| .body = b64.data, .body_len = b64.len, | ||
| .max_response_bytes = s->max_body, | ||
| .heap = s->heap, |
Contributor
There was a problem hiding this comment.
suggestion: the original = aligned declaration was more readable.
| if (strncasecmp(body + i, needle, nl) == 0) { | ||
| size_t i = 0; | ||
|
|
||
| if (body == NULL) |
Contributor
There was a problem hiding this comment.
Why check body for null, but not needle?
Does len need bounds checks?
|
|
||
| /* Parse the chunk-size line at raw[ri..]. On success store the chunk | ||
| * length in *csz, the offset just past its terminating CRLF in *next, | ||
| * and return 0. Return 1 when the CRLF ending the size line has not |
Contributor
There was a problem hiding this comment.
pedantic suggestion: more readable when the return comments are line separated and at the end.
* returns 0 on success
* returns 1 when the CRLF ending the blah blah
* returns -1 when the line is malformed.
* */
| if (c == ';') | ||
| break; /* chunk-ext */ | ||
|
|
||
| int d = (c >= '0' && c <= '9') ? c - '0' |
Contributor
There was a problem hiding this comment.
suggestion, feel free to ignore: this ascii math block is now in 3 separate places in different forms. Might be tidier to refactor to avoid a copy paste mistake or related.
| if (csz == 0) | ||
| return 1; /* terminating zero-length chunk received */ | ||
|
|
||
| if (raw_len - ri < 2 || csz > raw_len - ri - 2) |
Contributor
There was a problem hiding this comment.
Does the raw_len - ri math need underflow protection?
The while loop checks ri < raw_len, but ri is updated ri = next; mid while loop.
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple Fenrir security findings across wolfCert’s EST and SCEP implementations, tightening protocol parsing/encoding, hardening crypto/RNG error paths, and adding regression tests to lock in the fixes.
Changes:
- Harden EST HTTP handling: robust chunked-transfer framing (shared chunk-size parser), overflow-safe DER TL parsing, and session enroll now emits
Content-Transfer-Encoding: base64. - Harden SCEP behavior: RFC-correct CertRep FAILURE responses for enrollment rejection, strict GetCACaps token matching, and full RNG return-code checking with negative-path fault injection.
- Defense-in-depth for key material: zeroize decoded PEM key buffers after import; add/extend unit + integration regression coverage.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_est.c | Adds on-the-wire header capture and exercises blocking + non-blocking session enroll paths for required base64 transfer encoding. |
| tests/integration/test_scep_roundtrip.c | Updates expected client error surfacing for CertRep FAILURE, adds GetCACaps token-matching test server, and adds RNG-failure regression coverage. |
| tests/integration/test_est_chunked_robustness.c | Adds multi-segment chunked regression to prevent premature termination/truncation. |
| src/store.c | Zeroizes private-key PEM buffer after decoding, aligning with write-path hygiene. |
| src/scep/scep_server.c | Adds RNG failure fault injection; checks RNG return values; returns signed CertRep FAILURE for enrollment rejection cases. |
| src/scep/scep_client.c | Switches capability detection to exact newline-delimited token matching; checks RNG init/generate return codes for txid/nonce. |
| src/internal.h | Extends test-only SCEP fault injection API documentation/signature. |
| src/est/est_server.c | Introduces shared chunk-size parser + positional chunk completeness scan; fixes DER TL overflow check in CSR parsing helper. |
| src/est/est_client.c | Ensures session enroll requests include Content-Transfer-Encoding: base64 (blocking + non-blocking). |
| src/est/csr_attrs.c | Fixes DER TL overflow check using subtraction form to avoid 32-bit overflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+217
to
+219
| ri = next; | ||
| if (csz == 0) | ||
| return 1; /* terminating zero-length chunk received */ |
Contributor
There was a problem hiding this comment.
Not clear to me if this critique is valid
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes a batch of Fenrir security findings across the EST and SCEP client and server. Every change is a targeted, minimal fix; each input-driven failure mode gets a negative regression test that fails before the fix and passes after.
What changed
"0\r\n"byte sequence anywhere in the buffer as the terminator, so a chunk-size line that is a multiple of 16 (or chunk data) falsely ended the read and truncated multi-segment requests. It now tracks chunk framing positionally and stops only at a genuine zero-length chunk. The chunk-size-line parse is factored into a singleread_chunk_sizehelper shared by the completeness scan and the decode pass so the two cannot drift apart.Content-Transfer-Encoding: base64. Both now set it, matching the one-shot path (RFC 7030 section 4.2.1).csr__take_tlandder_take_tlvalidated a decoded DER length withhdr + len > avail, which overflows on 32-bit and lets an oversized length pass, enabling an out-of-bounds read on an attacker CSR. Both now use the overflow-safe subtraction form.wolfcert_store_read_keyfreed the decoded private-key PEM without zeroizing it. It nowwc_ForceZeros the buffer before free, matching the write path.pkiStatus=2,failInfo=2badRequest) and close the connection, so a conforming client can process the failure.wc_InitRng_exandwc_RNG_GenerateBlockreturn values, risking uninitialized stack bytes on the wire. Both sites now check every return, free the RNG and any live buffer on failure, and returnWOLFCERT_ERR_CRYPTO.Renewal-Extra,AESGCM) was misread as advertising that capability. It now matches each newline-delimited token exactly (RFC 8894 section 3.5.2).Testing
New negative regression tests:
tests/integration/test_est_chunked_robustness.c).Content-Transfer-Encoding: base64on the wire (tests/unit/test_est.c).WOLFCERT_ERR_PROTOCOL(a parsed CertRep FAILURE) rather than a transport error (tests/integration/test_scep_roundtrip.c).tests/integration/test_scep_roundtrip.c).tests/integration/test_scep_roundtrip.c).F-6886 (32-bit-only overflow, not reachable on the 64-bit test host) and F-6888 (defense-in-depth zeroize) have no dedicated test; both were verified through the existing suites.
Notes
scep_gapsfailInfo work already in the base: the server now emitsfailInfo, and the client surfaces it viaWolfCertScepResult.fail_info. A rejected enrollment therefore returnsWOLFCERT_ERR_PROTOCOLfrom the simplewolfcert_scep_pkcs_reqwrapper; callers needing the specific reason readfail_infothrough the_exAPI. This is an intentional, RFC 8894-correct shift from the previousWOLFCERT_ERR_HTTP.