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 <[email protected]>
> > ---
> > 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;
