[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-integrity
Subject:    Re: [PATCH v2] ima-evm-utils: Add some tests for evmctl
From:       Vitaly Chikunov <vt () altlinux ! org>
Date:       2019-07-29 4:09:57
Message-ID: 20190729040956.bbprdbpas46mjcgh () altlinux ! org
[Download RAW message or body]

Mimi,

On Sun, Jul 28, 2019 at 09:49:04PM -0400, Mimi Zohar wrote:
> On Mon, 2019-07-29 at 02:40 +0300, Vitaly Chikunov wrote:
> > > The functions "pos" and "neg" are written very concisely, but they are
> > > part of a common set of functions, which are the crux of the tests
> > > scripts. šI'm really hesitant about having common functions that
> > > execute any command passed to it, without any form of verification.
> > 
> > What verification and why?
> 
> Even though the tests are not running as root, it's still executing
> "$@", whatever that might be. šFor ima_hash.test, the first argument
> is "check".

I believe this will not add any security at all. As these are purely
internal functions. If somebody can call pos/neg they are already able
to call just anything they want. This is like protecting awk from
running something somehow not verified.

> > > > +_enable_gost_engine() {
> > > > +  # Do not enable if it's already working (enabled by user)
> > > > +  if ! openssl md_gost12_256 /dev/null >/dev/null 2>&1 \
> > > > +    && openssl engine gost >/dev/null 2>&1; then
> > > > +    ENGINE=gost
> > > > +  fi
> > > > +}
> > > 
> > > With gost provided as an Openssl engine, is it possible to be able to
> > > execute the first command without the gost engine enabled? šWith
> > > commit 782224f33cd7 ("ima-evm-utils: Rework openssl init"),
> > 
> > I don't understand question. What is 'first command'? `openssl
> > md_gost12_256` will not work if gost-engine is not configured somehow.
> 
> Exactly. š"openssl md_gost12_256 /dev/null" (returns 0, but is
> negated) will succeed only if the engine is enabled. šThe "openssl
> engine gost" test will never fail.

It can return non 0 and check looks correct. For example:

  debian:~$ openssl md_gost12_256 /dev/null
  Invalid command 'md_gost12_256'; type "help" for a list.
  debian:~$ echo $?
  1

If there is gost-engine in the system but it's not enabled via config - it will
be enabled for tests via `--engine gost' option.

> > > I'm now wondering if the "--engine e' option is still needed?
> > 
> > It's needed. Why you thinking it doesn't? Commit 782224f33cd7 will not
> > load gost (or any other) engine on its own.
> 
> If the gost engine is enabled in openssl.cnf then we don't need to set
> "ENGINE=gost". šI'm obviously missing something here.

There is two ways to load engine in openssl. I think it is mentioned in
the commit messages when this functionality was added to evmctl.

1. One way is via careful editing of openssl config.
2. Another is simply via --engine option.

Both ways are supported by openssl tools. (For example curl supports
--engine option.) But for tests I thought it would be simpler to use
--engine option instead of generating openssl.cnf.

If user have already configured, for example system wide, openssl.cnf to
load gost-engine this test will not add any option. I think this is
flexible.

> > (check* was supposed to be top-level tests. I will change this in v3.)
> > 
> > > > +  local alg=$1 pref=$2 hash=$3 file=$4
> > > > +
> > > > +  FOR=$alg DEL=$file 
> > > 
> > > Why not use ALG=$alg and FILE=$file as the global variable names?
> > 
> > check was called once for every algo. Are you proposing to change
> > call like
> 
> Although "FOR" is capitalized, I was reading it as "for". šIt took me
> a while to realize that "FOR" and "DEL" are global variables being
> used in "_evmctl_run". šAnything you can do to make it easier to read
> would be appreciated. šJust adding comments would help.

OK

> While further testing, "_require evmctl openssl" should also make sure
> that getfattr is installed.

Thanks!

[prev in list] [next in list] [prev in thread] [next in thread]