sa_get_from_spi: reject SAs where shivf_len > iv_len (sibling of arsn/shsnf guard)#513
Open
nurdymuny wants to merge 1 commit into
Open
sa_get_from_spi: reject SAs where shivf_len > iv_len (sibling of arsn/shsnf guard)#513nurdymuny wants to merge 1 commit into
nurdymuny wants to merge 1 commit into
Conversation
A Security Association loaded with shivf_len > iv_len causes every
per-frame IV walk of the form
for (i = sa_ptr->iv_len - sa_ptr->shivf_len; i < sa_ptr->iv_len; i++)
*(p_new_enc_frame + index_temp) = *(sa_ptr->iv + i);
(crypto_aos.c:318, 351, 697; crypto_tc.c:331, 631) to start at a
negative index. The loop then reads up to shivf_len bytes from before
sa_ptr->iv inside the SecurityAssociation_t struct and copies those
bytes into the transmitted frame. With the AOS / TC IV-walk loops
running on every authenticated/encrypted frame, that's a steady leak
of pre-iv struct fields into the channel.
sa_get_from_spi already enforces the sibling invariant
'arsn_len cannot be less than shsnf_len' immediately above this fix.
This commit adds the parallel 'iv_len cannot be less than shivf_len'
guard and a new CRYPTO_LIB_ERR_SHIVF_LEN_GREATER_THAN_IV_LEN (-86)
return code, registered in include/crypto_error.h and the names
table in src/core/crypto_error.c.
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
1 participant
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.
Summary
Adds the parallel
iv_len >= shivf_leninvariant check tosa_get_from_spialongside the existingarsn_len >= shsnf_lencheck on line 777. Without this, an SA loaded withshivf_len > iv_lenmakes the IV-walk loops atcrypto_aos.c:318/351/697andcrypto_tc.c:331/631start at a negative index and copy pre-iv struct bytes into every transmitted frame. Closes #512.Diff
The new error code is
CRYPTO_LIB_ERR_SHIVF_LEN_GREATER_THAN_IV_LEN (-86), registered in both the header definition and the name-table insrc/core/crypto_error.c(soCrypto_Get_Error_Code_Enum_Stringprints it correctly).Tests
shivf_len=20, iv_len=12(existingIV_SIZEallows shivf up toIV_SIZEso the existingsa_verify_datamax check doesn't catch this), calledsa_get_from_spion that SPI; before this fix the lookup succeeds andCrypto_TC_ApplySecuritylater leaks 8 bytes per frame; after this fix the lookup returns-86(CRYPTO_LIB_ERR_SHIVF_LEN_GREATER_THAN_IV_LEN).shivf_len <= iv_len) continue to pass.Scope
The mariadb backend (
src/sa/mariadb/sa_interface_mariadb.template.c) accepts the same SAs without checking either invariant. Adding the same two-line guard there is a good follow-up; left out of this PR to keep the change small.Linked
Closes #512.