Repository: randombit/botan
Pull request #1979 (2019-05-26 23:09:40)

Fix PKCS#11 C_Decrypt, C_Encrypt, C_Sign, C_SignFinal buffer output size.
~~The spec does say that modules should set pulBufLen to the exact byte
count when pBuf is not NULL. However, some modules fail to do this and
it's easy to work around here.~~

EDIT: See below.

This is a contribution from [Ribose Inc](https://www.ribose.com) (@riboseinc).

dewyatt: Actually I'm a little tired and it looks like this is actually a bug in botan and not the PKCS#11 module.
The first size is an estimate, which is allowed to be larger than the actual output, according to the spec (section 5.2). The vector needs to be resized after that second call.

Will update title and such.
codecov-io: # [Codecov](https://codecov.io/gh/randombit/botan/pull/1979?src=pr&el=h1) Report
> Merging [#1979](https://codecov.io/gh/randombit/botan/pull/1979?src=pr&el=desc) into [master](https://codecov.io/gh/randombit/botan/commit/0dd03c973f6c9ae6a38118385c82e64154e465f3?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/randombit/botan/pull/1979/graphs/tree.svg?width=650&token=VR6iYmSPH8&height=150&src=pr)](https://codecov.io/gh/randombit/botan/pull/1979?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1979      +/-   ##
==========================================
+ Coverage   91.64%   91.65%   +0.01%     
==========================================
  Files         533      533              
  Lines       57324    57323       -1     
  Branches     6083     6083              
==========================================
+ Hits        52533    52538       +5     
+ Misses       4791     4785       -6
```


| [Impacted Files](https://codecov.io/gh/randombit/botan/pull/1979?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/tests/test\_pkcs11\_high\_level.cpp](https://codecov.io/gh/randombit/botan/pull/1979/diff?src=pr&el=tree#diff-c3JjL3Rlc3RzL3Rlc3RfcGtjczExX2hpZ2hfbGV2ZWwuY3Bw) | `99.27% <ø> (-0.01%)` | :arrow_down: |
| [src/lib/pubkey/mce/mceliece\_key.cpp](https://codecov.io/gh/randombit/botan/pull/1979/diff?src=pr&el=tree#diff-c3JjL2xpYi9wdWJrZXkvbWNlL21jZWxpZWNlX2tleS5jcHA=) | `86.51% <0%> (+1.12%)` | :arrow_up: |
| [src/lib/pubkey/dl\_group/dl\_group.cpp](https://codecov.io/gh/randombit/botan/pull/1979/diff?src=pr&el=tree#diff-c3JjL2xpYi9wdWJrZXkvZGxfZ3JvdXAvZGxfZ3JvdXAuY3Bw) | `92.51% <0%> (+1.18%)` | :arrow_up: |
| [src/lib/misc/cryptobox/cryptobox.cpp](https://codecov.io/gh/randombit/botan/pull/1979/diff?src=pr&el=tree#diff-c3JjL2xpYi9taXNjL2NyeXB0b2JveC9jcnlwdG9ib3guY3Bw) | `95.23% <0%> (+1.58%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/randombit/botan/pull/1979?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/randombit/botan/pull/1979?src=pr&el=footer). Last update [0dd03c9...0bf1325](https://codecov.io/gh/randombit/botan/pull/1979?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/src/lib/prov/pkcs11/p11.h b/src/lib/prov/pkcs11/p11.h
index 043b1795a..46545b925 100644
--- a/src/lib/prov/pkcs11/p11.h
+++ b/src/lib/prov/pkcs11/p11.h
@@ -1817,11 +1817,16 @@ class BOTAN_PUBLIC_API(2,0) LowLevel
             }
 
          encrypted_data.resize(encrypted_size);
-         return C_Encrypt(session,
+         if (!C_Encrypt(session,
                           const_cast<Byte*>(plaintext_data.data()),
                           static_cast<Ulong>(plaintext_data.size()),
                           encrypted_data.data(),
-                          &encrypted_size, return_value);
+                          &encrypted_size, return_value))
+            {
+            return false;
+            }
+         encrypted_data.resize(encrypted_size);
+         return true;
          }
 
       /**
@@ -1950,11 +1955,16 @@ class BOTAN_PUBLIC_API(2,0) LowLevel
             }
 
          decrypted_data.resize(decrypted_size);
-         return C_Decrypt(session,
-                          const_cast<Byte*>(encrypted_data.data()),
-                          static_cast<Ulong>(encrypted_data.size()),
-                          decrypted_data.data(),
-                          &decrypted_size, return_value);
+         if(!C_Decrypt(session,
+                       const_cast<Byte*>(encrypted_data.data()),
+                       static_cast<Ulong>(encrypted_data.size()),
+                       decrypted_data.data(),
+                       &decrypted_size, return_value))
+            {
+            return false;
+            }
+         decrypted_data.resize(decrypted_size);
+         return true;
          }
 
       /**
@@ -2189,12 +2199,17 @@ class BOTAN_PUBLIC_API(2,0) LowLevel
             }
 
          signature.resize(signature_size);
-         return C_Sign(session,
+         if (!C_Sign(session,
                        const_cast<Byte*>(data.data()),
                        static_cast<Ulong>(data.size()),
                        signature.data(),
                        &signature_size,
-                       return_value);
+                       return_value))
+            {
+            return false;
+            }
+         signature.resize(signature_size);
+         return true;
          }
 
       /**
@@ -2290,7 +2305,12 @@ class BOTAN_PUBLIC_API(2,0) LowLevel
             }
 
          signature.resize(signature_size);
-         return C_SignFinal(session, signature.data(), &signature_size, return_value);
+         if (!C_SignFinal(session, signature.data(), &signature_size, return_value))
+            {
+            return false;
+            }
+         signature.resize(signature_size);
+         return true;
          }
 
       /**


Test patch:
diff --git a/src/tests/test_pkcs11_high_level.cpp b/src/tests/test_pkcs11_high_level.cpp
index c9c34a28b..077e0240f 100644
--- a/src/tests/test_pkcs11_high_level.cpp
+++ b/src/tests/test_pkcs11_high_level.cpp
@@ -831,9 +831,6 @@ Test::Result test_rsa_encrypt_decrypt()
       Botan::PK_Decryptor_EME decryptor(keypair.second, Test::rng(), padding);
       auto decrypted = decryptor.decrypt(encrypted);
 
-      // some token / middlewares do not remove the padding bytes
-      decrypted.resize(plaintext.size());
-
       result.test_eq("RSA PKCS11 encrypt and decrypt: " + padding, decrypted, plaintext);
       };