CHACHA20_POLY1305 different results for chunked/non-chunked updating
guidovranken opened this issue · 11 comments
The following SHOULD produce the same output whether you compile with -DCHUNKED or not, but the last byte differs.
I deliberately pass NULL to EVP_DecryptUpdate if the chunk size is 0.
If you pass a non-NULL pointer, the results are the same.
#include <openssl/evp.h>
#define CF_CHECK_EQ(expr, res) if ( (expr) != (res) ) { goto end; }
#define CF_CHECK_NE(expr, res) if ( (expr) == (res) ) { goto end; }
int main(void)
{
const EVP_CIPHER* cipher = NULL;
EVP_CIPHER_CTX* ctx = NULL;
const unsigned char key[32] = {0x00, 0x00, 0x00, 0x00, 0x00, 0xf9, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0xdc, 0x4d, 0xad, 0x6b, 0x06, 0x93, 0x4f, 0x29, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
unsigned char iv[8] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
unsigned char ciphertext[100] = {
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0xd5, 0x00, 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, 0xd5, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x9c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x27, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00 };
unsigned char cleartext[1024];
int len = 0;
size_t inIdx = 0;
size_t outIdx = 0;
OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CRYPTO_STRINGS, NULL);
CF_CHECK_NE(cipher = EVP_chacha20_poly1305(), NULL);
CF_CHECK_NE(ctx = EVP_CIPHER_CTX_new(), NULL);
CF_CHECK_EQ(EVP_DecryptInit_ex(ctx, cipher, NULL, NULL, NULL), 1);
CF_CHECK_EQ(EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_IVLEN, sizeof(iv), NULL), 1);
CF_CHECK_EQ(EVP_DecryptInit_ex(ctx, NULL, NULL, key, iv), 1);
#if defined(CHUNKED)
const int lengths[] = {100};
#else
const int lengths[] = {73, 11, 1, 8, 5, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}; /* sum = 100 */
#endif
for (size_t i = 0; i < sizeof(lengths) / sizeof(lengths[0]); i++) {
CF_CHECK_EQ(EVP_DecryptUpdate(ctx, cleartext + outIdx, &len, lengths[i] == 0 ? NULL : ciphertext + inIdx, lengths[i]), 1);
inIdx += lengths[i];
outIdx += len;
}
CF_CHECK_EQ(EVP_DecryptFinal_ex(ctx, cleartext + outIdx, &len), 1);
outIdx += len;
for (int i = 0; i < outIdx; i++) {
printf("%02X ", cleartext[i]);
}
printf("\n");
end:
return 0;
}Hmmm... The backend ChaCha20-Poly1305 cipher function understands a NULL input as a signal that "final" processing should be performed... and this is quite right, EVP_DecryptFinal does call the backend cipher function with a NULL input.
Of course, it can be argued that EVP_DecryptUpdate should check that in isn't NULL before passing it down to the baclend cipher function. However, the ChaCha20-Poly1305 is a custom cipher, which means that the EVP layer functions do nothing but passing arguments along to the backend with no checks. This is a fragility (and it should be fixed in my opinion)
Does the rest of @openssl have comments on this?
I believe the AES-GCM EVP_CIPHER also treats a NULL input funny. They also treat a NULL output funny as that's how you pass in the AD to an AEAD EVP_CIPHER. That one's certainly stuck. The NULL input thing may also be stuck given do_cipher is exposed out of EVP_Cipher... :-/
Not that this is at all a reasonable API. The problem is EVP_CIPHER tries to abstract together too many things that are fundamentally different objects. Go, for instance, which has multiple "cipher" interfaces. In BoringSSL, we have a separate EVP_AEAD interface which we've found to be far less error-prone.
(You mostly need an AEAD API at this point. Other modes are rare in modern protocols. Maybe the occasional low-level single-block operation for weird stuff.)
I believe the AES-GCM EVP_CIPHER also treats a NULL input funny.
Oops, forgot the link.
https://github.com/openssl/openssl/blob/master/crypto/evp/e_aes.c#L3355-L3369
All that I have to add is that a scenario in which NULL is passed to EVP_DecryptUpdate is quite conceivable from a C++ perspective, where an std::vector<unsigned char>'s data() method (that gives you access to the bytes in the vector) returns NULL if the vector is empty (as I explained in another thread as well).
I can think of all kinds of situations where data to be decrypted isn't neatly organized in a single block, but instead chunked, including empty chunks.
So as far as I'm concerned, OpenSSL (or any API really) should always treat a pointer/size pair that is NULL/0 the same as it would valid ptr/0. Either that, or return an error.
Yep this fixes it, thanks.
Reopening this since #8676 got reverted.
New fix for this in #9057.
A lot has happened since this issue. Is it still current?
Needs some investigation following all the recent merges in this area.