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

[Bruno Meneguele <bmeneg@redhat.com>]

[-- 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 --]