search

ARM-software / cryptocell-312-runtime

CryptoCell 312 runtime code
23 stars 13 forks source link

Memory stomp in mbedtls_rsa_rsaes_pkcs1_v15_decrypt error path? #7

Open charlesnicholson opened 2 years ago

charlesnicholson commented 2 years ago

https://github.com/ARM-software/cryptocell-312-runtime/blob/master/codesafe/src/mbedtls_api/rsa_alt.c#L2257

Cleanup:
    if ( Error != CC_OK )
    {
        mbedtls_zeroize_internal(output, ctx->len);
    }

The length of the output is specified by the caller via the output_max_len parameter. Should this cleanup path not instead be the following?

mbedtls_zeroize_internal(output, output_max_len);

In the case where the input buffer length (ctx->len) is longer than the output buffer, the current code overflows the output buffer and "zeroize"-s random memory.

Apologies if I'm misunderstanding this.

xuyong0528 commented 2 years ago

Hi Charles, Your concern is correct. In the comment on the function "CC_RsaSchemesDecrypt" definition, there is a limitation that

                1. for PKCS #1 v.2.1  *OutputSize_ptr >= PrivKey_ptr->N.len - 2*HashLen - 2.
                2. for PKCS #1 v.1.5  *OutputSize_ptr >= PrivKey_ptr->N.len - 11.

It means that the output buffer size should be longer than ctx->len.

If the output buffer size can't meet this requirement, as you mentioned, it can overflow the output buffer.

Best Regards, Yong

charlesnicholson commented 2 years ago

So we agree that this code can overwrite arbitrary memory if the user supplies bad parameters that are fully described.

The function in question has 8 dedicated validation steps before starting to do any load-bearing work, so I think it's reasonable to say that there exists concern for validation.

The user-supplied output buffer comes with its own length field; so it seems in the spirit of the existing code to either:

  1. Not overwrite random memory, because the user has supplied the output length already.
  2. Validate that the user-supplied output length meets the requirements documented by the comment.

If it's important enough to mention as a requirement in a comment, shouldn't it also be validated at runtime?

Also consider how I discovered this: I'm attempting to decrypt a ciphertext payload that contains a 16-byte AES key. So, I create a char aes_key_plaintext[16] on the stack and call the decryption function with this buffer and an output length of 16. My RSA-encrypted ciphertext is 256 bytes, but a bug on the server caused this payload to be malformed and fail the PKCSv15 validation.

Despite having full knowledge of how big my decrypted payload would be if decryption succeeded, simply attempting to decrypt with an invalid key/ciphertext combination caused a memory overrun.

Even if what you're arguing is reasonable, I think the implementation in this case is flawed: I provide the size of my output buffer, and this function ignores it when clearing this output buffer in a failure case.

I think it's maybe a more open question about what to do when the user-provided output buffer isn't big enough to hold the successfully-decrypted plaintext, but this isn't that case.

Or, put another way, allowing this to remain is arguing that it's absolutely OK for cryptography code to overrun user buffers even when that code is given the correct buffer size. Is that the stance this codebase takes?

xuyong0528 commented 2 years ago

Hi Charles, It is a great finding in my opinion. When the output buffer size is short, CC_RsaSchemesDecrypt() will return error “CC_RSA_15_ERROR_IN_DECRYPTED_DATA_SIZE”.
Then it clears ctx->len bytes buffer, becasue the ctx->len usually is longer than the output_max_len, so, it may cause the overflow memory to be cleared.

It does make sense to clear only output_max_len bytes for the output buffer.

Just some comments are not enough to make sure it can be used correctly. Welcome you to raise a request, I can involve more guys to review it.

For some reason, this code repository is only maintained by limited resources. I can't guarantee the next release time.

Best Regards, Yong

charlesnicholson commented 2 years ago

Which branch should I base a pull request off? I see master hasn't been touched for 3 years, and update-cc110-bu-00000-r1p4 is more recent but still 2 years old.

xuyong0528 commented 2 years ago

Hi Charles, The default and latest branch is update-cc110-bu-00000-r1p4. The masteris the main branch. I'm sorry, I thought I had the permission to create a new branch to accept new pull requests, and tried to create a new development branch named such as update-cc110-bu-00000-r1p4-dev. But, I don't have permission. Hi, @spanijel Can you create a new branch "update-cc110-bu-00000-r1p4-dev" to accept new requests? Thanks

Best Regards, Yong

xuyong0528 commented 2 years ago

Hi Charles, I have good news to let you know. This issue has been fixed in `commit 20e27a5bce43555b5e99e266a03b30e42254807e Author: Antonio de Angelis Antonio.deAngelis@arm.com Date: Tue Jun 7 11:53:15 2022 +0100

CC312: Use correct size on cleanup path in RSA alt API

Make sure that the RSA alt implementation uses the buffer size
of the output as passed from the caller instead of the context
length to zeroize the output buffer on cleanup.

Signed-off-by: Antonio de Angelis <antonio.deangelis@arm.com>
Change-Id: I6399d3f410d9d331d57a9a93f56cf8f3ee69dfb7

diff --git a/lib/ext/cryptocell-312-runtime/codesafe/src/mbedtls_api/rsa_alt.c b/lib/ext/cryptoce ll-312-runtime/codesafe/src/mbedtls_api/rsa_alt.c index 0dc4dbad..ece40ec8 100644 --- a/lib/ext/cryptocell-312-runtime/codesafe/src/mbedtls_api/rsa_alt.c +++ b/lib/ext/cryptocell-312-runtime/codesafe/src/mbedtls_api/rsa_alt.c @@ -2113,7 +2113,7 @@ int mbedtls_rsa_rsaes_oaep_decrypt( mbedtls_rsa_context *ctx, Cleanup: if ( Error != CC_OK ) {

This repository is maintained by Arm OSS team as one of the components in TF-M. You can refer to the repository in the future. The current GitHub repository is redunent as above one.

There is no coming release schedule for https://github.com/ARM-software/cryptocell-312-runtime, it is assumed that this repository will be closed.

Best Regards, Yong

charlesnicholson commented 2 years ago

Great, thanks! Also +1 to closing this repository ASAP. If this repository is just an out-of-date mirror, and not the place users are supposed to find the latest security patches, then it's probably actively harmful to leave it up.