Use secure heap for private keys and passwords

PATCH Superseded Vitaly Chikunov 3 participants 7 messages Aug 22, 2021 – Aug 25, 2021 Keys-trusted

Share & tools

Vitaly Chikunov

author

2021-08-22 00:11 UTC #0 lore raw

PATCH v3 1/1 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]>

---
 src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 27 deletions(-)

src/evmctl.c +121 ~~-27~~

index 5f7c2b8..cebe9ec 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;
 
+#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
+ * to plain OPENSSL_malloc.
+ */
+#define OPENSSL_secure_clear_free OPENSSL_clear_free
+#define OPENSSL_clear_free(ptr, num)		\
+	do {					\
+		OPENSSL_cleanse(ptr, num);	\
+		OPENSSL_free(ptr);		\
+	} while (0)
+#endif
+
 static int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
 {
 	FILE *fp;

@@ -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;
 
-	data = malloc(len);
+	if (secure)
+		data = OPENSSL_secure_malloc(len);
+	else
+		data = malloc(len);
 	if (!data) {
 		log_err("Failed to malloc %zu bytes: %s\n", len, name);
 		fclose(fp);

@@ -224,7 +248,10 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
 	if (fread(data, len, 1, fp) != 1) {
 		log_err("Failed to fread %zu bytes: %s\n", len, name);
 		fclose(fp);
-		free(data);
+		if (secure)
+			OPENSSL_secure_clear_free(data, len);
+		else
+			free(data);
 		return NULL;
 	}
 	fclose(fp);

@@ -872,7 +899,7 @@ static int verify_ima(const char *file)
 	int len;
 
 	if (sigfile) {
-		void *tmp = file2bin(file, "sig", &len);
+		void *tmp = file2bin(file, "sig", &len, 0);
 
 		if (!tmp) {
 			log_err("Failed reading: %s\n", file);

@@ -1001,7 +1028,7 @@ static int cmd_import(struct command *cmd)
 
 		if (!pkey)
 			return 1;
-		pub = file2bin(inkey, NULL, &len);
+		pub = file2bin(inkey, NULL, &len, 0);
 		if (!pub) {
 			EVP_PKEY_free(pkey);
 			return 1;

@@ -1040,9 +1067,9 @@ static int setxattr_ima(const char *file, char *sig_file)
 	int len, err;
 
 	if (sig_file)
-		sig = file2bin(sig_file, NULL, &len);
+		sig = file2bin(sig_file, NULL, &len, 0);
 	else
-		sig = file2bin(file, "sig", &len);
+		sig = file2bin(file, "sig", &len, 0);
 	if (!sig)
 		return 0;

@@ -1082,9 +1109,9 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 	unsigned int mdlen;
 	char **xattrname;
 	unsigned char xattr_value[1024];
-	unsigned char *key;
+	unsigned char *key; /* @secure heap */
 	int keylen;
-	unsigned char evmkey[MAX_KEY_SIZE];
+	unsigned char *evmkey; /* @secure heap */
 	char list[1024];
 	ssize_t list_size;
 	struct h_misc_64 hmac_misc;

@@ -1096,21 +1123,30 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 	pctx = HMAC_CTX_new();
 #endif
 
-	key = file2bin(keyfile, NULL, &keylen);
+	key = file2bin(keyfile, NULL, &keylen, 1);
 	if (!key) {
 		log_err("Failed to read a key: %s\n", keyfile);
 		return -1;
 	}
 
-	if (keylen > sizeof(evmkey)) {
+	evmkey = OPENSSL_secure_malloc(MAX_KEY_SIZE);
+	if (!evmkey) {
+		log_err("Failed to allocate %d bytes\n", MAX_KEY_SIZE);
+		goto out;
+	}
+
+	if (keylen > MAX_KEY_SIZE) {
 		log_err("key is too long: %d\n", keylen);
 		goto out;
 	}
 
 	/* EVM key is 128 bytes */
 	memcpy(evmkey, key, keylen);
-	if (keylen < sizeof(evmkey))
-		memset(evmkey + keylen, 0, sizeof(evmkey) - keylen);
+	if (keylen < MAX_KEY_SIZE)
+		memset(evmkey + keylen, 0, MAX_KEY_SIZE - keylen);
+
+	/* Shorten lifetime of key data. */
+	OPENSSL_cleanse(key, keylen);
 
 	if (lstat(file, &st)) {
 		log_err("Failed to stat: %s\n", file);

@@ -1147,12 +1183,15 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 		goto out;
 	}
 
-	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), md, NULL);
+	err = !HMAC_Init_ex(pctx, evmkey, MAX_KEY_SIZE, md, NULL);
 	if (err) {
 		log_err("HMAC_Init() failed\n");
 		goto out;
 	}
 
+	/* Shorten lifetime of evmkey data. */
+	OPENSSL_cleanse(evmkey, MAX_KEY_SIZE);
+
 	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
 		err = lgetxattr(file, *xattrname, xattr_value, sizeof(xattr_value));
 		if (err < 0) {

@@ -1222,7 +1261,9 @@ out_ctx_cleanup:
 	HMAC_CTX_free(pctx);
 #endif
 out:
-	free(key);
+	if (evmkey)
+		OPENSSL_secure_clear_free(evmkey, MAX_KEY_SIZE);
+	OPENSSL_secure_clear_free(key, keylen);
 	return err ?: mdlen;
 }

@@ -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);
+	if (keypass)
+		memcpy(keypass, optarg, len + 1);
+	else
+		log_err("OPENSSL_secure_malloc(%zu) failed\n", len + 1);
+	/*
+	 * 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)
 {
 	struct termios flags, tmp_flags;
 	char *password, *pwd;
-	int passlen = 64;
+	const int passlen = 64;
 
-	password = malloc(passlen);
+	password = OPENSSL_secure_malloc(passlen);
 	if (!password) {
-		perror("malloc");
+		log_err("OPENSSL_secure_malloc(%u) failed\n", passlen);
 		return NULL;
 	}

@@ -2614,8 +2681,8 @@ static char *get_password(void)
 	tmp_flags.c_lflag |= ECHONL;
 
 	if (tcsetattr(fileno(stdin), TCSANOW, &tmp_flags) != 0) {
-		perror("tcsetattr");
-		free(password);
+		log_err("tcsetattr: %s\n", strerror(errno));
+		OPENSSL_secure_free(password);
 		return NULL;
 	}

@@ -2624,13 +2691,15 @@ static char *get_password(void)
 
 	/* restore terminal */
 	if (tcsetattr(fileno(stdin), TCSANOW, &flags) != 0) {
-		perror("tcsetattr");
-		free(password);
-		return NULL;
+		log_err("tcsetattr: %s\n", strerror(errno));
+		/*
+		 * Password is already here, so there is no reason
+		 * to stop working on this petty error.
+		 */
 	}
 
 	if (pwd == NULL) {
-		free(password);
+		OPENSSL_secure_free(password);
 		return NULL;
 	}

@@ -2643,6 +2712,7 @@ int main(int argc, char *argv[])
 	ENGINE *eng = NULL;
 	unsigned long keyid;
 	char *eptr;
+	char *keypass = NULL; /* @secure heap */
 
 #if !(OPENSSL_VERSION_NUMBER < 0x10100000)
 	OPENSSL_init_crypto(

@@ -2651,6 +2721,17 @@ int main(int argc, char *argv[])
 #endif
 			    OPENSSL_INIT_ENGINE_ALL_BUILTIN, NULL);
 #endif
+#if OPENSSL_VERSION_NUMBER > 0x10100000
+	/*
+	 * This facility is available since OpenSSL_1_1_0-pre1.
+	 * 'Heap size' 8192 is chosen to be big enough, so that any single key
+	 * data could fit. 'Heap minsize' 64 is just to be efficient for small
+	 * buffers.
+	 */
+	if (!CRYPTO_secure_malloc_init(8192, 64))
+		log_err("CRYPTO_secure_malloc_init() failed\n");
+#endif
+
 	g_argv = argv;
 	g_argc = argc;

@@ -2682,10 +2763,18 @@ int main(int argc, char *argv[])
 			imaevm_params.hash_algo = optarg;
 			break;
 		case 'p':
+			if (keypass)
+				OPENSSL_secure_clear_free(keypass,
+							  strlen(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;

@@ -2841,7 +2930,9 @@ int main(int argc, char *argv[])
 		if (err < 0)
 			err = 125;
 	}
-
+quit:
+	if (keypass)
+		OPENSSL_secure_clear_free(keypass, strlen(keypass));
 	if (eng) {
 		ENGINE_finish(eng);
 		ENGINE_free(eng);

@@ -2849,6 +2940,9 @@ int main(int argc, char *argv[])
 		ENGINE_cleanup();
 #endif
 	}
+#if OPENSSL_VERSION_NUMBER > 0x10100000
+	CRYPTO_secure_malloc_done();
+#endif
 	ERR_free_strings();
 	EVP_cleanup();
 	BIO_free(NULL);

Signature · 1 line

2.29.3

Bruno Meneguele

Reviewed-by on v3 1/1

2021-08-23 13:22 UTC #1 lore raw

Quoted 175 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]>

---
 src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 27 deletions(-)

src/evmctl.c +53 ~~-14~~

index 5f7c2b8..cebe9ec 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;
 
+#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
+ * to plain OPENSSL_malloc.
+ */
+#define OPENSSL_secure_clear_free OPENSSL_clear_free
+#define OPENSSL_clear_free(ptr, num)		\
+	do {					\
+		OPENSSL_cleanse(ptr, num);	\
+		OPENSSL_free(ptr);		\
+	} while (0)
+#endif
+
 static int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
 {
 	FILE *fp;

@@ -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;
 
-	data = malloc(len);
+	if (secure)
+		data = OPENSSL_secure_malloc(len);
+	else
+		data = malloc(len);
 	if (!data) {
 		log_err("Failed to malloc %zu bytes: %s\n", len, name);
 		fclose(fp);

@@ -224,7 +248,10 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
 	if (fread(data, len, 1, fp) != 1) {
 		log_err("Failed to fread %zu bytes: %s\n", len, name);
 		fclose(fp);
-		free(data);
+		if (secure)
+			OPENSSL_secure_clear_free(data, len);
+		else
+			free(data);
 		return NULL;
 	}
 	fclose(fp);

@@ -872,7 +899,7 @@ static int verify_ima(const char *file)
 	int len;
 
 	if (sigfile) {
-		void *tmp = file2bin(file, "sig", &len);
+		void *tmp = file2bin(file, "sig", &len, 0);
 
 		if (!tmp) {
 			log_err("Failed reading: %s\n", file);

@@ -1001,7 +1028,7 @@ static int cmd_import(struct command *cmd)
 
 		if (!pkey)
 			return 1;
-		pub = file2bin(inkey, NULL, &len);
+		pub = file2bin(inkey, NULL, &len, 0);
 		if (!pub) {
 			EVP_PKEY_free(pkey);
 			return 1;

@@ -1040,9 +1067,9 @@ static int setxattr_ima(const char *file, char *sig_file)
 	int len, err;
 
 	if (sig_file)
-		sig = file2bin(sig_file, NULL, &len);
+		sig = file2bin(sig_file, NULL, &len, 0);
 	else
-		sig = file2bin(file, "sig", &len);
+		sig = file2bin(file, "sig", &len, 0);
 	if (!sig)
 		return 0;

@@ -1082,9 +1109,9 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 	unsigned int mdlen;
 	char **xattrname;
 	unsigned char xattr_value[1024];
-	unsigned char *key;
+	unsigned char *key; /* @secure heap */
 	int keylen;
-	unsigned char evmkey[MAX_KEY_SIZE];
+	unsigned char *evmkey; /* @secure heap */
 	char list[1024];
 	ssize_t list_size;
 	struct h_misc_64 hmac_misc;

@@ -1096,21 +1123,30 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 	pctx = HMAC_CTX_new();
 #endif
 
-	key = file2bin(keyfile, NULL, &keylen);
+	key = file2bin(keyfile, NULL, &keylen, 1);
 	if (!key) {
 		log_err("Failed to read a key: %s\n", keyfile);
 		return -1;
 	}
 
-	if (keylen > sizeof(evmkey)) {
+	evmkey = OPENSSL_secure_malloc(MAX_KEY_SIZE);
+	if (!evmkey) {
+		log_err("Failed to allocate %d bytes\n", MAX_KEY_SIZE);
+		goto out;
+	}
+
+	if (keylen > MAX_KEY_SIZE) {
 		log_err("key is too long: %d\n", keylen);
 		goto out;
 	}
 
 	/* EVM key is 128 bytes */
 	memcpy(evmkey, key, keylen);
-	if (keylen < sizeof(evmkey))
-		memset(evmkey + keylen, 0, sizeof(evmkey) - keylen);
+	if (keylen < MAX_KEY_SIZE)
+		memset(evmkey + keylen, 0, MAX_KEY_SIZE - keylen);
+
+	/* Shorten lifetime of key data. */
+	OPENSSL_cleanse(key, keylen);
 
 	if (lstat(file, &st)) {
 		log_err("Failed to stat: %s\n", file);

@@ -1147,12 +1183,15 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 		goto out;
 	}
 
-	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), md, NULL);
+	err = !HMAC_Init_ex(pctx, evmkey, MAX_KEY_SIZE, md, NULL);
 	if (err) {
 		log_err("HMAC_Init() failed\n");
 		goto out;
 	}
 
+	/* Shorten lifetime of evmkey data. */
+	OPENSSL_cleanse(evmkey, MAX_KEY_SIZE);
+
 	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
 		err = lgetxattr(file, *xattrname, xattr_value, sizeof(xattr_value));
 		if (err < 0) {

src/evmctl.c out_ctx_cleanup: · line 1266

Quoted 7 lines

src/evmctl.c +3 -1

@@ -1222,7 +1261,9 @@ out_ctx_cleanup:
 	HMAC_CTX_free(pctx);
 #endif
 out:
-	free(key);
+	if (evmkey)
+		OPENSSL_secure_clear_free(evmkey, MAX_KEY_SIZE);
+	OPENSSL_secure_clear_free(key, keylen);

Minor thing: considering you already OPENSSL_cleanse() both evmkey and key, is it necessary to call OPENSSL_secure_clear_free() instead of only OPENSSL_secure_free()?

But like I said, that's a minor thing :)

Reviewed-by Bruno Meneguele <[email protected]>

Signature · 2 lines

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

Vitaly Chikunov

author

2021-08-23 16:59 UTC #2 lore raw

Bruno,

Quoted 179 lines · Bruno Meneguele

Quoted 178 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]>

---
 src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 27 deletions(-)

src/evmctl.c +53 ~~-14~~

index 5f7c2b8..cebe9ec 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;
 
+#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
+ * to plain OPENSSL_malloc.
+ */
+#define OPENSSL_secure_clear_free OPENSSL_clear_free
+#define OPENSSL_clear_free(ptr, num)		\
+	do {					\
+		OPENSSL_cleanse(ptr, num);	\
+		OPENSSL_free(ptr);		\
+	} while (0)
+#endif
+
 static int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
 {
 	FILE *fp;

@@ -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;
 
-	data = malloc(len);
+	if (secure)
+		data = OPENSSL_secure_malloc(len);
+	else
+		data = malloc(len);
 	if (!data) {
 		log_err("Failed to malloc %zu bytes: %s\n", len, name);
 		fclose(fp);

@@ -224,7 +248,10 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
 	if (fread(data, len, 1, fp) != 1) {
 		log_err("Failed to fread %zu bytes: %s\n", len, name);
 		fclose(fp);
-		free(data);
+		if (secure)
+			OPENSSL_secure_clear_free(data, len);
+		else
+			free(data);
 		return NULL;
 	}
 	fclose(fp);

@@ -872,7 +899,7 @@ static int verify_ima(const char *file)
 	int len;
 
 	if (sigfile) {
-		void *tmp = file2bin(file, "sig", &len);
+		void *tmp = file2bin(file, "sig", &len, 0);
 
 		if (!tmp) {
 			log_err("Failed reading: %s\n", file);

@@ -1001,7 +1028,7 @@ static int cmd_import(struct command *cmd)
 
 		if (!pkey)
 			return 1;
-		pub = file2bin(inkey, NULL, &len);
+		pub = file2bin(inkey, NULL, &len, 0);
 		if (!pub) {
 			EVP_PKEY_free(pkey);
 			return 1;

@@ -1040,9 +1067,9 @@ static int setxattr_ima(const char *file, char *sig_file)
 	int len, err;
 
 	if (sig_file)
-		sig = file2bin(sig_file, NULL, &len);
+		sig = file2bin(sig_file, NULL, &len, 0);
 	else
-		sig = file2bin(file, "sig", &len);
+		sig = file2bin(file, "sig", &len, 0);
 	if (!sig)
 		return 0;

@@ -1082,9 +1109,9 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 	unsigned int mdlen;
 	char **xattrname;
 	unsigned char xattr_value[1024];
-	unsigned char *key;
+	unsigned char *key; /* @secure heap */
 	int keylen;
-	unsigned char evmkey[MAX_KEY_SIZE];
+	unsigned char *evmkey; /* @secure heap */
 	char list[1024];
 	ssize_t list_size;
 	struct h_misc_64 hmac_misc;

@@ -1096,21 +1123,30 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 	pctx = HMAC_CTX_new();
 #endif
 
-	key = file2bin(keyfile, NULL, &keylen);
+	key = file2bin(keyfile, NULL, &keylen, 1);
 	if (!key) {
 		log_err("Failed to read a key: %s\n", keyfile);
 		return -1;
 	}
 
-	if (keylen > sizeof(evmkey)) {
+	evmkey = OPENSSL_secure_malloc(MAX_KEY_SIZE);
+	if (!evmkey) {
+		log_err("Failed to allocate %d bytes\n", MAX_KEY_SIZE);
+		goto out;
+	}
+
+	if (keylen > MAX_KEY_SIZE) {
 		log_err("key is too long: %d\n", keylen);
 		goto out;
 	}
 
 	/* EVM key is 128 bytes */
 	memcpy(evmkey, key, keylen);
-	if (keylen < sizeof(evmkey))
-		memset(evmkey + keylen, 0, sizeof(evmkey) - keylen);
+	if (keylen < MAX_KEY_SIZE)
+		memset(evmkey + keylen, 0, MAX_KEY_SIZE - keylen);
+
+	/* Shorten lifetime of key data. */
+	OPENSSL_cleanse(key, keylen);
 
 	if (lstat(file, &st)) {
 		log_err("Failed to stat: %s\n", file);

@@ -1147,12 +1183,15 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 		goto out;
 	}
 
-	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), md, NULL);
+	err = !HMAC_Init_ex(pctx, evmkey, MAX_KEY_SIZE, md, NULL);
 	if (err) {
 		log_err("HMAC_Init() failed\n");
 		goto out;
 	}
 
+	/* Shorten lifetime of evmkey data. */
+	OPENSSL_cleanse(evmkey, MAX_KEY_SIZE);
+
 	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
 		err = lgetxattr(file, *xattrname, xattr_value, sizeof(xattr_value));
 		if (err < 0) {

@@ -1222,7 +1261,9 @@ out_ctx_cleanup:
 	HMAC_CTX_free(pctx);
 #endif
 out:

Quoted 8 lines

src/evmctl.c out_ctx_cleanup: · line 1266
Quoted 4 lines
src/evmctl.c +3 -1
-	free(key);
+	if (evmkey)
+		OPENSSL_secure_clear_free(evmkey, MAX_KEY_SIZE);
+	OPENSSL_secure_clear_free(key, keylen);
Minor thing: considering you already OPENSSL_cleanse() both evmkey and key, is it necessary to call OPENSSL_secure_clear_free() instead of only OPENSSL_secure_free()?

There is already comment to explain this - it's to shorten key lifetime on one execution path. Note there is possible goto out before OPENSSL_cleanse calls.

Thanks,

Quoted 8 lines

But like I said, that's a minor thing :)
Reviewed-by Bruno Meneguele <[email protected]>
Signature · 2 lines
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

Bruno Meneguele

2021-08-23 18:09 UTC #3 lore raw

Quoted 191 lines · Vitaly Chikunov

Bruno,

Quoted 179 lines · Bruno Meneguele

Quoted 178 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]>

---
 src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 27 deletions(-)

src/evmctl.c +53 ~~-14~~

index 5f7c2b8..cebe9ec 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;
 
+#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
+ * to plain OPENSSL_malloc.
+ */
+#define OPENSSL_secure_clear_free OPENSSL_clear_free
+#define OPENSSL_clear_free(ptr, num)		\
+	do {					\
+		OPENSSL_cleanse(ptr, num);	\
+		OPENSSL_free(ptr);		\
+	} while (0)
+#endif
+
 static int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
 {
 	FILE *fp;

@@ -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;
 
-	data = malloc(len);
+	if (secure)
+		data = OPENSSL_secure_malloc(len);
+	else
+		data = malloc(len);
 	if (!data) {
 		log_err("Failed to malloc %zu bytes: %s\n", len, name);
 		fclose(fp);

@@ -224,7 +248,10 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
 	if (fread(data, len, 1, fp) != 1) {
 		log_err("Failed to fread %zu bytes: %s\n", len, name);
 		fclose(fp);
-		free(data);
+		if (secure)
+			OPENSSL_secure_clear_free(data, len);
+		else
+			free(data);
 		return NULL;
 	}
 	fclose(fp);

@@ -872,7 +899,7 @@ static int verify_ima(const char *file)
 	int len;
 
 	if (sigfile) {
-		void *tmp = file2bin(file, "sig", &len);
+		void *tmp = file2bin(file, "sig", &len, 0);
 
 		if (!tmp) {
 			log_err("Failed reading: %s\n", file);

@@ -1001,7 +1028,7 @@ static int cmd_import(struct command *cmd)
 
 		if (!pkey)
 			return 1;
-		pub = file2bin(inkey, NULL, &len);
+		pub = file2bin(inkey, NULL, &len, 0);
 		if (!pub) {
 			EVP_PKEY_free(pkey);
 			return 1;

@@ -1040,9 +1067,9 @@ static int setxattr_ima(const char *file, char *sig_file)
 	int len, err;
 
 	if (sig_file)
-		sig = file2bin(sig_file, NULL, &len);
+		sig = file2bin(sig_file, NULL, &len, 0);
 	else
-		sig = file2bin(file, "sig", &len);
+		sig = file2bin(file, "sig", &len, 0);
 	if (!sig)
 		return 0;

@@ -1082,9 +1109,9 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 	unsigned int mdlen;
 	char **xattrname;
 	unsigned char xattr_value[1024];
-	unsigned char *key;
+	unsigned char *key; /* @secure heap */
 	int keylen;
-	unsigned char evmkey[MAX_KEY_SIZE];
+	unsigned char *evmkey; /* @secure heap */
 	char list[1024];
 	ssize_t list_size;
 	struct h_misc_64 hmac_misc;

@@ -1096,21 +1123,30 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 	pctx = HMAC_CTX_new();
 #endif
 
-	key = file2bin(keyfile, NULL, &keylen);
+	key = file2bin(keyfile, NULL, &keylen, 1);
 	if (!key) {
 		log_err("Failed to read a key: %s\n", keyfile);
 		return -1;
 	}
 
-	if (keylen > sizeof(evmkey)) {
+	evmkey = OPENSSL_secure_malloc(MAX_KEY_SIZE);
+	if (!evmkey) {
+		log_err("Failed to allocate %d bytes\n", MAX_KEY_SIZE);
+		goto out;
+	}
+
+	if (keylen > MAX_KEY_SIZE) {
 		log_err("key is too long: %d\n", keylen);
 		goto out;
 	}
 
 	/* EVM key is 128 bytes */
 	memcpy(evmkey, key, keylen);
-	if (keylen < sizeof(evmkey))
-		memset(evmkey + keylen, 0, sizeof(evmkey) - keylen);
+	if (keylen < MAX_KEY_SIZE)
+		memset(evmkey + keylen, 0, MAX_KEY_SIZE - keylen);
+
+	/* Shorten lifetime of key data. */
+	OPENSSL_cleanse(key, keylen);
 
 	if (lstat(file, &st)) {
 		log_err("Failed to stat: %s\n", file);

@@ -1147,12 +1183,15 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 		goto out;
 	}
 
-	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), md, NULL);
+	err = !HMAC_Init_ex(pctx, evmkey, MAX_KEY_SIZE, md, NULL);
 	if (err) {
 		log_err("HMAC_Init() failed\n");
 		goto out;
 	}
 
+	/* Shorten lifetime of evmkey data. */
+	OPENSSL_cleanse(evmkey, MAX_KEY_SIZE);
+
 	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
 		err = lgetxattr(file, *xattrname, xattr_value, sizeof(xattr_value));
 		if (err < 0) {

@@ -1222,7 +1261,9 @@ out_ctx_cleanup:
 	HMAC_CTX_free(pctx);
 #endif
 out:

Quoted 8 lines

src/evmctl.c out_ctx_cleanup: · line 1266
Quoted 4 lines
src/evmctl.c +3 -1
-	free(key);
+	if (evmkey)
+		OPENSSL_secure_clear_free(evmkey, MAX_KEY_SIZE);
+	OPENSSL_secure_clear_free(key, keylen);
Minor thing: considering you already OPENSSL_cleanse() both evmkey and key, is it necessary to call OPENSSL_secure_clear_free() instead of only OPENSSL_secure_free()?

Quoted 6 lines

There is already comment to explain this - it's to shorten key lifetime on one execution path. Note there is possible goto out before OPENSSL_cleanse calls.
Thanks,

Got it!

Thanks.

Quoted 10 lines

Quoted 8 lines
But like I said, that's a minor thing :)
Reviewed-by Bruno Meneguele <[email protected]>
Signature · 2 lines
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

Signature · 2 lines

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

Mimi Zohar

maintainer

2021-08-25 11:39 UTC #4 lore raw

In reply to Vitaly Chikunov

Hi Vitaly,

Quoted 1 line · Vitaly Chikunov

After CRYPTO_secure_malloc_init OpenSSL will store private keys

^and passwords

Quoted 4 lines

in
secure heap. This facility is only available since OpenSSL_1_1_0-pre1.
Signed-off-by Vitaly Chikunov <[email protected]>

Initially we started out discussing ways of protecting passwords, which this patch does. Thank you! I'm not sure, however, it is protecting the private keys. Does read_priv_pkey() also use the secure heap or is PEM_read_PrivateKey() already safe?

Quoted 7 lines

---
 src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 27 deletions(-)

src/evmctl.c static int bin2file(const char *file, const char *ext, const unsigned char *data · line 213

Quoted 8 lines

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)
 {

The only caller of file2bin() that sets "secure" is evm_calc_hmac(), but evm_calc_hmac() is a debugging tool, not meant for setting the real security.evm xattr.

The kernel EVM HMAC key is an "encrypted" key type, which should be based on a "trusted" key. Neither of which are exposed to userspace unencrypted.

Enabling DEBUG by default was suppose to be temporary. At this point, should it be disabled? As evm_calc_hmac() is only meant for debugging, do we really care whether evm_calc_hmac() uses a secure heap or stack for private keys or passwords?

thanks,

Mimi

Quoted 2 lines

src/evmctl.c +0 -0

 	FILE *fp;
 	size_t len;

Vitaly Chikunov

author

2021-08-25 18:41 UTC #5 lore raw

Mimi,

Quoted 6 lines · Mimi Zohar

Hi Vitaly,
Quoted 1 line · Vitaly Chikunov
After CRYPTO_secure_malloc_init OpenSSL will store private keys
^and passwords

No, it meant openssl automatically will handle private keys (and other internal private data) in secure heap.

Handling of passwords in openssl's secure heap we do ourselves. I can extend commit message about this.

Quoted 9 lines

Quoted 4 lines
in
secure heap. This facility is only available since OpenSSL_1_1_0-pre1.
Signed-off-by Vitaly Chikunov <[email protected]>
Initially we started out discussing ways of protecting passwords, which this patch does. Thank you! I'm not sure, however, it is protecting the private keys. Does read_priv_pkey() also use the secure heap or is PEM_read_PrivateKey() already safe?

After CRYPTO_secure_malloc_init call all openssl API functions that handle private keys and passwords should use secure heap. So, in that regard we don't need to add anything.

Quoted 11 lines

---
 src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 27 deletions(-)

src/evmctl.c +0 -0

@@ -188,7 +207,9 @@ static int bin2file(const char *file, const char *ext, const unsigned char *data
 	return err;
 }

Quoted 9 lines

src/evmctl.c static int bin2file(const char *file, const char *ext, const unsigned char *data · line 213
Quoted 5 lines
src/evmctl.c +3 -1
-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)
 {
The only caller of file2bin() that sets "secure" is evm_calc_hmac(), but evm_calc_hmac() is a debugging tool, not meant for setting the real security.evm xattr.

I can undo file2bin change if you wish, but for uniformity I would have it changed. We can add comment that it is handling only test & debugging data, thus not using security measures. May also output some warning at runtime.

Quoted 8 lines

The kernel EVM HMAC key is an "encrypted" key type, which should be based on a "trusted" key. Neither of which are exposed to userspace unencrypted.
Enabling DEBUG by default was suppose to be temporary. At this point, should it be disabled? As evm_calc_hmac() is only meant for debugging, do we really care whether evm_calc_hmac() uses a secure heap or stack for private keys or passwords?

Thanks,

Quoted 7 lines

thanks,
Mimi
Quoted 2 lines
src/evmctl.c +0 -0
 	FILE *fp;
 	size_t len;

Mimi Zohar

maintainer

2021-08-25 20:45 UTC #6 lore raw

Quoted 8 lines · Vitaly Chikunov

Mimi,
Quoted 5 lines · Mimi Zohar
Hi Vitaly,
Quoted 1 line · Vitaly Chikunov
After CRYPTO_secure_malloc_init OpenSSL will store private keys

Quoted 7 lines

Quoted 1 line
^and passwords
No, it meant openssl automatically will handle private keys (and other internal private data) in secure heap.
Handling of passwords in openssl's secure heap we do ourselves. I can extend commit message about this.

Ah, nice! Please expand the patch description, adding what you said
here.

Also, you might want to include that since OPENSSL_secure_malloc_init() is called from evmctl.c, any application linking with libimaevm would not be using secure heap.

Quoted 6 lines

Quoted 6 lines
Quoted 5 lines
in
secure heap. This facility is only available since OpenSSL_1_1_0-pre1.
Signed-off-by Vitaly Chikunov <[email protected]>

Quoted 8 lines

Quoted 4 lines
Initially we started out discussing ways of protecting passwords, which this patch does. Thank you! I'm not sure, however, it is protecting the private keys. Does read_priv_pkey() also use the secure heap or is PEM_read_PrivateKey() already safe?
After CRYPTO_secure_malloc_init call all openssl API functions that handle private keys and passwords should use secure heap. So, in that regard we don't need to add anything.

Quoted 21 lines

Quoted 11 lines
Quoted 11 lines
---
 src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 27 deletions(-)
src/evmctl.c +0 -0
@@ -188,7 +207,9 @@ static int bin2file(const char *file, const char *ext, const unsigned char *data
 	return err;
 }
 
Quoted 9 lines
src/evmctl.c static int bin2file(const char *file, const char *ext, const unsigned char *data · line 213
Quoted 5 lines
src/evmctl.c +3 -1
-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)
 {
The only caller of file2bin() that sets "secure" is evm_calc_hmac(), but evm_calc_hmac() is a debugging tool, not meant for setting the real security.evm xattr.

Quoted 4 lines

I can undo file2bin change if you wish, but for uniformity I would have it changed. We can add comment that it is handling only test & debugging data, thus not using security measures. May also output some warning at runtime.

Either way is fine, but somehow please document it.

thanks,

Mimi