[PATCH ima-evm-utils] Improve memory handling for private keys and passwords

[PATCH ima-evm-utils] Improve memory handling for private keys and passwords

[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
and enabled for 'ima_sign', 'sign', 'sign_hash', and 'hmac'.

setvbuf(3) _IONBF is used to hopefully avoid private key and password
being stored inside of stdio buffers.

Unfortunately, secure heap is not used for the passwords (`-p') due to
absence of its support in the older OpenSSL where ima-evem-utils still
should work, thus simple cleansing of password memory is used where
possible to shorten its lifespan.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
Perhaps, it will conflict with Bruno's patch, we should decide which
one goes first if this will be accepted.

 src/evmctl.c    | 83 ++++++++++++++++++++++++++++++++++++++++++++++---
 src/libimaevm.c | 14 ++++++++-
 2 files changed, 92 insertions(+), 5 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index a8065bb..3275464 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -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,23 @@ struct tpm_bank_info {
 static char *pcrfile[MAX_PCRFILE];
 static unsigned npcrfile;
 
+static void init_openssl_secure_memory(void)
+{
+#if OPENSSL_VERSION_NUMBER > 0x10100000
+	/*
+	 * man CRYPTO_secure_malloc_init for explanation of the values.
+	 * 8192 is chosen to be big enough, so that any single key data could fit.
+	 */
+# define SECURE_HEAP_SIZE 8192
+# define SECURE_HEAP_MINSIZE 64
+	/*
+	 * Enable secure storage when working with private keys.
+	 * This facility is available since OpenSSL_1_1_0-pre1.
+	 */
+	CRYPTO_secure_malloc_init(SECURE_HEAP_SIZE, SECURE_HEAP_MINSIZE);
+#endif
+}
+
 static int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
 {
 	FILE *fp;
@@ -221,6 +239,8 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
 		fclose(fp);
 		return NULL;
 	}
+	/* Disable buffering because it's used to read private HMAC key too. */
+	setvbuf(fp, NULL, _IONBF, 0);
 	if (fread(data, len, 1, fp) != 1) {
 		log_err("Failed to fread %zu bytes: %s\n", len, name);
 		fclose(fp);
@@ -723,6 +743,7 @@ static int sign_ima_file(const char *file)
 
 static int cmd_sign_ima(struct command *cmd)
 {
+	init_openssl_secure_memory();
 	return do_cmd(cmd, sign_ima_file);
 }
 
@@ -737,6 +758,7 @@ static int cmd_sign_hash(struct command *cmd)
 	unsigned char sig[MAX_SIGNATURE_SIZE] = "\x03";
 	int siglen;
 
+	init_openssl_secure_memory();
 	key = imaevm_params.keyfile ? : "/etc/keys/privkey_evm.pem";
 
 	/* support reading hash (eg. output of shasum) */
@@ -796,6 +818,7 @@ static int sign_evm_path(const char *file)
 
 static int cmd_sign_evm(struct command *cmd)
 {
+	init_openssl_secure_memory();
 	return do_cmd(cmd, sign_evm_path);
 }
 
@@ -1104,6 +1127,7 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 
 	if (keylen > sizeof(evmkey)) {
 		log_err("key is too long: %d\n", keylen);
+		OPENSSL_cleanse(key, keylen);
 		goto out;
 	}
 
@@ -1111,6 +1135,7 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 	memcpy(evmkey, key, keylen);
 	if (keylen < sizeof(evmkey))
 		memset(evmkey + keylen, 0, sizeof(evmkey) - keylen);
+	OPENSSL_cleanse(key, keylen);
 
 	if (lstat(file, &st)) {
 		log_err("Failed to stat: %s\n", file);
@@ -1148,6 +1173,7 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 	}
 
 	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), md, NULL);
+	OPENSSL_cleanse(evmkey, sizeof(evmkey));
 	if (err) {
 		log_err("HMAC_Init() failed\n");
 		goto out;
@@ -1259,6 +1285,7 @@ static int cmd_hmac_evm(struct command *cmd)
 	const char *key, *file = g_argv[optind++];
 	int err;
 
+	init_openssl_secure_memory();
 	if (!file) {
 		log_err("Parameters missing\n");
 		print_usage(cmd);
@@ -2592,6 +2619,32 @@ static struct option opts[] = {
 
 };
 
+/*
+ * Copy password from optarg into malloc'd memory, 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 = malloc(len + 1);
+	if (keypass)
+		memcpy(keypass, optarg, len + 1);
+	else
+		perror("malloc");
+	/*
+	 * 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 password from TTY. */
 static char *get_password(void)
 {
 	struct termios flags, tmp_flags;
@@ -2614,6 +2667,7 @@ static char *get_password(void)
 		free(password);
 		return NULL;
 	}
+	setvbuf(stdin, NULL, _IONBF, 0);
 
 	printf("PEM password: ");
 	pwd = fgets(password, passlen, stdin);
@@ -2621,8 +2675,10 @@ static char *get_password(void)
 	/* restore terminal */
 	if (tcsetattr(fileno(stdin), TCSANOW, &flags) != 0) {
 		perror("tcsetattr");
-		free(password);
-		return NULL;
+		/*
+		 * Password is already here, so there is no point
+		 * to stop working on this petty error.
+		 */
 	}
 
 	return pwd;
@@ -2634,6 +2690,7 @@ int main(int argc, char *argv[])
 	ENGINE *eng = NULL;
 	unsigned long keyid;
 	char *eptr;
+	char *keypass = NULL;
 
 #if !(OPENSSL_VERSION_NUMBER < 0x10100000)
 	OPENSSL_init_crypto(
@@ -2673,10 +2730,19 @@ int main(int argc, char *argv[])
 			imaevm_params.hash_algo = optarg;
 			break;
 		case 'p':
+			if (keypass) {
+				OPENSSL_cleanse(keypass, strlen(keypass));
+				free(keypass);
+			}
 			if (optarg)
-				imaevm_params.keypass = optarg;
+				keypass = optarg_password(optarg);
 			else
-				imaevm_params.keypass = get_password();
+				keypass = get_password();
+			if (!keypass) {
+				log_err("Cannot read password\n");
+				goto quit;
+			}
+			imaevm_params.keypass = keypass;
 			break;
 		case 'f':
 			sigfile = 1;
@@ -2833,6 +2899,15 @@ int main(int argc, char *argv[])
 			err = 125;
 	}
 
+quit:
+	if (keypass) {
+		OPENSSL_cleanse(keypass, strlen(keypass));
+		free(keypass);
+	}
+#if OPENSSL_VERSION_NUMBER > 0x10100000
+	CRYPTO_secure_malloc_done();
+#endif
+
 	if (eng) {
 		ENGINE_finish(eng);
 		ENGINE_free(eng);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 19f1041..7bf3ba9 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -726,6 +726,10 @@ static int read_keyid_from_cert(uint32_t *keyid_be, const char *certfile, int tr
 		log_err("Cannot open %s: %s\n", certfile, strerror(errno));
 		return -1;
 	}
+
+	/* There could be private key, thus disable buffering. */
+	setvbuf(fp, NULL, _IONBF, 0);
+
 	if (!PEM_read_X509(fp, &x, NULL, NULL)) {
 		if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) {
 			ERR_clear_error();
@@ -799,6 +803,7 @@ static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
 		log_err("Failed to open keyfile: %s\n", keyfile);
 		return NULL;
 	}
+	setvbuf(fp, NULL, _IONBF, 0);
 	pkey = PEM_read_PrivateKey(fp, NULL, NULL, (void *)keypass);
 	if (!pkey) {
 		log_err("Failed to PEM_read_PrivateKey key file: %s\n",
@@ -1021,8 +1026,15 @@ err:
 
 int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig)
 {
-	if (keypass)
+	if (keypass && keypass != imaevm_params.keypass) {
+		/*
+		 * Cannot cleanse previous imaevm_params.keypass value, due to
+		 * it's being const, but can warn user.
+		 */
+		if (imaevm_params.keypass)
+			log_err("Overwriting non-empty imaevm_params.keypass\n");
 		imaevm_params.keypass = keypass;
+	}
 
 	return imaevm_params.x509 ?
 		sign_hash_v2(hashalgo, hash, size, keyfile, sig) :
-- 
2.29.3

Re: [PATCH ima-evm-utils] Improve memory handling for private keys and passwords

[Vitaly Chikunov]

Hi,

On Fri, Aug 13, 2021 at 12:21:43AM +0300, Vitaly Chikunov wrote:
> After CRYPTO_secure_malloc_init OpenSSL will store private keys in
> secure heap. This facility is only available since OpenSSL_1_1_0-pre1
> and enabled for 'ima_sign', 'sign', 'sign_hash', and 'hmac'.

> setvbuf(3) _IONBF is used to hopefully avoid private key and password
> being stored inside of stdio buffers.

I should note that usefulness of this method (of avoiding buffering) is
not proven. I don't find other implementations doing it. So, I'm open to
suggestion of removing it.

Thanks,

> 
> Unfortunately, secure heap is not used for the passwords (`-p') due to
> absence of its support in the older OpenSSL where ima-evem-utils still
> should work, thus simple cleansing of password memory is used where
> possible to shorten its lifespan.
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
> Perhaps, it will conflict with Bruno's patch, we should decide which
> one goes first if this will be accepted.
> 
>  src/evmctl.c    | 83 ++++++++++++++++++++++++++++++++++++++++++++++---
>  src/libimaevm.c | 14 ++++++++-
>  2 files changed, 92 insertions(+), 5 deletions(-)

Re: [PATCH ima-evm-utils] Improve memory handling for private keys and passwords

[Mimi Zohar]

Hi Vitaly,

On Fri, 2021-08-13 at 00:46 +0300, Vitaly Chikunov wrote:
> Hi,
> 
> On Fri, Aug 13, 2021 at 12:21:43AM +0300, Vitaly Chikunov wrote:
> > After CRYPTO_secure_malloc_init OpenSSL will store private keys in
> > secure heap. This facility is only available since OpenSSL_1_1_0-pre1
> > and enabled for 'ima_sign', 'sign', 'sign_hash', and 'hmac'.

From the manpage:
CRYPTO_secure_malloc_init() returns 0 on failure, 1 if successful, and
2 if successful but the heap could not be protected by memory mapping.

Not sure what we would do on failure ( 0, 2), but we should at least
check the return code.
> 
> > setvbuf(3) _IONBF is used to hopefully avoid private key and password
> > being stored inside of stdio buffers.
> 
> I should note that usefulness of this method (of avoiding buffering) is
> not proven. I don't find other implementations doing it. So, I'm open to
> suggestion of removing it.
> 

Probably would be better to split the patch.

> 
> > 
> > Unfortunately, secure heap is not used for the passwords (`-p') due to
> > absence of its support in the older OpenSSL where ima-evem-utils still
> > should work, thus simple cleansing of password memory is used where
> > possible to shorten its lifespan.
> > 
> > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > ---
> > Perhaps, it will conflict with Bruno's patch, we should decide which
> > one goes first if this will be accepted.

FYI, I was able to apply this patch (--3way) with the proposed changes
to Bruno's patch.  

Mimi

Re: [PATCH ima-evm-utils] Improve memory handling for private keys and passwords

[Mimi Zohar]

On Fri, 2021-08-13 at 17:31 -0400, Mimi Zohar wrote:
> Hi Vitaly,
> 
> On Fri, 2021-08-13 at 00:46 +0300, Vitaly Chikunov wrote:
> > Hi,
> > 
> > On Fri, Aug 13, 2021 at 12:21:43AM +0300, Vitaly Chikunov wrote:
> > > After CRYPTO_secure_malloc_init OpenSSL will store private keys in
> > > secure heap. This facility is only available since OpenSSL_1_1_0-pre1
> > > and enabled for 'ima_sign', 'sign', 'sign_hash', and 'hmac'.
> 
> From the manpage:
> CRYPTO_secure_malloc_init() returns 0 on failure, 1 if successful, and
> 2 if successful but the heap could not be protected by memory mapping.
> 
> Not sure what we would do on failure ( 0, 2), but we should at least
> check the return code.
> > 
> > > setvbuf(3) _IONBF is used to hopefully avoid private key and password
> > > being stored inside of stdio buffers.
> > 
> > I should note that usefulness of this method (of avoiding buffering) is
> > not proven. I don't find other implementations doing it. So, I'm open to
> > suggestion of removing it.
> > 
> Probably would be better to split the patch.

According to the man page "OPENSSL_secure_malloc() allocates num bytes
from the heap.  If CRYPTO_secure_malloc_init() is not called, this is
equivalent to calling OPENSSL_malloc()".   OPENSSL_malloc() is
supported in the older openssl versions.

Does it make sense to replace allocating memory for the password via
malloc() with OPENSSL_secure_malloc()?  For older openssl versions,
define OPENSSL_secure_malloc() and OPENSSL_secure_free() as
OPENSSL_malloc() and OPENSSL_free().

This doesn't solve the memory handling for private keys and passwords
for older openssl versions, but it is a path forward.

thanks,

Mimi

Re: [PATCH ima-evm-utils] Improve memory handling for private keys and passwords

[Vitaly Chikunov]

Mimi,

On Wed, Aug 18, 2021 at 05:07:20PM -0400, Mimi Zohar wrote:
> On Fri, 2021-08-13 at 17:31 -0400, Mimi Zohar wrote:
> > On Fri, 2021-08-13 at 00:46 +0300, Vitaly Chikunov wrote:
> > > On Fri, Aug 13, 2021 at 12:21:43AM +0300, Vitaly Chikunov wrote:
> > > > After CRYPTO_secure_malloc_init OpenSSL will store private keys in
> > > > secure heap. This facility is only available since OpenSSL_1_1_0-pre1
> > > > and enabled for 'ima_sign', 'sign', 'sign_hash', and 'hmac'.
> > 
> > From the manpage:
> > CRYPTO_secure_malloc_init() returns 0 on failure, 1 if successful, and
> > 2 if successful but the heap could not be protected by memory mapping.
> > 
> > Not sure what we would do on failure ( 0, 2), but we should at least
> > check the return code.
> > > 
> > > > setvbuf(3) _IONBF is used to hopefully avoid private key and password
> > > > being stored inside of stdio buffers.
> > > 
> > > I should note that usefulness of this method (of avoiding buffering) is
> > > not proven. I don't find other implementations doing it. So, I'm open to
> > > suggestion of removing it.
> > > 
> > Probably would be better to split the patch.
> 
> According to the man page "OPENSSL_secure_malloc() allocates num bytes
> from the heap.  If CRYPTO_secure_malloc_init() is not called, this is
> equivalent to calling OPENSSL_malloc()".   OPENSSL_malloc() is
> supported in the older openssl versions.
> 
> Does it make sense to replace allocating memory for the password via
> malloc() with OPENSSL_secure_malloc()?  For older openssl versions,
> define OPENSSL_secure_malloc() and OPENSSL_secure_free() as
> OPENSSL_malloc() and OPENSSL_free().

It makes sense to use OPENSSL_secure_malloc for passwords, but it will
look strange to define OPENSSL_secure_malloc as OPENSSL_malloc for older
versions.

Maybe we could add a #warning in that case.

Thanks,

> 
> This doesn't solve the memory handling for private keys and passwords
> for older openssl versions, but it is a path forward.
> 
> thanks,
> 
> Mimi