rsa_decrypt_key() CRYPT_BUFFER_OVERFLOW handling #592

New issue

Closed

opened 2022-07-15 22:47:26 -05:00 by suiljex · 1 comment

suiljex commented

2022-07-15 22:47:26 -05:00

(Migrated from github.com)

Prerequisites

Checked the developer manual
Checked that your issue isn't already filed: https://github.com/issues?utf8=✓&q=repo%3Alibtom%2Flibtomcrypt
Checked that your issue isn't related to TomsFastMath's limitation that PK operations can by default only be done with max. 2048bit keys

Description

rsa_decrypt_key doesn't properly handle buffer overflows. Instead of setting outlen to the required size to match the output and returning CRYPT_BUFFER_OVERFLOW, it leaves outlen untouched and returns CRYPT_INVALID_PACKET.

Steps to Reproduce

Code snippet:

#include <tomcrypt.h>

#include <cstdio>
#include <cstdint>
#include <vector>

int rsa_error_example()
{
  int err;
  unsigned long outlen;

  rsa_key key;
  const int bitsize = 1024;

  std::vector<uint8_t> aes_key(16); // To be encrypted with RSA
  sprng_read(aes_key.data(), aes_key.size(), nullptr);

  // Generate key

  int prng_idx = register_prng(&sprng_desc);
  if (prng_idx < 0)
  {
    return -1;
  }

  err = rsa_make_key(NULL, prng_idx, bitsize / 8, 65537, &key);
  if (err != CRYPT_OK)
  {
    return err;
  }

  // Encrypt data

  int hash_idx = register_hash(&sha256_desc);
  if (hash_idx < 0 || prng_idx < 0)
  {
    return -1;
  }

  std::vector<uint8_t> aes_key_encr(1);
  outlen = aes_key_encr.size();
  err = rsa_encrypt_key(aes_key.data(), aes_key.size(), aes_key_encr.data(), &outlen, NULL, 0, NULL, prng_idx, hash_idx, &key);
  if (err == CRYPT_BUFFER_OVERFLOW)
  {
    aes_key_encr.resize(outlen);
    err = rsa_encrypt_key(aes_key.data(), aes_key.size(), aes_key_encr.data(), &outlen, NULL, 0, NULL, prng_idx, hash_idx, &key);
  }

  if (err != CRYPT_OK)
  {
    return err;
  }

  aes_key_encr.resize(outlen);

  // Decrypt data

  int stat;
  std::vector<uint8_t> aes_key_decr(1);
  outlen = aes_key_decr.size();
  err = rsa_decrypt_key(aes_key_encr.data(), aes_key_encr.size(), aes_key_decr.data(), &outlen, NULL, 0, hash_idx, &stat, &key);
  if (err == CRYPT_BUFFER_OVERFLOW) // Error here
  // Expected: err == CRYPT_BUFFER_OVERFLOW   outlen == 16
  // Real:     err == CRYPT_INVALID_PACKET    outlen == 1
  {
    aes_key_decr.resize(outlen);
    err = rsa_decrypt_key(aes_key_encr.data(), aes_key_encr.size(), aes_key_decr.data(), &outlen, NULL, 0, hash_idx, &stat, &key);
  }

  if (err != CRYPT_OK || stat != 1)
  {
    return err;
  }

  aes_key_decr.resize(outlen);

  // Check

  if (aes_key != aes_key_decr)
  {
    return -1;
  }

  return 0;
}

int main(void)
{
  ltc_mp = ltm_desc;

  if (rsa_error_example() != 0)
  {
    printf("ERROR\n");
    return 1;
  }

  return 0;
}

Version

v1.18.2-654-g06a81aeb-dirty

Additional Information

I think the problem is with these functions:
https://github.com/libtom/libtomcrypt/blob/910d6252770f1e517d9ed02dc0549a1d61dfe159/src/pk/pkcs1/pkcs_1_oaep_decode.c#L148
https://github.com/libtom/libtomcrypt/blob/910d6252770f1e517d9ed02dc0549a1d61dfe159/src/pk/pkcs1/pkcs_1_v1_5_decode.c#L88

### Prerequisites * [x] Checked the developer manual * [x] Checked that your issue isn't already filed: https://github.com/issues?utf8=✓&q=repo%3Alibtom%2Flibtomcrypt * [ ] Checked that your issue isn't related to TomsFastMath's limitation that PK operations can by default only be done with max. 2048bit keys ### Description `rsa_decrypt_key` doesn't properly handle buffer overflows. Instead of setting `outlen` to the required size to match the output and returning `CRYPT_BUFFER_OVERFLOW`, it leaves `outlen` untouched and returns `CRYPT_INVALID_PACKET`. ### Steps to Reproduce Code snippet: ``` C++ #include <tomcrypt.h> #include <cstdio> #include <cstdint> #include <vector> int rsa_error_example() { int err; unsigned long outlen; rsa_key key; const int bitsize = 1024; std::vector<uint8_t> aes_key(16); // To be encrypted with RSA sprng_read(aes_key.data(), aes_key.size(), nullptr); // Generate key int prng_idx = register_prng(&sprng_desc); if (prng_idx < 0) { return -1; } err = rsa_make_key(NULL, prng_idx, bitsize / 8, 65537, &key); if (err != CRYPT_OK) { return err; } // Encrypt data int hash_idx = register_hash(&sha256_desc); if (hash_idx < 0 || prng_idx < 0) { return -1; } std::vector<uint8_t> aes_key_encr(1); outlen = aes_key_encr.size(); err = rsa_encrypt_key(aes_key.data(), aes_key.size(), aes_key_encr.data(), &outlen, NULL, 0, NULL, prng_idx, hash_idx, &key); if (err == CRYPT_BUFFER_OVERFLOW) { aes_key_encr.resize(outlen); err = rsa_encrypt_key(aes_key.data(), aes_key.size(), aes_key_encr.data(), &outlen, NULL, 0, NULL, prng_idx, hash_idx, &key); } if (err != CRYPT_OK) { return err; } aes_key_encr.resize(outlen); // Decrypt data int stat; std::vector<uint8_t> aes_key_decr(1); outlen = aes_key_decr.size(); err = rsa_decrypt_key(aes_key_encr.data(), aes_key_encr.size(), aes_key_decr.data(), &outlen, NULL, 0, hash_idx, &stat, &key); if (err == CRYPT_BUFFER_OVERFLOW) // Error here // Expected: err == CRYPT_BUFFER_OVERFLOW outlen == 16 // Real: err == CRYPT_INVALID_PACKET outlen == 1 { aes_key_decr.resize(outlen); err = rsa_decrypt_key(aes_key_encr.data(), aes_key_encr.size(), aes_key_decr.data(), &outlen, NULL, 0, hash_idx, &stat, &key); } if (err != CRYPT_OK || stat != 1) { return err; } aes_key_decr.resize(outlen); // Check if (aes_key != aes_key_decr) { return -1; } return 0; } int main(void) { ltc_mp = ltm_desc; if (rsa_error_example() != 0) { printf("ERROR\n"); return 1; } return 0; } ``` ### Version v1.18.2-654-g06a81aeb-dirty ### Additional Information I think the problem is with these functions: <https://github.com/libtom/libtomcrypt/blob/910d6252770f1e517d9ed02dc0549a1d61dfe159/src/pk/pkcs1/pkcs_1_oaep_decode.c#L148> <https://github.com/libtom/libtomcrypt/blob/910d6252770f1e517d9ed02dc0549a1d61dfe159/src/pk/pkcs1/pkcs_1_v1_5_decode.c#L88>

sjaeckel commented

2022-07-16 07:20:53 -05:00

(Migrated from github.com)

When looking at RFC8017 Section 7.1.2 it clearly states that there shall only be two error cases: "message too long"; "label too long"

"message too long" translates to CRYPT_PK_INVALID_SIZE
"label too long" translates to CRYPT_HASH_OVERFLOW (even though it's not really implemented [0])

The correct way to work with the RSA API is: use rsa_get_size() and pre-allocate the maximum size for the output buffer.

I don't have the necessary cryptography knowledge to decide whether "rsa_encrypt_key() returning CRYPT_BUFFER_OVERFLOW" poses a security problem, but I hope someone reading this issue tracker and having that knowledge steps in here :)
My feeling tells me "we should stick to the standard", so if there's nobody commenting on this I'll remove that functionality in a future version.

[0] ltc won't allow you to process more than 2^64-1 octets through the RSA API, since its lparamlen argument is max. 64bit wide. This would also mean that we could potentially overflow the as of RFC8017 resp. RFC3447 proposed input limitation of SHA-1 (2^61 - 1 octets). I couldn't find a reference where 2^61 - 1 stems from, but I'd simply accept it. I also couldn't find any input limitation numbers for other hashes besides the statement that SHA3 isn't vulnerable anymore to length-extension attacks https://keccak.team/keccak_strengths.html.

I agree that this is not consistent, but I'm not entirely sure what the correct solution should look like ... I'm tempted to argue in a very different than you expected I guess, and that's: "`rsa_encrypt_key()` shouldn't return `CRYPT_BUFFER_OVERFLOW` in the first place" When looking at [RFC8017 Section 7.1.2](https://datatracker.ietf.org/doc/html/rfc8017#section-7.1.2) it clearly states that there shall only be two error cases: "message too long"; "label too long" * "message too long" translates to `CRYPT_PK_INVALID_SIZE` * "label too long" translates to `CRYPT_HASH_OVERFLOW` (even though it's not really implemented [0]) The correct way to work with the RSA API is: use `rsa_get_size()` and pre-allocate the maximum size for the output buffer. I don't have the necessary cryptography knowledge to decide whether "`rsa_encrypt_key()` returning `CRYPT_BUFFER_OVERFLOW`" poses a security problem, but I hope someone reading this issue tracker and having that knowledge steps in here :) My feeling tells me "we should stick to the standard", so if there's nobody commenting on this I'll remove that functionality in a future version. [0] ltc won't allow you to process more than 2^64-1 octets through the RSA API, since its `lparamlen` argument is max. 64bit wide. This would also mean that we could potentially overflow the as of RFC8017 resp. RFC3447 proposed input limitation of SHA-1 (2^61 - 1 octets). I couldn't find a reference where `2^61 - 1` stems from, but I'd simply accept it. I also couldn't find any input limitation numbers for other hashes besides the statement that SHA3 isn't vulnerable anymore to length-extension attacks https://keccak.team/keccak_strengths.html.