[PATCH ima-evm-utils v3] Use secure heap for private keys and passwords

[PATCH ima-evm-utils v3] Use secure heap 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.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 27 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 5f7c2b8..cebe9ec 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,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);
-- 
2.29.3

Re: [PATCH ima-evm-utils v3] Use secure heap for private keys and passwords

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 6313 bytes --]

On Sun, Aug 22, 2021 at 03:10:55AM +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.
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
>  src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 121 insertions(+), 27 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 5f7c2b8..cebe9ec 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,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);

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 <bmeneg@redhat.com>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH ima-evm-utils v3] Use secure heap for private keys and passwords

[Vitaly Chikunov]

Bruno,

On Mon, Aug 23, 2021 at 10:22:13AM -0300, Bruno Meneguele wrote:
> On Sun, Aug 22, 2021 at 03:10:55AM +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.
> > 
> > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > ---
> >  src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 121 insertions(+), 27 deletions(-)
> > 
> > diff --git a/src/evmctl.c b/src/evmctl.c
> > index 5f7c2b8..cebe9ec 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,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);
> 
> 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,

> 
> But like I said, that's a minor thing :)
> 
> Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
> 
> -- 
> bmeneg 
> PGP Key: http://bmeneg.com/pubkey.txt

Re: [PATCH ima-evm-utils v3] Use secure heap for private keys and passwords

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 7519 bytes --]

On Mon, Aug 23, 2021 at 07:59:37PM +0300, Vitaly Chikunov wrote:
> Bruno,
> 
> On Mon, Aug 23, 2021 at 10:22:13AM -0300, Bruno Meneguele wrote:
> > On Sun, Aug 22, 2021 at 03:10:55AM +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.
> > > 
> > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > ---
> > >  src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
> > >  1 file changed, 121 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/src/evmctl.c b/src/evmctl.c
> > > index 5f7c2b8..cebe9ec 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,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);
> > 
> > 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,
> 

Got it!

Thanks.

> > 
> > But like I said, that's a minor thing :)
> > 
> > Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
> > 
> > -- 
> > bmeneg 
> > PGP Key: http://bmeneg.com/pubkey.txt
> 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH ima-evm-utils v3] Use secure heap for private keys and passwords

[Mimi Zohar]

Hi Vitaly,

On Sun, 2021-08-22 at 03:10 +0300, Vitaly Chikunov wrote:
> After CRYPTO_secure_malloc_init OpenSSL will store private keys 

^and passwords

> in
> secure heap. This facility is only available since OpenSSL_1_1_0-pre1.
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>

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?

> ---
>  src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 121 insertions(+), 27 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> 
> @@ -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

>  	FILE *fp;
>  	size_t len;

Re: [PATCH ima-evm-utils v3] Use secure heap for private keys and passwords

[Vitaly Chikunov]

Mimi,

On Wed, Aug 25, 2021 at 07:39:30AM -0400, Mimi Zohar wrote:
> Hi Vitaly,
> 
> On Sun, 2021-08-22 at 03:10 +0300, Vitaly Chikunov wrote:
> > 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.

> > in
> > secure heap. This facility is only available since OpenSSL_1_1_0-pre1.
> > 
> > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> 
> 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.

> 
> > ---
> >  src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 121 insertions(+), 27 deletions(-)
> > 
> > diff --git a/src/evmctl.c b/src/evmctl.c
> > 
> > @@ -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.

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.

> 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,


> 
> thanks,
> 
> Mimi
> 
> >  	FILE *fp;
> >  	size_t len;

Re: [PATCH ima-evm-utils v3] Use secure heap for private keys and passwords

[Mimi Zohar]

On Wed, 2021-08-25 at 21:41 +0300, Vitaly Chikunov wrote:
> Mimi,
> 
> On Wed, Aug 25, 2021 at 07:39:30AM -0400, Mimi Zohar wrote:
> > Hi Vitaly,
> > 
> > On Sun, 2021-08-22 at 03:10 +0300, Vitaly Chikunov wrote:
> > > 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.

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.

> 
> > > in
> > > secure heap. This facility is only available since OpenSSL_1_1_0-pre1.
> > > 
> > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > 
> > 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.

Ok

> > 
> > > ---
> > >  src/evmctl.c | 148 +++++++++++++++++++++++++++++++++++++++++----------
> > >  1 file changed, 121 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/src/evmctl.c b/src/evmctl.c
> > > 
> > > @@ -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.
> 
> 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