Bug 1368859 (CVE-2017-7822)

AES-GCM implementation of Web Crypto API accepts 0-length IV

RESOLVED FIXED in Firefox 56

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: thanhb, Assigned: ttaubert)

Tracking

({sec-low})

53 Branch
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty -
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56 fixed)

Details

(Whiteboard: [domsecurity-active][adv-main56+][post-critsmash-triage])

Attachments

(2 attachments)

Reporter

Description

2 years ago
Posted file 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.
Reporter

Comment 1

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

Comment 2

2 years ago
Tim, looks like a webcrypto or nss issue? Either way, can you have a look / forward as appropriate? :-)
Group: firefox-core-security → core-security
Component: Untriaged → Security: PSM
Flags: needinfo?(ttaubert)
Product: Firefox → Core
(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.
Status: UNCONFIRMED → NEW
Component: Security: PSM → DOM: Security
Ever confirmed: true
Flags: needinfo?(ttaubert)
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Flags: sec-bounty?
Depends on: 1369353
Keywords: sec-low
Priority: -- → P2
Whiteboard: [domsecurity-active]
Flags: needinfo?(ttaubert)
Group: core-security → dom-core-security
Flags: needinfo?(ttaubert)
Bug 1369353 fixed the underlying issue, this patch just adds a few tests.
Attachment #8879466 - Flags: review?(dkeeler)
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?
Attachment #8879466 - Flags: review?(dkeeler) → review+
(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 :)
https://hg.mozilla.org/mozilla-central/rev/94365aa1f128
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Group: dom-core-security → core-security-release
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.
Flags: sec-bounty? → sec-bounty-
Alias: CVE-2017-7822
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main56+]
Flags: qe-verify-
Whiteboard: [domsecurity-active][adv-main56+] → [domsecurity-active][adv-main56+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.