1515011 - ChaCha20-Poly1305 decryption with empty message and fake tag is successful

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Jonas Allmann [:jallmann]]

Wycheproof testcases for ChaCha20-Poly1305 with empty message reveal that decryption with a manipulated tag for this empty message is successful.
The testcases are currently excluded for a different reason (see 
https://bugzilla.mozilla.org/show_bug.cgi?id=1515007
), fixing which reveals this seemingly unrelated bug.


<<>>
Steps to reproduce:

Checkout current version of NSS.

Execute './mach wycheproof' to re-include testcases.

Change the following line
https://searchfox.org/nss/rev/7bc70a3317b800aac07bad83e74b6c79a9ec5bff/lib/softoken/pkcs11c.c#1628

to

        *pulDataLen = 0;

Build NSS.

run '../dist/Debug/bin/pk11_gtest --gtest_filter="*ChaCha20*" '


<<>>
Expected result:

No test case failures.


<<>>
Actual result:

22 test cases fail, because several attempts of decryption with manipulated ciphertext, tag, IV or AAD are unexpectedly successful.

Comment 1

[J.C. Jones [:jcj] (he/they)]

Since it's with empty ciphertext, an attacker on the wire couldn't use this to directly affect the encrypted conversation, but it's clearly a major issue to be able to spoof the tag. Marking sec-moderate to start; we might need to up to a sec-high if we see any opportunity to affect TLS here.

Comment 2

[Jonas Allmann [:jallmann]]

    Further investigation showed that the same piece of code is responsible for this behaviour as for https://bugzilla.mozilla.org/show_bug.cgi?id=1515011

    In cases where the message originally encrypted was empty and we therefore expect an empty result from decryption, the test passes a null-pointer to the decryption function, which causes it to return success without any further checks on validity Tag, IV or AAD.

Comment 3

[Martin Thomson [:mt:]]

I don't see the code that is busted, where is it?

I do see that ChaCha20Poly1305_Open isn't constant time.  If the tag is bad, we don't bother to run the XOR.

Comment 4

[Franziskus Kiefer [:franziskus]]

It doesn't get into ChaCha20Poly1305_Open. The softoken bails at [1].

[1] https://searchfox.org/nss/rev/7bc70a3317b800aac07bad83e74b6c79a9ec5bff/lib/softoken/pkcs11c.c#1627

Comment 5

[Martin Thomson [:mt:]]

Thanks.  That is not good, especially when we promote "" to NULL.  What concerns me is that there might be code that relies on this succeeding when passed NULL.  We will have to test it carefully.

Comment 6

[J.C. Jones [:jcj] (he/they)]

Targeting NSS 3.43 for this.

Comment 7

[Kevin Jacobs [:kjacobs]]

The current implementation follows behavior described in PKCS11 ("1. If pBUF is NULL_PTR...") [0].

The test allocates an output decrypted vector of size plainTextBytes, i.e. zero. The vector.data() call then returns a NULL pointer, which NSC_Decrypt interprets as a request for the output size, rather than an actual decryption. For comparison, the AES-GCM unit test allocates the decrypted output vector with size cipherTextBytes (which in this case would be 16 - just the tag). Production uses of PK11_Decrypt() seem to correctly account for this and prevent NULL from being passed in (whether intentional or not...)

The correct (per-spec) fix is to change the test [1] to allocate cipherTextBytes for decryption output.

FWIW, Windows APIs [2] behave the same way, and mention AEAD Decrypt handling:

1. If output pBuf is NULL, it's a request for the plaintext size
2. Else if output pBuf and input pCtxt are both NULL, AND mechanism is AEAD, it's a decrypt/authenticate on a zero-length ciphertext*
3. Else Error

*In PKCS11 case for AEAD decryption, the tag is appended to the ciphertext (in Windows API it's in a separate struct).. Thus the decryption input should never be NULL (though we don't enforce it at the PK11 layer).

I have a patch for this, with the NULL check mentioned, being tested.

[0] - https://www.cryptsoft.com/pkcs11doc/v220/group__SEC__11__2__CONVENTIONS__FOR__FUNCTIONS__RETURNING__OUTPUT__IN__A__VARIABLE__LENGTH__BUFFER.html

[1] - https://searchfox.org/mozilla-central/source/security/nss/gtests/pk11_gtest/pk11_chacha20poly1305_unittest.cc#62

[2] - https://docs.microsoft.com/en-us/windows/desktop/api/bcrypt/nf-bcrypt-bcryptdecrypt

Comment 8

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1515011 - Update Chacha20poly1305 tests to not pass NULL pBuf to PK11_Decrypt when the plaintext message is empty. r=jcj

Comment 9

[J.C. Jones [:jcj] (he/they)]

mt, I'd like to get your eyes on this analysis, too. Kevin walked me through the code paths for TLS1.3 and SSL3 and I agree that we don't seem to have a path that would result in a null buffer as an input, so it does look like this is a test design error to me, too, but given the consequences, I'd appreciate a second opinion.

Comment 10

[Martin Thomson [:mt:]]

https://searchfox.org/nss/rev/e0bab5ba1837523ba9eb361b7502cb5076867283/lib/ssl/tls13con.c#5359 shows that we're safe in TLS 1.3, because we won't pass a zero-length input to PK11_Decrypt.

https://searchfox.org/nss/rev/e0bab5ba1837523ba9eb361b7502cb5076867283/lib/ssl/ssl3con.c#12312 shows the same check, albeit with a more complicated setup.

So TLS is fine.

The effect of failing to produce input should always be to fail. The effect of failing to produce an output buffer is exactly as Kevin has described.

I'll put some more comments on the bug.

Comment 11

[J.C. Jones [:jcj] (he/they)]

Thanks, mt. OK, so we agree that this is not a security bug at all.

(Assuming no one disagrees with that statement, I'll unhide the bug tomorrow).

Comment 12

[Martin Thomson [:mt:]]

Yes, there are no security implications. Unhide at will.

Comment 13

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/b8ff92e0509cf05b4ba0c59ecb905c7610596795