Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CHACHA20_POLY1305 different results for chunked/non-chunked updating #8675

Open
guidovranken opened this issue Apr 4, 2019 · 7 comments

Comments

Projects
None yet
4 participants
@guidovranken
Copy link
Contributor

commented Apr 4, 2019

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;
}
@levitte

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

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?

@davidben

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

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.)

@davidben

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

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

levitte added a commit to levitte/openssl that referenced this issue Apr 4, 2019

EVP_*Update: ensure that input NULL with length 0 isn't passed
Even with custome ciphers, the combination in == NULL && inl == 0
should not be passed down to the backend cipher function.  The reason
is that these are the values passed by EVP_*Final, and some of the
backend cipher functions do check for these to see if a "final" call
is made.

Fixes openssl#8675
@guidovranken

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

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.

@levitte

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

See if the fix in #8676 makes a difference for you.

@guidovranken

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Yep this fixes it, thanks.

levitte added a commit that referenced this issue Apr 10, 2019

EVP_*Update: ensure that input NULL with length 0 isn't passed
Even with custome ciphers, the combination in == NULL && inl == 0
should not be passed down to the backend cipher function.  The reason
is that these are the values passed by EVP_*Final, and some of the
backend cipher functions do check for these to see if a "final" call
is made.

Fixes #8675

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #8676)

(cherry picked from commit dcb982d)

@levitte levitte closed this in dcb982d Apr 10, 2019

@mattcaswell

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Reopening this since #8676 got reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.