Use secure heap for private keys and passwords

After CRYPTO_secure_malloc_init OpenSSL will store private keys in secure heap. This facility is only available since OpenSSL_1_1_0-pre1.

Signed-off-by Vitaly Chikunov <[email protected]>

---
Change from v1:
- Do not use setfbuf to disable buffering as this is not proven to be meaningful.
- Use secure heap for passwords too as suggested by Mimi Zohar.
- Fallback to OPENSSL_malloc for old OpenSSL as suggested by Mimi Zohar.
- Simplify logic of calling CRYPTO_secure_malloc_init (call it always on OpenSSL init.)
- Should be applied after Bruno Meneguele's "evmctl: fix memory leak in get_password" patch v2.

 src/evmctl.c | 143 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 118 insertions(+), 25 deletions(-)

src/evmctl.c +3 -1

@@ -188,7 +207,9 @@ static int bin2file(const char *file, const char *ext, const unsigned char *data
 	return err;
 }
 
-static unsigned char *file2bin(const char *file, const char *ext, int *size)
+/* Return data in OpenSSL secure heap if 'secure' is true. */
+static unsigned char *file2bin(const char *file, const char *ext, int *size,
+			       int secure)
 {
 	FILE *fp;
 	size_t len;

src/evmctl.c +0 -0

 	if (!data) {
 		log_err("Failed to malloc %zu bytes: %s\n", len, name);
 		fclose(fp);

Quoted 4 lines · Vitaly Chikunov

After CRYPTO_secure_malloc_init OpenSSL will store private keys in secure heap. This facility is only available since OpenSSL_1_1_0-pre1.

Signed-off-by Vitaly Chikunov <[email protected]>

Quoted 10 lines

---
Change from v1:
- Do not use setfbuf to disable buffering as this is not proven to be meaningful.
- Use secure heap for passwords too as suggested by Mimi Zohar.
- Fallback to OPENSSL_malloc for old OpenSSL as suggested by Mimi Zohar.
- Simplify logic of calling CRYPTO_secure_malloc_init (call it always on OpenSSL init.)
- Should be applied after Bruno Meneguele's "evmctl: fix memory leak in get_password" patch v2.

Not sure why it isn't applying with/without Bruno's v2 patch.

Quoted 22 lines

 src/evmctl.c | 143 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 118 insertions(+), 25 deletions(-)

src/evmctl.c +3 -1

@@ -188,7 +207,9 @@ static int bin2file(const char *file, const char *ext, const unsigned char *data
 	return err;
 }
 
-static unsigned char *file2bin(const char *file, const char *ext, int *size)
+/* Return data in OpenSSL secure heap if 'secure' is true. */
+static unsigned char *file2bin(const char *file, const char *ext, int *size,
+			       int secure)
 {
 	FILE *fp;
 	size_t len;

@@ -215,7 +236,10 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
 	}
 	len = stats.st_size;
 

src/evmctl.c static unsigned char *file2bin(const char *file, const char *ext, int *size) · line 242

Quoted 5 lines

src/evmctl.c +4 -1

-	data = malloc(len);
+	if (secure)
+		data = OPENSSL_secure_malloc(len);
+	else
+		data = malloc(len);

Without being able to apply the patch, it's hard to tell if there should be a preparatory patch that first replaces malloc() with OPENSSL_malloc(), and other similar changes.

thanks,

Mimi

Quoted 3 lines

src/evmctl.c +0 -0

 	if (!data) {
 		log_err("Failed to malloc %zu bytes: %s\n", len, name);
 		fclose(fp);

Mimi,

Quoted 16 lines · Mimi Zohar

Quoted 4 lines · Vitaly Chikunov

After CRYPTO_secure_malloc_init OpenSSL will store private keys in secure heap. This facility is only available since OpenSSL_1_1_0-pre1.

Signed-off-by Vitaly Chikunov <[email protected]>

Quoted 10 lines

---
Change from v1:
- Do not use setfbuf to disable buffering as this is not proven to be meaningful.
- Use secure heap for passwords too as suggested by Mimi Zohar.
- Fallback to OPENSSL_malloc for old OpenSSL as suggested by Mimi Zohar.
- Simplify logic of calling CRYPTO_secure_malloc_init (call it always on OpenSSL init.)
- Should be applied after Bruno Meneguele's "evmctl: fix memory leak in get_password" patch v2.

Quoted 1 line

Not sure why it isn't applying with/without Bruno's v2 patch.

It should be over next-testing + (manually git am'ed) Bruno's patch:

db25fcd 2021-08-19 03:20:48 +0300 Use secure heap for private keys and passwords (Vitaly Chikunov) d37ea6d 2021-08-16 12:15:59 -0300 evmctl: fix memory leak in get_password (Bruno Meneguele)
b1818c1 2021-08-03 16:40:08 -0400 Create alternative tpm2_pcr_read() that uses IBM TSS (Ken Goldman) (origin/next-testing)

Quoted 22 lines

Quoted 22 lines

 src/evmctl.c | 143 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 118 insertions(+), 25 deletions(-)

src/evmctl.c +3 -1

@@ -188,7 +207,9 @@ static int bin2file(const char *file, const char *ext, const unsigned char *data
 	return err;
 }
 
-static unsigned char *file2bin(const char *file, const char *ext, int *size)
+/* Return data in OpenSSL secure heap if 'secure' is true. */
+static unsigned char *file2bin(const char *file, const char *ext, int *size,
+			       int secure)
 {
 	FILE *fp;
 	size_t len;

@@ -215,7 +236,10 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
 	}
 	len = stats.st_size;
 

Quoted 9 lines

src/evmctl.c static unsigned char *file2bin(const char *file, const char *ext, int *size) · line 242

Quoted 5 lines

src/evmctl.c +4 -1

-	data = malloc(len);
+	if (secure)
+		data = OPENSSL_secure_malloc(len);
+	else
+		data = malloc(len);

Without being able to apply the patch, it's hard to tell if there should be a preparatory patch that first replaces malloc() with OPENSSL_malloc(), and other similar changes.

There is no OPENSSL_malloc use and I don't see why it should be.

Thanks,

Quoted 8 lines

thanks,

Mimi

Quoted 3 lines

src/evmctl.c +0 -0

 	if (!data) {
 		log_err("Failed to malloc %zu bytes: %s\n", len, name);
 		fclose(fp);

Quoted 14 lines · Vitaly Chikunov

Quoted 13 lines

src/evmctl.c static unsigned char *file2bin(const char *file, const char *ext, int *size) · line 242

Quoted 9 lines

src/evmctl.c +4 -1

@@ -215,7 +236,10 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
 	}
 	len = stats.st_size;
 
-	data = malloc(len);
+	if (secure)
+		data = OPENSSL_secure_malloc(len);
+	else
+		data = malloc(len);

Without being able to apply the patch, it's hard to tell if there should be a preparatory patch that first replaces malloc() with OPENSSL_malloc(), and other similar changes.

Quoted 2 lines

There is no OPENSSL_malloc use and I don't see why it should be.

Keeping the OPENSSL_* calls as a meaning of "secure calls" while keeping the standard C library calls for "non-secure" seems indeed cleaner.

Mimi,

Quoted 16 lines · Mimi Zohar

Quoted 14 lines · Vitaly Chikunov

After CRYPTO_secure_malloc_init OpenSSL will store private keys in secure heap. This facility is only available since OpenSSL_1_1_0-pre1.

Signed-off-by Vitaly Chikunov <[email protected]>

---
Change from v1:
- Do not use setfbuf to disable buffering as this is not proven to be meaningful.
- Use secure heap for passwords too as suggested by Mimi Zohar.
- Fallback to OPENSSL_malloc for old OpenSSL as suggested by Mimi Zohar.
- Simplify logic of calling CRYPTO_secure_malloc_init (call it always on OpenSSL init.)
- Should be applied after Bruno Meneguele's "evmctl: fix memory leak in get_password" patch v2.

Quoted 1 line

Not sure why it isn't applying with/without Bruno's v2 patch.

It should be over next-testing + (manually git am'ed) Bruno's patch:

db25fcd 2021-08-19 03:20:48 +0300 Use secure heap for private keys and passwords (Vitaly Chikunov) d37ea6d 2021-08-16 12:15:59 -0300 evmctl: fix memory leak in get_password (Bruno Meneguele)
b1818c1 2021-08-03 16:40:08 -0400 Create alternative tpm2_pcr_read() that uses IBM TSS (Ken Goldman) (origin/next-testing)

After CRYPTO_secure_malloc_init OpenSSL will store private keys in secure heap. This facility is only available since OpenSSL_1_1_0-pre1.

Signed-off-by Vitaly Chikunov <[email protected]>

---
Change from v1:
- Do not use setfbuf to disable buffering as this is not proven to be meaningful.
- Use secure heap for passwords too as suggested by Mimi Zohar.
- Fallback to OPENSSL_malloc for old OpenSSL as suggested by Mimi Zohar.
- Simplify logic of calling CRYPTO_secure_malloc_init (call it always on OpenSSL init.)
- Should be applied after Bruno Meneguele's "evmctl: fix memory leak in get_password" patch v2.

 src/evmctl.c | 143 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 118 insertions(+), 25 deletions(-)

src/evmctl.c +1 -0

index 5f7c2b8..a27e0b9 100644
@@ -59,6 +59,7 @@
 #include <assert.h>
 
 #include <openssl/asn1.h>
+#include <openssl/crypto.h>
 #include <openssl/sha.h>
 #include <openssl/pem.h>
 #include <openssl/hmac.h>

@@ -165,6 +166,24 @@ struct tpm_bank_info {
 static char *pcrfile[MAX_PCRFILE];

Quoted 34 lines · Vitaly Chikunov

After CRYPTO_secure_malloc_init OpenSSL will store private keys in secure heap. This facility is only available since OpenSSL_1_1_0-pre1.

Signed-off-by Vitaly Chikunov <[email protected]>

---
Change from v1:
- Do not use setfbuf to disable buffering as this is not proven to be meaningful.
- Use secure heap for passwords too as suggested by Mimi Zohar.
- Fallback to OPENSSL_malloc for old OpenSSL as suggested by Mimi Zohar.
- Simplify logic of calling CRYPTO_secure_malloc_init (call it always on OpenSSL init.)
- Should be applied after Bruno Meneguele's "evmctl: fix memory leak in get_password" patch v2.

 src/evmctl.c | 143 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 118 insertions(+), 25 deletions(-)

src/evmctl.c +1 -0

index 5f7c2b8..a27e0b9 100644
@@ -59,6 +59,7 @@
 #include <assert.h>
 
 #include <openssl/asn1.h>
+#include <openssl/crypto.h>
 #include <openssl/sha.h>
 #include <openssl/pem.h>
 #include <openssl/hmac.h>

@@ -165,6 +166,24 @@ struct tpm_bank_info {
 static char *pcrfile[MAX_PCRFILE];
 static unsigned npcrfile;
 

src/evmctl.c struct tpm_bank_info { · line 176

Quoted 8 lines

src/evmctl.c +8 -0

+#if OPENSSL_VERSION_NUMBER <= 0x10100000
+#warning Your OpenSSL version is too old to have OPENSSL_secure_malloc, \
+	falling back to use plain OPENSSL_malloc.
+#define OPENSSL_secure_malloc	  OPENSSL_malloc
+#define OPENSSL_secure_free	  OPENSSL_free
+/*
+ * Secure heap memory automatically cleared on free, but
+ * OPENSSL_secure_clear_free will be used in case of fallback

Shouldn't it be OPENSSL_clear_free instead of OPENSLL_secure_clear_free in the setence above?

Signature · 3 lines

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

Bruno,

Quoted 35 lines · Bruno Meneguele

Quoted 34 lines · Vitaly Chikunov

After CRYPTO_secure_malloc_init OpenSSL will store private keys in secure heap. This facility is only available since OpenSSL_1_1_0-pre1.

Signed-off-by Vitaly Chikunov <[email protected]>

---
Change from v1:
- Do not use setfbuf to disable buffering as this is not proven to be meaningful.
- Use secure heap for passwords too as suggested by Mimi Zohar.
- Fallback to OPENSSL_malloc for old OpenSSL as suggested by Mimi Zohar.
- Simplify logic of calling CRYPTO_secure_malloc_init (call it always on OpenSSL init.)
- Should be applied after Bruno Meneguele's "evmctl: fix memory leak in get_password" patch v2.

 src/evmctl.c | 143 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 118 insertions(+), 25 deletions(-)

src/evmctl.c +1 -0

index 5f7c2b8..a27e0b9 100644
@@ -59,6 +59,7 @@
 #include <assert.h>
 
 #include <openssl/asn1.h>
+#include <openssl/crypto.h>
 #include <openssl/sha.h>
 #include <openssl/pem.h>
 #include <openssl/hmac.h>

@@ -165,6 +166,24 @@ struct tpm_bank_info {
 static char *pcrfile[MAX_PCRFILE];
 static unsigned npcrfile;
 

Quoted 11 lines

src/evmctl.c struct tpm_bank_info { · line 176

Quoted 8 lines

src/evmctl.c +8 -0

+#if OPENSSL_VERSION_NUMBER <= 0x10100000
+#warning Your OpenSSL version is too old to have OPENSSL_secure_malloc, \
+	falling back to use plain OPENSSL_malloc.
+#define OPENSSL_secure_malloc	  OPENSSL_malloc
+#define OPENSSL_secure_free	  OPENSSL_free
+/*
+ * Secure heap memory automatically cleared on free, but
+ * OPENSSL_secure_clear_free will be used in case of fallback

Shouldn't it be OPENSSL_clear_free instead of OPENSLL_secure_clear_free in the setence above?

No. I meant our code will use OPENSSL_secure_clear_free, so when there is no secure heap it will fallback to OPENSSL_clear_free (and not just to OPENSSL_free if we used OPENSSL_secure_free).

Thanks,

After CRYPTO_secure_malloc_init OpenSSL will store private keys in secure heap. This facility is only available since OpenSSL_1_1_0-pre1.

Signed-off-by Vitaly Chikunov <[email protected]>

---
Change from v1:
- Do not use setfbuf to disable buffering as this is not proven to be meaningful.
- Use secure heap for passwords too as suggested by Mimi Zohar.
- Fallback to OPENSSL_malloc for old OpenSSL as suggested by Mimi Zohar.
- Simplify logic of calling CRYPTO_secure_malloc_init (call it always on OpenSSL init.)
- Should be applied after Bruno Meneguele's "evmctl: fix memory leak in get_password" patch v2.

 src/evmctl.c | 143 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 118 insertions(+), 25 deletions(-)

src/evmctl.c +0 -0

@@ -2651,6 +2721,16 @@ int main(int argc, char *argv[])
 #endif
 			    OPENSSL_INIT_ENGINE_ALL_BUILTIN, NULL);

After CRYPTO_secure_malloc_init OpenSSL will store private keys in secure heap. This facility is only available since OpenSSL_1_1_0-pre1.

Signed-off-by Vitaly Chikunov <[email protected]>

---
Change from v1:
- Do not use setfbuf to disable buffering as this is not proven to be meaningful.
- Use secure heap for passwords too as suggested by Mimi Zohar.
- Fallback to OPENSSL_malloc for old OpenSSL as suggested by Mimi Zohar.
- Simplify logic of calling CRYPTO_secure_malloc_init (call it always on OpenSSL init.)
- Should be applied after Bruno Meneguele's "evmctl: fix memory leak in get_password" patch v2.

 src/evmctl.c | 143 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 118 insertions(+), 25 deletions(-)

src/evmctl.c +7 -0

@@ -2596,15 +2637,41 @@ static struct option opts[] = {
 
 };
 
+/*
+ * Copy password from optarg into secure heap, so it could be
+ * freed in the same way as a result of get_password().
+ */
+static char *optarg_password(char *optarg)
+{
+	size_t len;

src/evmctl.c +9 -0

+	/*
+	 * This memset does not add real security, just increases
+	 * the chance of password being obscured in ps output.
+	 */
+	memset(optarg, 'X', len);
+	return keypass;
+}
+
+/* Read from TTY into secure heap. */
 static char *get_password(void)
 {

src/evmctl.c +0 -0

 		return NULL;
 	}
 

Quoted 35 lines · Vitaly Chikunov

After CRYPTO_secure_malloc_init OpenSSL will store private keys in secure heap. This facility is only available since OpenSSL_1_1_0-pre1.

Signed-off-by Vitaly Chikunov <[email protected]>

---
Change from v1:
- Do not use setfbuf to disable buffering as this is not proven to be meaningful.
- Use secure heap for passwords too as suggested by Mimi Zohar.
- Fallback to OPENSSL_malloc for old OpenSSL as suggested by Mimi Zohar.
- Simplify logic of calling CRYPTO_secure_malloc_init (call it always on OpenSSL init.)
- Should be applied after Bruno Meneguele's "evmctl: fix memory leak in get_password" patch v2.

 src/evmctl.c | 143 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 118 insertions(+), 25 deletions(-)

src/evmctl.c +13 -0

@@ -2596,15 +2637,41 @@ static struct option opts[] = {
 
 };
 
+/*
+ * Copy password from optarg into secure heap, so it could be
+ * freed in the same way as a result of get_password().
+ */
+static char *optarg_password(char *optarg)
+{
+	size_t len;
+	char *keypass;
+
+	if (!optarg)
+		return NULL;
+	len = strlen(optarg);
+	keypass = OPENSSL_secure_malloc(len + 1);

src/evmctl.c static struct option opts[] = { · line 2656

Quoted 4 lines

src/evmctl.c +4 -0

+	if (keypass)
+		memcpy(keypass, optarg, len + 1);
+	else
+		perror("OPENSSL_secure_malloc");

I also realized that OPENSSL_secure_malloc does not (intentionally) set errno, so using perror is perhaps wrong. Better method should be thanked out.