-
Notifications
You must be signed in to change notification settings - Fork 35
fix GCM tls iv length check and aead tag length validation #416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -458,6 +458,37 @@ static int wp_aead_get_ctx_params(wp_AeadCtx* ctx, OSSL_PARAM params[]) | |
| return ok; | ||
| } | ||
|
|
||
| /** | ||
| * Check whether a tag length is one of the algorithm's allowed sizes. | ||
| * | ||
| * GCM tags must be 4, 8, 12, 13, 14, 15 or 16 bytes (NIST SP 800-38D). | ||
| * CCM tags must be 4, 6, 8, 10, 12, 14 or 16 bytes (NIST SP 800-38C / | ||
| * RFC 3610). | ||
| * | ||
| * @param [in] mode Cipher mode: EVP_CIPH_GCM_MODE or EVP_CIPH_CCM_MODE. | ||
| * @param [in] sz Tag length in bytes to check. | ||
| * @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) | ||
| { | ||
| int ok; | ||
|
|
||
| if (mode == EVP_CIPH_GCM_MODE) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 [Medium] GCM tag-length validation is stricter than OpenSSL on decrypt (interop divergence) For GCM, Fix: Confirm intent; if intended (likely), document the divergence from OpenSSL's GCM tag-length acceptance so downstream consumers are aware. |
||
| ok = (sz == 4) || (sz == 8) || (sz == 12) || (sz == 13) || | ||
| (sz == 14) || (sz == 15) || (sz == 16); | ||
| } | ||
| else if (mode == EVP_CIPH_CCM_MODE) { | ||
| ok = (sz == 4) || (sz == 6) || (sz == 8) || (sz == 10) || | ||
| (sz == 12) || (sz == 14) || (sz == 16); | ||
| } | ||
| else { | ||
| ok = 0; | ||
| } | ||
|
|
||
| return ok; | ||
| } | ||
|
|
||
| /** | ||
| * Set the AEAD tag from the parameters. | ||
| * | ||
|
|
@@ -488,6 +519,9 @@ static int wp_aead_set_param_tag(wp_AeadCtx* ctx, | |
| if (ok && ((sz == 0) || ((p->data != NULL) && ctx->enc))) { | ||
| ok = 0; | ||
| } | ||
| if (ok && !wp_aead_tag_len_valid(ctx->mode, sz)) { | ||
| ok = 0; | ||
| } | ||
| if (ok) { | ||
| ctx->tagLen = sz; | ||
| } | ||
|
|
@@ -959,9 +993,10 @@ static int wp_aesgcm_tls_iv_set_fixed(wp_AeadCtx* ctx, unsigned char* iv, | |
| } | ||
| else { | ||
| /* Fixed field must be at least 4 bytes and invocation field at least 8 | ||
| */ | ||
| if ((len < EVP_GCM_TLS_FIXED_IV_LEN) || | ||
| (ctx->ivLen - (int)len) < EVP_GCM_TLS_EXPLICIT_IV_LEN) { | ||
| * bytes */ | ||
| if ((len < EVP_GCM_TLS_FIXED_IV_LEN) || (len > ctx->ivLen) || | ||
| (len > sizeof(ctx->iv)) || | ||
| (ctx->ivLen - len) < EVP_GCM_TLS_EXPLICIT_IV_LEN) { | ||
| return 0; | ||
| } | ||
| if (ctx->enc) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1416,6 +1416,159 @@ int test_aes_gcm_bad_tag(void *data) | |
| return err; | ||
| } | ||
|
|
||
| static int test_aes_gcm_tls_iv_fixed_oversized_helper(OSSL_LIB_CTX *libCtx, | ||
| const char *cipherName, int keyLen) | ||
| { | ||
| int err; | ||
| EVP_CIPHER *cipher = NULL; | ||
| EVP_CIPHER_CTX *ctx = NULL; | ||
| unsigned char key[32]; | ||
| /* ivLen (12, the GCM default) + EVP_GCM_TLS_EXPLICIT_IV_LEN (8) = 20: */ | ||
| unsigned char iv[20]; | ||
|
|
||
| memset(key, 0xCC, keyLen); | ||
| memset(iv, 0xDD, sizeof(iv)); | ||
|
|
||
| cipher = EVP_CIPHER_fetch(libCtx, cipherName, ""); | ||
| err = cipher == NULL; | ||
|
|
||
| /* A fixed len that leaves exactly the 8-byte | ||
| * explicit/invocation field must still be accepted. */ | ||
| if (err == 0) { | ||
| ctx = EVP_CIPHER_CTX_new(); | ||
| err = ctx == NULL; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_DecryptInit_ex(ctx, cipher, NULL, key, NULL) != 1; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_SET_IV_FIXED, | ||
| EVP_GCM_TLS_FIXED_IV_LEN, iv) != 1; | ||
| } | ||
| EVP_CIPHER_CTX_free(ctx); | ||
| ctx = NULL; | ||
|
|
||
| /* Oversized fixed len (> ctx->ivLen, default 12) must be rejected. */ | ||
| if (err == 0) { | ||
| ctx = EVP_CIPHER_CTX_new(); | ||
| err = ctx == NULL; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_DecryptInit_ex(ctx, cipher, NULL, key, NULL) != 1; | ||
| } | ||
| if (err == 0) { | ||
| if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_SET_IV_FIXED, | ||
| (int)sizeof(iv), iv) == 1) { | ||
| PRINT_ERR_MSG("%s: EVP_CTRL_GCM_SET_IV_FIXED incorrectly " | ||
| "accepted a fixed IV length (%d) larger than " | ||
| "the IV length", cipherName, (int)sizeof(iv)); | ||
| err = 1; | ||
| } | ||
| } | ||
|
|
||
| EVP_CIPHER_CTX_free(ctx); | ||
| EVP_CIPHER_free(cipher); | ||
| return err; | ||
| } | ||
|
|
||
| int test_aes_gcm_tls_iv_fixed_oversized(void *data) | ||
| { | ||
| int err; | ||
|
|
||
| (void)data; | ||
|
|
||
| PRINT_MSG("AES-128-GCM TLS1 fixed-IV oversized length rejection"); | ||
| err = test_aes_gcm_tls_iv_fixed_oversized_helper(wpLibCtx, | ||
| "AES-128-GCM", 16); | ||
|
|
||
| return err; | ||
| } | ||
|
|
||
| /* | ||
| * Test that GCM tags smaller than the 32-bit minimum are rejected. | ||
| */ | ||
| static int test_aes_gcm_tag_len_undersized_helper(OSSL_LIB_CTX *libCtx, | ||
| const char *cipherName, int keyLen) | ||
| { | ||
| int err; | ||
| EVP_CIPHER *cipher = NULL; | ||
| EVP_CIPHER_CTX *ctx = NULL; | ||
| unsigned char key[32]; | ||
| unsigned char iv[12]; | ||
| unsigned char aad[] = "additional data"; | ||
| unsigned char pt[] = "GCM plaintext for tag length test"; | ||
| int ptLen = (int)(sizeof(pt) - 1); | ||
| unsigned char ct[64]; | ||
| unsigned char tag[16]; | ||
| int outLen = 0, fLen = 0; | ||
|
|
||
| memset(key, 0xAA, keyLen); | ||
| memset(iv, 0xBB, sizeof(iv)); | ||
|
|
||
| cipher = EVP_CIPHER_fetch(libCtx, cipherName, ""); | ||
| err = cipher == NULL; | ||
|
|
||
| /* Encrypt normally to get a valid ciphertext/full-size tag pair. */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 [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 Fix: Simplify the helpers to test only the rejection path, decoupling them from encrypt-path success. |
||
| if (err == 0) { | ||
| ctx = EVP_CIPHER_CTX_new(); | ||
| err = ctx == NULL; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_EncryptInit(ctx, cipher, key, iv) != 1; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_EncryptUpdate(ctx, NULL, &outLen, aad, | ||
| (int)(sizeof(aad) - 1)) != 1; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_EncryptUpdate(ctx, ct, &outLen, pt, ptLen) != 1; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_EncryptFinal_ex(ctx, ct + outLen, &fLen) != 1; | ||
| outLen += fLen; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_GET_TAG, sizeof(tag), | ||
| tag) != 1; | ||
| } | ||
| EVP_CIPHER_CTX_free(ctx); | ||
| ctx = NULL; | ||
|
|
||
| /* A 1-byte tag length is below the NIST-mandated minimum (4 bytes) | ||
| * and must be rejected here. */ | ||
| if (err == 0) { | ||
| ctx = EVP_CIPHER_CTX_new(); | ||
| err = ctx == NULL; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_DecryptInit(ctx, cipher, NULL, NULL) != 1; | ||
| } | ||
| if (err == 0) { | ||
| if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, 1, tag) == 1) { | ||
| PRINT_ERR_MSG("%s: EVP_CTRL_AEAD_SET_TAG incorrectly accepted " | ||
| "a 1-byte tag length", cipherName); | ||
| err = 1; | ||
| } | ||
| } | ||
|
|
||
| EVP_CIPHER_CTX_free(ctx); | ||
| EVP_CIPHER_free(cipher); | ||
| return err; | ||
| } | ||
|
|
||
| int test_aes_gcm_tag_len_undersized(void *data) | ||
| { | ||
| int err; | ||
|
|
||
| (void)data; | ||
|
|
||
| PRINT_MSG("AES-128-GCM undersized tag length rejection"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 [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 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. |
||
| err = test_aes_gcm_tag_len_undersized_helper(wpLibCtx, "AES-128-GCM", | ||
| 16); | ||
|
|
||
| return err; | ||
| } | ||
|
|
||
| #endif /* WP_HAVE_AESGCM */ | ||
|
|
||
| /******************************************************************************/ | ||
|
|
@@ -1601,5 +1754,108 @@ int test_aes_ccm_bad_tag(void *data) | |
| return err; | ||
| } | ||
|
|
||
| /* | ||
| * Test that CCM tags are at least 32 bits. | ||
| */ | ||
| static int test_aes_ccm_tag_len_undersized_helper(OSSL_LIB_CTX *libCtx, | ||
| const char *cipherName, int keyLen) | ||
| { | ||
| int err; | ||
| EVP_CIPHER *cipher = NULL; | ||
| EVP_CIPHER_CTX *ctx = NULL; | ||
| unsigned char key[32]; | ||
| unsigned char iv[13]; | ||
| unsigned char aad[] = "additional data"; | ||
| unsigned char pt[] = "CCM plaintext for tag length test"; | ||
| int ptLen = (int)(sizeof(pt) - 1); | ||
| unsigned char ct[64]; | ||
| unsigned char tag[16]; | ||
| int outLen = 0, fLen = 0; | ||
|
|
||
| memset(key, 0xAA, keyLen); | ||
| memset(iv, 0xBB, sizeof(iv)); | ||
|
|
||
| cipher = EVP_CIPHER_fetch(libCtx, cipherName, ""); | ||
| err = cipher == NULL; | ||
|
|
||
| /* Encrypt normally to get a valid ciphertext/full-size tag pair. */ | ||
| if (err == 0) { | ||
| ctx = EVP_CIPHER_CTX_new(); | ||
| err = ctx == NULL; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_EncryptInit(ctx, cipher, NULL, NULL) != 1; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_IVLEN, | ||
| (int)sizeof(iv), NULL) != 1; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, sizeof(tag), | ||
| NULL) != 1; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_EncryptInit(ctx, NULL, key, iv) != 1; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_EncryptUpdate(ctx, NULL, &outLen, NULL, ptLen) != 1; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_EncryptUpdate(ctx, NULL, &outLen, aad, | ||
| (int)(sizeof(aad) - 1)) != 1; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_EncryptUpdate(ctx, ct, &outLen, pt, ptLen) != 1; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_EncryptFinal_ex(ctx, ct + outLen, &fLen) != 1; | ||
| outLen += fLen; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_GET_TAG, sizeof(tag), | ||
| tag) != 1; | ||
| } | ||
| EVP_CIPHER_CTX_free(ctx); | ||
| ctx = NULL; | ||
|
|
||
| /* A 1-byte tag length is below the NIST-mandated minimum (4 bytes) | ||
| * and must be rejected here. */ | ||
| if (err == 0) { | ||
| ctx = EVP_CIPHER_CTX_new(); | ||
| err = ctx == NULL; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_DecryptInit(ctx, cipher, NULL, NULL) != 1; | ||
| } | ||
| if (err == 0) { | ||
| err = EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_IVLEN, | ||
| (int)sizeof(iv), NULL) != 1; | ||
| } | ||
| if (err == 0) { | ||
| if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, 1, tag) == 1) { | ||
| PRINT_ERR_MSG("%s: EVP_CTRL_AEAD_SET_TAG incorrectly accepted " | ||
| "a 1-byte tag length", cipherName); | ||
| err = 1; | ||
| } | ||
| } | ||
|
|
||
| EVP_CIPHER_CTX_free(ctx); | ||
| EVP_CIPHER_free(cipher); | ||
| return err; | ||
| } | ||
|
|
||
| int test_aes_ccm_tag_len_undersized(void *data) | ||
| { | ||
| int err; | ||
|
|
||
| (void)data; | ||
|
|
||
| PRINT_MSG("AES-128-CCM undersized tag length rejection"); | ||
| err = test_aes_ccm_tag_len_undersized_helper(wpLibCtx, "AES-128-CCM", | ||
| 16); | ||
|
|
||
| return err; | ||
| } | ||
|
|
||
| #endif /* WP_HAVE_AESCCM */ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 [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 withWOLFPROV_ENTER(WP_LOG_COMP_AES, "...")and returns viaWOLFPROV_LEAVE. The newly addedwp_aead_tag_len_validhas 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.