[PATCH 0/2] ima-evm-utils: Fix use of sign_hash via API

[PATCH 0/2] ima-evm-utils: Fix use of sign_hash via API

[Patrick Uiterwijk]

When using sign_hash, the resulting signature is incorrect if any hash
algorithm other than sha1 is used.
This is because while the sign_hash function has a hashalgo argument,
the sign_hash_v2 function does not actually use this argument for
anything except setting the hash_algo value in the header.
This patch makes sure it uses the algo variable consistently.

Patrick Uiterwijk (2):
  Fix sign_hash not observing the hashalgo argument
  Add test for using sign_hash API

 src/evmctl.c                | 23 ----------------
 src/libimaevm.c             |  4 +--
 src/utils.c                 | 20 ++++++++++++++
 src/utils.h                 |  1 +
 tests/.gitignore            |  2 ++
 tests/Makefile.am           |  5 ++++
 tests/sign_verify.apitest.c | 55 +++++++++++++++++++++++++++++++++++++
 tests/sign_verify.test      | 30 ++++++++++++++++----
 8 files changed, 109 insertions(+), 31 deletions(-)
 create mode 100644 tests/sign_verify.apitest.c

-- 
2.26.2

[PATCH 1/2] Fix sign_hash not observing the hashalgo argument

[Patrick Uiterwijk]

This fixes sign_hash not using the correct algorithm for creating the
signature, by ensuring it uses the passed in variable value.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
---
 src/libimaevm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index fa6c278..72d5e67 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -916,7 +916,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
 		return -1;
 	}
 
-	log_info("hash(%s): ", imaevm_params.hash_algo);
+	log_info("hash(%s): ", algo);
 	log_dump(hash, size);
 
 	pkey = read_priv_pkey(keyfile, imaevm_params.keypass);
@@ -942,7 +942,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
 	if (!EVP_PKEY_sign_init(ctx))
 		goto err;
 	st = "EVP_get_digestbyname";
-	if (!(md = EVP_get_digestbyname(imaevm_params.hash_algo)))
+	if (!(md = EVP_get_digestbyname(algo)))
 		goto err;
 	st = "EVP_PKEY_CTX_set_signature_md";
 	if (!EVP_PKEY_CTX_set_signature_md(ctx, md))
-- 
2.26.2

[PATCH 2/2] Add test for using sign_hash API

[Patrick Uiterwijk]

This adds a test with a small program that calls the sign_hash API, to
ensure that that codepath works.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
---
 src/evmctl.c                | 23 ----------------
 src/utils.c                 | 20 ++++++++++++++
 src/utils.h                 |  1 +
 tests/.gitignore            |  2 ++
 tests/Makefile.am           |  5 ++++
 tests/sign_verify.apitest.c | 55 +++++++++++++++++++++++++++++++++++++
 tests/sign_verify.test      | 30 ++++++++++++++++----
 7 files changed, 107 insertions(+), 29 deletions(-)
 create mode 100644 tests/sign_verify.apitest.c

diff --git a/src/evmctl.c b/src/evmctl.c
index 1815f55..bb51688 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -165,29 +165,6 @@ struct tpm_bank_info {
 static char *pcrfile[MAX_PCRFILE];
 static unsigned npcrfile;
 
-static int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
-{
-	FILE *fp;
-	char name[strlen(file) + (ext ? strlen(ext) : 0) + 2];
-	int err;
-
-	if (ext)
-		sprintf(name, "%s.%s", file, ext);
-	else
-		sprintf(name, "%s", file);
-
-	log_info("Writing to %s\n", name);
-
-	fp = fopen(name, "w");
-	if (!fp) {
-		log_err("Failed to open: %s\n", name);
-		return -1;
-	}
-	err = fwrite(data, len, 1, fp);
-	fclose(fp);
-	return err;
-}
-
 static unsigned char *file2bin(const char *file, const char *ext, int *size)
 {
 	FILE *fp;
diff --git a/src/utils.c b/src/utils.c
index fbb6a4b..6b99e78 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -112,3 +112,23 @@ int hex2bin(void *dst, const char *src, size_t count)
 	}
 	return 0;
 }
+
+int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
+{
+	FILE *fp;
+	char name[strlen(file) + (ext ? strlen(ext) : 0) + 2];
+	int err;
+
+	if (ext)
+		sprintf(name, "%s.%s", file, ext);
+	else
+		sprintf(name, "%s", file);
+
+	fp = fopen(name, "w");
+	if (!fp) {
+		return -1;
+	}
+	err = fwrite(data, len, 1, fp);
+	fclose(fp);
+	return err;
+}
diff --git a/src/utils.h b/src/utils.h
index 9ea179f..081997a 100644
--- a/src/utils.h
+++ b/src/utils.h
@@ -4,3 +4,4 @@
 int get_cmd_path(const char *prog_name, char *buf, size_t buf_len);
 int hex_to_bin(char ch);
 int hex2bin(void *dst, const char *src, size_t count);
+int bin2file(const char *file, const char *ext, const unsigned char *data, int len);
diff --git a/tests/.gitignore b/tests/.gitignore
index 9ecc984..c40735d 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -14,3 +14,5 @@
 *.key
 *.conf
 
+# Compiled version of apitest
+sign_verify_apitest
diff --git a/tests/Makefile.am b/tests/Makefile.am
index ff928e1..74f6125 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -10,3 +10,8 @@ distclean: distclean-keys
 .PHONY: distclean-keys
 distclean-keys:
 	./gen-keys.sh clean
+
+AUTOMAKE_OPTIONS = subdir-objects
+bin_PROGRAMS = sign_verify_apitest
+sign_verify_apitest_SOURCES = sign_verify.apitest.c ../src/utils.c
+sign_verify_apitest_LDFLAGS = -limaevm -L../src/.libs
diff --git a/tests/sign_verify.apitest.c b/tests/sign_verify.apitest.c
new file mode 100644
index 0000000..20e2160
--- /dev/null
+++ b/tests/sign_verify.apitest.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * sign_verify.apitest: Test program for verifying sign_hash
+ *
+ * Copyright (C) 2020 Patrick Uiterwijk <patrick@puiterwijk.org>
+ * Copyright (C) 2013,2014 Samsung Electronics
+ * Copyright (C) 2011,2012,2013 Intel Corporation
+ * Copyright (C) 2011 Nokia Corporation
+ * Copyright (C) 2010 Cyril Hrubis <chrubis@suse.cz>
+ */
+
+#include <assert.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/types.h>
+#include <attr/xattr.h>
+
+#include "../src/imaevm.h"
+#include "../src/utils.h"
+
+int main(int argc, char **argv) {
+	unsigned char hash[MAX_DIGEST_SIZE];
+	unsigned char sig[MAX_SIGNATURE_SIZE];
+	int len, err;
+    char *file = argv[1];
+    char *key = argv[2];
+    char *algo = argv[3];
+    char *digest = argv[4];
+
+    len = strlen(digest) / 2;
+    if (hex2bin(hash, digest, len) != 0) {
+        fprintf(stderr, "Error during hex2bin\n");
+        return 1;
+    }
+
+    len = sign_hash(algo, hash, len, key, NULL, sig + 1);
+	if (len <= 1) {
+        fprintf(stderr, "Error signing\n");
+        return 1;
+    }
+
+	/* add header */
+	len++;
+	sig[0] = EVM_IMA_XATTR_DIGSIG;
+
+    bin2file(file, "sig", sig, len);
+
+    err = lsetxattr(file, "user.ima", sig, len, 0);
+    if (err < 0) {
+        log_err("setxattr failed: %s\n", file);
+        return 1;
+    }
+
+	return 0;
+}
diff --git a/tests/sign_verify.test b/tests/sign_verify.test
index 288e133..e909d01 100755
--- a/tests/sign_verify.test
+++ b/tests/sign_verify.test
@@ -65,14 +65,14 @@ _keyid_from_cert() {
 
 # Convert test $type into evmctl op prefix
 _op() {
-  if [ "$1" = ima ]; then
+  if [ "$1" = ima -o "$1" = ima_api ]; then
     echo ima_
   fi
 }
 
 # Convert test $type into xattr name
 _xattr() {
-  if [ "$1" = ima ]; then
+  if [ "$1" = ima -o "$1" = ima_api ]; then
     echo user.ima
   else
     echo user.evm
@@ -112,11 +112,13 @@ _evmctl_sign() {
   [ "$type" = ima ] && opts+=" --sigfile"
 
   # shellcheck disable=SC2086
-  ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \
+  [ "$type" = ima -o "$type" = evm ] && (ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \
     _evmctl_run "$(_op "$type")sign" $opts \
-    --hashalgo "$alg" --key "$key" --xattr-user "$file" || return
+    --hashalgo "$alg" --key "$key" --xattr-user "$file" || return)
+  [ "$type" = ima_api ] && ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \
+    ./sign_verify_apitest "$file" "$key" "$alg" "$(openssl dgst $OPENSSL_ENGINE -$ALG -hex -r $FILE | awk '{print $1}')"
 
-  if [ "$type" = ima ]; then
+  if [ "$type" = ima -o "$type" = ima_api ]; then
     _test_sigfile "$file" "$(_xattr "$type")" "$file.sig" "$file.sig2"
   fi
 }
@@ -124,12 +126,14 @@ _evmctl_sign() {
 # Run and test {ima_,}sign operation
 check_sign() {
   # Arguments are passed via global vars:
-  # TYPE (ima or evm),
+  # TYPE (ima, ima_api or evm),
   # KEY,
   # ALG (hash algo),
   # PREFIX (signature header prefix in hex),
   # OPTS (additional options for evmctl),
   # FILE (working file to sign).
+  [ "$TYPE" = ima_api ] && [[ "$OPTS" =~ --rsa ]] && return "$SKIP"
+
   local "$@"
   local KEY=${KEY%.*}.key
   local FILE=${FILE:-$ALG.txt}
@@ -267,6 +271,20 @@ sign_verify() {
     # Multiple files and some don't verify
     expect_fail check_verify FILE="/dev/null $file"
 
+    setfattr -x user.ima "$FILE"
+    rm "$FILE.sig"
+  fi
+
+  TYPE=ima_api
+  if expect_pass check_sign; then
+
+    # Normal verify with proper key should pass
+    expect_pass check_verify
+    expect_pass check_verify OPTS="--sigfile"
+
+    # Multiple files and some don't verify
+    expect_fail check_verify FILE="/dev/null $file"
+
     rm "$FILE.sig"
   fi
 
-- 
2.26.2

Re: [PATCH 1/2] Fix sign_hash not observing the hashalgo argument

[Mimi Zohar]

Hi Patrick,

On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote:
> This fixes sign_hash not using the correct algorithm for creating the
> signature, by ensuring it uses the passed in variable value.
> 
> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>

Thank you.  This is a regression first introduced in commit
07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API").

Mimi

Re: [PATCH 2/2] Add test for using sign_hash API

[Mimi Zohar]

Hi Patrick,

On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote:
> This adds a test with a small program that calls the sign_hash API, to
> ensure that that codepath works.
> 
> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>

Thank you for the regression test.  Other than the couple of comments
below,

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

> ---

> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index ff928e1..74f6125 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -10,3 +10,8 @@ distclean: distclean-keys
>  .PHONY: distclean-keys
>  distclean-keys:
>  	./gen-keys.sh clean
> +
> +AUTOMAKE_OPTIONS = subdir-objects
> +bin_PROGRAMS = sign_verify_apitest
> +sign_verify_apitest_SOURCES = sign_verify.apitest.c ../src/utils.c
> +sign_verify_apitest_LDFLAGS = -limaevm -L../src/.libs
> diff --git a/tests/sign_verify.apitest.c b/tests/sign_verify.apitest.c
> new file mode 100644
> index 0000000..20e2160
> --- /dev/null
> +++ b/tests/sign_verify.apitest.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * sign_verify.apitest: Test program for verifying sign_hash
> + *
> + * Copyright (C) 2020 Patrick Uiterwijk <patrick@puiterwijk.org>
> + * Copyright (C) 2013,2014 Samsung Electronics
> + * Copyright (C) 2011,2012,2013 Intel Corporation
> + * Copyright (C) 2011 Nokia Corporation
> + * Copyright (C) 2010 Cyril Hrubis <chrubis@suse.cz>
> + */

Looking at the history, Dmitry Kasatkin must have been involved.  From
which software package is this from?

> +
> +#include <assert.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <attr/xattr.h>

This include fails on a couple of distros (e.g. alpine, tumbleweed, and
RHEL 8.3).  src/evmctl.c is using sys/xattr.h.

Mimi

> +
> +#include "../src/imaevm.h"
> +#include "../src/utils.h"

Re: [PATCH 2/2] Add test for using sign_hash API

[Vitaly Chikunov]

On Thu, Jan 07, 2021 at 07:25:03AM -0500, Mimi Zohar wrote:
> > diff --git a/tests/sign_verify.apitest.c b/tests/sign_verify.apitest.c
> > new file mode 100644
> > index 0000000..20e2160
> > --- /dev/null
> > +++ b/tests/sign_verify.apitest.c
> > @@ -0,0 +1,55 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * sign_verify.apitest: Test program for verifying sign_hash
> > + *
> > + * Copyright (C) 2020 Patrick Uiterwijk <patrick@puiterwijk.org>
> > + * Copyright (C) 2013,2014 Samsung Electronics
> > + * Copyright (C) 2011,2012,2013 Intel Corporation
> > + * Copyright (C) 2011 Nokia Corporation
> > + * Copyright (C) 2010 Cyril Hrubis <chrubis@suse.cz>
> > + */

Woah, so much copyright for a simple sign_hash() call?

> 
> Looking at the history, Dmitry Kasatkin must have been involved.  From
> which software package is this from?

Re: [PATCH 1/2] Fix sign_hash not observing the hashalgo argument

[Vitaly Chikunov]

Patrick, Mimi,

On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote:
> On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote:
> > This fixes sign_hash not using the correct algorithm for creating the
> > signature, by ensuring it uses the passed in variable value.
> > 
> > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
> 
> Thank you.  This is a regression first introduced in commit
> 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API").

Ah, when sign_hash() is used not via evmctl, but as a library
imaevm_params may be not set.

Thanks!

Re: [PATCH 1/2] Fix sign_hash not observing the hashalgo argument

[Vitaly Chikunov]

On Thu, Jan 07, 2021 at 04:08:16PM +0300, Vitaly Chikunov wrote:
> Patrick, Mimi,
> 
> On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote:
> > On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote:
> > > This fixes sign_hash not using the correct algorithm for creating the
> > > signature, by ensuring it uses the passed in variable value.
> > > 
> > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
> > 
> > Thank you.  This is a regression first introduced in commit
> > 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API").
> 
> Ah, when sign_hash() is used not via evmctl, but as a library
> imaevm_params may be not set.

Thinking about imaevm_params -- it's still belong to a library and
extensively used in libimaevm.c, so I wonder if the fix is perfect.
Since, now there is hash_algo and algo duplication which one to prefer
and why?

Maybe, it should be [also] set like keypass in sign_hash?

  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;

+	  if (hashalgo)
+		  imaevm_params.hash_algo = hashalgo;

	  return imaevm_params.x509 ?
		  sign_hash_v2(hashalgo, hash, size, keyfile, sig) :
		  sign_hash_v1(hashalgo, hash, size, keyfile, sig);
  }

Re: [PATCH 1/2] Fix sign_hash not observing the hashalgo argument

[Mimi Zohar]

On Thu, 2021-01-07 at 16:15 +0300, Vitaly Chikunov wrote:
> On Thu, Jan 07, 2021 at 04:08:16PM +0300, Vitaly Chikunov wrote:
> > Patrick, Mimi,
> > 
> > On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote:
> > > On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote:
> > > > This fixes sign_hash not using the correct algorithm for creating the
> > > > signature, by ensuring it uses the passed in variable value.
> > > > 
> > > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
> > > 
> > > Thank you.  This is a regression first introduced in commit
> > > 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API").
> > 
> > Ah, when sign_hash() is used not via evmctl, but as a library
> > imaevm_params may be not set.
> 
> Thinking about imaevm_params -- it's still belong to a library and
> extensively used in libimaevm.c, so I wonder if the fix is perfect.
> Since, now there is hash_algo and algo duplication which one to prefer
> and why?
> 
> Maybe, it should be [also] set like keypass in sign_hash?
> 
>   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;
> 
> +	  if (hashalgo)
> +		  imaevm_params.hash_algo = hashalgo;
> 
> 	  return imaevm_params.x509 ?
> 		  sign_hash_v2(hashalgo, hash, size, keyfile, sig) :
> 		  sign_hash_v1(hashalgo, hash, size, keyfile, sig);
>   }
> 
> 

As seen above, the main difference between keypass and hashalgo is that
hashalgo is being passed as an argument, while keypass isn't.  Instead
of changing the function arguments, it looks like keypass was stored as
global variable.  For this reason, the priority should be the function
argument, not the global variable.

thanks,

Mimi

Re: [PATCH 2/2] Add test for using sign_hash API

[Patrick Uiterwijk]

On Thu, Jan 7, 2021 at 1:54 PM Vitaly Chikunov <vt@altlinux.org> wrote:
>
> On Thu, Jan 07, 2021 at 07:25:03AM -0500, Mimi Zohar wrote:
> > > diff --git a/tests/sign_verify.apitest.c b/tests/sign_verify.apitest.c
> > > new file mode 100644
> > > index 0000000..20e2160
> > > --- /dev/null
> > > +++ b/tests/sign_verify.apitest.c
> > > @@ -0,0 +1,55 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * sign_verify.apitest: Test program for verifying sign_hash
> > > + *
> > > + * Copyright (C) 2020 Patrick Uiterwijk <patrick@puiterwijk.org>
> > > + * Copyright (C) 2013,2014 Samsung Electronics
> > > + * Copyright (C) 2011,2012,2013 Intel Corporation
> > > + * Copyright (C) 2011 Nokia Corporation
> > > + * Copyright (C) 2010 Cyril Hrubis <chrubis@suse.cz>
> > > + */
>
> Woah, so much copyright for a simple sign_hash() call?

I basically copied the copyright statements from evmctl.c (and put
them in the reversed order as per the new syntax as Mimi did in
utils.c as a fix to my previous patchset).
This was because the "int main" function started out as a copy of
"sign_ima" from evmctl.c.
The Samsung, Intel and Nokia entries are all from Dmitry Kasatkin.
Cyril's entry is there because I initially copied hex2bin from utils.c
before just linking against it, I'll remove that one in a next
version.

>
> >
> > Looking at the history, Dmitry Kasatkin must have been involved.  From
> > which software package is this from?

Re: [PATCH 1/2] Fix sign_hash not observing the hashalgo argument

[Patrick Uiterwijk]

On Thu, Jan 7, 2021 at 2:15 PM Vitaly Chikunov <vt@altlinux.org> wrote:
>
> On Thu, Jan 07, 2021 at 04:08:16PM +0300, Vitaly Chikunov wrote:
> > Patrick, Mimi,
> >
> > On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote:
> > > On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote:
> > > > This fixes sign_hash not using the correct algorithm for creating the
> > > > signature, by ensuring it uses the passed in variable value.
> > > >
> > > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
> > >
> > > Thank you.  This is a regression first introduced in commit
> > > 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API").
> >
> > Ah, when sign_hash() is used not via evmctl, but as a library
> > imaevm_params may be not set.
>
> Thinking about imaevm_params -- it's still belong to a library and
> extensively used in libimaevm.c, so I wonder if the fix is perfect.
> Since, now there is hash_algo and algo duplication which one to prefer
> and why?
>
> Maybe, it should be [also] set like keypass in sign_hash?

I had this exact diff as an initial version of this patch, but then
personally thought that because "hashalgo" is passed to sign_hash_v2
anyway, and sign_hash_v1 already prefers the argument (and ignores
imaevm_params.hash_algo), keeping this behaviour in sync is more
consistent.
With my patch, imaevm_params.hash_algo is only used in verify_hash_v2
and ima_calc_hash, both of which are primarily called by
ima_verify_signature which sets it, as a way to pass the argument
without having it explicit everywhere.

>
>   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;
>
> +         if (hashalgo)
> +                 imaevm_params.hash_algo = hashalgo;
>
>           return imaevm_params.x509 ?
>                   sign_hash_v2(hashalgo, hash, size, keyfile, sig) :
>                   sign_hash_v1(hashalgo, hash, size, keyfile, sig);
>   }
>
>

[PATCH ima-evm-utils v2 0/2] Fix use of sign_hash via API

[Patrick Uiterwijk]

When using sign_hash, the resulting signature is incorrect if any hash
algorithm other than sha1 is used.
This is because while the sign_hash function has a hashalgo argument,
the sign_hash_v2 function does not actually use this argument for
anything except setting the hash_algo value in the header.
This patch makes sure it uses the algo variable consistently.

Changes since v1:
  - Using sys/xattr.h
  - Removed copyright line for sign_ima

Patrick Uiterwijk (2):
  Fix sign_hash not observing the hashalgo argument
  Add test for using sign_hash API

 src/evmctl.c                | 23 ----------------
 src/libimaevm.c             |  4 +--
 src/utils.c                 | 20 ++++++++++++++
 src/utils.h                 |  1 +
 tests/.gitignore            |  2 ++
 tests/Makefile.am           |  5 ++++
 tests/sign_verify.apitest.c | 55 +++++++++++++++++++++++++++++++++++++
 tests/sign_verify.test      | 30 ++++++++++++++++----
 8 files changed, 109 insertions(+), 31 deletions(-)
 create mode 100644 tests/sign_verify.apitest.c

-- 
2.31.1

[PATCH ima-evm-utils v2 1/2] Fix sign_hash not observing the hashalgo argument

[Patrick Uiterwijk]

This fixes sign_hash not using the correct algorithm for creating the
signature, by ensuring it uses the passed in variable value.

Fixes: 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API").
Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
---
 src/libimaevm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index 06f1063..2856270 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -913,7 +913,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
 		return -1;
 	}
 
-	log_info("hash(%s): ", imaevm_params.hash_algo);
+	log_info("hash(%s): ", algo);
 	log_dump(hash, size);
 
 	pkey = read_priv_pkey(keyfile, imaevm_params.keypass);
@@ -939,7 +939,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
 	if (!EVP_PKEY_sign_init(ctx))
 		goto err;
 	st = "EVP_get_digestbyname";
-	if (!(md = EVP_get_digestbyname(imaevm_params.hash_algo)))
+	if (!(md = EVP_get_digestbyname(algo)))
 		goto err;
 	st = "EVP_PKEY_CTX_set_signature_md";
 	if (!EVP_PKEY_CTX_set_signature_md(ctx, md))
-- 
2.31.1

[PATCH ima-evm-utils v2 2/2] Add test for using sign_hash API

[Patrick Uiterwijk]

This adds a test with a small program that calls the sign_hash API, to
ensure that that codepath works.

Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c                | 23 ----------------
 src/utils.c                 | 20 ++++++++++++++
 src/utils.h                 |  1 +
 tests/.gitignore            |  2 ++
 tests/Makefile.am           |  5 ++++
 tests/sign_verify.apitest.c | 55 +++++++++++++++++++++++++++++++++++++
 tests/sign_verify.test      | 30 ++++++++++++++++----
 7 files changed, 107 insertions(+), 29 deletions(-)
 create mode 100644 tests/sign_verify.apitest.c

diff --git a/src/evmctl.c b/src/evmctl.c
index 7a6f202..867b142 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -166,29 +166,6 @@ struct tpm_bank_info {
 static char *pcrfile[MAX_PCRFILE];
 static unsigned npcrfile;
 
-static int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
-{
-	FILE *fp;
-	char name[strlen(file) + (ext ? strlen(ext) : 0) + 2];
-	int err;
-
-	if (ext)
-		sprintf(name, "%s.%s", file, ext);
-	else
-		sprintf(name, "%s", file);
-
-	log_info("Writing to %s\n", name);
-
-	fp = fopen(name, "w");
-	if (!fp) {
-		log_err("Failed to open: %s\n", name);
-		return -1;
-	}
-	err = fwrite(data, len, 1, fp);
-	fclose(fp);
-	return err;
-}
-
 static unsigned char *file2bin(const char *file, const char *ext, int *size)
 {
 	FILE *fp;
diff --git a/src/utils.c b/src/utils.c
index fbb6a4b..ce2cc28 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -112,3 +112,23 @@ int hex2bin(void *dst, const char *src, size_t count)
 	}
 	return 0;
 }
+
+int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
+{
+	FILE *fp;
+	char name[strlen(file) + (ext ? strlen(ext) : 0) + 2];
+	int err;
+
+	if (ext)
+		sprintf(name, "%s.%s", file, ext);
+	else
+		sprintf(name, "%s", file);
+
+	fp = fopen(name, "w");
+	if (!fp)
+		return -1;
+
+	err = fwrite(data, len, 1, fp);
+	fclose(fp);
+	return err;
+}
diff --git a/src/utils.h b/src/utils.h
index 9ea179f..081997a 100644
--- a/src/utils.h
+++ b/src/utils.h
@@ -4,3 +4,4 @@
 int get_cmd_path(const char *prog_name, char *buf, size_t buf_len);
 int hex_to_bin(char ch);
 int hex2bin(void *dst, const char *src, size_t count);
+int bin2file(const char *file, const char *ext, const unsigned char *data, int len);
diff --git a/tests/.gitignore b/tests/.gitignore
index 9ecc984..c40735d 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -14,3 +14,5 @@
 *.key
 *.conf
 
+# Compiled version of apitest
+sign_verify_apitest
diff --git a/tests/Makefile.am b/tests/Makefile.am
index ff928e1..74f6125 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -10,3 +10,8 @@ distclean: distclean-keys
 .PHONY: distclean-keys
 distclean-keys:
 	./gen-keys.sh clean
+
+AUTOMAKE_OPTIONS = subdir-objects
+bin_PROGRAMS = sign_verify_apitest
+sign_verify_apitest_SOURCES = sign_verify.apitest.c ../src/utils.c
+sign_verify_apitest_LDFLAGS = -limaevm -L../src/.libs
diff --git a/tests/sign_verify.apitest.c b/tests/sign_verify.apitest.c
new file mode 100644
index 0000000..3fcd043
--- /dev/null
+++ b/tests/sign_verify.apitest.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * sign_verify.apitest: Test program for verifying sign_hash
+ *
+ * Copyright (C) 2021 Patrick Uiterwijk <patrick@puiterwijk.org>
+ * Copyright (C) 2013,2014 Samsung Electronics
+ * Copyright (C) 2011,2012,2013 Intel Corporation
+ * Copyright (C) 2011 Nokia Corporation
+ */
+
+#include <assert.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/xattr.h>
+
+#include "../src/imaevm.h"
+#include "../src/utils.h"
+
+int main(int argc, char **argv)
+{
+	unsigned char hash[MAX_DIGEST_SIZE];
+	unsigned char sig[MAX_SIGNATURE_SIZE];
+	int len, err;
+	char *file = argv[1];
+	char *key = argv[2];
+	char *algo = argv[3];
+	char *digest = argv[4];
+
+	len = strlen(digest) / 2;
+	if (hex2bin(hash, digest, len) != 0) {
+		fprintf(stderr, "Error during hex2bin\n");
+		return 1;
+	}
+
+	len = sign_hash(algo, hash, len, key, NULL, sig + 1);
+	if (len <= 1) {
+		fprintf(stderr, "Error signing\n");
+		return 1;
+	}
+
+	/* add header */
+	len++;
+	sig[0] = EVM_IMA_XATTR_DIGSIG;
+
+	bin2file(file, "sig", sig, len);
+
+	err = lsetxattr(file, "user.ima", sig, len, 0);
+	if (err < 0) {
+		log_err("setxattr failed: %s\n", file);
+		return 1;
+	}
+
+	return 0;
+}
diff --git a/tests/sign_verify.test b/tests/sign_verify.test
index 3d7aa51..6f92801 100755
--- a/tests/sign_verify.test
+++ b/tests/sign_verify.test
@@ -66,14 +66,14 @@ _keyid_from_cert() {
 
 # Convert test $type into evmctl op prefix
 _op() {
-  if [ "$1" = ima ]; then
+  if [ "$1" = ima -o "$1" = ima_api ]; then
     echo ima_
   fi
 }
 
 # Convert test $type into xattr name
 _xattr() {
-  if [ "$1" = ima ]; then
+  if [ "$1" = ima -o "$1" = ima_api ]; then
     echo user.ima
   else
     echo user.evm
@@ -113,11 +113,13 @@ _evmctl_sign() {
   [ "$type" = ima ] && opts+=" --sigfile"
 
   # shellcheck disable=SC2086
-  ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \
+  [ "$type" = ima -o "$type" = evm ] && (ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \
     _evmctl_run "$(_op "$type")sign" $opts \
-    --hashalgo "$alg" --key "$key" --xattr-user "$file" || return
+    --hashalgo "$alg" --key "$key" --xattr-user "$file" || return)
+  [ "$type" = ima_api ] && ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \
+    ./sign_verify_apitest "$file" "$key" "$alg" "$(openssl dgst $OPENSSL_ENGINE -$ALG -hex -r $FILE | awk '{print $1}')"
 
-  if [ "$type" = ima ]; then
+  if [ "$type" = ima -o "$type" = ima_api ]; then
     _test_sigfile "$file" "$(_xattr "$type")" "$file.sig" "$file.sig2"
   fi
 }
@@ -125,12 +127,14 @@ _evmctl_sign() {
 # Run and test {ima_,}sign operation
 check_sign() {
   # Arguments are passed via global vars:
-  # TYPE (ima or evm),
+  # TYPE (ima, ima_api or evm),
   # KEY,
   # ALG (hash algo),
   # PREFIX (signature header prefix in hex),
   # OPTS (additional options for evmctl),
   # FILE (working file to sign).
+  [ "$TYPE" = ima_api ] && [[ "$OPTS" =~ --rsa ]] && return "$SKIP"
+
   local "$@"
   local KEY=${KEY%.*}.key
   local FILE=${FILE:-$ALG.txt}
@@ -268,6 +272,20 @@ sign_verify() {
     # Multiple files and some don't verify
     expect_fail check_verify FILE="/dev/null $file"
 
+    setfattr -x user.ima "$FILE"
+    rm "$FILE.sig"
+  fi
+
+  TYPE=ima_api
+  if expect_pass check_sign; then
+
+    # Normal verify with proper key should pass
+    expect_pass check_verify
+    expect_pass check_verify OPTS="--sigfile"
+
+    # Multiple files and some don't verify
+    expect_fail check_verify FILE="/dev/null $file"
+
     rm "$FILE.sig"
   fi
 
-- 
2.31.1

Re: [PATCH ima-evm-utils v2 2/2] Add test for using sign_hash API

[Mimi Zohar]

Hi Patrick,

On Mon, 2021-07-05 at 17:49 +0200, Patrick Uiterwijk wrote:
> This adds a test with a small program that calls the sign_hash API, to
> ensure that that codepath works.
> 
> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

Somehow I missed that running the test without this patch the summary
"SKIP" info matches the "skipped" messages, but running with the patch
there's a discrepancy.

$ ./sign_verify.test &> /tmp/sign_verify.log
$ tail -5  /tmp/sign_verify.log
evmctl ima_sign failed properly with (1) 
  EVP_get_digestbyname(md_gost12_512) failed
  errno: No such file or directory (2)
PASS: 127 SKIP: 20 FAIL: 0

$ grep "skipped" /tmp/sign_verify.log | wc -l
20

$ tail -5  /tmp/sign_verify.log-api
evmctl ima_sign: no detached signature md_gost12_512.txt~.sig

rm: cannot remove 'md_gost12_512.txt~': No such file or directory
PASS: 175 SKIP: 32 FAIL: 0

$ grep "skipped" /tmp/sign_verify.log-api | wc -l
30

> diff --git a/tests/sign_verify.apitest.c b/tests/sign_verify.apitest.c
> new file mode 100644
> index 0000000..3fcd043
> --- /dev/null
> +++ b/tests/sign_verify.apitest.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * sign_verify.apitest: Test program for verifying sign_hash
> + *
> + * Copyright (C) 2021 Patrick Uiterwijk <patrick@puiterwijk.org>
> + * Copyright (C) 2013,2014 Samsung Electronics
> + * Copyright (C) 2011,2012,2013 Intel Corporation
> + * Copyright (C) 2011 Nokia Corporation
> + */

As this is a new file and test, the copyrights other than your own are
unnecessary.

> +
> +#include <assert.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/xattr.h>
> +
> +#include "../src/imaevm.h"
> +#include "../src/utils.h"
> +
> +int main(int argc, char **argv)
> +{
> +	unsigned char hash[MAX_DIGEST_SIZE];
> +	unsigned char sig[MAX_SIGNATURE_SIZE];
> +	int len, err;
> +	char *file = argv[1];
> +	char *key = argv[2];
> +	char *algo = argv[3];
> +	char *digest = argv[4];
> +

How about testing 'argc' before continuing?

> +	len = strlen(digest) / 2;
> +	if (hex2bin(hash, digest, len) != 0) {
> +		fprintf(stderr, "Error during hex2bin\n");
> +		return 1;
> +	}
> +
> +	len = sign_hash(algo, hash, len, key, NULL, sig + 1);
> +	if (len <= 1) {
> +		fprintf(stderr, "Error signing\n");
> +		return 1;
> +	}
> +
> +	/* add header */
> +	len++;
> +	sig[0] = EVM_IMA_XATTR_DIGSIG;
> +
> +	bin2file(file, "sig", sig, len);
> +
> +	err = lsetxattr(file, "user.ima", sig, len, 0);
> +	if (err < 0) {
> +		log_err("setxattr failed: %s\n", file);
> +		return 1;
> +	}
> +
> +	return 0;
> +}


> diff --git a/tests/sign_verify.test b/tests/sign_verify.test
> index 3d7aa51..6f92801 100755
> --- a/tests/sign_verify.test
> +++ b/tests/sign_verify.test

> @@ -125,12 +127,14 @@ _evmctl_sign() {
>  # Run and test {ima_,}sign operation
>  check_sign() {
>    # Arguments are passed via global vars:
> -  # TYPE (ima or evm),
> +  # TYPE (ima, ima_api or evm),

Similarly TYPE should be updated in verify_sign as well.

thanks,

Mimi

>    # KEY,
>    # ALG (hash algo),
>    # PREFIX (signature header prefix in hex),
>    # OPTS (additional options for evmctl),
>    # FILE (working file to sign).
> +  [ "$TYPE" = ima_api ] && [[ "$OPTS" =~ --rsa ]] && return "$SKIP"
> +
>    local "$@"
>    local KEY=${KEY%.*}.key
>    local FILE=${FILE:-$ALG.txt}
> @@ -268,6 +272,20 @@ sign_verify() {
>      # Multiple files and some don't verify
>      expect_fail check_verify FILE="/dev/null $file"
>  
> +    setfattr -x user.ima "$FILE"
> +    rm "$FILE.sig"
> +  fi
> +
> +  TYPE=ima_api
> +  if expect_pass check_sign; then
> +
> +    # Normal verify with proper key should pass
> +    expect_pass check_verify
> +    expect_pass check_verify OPTS="--sigfile"
> +
> +    # Multiple files and some don't verify
> +    expect_fail check_verify FILE="/dev/null $file"
> +
>      rm "$FILE.sig"
>    fi
>