diff mbox series

[ima-evm-utils,12/12] Define and use a file specific "keypass" variable

Message ID 20231119165043.46960-13-zohar@linux.ibm.com (mailing list archive)
State New
Headers show
Series Address non concurrency-safe libimaevm global variables | expand

Commit Message

Mimi Zohar Nov. 19, 2023, 4:50 p.m. UTC 
Instead of relying on the "imaevm_params.keypass" global variable, which
is not concurrency-safe, add keypass as a parameter to the static library
functions definitions.  Update function callers.

To avoid library incompatablity, don't remove imaevm_params.keypass
variable.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c    |  9 +++++----
 src/libimaevm.c | 17 ++++++++---------
 2 files changed, 13 insertions(+), 13 deletions(-)

Comments

Stefan Berger Nov. 22, 2023, 2:22 p.m. UTC | #1 
On 11/19/23 11:50, Mimi Zohar wrote:
> Instead of relying on the "imaevm_params.keypass" global variable, which
> is not concurrency-safe, add keypass as a parameter to the static library
> functions definitions.  Update function callers.
> 
> To avoid library incompatablity, don't remove imaevm_params.keypass
> variable.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/evmctl.c    |  9 +++++----
>   src/libimaevm.c | 17 ++++++++---------
>   2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index b802eeb1bf15..6d6160159a1f 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -141,6 +141,7 @@ static bool evm_portable;
>   static bool veritysig;
>   static bool hwtpm;
>   static char *hash_algo;
> +static char *keypass;
>   
>   #define HMAC_FLAG_NO_UUID	0x0001
>   #define HMAC_FLAG_CAPS_SET	0x0002
> @@ -3082,9 +3083,9 @@ int main(int argc, char *argv[])
>   			break;
>   		case 'p':
>   			if (optarg)
> -				imaevm_params.keypass = optarg;
> +				keypass = optarg;
>   			else
> -				imaevm_params.keypass = get_password();
> +				keypass = get_password();
>   			break;
>   		case 'f':
>   			sigfile = 1;
> @@ -3226,8 +3227,8 @@ int main(int argc, char *argv[])
>   		}
>   	}
>   
> -	if (!imaevm_params.keypass)
> -		imaevm_params.keypass = getenv("EVMCTL_KEY_PASSWORD");
> +	if (!keypass)
> +		keypass = getenv("EVMCTL_KEY_PASSWORD");
>   
>   	if (imaevm_params.keyfile != NULL &&
>   	    imaevm_params.eng == NULL &&


The problem at this point in evmctl is that keypass is never passed to 
anywhere. You have to pass it to sign_hash().

> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 18b6a6f27237..10ec847da08a 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -1124,7 +1124,8 @@ static int get_hash_algo_v1(const char *algo)
>   }
>   
>   static int sign_hash_v1(const char *hashalgo, const unsigned char *hash,
> -			int size, const char *keyfile, unsigned char *sig)
> +			int size, const char *keyfile, const char *keypass,
> +			unsigned char *sig)
>   {
>   	int len = -1, hashalgo_idx;
>   	SHA_CTX ctx;
> @@ -1158,7 +1159,7 @@ static int sign_hash_v1(const char *hashalgo, const unsigned char *hash,
>   	log_info("hash(%s): ", hashalgo);
>   	log_dump(hash, size);
>   
> -	key = read_priv_key(keyfile, imaevm_params.keypass);
> +	key = read_priv_key(keyfile, keypass);
>   	if (!key)
>   		return -1;
>   
> @@ -1211,7 +1212,8 @@ out:
>    * Return: -1 signing error, >0 length of signature
>    */
>   static int sign_hash_v2(const char *algo, const unsigned char *hash,
> -			int size, const char *keyfile, unsigned char *sig)
> +			int size, const char *keyfile, const char *keypass,
> +			unsigned char *sig)
>   {
>   	struct signature_v2_hdr *hdr;
>   	int len = -1;
> @@ -1246,7 +1248,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
>   	log_info("hash(%s): ", algo);
>   	log_dump(hash, size);
>   
> -	pkey = read_priv_pkey(keyfile, imaevm_params.keypass);
> +	pkey = read_priv_pkey(keyfile, keypass);
>   	if (!pkey)
>   		return -1;
>   
> @@ -1316,14 +1318,11 @@ err:
>   
>   int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig)
>   {
> -	if (keypass)
> -		imaevm_params.keypass = keypass;
> -

I hope all library callers, other than evmctl, passed the keypass in 
already, otherwise they could have set the password via 
imaevm_params.keypass global and passed in NULL and for them this will 
be a behavioral change. Anyway, you have no other choice to get rid of 
imaevm_params as much as possible, so I guess it's ok.

It looks like at this point imaevm_params.keypass doesn't have a single 
user anymore so you could rename it to 'unused1' or so in the stucture 
since it will have zero effect for anyone to set it.

>   	if (imaevm_params.x509)
> -		return sign_hash_v2(hashalgo, hash, size, keyfile, sig);
> +		return sign_hash_v2(hashalgo, hash, size, keyfile, keypass, sig);
>   #if CONFIG_SIGV1
>   	else
> -		return sign_hash_v1(hashalgo, hash, size, keyfile, sig);
> +		return sign_hash_v1(hashalgo, hash, size, keyfile, keypass, sig);
>   #endif
>   	log_info("Signature version 1 deprecated.");
>   	return -1;
Mimi Zohar Nov. 29, 2023, 4:07 p.m. UTC | #2 
On Wed, 2023-11-22 at 09:22 -0500, Stefan Berger wrote:
> 
> On 11/19/23 11:50, Mimi Zohar wrote:
> > Instead of relying on the "imaevm_params.keypass" global variable, which
> > is not concurrency-safe, add keypass as a parameter to the static library
> > functions definitions.  Update function callers.
> > 
> > To avoid library incompatablity, don't remove imaevm_params.keypass
> > variable.
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >   src/evmctl.c    |  9 +++++----
> >   src/libimaevm.c | 17 ++++++++---------
> >   2 files changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/src/evmctl.c b/src/evmctl.c
> > index b802eeb1bf15..6d6160159a1f 100644
> > --- a/src/evmctl.c
> > +++ b/src/evmctl.c
> > @@ -141,6 +141,7 @@ static bool evm_portable;
> >   static bool veritysig;
> >   static bool hwtpm;
> >   static char *hash_algo;
> > +static char *keypass;
> >   
> >   #define HMAC_FLAG_NO_UUID	0x0001
> >   #define HMAC_FLAG_CAPS_SET	0x0002
> > @@ -3082,9 +3083,9 @@ int main(int argc, char *argv[])
> >   			break;
> >   		case 'p':
> >   			if (optarg)
> > -				imaevm_params.keypass = optarg;
> > +				keypass = optarg;
> >   			else
> > -				imaevm_params.keypass = get_password();
> > +				keypass = get_password();
> >   			break;
> >   		case 'f':
> >   			sigfile = 1;
> > @@ -3226,8 +3227,8 @@ int main(int argc, char *argv[])
> >   		}
> >   	}
> >   
> > -	if (!imaevm_params.keypass)
> > -		imaevm_params.keypass = getenv("EVMCTL_KEY_PASSWORD");
> > +	if (!keypass)
> > +		keypass = getenv("EVMCTL_KEY_PASSWORD");
> >   
> >   	if (imaevm_params.keyfile != NULL &&
> >   	    imaevm_params.eng == NULL &&
> 
> 
> The problem at this point in evmctl is that keypass is never passed to 
> anywhere. You have to pass it to sign_hash().

Thanks for catching this.  Part of the patch was missing was obviously
missing.

> 
> > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > index 18b6a6f27237..10ec847da08a 100644
> > --- a/src/libimaevm.c
> > +++ b/src/libimaevm.c
> > @@ -1124,7 +1124,8 @@ static int get_hash_algo_v1(const char *algo)
> >   }
> >   
> >   static int sign_hash_v1(const char *hashalgo, const unsigned char *hash,
> > -			int size, const char *keyfile, unsigned char *sig)
> > +			int size, const char *keyfile, const char *keypass,
> > +			unsigned char *sig)
> >   {
> >   	int len = -1, hashalgo_idx;
> >   	SHA_CTX ctx;
> > @@ -1158,7 +1159,7 @@ static int sign_hash_v1(const char *hashalgo, const unsigned char *hash,
> >   	log_info("hash(%s): ", hashalgo);
> >   	log_dump(hash, size);
> >   
> > -	key = read_priv_key(keyfile, imaevm_params.keypass);
> > +	key = read_priv_key(keyfile, keypass);
> >   	if (!key)
> >   		return -1;
> >   
> > @@ -1211,7 +1212,8 @@ out:
> >    * Return: -1 signing error, >0 length of signature
> >    */
> >   static int sign_hash_v2(const char *algo, const unsigned char *hash,
> > -			int size, const char *keyfile, unsigned char *sig)
> > +			int size, const char *keyfile, const char *keypass,
> > +			unsigned char *sig)
> >   {
> >   	struct signature_v2_hdr *hdr;
> >   	int len = -1;
> > @@ -1246,7 +1248,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
> >   	log_info("hash(%s): ", algo);
> >   	log_dump(hash, size);
> >   
> > -	pkey = read_priv_pkey(keyfile, imaevm_params.keypass);
> > +	pkey = read_priv_pkey(keyfile, keypass);
> >   	if (!pkey)
> >   		return -1;
> >   
> > @@ -1316,14 +1318,11 @@ err:
> >   
> >   int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig)
> >   {
> > -	if (keypass)
> > -		imaevm_params.keypass = keypass;
> > -
> 
> I hope all library callers, other than evmctl, passed the keypass in 
> already, otherwise they could have set the password via 
> imaevm_params.keypass global and passed in NULL and for them this will 
> be a behavioral change. Anyway, you have no other choice to get rid of 
> imaevm_params as much as possible, so I guess it's ok.

For now we could invert the test and do exactly the opposite, like
this:

+       if (!keypass)   /* Avoid breaking existing libimaevm usage */
+               keypass = imaevm_params.keypass;
+

thanks,

Mimi

> 
> It looks like at this point imaevm_params.keypass doesn't have a single 
> user anymore so you could rename it to 'unused1' or so in the stucture 
> since it will have zero effect for anyone to set it.
> 
> >   	if (imaevm_params.x509)
> > -		return sign_hash_v2(hashalgo, hash, size, keyfile, sig);
> > +		return sign_hash_v2(hashalgo, hash, size, keyfile, keypass, sig);
> >   #if CONFIG_SIGV1
> >   	else
> > -		return sign_hash_v1(hashalgo, hash, size, keyfile, sig);
> > +		return sign_hash_v1(hashalgo, hash, size, keyfile, keypass, sig);
> >   #endif
> >   	log_info("Signature version 1 deprecated.");
> >   	return -1;
diff mbox series

Patch

diff --git a/src/evmctl.c b/src/evmctl.c
index b802eeb1bf15..6d6160159a1f 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -141,6 +141,7 @@  static bool evm_portable;
 static bool veritysig;
 static bool hwtpm;
 static char *hash_algo;
+static char *keypass;
 
 #define HMAC_FLAG_NO_UUID	0x0001
 #define HMAC_FLAG_CAPS_SET	0x0002
@@ -3082,9 +3083,9 @@  int main(int argc, char *argv[])
 			break;
 		case 'p':
 			if (optarg)
-				imaevm_params.keypass = optarg;
+				keypass = optarg;
 			else
-				imaevm_params.keypass = get_password();
+				keypass = get_password();
 			break;
 		case 'f':
 			sigfile = 1;
@@ -3226,8 +3227,8 @@  int main(int argc, char *argv[])
 		}
 	}
 
-	if (!imaevm_params.keypass)
-		imaevm_params.keypass = getenv("EVMCTL_KEY_PASSWORD");
+	if (!keypass)
+		keypass = getenv("EVMCTL_KEY_PASSWORD");
 
 	if (imaevm_params.keyfile != NULL &&
 	    imaevm_params.eng == NULL &&
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 18b6a6f27237..10ec847da08a 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -1124,7 +1124,8 @@  static int get_hash_algo_v1(const char *algo)
 }
 
 static int sign_hash_v1(const char *hashalgo, const unsigned char *hash,
-			int size, const char *keyfile, unsigned char *sig)
+			int size, const char *keyfile, const char *keypass,
+			unsigned char *sig)
 {
 	int len = -1, hashalgo_idx;
 	SHA_CTX ctx;
@@ -1158,7 +1159,7 @@  static int sign_hash_v1(const char *hashalgo, const unsigned char *hash,
 	log_info("hash(%s): ", hashalgo);
 	log_dump(hash, size);
 
-	key = read_priv_key(keyfile, imaevm_params.keypass);
+	key = read_priv_key(keyfile, keypass);
 	if (!key)
 		return -1;
 
@@ -1211,7 +1212,8 @@  out:
  * Return: -1 signing error, >0 length of signature
  */
 static int sign_hash_v2(const char *algo, const unsigned char *hash,
-			int size, const char *keyfile, unsigned char *sig)
+			int size, const char *keyfile, const char *keypass,
+			unsigned char *sig)
 {
 	struct signature_v2_hdr *hdr;
 	int len = -1;
@@ -1246,7 +1248,7 @@  static int sign_hash_v2(const char *algo, const unsigned char *hash,
 	log_info("hash(%s): ", algo);
 	log_dump(hash, size);
 
-	pkey = read_priv_pkey(keyfile, imaevm_params.keypass);
+	pkey = read_priv_pkey(keyfile, keypass);
 	if (!pkey)
 		return -1;
 
@@ -1316,14 +1318,11 @@  err:
 
 int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig)
 {
-	if (keypass)
-		imaevm_params.keypass = keypass;
-
 	if (imaevm_params.x509)
-		return sign_hash_v2(hashalgo, hash, size, keyfile, sig);
+		return sign_hash_v2(hashalgo, hash, size, keyfile, keypass, sig);
 #if CONFIG_SIGV1
 	else
-		return sign_hash_v1(hashalgo, hash, size, keyfile, sig);
+		return sign_hash_v1(hashalgo, hash, size, keyfile, keypass, sig);
 #endif
 	log_info("Signature version 1 deprecated.");
 	return -1;