search

tpm2-software / tpm2-tss-engine

OpenSSL Engine for TPM2 devices
https://tpm2-software.github.io
BSD 3-Clause "New" or "Revised" License
151 stars 100 forks source link

Potential memory corruptions: patch proposals #231

Closed c-skills closed 3 years ago

c-skills commented 3 years ago

We created https://github.com/c-skills/patches/blob/master/tpm2-tss-engine/tpm2-tss-engine-7ce6186bf598-overflows.diff to fix some issues were we think are worth fixing.

AndreasFuchsTPM commented 3 years ago

These are auto-generated, right ? Obviously they were not manually assessed, so I'm closing this issue, unless you tell me otherwise.

c-skills commented 3 years ago

No its not, how can you think so? Are all other security patches you receive auto-generated?

AndreasFuchsTPM commented 3 years ago

Because I've had people just take the output of any random "security scanner tool" put it into a diff and send it in the past already. Any proper patch comes in a commit with an explanation of what it fixes and is submitted via a PR.

As for your post, I assume to be another one of these, because

  1. There is a random diff instead of a regular PR with commits and signed-off tags.
  2. There are a bunch of fixes in a single diff, instead of multiple commits

  3. They don't make any sense (see my inline comments)

    diff --git a/src/tpm2-tss-engine-ecc.c b/src/tpm2-tss-engine-ecc.c
index 0bd7419..7537cf5 100644
--- a/src/tpm2-tss-engine-ecc.c
+++ b/src/tpm2-tss-engine-ecc.c
@@ -131,15 +131,28 @@ init_tpm_public_point(TPM2B_ECC_POINT *point, const EC_POINT *ec_point,
                     const EC_GROUP *ec_group)
{
 unsigned char buffer[1 + sizeof(point->point.x.buffer)
-                           + sizeof(point->point.y.buffer)];
+                           + sizeof(point->point.y.buffer)] = {0};
+
!!! Why would we need this ??? Please point out the case of uninitialized dereferencing.
 BN_CTX *ctx = BN_CTX_new();
 if (!ctx)
     return 0;

 BN_CTX_start(ctx);
-    size_t len = EC_POINT_point2oct(ec_group, ec_point,
+
+    size_t len = 0;
+
+    // first, check for actual buffer size required
!!! Why ? We overestimate this during the buffer allocation above... How should the returned point be larger ?
+    if ((len = EC_POINT_point2oct(ec_group, ec_point, POINT_CONVERSION_UNCOMPRESSED, NULL, 0, ctx)) <= sizeof(buffer)) {
+        len = EC_POINT_point2oct(ec_group, ec_point,
                 POINT_CONVERSION_UNCOMPRESSED, buffer, sizeof(buffer), ctx);
+    }
+
+    BN_CTX_end(ctx);
 BN_CTX_free(ctx);
+
+    if (len == 0 || len > sizeof(buffer))
+        return 0;
+
 len = (len - 1) / 2;

 point->point.x.size = len;
@@ -309,7 +322,7 @@ ecdsa_sign(const unsigned char *dgst, int dgst_len, const BIGNUM *inv,
     ERR(ecdsa_sign, TPM2TSS_R_DIGEST_TOO_LARGE);
     goto error;
 }
-    memcpy(&digest.buffer[0], dgst, dgst_len);
+    memcpy(&digest.buffer[0], dgst, digest.size);
!!! digest.size is initialized from dgst_len. How is this a security bug or any bug to begin with ?

 r = init_tpm_key(&esys_ctx, &keyHandle, tpm2Data);
 ERRchktss(ecdsa_sign, r, goto error);
@@ -596,7 +609,7 @@ tpm2tss_ecc_genkey(EC_KEY *key, TPMI_ECC_CURVE curve, const char *password,

 if (password) {
     DBG("Setting a password for the created key.\n");
-        if (strlen(password) > sizeof(tpm2Data->userauth.buffer) - 1) {
+        if (strlen(password) > sizeof(tpm2Data->userauth.buffer) - 1 || strlen(password) > sizeof(inSensitive.sensitive.userAuth.buffer) - 1) {
!!! TPM2_DATA.userauth and TPM2B_SENSITIVE.sensitive.userAuth are of type TPM2B_DIGEST. So what is the security issue here ?
         goto error;
     }
     tpm2Data->userauth.size = strlen(password);
diff --git a/src/tpm2-tss-engine-rsa.c b/src/tpm2-tss-engine-rsa.c
index adab9cd..a2c0f33 100644
--- a/src/tpm2-tss-engine-rsa.c
+++ b/src/tpm2-tss-engine-rsa.c
@@ -169,6 +169,10 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa,
 DBGBUF(&sig->buffer[0], sig->size);

 ret = sig->size;
+    if (ret > RSA_size(rsa) || ret <= 0) {
!!! sig->size is a UINT, so ret cannot be < 0. And what is the security implication of ret==0 ?
!!! If ret was larger than the to-buffer this might be a problem. But that can only happen if the path between TPM and this engine was infiltrated which is a highly unlikely scenario.
!!! Anyways, I would have accepted this as an improvement with proper explanation in a good PR.
+        ERR(rsa_priv_enc, TPM2TSS_R_DIGEST_TOO_LARGE);
+        goto error;
+    }
 memcpy(to, &sig->buffer[0], ret);

 goto out;
@@ -226,7 +230,7 @@ rsa_priv_dec(int flen, const unsigned char *from, unsigned char *to, RSA * rsa,
 TPMT_RSA_DECRYPT inScheme;

 TPM2B_PUBLIC_KEY_RSA cipher = { .size = flen };
-    if (flen > (int)sizeof(cipher.buffer)) {
+    if (flen > (int)sizeof(cipher.buffer) || flen < 0) {
!!! Again not a security thing, since this is the application calling its module incorrectly, not an attacker path with external data. Also what would the security implications of flen<0 be ?
     ERR(rsa_priv_dec, TPM2TSS_R_DIGEST_TOO_LARGE);
     goto error;
 }
@@ -257,6 +261,10 @@ rsa_priv_dec(int flen, const unsigned char *from, unsigned char *to, RSA * rsa,
 DBGBUF(&message->buffer[0], message->size);

 flen = message->size;
+    if (flen > RSA_size(rsa) || flen <= 0) {
+        ERR(rsa_priv_dec, TPM2TSS_R_DIGEST_TOO_LARGE);
+        goto error;
+    }
 memcpy(to, &message->buffer[0], flen);

 goto out;

So I'll assume that you're new to the field and I did not discourage you from continuing to contribute, but please save yourself and other people time and think a bit further beforehand and also put things into a good reviewable form (i.e. multiple commits with explanations inside a PR).

AndreasFuchsTPM commented 3 years ago

P.S. Please attempt to exercise Responsible Disclosure in addition to proper PRs instead of putting patches on a fame-hunting repo...

c-skills commented 3 years ago

So I'll assume that you're new to the field and I did not discourage you from continuing to contribute, but please save yourself and other people time and think a bit further beforehand and also put things into a good reviewable form (i.e. >multiple commits with explanations inside a PR).

By your arrogant reply, I assume you are new to the field of programming and Open Source. I don't want to discourage you from continuing to try to write correct tpm2 software and learn about return value checks from OpenSSL functions and before len parameters are subtracted and passed to memcpy(). I also don't want to discourage you from learning about the defense-in-depth idiom for crypto software. After all, I do not really care if you fix broken software or not, it's just a proposal.

AndreasFuchsTPM commented 3 years ago

Let me apologize. I did not mean to be arrogant. I though I was too harsh in the assessment and wanted to compensate. That went completely wrong. My mistake.

Content-wise, I stand by my assessment: You should send a PR or something else that at least contains a sign-off (so we can use it license-wise). For a "security patch" I want to know the security implications of each change so I can assess them.

c-skills commented 3 years ago

Thanks. Reset. :) Some of the changes are indeed more of a theoretical nature. For this particular case the reason is the idiom of using the same variable that was used for checking is also used for the memcpy() length. This is because dgst_len may be of a different type (e.g. short vs. long) and the assignment in the first line might have truncated bits that are missing in the check, but visible in the memcpy(). Even if both types are the same, struct definitions may change the type in future, so the code becomes more fault-proof in the event that other developers change something later without realizing the implicit dependencies.

AndreasFuchsTPM commented 3 years ago

Thanks as well.

Would you mind sending a PR with a signed-off tag (because licenses) ? And would you use a different headline for the PR and commits so people don't get heart attacks ? ;-)