1368859 - (CVE-2017-7822) AES-GCM implementation of Web Crypto API accepts 0-length IV

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Thanh Bui]

Attachment: AesGcmBug.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.81 Safari/537.36

Steps to reproduce:

- Generated a AES-GCM key.
- Used the key to encrypt an arbitrary message using an empty buffer as the IV.
(The attached file demonstrates the steps.)


Actual results:

The encryption didn't throw any error.


Expected results:

The encryption function shouldn't accept 0-length IV. 

According to NIST SP 800 38d, Section 5.2.1.1 (http://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf), AES-GCM should allow only IVs of bit length 1 .. 2^64-1. The reason is that from ciphertexts encrypted with 0-length IVs or all-zero IVs, it is possible to determine the authentication key.

Comment 1

[Thanh Bui]

The bug was found by Google's Wycheproof project (https://github.com/google/wycheproof).

Comment 2

[:Gijs (he/him)]

Tim, looks like a webcrypto or nss issue? Either way, can you have a look / forward as appropriate? :-)

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to thanhb from comment #0)
> The encryption function shouldn't accept 0-length IV. 

Indeed, it shouldn't. The WebCrypto spec [1] unfortunately says nothing about that.

[1] https://w3c.github.io/webcrypto/Overview.html#aes-gcm-operations

> According to NIST SP 800 38d, Section 5.2.1.1
> (http://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.
> pdf), AES-GCM should allow only IVs of bit length 1 .. 2^64-1. The reason is

The GCM spec also says: "Although GCM is defined on bit strings, the bit lengths of the plaintext, the AAD, and the IV shall all be multiples of 8, so that these values are byte strings."

So effectively we should accept IVs of length [1*8..2^64/8) = [1..2^61) bytes. The WebCrypto spec talks about a max. IV length of 2^64 bytes - that's 8 times what GCM allows. I don't think people actually use IVs of that size but the spec seems definitely wrong here.

> that from ciphertexts encrypted with 0-length IVs or all-zero IVs, it is
> possible to determine the authentication key.

We should reject 0-length IVs in the WebCrypto API and probably also NSS itself. For IVs with len(IV) != 96 bits we could have a check to ensure the GHASH result is non-zero. This is not an issue for TLS but it might be for anyone linking against NSS and using AES-GCM directly.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1368859-Web-Crypto-API-should-reject-0-length-AE.patch

Bug 1369353 fixed the underlying issue, this patch just adds a few tests.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8879466 [details] [diff] [review]
0001-Bug-1368859-Web-Crypto-API-should-reject-0-length-AE.patch

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

LGTM - just a few nits.

::: dom/crypto/test/test_WebCrypto.html
@@ +752,5 @@
> +
> +    crypto.subtle.importKey("raw", tv.aes_gcm_enc.key, "AES-GCM", false, ['encrypt'])
> +      .then(doEncrypt)
> +      .then(error(that))
> +      .catch(complete(that));

I think it would be most consistent to format these last two lines as:

.then(error(that), complete(that));

but I realize they're equivalent.

@@ +758,5 @@
> +);
> +
> +// -----------------------------------------------------------------------------
> +TestArray.addTest(
> +  "AES-GCM encryption, accept an all-zero IV (1 byte)",

So, is the all-zero IV concern from comment 0 not an issue here?

@@ +780,5 @@
> +);
> +
> +// -----------------------------------------------------------------------------
> +TestArray.addTest(
> +  "AES-GCM encryption, accept an all-zero, 12-byte IV",

nit: might make this description consistent with the other all-zero x-byte IV testcases

@@ +813,5 @@
> +      tagLength: 128
> +    };
> +
> +    function doEncrypt(x) {
> +      return crypto.subtle.encrypt(alg, x, tv.aes_gcm_dec_fail.data);

Not that it really matters since we're not decrypting this again, but shouldn't this be tv.aes_gcm_enc.data?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #5)
> > +// -----------------------------------------------------------------------------
> > +TestArray.addTest(
> > +  "AES-GCM encryption, accept an all-zero IV (1 byte)",
>
> So, is the all-zero IV concern from comment 0 not an issue here?

It would certainly be ideal if we rejected that, but the standard actually mentions this as a possible use-case... I know. Also I think this is semantically a little different from just forgetting to specify the IV, or have it be completely empty. Another reason to not make this change is that we'd likely break communication with not-so-intelligent middle boxes.

> > +    function doEncrypt(x) {
> > +      return crypto.subtle.encrypt(alg, x, tv.aes_gcm_dec_fail.data);
>
> Not that it really matters since we're not decrypting this again, but
> shouldn't this be tv.aes_gcm_enc.data?

Oops, yeah. I actually tested rejecting all-zero IVs first :)

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94365aa1f128

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/94365aa1f128

Comment 9

[Daniel Veditz [:dveditz]]

This bug does not qualify for a bug bounty. Although a bug that allows a user of the API to do the wrong thing, it would be a vulnerability in that using code not Firefox itself. Bad code could re-use IV's, for an example of misuse we can't prevent.