Repository navigation
Conversation
The SCEP server compared header names with a prefix strncasecmp, so a Content-Length-Foo field set the body length. It now uses the EST server's exact match, moved to http.c as wolfcert_http_hdr_is, and rejects whitespace before the colon (RFC 9112 section 5.1) like the EST server does. strncasecmp also folds case by the C locale, which breaks on a Turkish locale where 'I' does not lower to 'i'. The SCEP server's Connection value, the SCEP client's GetCACaps keywords and the EST server's Basic scheme now use wolfcert_ascii_ncasecmp, which leaves no strncasecmp in src/ and lets the three files drop <strings.h>.
wolfSSL encodes the country and serialNumber RDNs as PrintableString, and wolfcert_csr_build copied any value into them, so "C=U_" or a serialNumber with '@' produced malformed DER. Both now fail with WOLFCERT_ERR_BAD_ARG. emailAddress, which wolfSSL encodes as IA5String, now rejects bytes above 0x7F the same way.
enroll and getcert ignored the return of wolfcert_scep_get_ca_caps. A non-200 reply still means a CA without capabilities and the command goes on, now with a warning that the content cipher falls back to 3DES unless --content-cipher is given. Any other failure, such as an I/O or TLS error, stops the command instead of enrolling with empty caps.
There was a problem hiding this comment.
🟡 Changes recommended
Some truncated SCEP attribute encodings are still treated as absent instead of protocol errors.
1 open finding
What changed in this PR
Improves protocol parsing, CSR validation, timeout safety, CLI error handling, and CI reliability.
Changes:
- Tightens SCEP/HTTP parsing and allocation-error handling.
- Validates CSR fields, adds hash fallbacks, and uses 64-bit deadlines.
- Expands tests and hardens Zephyr workflow input handling.
| File | Description |
|---|---|
wolfcert/types.h |
Documents signature-hash fallback behavior. |
tests/unit/test_server_ca_store.c |
Enables the POSIX nanosleep declaration. |
tests/unit/test_scep_msg.c |
Tests malformed attributes and allocation failures. |
tests/unit/test_csr.c |
Tests CSR validation and ECC signature selection. |
tests/integration/test_scep_roundtrip.c |
Tests strict HTTP header parsing. |
src/scep/scep_server.c |
Tightens headers and maps OOM to HTTP 500. |
src/scep/scep_msg.c |
Distinguishes attribute errors and allocation failures. |
src/scep/scep_client.c |
Uses locale-independent capability matching. |
src/net_posix.c |
Converts monotonic deadlines to 64-bit values. |
src/internal.h |
Updates internal deadline and header APIs. |
src/http.c |
Adds shared exact header-name matching. |
src/est/est_server.c |
Reuses header matching and 64-bit deadlines. |
src/csr.c |
Validates ASN.1 strings and supports hash fallback. |
cli/wolfcert_client.c |
Handles GetCACaps failures explicitly. |
.github/actions/zephyr-workspace/action.yml |
Safely passes inputs and retries SDK downloads. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfcert_scep_parse_pki_message reported an attribute value that overruns its SET as "multi-valued", and a failed copy of an attribute or the signer certificate returned WOLFCERT_OK with the output left NULL, which surfaced later as a misleading AUTH or "Bad Message" error. A truncated value now has its own message, an empty one is rejected before it reaches XMALLOC(0), and an allocation failure returns WOLFCERT_ERR_MEMORY, which the SCEP server answers with 500. A value whose tag or length was cut short (e.g. 13, or 13 81) was skipped as if the attribute were absent, so the parse could still return WOLFCERT_OK. The SET and value headers are now read with der_read_tlv(), which also bounds the value by its SET and handles every definite length form instead of only 81 and 82, and any header it rejects is reported as truncated. test_parse_reports_oom fails each allocation in turn and requires every output to stay NULL and the result to be WOLFCERT_ERR_MEMORY, unless the failure came back from wolfSSL's PKCS#7 verify, which can report a failed allocation as a parse error. test_text_attrib_truncated covers the cut tag, the cut 81 and 82 lengths, an indefinite length, a cut SET length and a value overrunning its SET.
The CSR signature type picked SHA-384 for P-384 and SHA-512 for P-521, and honoured a preferred_hash of 384 or 512, without checking the wolfSSL build had those hashes, so signing failed on a build without them (est_csr_attrs_apply_roundtrip fails on main against one). Those choices now fall back to the key type's default, except that P-521 takes SHA-384 when only SHA-512 is missing. test_csr checks the hash of a P-256, P-384 and P-521 CSR against what the wolfSSL build provides.
test_server_ca_store.c includes tls_test_util.h, which calls nanosleep, but did not define _POSIX_C_SOURCE like the other tests that include it. Under -std=c11 glibc then hides the prototype, and GCC 14 and newer reject the implicit declaration.
wolfcert_mono_ms and the deadlines built on it were long, which on a 32-bit POSIX host wraps after about 24.8 days of uptime and then breaks the server's request deadline, the PHA wait and the client connect timeout.
The zephyr-workspace action expanded ${{ inputs.* }} straight into run
scripts; they now reach the shell as environment variables. The SDK
download also retries transient server errors, which have failed the
qemu_x86 job before.
Member
Author
|
@wolfSSL-Fenrir-bot review balanced |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #51
Scan targets checked: wolfcert-src, wolfcert-bugs
Coverage: 6 of 10 in-scope changed file(s) opened by the reviewer; not opened: src/scep/scep_client.c, tests/unit/test_csr.c, tests/unit/test_server_ca_store.c, wolfcert/types.h
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Balanced
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.
Various more minor fixes identified while doing the comment trimming of #49:
Content-Length-Fooset the body length) and rejects whitespace before the colon. Header and keyword comparisons no longer fold case by the C locale, so there is nostrncasecmpleft insrc/.wolfcert_csr_buildrejects a C or serialNumber that is not a PrintableString, and an emailAddress with bytes above 0x7F, withWOLFCERT_ERR_BAD_ARG, instead of emitting malformed DER.enrollandgetcertcheck the GetCACaps result: a non-200 reply warns and continues as a CA without capabilities, and any other failure stops the command.wolfcert_scep_parse_pki_messagerejects an attribute whose tag or length is cut short instead of skipping it as absent, tells truncated, empty and unallocated attributes apart, and returnsWOLFCERT_ERR_MEMORYon allocation failure, which the SCEP server answers with 500.nanosleep(GCC 14+ rejects the implicit declaration under-std=c11).envinstead of expanding them into scripts, and retries the SDK download.Behaviour changes: CSR subjects with the invalid values above now fail, and the SCEP CLI stops on GetCACaps transport errors.