[v1,0/5] ima-evm-utils: Assorted fixes and improvements
mbox series

Message ID 20190707234837.4866-1-vt@altlinux.org
Headers show
Series
  • ima-evm-utils: Assorted fixes and improvements
Related show

Message

Vitaly Chikunov July 7, 2019, 11:48 p.m. UTC
There is small fixes and improvements to ima-evm-utils.
Tested on x86_64.

Vitaly Chikunov (5):
  ima-evm-utils: Fix EVP_MD_CTX leak in ima_calc_hash
  ima-evm-utils: Fix memory leak in init_public_keys
  ima-evm-utils: Preload public keys for ima_verify
  ima-evm-utils: Allow multiple files in ima_verify
  ima-evm-utils: Fix clang warning about possible unaligned pointer for
    hdr->keyid

 src/evmctl.c    | 11 ++++++++---
 src/libimaevm.c | 38 ++++++++++++++++++++++++++------------
 2 files changed, 34 insertions(+), 15 deletions(-)

Comments

Mimi Zohar July 8, 2019, 3:30 p.m. UTC | #1
[Cc'ing Roberto, Petr, Thiago, Prakhar]

Hi Vitaly,

On Mon, 2019-07-08 at 02:48 +0300, Vitaly Chikunov wrote:
> There is small fixes and improvements to ima-evm-utils.
> Tested on x86_64.
> 
> Vitaly Chikunov (5):
>   ima-evm-utils: Fix EVP_MD_CTX leak in ima_calc_hash
>   ima-evm-utils: Fix memory leak in init_public_keys
>   ima-evm-utils: Preload public keys for ima_verify
>   ima-evm-utils: Allow multiple files in ima_verify
>   ima-evm-utils: Fix clang warning about possible unaligned pointer for
>     hdr->keyid
> 
>  src/evmctl.c    | 11 ++++++++---
>  src/libimaevm.c | 38 ++++++++++++++++++++++++++------------
>  2 files changed, 34 insertions(+), 15 deletions(-)

Thanks, this patch set looks good.  These patches, the "ima-evm-utils: 
Convert v2 signatures from RSA to EVP_PKEY AP", and the two patches I
posted today are now in #next, but I'd really appreciate some
additional Review's/Tested's on these patches.

Now that we're including ALL the kernel exported hash_info algorithms,
a colleague suggested defining a list of deprecated hash algorithms.
 Instead of preventing the usage of these deprecated hash algorithms,
initially I would start out with a warning.  It would be helpful to
indicate which standard deprecated the hash algorithm and year.  At
some point, we might want to prevent their usage in signing files, but
not verifying file signatures.

evmctl "ima_measurement" doesn't support custom template definitions.
Also missing is support for verifying the "ima-buf" kexec command boot
command line and the "ima-modsig" template appended signature.

David Jacobson started writing a regression framework and posted a v2
version.  I'd really appreciate help with cleaning up that code. 

Any other comments/suggestions/ideas?

thanks,

Mimi
Vitaly Chikunov July 9, 2019, 3:43 p.m. UTC | #2
Mimi,

On Mon, Jul 08, 2019 at 11:30:50AM -0400, Mimi Zohar wrote:
> [Cc'ing Roberto, Petr, Thiago, Prakhar]
> Now that we're including ALL the kernel exported hash_info algorithms,
> a colleague suggested defining a list of deprecated hash algorithms.
>  Instead of preventing the usage of these deprecated hash algorithms,
> initially I would start out with a warning.  It would be helpful to
> indicate which standard deprecated the hash algorithm and year.  At
> some point, we might want to prevent their usage in signing files, but
> not verifying file signatures.

I think this is not a problem, because user explicitly states which hash
algorithm he wants to use. Except for SHA1, which is also silent
fallback algorithm. I think this fallback mechanism should be removed.

Also, return values of sign_hash/ima_calc_hash/etc are not defined
clearly and callers have weird checks such as `if (len <= 1)`. I think
this should be conceptually simplified and made them `return -1` on any
error.


> evmctl "ima_measurement" doesn't support custom template definitions.
> Also missing is support for verifying the "ima-buf" kexec command boot
> command line and the "ima-modsig" template appended signature.
> 
> David Jacobson started writing a regression framework and posted a v2
> version.  I'd really appreciate help with cleaning up that code. 

Maybe tests should be integrated into ima-evm-utils too.

Thanks,
Mimi Zohar July 11, 2019, 7:25 p.m. UTC | #3
On Tue, 2019-07-09 at 18:43 +0300, Vitaly Chikunov wrote:
> On Mon, Jul 08, 2019 at 11:30:50AM -0400, Mimi Zohar wrote:
> > [Cc'ing Roberto, Petr, Thiago, Prakhar]
> > Now that we're including ALL the kernel exported hash_info algorithms,
> > a colleague suggested defining a list of deprecated hash algorithms.
> >  Instead of preventing the usage of these deprecated hash algorithms,
> > initially I would start out with a warning.  It would be helpful to
> > indicate which standard deprecated the hash algorithm and year.  At
> > some point, we might want to prevent their usage in signing files, but
> > not verifying file signatures.
> 
> I think this is not a problem, because user explicitly states which hash
> algorithm he wants to use. Except for SHA1, which is also silent
> fallback algorithm. I think this fallback mechanism should be removed.

I don't see a problem with informing whoever is using ima-evm-utils,
that the requested hash algorithm has been deprecated.  Just as NIST
has deprecated the use of sha1 for most usecases, certain versions of
the gost standard have also been deprecated.

Someone verifying the measurement list or the filesystem signatures
should be informed that although the verification succeeded, or not,
some of the signatures verified were based on deprecated hash
algorithms.  Maybe something along the lines of "<algorithm> hash
algorithm was deprecated (<year> - <standard>)"?  In verbose mode, the
message could include the filename.

The EVM hmac is still sha1 based, but that shouldn't affect ima-evm-
utils.  So yes we should explicitly require a valid hash algorithm and
not fall back to using sha1.

> 
> Also, return values of sign_hash/ima_calc_hash/etc are not defined
> clearly and callers have weird checks such as `if (len <= 1)`. I think
> this should be conceptually simplified and made them `return -1` on any
> error.

Agreed.  Mostly these functions are returning "-1" on error.  There
are a few places returning 1 instead.

> 
> 
> > evmctl "ima_measurement" doesn't support custom template definitions.
> > Also missing is support for verifying the "ima-buf" kexec command boot
> > command line and the "ima-modsig" template appended signature.
> > 
> > David Jacobson started writing a regression framework and posted a v2
> > version.  I'd really appreciate help with cleaning up that code. 
> 
> Maybe tests should be integrated into ima-evm-utils too.

Including regression tests in ima-evm-utils is the plan. :)

Mimi
Petr Vorel July 17, 2019, 4:38 p.m. UTC | #4
Hi Mimi, Vitaly,

> Mimi,

> On Mon, Jul 08, 2019 at 11:30:50AM -0400, Mimi Zohar wrote:
> > [Cc'ing Roberto, Petr, Thiago, Prakhar]
> > Now that we're including ALL the kernel exported hash_info algorithms,
> > a colleague suggested defining a list of deprecated hash algorithms.
> >  Instead of preventing the usage of these deprecated hash algorithms,
> > initially I would start out with a warning.  It would be helpful to
> > indicate which standard deprecated the hash algorithm and year.  At
> > some point, we might want to prevent their usage in signing files, but
> > not verifying file signatures.
Looks useful to me.

> I think this is not a problem, because user explicitly states which hash
> algorithm he wants to use. Except for SHA1, which is also silent
> fallback algorithm. I think this fallback mechanism should be removed.
Agree with removing fallback mechanism.

> Also, return values of sign_hash/ima_calc_hash/etc are not defined
> clearly and callers have weird checks such as `if (len <= 1)`. I think
> this should be conceptually simplified and made them `return -1` on any
> error.


> > evmctl "ima_measurement" doesn't support custom template definitions.
> > Also missing is support for verifying the "ima-buf" kexec command boot
> > command line and the "ima-modsig" template appended signature.

> > David Jacobson started writing a regression framework and posted a v2
> > version.  I'd really appreciate help with cleaning up that code. 

> Maybe tests should be integrated into ima-evm-utils too.
https://patchwork.kernel.org/project/linux-integrity/list/?series=95303

> Thanks,

Kind regards,
Petr