1610687 - Crash on unaligned CMACContext.aes.keySchedule when using AES-NI intrinsics

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kevin Jacobs [:kjacobs]]

Native AES implementation in aes-x86.c assumes that keySchedule will be 16B aligned. This enforced by AES_AllocateContext [1], but within CMACContext the offset and allocation method provide no such guarantee [2]. This causes random crashes when in CMACContext is 8B-aligned, which happens pretty frequently on an Ubuntu 16.04 32-bit VM. I'm not sure why this hasn't been seen in CI yet.

It would be preferable to store an AESContext * in CMACContext, and use the existing methods for allocation and destruction.

Two example crashes below. This may be triggered from Cmac or Kbkdf tests.

[1] https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/rijndael.c#821
[2] https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/cmac.c#171

[----------] 308 tests from WycheproofTestVector/Pkcs11AesCmacTest
[ RUN      ] WycheproofTestVector/Pkcs11AesCmacTest.TestVectors/0

Program received signal SIGSEGV, Segmentation fault.
0xb79059be in rijndael_native_encryptBlock (cx=0x8712e78, output=0xbfffee7c "", input=0xbfffee6c "")
    at ../../lib/freebl/aes-x86.c:160
160	    m = _mm_xor_si128(m, cx->k.keySchedule[0]);
(gdb) p &cx->k.keySchedule[0]
$1 = (__m128i *) 0x8712e78
(gdb) bt
#0  0xb79059be in rijndael_native_encryptBlock (cx=0x8712e78, output=0xbfffee7c "", input=0xbfffee6c "")
    at ../../lib/freebl/aes-x86.c:160
#1  0xb78dc3a4 in rijndael_encryptECB (cx=0x8712e78, output=0xbfffee7c "", outputLen=0xbfffee24, maxOutputLen=16, 
    input=0xbfffee6c "", inputLen=16) at ../../lib/freebl/rijndael.c:719
#2  0xb78dce15 in AES_Encrypt (cx=0x8712e78, output=0xbfffee7c "", outputLen=0xbfffee24, maxOutputLen=16, 
    input=0xbfffee6c "", inputLen=16) at ../../lib/freebl/rijndael.c:1110
#3  0xb788e55b in cmac_Encrypt (ctx=0x8712e68, output=0xbfffee7c "", input=0xbfffee6c "", inputLen=16)
    at ../../lib/freebl/cmac.c:65
#4  0xb788e608 in cmac_GenerateSubkeys (ctx=0x8712e68) at ../../lib/freebl/cmac.c:88
#5  0xb788e9b7 in CMAC_Begin (ctx=0x8712e68) at ../../lib/freebl/cmac.c:193
#6  0xb788e8fd in CMAC_Init (ctx=0x8712e68, type=CMAC_AES, 
    key=0x871e18c "\343O\025ǽ\201\231\060\376\235f\340\301f\346\034", key_len=16) at ../../lib/freebl/cmac.c:164
#7  0xb788e93b in CMAC_Create (type=CMAC_AES, key=0x871e18c "\343O\025ǽ\201\231\060\376\235f\340\301f\346\034", 
    key_len=16) at ../../lib/freebl/cmac.c:173
#8  0xb7a3c968 in CMAC_Create (type=CMAC_AES, key=0x871e18c "\343O\025ǽ\201\231\060\376\235f\340\301f\346\034", 
    key_len=16) at ../../lib/freebl/loader.c:2264
#9  0xb7a335db in sftk_MAC_InitRaw (ctx=0x87138e8, mech=4235, 

[ RUN      ] WycheproofTestVector/Pkcs11AesCmacTest.TestVectors/19

Program received signal SIGSEGV, Segmentation fault.
0xb7902c1e in native_key_expansion128 (cx=0x86966f8, key=0x86956ac "\233Ӑ.Йl\206\233W\"r\347o8\211")
    at ../../lib/freebl/aes-x86.c:27
27	    keySchedule[0] = _mm_loadu_si128((__m128i *)key);
(gdb) p &keySchedule[0]
$1 = (__m128i *) 0x86966f8
(gdb) bt
#0  0xb7902c1e in native_key_expansion128 (cx=0x86966f8, key=0x86956ac "\233Ӑ.Йl\206\233W\"r\347o8\211")
    at ../../lib/freebl/aes-x86.c:27
#1  0xb7905881 in rijndael_native_key_expansion (cx=0x86966f8, key=0x86956ac "\233Ӑ.Йl\206\233W\"r\347o8\211", 
    Nk=4) at ../../lib/freebl/aes-x86.c:130
#2  0xb78dc98f in aes_InitContext (cx=0x86966f8, key=0x86956ac "\233Ӑ.Йl\206\233W\"r\347o8\211", keysize=16, 
    iv=0x0, mode=0, encrypt=1) at ../../lib/freebl/rijndael.c:914
#3  0xb78dca99 in AES_InitContext (cx=0x86966f8, key=0x86956ac "\233Ӑ.Йl\206\233W\"r\347o8\211", keysize=16, 
    iv=0x0, mode=0, encrypt=1, blocksize=16) at ../../lib/freebl/rijndael.c:956
#4  0xb788e8e4 in CMAC_Init (ctx=0x86966e8, type=CMAC_AES, key=0x86956ac "\233Ӑ.Йl\206\233W\"r\347o8\211", 
    key_len=16) at ../../lib/freebl/cmac.c:159
#5  0xb788e93b in CMAC_Create (type=CMAC_AES, key=0x86956ac "\233Ӑ.Йl\206\233W\"r\347o8\211", key_len=16)
    at ../../lib/freebl/cmac.c:173

Comment 1

[Alexander Scheel]

Ah, this is an issue because we directly store AESContext in CMACContext. My original preference at the time was to make this a pointer to a context.

See https://phabricator.services.mozilla.com/D40120?id=140421#inline-243676 -- I'm hoping that links to the older context:

struct CMACContextStr {
    CMACCiphers cipherType;
    union {
        void *raw;
        AESContext *aes;
    } cipher;
    int blockSize;
}

@mt had this comment at the time: "You might avoid an extra allocation and associated indirection by including the aes value directly and using AES_InitContext rather than AES_CreateContext."

In particular, we no longer care about the size of CMACCiphers and trying to guess the alignment of the inner cipher struct; we can just allocate the new AES context with the correct alignment via AES_AllocateContext(...) and be done with it. Since we're likely to incur the special allocation anyway (most people have AES-NI...), I think this is a fine compromise. It also lets us deal with alignment for different ciphers as necessary, and better, defer to their allocators to handle this best.

My 2c.

I'm happy to write the patch if that's the patch we want to go this route. But if there's a better way of doing this, I'm happy to defer to someone else's patch and just review it.

Comment 2

[Kevin Jacobs [:kjacobs]]

I agree. Avoiding the allocation is nice, but will only become harder to maintain alignment with additional mechanisms.

@mt, do you have any objection to making this a pointer (or perhaps see a better solution here)?

This probably doesn't need to a be a security issue, but I'm not sure how these functions are being used outside Firefox.

Comment 3

[Alexander Scheel]

These changes are available in RHEL 8.2 beta made available today, but we haven't triggered this in our testing as far as I can tell. We (Dogtag via JSS) have plans to use this code at a later time, but hit some issues testing this with HSMs so it is currently unused. As far as I know, we're the only consumer in RHEL and we're likely the only consumer from NSS as we just introduced it.

Could we add @rrelyea to the CC list to ask his thoughts as well?

Comment 4

[Kevin Jacobs [:kjacobs]]

+Bob for feedback, as requested.

Comment 5

[Martin Thomson [:mt:]]

If this is an alignment issue, then using the pointer and deferring to the proper constructor is safest. It's annoying, but that's C.

Comment 6

[Alexander Scheel]

Attachment: AES-Alignment-in-CMAC.patch

Not sure if we're supposed to use Moz-Phab for embargoed security issues, so attaching the patch here instead.

Comment 7

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9122353 [details] [diff] [review]
AES-Alignment-in-CMAC.patch

Review of attachment 9122353 [details] [diff] [review]:
-----------------------------------------------------------------

Two fixups needed, but otherwise this fixes the issue. You can use phabricator as well - the revision will inherit permissions from the linked bug. 

Thanks!

::: lib/freebl/cmac.c
@@ +156,5 @@
>  
>      ctx->blockSize = AES_BLOCK_SIZE;
>      ctx->cipherType = CMAC_AES;
> +    ctx->cipher.aes = AES_CreateContext(key, key_len, NULL, NSS_AES, 1,
> +                                        ctx->blockSize);

This should be `AES_CreateContext(key, NULL, NSS_AES, 1, key_len, ctx->blockSize)`

@@ +310,4 @@
>      }
>  
>      if (ctx->cipherType == CMAC_AES) {
> +        AES_DestroyContext(ctx->cipher.aes, PR_FALSE);

`, PR_TRUE);`

Comment 8

[Alexander Scheel]

Attachment: Bug 1610687 - Crash on unaligned CMACContext.aes.keySchedule when using AES-NI intrinsics r=kjacobs

Comment 9

[Alexander Scheel]

Well that's embarrassing. My build script was executed out of the wrong directory. We also need ctx->cipher.aes != NULL in the prior condition, to handle a branch in AES_DestroyContext. Moved to moz-phab.

Comment 10

[Alexander Scheel]

Diaki, since Bob is out today, please see his needinfo above. Is there something we need to do from our end? Thanks!

Comment 11

[Robert Relyea]

No, I'm pickup up the patch as it appears in this bug downstream today. We have no reason to avoid the additional allocation, and leaving all the alignment worries in a single piece of well tested code is very desirable.

Should I go ahead and land this patch since it has 2 reviews and Alex doesn't have landing privelleges.

bob

Comment 12

[Martin Thomson [:mt:]]

Yes, please land the patch at your convenience Bob.

Comment 13

[Robert Relyea]

https://hg.mozilla.org/projects/nss/rev/046a6f5bfb271ed03ed6a93e3f879d57905999c5