Repository: cossacklabs/themis (language: rust)

Issue:
RSA key generation silently truncates private keys
### Description

RSA key generation incorrectly handles invalid arguments, silently producing truncated key data. This may cause sleeper bugs in user code which may be relatively hard to spot and debug.

### Environment

**OS:** Ubuntu 18.04

**Themis version:** 0.10 release

**Crypto provider:** OpenSSL 1.1.0g

### Details

Steps to reproduce:

1. Use [this code](https://gist.github.com/ilammy/76cb5f8de8ceeac583b9fd9645673b76).
2. Play with `MAX_KEY_LENGTH` constant.
3. Compile, run, observe.

**Expected result:**

I'd expect `themis_gen_rsa_key_pair()` to fail with THEMIS_BUFFER_TOO_SMALL when the buffer is actually too small to fit either private or public key.

**Actual result:**

Key generation appears to be successful, while in fact the key has been truncated and is unusable. The following call to `themis_secure_message_wrap()` fails later with THEMIS_INVALID_PARAMETER.

### Notes

This issue does not seem to be actual for ECDSA key generation. ECDSA keys have quite compact representation (~40 bytes) and both private and public keys have the same length. If the buffer is too small then `themis_gen_ec_key_pair()` always complains about that.

The issue is not actual if the key pointers are NULL. In this case the length checks are performed correctly and key generation fails with the expected status code. Therefore the issue is unlikely to affect the user code which follows the recommended check–allocate–call pattern.

However, it may catch some overly optimistic users off guard. Those smart ones who _just know_ that 512 bytes should _definitely_ be enough for an RSA-2048 key and they don't need to check the length parameter afterwards. Or if they do use the key length blindly (expecting it to be less than 512) and cause a buffer overflow in their code because the returned length is actually _bigger_ than their buffer.


Discussion / hints:

Fix patch:
diff --git a/src/themis/secure_message.c b/src/themis/secure_message.c
index 6f5abcba8..d478e4cf2 100644
--- a/src/themis/secure_message.c
+++ b/src/themis/secure_message.c
@@ -31,7 +31,7 @@ themis_status_t themis_gen_key_pair(soter_sign_alg_t alg,
   soter_sign_ctx_t* ctx=soter_sign_create(alg,NULL,0,NULL,0);
   THEMIS_CHECK(ctx!=NULL);
   soter_status_t res=soter_sign_export_key(ctx, private_key, private_key_length, true);
-  if(res!=THEMIS_SUCCESS && res != THEMIS_BUFFER_TOO_SMALL){
+  if(res!=THEMIS_SUCCESS && res!=THEMIS_BUFFER_TOO_SMALL){
     soter_sign_destroy(ctx);
     return res;
   }
@@ -52,10 +52,22 @@ themis_status_t themis_gen_rsa_key_pair(uint8_t* private_key,
 				    uint8_t* public_key,
 				    size_t* public_key_length){
   soter_rsa_key_pair_gen_t* key_pair_ctx=soter_rsa_key_pair_gen_create(THEMIS_RSA_KEY_LENGTH);
-  themis_status_t res=soter_rsa_key_pair_gen_export_key(key_pair_ctx, private_key, private_key_length, true);
-  themis_status_t res1=soter_rsa_key_pair_gen_export_key(key_pair_ctx, public_key, public_key_length, false);
+  THEMIS_CHECK(key_pair_ctx!=NULL);
+  soter_status_t res=soter_rsa_key_pair_gen_export_key(key_pair_ctx, private_key, private_key_length, true);
+  if(res!=THEMIS_SUCCESS && res != THEMIS_BUFFER_TOO_SMALL){
+    soter_rsa_key_pair_gen_destroy(key_pair_ctx);
+    return res;
+  }
+  soter_status_t res2=soter_rsa_key_pair_gen_export_key(key_pair_ctx, public_key, public_key_length, false);
+  if(res2!=THEMIS_SUCCESS && res2!=THEMIS_BUFFER_TOO_SMALL){
+    soter_rsa_key_pair_gen_destroy(key_pair_ctx);
+    return res2;
+  }
   soter_rsa_key_pair_gen_destroy(key_pair_ctx);
-  return (res<res1)?res:res1;
+  if(res==THEMIS_BUFFER_TOO_SMALL || res2==THEMIS_BUFFER_TOO_SMALL){
+    return THEMIS_BUFFER_TOO_SMALL;
+  }
+  return THEMIS_SUCCESS;
 }
 
 themis_status_t themis_gen_ec_key_pair(uint8_t* private_key,


Test patch: