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

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

[Vitaly Chikunov]

After CRYPTO_secure_malloc_init OpenSSL will automatically store private
keys in secure heap. OPENSSL_secure_malloc(3):

  If a secure heap is used, then private key BIGNUM values are stored
  there. This protects long-term storage of private keys, but will not
  necessarily put all intermediate values and computations there.

Additionally, we try to keep user passwords in secure heap too.
This facility is only available since OpenSSL_1_1_0-pre1.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
Cc: Stefan Berger <stefanb@linux.ibm.com>
---
Change since v3:
- Undo secure heap handling from (file2bin and) calc_evm_hmac since this
  is debugging tool only. Add comment about this.
- Since there is only code removals and new comments I keep Reviewed-by
  tag.

 src/evmctl.c | 97 +++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 85 insertions(+), 12 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 5f7c2b8..7bd20f8 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;
@@ -1072,6 +1091,7 @@ static int cmd_setxattr_ima(struct command *cmd)
 
 #define MAX_KEY_SIZE 128
 
+/* This function is debugging tool and should not be used for real private data. */
 static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *hash)
 {
         const EVP_MD *md;
@@ -2596,15 +2616,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 +2660,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 +2670,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 +2691,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 +2700,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 +2742,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 +2909,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 +2919,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 v4] evmctl: Use secure heap for private keys and passwords

[Stefan Berger]

On 9/4/21 6:50 AM, Vitaly Chikunov wrote:
> After CRYPTO_secure_malloc_init OpenSSL will automatically store private
> keys in secure heap. OPENSSL_secure_malloc(3):
>
>    If a secure heap is used, then private key BIGNUM values are stored
>    there. This protects long-term storage of private keys, but will not
>    necessarily put all intermediate values and computations there.
>
> Additionally, we try to keep user passwords in secure heap too.
> This facility is only available since OpenSSL_1_1_0-pre1.
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
> Cc: Stefan Berger <stefanb@linux.ibm.com>
> ---
> Change since v3:
> - Undo secure heap handling from (file2bin and) calc_evm_hmac since this
>    is debugging tool only. Add comment about this.
> - Since there is only code removals and new comments I keep Reviewed-by
>    tag.
>
>   src/evmctl.c | 97 +++++++++++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 85 insertions(+), 12 deletions(-)
>
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 5f7c2b8..7bd20f8 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;
> @@ -1072,6 +1091,7 @@ static int cmd_setxattr_ima(struct command *cmd)
>   
>   #define MAX_KEY_SIZE 128
>   
> +/* This function is debugging tool and should not be used for real private data. */
>   static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *hash)
>   {
>           const EVP_MD *md;
> @@ -2596,15 +2616,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)
> +{


Mimi applied my patch that takes the password from an environment 
variable to the next-testing branch. The code there 
(imaevm_params.keypass  = getenv("..."))would have to change as well 
calling this function here. Even though the man page of getenv says that 
'The caller must take care not to modify this string, since that would 
change the environment of the process' we should be able to overwrite 
the env variable's password value just like here. Maybe for this purpose 
rename this function to dup_password() ?


> +	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 +2660,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 +2670,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 +2691,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 +2700,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.


Shouldn't the comparison then not include 0x10100000?

#if OPENSSL_VERSION_NUMBER >= 0x10100000



> +	 * '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 +2742,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 +2909,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 +2919,9 @@ int main(int argc, char *argv[])
>   		ENGINE_cleanup();
>   #endif
>   	}
> +#if OPENSSL_VERSION_NUMBER > 0x10100000
same comment here
> +	CRYPTO_secure_malloc_done();
> +#endif
>   	ERR_free_strings();
>   	EVP_cleanup();
>   	BIO_free(NULL);

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

[Vitaly Chikunov]

Stefan,

On Sat, Sep 04, 2021 at 09:10:50PM -0400, Stefan Berger wrote:
> 
> On 9/4/21 6:50 AM, Vitaly Chikunov wrote:
> > After CRYPTO_secure_malloc_init OpenSSL will automatically store private
> > keys in secure heap. OPENSSL_secure_malloc(3):
> > 
> >    If a secure heap is used, then private key BIGNUM values are stored
> >    there. This protects long-term storage of private keys, but will not
> >    necessarily put all intermediate values and computations there.
> > 
> > Additionally, we try to keep user passwords in secure heap too.
> > This facility is only available since OpenSSL_1_1_0-pre1.
> > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
> > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> > Change since v3:
> > - Undo secure heap handling from (file2bin and) calc_evm_hmac since this
> >    is debugging tool only. Add comment about this.
> > - Since there is only code removals and new comments I keep Reviewed-by
> >    tag.
> > 
> >   src/evmctl.c | 97 +++++++++++++++++++++++++++++++++++++++++++++-------
> >   1 file changed, 85 insertions(+), 12 deletions(-)
> > 
> > @@ -2596,15 +2616,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)
> > +{
> 
> 
> Mimi applied my patch that takes the password from an environment variable
> to the next-testing branch. The code there (imaevm_params.keypass  =
> getenv("..."))would have to change as well calling this function here. Even
> though the man page of getenv says that 'The caller must take care not to
> modify this string, since that would change the environment of the process'
> we should be able to overwrite the env variable's password value just like
> here. Maybe for this purpose rename this function to dup_password() ?

`/proc/<pid>/environ' is never world readable (unlike `cmdline'), so we
don't even need to overwrite it there. But, we can.

(Btw, I found the 'EVMCTL_KEY_PASSWORD' patch.)

> > @@ -2643,6 +2691,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 +2700,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.
> 
> 
> Shouldn't the comparison then not include 0x10100000?
> 
> #if OPENSSL_VERSION_NUMBER >= 0x10100000

The number is from `include/openssl/opensslv.h' from the time when
relevant commit is already applied. So, I use `>'. It seems good match
for OpenSSL_1_1_0-pre1 too:

  $ git grep OPENSSL_VERSION_NUMBER OpenSSL_1_1_0-pre1:include/openssl/opensslv.h
  OpenSSL_1_1_0-pre1:include/openssl/opensslv.h:# define OPENSSL_VERSION_NUMBER  0x10100001L

Thanks,

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

[Stefan Berger]

On 9/5/21 4:11 AM, Vitaly Chikunov wrote:
> Stefan,
>
> On Sat, Sep 04, 2021 at 09:10:50PM -0400, Stefan Berger wrote:
>> On 9/4/21 6:50 AM, Vitaly Chikunov wrote:
>>> After CRYPTO_secure_malloc_init OpenSSL will automatically store private
>>> keys in secure heap. OPENSSL_secure_malloc(3):
>>>
>>>     If a secure heap is used, then private key BIGNUM values are stored
>>>     there. This protects long-term storage of private keys, but will not
>>>     necessarily put all intermediate values and computations there.
>>>
>>> Additionally, we try to keep user passwords in secure heap too.
>>> This facility is only available since OpenSSL_1_1_0-pre1.
>>> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
>>> Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
>>> Cc: Stefan Berger <stefanb@linux.ibm.com>
>>> ---
>>> Change since v3:
>>> - Undo secure heap handling from (file2bin and) calc_evm_hmac since this
>>>     is debugging tool only. Add comment about this.
>>> - Since there is only code removals and new comments I keep Reviewed-by
>>>     tag.
>>>
>>>    src/evmctl.c | 97 +++++++++++++++++++++++++++++++++++++++++++++-------
>>>    1 file changed, 85 insertions(+), 12 deletions(-)
>>>
>>> @@ -2596,15 +2616,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)
>>> +{
>>
>> Mimi applied my patch that takes the password from an environment variable
>> to the next-testing branch. The code there (imaevm_params.keypass  =
>> getenv("..."))would have to change as well calling this function here. Even
>> though the man page of getenv says that 'The caller must take care not to
>> modify this string, since that would change the environment of the process'
>> we should be able to overwrite the env variable's password value just like
>> here. Maybe for this purpose rename this function to dup_password() ?
> `/proc/<pid>/environ' is never world readable (unlike `cmdline'), so we
> don't even need to overwrite it there. But, we can.

Or you could add an option for whether to overwrite the source...


>
> (Btw, I found the 'EVMCTL_KEY_PASSWORD' patch.)
>
>>> @@ -2643,6 +2691,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 +2700,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.
>>
>> Shouldn't the comparison then not include 0x10100000?
>>
>> #if OPENSSL_VERSION_NUMBER >= 0x10100000
> The number is from `include/openssl/opensslv.h' from the time when
> relevant commit is already applied. So, I use `>'. It seems good match
> for OpenSSL_1_1_0-pre1 too:
>
>    $ git grep OPENSSL_VERSION_NUMBER OpenSSL_1_1_0-pre1:include/openssl/opensslv.h
>    OpenSSL_1_1_0-pre1:include/openssl/opensslv.h:# define OPENSSL_VERSION_NUMBER  0x10100001L

You are right. I had checkout out this commit here:

commit abd30777cc72029e8a44e4b67201cae8ed3d19c1 (HEAD -> OpenSSL_1_1_0, 
tag: OpenSSL_1_1_0)
Author: Matt Caswell <matt@openssl.org>
Date:   Thu Aug 25 16:29:18 2016 +0100

     Prepare for 1.1.0 release

     Reviewed-by: Richard Levitte <levitte@openssl.org


It shows this here:

$ grep -r OPENSSL_VERSION_NUM ./include/
./include/openssl/crypto.h:#  define SSLEAY_VERSION_NUMBER 
OPENSSL_VERSION_NUMBER
./include/openssl/opensslv.h:# define OPENSSL_VERSION_NUMBER 0x1010000fL

There's an 'f' at the 'end'.

   Stefan


>
> Thanks,
>

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

[Mimi Zohar]

On Sat, 2021-09-04 at 13:50 +0300, Vitaly Chikunov wrote:
> After CRYPTO_secure_malloc_init OpenSSL will automatically store private
> keys in secure heap. OPENSSL_secure_malloc(3):
> 
>   If a secure heap is used, then private key BIGNUM values are stored
>   there. This protects long-term storage of private keys, but will not
>   necessarily put all intermediate values and computations there.
> 
> Additionally, we try to keep user passwords in secure heap too.
> This facility is only available since OpenSSL_1_1_0-pre1.
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
> Cc: Stefan Berger <stefanb@linux.ibm.com>

Thanks, it looks good!  Just a few questions inline below.

> ---
> Change since v3:
> - Undo secure heap handling from (file2bin and) calc_evm_hmac since this
>   is debugging tool only. Add comment about this.
> - Since there is only code removals and new comments I keep Reviewed-by
>   tag.
> 
>  src/evmctl.c | 97 +++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 85 insertions(+), 12 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 5f7c2b8..7bd20f8 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;
> @@ -1072,6 +1091,7 @@ static int cmd_setxattr_ima(struct command *cmd)
> 
>  #define MAX_KEY_SIZE 128
> 
> +/* This function is debugging tool and should not be used for real private data. */
>  static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *hash)
>  {
>          const EVP_MD *md;
> @@ -2596,15 +2616,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().
> + */

The reason given for copying the password to secure heap is technically
correct, but could we also include the reason that providing the
password on the command line is unsafe?  Perhaps a modified version of
the comment, below, should be included at the beginning of the function
comment.

> +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 +2660,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 +2670,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 +2691,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 +2700,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.
> +	 */

Assuming we aren't guaranteed that the heap size is allocated, should
we check (e.g. OPENSSL_secure_actual_size())?

> +	if (!CRYPTO_secure_malloc_init(8192, 64))
> +		log_err("CRYPTO_secure_malloc_init() failed\n");

Either change the test to "!= 1" or also log "and 2 if successful but
the heap could not be protected by memory mapping."

> +#endif
> +
>  	g_argv = argv;
>  	g_argc = argc;
> 
> @@ -2682,10 +2742,18 @@ int main(int argc, char *argv[])
>  			imaevm_params.hash_algo = optarg;
>  			break;
>  		case 'p':
> +			if (keypass)
> +				OPENSSL_secure_clear_free(keypass,
> +							  strlen(keypass));

Is this test needed in case the pasword is provided multiple times?  
If so, why not skip subsequent passwords?

>  			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 +2909,9 @@ int main(int argc, char *argv[])
>  		if (err < 0)
>  			err = 125;
>  	}
> -
> +quit:

Stefan's patch introduced "error:".

> +	if (keypass)
> +		OPENSSL_secure_clear_free(keypass, strlen(keypass));
>  	if (eng) {
>  		ENGINE_finish(eng);
>  		ENGINE_free(eng);

and stores the the engine in imaevm_params.

> @@ -2849,6 +2919,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);

What was the conclusion in terms of reading the password stored in the
environment variable?

thanks,

Mimi

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

[Vitaly Chikunov]

Mimi,

On Fri, Sep 10, 2021 at 10:55:04AM -0400, Mimi Zohar wrote:
> On Sat, 2021-09-04 at 13:50 +0300, Vitaly Chikunov wrote:
> > After CRYPTO_secure_malloc_init OpenSSL will automatically store private
> > keys in secure heap. OPENSSL_secure_malloc(3):
> > 
> >   If a secure heap is used, then private key BIGNUM values are stored
> >   there. This protects long-term storage of private keys, but will not
> >   necessarily put all intermediate values and computations there.
> > 
> > Additionally, we try to keep user passwords in secure heap too.
> > This facility is only available since OpenSSL_1_1_0-pre1.
> > 
> > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
> > Cc: Stefan Berger <stefanb@linux.ibm.com>
> 
> Thanks, it looks good!  Just a few questions inline below.
> 
> > ---
> > Change since v3:
> > - Undo secure heap handling from (file2bin and) calc_evm_hmac since this
> >   is debugging tool only. Add comment about this.
> > - Since there is only code removals and new comments I keep Reviewed-by
> >   tag.
> > 
> >  src/evmctl.c | 97 +++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 85 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/evmctl.c b/src/evmctl.c
> > index 5f7c2b8..7bd20f8 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;
> > @@ -1072,6 +1091,7 @@ static int cmd_setxattr_ima(struct command *cmd)
> > 
> >  #define MAX_KEY_SIZE 128
> > 
> > +/* This function is debugging tool and should not be used for real private data. */
> >  static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *hash)
> >  {
> >          const EVP_MD *md;
> > @@ -2596,15 +2616,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().
> > + */
> 
> The reason given for copying the password to secure heap is technically
> correct, but could we also include the reason that providing the
> password on the command line is unsafe?  Perhaps a modified version of
> the comment, below, should be included at the beginning of the function
> comment.
> 
> > +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 +2660,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 +2670,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 +2691,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 +2700,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.
> > +	 */
> 
> Assuming we aren't guaranteed that the heap size is allocated, should
> we check (e.g. OPENSSL_secure_actual_size())?

We guaranteed that secure heap is allocated by return value of
CRYPTO_secure_malloc_init.

OPENSSL_secure_actual_size tell if OPENSSL_secure_malloc is actually
provided bigger memory fragment than we requested (to reduce
fragmentation). Why care about it?

> 
> > +	if (!CRYPTO_secure_malloc_init(8192, 64))
> > +		log_err("CRYPTO_secure_malloc_init() failed\n");
> 
> Either change the test to "!= 1" or also log "and 2 if successful but
> the heap could not be protected by memory mapping."

Why should we care about implementation details of successful
allocation? If they don't think it's secure heap they should not return
success. And they say about its return value "and 2 if successful but",
so it's successful.


> 
> > +#endif
> > +
> >  	g_argv = argv;
> >  	g_argc = argc;
> > 
> > @@ -2682,10 +2742,18 @@ int main(int argc, char *argv[])
> >  			imaevm_params.hash_algo = optarg;
> >  			break;
> >  		case 'p':
> > +			if (keypass)
> > +				OPENSSL_secure_clear_free(keypass,
> > +							  strlen(keypass));
> 
> Is this test needed in case the pasword is provided multiple times?  

It does not care, it just frees memory so there is no leaks. (Perhaps.
it will be cleaned on CRYPTO_secure_malloc_done anyway). I can remove
this check, but why. Then we don't need to free() most of allocations,
because exit will care about all memory. But we free them anyway.

> If so, why not skip subsequent passwords?

Old code favored last specified password and I'm just adding secure heap to
it. I don't see that preference to first of last password is better for
UX.

> 
> >  			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 +2909,9 @@ int main(int argc, char *argv[])
> >  		if (err < 0)
> >  			err = 125;
> >  	}
> > -
> > +quit:
> 
> Stefan's patch introduced "error:".
> 
> > +	if (keypass)
> > +		OPENSSL_secure_clear_free(keypass, strlen(keypass));
> >  	if (eng) {
> >  		ENGINE_finish(eng);
> >  		ENGINE_free(eng);
> 
> and stores the the engine in imaevm_params.

OK I will wait when his patches will appear in git, so we do not collide
with each other.

> 
> > @@ -2849,6 +2919,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);
> 
> What was the conclusion in terms of reading the password stored in the
> environment variable?

I will add handling for it similar to password from optarg.

Thanks,

> 
> thanks,
> 
> Mimi

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

[Mimi Zohar]

Hi Vitaly, Stefan,

On Fri, 2021-09-10 at 10:55 -0400, Mimi Zohar wrote:
> What was the conclusion in terms of reading the password stored in the
> environment variable?

If this is an issue, perhaps call the equivalent of optarg_password()
to store the environment variable in secure heap memory.

Something like:
	imaevm_params.keypass =
optarg_password(getenv("EVMCTL_KEY_PASSWORD"));

optarg_password() would then become a wrapper around the new function.

thanks,

Mimi

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

[Mimi Zohar]

Hi Vitaly,

On Fri, 2021-09-10 at 21:13 +0300, Vitaly Chikunov wrote:
> Mimi,
> 
> On Fri, Sep 10, 2021 at 10:55:04AM -0400, Mimi Zohar wrote:
> > On Sat, 2021-09-04 at 13:50 +0300, Vitaly Chikunov wrote:
> > > After CRYPTO_secure_malloc_init OpenSSL will automatically store private
> > > keys in secure heap. OPENSSL_secure_malloc(3):
> > > 
> > >   If a secure heap is used, then private key BIGNUM values are stored
> > >   there. This protects long-term storage of private keys, but will not
> > >   necessarily put all intermediate values and computations there.
> > > 
> > > Additionally, we try to keep user passwords in secure heap too.
> > > This facility is only available since OpenSSL_1_1_0-pre1.
> > > 
> > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
> > > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > 
> > Thanks, it looks good!  Just a few questions inline below.
> > 
> > > ---
> > > Change since v3:
> > > - Undo secure heap handling from (file2bin and) calc_evm_hmac since this
> > >   is debugging tool only. Add comment about this.
> > > - Since there is only code removals and new comments I keep Reviewed-by
> > >   tag.
> > > 
> > >  src/evmctl.c | 97 +++++++++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 85 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/src/evmctl.c b/src/evmctl.c
> > > index 5f7c2b8..7bd20f8 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;
> > > @@ -1072,6 +1091,7 @@ static int cmd_setxattr_ima(struct command *cmd)
> > > 
> > >  #define MAX_KEY_SIZE 128
> > > 
> > > +/* This function is debugging tool and should not be used for real private data. */
> > >  static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *hash)
> > >  {
> > >          const EVP_MD *md;
> > > @@ -2596,15 +2616,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().
> > > + */
> > 
> > The reason given for copying the password to secure heap is technically
> > correct, but could we also include the reason that providing the
> > password on the command line is unsafe?  Perhaps a modified version of
> > the comment, below, should be included at the beginning of the function
> > comment.
> > 
> > > +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 +2660,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 +2670,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 +2691,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 +2700,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.
> > > +	 */
> > 
> > Assuming we aren't guaranteed that the heap size is allocated, should
> > we check (e.g. OPENSSL_secure_actual_size())?
> 
> We guaranteed that secure heap is allocated by return value of
> CRYPTO_secure_malloc_init.
> 
> OPENSSL_secure_actual_size tell if OPENSSL_secure_malloc is actually
> provided bigger memory fragment than we requested (to reduce
> fragmentation). Why care about it?

We wouldn't care, assuming we're guaranteed the heap size requested was
allocated and not the minimum heap size.  Hm, wondering what would be
the purpose of the minimum heap size ...

> 
> > 
> > > +	if (!CRYPTO_secure_malloc_init(8192, 64))
> > > +		log_err("CRYPTO_secure_malloc_init() failed\n");
> > 
> > Either change the test to "!= 1" or also log "and 2 if successful but
> > the heap could not be protected by memory mapping."
> 
> Why should we care about implementation details of successful
> allocation? If they don't think it's secure heap they should not return
> success. And they say about its return value "and 2 if successful but",
> so it's successful.

If you're correct, then there is no reason ever for returning 2. 
Without documentation or digging into OpenSSL, this makes no sense to
me.  Perhaps "secure heap" has some kernel dependency, requires HW, or
something else entirely.  Not that there is much we could or would do,
but simply ignoring it just seems wrong.

> 
> 
> > 
> > > +#endif
> > > +
> > >  	g_argv = argv;
> > >  	g_argc = argc;
> > > 
> > > @@ -2682,10 +2742,18 @@ int main(int argc, char *argv[])
> > >  			imaevm_params.hash_algo = optarg;
> > >  			break;
> > >  		case 'p':
> > > +			if (keypass)
> > > +				OPENSSL_secure_clear_free(keypass,
> > > +							  strlen(keypass));
> > 
> > Is this test needed in case the pasword is provided multiple times?  
> 
> It does not care, it just frees memory so there is no leaks. (Perhaps.
> it will be cleaned on CRYPTO_secure_malloc_done anyway). I can remove
> this check, but why. Then we don't need to free() most of allocations,
> because exit will care about all memory. But we free them anyway.
> 
> > If so, why not skip subsequent passwords?
> 
> Old code favored last specified password and I'm just adding secure heap to
> it. I don't see that preference to first of last password is better for
> UX.

Ok

> > 
> > >  			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 +2909,9 @@ int main(int argc, char *argv[])
> > >  		if (err < 0)
> > >  			err = 125;
> > >  	}
> > > -
> > > +quit:
> > 
> > Stefan's patch introduced "error:".
> > 
> > > +	if (keypass)
> > > +		OPENSSL_secure_clear_free(keypass, strlen(keypass));
> > >  	if (eng) {
> > >  		ENGINE_finish(eng);
> > >  		ENGINE_free(eng);
> > 
> > and stores the the engine in imaevm_params.
> 
> OK I will wait when his patches will appear in git, so we do not collide
> with each other.
> 
> > 
> > > @@ -2849,6 +2919,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);
> > 
> > What was the conclusion in terms of reading the password stored in the
> > environment variable?
> 
> I will add handling for it similar to password from optarg.

thanks,

Mimi

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

[Vitaly Chikunov]

Mimi,

On Mon, Sep 13, 2021 at 01:51:58PM -0400, Mimi Zohar wrote:
> > > Assuming we aren't guaranteed that the heap size is allocated, should
> > > we check (e.g. OPENSSL_secure_actual_size())?
> > 
> > We guaranteed that secure heap is allocated by return value of
> > CRYPTO_secure_malloc_init.
> > 
> > OPENSSL_secure_actual_size tell if OPENSSL_secure_malloc is actually
> > provided bigger memory fragment than we requested (to reduce
> > fragmentation). Why care about it?
> 
> We wouldn't care, assuming we're guaranteed the heap size requested was
> allocated and not the minimum heap size.  Hm, wondering what would be
> the purpose of the minimum heap size ...

Perhaps, just to control heap fragmentation.

> > > > +	if (!CRYPTO_secure_malloc_init(8192, 64))
> > > > +		log_err("CRYPTO_secure_malloc_init() failed\n");
> > > 
> > > Either change the test to "!= 1" or also log "and 2 if successful but
> > > the heap could not be protected by memory mapping."
> > 
> > Why should we care about implementation details of successful
> > allocation? If they don't think it's secure heap they should not return
> > success. And they say about its return value "and 2 if successful but",
> > so it's successful.
> 
> If you're correct, then there is no reason ever for returning 2. 
> Without documentation or digging into OpenSSL, this makes no sense to
> me.  Perhaps "secure heap" has some kernel dependency, requires HW, or
> something else entirely.  Not that there is much we could or would do,
> but simply ignoring it just seems wrong.

I looked at CRYPTO_secure_malloc_init and it seems they return 2 if
mlock/madvise is failed. I think it's major enough to report it, so I
will add it.

Thanks,