Possible memory corruption / leak in mbedtls AES implementation (iv)

[adelinu]

Hi,

I found a bug in the implementation of AES under mbedtls (block implementation).

When encrypting something using CBC by calling mbedtls_aes_crypt_cbc, you are actually calling esp_aes_crypt_cbc.
Notice that the function expects the initialization vector to be given as unsigned char iv[16] (so not a pointer).

However, while encrypting, at line 251, the IV argument memory block (which is referenced at line 210 by uint32_t *iv_words = (uint32_t *)iv; ), is rewritten with newly calculated values of the output words.

This makes the memory contents of the given argument "iv" to change to those values, so when you try to work with the variable after calling mbedtls_aes_crypt_cbc() you'll be surprised that it does not have the initial value.
This shouldn't happen as the IV contents shouldn't change, even more when the argument is not a pointer.

PS: I saw that there is an alternative DMA implementation for mbedtls, however I don't know when it's used.
Including "mbedtls/aes.h" links to the function metioned above (block implementation).

Hope it helps,
Thanks,

A.

[ESP_Angus]

Hi A,

Notice that the function expects the initialization vector to be given as unsigned char iv[16] (so not a pointer).

The C language has a very simple type system, so simple that it barely distinguishes between arrays and pointers.

There is almost no difference between declaring an argument as "unsigned char *iv" and "unsigned char iv[16]". Array parameters of this kind are not passed by value, they're passed by reference as if this was a pointer.

This shouldn't happen as the IV contents shouldn't change, even more when the argument is not a pointer.

If the same AES-CBC session is going to be used to encrypt more data then it is necessary for the caller to receive the updated IV, as the IV value changes after each block is processed.

[adelinu]

My point was that usually the IV is sent along with the encrypted payload, so it will be in an output string at some point.
As its value is changed by the function, it means that the output will no longer be valid as the IV won't match anymore the encrypted string. That's why the function shouldn't change it...

And also, it defeats the general purpose of an non-reference parameter given to a function. The memory value of the parameter should remain the same and not violated by the internal mechanisms of the function, unless it's referenced and documented accordingly.

[ESP_Sprite]

My point was that usually the IV is sent along with the encrypted payload, so it will be in an output string at some point.
As its value is changed by the function, it means that the output will no longer be valid as the IV won't match anymore the encrypted string. That's why the function shouldn't change it...

So what should it do with the new IV? If you use another parameter for this, you're complicating the API, just in case someone happens to want to keep the original IV. (Which tends to be e.g. a constant anyway, so there's normally very little use in keeping it.)

And also, it defeats the general purpose of an non-reference parameter given to a function. The memory value of the parameter should remain the same and not violated by the internal mechanisms of the function, unless it's referenced and documented accordingly.

C has the very useful keyword 'const' for this. If a parameter is not 'const', you can assume that the function may mess with the (pointed-at) constants.

[adelinu]

I'm afraid you don't get my point.

To encrypt or decrypt a string using AES (e.g. CBC), you need the following:
-An initialization vector (IV), which is a random string you generate before encryption (which is sent along with the encrypted payload)
-An encryption key
-The payload (text) to be encrypted

After calling the mbedtls encrypt function with these parameters, you get the char array that actually contains the encrypted string.

The key remains secret between parties, while the IV and Encrypted Payload are sent together (in a concatenated form) for decryption.

The current implementation changes the IV when it does the encryption, which means, that if you don't previously save the IV in a different variable, the IV will no longer correspond to the encrypted string - therefore the decryption at the other end - will fail.

That's why I keep saying that the IV should not be changed by the encryption function.
Internally, it should save it in another variable if it's really imperative and change that one eventually, but not the given IV parameter.

It will not complicate the API in any way, the arguments will remain the same.
It's a bug and can be easily fixed with 3-5 lines of code. I can make a PR with it if it's accepted.

[WiFive]

It has to be consistent with the original mbedtls implementation

https://github.com/ARMmbed/mbedtls/blob ... #L280-L285

[adelinu]

Oh, I get it now (and somehow makes sense)...
Thank you for your clarification, really helps!
A.