fix GCM tls iv length check and aead tag length validation#416
fix GCM tls iv length check and aead tag length validation#416gasbytes wants to merge 2 commits into
Conversation
… than the iv buffer
There was a problem hiding this comment.
Pull request overview
This PR tightens AES-GCM/TLS fixed-IV handling and AEAD tag-length validation in wp_aes_aead.c, and adds unit tests to prevent regressions around oversized fixed-IV inputs and undersized authentication tags.
Changes:
- Add
wp_aead_tag_len_valid()and enforce algorithm-specific allowed tag sizes for GCM and CCM when settingOSSL_CIPHER_PARAM_AEAD_TAG. - Fix AES-GCM TLS fixed-IV length validation to reject oversized fixed-IV lengths (including those exceeding the configured IV length).
- Add new unit tests covering oversized GCM TLS fixed-IV inputs and undersized tag-length rejection for both GCM and CCM.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/unit.h | Declares new GCM/CCM regression tests. |
| test/unit.c | Registers the new tests in the unit test suite. |
| test/test_aestag.c | Implements new test cases for oversized TLS fixed-IV and undersized tag length rejection. |
| src/wp_aes_aead.c | Adds tag-length validation helper and tightens GCM TLS fixed-IV length checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aececf4 to
4f8d057
Compare
ColtonWilley
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] GCM tag-length validation is stricter than OpenSSL on decrypt (interop divergence) —
src/wp_aes_aead.c:473-490, 522 - [Medium] New validation paths only partially exercised by tests —
test/test_aestag.c:1474-1485, 1559-1570, 1847-1858 - [Low] New helper omits WOLFPROV_ENTER/LEAVE tracing used by sibling functions —
src/wp_aes_aead.c:473-490 - [Low] Undersized-tag tests perform an unnecessary full encrypt preamble —
test/test_aestag.c:1511-1535, 1779-1820
Review generated by Skoll
| { | ||
| int ok; | ||
|
|
||
| if (mode == EVP_CIPH_GCM_MODE) { |
There was a problem hiding this comment.
🟠 [Medium] GCM tag-length validation is stricter than OpenSSL on decrypt (interop divergence)
For GCM, wp_aead_tag_len_valid only accepts tag sizes {4,8,12,13,14,15,16} and wp_aead_set_param_tag now rejects anything else, including on the decrypt (verify) path. OpenSSL's own GCM provider does NOT enforce this discrete set on EVP_CTRL_AEAD_SET_TAG; it accepts any non-zero size up to 16 (e.g. 5,6,7,9,10,11). For CCM this exactly matches OpenSSL (4..16 even), so only GCM diverges. An application that previously set a non-standard GCM tag length (legal in stock OpenSSL) will now get a failure from wolfProvider. This is almost certainly the intended, more NIST-correct behavior, but it is a behavioral/interop divergence worth confirming and noting in a changelog.
Fix: Confirm intent; if intended (likely), document the divergence from OpenSSL's GCM tag-length acceptance so downstream consumers are aware.
|
|
||
| (void)data; | ||
|
|
||
| PRINT_MSG("AES-128-GCM undersized tag length rejection"); |
There was a problem hiding this comment.
🟠 [Medium] New validation paths only partially exercised by tests
The tests cover the headline cases (1-byte tag rejected for GCM/CCM; oversized fixed IV rejected for GCM) but leave gaps for the new logic: (1) only AES-128 variants are exercised, whereas sibling tests in this file (e.g. test_aes_gcm_bad_tag) also cover 256-bit; (2) no positive assertion that a valid non-default tag length (e.g. 12/13/14 for GCM, 8 for CCM) is still accepted, so a regression that over-rejects allowed sizes would go undetected; (3) for GCM, the 'in-between' sizes that the new function specifically excludes (e.g. 5/6/7) are not tested; (4) the fixed-IV test does not cover the len == ctx->ivLen boundary (which must be rejected because it leaves <8 bytes for the explicit field).
Fix: Extend the helpers to also assert accepted lengths and a couple of additional rejected lengths/key sizes to lock in the full validation table.
| * @return 1 if sz is an allowed length for mode. | ||
| * @return 0 otherwise. | ||
| */ | ||
| static int wp_aead_tag_len_valid(int mode, size_t sz) |
There was a problem hiding this comment.
🔵 [Low] New helper omits WOLFPROV_ENTER/LEAVE tracing used by sibling functions
Nearly every other static helper in this file (e.g. wp_aead_set_param_tag, wp_aead_set_param_iv_len, wp_aead_cache_aad) opens with WOLFPROV_ENTER(WP_LOG_COMP_AES, "...") and returns via WOLFPROV_LEAVE. The newly added wp_aead_tag_len_valid has neither. It is a trivial pure predicate so the omission is harmless, but it is inconsistent with the established convention.
Fix: Optional: add tracing to match the file's convention, or accept the omission for a pure helper.
| cipher = EVP_CIPHER_fetch(libCtx, cipherName, ""); | ||
| err = cipher == NULL; | ||
|
|
||
| /* Encrypt normally to get a valid ciphertext/full-size tag pair. */ |
There was a problem hiding this comment.
🔵 [Low] Undersized-tag tests perform an unnecessary full encrypt preamble
Both undersized-tag helpers run a complete encrypt + GET_TAG sequence solely to populate the tag buffer, but the behavior under test (rejecting a 1-byte tag length at EVP_CTRL_AEAD_SET_TAG) is rejected before any tag bytes are consumed, so the encryption is not needed. This couples the rejection test to unrelated encrypt-path success: if CCM/GCM encryption ever regresses, these tests would fail for a reason unrelated to tag-length validation, obscuring the signal.
Fix: Simplify the helpers to test only the rejection path, decoupling them from encrypt-path success.
Fixes the gcm tls fixed-iv length check to properly reject oversized inputs and adds tag length validation against each algorithm's allowed sizes for GCM and CCM, in particular this last one was wrapped in a new function called wp_aead_tag_len_valid.