Skip to content

scep: keep the caps-driven signing hash within the wolfSSL build - #34

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_8024
Open

yosuke-wolfssl wants to merge 1 commit into
wolfSSL:mainfrom
yosuke-wolfssl:fix/f_8024

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

Problem

pick_hash_oid() turned the peer's GetCACaps answer straight into a PKCS#7
hashOID without intersecting it with the digests wolfSSL was actually built
with. wolfCert hard-requires only SHA-256 (wolfcert/check_config.h);
SHA-512/SHA-384 are optional, and wolfSSL makes them independently selectable.

Against a CA advertising SHA-512, a wolfSSL built --disable-sha512 selects
SHA512h, wc_PKCS7_SignedDataGetEncAlgoId() leaves algoId == 0, and
wc_PKCS7_EncodeSignedData() returns BAD_FUNC_ARG. Every SCEP enrollment
fails with a cryptic wolfSSL error instead of falling back to the SHA-256 that
RFC 8894 makes mandatory for both ends. pick_hash_oid() backs scep_prepare(),
so this covers all one-shot and session SCEP message construction.

Closes f-8024.

Fix (src/scep/scep_client.c)

#ifdef WOLFSSL_SHA512
    if (caps->sha512)
        return SHA512h;
#endif
#ifdef WOLFSSL_SHA384
    if (caps->sha384)
        return SHA384h;
#endif

A digest this build lacks falls through to the existing return SHA256h. This
mirrors the AES-128 / 3DES gating already applied to the caps-driven content
cipher a few lines below, which was the one caps-driven selection left ungated.

Tests (tests/integration/test_scep_roundtrip.c)

Two more enrollments in the existing scep_roundtrip, with sha512 and then
sha384 set on a copy of the fetched capabilities. Caps are a client-side
input, so no server change is needed — check_get_fallback() already clears
post_pki_operation on a copy the same way. No new binary, no buildsystem edit.

Verification

  • Default build: 29/29 ctest pass, clean under -Werror.
  • wolfSSL built --disable-sha512 --disable-sha384: scep_roundtrip fails at
    test_scep_roundtrip.c:1196 before the fix (BAD_FUNC_ARG, nothing POSTed)
    and passes after.
  • Compile-only diff, so no local sanitizer run; CI's Linux ASan/UBSan jobs cover
    that class.

Intentionally not in this PR

  • Wire-level digest assertion. The new cases assert enrollment succeeds,
    which stays true if the caps were ignored and SHA-256 chosen — so the default
    build exercises the selection branches without verifying their effect. The
    fallback branch the fix targets is covered by the constrained build above.
    A check_content_cipher_wire() analogue for the signing hash was weighed and
    declined as disproportionate to a four-line fix.
  • src/csr.c. ecdsa_sig_for_curve() and sig_type_for_hash() return
    CTC_SHA384* / CTC_SHA512* unguarded. That path is caller-driven (the app
    picked P-521 or asked for a 512-bit hash) rather than peer-driven, and it is
    reachable: est_csr_attrs_apply_roundtrip fails on the SHA-384-less build
    both before and after this change. Tracked separately.

- pick_hash_oid() returns SHA512h only under WOLFSSL_SHA512 and
  SHA384h only under WOLFSSL_SHA384; otherwise it falls through to
  SHA256h.
- test_scep_roundtrip enrolls twice more, once with caps.sha512 and
  once with caps.sha384 set on a copy of the fetched capabilities.

Issue: F-8024
@yosuke-wolfssl yosuke-wolfssl self-assigned this Sep 17, 2026
Copilot AI lite review requested due to automatic review settings September 17, 2026 05:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The implementation addresses the compatibility issue; the remaining test-coverage feedback is non-blocking.

Pull request overview

Updates SCEP signing-hash selection to use only SHA-384/SHA-512 algorithms compiled into wolfSSL, falling back to SHA-256 otherwise.

Changes:

  • Gates SHA-512 and SHA-384 OID selection on build support.
  • Adds round-trip enrollment coverage for advertised hash capabilities.
File summaries
File Summary Review note
tests/integration/test_scep_roundtrip.c Adds SHA-512 and SHA-384 capability enrollment tests. Nit: assertions do not distinguish fallback behavior from the pre-change behavior in standard CI configurations.
src/scep/scep_client.c Restricts caps-driven hash selection to supported wolfSSL algorithms.
Review details

Suppressed comments (1)

tests/integration/test_scep_roundtrip.c:1196

  • These assertions do not distinguish the fix from the pre-change behavior in the configurations exercised by CI: with both digest macros enabled, the old selector also succeeds, while the CI matrix has no SCEP roundtrip job with SHA-512/SHA-384 disabled. The regression being fixed—falling back to SHA-256 when an advertised digest is unavailable—is therefore not automatically protected. Please either add a constrained SCEP roundtrip variant or assert the generated PKCS#7 digest OID on the wire.
    caps_hash.sha512 = 1;
    REQUIRE(wolfcert_scep_pkcs_req(&cli, &caps_hash, ca_der->buffer,
                                   ca_der->length, dk, csr.data, csr.len,
                                   &issued_hash) == WOLFCERT_OK);
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #34

Scan targets checked: wolfcert-src, wolfcert-bugs

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Comment thread tests/integration/test_scep_roundtrip.c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants