Hello list, I am maintainer of the tboot package in SUSE distributions. One of our customers reported issues with the lcp2_crtpollist command:
``` sles15beta7:~/tpm2.0 # lcp2_crtpollist --sign --nosig --pub txt-pub.pem --out signed.lst *** Error in `lcp2_crtpollist': free(): invalid pointer: 0x000056515bf33ff0 *** Aborted (core dumped) ``` The customer also pointed me to a possible fix: http://hg.code.sf.net/p/tboot/code/code?cmd=changeset;node=09fae64a7515 While trying to integrate this patch it turned out that it does not really fix the issue. There is still memory corruption and `valgrind` shows a lot invalid reads and writes. As far as I can see it both, the code for OpenSSL < 1.1.0 and the code for OpenSSL >= 1.1.0 is not working correctly at the moment. There seems to have been a misunderstanding of how the OpenSSL APIs work. Please find attached two patches that I have worked on that should fix the issues. The first patch is unrelated to OpenSSL but fixes a memory leak with fopen()/fclose(). I have tested these patches only for the following codepath: ``` lcp2_crtpollist --verbose --sign --nosig --pub /root/tboot/pubkey.pem --out /root/tboot/list_unsig.lst ``` After applying my patches both OpenSSL 1.0.x and OpenSSL 1.1.x versions worked without any memory corruptions and no memory leaks remained. Best regards Matthias -- Matthias Gerstner <matthias.gerst...@suse.de> Dipl.-Wirtsch.-Inf. (FH), Security Engineer https://www.suse.com/security Telefon: +49 911 740 53 290 GPG Key ID: 0x14C405C971923553 SUSE Linux GmbH GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nuernberg)
From 3cc667a713fde7dcc0efec78d5026638cfcb7b5c Mon Sep 17 00:00:00 2001
From: Matthias Gerstner <matthias.gerst...@suse.de>
Date: Fri, 9 Mar 2018 11:28:20 +0100
Subject: [PATCH 1/2] crtpollist: fixed FILE* leak
Signed-off-by: Matthias Gerstner <mgerst...@suse.de>
---
lcptools-v2/crtpollist.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/lcptools-v2/crtpollist.c b/lcptools-v2/crtpollist.c
index 3a9d349..7dbc379 100644
--- a/lcptools-v2/crtpollist.c
+++ b/lcptools-v2/crtpollist.c
@@ -132,6 +132,7 @@ static lcp_signature_t2 *read_rsa_pubkey_file(const char *file)
if ( fp == NULL ) {
ERROR("Error: failed to open .pem file %s: %s\n", file,
strerror(errno));
+ fclose(fp);
return NULL;
}
@@ -141,6 +142,7 @@ static lcp_signature_t2 *read_rsa_pubkey_file(const char *file)
ERROR("Error: failed to read .pem file %s: %s\n", file,
ERR_error_string(ERR_get_error(), NULL));
ERR_free_strings();
+ fclose(fp);
return NULL;
}
@@ -148,6 +150,7 @@ static lcp_signature_t2 *read_rsa_pubkey_file(const char *file)
if ( keysize == 0 ) {
ERROR("Error: public key size is 0\n");
RSA_free(pubkey);
+ fclose(fp);
return NULL;
}
@@ -155,6 +158,7 @@ static lcp_signature_t2 *read_rsa_pubkey_file(const char *file)
if ( sig == NULL ) {
ERROR("Error: failed to allocate sig\n");
RSA_free(pubkey);
+ fclose(fp);
return NULL;
}
--
2.13.6
From 83f5784b5cf20a193c2710b70709a01ee7e93058 Mon Sep 17 00:00:00 2001
From: Matthias Gerstner <matthias.gerst...@suse.de>
Date: Fri, 9 Mar 2018 11:28:48 +0100
Subject: [PATCH 2/2] OpenSSL usage: fix double frees and memory leaks
When setting BIGNUM pointers in RSA structures then the ownership of
them is transferred into the RSA object. RSA_free() is enough to free
all of them.
When getting BIGNUM pointers from RSA structures then the ownership
remains with the RSA object. There is no need to allocate the BIGNUMs
before, or free them afterwards.
BN_bin2bn with NULL as third parameter returns a newly allocated BIGNUM.
Signed-off-by: Matthias Gerstner <mgerst...@suse.de>
---
lcptools-v2/crtpollist.c | 19 +++++++++----------
lcptools-v2/lcputils.c | 22 ++++------------------
lcptools/crtpollist.c | 10 +++++-----
lcptools/lcputils2.c | 24 +++++++++---------------
4 files changed, 27 insertions(+), 48 deletions(-)
diff --git a/lcptools-v2/crtpollist.c b/lcptools-v2/crtpollist.c
index 7dbc379..a22e002 100644
--- a/lcptools-v2/crtpollist.c
+++ b/lcptools-v2/crtpollist.c
@@ -165,13 +165,13 @@ static lcp_signature_t2 *read_rsa_pubkey_file(const char *file)
memset(sig, 0, sizeof(lcp_rsa_signature_t) + 2*keysize);
sig->rsa_signature.pubkey_size = keysize;
+ const BIGNUM *modulus = NULL;
/* OpenSSL Version 1.1.0 and later don't allow direct access to RSA
stuct */
#if OPENSSL_VERSION_NUMBER >= 0x10100000L
- BIGNUM *modulus = BN_new();
- RSA_get0_key(pubkey, (const BIGNUM **)&modulus, NULL, NULL);
+ RSA_get0_key(pubkey, &modulus, NULL, NULL);
#else
- BIGNUM *modulus = BN_dup(pubkey->n);
+ modulus = pubkey->n;
#endif
unsigned char key[keysize];
@@ -187,8 +187,8 @@ static lcp_signature_t2 *read_rsa_pubkey_file(const char *file)
}
LOG("read rsa pubkey succeed!\n");
- BN_free(modulus);
RSA_free(pubkey);
+ fclose(fp);
return sig;
}
@@ -390,13 +390,13 @@ static bool ecdsa_sign_tpm20_list_data(lcp_policy_list_t2 *pollist, EC_KEY *ecke
return false;
}
- BIGNUM *r = BN_new();
- BIGNUM *s = BN_new();
-
+ const BIGNUM *r = NULL;
+ const BIGNUM *s = NULL;
+
/* OpenSSL Version 1.1.0 and later don't allow direct access to
ECDSA_SIG stuct */
#if OPENSSL_VERSION_NUMBER >= 0x10100000L
- ECDSA_SIG_get0(ecdsasig, (const BIGNUM **)&r, (const BIGNUM **)&s);
+ ECDSA_SIG_get0(ecdsasig, &r, &s);
#else
r = ecdsasig->r;
s = ecdsasig->s;
@@ -419,8 +419,7 @@ static bool ecdsa_sign_tpm20_list_data(lcp_policy_list_t2 *pollist, EC_KEY *ecke
display_tpm20_signature(" ", sig, pollist->sig_alg, false);
}
- BN_free(r);
- BN_free(s);
+ ECDSA_SIG_free(ecdsasig);
return true;
}
return false;
diff --git a/lcptools-v2/lcputils.c b/lcptools-v2/lcputils.c
index a81a02f..7079890 100644
--- a/lcptools-v2/lcputils.c
+++ b/lcptools-v2/lcputils.c
@@ -444,9 +444,8 @@ bool verify_signature(const uint8_t *data, size_t data_size,
return false;
}
- BIGNUM *modulus = BN_new();
+ BIGNUM *modulus = BN_bin2bn(key, pubkey_size, NULL);
BIGNUM *exponent = BN_new();
- modulus = BN_bin2bn(key, pubkey_size, NULL);
/* uses fixed exponent (LCP_SIG_EXPONENT) */
char exp[32];
@@ -457,8 +456,8 @@ bool verify_signature(const uint8_t *data, size_t data_size,
#if OPENSSL_VERSION_NUMBER >= 0x10100000L
RSA_set0_key(rsa_pubkey, modulus, exponent, NULL);
#else
- rsa_pubkey->n = BN_dup(modulus);
- rsa_pubkey->e = BN_dup(exponent);
+ rsa_pubkey->n = modulus;
+ rsa_pubkey->e = exponent;
rsa_pubkey->d = rsa_pubkey->p = rsa_pubkey->q = NULL;
#endif
@@ -480,8 +479,6 @@ bool verify_signature(const uint8_t *data, size_t data_size,
tb_hash_t digest;
if ( !hash_buffer(data, data_size, &digest, hashalg) ) {
ERROR("Error: failed to hash list\n");
- BN_free(modulus);
- BN_free(exponent);
RSA_free(rsa_pubkey);
return false;
}
@@ -524,8 +521,6 @@ bool verify_signature(const uint8_t *data, size_t data_size,
ERROR("Error: failed to verify list: %s\n",
ERR_error_string(ERR_get_error(), NULL));
ERR_free_strings();
- BN_free(modulus);
- BN_free(exponent);
RSA_free(rsa_pubkey);
return false;
}
@@ -540,8 +535,6 @@ bool verify_signature(const uint8_t *data, size_t data_size,
ERROR("Error: failed to verify list: %s\n",
ERR_error_string(ERR_get_error(), NULL));
ERR_free_strings();
- BN_free(modulus);
- BN_free(exponent);
RSA_free(rsa_pubkey);
return false;
}
@@ -556,8 +549,6 @@ bool verify_signature(const uint8_t *data, size_t data_size,
ERROR("Error: failed to verify list: %s\n",
ERR_error_string(ERR_get_error(), NULL));
ERR_free_strings();
- BN_free(modulus);
- BN_free(exponent);
RSA_free(rsa_pubkey);
return false;
}
@@ -572,8 +563,6 @@ bool verify_signature(const uint8_t *data, size_t data_size,
ERROR("Error: failed to verify list: %s\n",
ERR_error_string(ERR_get_error(), NULL));
ERR_free_strings();
- BN_free(modulus);
- BN_free(exponent);
RSA_free(rsa_pubkey);
return false;
}
@@ -581,13 +570,10 @@ bool verify_signature(const uint8_t *data, size_t data_size,
default :
LOG("unknown hash alg\n");
- BN_free(modulus);
- BN_free(exponent);
+ RSA_free(rsa_pubkey);
return false;
}
- BN_free(modulus);
- BN_free(exponent);
RSA_free(rsa_pubkey);
return true;
}
diff --git a/lcptools/crtpollist.c b/lcptools/crtpollist.c
index 01c45f1..661e3af 100644
--- a/lcptools/crtpollist.c
+++ b/lcptools/crtpollist.c
@@ -156,13 +156,14 @@ static lcp_signature_t *read_pubkey_file(const char *file)
memset(sig, 0, sizeof(*sig) + 2*keysize);
sig->pubkey_size = keysize;
+ const BIGNUM *modulus = NULL;
+
/* OpenSSL Version 1.1.0 and later don't allow direct access to RSA
stuct */
#if OPENSSL_VERSION_NUMBER >= 0x10100000L
- BIGNUM *modulus = BN_new();
- RSA_get0_key(pubkey, (const BIGNUM **)&modulus, NULL, NULL);
+ RSA_get0_key(pubkey, &modulus, NULL, NULL);
#else
- BIGNUM *modulus = BN_dup(pubkey->n);
+ modulus = pubkey->n;
#endif
unsigned char key[keysize];
BN_bn2bin(modulus, key);
@@ -175,8 +176,7 @@ static lcp_signature_t *read_pubkey_file(const char *file)
LOG("signature:\n");
display_signature(" ", sig, false);
}
-
- BN_free(modulus);
+
RSA_free(pubkey);
return sig;
}
diff --git a/lcptools/lcputils2.c b/lcptools/lcputils2.c
index 797b71d..fe3b266 100644
--- a/lcptools/lcputils2.c
+++ b/lcptools/lcputils2.c
@@ -274,31 +274,29 @@ bool verify_signature(const uint8_t *data, size_t data_size,
ERROR("Error: failed to allocate key\n");
return false;
}
- BIGNUM *modulus = BN_new();
+
+ BIGNUM *modulus = BN_bin2bn(key, pubkey_size, NULL);
BIGNUM *exponent = BN_new();
- modulus = BN_bin2bn(key, pubkey_size, NULL);
/* uses fixed exponent (LCP_SIG_EXPONENT) */
char exp[32];
snprintf(exp, sizeof(exp), "%u", LCP_SIG_EXPONENT);
BN_dec2bn(&exponent, exp);
-
+
/* OpenSSL Version 1.1.0 and later don't allow direct access to RSA
stuct */
#if OPENSSL_VERSION_NUMBER >= 0x10100000L
- RSA_set0_key(rsa_pubkey, modulus, exponent, NULL);
+ RSA_set0_key(rsa_pubkey, modulus, exponent, NULL);
#else
- rsa_pubkey->n = BN_dup(modulus);
- rsa_pubkey->e = BN_dup(exponent);
- rsa_pubkey->d = rsa_pubkey->p = rsa_pubkey->q = NULL;
+ rsa_pubkey->n = modulus;
+ rsa_pubkey->e = exponent;
+ rsa_pubkey->d = rsa_pubkey->p = rsa_pubkey->q = NULL;
#endif
/* first create digest of data */
tb_hash_t digest;
if ( !hash_buffer(data, data_size, &digest, TB_HALG_SHA1_LG) ) {
ERROR("Error: failed to hash list\n");
- BN_free(modulus);
- BN_free(exponent);
RSA_free(rsa_pubkey);
return false;
}
@@ -339,14 +337,10 @@ bool verify_signature(const uint8_t *data, size_t data_size,
ERROR("Error: failed to verify list: %s\n",
ERR_error_string(ERR_get_error(), NULL));
ERR_free_strings();
- BN_free(modulus);
- BN_free(exponent);
- RSA_free(rsa_pubkey);
+ RSA_free(rsa_pubkey);
return false;
}
-
- BN_free(modulus);
- BN_free(exponent);
+
RSA_free(rsa_pubkey);
return true;
}
--
2.13.6
signature.asc
Description: Digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ tboot-devel mailing list tboot-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tboot-devel
