Skip to content
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

FIPS 140-2 IG A.9 AES-XTS key handling. #7120

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@paulidale
Copy link
Contributor

paulidale commented Sep 5, 2018

This is an implementation of the extra check required by FIPS 140-2 IG A.9. The two uses for AES-XTS are mandated to be different. If they are the same, there is a vulnerability published by Rogaway in September 2004 in the paper: Efficient Instantiations of Tweakable Blockciphers and Refinements to Modes OCB and PMAC.

The main negative side effect is that old encrypted data that used the same key twice will be unable to be decrypted. Thus, this change should be conditional on a FIPS mode of some kind (which is currently undefined). One of the AES-XTS test cases uses the same key twice, this test has been modified so an error is expected. Ideally, this would also be dependent on a FIPS mode -- possibly by duplicating the test case and flagging one as FIPS and the other as not.

The WIP tag depends on a resolution of the FIPS mode question.

  • tests are added or updated

@paulidale paulidale added the FIPS label Sep 5, 2018

@@ -3410,10 +3410,28 @@ static int aes_xts_cipher(EVP_CIPHER_CTX *ctx, unsigned char *out,
const unsigned char *in, size_t len)
{
EVP_AES_XTS_CTX *xctx = EVP_C_DATA(EVP_AES_XTS_CTX,ctx);

if (!xctx->xts.key1 || !xctx->xts.key2)

This comment has been minimized.

@richsalz

richsalz Sep 5, 2018

Contributor

While you're here, maybe change the ! in pointers to explicit NULL tests.

This comment has been minimized.

@paulidale

paulidale Sep 5, 2018

Author Contributor

There is a lot of clean up possible in this file.

@t-j-h

t-j-h approved these changes Sep 5, 2018

@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 5, 2018

@t-j-h does the approval imply that a not in FIPS mode check to allow backwards compatibility isn't desirable?

@t-j-h

This comment has been minimized.

Copy link
Member

t-j-h commented Sep 5, 2018

You should add a note to CHANGES noting that this specification requirement (that key1!=key2) is now enforced.

@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 5, 2018

@mattcaswell is this worthwhile being included in the 1.1.1 release?

@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 5, 2018

The attack was detailed fourteen years ago and despite the prior recommendation being to use a single key twice, that seems long enough to drop support. Thoughts?

* using the keys in the XTS-AES algorithm to process data with them."
*/
if (memcmp(xctx->xts.key1, xctx->xts.key2,
EVP_CIPHER_CTX_key_length(ctx) / 2) == 0)

This comment has been minimized.

@dot-asm

dot-asm Sep 5, 2018

Contributor

In the spirit of tell-as-little-as-possible it's CRYPTO_memcmp.

This comment has been minimized.

@t-j-h

t-j-h Sep 5, 2018

Member

Still a +1 approved with @dot-asm suggested change ... pending matt's view

This comment has been minimized.

@paulidale

paulidale Sep 5, 2018

Author Contributor

I don't see a viable attack vector here but I've changed to CRYPTO_memcmp just in case.

If an attacker could control one of the keys, they could discover the other.

@mattcaswell mattcaswell added this to the Post 1.1.1 milestone Sep 5, 2018

@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Sep 5, 2018

I don't think we've made it impossible to decrypt existing text before. (You could always spec the old digest for the "enc" command, e.g.) Doing this in a compatible release seems like a bad idea.

@paulidale paulidale force-pushed the paulidale:ig_a_9_aes_xts branch from 6e2676b to 118e4df Sep 5, 2018

@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 5, 2018

Updated with @dot-asm 's change and squashed.

@paulidale paulidale changed the title WIP: FIPS 140-2 IG A.9 AES-XTS key handling. FIPS 140-2 IG A.9 AES-XTS key handling. Sep 6, 2018

FIPS 140-2 IG A.9 XTS key check.
Add a check that the two keys used for AES-XTS are different.

One test case uses the same key for both of the AES-XTS keys.  This causes
a failure under FIP 140-2 IG A.9.  Mark the test as returning a failure.

@paulidale paulidale force-pushed the paulidale:ig_a_9_aes_xts branch from 118e4df to eea995a Sep 11, 2018

@paulidale paulidale force-pushed the paulidale:ig_a_9_aes_xts branch from eea995a to 7b12219 Sep 11, 2018

@paulidale

This comment has been minimized.

Copy link
Contributor Author

paulidale commented Sep 11, 2018

Merged to master.

@paulidale paulidale closed this Sep 11, 2018

levitte pushed a commit that referenced this pull request Sep 11, 2018

FIPS 140-2 IG A.9 XTS key check.
Add a check that the two keys used for AES-XTS are different.

One test case uses the same key for both of the AES-XTS keys.  This causes
a failure under FIP 140-2 IG A.9.  Mark the test as returning a failure.

Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #7120)

levitte pushed a commit that referenced this pull request Sep 11, 2018

Add a note to CHANGES indicating that AES-XTS now enforces two different
keys.

Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #7120)

@paulidale paulidale deleted the paulidale:ig_a_9_aes_xts branch Oct 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.