[PATCH v5 00/11] ima-evm-utils: Convert sign v2 from RSA to EVP_PKEY API

[PATCH v5 00/11] ima-evm-utils: Convert sign v2 from RSA to EVP_PKEY API

[Vitaly Chikunov]

Convert sign v2 from RSA API (with manual formatting PKCS1) to more generic
EVP_PKEY API, allowing to generate more types of OpenSSL supported signatures.
This is done to enable EC-RDSA signatures, which are already supported in the
Kernel.

Changes since v4:
- Split conversion into more patches, as suggested by Mimi Zohar.
- Small fixes suggested by Mimi Zohar.

Changes since v3:
- As suggested by Mimi Zohar this is v3 splitted into several patches to
  simplify review. No code changes.

Changes since v2:
- Just rebase over newer commits.

Changes since v1:
- More key neutral code in calc_keyid_v1().
- Fix uninitialized sigsize for EVP_PKEY_sign().
- Fix memory leaks for openssl types.

Vitaly Chikunov (11):
  ima-evm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE
  ima-evm-utils: Change read_pub_key to use EVP_PKEY API
  ima-evm-utils: Change read_priv_key to use EVP_PKEY API
  ima-evm-utils: Start converting calc keyid v2 to EVP_PKEY API
  ima-evm-utils: Convert cmd_import to use EVP_PKEY API
  ima-evm-utils: Start converting find_keyid to use EVP_PKEY API
  ima-evm-utils: Convert verify_hash_v2 to EVP_PKEY API
  ima-evm-utils: Finish conversion of find_keyid to EVP_PKEY API
  ima-evm-utils: Convert sign_hash_v2 to use EVP_PKEY API
  ima-evm-utils: Finish converting calc keyid v2 to EVP_PKEY API
  ima-evm-utils: Remove RSA_ASN1_templates

 src/evmctl.c    |  29 ++++---
 src/imaevm.h    |   4 +-
 src/libimaevm.c | 261 +++++++++++++++++++++++++-------------------------------
 3 files changed, 136 insertions(+), 158 deletions(-)

-- 
2.11.0

[PATCH v5 01/11] ima-evm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE

[Vitaly Chikunov]

Fix off-by-one error of the output buffer passed to sign_hash().

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/evmctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 15a7226..03f41fe 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -510,7 +510,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 static int sign_evm(const char *file, const char *key)
 {
 	unsigned char hash[MAX_DIGEST_SIZE];
-	unsigned char sig[MAX_SIGNATURE_SIZE];
+	unsigned char sig[MAX_SIGNATURE_SIZE + 1];
 	int len, err;
 
 	len = calc_evm_hash(file, hash);
@@ -519,7 +519,7 @@ static int sign_evm(const char *file, const char *key)
 		return len;
 
 	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
-	assert(len < sizeof(sig));
+	assert(len <= MAX_SIGNATURE_SIZE);
 	if (len <= 1)
 		return len;
 
-- 
2.11.0

Re: [PATCH v5 01/11] ima-evm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE

[Mimi Zohar]

On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> Fix off-by-one error of the output buffer passed to sign_hash().
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
>  src/evmctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 15a7226..03f41fe 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -510,7 +510,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
>  static int sign_evm(const char *file, const char *key)
>  {
>  	unsigned char hash[MAX_DIGEST_SIZE];
> -	unsigned char sig[MAX_SIGNATURE_SIZE];
> +	unsigned char sig[MAX_SIGNATURE_SIZE + 1];
>  	int len, err;
> 
>  	len = calc_evm_hash(file, hash);
> @@ -519,7 +519,7 @@ static int sign_evm(const char *file, const char *key)
>  		return len;
> 
>  	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
> -	assert(len < sizeof(sig));
> +	assert(len <= MAX_SIGNATURE_SIZE);
>  	if (len <= 1)
>  		return len;
> 

A similar problem occurs in sign_ima.  Without these changes
sign_hash() succeeds, returning a length of 520 for
sha256/streebog256.  With these patches, for streebog256
EVP_PKEY_CTX_set_signature_md is failing, returning -1, but works for
sha256.  With a similar change as this patch, it also works, returning
520.

Obviously it isn't an issue of the buffer size.

Mimi

Re: [PATCH v5 01/11] ima-evm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE

[Vitaly Chikunov]

On Thu, Jun 20, 2019 at 05:42:18PM -0400, Mimi Zohar wrote:
> On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> > Fix off-by-one error of the output buffer passed to sign_hash().
> > 
> > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > ---
> >  src/evmctl.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/evmctl.c b/src/evmctl.c
> > index 15a7226..03f41fe 100644
> > --- a/src/evmctl.c
> > +++ b/src/evmctl.c
> > @@ -510,7 +510,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
> >  static int sign_evm(const char *file, const char *key)
> >  {
> >  	unsigned char hash[MAX_DIGEST_SIZE];
> > -	unsigned char sig[MAX_SIGNATURE_SIZE];
> > +	unsigned char sig[MAX_SIGNATURE_SIZE + 1];
> >  	int len, err;
> > 
> >  	len = calc_evm_hash(file, hash);
> > @@ -519,7 +519,7 @@ static int sign_evm(const char *file, const char *key)
> >  		return len;
> > 
> >  	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
> > -	assert(len < sizeof(sig));
> > +	assert(len <= MAX_SIGNATURE_SIZE);
> >  	if (len <= 1)
> >  		return len;
> > 
> 
> A similar problem occurs in sign_ima.  Without these changes
> sign_hash() succeeds, returning a length of 520 for
> sha256/streebog256. 

I will add it. Also, I found more similar errors and will fix them together.

> With these patches, for streebog256
> EVP_PKEY_CTX_set_signature_md is failing, returning -1,
> but works for sha256.

Probably your openssl does not support streebog256.

> With a similar change as this patch, it also works, returning
> 520.

This is above level than this change so it can not be related.

When I try streebog256 with similar change to sign_ima() I get error
like this:

  $ evmctl ima_sign --key privkey_rsa.pem -a streebog256 --xattr-user test.txt
  sign_hash_v2: signing failed: (invalid digest)
  error:0408C09D:rsa routines:check_padding_md:invalid digest

Which is correct.

Thanks,

Re: [PATCH v5 01/11] ima-evm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE

[Mimi Zohar]

On Fri, 2019-06-21 at 09:59 +0300, Vitaly Chikunov wrote:
> On Thu, Jun 20, 2019 at 05:42:18PM -0400, Mimi Zohar wrote:
> > On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> > > Fix off-by-one error of the output buffer passed to sign_hash().
> > > 
> > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > ---
> > >  src/evmctl.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/evmctl.c b/src/evmctl.c
> > > index 15a7226..03f41fe 100644
> > > --- a/src/evmctl.c
> > > +++ b/src/evmctl.c
> > > @@ -510,7 +510,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
> > >  static int sign_evm(const char *file, const char *key)
> > >  {
> > >  	unsigned char hash[MAX_DIGEST_SIZE];
> > > -	unsigned char sig[MAX_SIGNATURE_SIZE];
> > > +	unsigned char sig[MAX_SIGNATURE_SIZE + 1];
> > >  	int len, err;
> > > 
> > >  	len = calc_evm_hash(file, hash);
> > > @@ -519,7 +519,7 @@ static int sign_evm(const char *file, const char *key)
> > >  		return len;
> > > 
> > >  	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
> > > -	assert(len < sizeof(sig));
> > > +	assert(len <= MAX_SIGNATURE_SIZE);
> > >  	if (len <= 1)
> > >  		return len;
> > > 
> > 
> > A similar problem occurs in sign_ima.  Without these changes
> > sign_hash() succeeds, returning a length of 520 for
> > sha256/streebog256. 
> 
> I will add it. Also, I found more similar errors and will fix them together.

The first byte of sig is reserved for the type of signature.  The
remaining buffer is for the signature itself.  The existing
"assert(len < sizeof(sig))" is therefore correct.  The sig size being
returned is less than 1023, so why is this change needed?

Mimi

Re: [PATCH v5 01/11] ima-evm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE

[Vitaly Chikunov]

Mimi,

On Fri, Jun 21, 2019 at 07:08:12AM -0400, Mimi Zohar wrote:
> On Fri, 2019-06-21 at 09:59 +0300, Vitaly Chikunov wrote:
> > On Thu, Jun 20, 2019 at 05:42:18PM -0400, Mimi Zohar wrote:
> > > On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> > > > Fix off-by-one error of the output buffer passed to sign_hash().
> > > > 
> > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > > ---
> > > >  src/evmctl.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/src/evmctl.c b/src/evmctl.c
> > > > index 15a7226..03f41fe 100644
> > > > --- a/src/evmctl.c
> > > > +++ b/src/evmctl.c
> > > > @@ -510,7 +510,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
> > > >  static int sign_evm(const char *file, const char *key)
> > > >  {
> > > >  	unsigned char hash[MAX_DIGEST_SIZE];
> > > > -	unsigned char sig[MAX_SIGNATURE_SIZE];
> > > > +	unsigned char sig[MAX_SIGNATURE_SIZE + 1];
> > > >  	int len, err;
> > > > 
> > > >  	len = calc_evm_hash(file, hash);
> > > > @@ -519,7 +519,7 @@ static int sign_evm(const char *file, const char *key)
> > > >  		return len;
> > > > 
> > > >  	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
> > > > -	assert(len < sizeof(sig));
> > > > +	assert(len <= MAX_SIGNATURE_SIZE);
> > > >  	if (len <= 1)
> > > >  		return len;
> > > > 
> > > 
> > > A similar problem occurs in sign_ima.  Without these changes
> > > sign_hash() succeeds, returning a length of 520 for
> > > sha256/streebog256. 
> > 
> > I will add it. Also, I found more similar errors and will fix them together.
> 
> The first byte of sig is reserved for the type of signature.  The
> remaining buffer is for the signature itself.  The existing
> "assert(len < sizeof(sig))" is therefore correct.  The sig size being
> returned is less than 1023, so why is this change needed?

Well, it looked more straightforward to check explicit
MAX_SIGNATURE_SIZE instead of relying on that '<' accounts for
that additional byte.

Main fix is of course this:

> > > > -       unsigned char sig[MAX_SIGNATURE_SIZE];
> > > > +       unsigned char sig[MAX_SIGNATURE_SIZE + 1];

I will revert all that `assert(len <= MAX_SIGNATURE_SIZE)` back to
`assert(len < sizeof(sig))`.

Thanks,

Re: [PATCH v5 01/11] ima-evm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE

[Mimi Zohar]

On Fri, 2019-06-21 at 14:22 +0300, Vitaly Chikunov wrote:
> Mimi,
> 
> On Fri, Jun 21, 2019 at 07:08:12AM -0400, Mimi Zohar wrote:
> > On Fri, 2019-06-21 at 09:59 +0300, Vitaly Chikunov wrote:
> > > On Thu, Jun 20, 2019 at 05:42:18PM -0400, Mimi Zohar wrote:
> > > > On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> > > > > Fix off-by-one error of the output buffer passed to sign_hash().
> > > > > 
> > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > > > ---
> > > > >  src/evmctl.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/src/evmctl.c b/src/evmctl.c
> > > > > index 15a7226..03f41fe 100644
> > > > > --- a/src/evmctl.c
> > > > > +++ b/src/evmctl.c
> > > > > @@ -510,7 +510,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
> > > > >  static int sign_evm(const char *file, const char *key)
> > > > >  {
> > > > >  	unsigned char hash[MAX_DIGEST_SIZE];
> > > > > -	unsigned char sig[MAX_SIGNATURE_SIZE];
> > > > > +	unsigned char sig[MAX_SIGNATURE_SIZE + 1];
> > > > >  	int len, err;
> > > > > 
> > > > >  	len = calc_evm_hash(file, hash);
> > > > > @@ -519,7 +519,7 @@ static int sign_evm(const char *file, const char *key)
> > > > >  		return len;
> > > > > 
> > > > >  	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
> > > > > -	assert(len < sizeof(sig));
> > > > > +	assert(len <= MAX_SIGNATURE_SIZE);
> > > > >  	if (len <= 1)
> > > > >  		return len;
> > > > > 
> > > > 
> > > > A similar problem occurs in sign_ima.  Without these changes
> > > > sign_hash() succeeds, returning a length of 520 for
> > > > sha256/streebog256. 
> > > 
> > > I will add it. Also, I found more similar errors and will fix them together.
> > 
> > The first byte of sig is reserved for the type of signature.  The
> > remaining buffer is for the signature itself.  The existing
> > "assert(len < sizeof(sig))" is therefore correct.  The sig size being
> > returned is less than 1023, so why is this change needed?
> 
> Well, it looked more straightforward to check explicit
> MAX_SIGNATURE_SIZE instead of relying on that '<' accounts for
> that additional byte.
> 
> Main fix is of course this:
> 
> > > > > -       unsigned char sig[MAX_SIGNATURE_SIZE];
> > > > > +       unsigned char sig[MAX_SIGNATURE_SIZE + 1];

That is the question.  Why does the buffer need to be
"MAX_SIGNATURE_SIZE + 1", making it 1025 bytes?  MAX_SIGNATURE_SIZE -
1 is large enough for the signature.

Mimi

> 
> I will revert all that `assert(len <= MAX_SIGNATURE_SIZE)` back to
> `assert(len < sizeof(sig))`.
> 
> Thanks,
> 

Re: [PATCH v5 01/11] ima-evm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE

[Vitaly Chikunov]

Mimi,

On Fri, Jun 21, 2019 at 07:27:30AM -0400, Mimi Zohar wrote:
> On Fri, 2019-06-21 at 14:22 +0300, Vitaly Chikunov wrote:
> > On Fri, Jun 21, 2019 at 07:08:12AM -0400, Mimi Zohar wrote:
> > > On Fri, 2019-06-21 at 09:59 +0300, Vitaly Chikunov wrote:
> > > > On Thu, Jun 20, 2019 at 05:42:18PM -0400, Mimi Zohar wrote:
> > > > > On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> > > > > > Fix off-by-one error of the output buffer passed to sign_hash().
> > > > > > 
> > > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > > > > ---
> > > > > >  src/evmctl.c | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/src/evmctl.c b/src/evmctl.c
> > > > > > index 15a7226..03f41fe 100644
> > > > > > --- a/src/evmctl.c
> > > > > > +++ b/src/evmctl.c
> > > > > > @@ -510,7 +510,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
> > > > > >  static int sign_evm(const char *file, const char *key)
> > > > > >  {
> > > > > >  	unsigned char hash[MAX_DIGEST_SIZE];
> > > > > > -	unsigned char sig[MAX_SIGNATURE_SIZE];
> > > > > > +	unsigned char sig[MAX_SIGNATURE_SIZE + 1];
> > > > > >  	int len, err;
> > > > > > 
> > > > > >  	len = calc_evm_hash(file, hash);
> > > > > > @@ -519,7 +519,7 @@ static int sign_evm(const char *file, const char *key)
> > > > > >  		return len;
> > > > > > 
> > > > > >  	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
> > > > > > -	assert(len < sizeof(sig));
> > > > > > +	assert(len <= MAX_SIGNATURE_SIZE);
> > > > > >  	if (len <= 1)
> > > > > >  		return len;
> > > > > > 
> > > > > 
> > > > > A similar problem occurs in sign_ima.  Without these changes
> > > > > sign_hash() succeeds, returning a length of 520 for
> > > > > sha256/streebog256. 
> > > > 
> > > > I will add it. Also, I found more similar errors and will fix them together.
> > > 
> > > The first byte of sig is reserved for the type of signature.  The
> > > remaining buffer is for the signature itself.  The existing
> > > "assert(len < sizeof(sig))" is therefore correct.  The sig size being
> > > returned is less than 1023, so why is this change needed?
> > 
> > Well, it looked more straightforward to check explicit
> > MAX_SIGNATURE_SIZE instead of relying on that '<' accounts for
> > that additional byte.
> > 
> > Main fix is of course this:
> > 
> > > > > > -       unsigned char sig[MAX_SIGNATURE_SIZE];
> > > > > > +       unsigned char sig[MAX_SIGNATURE_SIZE + 1];
> 
> That is the question.  Why does the buffer need to be
> "MAX_SIGNATURE_SIZE + 1", making it 1025 bytes?  MAX_SIGNATURE_SIZE -
> 1 is large enough for the signature.

Because maximum signature size is supposed to be MAX_SIGNATURE_SIZE,
isn't it? Why in reality it should be some other value?

That give me idea to add check if a generated signature will fit into
`sig` (assuming it's of MAX_SIGNATURE_SIZE) in sign_hash_v2() before we
call EVP_PKEY_sign().

Re: [PATCH v5 01/11] ima-evm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE

[Mimi Zohar]

On Fri, 2019-06-21 at 15:28 +0300, Vitaly Chikunov wrote:
> Mimi,
> 
> On Fri, Jun 21, 2019 at 07:27:30AM -0400, Mimi Zohar wrote:
> > On Fri, 2019-06-21 at 14:22 +0300, Vitaly Chikunov wrote:
> > > On Fri, Jun 21, 2019 at 07:08:12AM -0400, Mimi Zohar wrote:
> > > > On Fri, 2019-06-21 at 09:59 +0300, Vitaly Chikunov wrote:
> > > > > On Thu, Jun 20, 2019 at 05:42:18PM -0400, Mimi Zohar wrote:
> > > > > > On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> > > > > > > Fix off-by-one error of the output buffer passed to sign_hash().
> > > > > > > 
> > > > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > > > > > ---
> > > > > > >  src/evmctl.c | 4 ++--
> > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/src/evmctl.c b/src/evmctl.c
> > > > > > > index 15a7226..03f41fe 100644
> > > > > > > --- a/src/evmctl.c
> > > > > > > +++ b/src/evmctl.c
> > > > > > > @@ -510,7 +510,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
> > > > > > >  static int sign_evm(const char *file, const char *key)
> > > > > > >  {
> > > > > > >  	unsigned char hash[MAX_DIGEST_SIZE];
> > > > > > > -	unsigned char sig[MAX_SIGNATURE_SIZE];
> > > > > > > +	unsigned char sig[MAX_SIGNATURE_SIZE + 1];
> > > > > > >  	int len, err;
> > > > > > > 
> > > > > > >  	len = calc_evm_hash(file, hash);
> > > > > > > @@ -519,7 +519,7 @@ static int sign_evm(const char *file, const char *key)
> > > > > > >  		return len;
> > > > > > > 
> > > > > > >  	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
> > > > > > > -	assert(len < sizeof(sig));
> > > > > > > +	assert(len <= MAX_SIGNATURE_SIZE);
> > > > > > >  	if (len <= 1)
> > > > > > >  		return len;
> > > > > > > 
> > > > > > 
> > > > > > A similar problem occurs in sign_ima.  Without these changes
> > > > > > sign_hash() succeeds, returning a length of 520 for
> > > > > > sha256/streebog256. 
> > > > > 
> > > > > I will add it. Also, I found more similar errors and will fix them together.
> > > > 
> > > > The first byte of sig is reserved for the type of signature.  The
> > > > remaining buffer is for the signature itself.  The existing
> > > > "assert(len < sizeof(sig))" is therefore correct.  The sig size being
> > > > returned is less than 1023, so why is this change needed?
> > > 
> > > Well, it looked more straightforward to check explicit
> > > MAX_SIGNATURE_SIZE instead of relying on that '<' accounts for
> > > that additional byte.
> > > 
> > > Main fix is of course this:
> > > 
> > > > > > > -       unsigned char sig[MAX_SIGNATURE_SIZE];
> > > > > > > +       unsigned char sig[MAX_SIGNATURE_SIZE + 1];
> > 
> > That is the question.  Why does the buffer need to be
> > "MAX_SIGNATURE_SIZE + 1", making it 1025 bytes?  MAX_SIGNATURE_SIZE -
> > 1 is large enough for the signature.
> 
> Because maximum signature size is supposed to be MAX_SIGNATURE_SIZE,
> isn't it? Why in reality it should be some other value?

No, I think it was chosen as an upper bound, simply used for buffer
bounds checking.  I wouldn't make sig 1025.  If you want to make
MAX_SIGNATuRE_SIZE 1023 and keep the + 1, that would be fine.

> 
> That give me idea to add check if a generated signature will fit into
> `sig` (assuming it's of MAX_SIGNATURE_SIZE) in sign_hash_v2() before we
> call EVP_PKEY_sign().

Yes, a call to EVP_PKEY_sign(), without providing the "sig", will
return the length.  evmctl can be called recursively (-r).  I would
hope that EVP_PKEY_sign() would check the buffer size before
calculating the sig.  If it does, then checking is duplication.  I'm a
bit concerned about the performance impact of checking the sig size
each time.

Mimi

Re: [PATCH v5 01/11] ima-evm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE

[Vitaly Chikunov]

Mimi,

On Fri, Jun 21, 2019 at 09:03:56AM -0400, Mimi Zohar wrote:
> On Fri, 2019-06-21 at 15:28 +0300, Vitaly Chikunov wrote:
> > On Fri, Jun 21, 2019 at 07:27:30AM -0400, Mimi Zohar wrote:
> > > On Fri, 2019-06-21 at 14:22 +0300, Vitaly Chikunov wrote:
> > > > On Fri, Jun 21, 2019 at 07:08:12AM -0400, Mimi Zohar wrote:
> > > > > On Fri, 2019-06-21 at 09:59 +0300, Vitaly Chikunov wrote:
> > > > > > On Thu, Jun 20, 2019 at 05:42:18PM -0400, Mimi Zohar wrote:
> > > > > > > On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> > > > > > > > Fix off-by-one error of the output buffer passed to sign_hash().
> > > > > > > > 
> > > > > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > > > > > > ---
> > > > > > > >  src/evmctl.c | 4 ++--
> > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/src/evmctl.c b/src/evmctl.c
> > > > > > > > index 15a7226..03f41fe 100644
> > > > > > > > --- a/src/evmctl.c
> > > > > > > > +++ b/src/evmctl.c
> > > > > > > > @@ -510,7 +510,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
> > > > > > > >  static int sign_evm(const char *file, const char *key)
> > > > > > > >  {
> > > > > > > >  	unsigned char hash[MAX_DIGEST_SIZE];
> > > > > > > > -	unsigned char sig[MAX_SIGNATURE_SIZE];
> > > > > > > > +	unsigned char sig[MAX_SIGNATURE_SIZE + 1];
> > > > > > > >  	int len, err;
> > > > > > > > 
> > > > > > > >  	len = calc_evm_hash(file, hash);
> > > > > > > > @@ -519,7 +519,7 @@ static int sign_evm(const char *file, const char *key)
> > > > > > > >  		return len;
> > > > > > > > 
> > > > > > > >  	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
> > > > > > > > -	assert(len < sizeof(sig));
> > > > > > > > +	assert(len <= MAX_SIGNATURE_SIZE);
> > > > > > > >  	if (len <= 1)
> > > > > > > >  		return len;
> > > > > > > > 
> > > > > > > 
> > > > > > > A similar problem occurs in sign_ima.  Without these changes
> > > > > > > sign_hash() succeeds, returning a length of 520 for
> > > > > > > sha256/streebog256. 
> > > > > > 
> > > > > > I will add it. Also, I found more similar errors and will fix them together.
> > > > > 
> > > > > The first byte of sig is reserved for the type of signature.  The
> > > > > remaining buffer is for the signature itself.  The existing
> > > > > "assert(len < sizeof(sig))" is therefore correct.  The sig size being
> > > > > returned is less than 1023, so why is this change needed?
> > > > 
> > > > Well, it looked more straightforward to check explicit
> > > > MAX_SIGNATURE_SIZE instead of relying on that '<' accounts for
> > > > that additional byte.
> > > > 
> > > > Main fix is of course this:
> > > > 
> > > > > > > > -       unsigned char sig[MAX_SIGNATURE_SIZE];
> > > > > > > > +       unsigned char sig[MAX_SIGNATURE_SIZE + 1];
> > > 
> > > That is the question.  Why does the buffer need to be
> > > "MAX_SIGNATURE_SIZE + 1", making it 1025 bytes?  MAX_SIGNATURE_SIZE -
> > > 1 is large enough for the signature.
> > 
> > Because maximum signature size is supposed to be MAX_SIGNATURE_SIZE,
> > isn't it? Why in reality it should be some other value?
> 
> No, I think it was chosen as an upper bound, simply used for buffer
> bounds checking.  I wouldn't make sig 1025.  If you want to make
> MAX_SIGNATuRE_SIZE 1023 and keep the + 1, that would be fine.

I will rework these 'fixes' with my new understanding.

> > That give me idea to add check if a generated signature will fit into
> > `sig` (assuming it's of MAX_SIGNATURE_SIZE) in sign_hash_v2() before we
> > call EVP_PKEY_sign().
> 
> Yes, a call to EVP_PKEY_sign(), without providing the "sig", will
> return the length.  evmctl can be called recursively (-r).  I would
> hope that EVP_PKEY_sign() would check the buffer size before
> calculating the sig.  If it does, then checking is duplication.  I'm a
> bit concerned about the performance impact of checking the sig size
> each time.

You are right, EVP_PKEY_sign() is already checking the buffer size. So,
proposed check would be redundant.

Thanks,

[PATCH v5 02/11] ima-evm-utils: Change read_pub_key to use EVP_PKEY API

[Vitaly Chikunov]

Introduce read_pub_pkey() to read keys using EVP_PKEY, and change
read_pub_key() to be wrapper for it.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/imaevm.h    |  1 +
 src/libimaevm.c | 33 ++++++++++++++++++++++-----------
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/src/imaevm.h b/src/imaevm.h
index c81bf21..6d5eabd 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -216,6 +216,7 @@ int get_filesize(const char *filename);
 int ima_calc_hash(const char *file, uint8_t *hash);
 int get_hash_algo(const char *algo);
 RSA *read_pub_key(const char *keyfile, int x509);
+EVP_PKEY *read_pub_pkey(const char *keyfile, int x509);
 
 void calc_keyid_v1(uint8_t *keyid, char *str, const unsigned char *pkey, int len);
 void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 3a9ab63..da0f422 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -355,10 +355,9 @@ int ima_calc_hash(const char *file, uint8_t *hash)
 	return mdlen;
 }
 
-RSA *read_pub_key(const char *keyfile, int x509)
+EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
 {
 	FILE *fp;
-	RSA *key = NULL;
 	X509 *crt = NULL;
 	EVP_PKEY *pkey = NULL;
 
@@ -375,24 +374,36 @@ RSA *read_pub_key(const char *keyfile, int x509)
 			goto out;
 		}
 		pkey = X509_extract_key(crt);
+		X509_free(crt);
 		if (!pkey) {
 			log_err("X509_extract_key() failed\n");
 			goto out;
 		}
-		key = EVP_PKEY_get1_RSA(pkey);
 	} else {
-		key = PEM_read_RSA_PUBKEY(fp, NULL, NULL, NULL);
+		pkey = PEM_read_PUBKEY(fp, NULL, NULL, NULL);
+		if (!pkey)
+			log_err("PEM_read_PUBKEY() failed\n");
 	}
 
-	if (!key)
-		log_err("PEM_read_RSA_PUBKEY() failed\n");
-
 out:
-	if (pkey)
-		EVP_PKEY_free(pkey);
-	if (crt)
-		X509_free(crt);
 	fclose(fp);
+	return pkey;
+}
+
+RSA *read_pub_key(const char *keyfile, int x509)
+{
+	EVP_PKEY *pkey;
+	RSA *key;
+
+	pkey = read_pub_pkey(keyfile, x509);
+	if (!pkey)
+		return NULL;
+	key = EVP_PKEY_get1_RSA(pkey);
+	EVP_PKEY_free(pkey);
+	if (!key) {
+		log_err("read_pub_key: unsupported key type\n");
+		return NULL;
+	}
 	return key;
 }
 
-- 
2.11.0

[PATCH v5 03/11] ima-evm-utils: Change read_priv_key to use EVP_PKEY API

[Vitaly Chikunov]

Introduce read_priv_pkey() to read keys using EVP_PKEY, and change
read_priv_key() to be wrapper for it.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/libimaevm.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index da0f422..23fa804 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -753,10 +753,10 @@ void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key)
 	free(pkey);
 }
 
-static RSA *read_priv_key(const char *keyfile, const char *keypass)
+static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
 {
 	FILE *fp;
-	RSA *key;
+	EVP_PKEY *pkey;
 
 	fp = fopen(keyfile, "r");
 	if (!fp) {
@@ -764,15 +764,32 @@ static RSA *read_priv_key(const char *keyfile, const char *keypass)
 		return NULL;
 	}
 	ERR_load_crypto_strings();
-	key = PEM_read_RSAPrivateKey(fp, NULL, NULL, (void *)keypass);
-	if (!key) {
+	pkey = PEM_read_PrivateKey(fp, NULL, NULL, (void *)keypass);
+	if (!pkey) {
 		char str[256];
 
 		ERR_error_string(ERR_get_error(), str);
-		log_err("PEM_read_RSAPrivateKey() failed: %s\n", str);
+		log_err("PEM_read_PrivateKey() failed: %s\n", str);
 	}
 
 	fclose(fp);
+	return pkey;
+}
+
+static RSA *read_priv_key(const char *keyfile, const char *keypass)
+{
+	EVP_PKEY *pkey;
+	RSA *key;
+
+	pkey = read_priv_pkey(keyfile, keypass);
+	if (!pkey)
+		return NULL;
+	key = EVP_PKEY_get1_RSA(pkey);
+	EVP_PKEY_free(pkey);
+	if (!key) {
+		log_err("read_priv_key: unsupported key type\n");
+		return NULL;
+	}
 	return key;
 }
 
-- 
2.11.0

[PATCH v5 04/11] ima-evm-utils: Start converting calc keyid v2 to EVP_PKEY API

[Vitaly Chikunov]

Introduce calc_pkeyid_v2() to replace calc_keyid_v2() when we switch to
EVP_PKEY from RSA keys.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/imaevm.h    |  1 +
 src/libimaevm.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/src/imaevm.h b/src/imaevm.h
index 6d5eabd..48d2663 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -220,6 +220,7 @@ EVP_PKEY *read_pub_pkey(const char *keyfile, int x509);
 
 void calc_keyid_v1(uint8_t *keyid, char *str, const unsigned char *pkey, int len);
 void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key);
+void calc_pkeyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey);
 int key2bin(RSA *key, unsigned char *pub);
 
 int sign_hash(const char *algo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 23fa804..707b2e9 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -753,6 +753,36 @@ void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key)
 	free(pkey);
 }
 
+/*
+ * Calculate keyid of the public_key part of EVP_PKEY
+ */
+void calc_pkeyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey)
+{
+	X509_PUBKEY *pk = NULL;
+	const unsigned char *public_key = NULL;
+	int len;
+
+	/* This is more generic than i2d_PublicKey() */
+	if (X509_PUBKEY_set(&pk, pkey) &&
+	    X509_PUBKEY_get0_param(NULL, &public_key, &len, NULL, pk)) {
+		uint8_t sha1[SHA_DIGEST_LENGTH];
+
+		SHA1(public_key, len, sha1);
+		/* sha1[12 - 19] is exactly keyid from gpg file */
+		memcpy(keyid, sha1 + 16, 4);
+	} else
+		*keyid = 0;
+
+	log_debug("keyid: ");
+	log_debug_dump(keyid, 4);
+	sprintf(str, "%x", __be32_to_cpup(keyid));
+
+	if (params.verbose > LOG_INFO)
+		log_info("keyid: %s\n", str);
+
+	X509_PUBKEY_free(pk);
+}
+
 static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
 {
 	FILE *fp;
-- 
2.11.0

Re: [PATCH v5 04/11] ima-evm-utils: Start converting calc keyid v2 to EVP_PKEY API

[Mimi Zohar]

On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> Introduce calc_pkeyid_v2() to replace calc_keyid_v2() when we switch to
> EVP_PKEY from RSA keys.
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>

Nice, but instead of making this an entirely separate patch, I would
squash "5/11 ima-evm-utils: Convert cmd_import to use EVP_PKEY API"
with this patch.

In general, patches should contain both the new function and a caller
of the new function.  For example, the previous patches defined
function wrappers.  Both the new function and the caller of the new
function were included in one patch.

Mimi

[PATCH v5 05/11] ima-evm-utils: Convert cmd_import to use EVP_PKEY API

[Vitaly Chikunov]

This is where we call calc_pkeyid_v2() for a first time.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/evmctl.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 03f41fe..224cb1f 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -891,7 +891,6 @@ static int cmd_import(struct command *cmd)
 	int id, len, err = 0;
 	char name[20];
 	uint8_t keyid[8];
-	RSA *key;
 
 	inkey = g_argv[optind++];
 	if (!inkey) {
@@ -925,18 +924,26 @@ static int cmd_import(struct command *cmd)
 		}
 	}
 
-	key = read_pub_key(inkey, params.x509);
-	if (!key)
-		return 1;
-
 	if (params.x509) {
+		EVP_PKEY *pkey = read_pub_pkey(inkey, params.x509);
+
+		if (!pkey)
+			return 1;
 		pub = file2bin(inkey, NULL, &len);
-		if (!pub)
-			goto out;
-		calc_keyid_v2((uint32_t *)keyid, name, key);
+		if (!pub) {
+			EVP_PKEY_free(pkey);
+			return 1;
+		}
+		calc_pkeyid_v2((uint32_t *)keyid, name, pkey);
+		EVP_PKEY_free(pkey);
 	} else {
+		RSA *key = read_pub_key(inkey, params.x509);
+
+		if (!key)
+			return 1;
 		len = key2bin(key, pub);
 		calc_keyid_v1(keyid, name, pub, len);
+		RSA_free(key);
 	}
 
 	log_info("Importing public key %s from file %s into keyring %d\n", name, inkey, id);
@@ -951,8 +958,6 @@ static int cmd_import(struct command *cmd)
 	}
 	if (params.x509)
 		free(pub);
-out:
-	RSA_free(key);
 	return err;
 }
 
-- 
2.11.0

[PATCH v5 06/11] ima-evm-utils: Start converting find_keyid to use EVP_PKEY API

[Vitaly Chikunov]

New find_keyid_pkey() accepts EVP_PKEY. Old find_keyid() calls
find_keyid_pkey(), but still return RSA key.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/libimaevm.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index 707b2e9..ae18005 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -452,11 +452,11 @@ struct public_key_entry {
 	struct public_key_entry *next;
 	uint32_t keyid;
 	char name[9];
-	RSA *key;
+	EVP_PKEY *key;
 };
 static struct public_key_entry *public_keys = NULL;
 
-static RSA *find_keyid(uint32_t keyid)
+static EVP_PKEY *find_keyid_pkey(uint32_t keyid)
 {
 	struct public_key_entry *entry;
 
@@ -467,6 +467,22 @@ static RSA *find_keyid(uint32_t keyid)
 	return NULL;
 }
 
+static RSA *find_keyid(uint32_t keyid)
+{
+	EVP_PKEY *pkey;
+	RSA *key;
+
+	pkey = find_keyid_pkey(keyid);
+	if (!pkey)
+		return NULL;
+	key = EVP_PKEY_get0_RSA(pkey);
+	if (!key) {
+		log_err("find_keyid: unsupported key type\n");
+		return NULL;
+	}
+	return key;
+}
+
 void init_public_keys(const char *keyfiles)
 {
 	struct public_key_entry *entry;
@@ -489,13 +505,13 @@ void init_public_keys(const char *keyfiles)
 			break;
 		}
 
-		entry->key = read_pub_key(keyfile, 1);
+		entry->key = read_pub_pkey(keyfile, 1);
 		if (!entry->key) {
 			free(entry);
 			continue;
 		}
 
-		calc_keyid_v2(&entry->keyid, entry->name, entry->key);
+		calc_pkeyid_v2(&entry->keyid, entry->name, entry->key);
 		sprintf(entry->name, "%x", __be32_to_cpup(&entry->keyid));
 		log_info("key %d: %s %s\n", i++, entry->name, keyfile);
 		entry->next = public_keys;
-- 
2.11.0

Re: [PATCH v5 06/11] ima-evm-utils: Start converting find_keyid to use EVP_PKEY API

[Mimi Zohar]

On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> New find_keyid_pkey() accepts EVP_PKEY. Old find_keyid() calls
> find_keyid_pkey(), but still return RSA key.
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>

With titles starting with "Start converting", it leaves me wondering
whether these patches are bisect safe.  Does this patch make
find_keyid() a wrapper for find_keyid_pkey()?  Do all callers of
find_keyid() continue to work properly?  If so, why are there other
changes in this patch?

If you haven't already, please make sure that after each patch is
applied, the code not only compiles cleanly, but works properly.

Mimi

> ---
>  src/libimaevm.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 707b2e9..ae18005 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -452,11 +452,11 @@ struct public_key_entry {
>  	struct public_key_entry *next;
>  	uint32_t keyid;
>  	char name[9];
> -	RSA *key;
> +	EVP_PKEY *key;
>  };
>  static struct public_key_entry *public_keys = NULL;
> 
> -static RSA *find_keyid(uint32_t keyid)
> +static EVP_PKEY *find_keyid_pkey(uint32_t keyid)
>  {
>  	struct public_key_entry *entry;
> 
> @@ -467,6 +467,22 @@ static RSA *find_keyid(uint32_t keyid)
>  	return NULL;
>  }
> 
> +static RSA *find_keyid(uint32_t keyid)
> +{
> +	EVP_PKEY *pkey;
> +	RSA *key;
> +
> +	pkey = find_keyid_pkey(keyid);
> +	if (!pkey)
> +		return NULL;
> +	key = EVP_PKEY_get0_RSA(pkey);
> +	if (!key) {
> +		log_err("find_keyid: unsupported key type\n");
> +		return NULL;
> +	}
> +	return key;
> +}
> +
>  void init_public_keys(const char *keyfiles)
>  {
>  	struct public_key_entry *entry;
> @@ -489,13 +505,13 @@ void init_public_keys(const char *keyfiles)
>  			break;
>  		}
> 
> -		entry->key = read_pub_key(keyfile, 1);
> +		entry->key = read_pub_pkey(keyfile, 1);
>  		if (!entry->key) {
>  			free(entry);
>  			continue;
>  		}
> 
> -		calc_keyid_v2(&entry->keyid, entry->name, entry->key);
> +		calc_pkeyid_v2(&entry->keyid, entry->name, entry->key);
>  		sprintf(entry->name, "%x", __be32_to_cpup(&entry->keyid));
>  		log_info("key %d: %s %s\n", i++, entry->name, keyfile);
>  		entry->next = public_keys;

Re: [PATCH v5 06/11] ima-evm-utils: Start converting find_keyid to use EVP_PKEY API

[Vitaly Chikunov]

Mimi,

On Wed, Jun 19, 2019 at 08:26:30AM -0400, Mimi Zohar wrote:
> On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> > New find_keyid_pkey() accepts EVP_PKEY. Old find_keyid() calls
> > find_keyid_pkey(), but still return RSA key.
> > 
> > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> 
> With titles starting with "Start converting", it leaves me wondering
> whether these patches are bisect safe.  Does this patch make
> find_keyid() a wrapper for find_keyid_pkey()?

Yes.

> Do all callers of find_keyid() continue to work properly?

Yes.

> If so, why are there other changes in this patch?

There is no other changes beside stated in description.

> If you haven't already, please make sure that after each patch is
> applied, the code not only compiles cleanly, but works properly.

Yes, they compile and work properly after each patch.

Thanks,

Re: [PATCH v5 06/11] ima-evm-utils: Start converting find_keyid to use EVP_PKEY API

[Mimi Zohar]

On Wed, 2019-06-19 at 18:43 +0300, Vitaly Chikunov wrote:
> Mimi,
> 
> On Wed, Jun 19, 2019 at 08:26:30AM -0400, Mimi Zohar wrote:
> > On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> > > New find_keyid_pkey() accepts EVP_PKEY. Old find_keyid() calls
> > > find_keyid_pkey(), but still return RSA key.
> > > 
> > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > 
> > With titles starting with "Start converting", it leaves me wondering
> > whether these patches are bisect safe.  Does this patch make
> > find_keyid() a wrapper for find_keyid_pkey()?
> 
> Yes.
> 
> > Do all callers of find_keyid() continue to work properly?
> 
> Yes.
> 
> > If so, why are there other changes in this patch?
> 
> There is no other changes beside stated in description.

Are the changes from read_pub_key() to read_pub_pkey() and
calc_keyid_v2() to calc_pkeyid_v2() needed for making find_keyid() a
wrapper for find_keyid_pkey()?

> 
> > If you haven't already, please make sure that after each patch is
> > applied, the code not only compiles cleanly, but works properly.
> 
> Yes, they compile and work properly after each patch.

thanks!

Mimi

Re: [PATCH v5 06/11] ima-evm-utils: Start converting find_keyid to use EVP_PKEY API

[Vitaly Chikunov]

Mimi,

On Wed, Jun 19, 2019 at 12:46:50PM -0400, Mimi Zohar wrote:
> On Wed, 2019-06-19 at 18:43 +0300, Vitaly Chikunov wrote:
> > On Wed, Jun 19, 2019 at 08:26:30AM -0400, Mimi Zohar wrote:
> > > On Tue, 2019-06-18 at 16:56 +0300, Vitaly Chikunov wrote:
> > > > New find_keyid_pkey() accepts EVP_PKEY. Old find_keyid() calls
> > > > find_keyid_pkey(), but still return RSA key.
> > > > 
> > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > 
> > > With titles starting with "Start converting", it leaves me wondering
> > > whether these patches are bisect safe.  Does this patch make
> > > find_keyid() a wrapper for find_keyid_pkey()?
> > 
> > Yes.
> > 
> > > Do all callers of find_keyid() continue to work properly?
> > 
> > Yes.
> > 
> > > If so, why are there other changes in this patch?
> > 
> > There is no other changes beside stated in description.
> 
> Are the changes from read_pub_key() to read_pub_pkey() and
> calc_keyid_v2() to calc_pkeyid_v2() needed for making find_keyid() a
> wrapper for find_keyid_pkey()?

Of course. `entry->key' now have different type. If we keep old type
(RSA) where will be nothing to wrap.

Thanks,

Re: [PATCH v5 06/11] ima-evm-utils: Start converting find_keyid to use EVP_PKEY API

[Mimi Zohar]

Hi Vitaly,

> > > > If so, why are there other changes in this patch?
> > > 
> > > There is no other changes beside stated in description.
> > 
> > Are the changes from read_pub_key() to read_pub_pkey() and
> > calc_keyid_v2() to calc_pkeyid_v2() needed for making find_keyid() a
> > wrapper for find_keyid_pkey()?
> 
> Of course. `entry->key' now have different type. If we keep old type
> (RSA) where will be nothing to wrap.

The question wasn't if the changes in init_public_keys() need to be
made, the question is its correlation to find_keyid().  Unlesss I'm
missing something, find_keyid() is only called by verify_hash_v2(),
not calc_keyid_v2().

Mimi

Re: [PATCH v5 06/11] ima-evm-utils: Start converting find_keyid to use EVP_PKEY API

[Mimi Zohar]

On Thu, 2019-06-20 at 09:21 -0400, Mimi Zohar wrote:
> Hi Vitaly,
> 
> > > > > If so, why are there other changes in this patch?
> > > > 
> > > > There is no other changes beside stated in description.
> > > 
> > > Are the changes from read_pub_key() to read_pub_pkey() and
> > > calc_keyid_v2() to calc_pkeyid_v2() needed for making find_keyid() a
> > > wrapper for find_keyid_pkey()?
> > 
> > Of course. `entry->key' now have different type. If we keep old type
> > (RSA) where will be nothing to wrap.
> 
> The question wasn't if the changes in init_public_keys() need to be
> made, the question is its correlation to find_keyid().  Unlesss I'm
> missing something, find_keyid() is only called by verify_hash_v2(),
> not calc_keyid_v2().

Ah, the list of keys needs to be in the appropriate format.  Perhaps
add that info in the patch description.

Mimi

Re: [PATCH v5 06/11] ima-evm-utils: Start converting find_keyid to use EVP_PKEY API

[Vitaly Chikunov]

Mimi,

On Thu, Jun 20, 2019 at 09:40:49AM -0400, Mimi Zohar wrote:
> On Thu, 2019-06-20 at 09:21 -0400, Mimi Zohar wrote:
> > > > > > If so, why are there other changes in this patch?
> > > > > 
> > > > > There is no other changes beside stated in description.
> > > > 
> > > > Are the changes from read_pub_key() to read_pub_pkey() and
> > > > calc_keyid_v2() to calc_pkeyid_v2() needed for making find_keyid() a
> > > > wrapper for find_keyid_pkey()?
> > > 
> > > Of course. `entry->key' now have different type. If we keep old type
> > > (RSA) where will be nothing to wrap.
> > 
> > The question wasn't if the changes in init_public_keys() need to be
> > made, the question is its correlation to find_keyid().  Unlesss I'm
> > missing something, find_keyid() is only called by verify_hash_v2(),
> > not calc_keyid_v2().
> 
> Ah, the list of keys needs to be in the appropriate format.  Perhaps
> add that info in the patch description.

Isn't this is implied by find_keyid()? It finds appropriate
`entry->key', and it's type is changed to EVP_PKEY, so both
init_public_keys() (where it fills `entry->key`) and find_keyid() (where
it returns `entry->key`) need to be changed.

Vitaly.

[PATCH v5 07/11] ima-evm-utils: Convert verify_hash_v2 to EVP_PKEY API

[Vitaly Chikunov]

Rely on OpenSSL API to verify v2 signatures instead of manual PKCS1
decoding.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/libimaevm.c | 59 ++++++++++++++++++++++++---------------------------------
 1 file changed, 25 insertions(+), 34 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index ae18005..73beb28 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -522,11 +522,11 @@ void init_public_keys(const char *keyfiles)
 int verify_hash_v2(const char *file, const unsigned char *hash, int size,
 		   unsigned char *sig, int siglen, const char *keyfile)
 {
-	int err, len;
-	unsigned char out[1024];
-	RSA *key;
+	int err;
+	EVP_PKEY *pkey;
 	struct signature_v2_hdr *hdr = (struct signature_v2_hdr *)sig;
-	const struct RSA_ASN1_template *asn1;
+	EVP_PKEY_CTX *ctx;
+	const EVP_MD *md;
 
 	if (params.verbose > LOG_INFO) {
 		log_info("hash: ");
@@ -534,45 +534,36 @@ int verify_hash_v2(const char *file, const unsigned char *hash, int size,
 	}
 
 	if (public_keys) {
-		key = find_keyid(hdr->keyid);
-		if (!key) {
+		pkey = find_keyid_pkey(hdr->keyid);
+		if (!pkey) {
 			log_err("%s: unknown keyid: %x\n", file,
 				__be32_to_cpup(&hdr->keyid));
 			return -1;
 		}
 	} else {
-		key = read_pub_key(keyfile, 1);
-		if (!key)
+		pkey = read_pub_pkey(keyfile, 1);
+		if (!pkey)
 			return 1;
 	}
 
+	if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))
+		goto err;
+	if (!EVP_PKEY_verify_init(ctx))
+		goto err;
+	if (!(md = EVP_get_digestbyname(params.hash_algo)))
+		goto err;
+	if (!EVP_PKEY_CTX_set_signature_md(ctx, md))
+		goto err;
+	err = EVP_PKEY_verify(ctx, sig + sizeof(*hdr),
+			      siglen - sizeof(*hdr), hash, size);
+	EVP_PKEY_CTX_free(ctx);
+	EVP_PKEY_free(pkey);
 
-	err = RSA_public_decrypt(siglen - sizeof(*hdr), sig + sizeof(*hdr),
-				 out, key, RSA_PKCS1_PADDING);
-	if (err < 0) {
-		log_err("%s: RSA_public_decrypt() failed: %d\n", file, err);
-		return 1;
-	}
-
-	len = err;
-
-	asn1 = &RSA_ASN1_templates[hdr->hash_algo];
-
-	if (len < asn1->size || memcmp(out, asn1->data, asn1->size)) {
-		log_err("%s: verification failed: %d (asn1 mismatch)\n",
-			file, err);
-		return -1;
-	}
-
-	len -= asn1->size;
-
-	if (len != size || memcmp(out + asn1->size, hash, len)) {
-		log_err("%s: verification failed: %d (digest mismatch)\n",
-			file, err);
-		return -1;
-	}
-
-	return 0;
+	return err != 1;
+err:
+	EVP_PKEY_CTX_free(ctx);
+	EVP_PKEY_free(pkey);
+	return -1;
 }
 
 int get_hash_algo(const char *algo)
-- 
2.11.0

[PATCH v5 08/11] ima-evm-utils: Finish conversion of find_keyid to EVP_PKEY API

[Vitaly Chikunov]

Replace old find_keyid() with new find_keyid_pkey().

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/libimaevm.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index 73beb28..3b646e5 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -456,7 +456,7 @@ struct public_key_entry {
 };
 static struct public_key_entry *public_keys = NULL;
 
-static EVP_PKEY *find_keyid_pkey(uint32_t keyid)
+static EVP_PKEY *find_keyid(uint32_t keyid)
 {
 	struct public_key_entry *entry;
 
@@ -467,22 +467,6 @@ static EVP_PKEY *find_keyid_pkey(uint32_t keyid)
 	return NULL;
 }
 
-static RSA *find_keyid(uint32_t keyid)
-{
-	EVP_PKEY *pkey;
-	RSA *key;
-
-	pkey = find_keyid_pkey(keyid);
-	if (!pkey)
-		return NULL;
-	key = EVP_PKEY_get0_RSA(pkey);
-	if (!key) {
-		log_err("find_keyid: unsupported key type\n");
-		return NULL;
-	}
-	return key;
-}
-
 void init_public_keys(const char *keyfiles)
 {
 	struct public_key_entry *entry;
@@ -534,7 +518,7 @@ int verify_hash_v2(const char *file, const unsigned char *hash, int size,
 	}
 
 	if (public_keys) {
-		pkey = find_keyid_pkey(hdr->keyid);
+		pkey = find_keyid(hdr->keyid);
 		if (!pkey) {
 			log_err("%s: unknown keyid: %x\n", file,
 				__be32_to_cpup(&hdr->keyid));
-- 
2.11.0

[PATCH v5 09/11] ima-evm-utils: Convert sign_hash_v2 to use EVP_PKEY API

[Vitaly Chikunov]

Convert sign_hash_v2() to use more generic EVP_PKEY API instead of RSA
API. This enables generation of more signatures out of the box, such as
EC-RDSA (GOST) and any other that OpenSSL supports. This conversion also
fixes generation of MD4 signatures, because it didn't have proper
RSA_ASN1_template.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/libimaevm.c | 51 +++++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index 3b646e5..7b8ce92 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -905,14 +905,16 @@ out:
 	return len;
 }
 
+/* @sig is assumed to be of MAX_SIGNATURE_SIZE size */
 int sign_hash_v2(const char *algo, const unsigned char *hash, int size, const char *keyfile, unsigned char *sig)
 {
 	struct signature_v2_hdr *hdr;
 	int len = -1;
-	RSA *key;
+	EVP_PKEY *pkey;
 	char name[20];
-	unsigned char *buf;
-	const struct RSA_ASN1_template *asn1;
+	EVP_PKEY_CTX *ctx = NULL;
+	const EVP_MD *md;
+	size_t sigsize;
 
 	if (!hash) {
 		log_err("sign_hash_v2: hash is null\n");
@@ -937,8 +939,8 @@ int sign_hash_v2(const char *algo, const unsigned char *hash, int size, const ch
 	log_info("hash: ");
 	log_dump(hash, size);
 
-	key = read_priv_key(keyfile, params.keypass);
-	if (!key)
+	pkey = read_priv_pkey(keyfile, params.keypass);
+	if (!pkey)
 		return -1;
 
 	hdr = (struct signature_v2_hdr *)sig;
@@ -946,32 +948,33 @@ int sign_hash_v2(const char *algo, const unsigned char *hash, int size, const ch
 
 	hdr->hash_algo = get_hash_algo(algo);
 
-	calc_keyid_v2(&hdr->keyid, name, key);
+	calc_pkeyid_v2(&hdr->keyid, name, pkey);
 
-	asn1 = &RSA_ASN1_templates[hdr->hash_algo];
-
-	buf = malloc(size + asn1->size);
-	if (!buf)
-		goto out;
-
-	memcpy(buf, asn1->data, asn1->size);
-	memcpy(buf + asn1->size, hash, size);
-	len = RSA_private_encrypt(size + asn1->size, buf, hdr->sig,
-				  key, RSA_PKCS1_PADDING);
-	if (len < 0) {
-		log_err("RSA_private_encrypt() failed: %d\n", len);
-		goto out;
-	}
+	if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))
+		goto err;
+	if (!EVP_PKEY_sign_init(ctx))
+		goto err;
+	if (!(md = EVP_get_digestbyname(params.hash_algo)))
+		goto err;
+	if (!EVP_PKEY_CTX_set_signature_md(ctx, md))
+		goto err;
+	sigsize = MAX_SIGNATURE_SIZE - sizeof(struct signature_v2_hdr);
+	if (!EVP_PKEY_sign(ctx, hdr->sig, &sigsize, hash, size))
+		goto err;
+	len = (int)sigsize;
 
 	/* we add bit length of the signature to make it gnupg compatible */
 	hdr->sig_size = __cpu_to_be16(len);
 	len += sizeof(*hdr);
 	log_info("evm/ima signature: %d bytes\n", len);
-out:
-	if (buf)
-		free(buf);
-	RSA_free(key);
+
+	EVP_PKEY_CTX_free(ctx);
+	EVP_PKEY_free(pkey);
 	return len;
+err:
+	EVP_PKEY_CTX_free(ctx);
+	EVP_PKEY_free(pkey);
+	return -1;
 }
 
 
-- 
2.11.0

[PATCH v5 10/11] ima-evm-utils: Finish converting calc keyid v2 to EVP_PKEY API

[Vitaly Chikunov]

Replace RSA-specific calc_keyid_v2() with calc_pkeyid_v2() which accepts
EVP_PKEY.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/evmctl.c    |  2 +-
 src/imaevm.h    |  3 +--
 src/libimaevm.c | 28 +++-------------------------
 3 files changed, 5 insertions(+), 28 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 224cb1f..ded18c6 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -934,7 +934,7 @@ static int cmd_import(struct command *cmd)
 			EVP_PKEY_free(pkey);
 			return 1;
 		}
-		calc_pkeyid_v2((uint32_t *)keyid, name, pkey);
+		calc_keyid_v2((uint32_t *)keyid, name, pkey);
 		EVP_PKEY_free(pkey);
 	} else {
 		RSA *key = read_pub_key(inkey, params.x509);
diff --git a/src/imaevm.h b/src/imaevm.h
index 48d2663..9af43a2 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -219,8 +219,7 @@ RSA *read_pub_key(const char *keyfile, int x509);
 EVP_PKEY *read_pub_pkey(const char *keyfile, int x509);
 
 void calc_keyid_v1(uint8_t *keyid, char *str, const unsigned char *pkey, int len);
-void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key);
-void calc_pkeyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey);
+void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey);
 int key2bin(RSA *key, unsigned char *pub);
 
 int sign_hash(const char *algo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 7b8ce92..008cf4a 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -495,7 +495,7 @@ void init_public_keys(const char *keyfiles)
 			continue;
 		}
 
-		calc_pkeyid_v2(&entry->keyid, entry->name, entry->key);
+		calc_keyid_v2(&entry->keyid, entry->name, entry->key);
 		sprintf(entry->name, "%x", __be32_to_cpup(&entry->keyid));
 		log_info("key %d: %s %s\n", i++, entry->name, keyfile);
 		entry->next = public_keys;
@@ -722,32 +722,10 @@ void calc_keyid_v1(uint8_t *keyid, char *str, const unsigned char *pkey, int len
 		log_info("keyid-v1: %s\n", str);
 }
 
-void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key)
-{
-	uint8_t sha1[SHA_DIGEST_LENGTH];
-	unsigned char *pkey = NULL;
-	int len;
-
-	len = i2d_RSAPublicKey(key, &pkey);
-
-	SHA1(pkey, len, sha1);
-
-	/* sha1[12 - 19] is exactly keyid from gpg file */
-	memcpy(keyid, sha1 + 16, 4);
-	log_debug("keyid: ");
-	log_debug_dump(keyid, 4);
-	sprintf(str, "%x", __be32_to_cpup(keyid));
-
-	if (params.verbose > LOG_INFO)
-		log_info("keyid: %s\n", str);
-
-	free(pkey);
-}
-
 /*
  * Calculate keyid of the public_key part of EVP_PKEY
  */
-void calc_pkeyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey)
+void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey)
 {
 	X509_PUBKEY *pk = NULL;
 	const unsigned char *public_key = NULL;
@@ -948,7 +926,7 @@ int sign_hash_v2(const char *algo, const unsigned char *hash, int size, const ch
 
 	hdr->hash_algo = get_hash_algo(algo);
 
-	calc_pkeyid_v2(&hdr->keyid, name, pkey);
+	calc_keyid_v2(&hdr->keyid, name, pkey);
 
 	if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))
 		goto err;
-- 
2.11.0

[PATCH v5 11/11] ima-evm-utils: Remove RSA_ASN1_templates

[Vitaly Chikunov]

RSA_ASN1_templates[] are not needed anymore, because we switched to the
generic EVP_PKEY OpenSSL API to generate signatures instead of
constructing PKCS1 ourselves.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/imaevm.h    |  1 -
 src/libimaevm.c | 57 ---------------------------------------------------------
 2 files changed, 58 deletions(-)

diff --git a/src/imaevm.h b/src/imaevm.h
index 9af43a2..dc81a3a 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -207,7 +207,6 @@ struct RSA_ASN1_template {
 #define	NUM_PCRS 20
 #define DEFAULT_PCR 10
 
-extern const struct RSA_ASN1_template RSA_ASN1_templates[PKEY_HASH__LAST];
 extern struct libevm_params params;
 
 void do_dump(FILE *fp, const void *ptr, int len, bool cr);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 008cf4a..4e1da68 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -81,63 +81,6 @@ const char *const pkey_hash_algo_kern[PKEY_HASH__LAST] = {
 	[PKEY_HASH_STREEBOG_512] = "streebog512",
 };
 
-/*
- * Hash algorithm OIDs plus ASN.1 DER wrappings [RFC4880 sec 5.2.2].
- */
-static const uint8_t RSA_digest_info_MD5[] = {
-	0x30, 0x20, 0x30, 0x0C, 0x06, 0x08,
-	0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x02, 0x05, /* OID */
-	0x05, 0x00, 0x04, 0x10
-};
-
-static const uint8_t RSA_digest_info_SHA1[] = {
-	0x30, 0x21, 0x30, 0x09, 0x06, 0x05,
-	0x2B, 0x0E, 0x03, 0x02, 0x1A,
-	0x05, 0x00, 0x04, 0x14
-};
-
-static const uint8_t RSA_digest_info_RIPE_MD_160[] = {
-	0x30, 0x21, 0x30, 0x09, 0x06, 0x05,
-	0x2B, 0x24, 0x03, 0x02, 0x01,
-	0x05, 0x00, 0x04, 0x14
-};
-
-static const uint8_t RSA_digest_info_SHA224[] = {
-	0x30, 0x2d, 0x30, 0x0d, 0x06, 0x09,
-	0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x04,
-	0x05, 0x00, 0x04, 0x1C
-};
-
-static const uint8_t RSA_digest_info_SHA256[] = {
-	0x30, 0x31, 0x30, 0x0d, 0x06, 0x09,
-	0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01,
-	0x05, 0x00, 0x04, 0x20
-};
-
-static const uint8_t RSA_digest_info_SHA384[] = {
-	0x30, 0x41, 0x30, 0x0d, 0x06, 0x09,
-	0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x02,
-	0x05, 0x00, 0x04, 0x30
-};
-
-static const uint8_t RSA_digest_info_SHA512[] = {
-	0x30, 0x51, 0x30, 0x0d, 0x06, 0x09,
-	0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03,
-	0x05, 0x00, 0x04, 0x40
-};
-
-const struct RSA_ASN1_template RSA_ASN1_templates[PKEY_HASH__LAST] = {
-#define _(X) { RSA_digest_info_##X, sizeof(RSA_digest_info_##X) }
-	[PKEY_HASH_MD5]		= _(MD5),
-	[PKEY_HASH_SHA1]	= _(SHA1),
-	[PKEY_HASH_RIPE_MD_160]	= _(RIPE_MD_160),
-	[PKEY_HASH_SHA256]	= _(SHA256),
-	[PKEY_HASH_SHA384]	= _(SHA384),
-	[PKEY_HASH_SHA512]	= _(SHA512),
-	[PKEY_HASH_SHA224]	= _(SHA224),
-#undef _
-};
-
 struct libevm_params params = {
 	.verbose = LOG_INFO - 1,
 	.x509 = 1,
-- 
2.11.0