Hi Petr,
On Tue, 2022-08-30 at 13:55 +0200, Petr Vorel wrote:
> > The original IMA file signatures were based on a SHA1 hash. Kernel
> > support for other hash algorithms was subsequently upstreamed. Deprecate
> > "--rsa" support.
>
> > Define "--enable-sigv1" option to configure signature v1 support.
>
> LGTM, few minor comments below.
>
> Reviewed-by: Petr Vorel <pvorel@xxxxxxx>
Thank you for all the reviews!
>
> ...
> > +++ b/configure.ac
> ...
>
> > static int cmd_convert(struct command *cmd)
> > {
> > +#if CONFIG_SIGV1
> > char *inkey;
> > unsigned char _pub[1024], *pub = _pub;
> > int len, err = 0;
> > @@ -1033,6 +1033,8 @@ static int cmd_convert(struct command *cmd)
>
> > RSA_free(key);
> > return err;
> > +#endif
> > + return 77;
> What is this this magic number? EBADFD?
> Well, git grep shows many places with 77, so it's just a tip for next cleanup :).
SKIP is defined as 77 in the tests/ directory. Using 77 in src/*.c is
incorrect. v2 ifdefs all of cmd_convert(), so this is going away.
I'll remove the other occurance of 77 in the src/ directory.
>
> ...
> > log_info("Importing public key %s from file %s into keyring %d\n", name, inkey, id);
> > @@ -2598,7 +2605,8 @@ static void usage(void)
> > " -d, --imahash make IMA hash\n"
> > " -f, --sigfile store IMA signature in .sig file instead of xattr\n"
> > " --xattr-user store xattrs in user namespace (for testing purposes)\n"
> > - " --rsa use RSA key type and signing scheme v1\n"
> > +
> nit: was this blank line intentional?
> > + " --rsa use RSA key type and signing scheme v1 (deprecated)\n"
> > " -k, --key path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem)\n"
> > " or a pkcs11 URI\n"
> > " --keyid n overwrite signature keyid with a 32-bit value in hex (for signing)\n"
> > @@ -2637,8 +2645,8 @@ static void usage(void)
> > struct command cmds[] = {
> > {"--version", NULL, 0, ""},
> > {"help", cmd_help, 0, "<command>"},
> > - {"import", cmd_import, 0, "[--rsa] pubkey keyring", "Import public key into the keyring.\n"},
> > - {"convert", cmd_convert, 0, "key", "convert public key into the keyring.\n"},
> > + {"import", cmd_import, 0, "[--rsa] pubkey keyring", "Import public key into the keyring. (deprecated)\n"},
> > + {"convert", cmd_convert, 0, "key", "convert public key into the keyring. (deprecated)\n"},
> > {"sign", cmd_sign_evm, 0, "[-r] [--imahash | --imasig ] [--key key] [--pass [password] file", "Sign file metadata.\n"},
> > {"verify", cmd_verify_evm, 0, "file", "Verify EVM signature (for debugging).\n"},
> > {"ima_sign", cmd_sign_ima, 0, "[--sigfile] [--key key] [--pass [password] file", "Make file content signature.\n"},
> ...
> > +++ b/src/libimaevm.c
> ...
>
> > +#if CONFIG_SIGV1
> > static RSA *read_priv_key(const char *keyfile, const char *keypass)
> > {
> > + RSA *key = NULL;
> nit: NULL is safe, I wonder if it is necessary (was needed before).
Looking at this again, it isn't needed. On failure to set either pkey
or key, NULL is returned.
> > EVP_PKEY *pkey;
> > - RSA *key;
>
> > pkey = read_priv_pkey(keyfile, keypass);
> > if (!pkey)
> > @@ -1018,10 +1034,12 @@ static int get_hash_algo_v1(const char *algo)
>
> > return -1;
> > }
> > +#endif
--
thanks,
Mimi
