Repository navigation
fix: return errors for refused requests (FIPS HMAC forgery) - #87
Closed
MarkAtwood wants to merge 2 commits into
Closed
MarkAtwood wants to merge 2 commits into
MarkAtwood wants to merge 2 commits into
Conversation
GnuTLS treats a provider return >= 0 as success. Under HAVE_FIPS the wrapper returned GNUTLS_FIPS140_OP_NOT_APPROVED, a positive enum value, when wolfCrypt refused a request: - wc_HmacSetKey (FIPS refuses keys shorter than 14 bytes): gnutls_hmac_fast() returned 0 without writing the MAC, and gnutls_hmac_init() succeeded with an HMAC whose output does not depend on the key: SHA256(0^64 || SHA256(0^64 || msg)) for SHA-256. Anyone can compute it, so a forged tag verifies for every short-key HMAC user on a FIPS build; - wc_HKDF_Extract, and wc_PBKDF2_ex with HMAC_MIN_KEYLEN_E: gnutls_hkdf_extract()/gnutls_pbkdf2() returned 2 with the output unwritten; - RSA key generation failure and the 1024-bit refusal: the key was left without parameters. Return the negative error the non-FIPS path returns, and GNUTLS_E_INVALID_REQUEST for 1024-bit RSA. The Ed25519/Ed448/X25519/ X448 key generation curve checks returned GNUTLS_ECC_CURVE_INVALID, which is 0; return GNUTLS_E_ECC_UNSUPPORTED_CURVE. wolfssl_hmac_fast() now frees its context when the key is refused. New test test_fips_refusal_errors: HMAC-SHA256 with a 4-byte key (RFC 4231 test case 2) must fail with an error or give the RFC value, and two different short keys must not give the same MAC.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The regression test has incomplete return validation, and several corrected failure paths remain untested.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Ensures wolfSSL provider refusals propagate as negative GnuTLS errors, preventing FIPS HMAC forgery and invalid key acceptance.
Changes:
- Corrects HMAC, KDF, RSA, and ECC error handling.
- Frees failed one-shot HMAC contexts.
- Adds short-key HMAC regression coverage.
| File | Description |
|---|---|
src/mac.c |
Fixes HMAC errors and cleanup. |
src/kdf.c |
Returns negative KDF failure codes. |
src/pk.c |
Corrects RSA and curve errors. |
tests/test_fips_refusal_errors.c |
Tests short-key HMAC refusals. |
tests/Makefile |
Registers the regression test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+87
to
+91
| if (ret == 0 && ret2 == 0 && memcmp(mac, mac2, sizeof(mac)) == 0) { | ||
| printf("FAILURE - two different keys gave the same MAC\n"); | ||
| gnutls_global_deinit(); | ||
| return 1; | ||
| } |
| @@ -1,4 +1,4 @@ | |||
| TESTS = test_hash test_shake test_aescbc test_aescfb8 test_aesgcm test_aesccm test_aesxts test_hmac test_cmac test_gmac test_rnd test_rnd_fork test_tls_prf test_hkdf test_pbkdf2 test_ecdsa_sign_and_verify test_ecdh_encrypt_and_decrypt test_eddsa_sign_and_verify test_rsa_sign_and_verify test_rsa_encrypt_and_decrypt test_dh_encrypt_and_decrypt test_pk_import_export test_long_hash test_fips test_aessiv | |||
| TESTS = test_hash test_shake test_aescbc test_aescfb8 test_aesgcm test_aesccm test_aesxts test_hmac test_cmac test_gmac test_rnd test_rnd_fork test_tls_prf test_hkdf test_pbkdf2 test_ecdsa_sign_and_verify test_ecdh_encrypt_and_decrypt test_eddsa_sign_and_verify test_rsa_sign_and_verify test_rsa_encrypt_and_decrypt test_dh_encrypt_and_decrypt test_pk_import_export test_long_hash test_fips test_aessiv test_fips_refusal_errors | |||
When wc_InitCmac (in the CMAC setkey, hash and output callbacks) or wc_GmacSetKey failed, the wrapper gnutls_free()d its context. GnuTLS owns that context and calls deinit on it afterwards, so it was used after free and freed twice. A failed CAST makes these calls fail on a FIPS build. Leave the context to deinit, and have the CMAC/GMAC fast functions free it when setkey fails (they leaked it on a wrong-size key). With wc_InitCmac/wc_GmacSetKey forced to fail, gnutls_hmac_init( AES-CMAC-128 / AES-GMAC-128) showed invalid reads/writes and a double free under valgrind; it now returns an error and runs clean.
Author
|
Closing in favour of #90, per Reda. |
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.
Security: on FIPS builds, a forged tag verifies for HMAC with a key shorter than 14 bytes.
GnuTLS treats a provider return >= 0 as success. When wolfCrypt refuses a request under
HAVE_FIPS, the wrapper returnsGNUTLS_FIPS140_OP_NOT_APPROVED, which is a positive enum value (2), so GnuTLS treats the refusal as success:wc_HmacSetKeyreturnsHMAC_MIN_KEYLEN_E):gnutls_hmac_init()succeeds. The MAC is then computed with all-zero pads, giving SHA256(0^64 || SHA256(0^64 || msg)) for SHA-256. That value doesn't depend on the key, so anyone can compute it.gnutls_hmac_fast()returns 0 and never writes the output.wc_HKDF_Extractorwc_PBKDF2_exfailure:gnutls_hkdf_extract()/gnutls_pbkdf2()return 2 to the caller, with the output unwritten.PRIME_GEN_E) or the 1024-bit refusal: GnuTLS keeps a key with no parameters.The fix returns the same negative error as the non-FIPS path (
GNUTLS_E_HASH_FAILED,GNUTLS_E_INTERNAL_ERROR,GNUTLS_E_PK_GENERATION_ERROR), andGNUTLS_E_INVALID_REQUESTfor 1024-bit RSA. It also fixes two smaller issues:GNUTLS_ECC_CURVE_INVALID, which is 0; they now returnGNUTLS_E_ECC_UNSUPPORTED_CURVE;wolfssl_hmac_fast()leaked its context when the key was refused; it now frees it.New test
test_fips_refusal_errors: HMAC-SHA256 with the 4-byte key from RFC 4231 test case 2, one-shot and streaming, must either fail with an error or give the RFC value, and two different short keys must not give the same MAC.Results:
wc_HKDF_Extract/wc_PBKDF2_exfailures andPRIME_GEN_Enow return negative errors. Normal-length HMAC/HKDF/PBKDF2 and RSA-2048 results are unchanged and match OpenSSL.make testpasses 26/26 with the provider and 26/26 without;run_valgrindis clean.CI builds wolfSSL master non-FIPS, where short HMAC keys are allowed, so CI can't exercise the FIPS path.