tls1_prf: reject unsupported digests instead of coercing to SHA-384#431
tls1_prf: reject unsupported digests instead of coercing to SHA-384#431yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a TLS1-PRF correctness issue in wolfProvider where requesting an unsupported digest could succeed while silently deriving with SHA-384, and adds a regression test to ensure unsupported digests are rejected.
Changes:
- Add an explicit digest whitelist in
wp_kdf_tls1_prf_derive(MD5-SHA1, SHA-256, SHA-384) and fail derivation for any other digest. - Add a negative regression test that requests SHA-512 and asserts
EVP_PKEY_derivefails when using wolfProvider’s TLS1-PRF.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/wp_tls1_prf.c | Rejects unsupported digest types up-front so derivation cannot silently coerce to SHA-384. |
| test/test_tls1_prf.c | Adds a regression test ensuring an unsupported digest (SHA-512) is rejected by wolfProvider. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6788cd4 to
ea90736
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #431
Scan targets checked: wolfprovider-bugs, wolfprovider-src
No new issues found in the changed files. ✅
aidangarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: COMMENT
Findings: 1 total — 1 posted, 0 skipped
Posted findings
- [Low] Negative test only exercises SHA-512, not the other silently-coerced digests —
test/test_tls1_prf.c:294-341
Review generated by Skoll.
| /* An unsupported TLS1-PRF digest must be rejected, not silently computed | ||
| * with SHA-384. Only wolfProvider is exercised; the OpenSSL default | ||
| * provider accepts SHA-512 for the TLS 1.2 PRF. */ | ||
| static int test_tls1_prf_unsupported_md(void) |
There was a problem hiding this comment.
🔵 [Low] Negative test only exercises SHA-512, not the other silently-coerced digests
💡 SUGGEST test
The regression test proves the new guard rejects SHA-512, but the PR description lists SHA-1, SHA-224, and SHA-512 as digests that were previously silently coerced to SHA-384. Only SHA-512 is exercised. The guard is a single whitelist check so SHA-512 is a reasonable representative, but adding SHA-1/SHA-224 cases (guarded appropriately) would confirm the full set of previously-affected digests is now rejected and would not depend on WP_HAVE_SHA512 being enabled to run any negative coverage at all.
Recommendation: Optional: parameterize test_tls1_prf_unsupported_md() over EVP_sha1()/EVP_sha224()/EVP_sha512() so at least one negative case runs regardless of which digests are compiled in. Not blocking since the guard logic is a single check.
Description
wp_kdf_tls1_prf_deriveaccepted any digest thatwp_params_get_digestcould map to a wolfCrypt hash type, but only handled
WC_HASH_TYPE_MD5_SHAand
WC_HASH_TYPE_SHA256distinctly. Every other mapped digest fell throughto the
elsebranch and was passed towc_PRF_TLSassha384_mac. Becausewc_PRF_TLSalso silently coerces sub-SHA256 hash types upward, no error wasever returned.
As a result a caller requesting SHA-512, SHA-1, SHA-224, or any other mapped
digest received a successful TLS1-PRF result silently computed with
SHA-384 instead of a failure.
Addressed by f_5535.
Fix
Reject any digest outside the set the TLS PRF actually uses before
derivation —
MD5-SHA1(TLS 1.0/1.1),SHA-256andSHA-384(TLS 1.2) —and fail for anything else. These three are the complete set of PRF hashes
used by standardized TLS cipher suites, so restricting (rather than adding
support for SHA-512/SHA-1/etc.) is the correct behavior.
Testing
test_tls1_prf_unsupported_md) thatrequests SHA-512 via wolfProvider and asserts
EVP_PKEY_derivefails.test_tls1_prf) passes with the fix, including theexisting MD5-SHA1 / SHA-256 / SHA-384 interop cases.
(
FAILED unsupported digest was accepted), confirming it catches thecoercion.
-Werror -Wextraflags, andcompiles/links clean with
-fsanitize=address,undefined.Files changed
src/wp_tls1_prf.c— reject unsupported digests before derivation.test/test_tls1_prf.c— negative test for an unsupported digest.