[PATCH ima-evm-utils v4 00/17] address deprecated warnings

[PATCH ima-evm-utils v4 00/17] address deprecated warnings

[Mimi Zohar]

Between travis/ci and OpenSSL v3 a large number of deprecated warnings
are being emitted when compiling ima-evm-utils.  Start addressing these
deprecated warnings by replacing the low level SHA1 and HMAC calls with
the EVP_ functions.  IMA signature version 1 also uses low level calls,
but instead of fixing it, deprecate it as nobody should be using it
anyway.

OpenSSL v3 "engine" support is deprecated and replaced with "providers".
Engine support will continue to work for a while, but results in
deprecated declaration and other messages.  Define a "--disable-engine"
and the equivalent "--enable-engine=no" configuration option.

Changelog v4:
Based on Vitaly's comments:
- instead of setting errno to zero, define a macro to log a message
with the errno and reset errno.
- verify ENGINE_init symbol is defined in libcrypto.
- disable engine support if either OPENSSL_NO_DYNAMIC_ENGINE or
OPENSSL_NO_ENGINE variables are defined.
- Rename CONFIG_ENGINE to CONFIG_IMA_EVM_ENGINE

Changelog v3:
- Make the SM2/SM3 tests dependent on the OpenSSL v3, rather than
compiling OpenSSL v3, based on Vitaly's suggestion.
- Re-use the existing infrastructure for compiling OpenSSL to compile
OpenSSL without engine support.  ima-evm-utils compiles cleanly without
any deprecated messages.
- Instead of using the distro OpenSSL version on jammy, compile OpenSSL
to test building ima-evm-utils without OpenSSL engine support.
- Make sure the keyfile is a regular file before using it.
- Based on Vitaly's suggestion: undefine CONFIG_ENGINE when OpenSSL
engine support is not configured, run the tests normally without
checking whether engine support is configured.
- Add Stefan's Reviewed-by tags.

Changelog v2:
- Based on Vitaly's comments, base enabling engine support on
OPENSSL_NO_ENGINE/OPENSSL_NO_DYNAMIC_ENGINE support.  Also don't limit
disabling ima-evm-utils engine support to v3, make it generic.
- Added Stefan's Reviewed-by tags.

Changelog v1:
- Based on Stefan's comments, removed deprecated functions when not
used and added missing word. Updated the usage and options accordingly.
- Based on Vitaly's comments, explicitly require "--disable-engine"
configuration to compile ima-evm-utils without OpenSSL v3 engine support
and typo.
- Based on Petr's comments, addressed the "return 77" by removing it,
  updated the travis patch description, and added his Reviewed-by tags.

Mimi Zohar (17):
  Revert "Reset 'errno' after failure to open or access a file"
  log and reset 'errno' after failure to open non-critical files
  Log and reset 'errno' on lsetxattr failure
  travis: update dist=focal
  Update configure.ac to address a couple of obsolete warnings
  Deprecate IMA signature version 1
  Replace the low level SHA1 calls when calculating the TPM 1.2 PCRs
  Replace the low level HMAC calls when calculating the EVM HMAC
  Add missing EVP_MD_CTX_free() call in calc_evm_hash()
  Disable use of OpenSSL "engine" support
  Fix potential use after free in read_tpm_banks()
  Limit the file hash algorithm name length
  Missing template data size lower bounds checking
  Base sm2/sm3 test on openssl version installed
  Compile a newer version of OpenSSL
  Build OpenSSL without engine support
  Fix d2i_x509_fp failure

 .github/workflows/ci.yml  |  10 +-
 .travis.yml               |  10 +-
 acinclude.m4              |   2 +-
 build.sh                  |   8 ++
 configure.ac              |  16 ++-
 m4/manpage-docbook-xsl.m4 |   2 +-
 src/Makefile.am           |  18 +++
 src/evmctl.c              | 282 ++++++++++++++++++++++++++------------
 src/imaevm.h              |   6 +
 src/libimaevm.c           |  44 ++++--
 tests/install-openssl3.sh |   9 +-
 tests/sign_verify.test    |  25 ++--
 12 files changed, 311 insertions(+), 121 deletions(-)

-- 
2.31.1

[PATCH ima-evm-utils v4 01/17] Revert "Reset 'errno' after failure to open or access a file"

[Mimi Zohar]

This reverts commit acb19d1894a4a95471b8d2346cd6c3ecf3385110.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c    | 18 ++----------------
 src/libimaevm.c |  4 ----
 2 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 76e2561798fa..0412bc0ac2b0 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -182,7 +182,6 @@ static int bin2file(const char *file, const char *ext, const unsigned char *data
 	fp = fopen(name, "w");
 	if (!fp) {
 		log_err("Failed to open: %s\n", name);
-		errno = 0;
 		return -1;
 	}
 	err = fwrite(data, len, 1, fp);
@@ -208,7 +207,6 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
 	fp = fopen(name, "r");
 	if (!fp) {
 		log_err("Failed to open: %s\n", name);
-		errno = 0;
 		return NULL;
 	}
 	if (fstat(fileno(fp), &stats) == -1) {
@@ -315,10 +313,8 @@ static int get_uuid(struct stat *st, char *uuid)
 	sprintf(path, "blkid -s UUID -o value /dev/block/%u:%u", major, minor);
 
 	fp = popen(path, "r");
-	if (!fp) {
-		errno = 0;
+	if (!fp)
 		goto err;
-	}
 
 	len = fread(_uuid, 1, sizeof(_uuid), fp);
 	pclose(fp);
@@ -375,7 +371,6 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 
 			if (fd < 0) {
 				log_err("Failed to open: %s\n", file);
-				errno = 0;
 				return -1;
 			}
 			if (ioctl(fd, FS_IOC_GETVERSION, &generation)) {
@@ -1201,7 +1196,6 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 
 		if (fd < 0) {
 			log_err("Failed to open %s\n", file);
-			errno = 0;
 			goto out;
 		}
 		if (ioctl(fd, FS_IOC_GETVERSION, &generation)) {
@@ -1392,7 +1386,6 @@ static int ima_fix(const char *path)
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
 		log_errno("Failed to open file: %s", path);
-		errno = 0;
 		return -1;
 	}
 
@@ -1916,10 +1909,8 @@ static int read_sysfs_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
 	int i, result;
 
 	fp = fopen(pcrs, "r");
-	if (!fp) {
+	if (!fp)
 		fp = fopen(misc_pcrs, "r");
-		errno = 0;
-	}
 	if (!fp)
 		return -1;
 
@@ -1982,7 +1973,6 @@ static int read_file_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
 		fp = fopen(path, "r");
 		if (!fp) {
 			log_err("Could not open '%s'\n", path);
-			errno = 0;
 			return -1;
 		}
 
@@ -2075,7 +2065,6 @@ static int ima_measurement(const char *file)
 	fp = fopen(file, "rb");
 	if (!fp) {
 		log_err("Failed to open measurement file: %s\n", file);
-		errno = 0;
 		return -1;
 	}
 
@@ -2321,7 +2310,6 @@ static int read_binary_bios_measurements(char *file, struct tpm_bank_info *bank)
 	fp = fopen(file, "r");
 	if (!fp) {
 		log_errno("Failed to open TPM 1.2 event log.\n");
-		errno = 0;
 		return 1;
 	}
 
@@ -2758,8 +2746,6 @@ int main(int argc, char *argv[])
 	unsigned long keyid;
 	char *eptr;
 
-	errno = 0;	/* initialize errno */
-
 #if !(OPENSSL_VERSION_NUMBER < 0x10100000)
 	OPENSSL_init_crypto(
 #ifndef DISABLE_OPENSSL_CONF
diff --git a/src/libimaevm.c b/src/libimaevm.c
index e4b62b4989b2..611631c57f14 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -144,7 +144,6 @@ static int add_file_hash(const char *file, EVP_MD_CTX *ctx)
 	fp = fopen(file, "r");
 	if (!fp) {
 		log_err("Failed to open: %s\n", file);
-		errno = 0;
 		return -1;
 	}
 
@@ -259,7 +258,6 @@ EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
 	if (!fp) {
 		if (imaevm_params.verbose > LOG_INFO)
 			log_info("Failed to open keyfile: %s\n", keyfile);
-		errno = 0;
 		return NULL;
 	}
 
@@ -878,7 +876,6 @@ static int read_keyid_from_cert(uint32_t *keyid_be, const char *certfile, int tr
 
 	if (!(fp = fopen(certfile, "r"))) {
 		log_err("Cannot open %s: %s\n", certfile, strerror(errno));
-		errno = 0;
 		return -1;
 	}
 	if (!PEM_read_X509(fp, &x, NULL, NULL)) {
@@ -970,7 +967,6 @@ static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
 		fp = fopen(keyfile, "r");
 		if (!fp) {
 			log_err("Failed to open keyfile: %s\n", keyfile);
-			errno = 0;
 			return NULL;
 		}
 		pkey = PEM_read_PrivateKey(fp, NULL, NULL, (void *)keypass);
-- 
2.31.1

[PATCH ima-evm-utils v4 02/17] log and reset 'errno' after failure to open non-critical files

[Mimi Zohar]

Define a log_errno_reset macro to emit the errno string at or near the
time of error, similar to the existing log_errno macro, but also reset
errno to avoid dangling or duplicate errno messages on exit.

The initial usage is for non-critical file open failures.

Suggested-by: Vitaly Chikunov <vt@altlinux.org>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 0412bc0ac2b0..54123bf20f03 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -166,6 +166,9 @@ struct tpm_bank_info {
 static char *pcrfile[MAX_PCRFILE];
 static unsigned npcrfile;
 
+#define log_errno_reset(level, fmt, args...) \
+	{do_log(level, fmt " (errno: %s)\n", ##args, strerror(errno)); errno = 0; }
+
 static int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
 {
 	FILE *fp;
@@ -1911,8 +1914,10 @@ static int read_sysfs_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
 	fp = fopen(pcrs, "r");
 	if (!fp)
 		fp = fopen(misc_pcrs, "r");
-	if (!fp)
+	if (!fp) {
+		log_errno_reset(LOG_DEBUG, "Failed to read TPM 1.2 PCRs");
 		return -1;
+	}
 
 	result = read_one_bank(&tpm_banks[0], fp);
 	fclose(fp);
@@ -2055,7 +2060,6 @@ static int ima_measurement(const char *file)
 	int err_padded = -1;
 	int err = -1;
 
-	errno = 0;
 	memset(zero, 0, MAX_DIGEST_SIZE);
 
 	pseudo_padded_banks = init_tpm_banks(&num_banks);
@@ -2072,6 +2076,8 @@ static int ima_measurement(const char *file)
 		init_public_keys(imaevm_params.keyfile);
 	else				/* assume read pubkey from x509 cert */
 		init_public_keys("/etc/keys/x509_evm.der");
+	if (errno)
+		log_errno_reset(LOG_DEBUG, "Failed to initialize public keys");
 
 	/*
 	 * Reading the PCRs before walking the IMA measurement list
@@ -2746,6 +2752,8 @@ int main(int argc, char *argv[])
 	unsigned long keyid;
 	char *eptr;
 
+	errno = 0;	/* initialize global errno */
+
 #if !(OPENSSL_VERSION_NUMBER < 0x10100000)
 	OPENSSL_init_crypto(
 #ifndef DISABLE_OPENSSL_CONF
-- 
2.31.1

[PATCH ima-evm-utils v4 03/17] Log and reset 'errno' on lsetxattr failure

[Mimi Zohar]

Writing either security.ima hashes or security.evm hmacs from userspace
will fail regardless of the IMA or EVM fix mode.  In fix mode, 'touch'
will force security.ima and security.evm to be updated.

Make the setxattr error messages more explicit and clear errno.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 54123bf20f03..b1dcd9b1c1ef 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -572,7 +572,7 @@ static int sign_evm(const char *file, const char *key)
 	if (xattr) {
 		err = lsetxattr(file, xattr_evm, sig, len, 0);
 		if (err < 0) {
-			log_err("setxattr failed: %s\n", file);
+			log_errno_reset(LOG_ERR, "Set EVM xattr failed: %s", file);
 			return err;
 		}
 	}
@@ -615,7 +615,8 @@ static int hash_ima(const char *file)
 	if (xattr) {
 		err = lsetxattr(file, xattr_ima, hash, len, 0);
 		if (err < 0) {
-			log_err("setxattr failed: %s\n", file);
+			log_errno_reset(LOG_ERR, "Set IMA hash xattr failed: %s",
+					file);
 			return err;
 		}
 	}
@@ -652,7 +653,8 @@ static int sign_ima(const char *file, const char *key)
 	if (xattr) {
 		err = lsetxattr(file, xattr_ima, sig, len, 0);
 		if (err < 0) {
-			log_err("setxattr failed: %s\n", file);
+			log_errno_reset(LOG_ERR, "Set IMA sig xattr failed: %s",
+					file);
 			return err;
 		}
 	}
@@ -1125,7 +1127,7 @@ static int setxattr_ima(const char *file, char *sig_file)
 
 	err = lsetxattr(file, xattr_ima, sig, len, 0);
 	if (err < 0)
-		log_err("setxattr failed: %s\n", file);
+		log_errno_reset(LOG_ERR, "Set IMA sig xattr failed: %s", file);
 	free(sig);
 	return err;
 }
@@ -1323,7 +1325,8 @@ static int hmac_evm(const char *file, const char *key)
 		sig[0] = EVM_XATTR_HMAC;
 		err = lsetxattr(file, xattr_evm, sig, len + 1, 0);
 		if (err < 0) {
-			log_err("setxattr failed: %s\n", file);
+			log_errno_reset(LOG_ERR, "Set EVM hmac xattr failed: %s",
+					file);
 			return err;
 		}
 	}
-- 
2.31.1

[PATCH ima-evm-utils v4 04/17] travis: update dist=focal

[Mimi Zohar]

Although Github Actions is available on Github Enterprise Server 3.x
single server edition, as well as the unpaid version, it is not
available in Github Enterprise Server 3.x cluster edition[1].

Continue updating travis.yml.

[1] https://docs.github.com/en/enterprise-server@3.0/admin/release-notes#github-packages

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 .travis.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 5741116e418a..edd2a21b83d3 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,6 +1,6 @@
 # Copyright (c) 2017-2021 Petr Vorel <pvorel@suse.cz>
 
-dist: bionic
+dist: focal
 language: C
 services:
     - docker
-- 
2.31.1

[PATCH ima-evm-utils v4 05/17] Update configure.ac to address a couple of obsolete warnings

[Mimi Zohar]

Remove AC_PROG_LIBTOOL and AC_HEAD_STDC. Replace AC_HELP_STRING with
AS_HELP_STRING.

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 acinclude.m4              | 2 +-
 configure.ac              | 4 ----
 m4/manpage-docbook-xsl.m4 | 2 +-
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index dd430d4f0565..bb962f81a9c0 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -2,7 +2,7 @@
 AC_DEFUN([PKG_ARG_ENABLE],
 	[
 	AC_MSG_CHECKING(whether to enable $1)
-	AC_ARG_ENABLE([$1], AC_HELP_STRING([--enable-$1], [enable $1 (default is $2)]), 
+	AC_ARG_ENABLE([$1], AS_HELP_STRING([--enable-$1], [enable $1 (default is $2)]),
 	[pkg_cv_enable_$1=$enableval],	
 	[AC_CACHE_VAL([pkg_cv_enable_$1], [pkg_cv_enable_$1=$2])])
 	if test $pkg_cv_enable_$1 = yes; then
diff --git a/configure.ac b/configure.ac
index 1a0f093df562..9d3b23ff8def 100644
--- a/configure.ac
+++ b/configure.ac
@@ -15,16 +15,12 @@ AM_PROG_CC_C_O
 #AC_PROG_CXX
 #AC_PROG_CPP
 AC_PROG_INSTALL
-AC_PROG_LIBTOOL
 #AC_PROG_LN_S
 LT_INIT
 
 # FIXME: Replace `main' with a function in `-lpthread':
 #AC_CHECK_LIB([pthread], [main])
 
-# Checks for header files.
-AC_HEADER_STDC
-
 PKG_CHECK_MODULES(LIBCRYPTO, [libcrypto >= 0.9.8 ])
 AC_SUBST(KERNEL_HEADERS)
 AC_CHECK_HEADER(unistd.h)
diff --git a/m4/manpage-docbook-xsl.m4 b/m4/manpage-docbook-xsl.m4
index 25c8ce54b068..f2ee912ed1be 100644
--- a/m4/manpage-docbook-xsl.m4
+++ b/m4/manpage-docbook-xsl.m4
@@ -7,7 +7,7 @@ AC_DEFUN([EVMCTL_MANPAGE_DOCBOOK_XSL], [
 
 	AC_PATH_PROGS(XMLCATALOG, xmlcatalog)
 	AC_ARG_WITH([xml-catalog],
-		AC_HELP_STRING([--with-xml-catalog=CATALOG],
+		AS_HELP_STRING([--with-xml-catalog=CATALOG],
 				[path to xml catalog to use]),,
 				[with_xml_catalog=/etc/xml/catalog])
 	XML_CATALOG_FILE="$with_xml_catalog"
-- 
2.31.1

[PATCH ima-evm-utils v4 06/17] Deprecate IMA signature version 1

[Mimi Zohar]

The original IMA file signatures were based on a SHA1 hash.  Kernel
support for other hash algorithms was subsequently upstreamed.  Deprecate
"--rsa" support.

Define "--enable-sigv1" option to configure signature v1 support.

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 configure.ac           |  6 ++++++
 src/Makefile.am        | 10 ++++++++++
 src/evmctl.c           | 20 ++++++++++++++++----
 src/libimaevm.c        | 22 +++++++++++++++++++---
 tests/sign_verify.test | 21 +++++++++++++++------
 5 files changed, 66 insertions(+), 13 deletions(-)

diff --git a/configure.ac b/configure.ac
index 9d3b23ff8def..dc666f2bb1fa 100644
--- a/configure.ac
+++ b/configure.ac
@@ -49,6 +49,11 @@ AC_ARG_ENABLE([openssl_conf],
 		AC_DEFINE(DISABLE_OPENSSL_CONF, 1, [Define to disable loading of openssl config by evmctl.])
 	      fi], [enable_openssl_conf=yes])
 
+AC_ARG_ENABLE(sigv1,
+	      AS_HELP_STRING([--enable-sigv1], [Build ima-evm-utils with signature v1 support]))
+	AM_CONDITIONAL([CONFIG_SIGV1], [test "x$enable_sigv1" = "xyes"])
+	AS_IF([test "$enable_sigv1"  != "yes"], [enable_sigv1="no"])
+
 #debug support - yes for a while
 PKG_ARG_ENABLE(debug, "yes", DEBUG, [Enable Debug support])
 if test $pkg_cv_enable_debug = yes; then
@@ -83,5 +88,6 @@ echo	"   openssl-conf: $enable_openssl_conf"
 echo	"      tss2-esys: $ac_cv_lib_tss2_esys_Esys_Free"
 echo	" tss2-rc-decode: $ac_cv_lib_tss2_rc_Tss2_RC_Decode"
 echo    "         ibmtss: $ac_cv_header_ibmtss_tss_h"
+echo    "         sigv1:  $enable_sigv1"
 echo	"            doc: $have_doc"
 echo
diff --git a/src/Makefile.am b/src/Makefile.am
index 396496bb439d..90c7249020cf 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -7,6 +7,10 @@ libimaevm_la_CPPFLAGS = $(AM_CPPFLAGS) $(LIBCRYPTO_CFLAGS)
 libimaevm_la_LDFLAGS = -version-info 3:0:0
 libimaevm_la_LIBADD =  $(LIBCRYPTO_LIBS)
 
+if CONFIG_SIGV1
+libimaevm_la_CFLAGS = -DCONFIG_SIGV1
+endif
+
 include_HEADERS = imaevm.h
 
 nodist_libimaevm_la_SOURCES = hash_info.h
@@ -22,6 +26,12 @@ evmctl_CPPFLAGS = $(AM_CPPFLAGS) $(LIBCRYPTO_CFLAGS)
 evmctl_LDFLAGS = $(LDFLAGS_READLINE)
 evmctl_LDADD =  $(LIBCRYPTO_LIBS) -lkeyutils libimaevm.la
 
+# Enable IMA signature version 1
+if CONFIG_SIGV1
+evmctl_CFLAGS = -DCONFIG_SIGV1
+endif
+
+
 # USE_PCRTSS uses the Intel TSS
 if USE_PCRTSS
  evmctl_SOURCES += pcr_tss.c
diff --git a/src/evmctl.c b/src/evmctl.c
index b1dcd9b1c1ef..158c1f9bf7a1 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -987,7 +987,6 @@ static int cmd_verify_ima(struct command *cmd)
 			init_public_keys("/etc/keys/x509_evm.der");
 	}
 
-	errno = 0;
 	if (!file) {
 		log_err("Parameters missing\n");
 		print_usage(cmd);
@@ -1004,6 +1003,7 @@ static int cmd_verify_ima(struct command *cmd)
 	return fails > 0;
 }
 
+#if CONFIG_SIGV1
 static int cmd_convert(struct command *cmd)
 {
 	char *inkey;
@@ -1034,6 +1034,7 @@ static int cmd_convert(struct command *cmd)
 	RSA_free(key);
 	return err;
 }
+#endif
 
 static int cmd_import(struct command *cmd)
 {
@@ -1088,6 +1089,7 @@ static int cmd_import(struct command *cmd)
 		calc_keyid_v2((uint32_t *)keyid, name, pkey);
 		EVP_PKEY_free(pkey);
 	} else {
+#if CONFIG_SIGV1
 		RSA *key = read_pub_key(inkey, imaevm_params.x509);
 
 		if (!key)
@@ -1095,6 +1097,10 @@ static int cmd_import(struct command *cmd)
 		len = key2bin(key, pub);
 		calc_keyid_v1(keyid, name, pub, len);
 		RSA_free(key);
+#else
+		log_info("Importing public RSA key is not supported\n");
+		return 1;
+#endif
 	}
 
 	log_info("Importing public key %s from file %s into keyring %d\n", name, inkey, id);
@@ -2595,7 +2601,9 @@ static void usage(void)
 		"  -d, --imahash      make IMA hash\n"
 		"  -f, --sigfile      store IMA signature in .sig file instead of xattr\n"
 		"      --xattr-user   store xattrs in user namespace (for testing purposes)\n"
-		"      --rsa          use RSA key type and signing scheme v1\n"
+#if CONFIG_SIGV1
+		"      --rsa          use RSA key type and signing scheme v1 (deprecated)\n"
+#endif
 		"  -k, --key          path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem)\n"
 		"                     or a pkcs11 URI\n"
 		"      --keyid n      overwrite signature keyid with a 32-bit value in hex (for signing)\n"
@@ -2634,8 +2642,12 @@ static void usage(void)
 struct command cmds[] = {
 	{"--version", NULL, 0, ""},
 	{"help", cmd_help, 0, "<command>"},
-	{"import", cmd_import, 0, "[--rsa] pubkey keyring", "Import public key into the keyring.\n"},
-	{"convert", cmd_convert, 0, "key", "convert public key into the keyring.\n"},
+#if CONFIG_SIGV1
+	{"import", cmd_import, 0, "[--rsa] pubkey keyring", "Import public key into the keyring. ([--rsa] deprecated)\n"},
+	{"convert", cmd_convert, 0, "key", "convert public key into the keyring. (deprecated)\n"},
+#else
+	{"import", cmd_import, 0, "pubkey keyring", "Import public key into the keyring.\n"},
+#endif
 	{"sign", cmd_sign_evm, 0, "[-r] [--imahash | --imasig ] [--key key] [--pass [password] file", "Sign file metadata.\n"},
 	{"verify", cmd_verify_evm, 0, "file", "Verify EVM signature (for debugging).\n"},
 	{"ima_sign", cmd_sign_ima, 0, "[--sigfile] [--key key] [--pass [password] file", "Make file content signature.\n"},
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 611631c57f14..b12b7ff14d95 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -290,6 +290,7 @@ out:
 	return pkey;
 }
 
+#if CONFIG_SIGV1
 RSA *read_pub_key(const char *keyfile, int x509)
 {
 	EVP_PKEY *pkey;
@@ -349,6 +350,7 @@ static int verify_hash_v1(const char *file, const unsigned char *hash, int size,
 
 	return 0;
 }
+#endif  /* CONFIG_SIGV1 */
 
 struct public_key_entry {
 	struct public_key_entry *next;
@@ -684,6 +686,7 @@ int verify_hash(const char *file, const unsigned char *hash, int size,
 {
 	/* Get signature type from sig header */
 	if (sig[1] == DIGSIG_VERSION_1) {
+#if CONFIG_SIGV1
 		const char *key = NULL;
 
 		/* Read pubkey from RSA key */
@@ -693,6 +696,10 @@ int verify_hash(const char *file, const unsigned char *hash, int size,
 			key = imaevm_params.keyfile;
 		return verify_hash_v1(file, hash, size, sig + 1, siglen - 1,
 					 key);
+#else
+		log_info("Signature version 1 deprecated.");
+		return -1;
+#endif
 	} else if (sig[1] == DIGSIG_VERSION_2) {
 		return verify_hash_v2(file, hash, size, sig, siglen);
 	} else if (sig[1] == DIGSIG_VERSION_3) {
@@ -740,6 +747,7 @@ int ima_verify_signature(const char *file, unsigned char *sig, int siglen,
 	return verify_hash(file, hash, hashlen, sig, siglen);
 }
 
+#if CONFIG_SIGV1
 /*
  * Create binary key representation suitable for kernel
  */
@@ -798,6 +806,7 @@ void calc_keyid_v1(uint8_t *keyid, char *str, const unsigned char *pkey, int len
 	if (imaevm_params.verbose > LOG_INFO)
 		log_info("keyid-v1: %s\n", str);
 }
+#endif /* CONFIG_SIGV1 */
 
 /*
  * Calculate keyid of the public_key part of EVP_PKEY
@@ -986,6 +995,7 @@ err_engine:
 	return NULL;
 }
 
+#if CONFIG_SIGV1
 static RSA *read_priv_key(const char *keyfile, const char *keypass)
 {
 	EVP_PKEY *pkey;
@@ -1096,6 +1106,7 @@ out:
 	RSA_free(key);
 	return len;
 }
+#endif /* CONFIG_SIGV1 */
 
 /*
  * @sig is assumed to be of (MAX_SIGNATURE_SIZE - 1) size
@@ -1210,9 +1221,14 @@ int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const c
 	if (keypass)
 		imaevm_params.keypass = keypass;
 
-	return imaevm_params.x509 ?
-		sign_hash_v2(hashalgo, hash, size, keyfile, sig) :
-		sign_hash_v1(hashalgo, hash, size, keyfile, sig);
+	if (imaevm_params.x509)
+		return sign_hash_v2(hashalgo, hash, size, keyfile, sig);
+#if CONFIG_SIGV1
+	else
+		return sign_hash_v1(hashalgo, hash, size, keyfile, sig);
+#endif
+	log_info("Signature version 1 deprecated.");
+	return -1;
 }
 
 static void libinit()
diff --git a/tests/sign_verify.test b/tests/sign_verify.test
index c56290aa4932..3e9548e1aeb7 100755
--- a/tests/sign_verify.test
+++ b/tests/sign_verify.test
@@ -17,6 +17,10 @@
 
 cd "$(dirname "$0")" || exit 1
 PATH=../src:$PATH
+
+# set the env SIGV1=1 to execute the signature v1 tests
+SIGV1=${SIGV1:-0}
+
 source ./functions.sh
 
 _require cmp evmctl getfattr openssl xxd
@@ -368,13 +372,18 @@ try_different_sigs() {
 
 ## Test v1 signatures
 # Signature v1 only supports sha1 and sha256 so any other should fail
-expect_fail \
-  check_sign TYPE=ima KEY=rsa1024 ALG=md5 PREFIX=0x0301 OPTS=--rsa
+if [ $SIGV1 -eq 0 ]; then
+  __skip() { echo "IMA signature v1 tests are skipped: not supported"; return $SKIP; }
+  expect_pass __skip
+else
+   expect_fail \
+      check_sign TYPE=ima KEY=rsa1024 ALG=md5 PREFIX=0x0301 OPTS=--rsa
 
-sign_verify  rsa1024  sha1    0x0301 --rsa
-sign_verify  rsa1024  sha256  0x0301 --rsa
-  try_different_keys
-  try_different_sigs
+   sign_verify  rsa1024  sha1    0x0301 --rsa
+   sign_verify  rsa1024  sha256  0x0301 --rsa
+      try_different_keys
+      try_different_sigs
+fi
 
 ## Test v2 signatures with RSA PKCS#1
 # List of allowed hashes much greater but not all are supported.
-- 
2.31.1

[PATCH ima-evm-utils v4 07/17] Replace the low level SHA1 calls when calculating the TPM 1.2 PCRs

[Mimi Zohar]

OpenSSL v3 emits deprecated warnings for SHA1 functions.  Use the
EVP_ functions when walking the TPM 1.2 binary bios measurements
to calculate the TPM 1.2 PCRs.

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 78 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 63 insertions(+), 15 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 158c1f9bf7a1..808f7d43bd18 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -2293,6 +2293,11 @@ static int cmd_ima_measurement(struct command *cmd)
 	return ima_measurement(file);
 }
 
+/*
+ * read_binary_bios_measurements - read the TPM 1.2 event log
+ *
+ * Returns 0 on success, 1 on failure.
+ */
 #define MAX_EVENT_DATA_SIZE 200000
 static int read_binary_bios_measurements(char *file, struct tpm_bank_info *bank)
 {
@@ -2305,17 +2310,22 @@ static int read_binary_bios_measurements(char *file, struct tpm_bank_info *bank)
 		} header;
 		unsigned char data[MAX_EVENT_DATA_SIZE];
 	} event;
+	EVP_MD_CTX *mdctx;
+	const EVP_MD *md;
+	unsigned int mdlen;
+	int evp_err = 1;	/* success */
 	struct stat s;
 	FILE *fp;
-	SHA_CTX c;
 	int err = 0;
 	int len;
 	int i;
+#if OPENSSL_VERSION_NUMBER < 0x10100000
+	EVP_MD_CTX ctx;
+	mdctx = &ctx;
+#endif
 
-	if (stat(file, &s) == -1) {
-		errno = 0;
+	if (stat(file, &s) == -1)
 		return 1;
-	}
 
 	if (!S_ISREG(s.st_mode)) {
 		log_info("Bios event log: not a regular file or link to regular file\n");
@@ -2323,13 +2333,27 @@ static int read_binary_bios_measurements(char *file, struct tpm_bank_info *bank)
 	}
 
 	fp = fopen(file, "r");
-	if (!fp) {
-		log_errno("Failed to open TPM 1.2 event log.\n");
+	if (!fp)
 		return 1;
-	}
 
 	if (imaevm_params.verbose > LOG_INFO)
-		log_info("Reading the TPM 1.2 event log %s.\n", file);
+		log_info("Reading the TPM 1.2 event log (%s)\n", file);
+
+	md = EVP_get_digestbyname(bank->algo_name);
+	if (!md) {
+		log_err("Unknown message digest %s\n", bank->algo_name);
+		fclose(fp);
+		return 1;
+	}
+
+#if OPENSSL_VERSION_NUMBER >= 0x10100000
+	mdctx = EVP_MD_CTX_new();
+	if (!mdctx) {
+		log_err("EVP_MD_CTX_new failed\n");
+		fclose(fp);
+		return 1;
+	}
+#endif
 
 	/* Extend the pseudo TPM PCRs with the event digest */
 	while (fread(&event, sizeof(event.header), 1, fp) == 1) {
@@ -2339,13 +2363,30 @@ static int read_binary_bios_measurements(char *file, struct tpm_bank_info *bank)
 		}
 		if (event.header.pcr >= NUM_PCRS) {
 			log_err("Invalid PCR %d.\n", event.header.pcr);
-			err = 1;
 			break;
 		}
-		SHA1_Init(&c);
-		SHA1_Update(&c, bank->pcr[event.header.pcr], 20);
-		SHA1_Update(&c, event.header.digest, 20);
-		SHA1_Final(bank->pcr[event.header.pcr], &c);
+
+		evp_err = EVP_DigestInit(mdctx, md);
+		if (evp_err == 0) {
+			log_err("EVP_DigestInit() failed\n");
+			break;
+		}
+
+		evp_err = EVP_DigestUpdate(mdctx, bank->pcr[event.header.pcr], 20);
+		if (evp_err == 0) {
+			log_err("EVP_DigestUpdate() failed\n");
+			break;
+		}
+		evp_err = EVP_DigestUpdate(mdctx, event.header.digest, 20);
+		if (evp_err == 0) {
+			log_err("EVP_DigestUpdate() failed\n");
+			break;
+		}
+		evp_err = EVP_DigestFinal(mdctx, bank->pcr[event.header.pcr], &mdlen);
+		if (evp_err == 0) {
+			log_err("EVP_DigestFinal() failed\n");
+			break;
+		}
 		if (event.header.len > MAX_EVENT_DATA_SIZE) {
 			log_err("Event data event too long.\n");
 			err = 1;
@@ -2354,10 +2395,17 @@ static int read_binary_bios_measurements(char *file, struct tpm_bank_info *bank)
 		len = fread(event.data, event.header.len, 1, fp);
 		if (len != 1) {
 			log_errno("Failed reading event data (short read)\n");
+			err = 1;
 			break;
 		}
 	}
+
+	if (evp_err == 0) /* EVP_ functions return 1 on success, 0 on failure */
+		err = 1;
 	fclose(fp);
+#if OPENSSL_VERSION_NUMBER >= 0x10100000
+	EVP_MD_CTX_free(mdctx);
+#endif
 
 	if (imaevm_params.verbose <= LOG_INFO)
 		return err;
@@ -2481,8 +2529,8 @@ static int cmd_ima_bootaggr(struct command *cmd)
 
 		err = read_binary_bios_measurements(file, tpm_banks);
 		if (err) {
-			log_info("Failed reading the TPM 1.2 event log %s.\n",
-				 file);
+			log_err("Failed reading the TPM 1.2 event log (%s)\n",
+				file);
 			return -1;
 		}
 	} else {
-- 
2.31.1

[PATCH ima-evm-utils v4 08/17] Replace the low level HMAC calls when calculating the EVM HMAC

[Mimi Zohar]

Calculating the EVM HMAC and labeling the filesystem was originally
included in ima-evm-utils for debugging purposes only.  For now,
instead of removing EVM HMAC support just replace the low level
HMAC_ calls with EVP_ calls.

The '-a, --hashalgo' specifies the IMA hash or signature algorithm.
The kernel EVM HMAC is limited to SHA1.  Fix ima-evm-utils by hard
coding the EVM HMAC algorithm to SHA1.

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 57 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 25 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 808f7d43bd18..5306d3b6356d 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -61,6 +61,7 @@
 #include <openssl/asn1.h>
 #include <openssl/sha.h>
 #include <openssl/pem.h>
+#include <openssl/evp.h>
 #include <openssl/hmac.h>
 #include <openssl/err.h>
 #include <openssl/rsa.h>
@@ -1159,12 +1160,12 @@ static int cmd_setxattr_ima(struct command *cmd)
 
 static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *hash)
 {
-        const EVP_MD *md;
+	size_t mdlen;
+	EVP_MD_CTX *pctx;
+	EVP_PKEY *pkey = NULL;
 	struct stat st;
 	int err = -1;
 	uint32_t generation = 0;
-	HMAC_CTX *pctx;
-	unsigned int mdlen;
 	char **xattrname;
 	unsigned char xattr_value[1024];
 	unsigned char *key;
@@ -1175,10 +1176,8 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 	struct h_misc_64 hmac_misc;
 	int hmac_size;
 #if OPENSSL_VERSION_NUMBER < 0x10100000
-	HMAC_CTX ctx;
+	EVP_MD_CTX ctx;
 	pctx = &ctx;
-#else
-	pctx = HMAC_CTX_new();
 #endif
 
 	key = file2bin(keyfile, NULL, &keylen);
@@ -1225,19 +1224,26 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 		goto out;
 	}
 
-	md = EVP_get_digestbyname(imaevm_params.hash_algo);
-	if (!md) {
-		log_err("EVP_get_digestbyname(%s) failed\n",
-			imaevm_params.hash_algo);
+#if OPENSSL_VERSION_NUMBER >= 0x10100000
+	pctx = EVP_MD_CTX_new();
+	if (!pctx) {
+		log_err("EVP_MD_CTX_new failed\n");
 		goto out;
 	}
+#endif
 
-	err = !HMAC_Init_ex(pctx, evmkey, sizeof(evmkey), md, NULL);
-	if (err) {
+	pkey = EVP_PKEY_new_mac_key(EVP_PKEY_HMAC, NULL, evmkey, sizeof(evmkey));
+	if (!pkey) {
 		log_err("HMAC_Init() failed\n");
 		goto out;
 	}
 
+	err = EVP_DigestSignInit(pctx, NULL, EVP_sha1(), NULL, pkey);
+	if (err != 1) {
+		log_err("EVP_DigestSignInit() failed\n");
+		goto out;
+	}
+
 	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
 		err = lgetxattr(file, *xattrname, xattr_value, sizeof(xattr_value));
 		if (err < 0) {
@@ -1248,12 +1254,12 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 			log_info("skipping xattr: %s\n", *xattrname);
 			continue;
 		}
-		/*log_debug("name: %s, value: %s, size: %d\n", *xattrname, xattr_value, err);*/
 		log_info("name: %s, size: %d\n", *xattrname, err);
 		log_debug_dump(xattr_value, err);
-		err = !HMAC_Update(pctx, xattr_value, err);
-		if (err) {
-			log_err("HMAC_Update() failed\n");
+
+		err = EVP_DigestSignUpdate(pctx, xattr_value, err);
+		if (err != 1) {
+			log_err("EVP_DigestSignUpdate() failed\n");
 			goto out_ctx_cleanup;
 		}
 	}
@@ -1292,23 +1298,24 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 	log_debug("hmac_misc (%d): ", hmac_size);
 	log_debug_dump(&hmac_misc, hmac_size);
 
-	err = !HMAC_Update(pctx, (const unsigned char *)&hmac_misc, hmac_size);
-	if (err) {
+	err = EVP_DigestSignUpdate(pctx, &hmac_misc, hmac_size);
+	if (err != 1) {
 		log_err("HMAC_Update() failed\n");
 		goto out_ctx_cleanup;
 	}
-	err = !HMAC_Final(pctx, hash, &mdlen);
-	if (err)
+	err = EVP_DigestSignFinal(pctx, hash, &mdlen);
+	if (err != 1)
 		log_err("HMAC_Final() failed\n");
 out_ctx_cleanup:
-#if OPENSSL_VERSION_NUMBER < 0x10100000
-	HMAC_CTX_cleanup(pctx);
-#else
-	HMAC_CTX_free(pctx);
+	EVP_PKEY_free(pkey);
+#if OPENSSL_VERSION_NUMBER >= 0x10100000
+	EVP_MD_CTX_free(pctx);
 #endif
 out:
 	free(key);
-	return err ?: mdlen;
+	if (err == 1)
+		return mdlen;
+	return err;
 }
 
 static int hmac_evm(const char *file, const char *key)
-- 
2.31.1

[PATCH ima-evm-utils v4 09/17] Add missing EVP_MD_CTX_free() call in calc_evm_hash()

[Mimi Zohar]

When EVP_MD_CTX_new() call was added, the corresponding EVP_MD_CTX_free()
was never called.  Properly free it.

Fixes: 81010f0d87ef ("ima-evm-utils: Add backward compatible support for openssl 1.1")
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 58 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 5306d3b6356d..039571577448 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -331,11 +331,17 @@ err:
 	return -1;
 }
 
+/*
+ * calc_evm_hash - calculate the file metadata hash
+ *
+ * Returns 0 for EVP_ function failures. Return -1 for other failures.
+ * Return hash algorithm size on success.
+ */
 static int calc_evm_hash(const char *file, unsigned char *hash)
 {
         const EVP_MD *md;
 	struct stat st;
-	int err;
+	int err = -1;
 	uint32_t generation = 0;
 	EVP_MD_CTX *pctx;
 	unsigned int mdlen;
@@ -349,12 +355,10 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 #if OPENSSL_VERSION_NUMBER < 0x10100000
 	EVP_MD_CTX ctx;
 	pctx = &ctx;
-#else
-	pctx = EVP_MD_CTX_new();
 #endif
 
 	if (lstat(file, &st)) {
-		log_err("Failed to stat: %s\n", file);
+		log_errno_reset(LOG_ERR, "Failed to stat: %s", file);
 		return -1;
 	}
 
@@ -389,21 +393,30 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 
 	list_size = llistxattr(file, list, sizeof(list));
 	if (list_size < 0) {
-		log_err("llistxattr() failed\n");
+		log_errno_reset(LOG_ERR, "llistxattr() failed");
 		return -1;
 	}
 
+#if OPENSSL_VERSION_NUMBER >= 0x10100000
+	pctx = EVP_MD_CTX_new();
+	if (!pctx) {
+		log_err("EVP_MD_CTX_new() failed\n");
+		return 0;
+	}
+#endif
+
 	md = EVP_get_digestbyname(imaevm_params.hash_algo);
 	if (!md) {
 		log_err("EVP_get_digestbyname(%s) failed\n",
 			imaevm_params.hash_algo);
-		return 1;
+		err = 0;
+		goto out;
 	}
 
 	err = EVP_DigestInit(pctx, md);
 	if (!err) {
 		log_err("EVP_DigestInit() failed\n");
-		return 1;
+		goto out;
 	}
 
 	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
@@ -414,7 +427,8 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 			if (err > sizeof(xattr_value)) {
 				log_err("selinux[%u] value is too long to fit into xattr[%zu]\n",
 					err, sizeof(xattr_value));
-				return -1;
+				err = -1;
+				goto out;
 			}
 			strcpy(xattr_value, selinux_str);
 		} else if (!strcmp(*xattrname, XATTR_NAME_IMA) && ima_str) {
@@ -422,7 +436,8 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 			if (err > sizeof(xattr_value)) {
 				log_err("ima[%u] value is too long to fit into xattr[%zu]\n",
 					err, sizeof(xattr_value));
-				return -1;
+				err = -1;
+				goto out;
 			}
 			hex2bin(xattr_value, ima_str, err);
 		} else if (!strcmp(*xattrname, XATTR_NAME_IMA) && evm_portable){
@@ -431,7 +446,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 			if (err < 0) {
 				log_err("EVM portable sig: %s required\n",
 					xattr_ima);
-				return -1;
+				goto out;
 			}
 			use_xattr_ima = 1;
 		} else if (!strcmp(*xattrname, XATTR_NAME_CAPS) && (hmac_flags & HMAC_FLAG_CAPS_SET)) {
@@ -441,7 +456,8 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 			if (err >= sizeof(xattr_value)) {
 				log_err("caps[%u] value is too long to fit into xattr[%zu]\n",
 					err + 1, sizeof(xattr_value));
-				return -1;
+				err = -1;
+				goto out;
 			}
 			strcpy(xattr_value, caps_str);
 		} else {
@@ -462,7 +478,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 		err = EVP_DigestUpdate(pctx, xattr_value, err);
 		if (!err) {
 			log_err("EVP_DigestUpdate() failed\n");
-			return 1;
+			goto out;
 		}
 	}
 
@@ -516,29 +532,33 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 	err = EVP_DigestUpdate(pctx, &hmac_misc, hmac_size);
 	if (!err) {
 		log_err("EVP_DigestUpdate() failed\n");
-		return 1;
+		goto out;
 	}
 
 	if (!evm_immutable && !evm_portable &&
 	    !(hmac_flags & HMAC_FLAG_NO_UUID)) {
 		err = get_uuid(&st, uuid);
 		if (err)
-			return -1;
+			goto out;
 
 		err = EVP_DigestUpdate(pctx, (const unsigned char *)uuid, sizeof(uuid));
 		if (!err) {
 			log_err("EVP_DigestUpdate() failed\n");
-			return 1;
+			goto out;
 		}
 	}
 
 	err = EVP_DigestFinal(pctx, hash, &mdlen);
-	if (!err) {
+	if (!err)
 		log_err("EVP_DigestFinal() failed\n");
-		return 1;
-	}
 
-	return mdlen;
+out:
+#if OPENSSL_VERSION_NUMBER >= 0x10100000
+	EVP_MD_CTX_free(pctx);
+#endif
+	if (err == 1)
+		return mdlen;
+	return err;
 }
 
 static int sign_evm(const char *file, const char *key)
-- 
2.31.1

[PATCH ima-evm-utils v4 10/17] Disable use of OpenSSL "engine" support

[Mimi Zohar]

OpenSSL v3 "engine" support is deprecated and replaced with "providers".
Engine support will continue to work for a while, but results in
deprecated declaration and other messages.  One option is simply to hide
them ("-Wno-deprecated-declarations").  The other alternative is to
conditionally build ima-evm-utils without OpenSSL engine support and
without disabling deprecated declarations.

Based on "--disable-engine" or "--enable-engine=no" configuration
option, disable OpenSSL "engine" support.

As suggested by Vitaly,
- verify ENGINE_init symbol is defined in libcrypto
- disable engine support if either OPENSSL_NO_DYNAMIC_ENGINE or
OPENSSL_NO_ENGINE variables are defined

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 configure.ac    |  6 ++++++
 src/Makefile.am |  8 ++++++++
 src/evmctl.c    | 17 ++++++++++++++++-
 src/imaevm.h    |  6 ++++++
 src/libimaevm.c |  7 ++++++-
 5 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index dc666f2bb1fa..90646da22061 100644
--- a/configure.ac
+++ b/configure.ac
@@ -54,6 +54,11 @@ AC_ARG_ENABLE(sigv1,
 	AM_CONDITIONAL([CONFIG_SIGV1], [test "x$enable_sigv1" = "xyes"])
 	AS_IF([test "$enable_sigv1"  != "yes"], [enable_sigv1="no"])
 
+AC_ARG_ENABLE(engine,
+	      [AS_HELP_STRING([--disable-engine], [build ima-evm-utils without OpenSSL engine support])],,[enable_engine=yes])
+	AC_CHECK_LIB([crypto], [ENGINE_init],, [enable_engine=no])
+	AM_CONDITIONAL([CONFIG_IMA_EVM_ENGINE], [test "x$enable_engine" = "xyes"])
+
 #debug support - yes for a while
 PKG_ARG_ENABLE(debug, "yes", DEBUG, [Enable Debug support])
 if test $pkg_cv_enable_debug = yes; then
@@ -89,5 +94,6 @@ echo	"      tss2-esys: $ac_cv_lib_tss2_esys_Esys_Free"
 echo	" tss2-rc-decode: $ac_cv_lib_tss2_rc_Tss2_RC_Decode"
 echo    "         ibmtss: $ac_cv_header_ibmtss_tss_h"
 echo    "         sigv1:  $enable_sigv1"
+echo    "         engine: $enable_engine"
 echo	"            doc: $have_doc"
 echo
diff --git a/src/Makefile.am b/src/Makefile.am
index 90c7249020cf..0527a7b9df5c 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -11,6 +11,10 @@ if CONFIG_SIGV1
 libimaevm_la_CFLAGS = -DCONFIG_SIGV1
 endif
 
+if CONFIG_IMA_EVM_ENGINE
+libimaevm_la_CFLAGS = -DCONFIG_IMA_EVM_ENGINE
+endif
+
 include_HEADERS = imaevm.h
 
 nodist_libimaevm_la_SOURCES = hash_info.h
@@ -31,6 +35,10 @@ if CONFIG_SIGV1
 evmctl_CFLAGS = -DCONFIG_SIGV1
 endif
 
+# Enable "--engine" support
+if CONFIG_IMA_EVM_ENGINE
+evmctl_CFLAGS = -DCONFIG_IMA_EVM_ENGINE
+endif
 
 # USE_PCRTSS uses the Intel TSS
 if USE_PCRTSS
diff --git a/src/evmctl.c b/src/evmctl.c
index 039571577448..4817eeba01c0 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -65,7 +65,9 @@
 #include <openssl/hmac.h>
 #include <openssl/err.h>
 #include <openssl/rsa.h>
+#if CONFIG_IMA_EVM_ENGINE
 #include <openssl/engine.h>
+#endif
 #include <openssl/x509v3.h>
 #include "hash_info.h"
 #include "pcr.h"
@@ -2704,7 +2706,9 @@ static void usage(void)
 		"      --selinux      use custom Selinux label for EVM\n"
 		"      --caps         use custom Capabilities for EVM(unspecified: from FS, empty: do not use)\n"
 		"      --verify-sig   verify measurement list signatures\n"
-		"      --engine e     preload OpenSSL engine e (such as: gost)\n"
+#if CONFIG_IMA_EVM_ENGINE
+		"      --engine e     preload OpenSSL engine e (such as: gost) is deprecated\n"
+#endif
 		"      --ignore-violations ignore ToMToU measurement violations\n"
 		"  -v                 increase verbosity level\n"
 		"  -h, --help         display this help and exit\n"
@@ -2766,7 +2770,9 @@ static struct option opts[] = {
 	{"selinux", 1, 0, 136},
 	{"caps", 2, 0, 137},
 	{"verify-sig", 0, 0, 138},
+#if CONFIG_IMA_EVM_ENGINE
 	{"engine", 1, 0, 139},
+#endif
 	{"xattr-user", 0, 0, 140},
 	{"ignore-violations", 0, 0, 141},
 	{"pcrs", 1, 0, 142},
@@ -2819,9 +2825,11 @@ static char *get_password(void)
 	return password;
 }
 
+#if CONFIG_IMA_EVM_ENGINE
 static ENGINE *setup_engine(const char *engine_id)
 {
 	ENGINE *eng = ENGINE_by_id(engine_id);
+
 	if (!eng) {
 		log_err("engine %s isn't available\n", optarg);
 		ERR_print_errors_fp(stderr);
@@ -2835,6 +2843,7 @@ static ENGINE *setup_engine(const char *engine_id)
 		ENGINE_set_default(eng, ENGINE_METHOD_ALL);
 	return eng;
 }
+#endif
 
 int main(int argc, char *argv[])
 {
@@ -2960,11 +2969,13 @@ int main(int argc, char *argv[])
 		case 138:
 			verify_list_sig = 1;
 			break;
+#if CONFIG_IMA_EVM_ENGINE
 		case 139: /* --engine e */
 			imaevm_params.eng = setup_engine(optarg);
 			if (!imaevm_params.eng)
 				goto error;
 			break;
+#endif
 		case 140: /* --xattr-user */
 			xattr_ima = "user.ima";
 			xattr_evm = "user.evm";
@@ -3023,7 +3034,9 @@ int main(int argc, char *argv[])
 	if (imaevm_params.keyfile != NULL &&
 	    imaevm_params.eng == NULL &&
 	    !strncmp(imaevm_params.keyfile, "pkcs11:", 7)) {
+#if CONFIG_IMA_EVM_ENGINE
 		imaevm_params.eng = setup_engine("pkcs11");
+#endif
 		if (!imaevm_params.eng)
 			goto error;
 	}
@@ -3049,6 +3062,7 @@ int main(int argc, char *argv[])
 	}
 
 error:
+#if CONFIG_IMA_EVM_ENGINE
 	if (imaevm_params.eng) {
 		ENGINE_finish(imaevm_params.eng);
 		ENGINE_free(imaevm_params.eng);
@@ -3056,6 +3070,7 @@ error:
 		ENGINE_cleanup();
 #endif
 	}
+#endif
 	ERR_free_strings();
 	EVP_cleanup();
 	BIO_free(NULL);
diff --git a/src/imaevm.h b/src/imaevm.h
index afcf1e042014..884321670fa7 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -48,7 +48,13 @@
 #include <errno.h>
 #include <sys/types.h>
 #include <openssl/rsa.h>
+#ifdef CONFIG_IMA_EVM_ENGINE
 #include <openssl/engine.h>
+#endif
+
+#if defined(OPENSSL_NO_ENGINE) || defined(OPENSSL_NO_DYNAMIC_ENGINE)
+#undef CONFIG_IMA_EVM_ENGINE
+#endif
 
 #ifdef USE_FPRINTF
 #define do_log(level, fmt, args...)	\
diff --git a/src/libimaevm.c b/src/libimaevm.c
index b12b7ff14d95..8070ffd61a2c 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -953,9 +953,10 @@ uint32_t imaevm_read_keyid(const char *certfile)
 static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
 {
 	FILE *fp;
-	EVP_PKEY *pkey;
+	EVP_PKEY *pkey = NULL;
 
 	if (!strncmp(keyfile, "pkcs11:", 7)) {
+#ifdef CONFIG_IMA_EVM_ENGINE
 		if (!imaevm_params.keyid) {
 			log_err("When using a pkcs11 URI you must provide the keyid with an option\n");
 			return NULL;
@@ -972,6 +973,10 @@ static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
 			log_err("Failed to load private key %s\n", keyfile);
 			goto err_engine;
 		}
+#else
+		log_err("OpenSSL \"engine\" support is disabled\n");
+		goto err_engine;
+#endif
 	} else {
 		fp = fopen(keyfile, "r");
 		if (!fp) {
-- 
2.31.1

[PATCH ima-evm-utils v4 11/17] Fix potential use after free in read_tpm_banks()

[Mimi Zohar]

On failure to read TPM 2.0 bank PCRs 'errmsg' is not properly set to
NULL after being freed.  Fix potential use after free.

Fixes: 3472f9ba9c05 ("ima-evm-utils: read the PCRs for the requested TPM banks")
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/evmctl.c b/src/evmctl.c
index 4817eeba01c0..c224e17ee620 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -2070,6 +2070,7 @@ static int read_tpm_banks(int num_banks, struct tpm_bank_info *bank)
 				log_debug("Failed to read %s PCRs: (%s)\n",
 					  bank[i].algo_name, errmsg);
 				free(errmsg);
+				errmsg = NULL;
 				bank[i].supported = 0;
 			}
 		}
-- 
2.31.1

[PATCH ima-evm-utils v4 12/17] Limit the file hash algorithm name length

[Mimi Zohar]

Instead of assuming the file hash algorithm is a properly NULL terminated
string, properly limit the "algo:<hash>" field size.

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index c224e17ee620..17cc2188fe73 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1598,7 +1598,7 @@ void ima_ng_show(struct template_entry *entry)
 	total_len -= sizeof(field_len);
 
 	algo = (char *)fieldp;
-	len = strlen(algo) + 1;
+	len = strnlen(algo, field_len - 1) + 1;
 	digest_len = field_len - len;
 	digest = fieldp + len;
 
-- 
2.31.1

[PATCH ima-evm-utils v4 13/17] Missing template data size lower bounds checking

[Mimi Zohar]

Each record in the IMA measurement list must contain some template data.
Ensure the template data is not zero length.

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/evmctl.c b/src/evmctl.c
index 17cc2188fe73..2fc11db77ec3 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -2183,6 +2183,10 @@ static int ima_measurement(const char *file)
 				log_err("Unable to read template length\n");
 				goto out;
 			}
+			if (entry.template_len == 0) {
+				log_err("Invalid template data len\n");
+				goto out;
+			}
 		} else {
 			entry.template_len = SHA_DIGEST_LENGTH +
 					     TCG_EVENT_NAME_LEN_MAX + 1;
-- 
2.31.1

[PATCH ima-evm-utils v4 14/17] Base sm2/sm3 test on openssl version installed

[Mimi Zohar]

Since the distros are now shipping with OpenSSL 3, no need
to build it.  Limit the sm2/sm3 test to OpenSSL 3.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 tests/sign_verify.test | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/sign_verify.test b/tests/sign_verify.test
index 3e9548e1aeb7..ed95f6955dc1 100755
--- a/tests/sign_verify.test
+++ b/tests/sign_verify.test
@@ -416,8 +416,8 @@ sign_verify  prime256v1 sha384 0x030205:K:004[345678]
 sign_verify  prime256v1 sha512 0x030206:K:004[345678]
 
 # If openssl 3.0 is installed, test the SM2/3 algorithm combination
-if [ -x /opt/openssl3/bin/openssl ]; then
-  PATH=/opt/openssl3/bin:$PATH LD_LIBRARY_PATH=/opt/openssl3/lib \
+ssl_version=$(openssl version | sed -e 's/^OpenSSL //' | sed -e 's/ .*//')
+if test "${ssl_version::1}" = "3"; then
     sign_verify  sm2    sm3    0x030211:K:004[345678]
 fi
 
-- 
2.31.1

[PATCH ima-evm-utils v4 15/17] Compile a newer version of OpenSSL

[Mimi Zohar]

With the distros shipping OpenSSL 3 with engine support, the original
purpose for compiling OpenSSL 3 to test sm2/sm3 is no longer necessary
and could be removed.  Or, it could be re-purposed for building OpenSSL
without engine support, which is needed for testing.

For both travis and github actions, update openssl-3.0.0-beta1 with
openssl-3.0.5.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 .github/workflows/ci.yml | 6 +++---
 .travis.yml              | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index dc06fce64ba5..5595855512e4 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -17,7 +17,7 @@ jobs:
               ARCH: i386
               TSS: tpm2-tss
               VARIANT: i386
-              COMPILE_SSL: openssl-3.0.0-beta1
+              COMPILE_SSL: openssl-3.0.5
 
           # cross compilation builds
           - container: "debian:stable"
@@ -52,7 +52,7 @@ jobs:
             env:
               CC: clang
               TSS: ibmtss
-              COMPILE_SSL: openssl-3.0.0-beta1
+              COMPILE_SSL: openssl-3.0.5
 
           - container: "opensuse/leap"
             env:
@@ -63,7 +63,7 @@ jobs:
             env:
               CC: gcc
               TSS: ibmtss
-              COMPILE_SSL: openssl-3.0.0-beta1
+              COMPILE_SSL: openssl-3.0.5
 
           - container: "ubuntu:xenial"
             env:
diff --git a/.travis.yml b/.travis.yml
index edd2a21b83d3..09db401928f6 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -9,7 +9,7 @@ matrix:
     include:
         # 32 bit build
         - os: linux
-          env: DISTRO=debian:stable VARIANT=i386 ARCH=i386 TSS=tpm2-tss COMPILE_SSL=openssl-3.0.0-beta1
+          env: DISTRO=debian:stable VARIANT=i386 ARCH=i386 TSS=tpm2-tss COMPILE_SSL=openssl-3.0.5
           compiler: gcc
 
         # cross compilation builds
@@ -32,7 +32,7 @@ matrix:
 
         # glibc (gcc/clang)
         - os: linux
-          env: DISTRO=opensuse/tumbleweed TSS=ibmtss CONTAINER=podman CONTAINER_ARGS="--runtime=/usr/bin/crun --network=host" COMPILE_SSL=openssl-3.0.0-beta1
+          env: DISTRO=opensuse/tumbleweed TSS=ibmtss CONTAINER=podman CONTAINER_ARGS="--runtime=/usr/bin/crun --network=host" COMPILE_SSL=openssl-3.0.5
           compiler: clang
 
         - os: linux
@@ -40,7 +40,7 @@ matrix:
           compiler: gcc
 
         - os: linux
-          env: DISTRO=ubuntu:jammy TSS=ibmtss COMPILE_SSL=openssl-3.0.0-beta1
+          env: DISTRO=ubuntu:jammy TSS=ibmtss COMPILE_SSL=openssl-3.0.5
           compiler: gcc
 
         - os: linux
-- 
2.31.1

[PATCH ima-evm-utils v4 16/17] Build OpenSSL without engine support

[Mimi Zohar]

Fix COMPILE_SSL to build for the proper architecture, link with the
appropriate library, and set up library path for evmctl.

Compile OpenSSL with "no-engine" and "no-dynamic-engine" support.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 .github/workflows/ci.yml  | 4 ++--
 .travis.yml               | 2 +-
 build.sh                  | 8 ++++++++
 src/evmctl.c              | 4 +++-
 tests/install-openssl3.sh | 9 ++++++++-
 5 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 5595855512e4..8223b8767351 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -114,7 +114,7 @@ jobs:
         INSTALL="${INSTALL%%/*}"
         if [ "$VARIANT" ]; then ARCH="$ARCH" ./ci/$INSTALL.$VARIANT.sh; fi
         ARCH="$ARCH" CC="$CC" TSS="$TSS" ./ci/$INSTALL.sh
-        if [ "$COMPILE_SSL" ]; then COMPILE_SSL="$COMPILE_SSL" ./tests/install-openssl3.sh; fi
+        if [ "$COMPILE_SSL" ]; then COMPILE_SSL="$COMPILE_SSL" VARIANT="$VARIANT" ./tests/install-openssl3.sh; fi
 
     - name: Build swtpm
       run: |
@@ -129,4 +129,4 @@ jobs:
       run: $CC --version
 
     - name: Compile
-      run: CC="$CC" VARIANT="$VARIANT" ./build.sh
+      run: CC="$CC" VARIANT="$VARIANT" COMPILE_SSL="$COMPILE_SSL" ./build.sh
diff --git a/.travis.yml b/.travis.yml
index 09db401928f6..faaa4b5d675b 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -95,4 +95,4 @@ script:
     - INSTALL="${DISTRO#${REPO}}"
     - INSTALL="${INSTALL%%:*}"
     - INSTALL="${INSTALL%%/*}"
-    - $CONTAINER run $CONTAINER_ARGS -t ima-evm-utils /bin/sh -c "if [ \"$VARIANT\" ]; then ARCH=\"$ARCH\" ./ci/$INSTALL.$VARIANT.sh; fi && ARCH=\"$ARCH\" CC=\"$CC\" TSS=\"$TSS\" ./ci/$INSTALL.sh && if [ "$COMPILE_SSL" ]; then COMPILE_SSL="$COMPILE_SSL" ./tests/install-openssl3.sh; fi && if [ ! \"$VARIANT\" ]; then which tpm_server || which swtpm || if which tssstartup; then ./tests/install-swtpm.sh; fi; fi && CC=\"$CC\" VARIANT=\"$VARIANT\" ./build.sh"
+    - $CONTAINER run $CONTAINER_ARGS -t ima-evm-utils /bin/sh -c "if [ \"$VARIANT\" ]; then ARCH=\"$ARCH\" ./ci/$INSTALL.$VARIANT.sh; fi && ARCH=\"$ARCH\" CC=\"$CC\" TSS=\"$TSS\" ./ci/$INSTALL.sh && if [ \"$COMPILE_SSL\" ]; then COMPILE_SSL=\"$COMPILE_SSL\" VARIANT=\"$VARIANT\" ./tests/install-openssl3.sh; fi && if [ ! \"$VARIANT\" ]; then which tpm_server || which swtpm || if which tssstartup; then ./tests/install-swtpm.sh; fi; fi && CC=\"$CC\" VARIANT=\"$VARIANT\" COMPILE_SSL=\"$COMPILE_SSL\" ./build.sh"
diff --git a/build.sh b/build.sh
index c4d28f1302c6..0c2fdd9e995d 100755
--- a/build.sh
+++ b/build.sh
@@ -32,6 +32,14 @@ log_exit()
 
 cd `dirname $0`
 
+if [ "$COMPILE_SSL" ]; then
+	echo "COMPILE_SSL: $COMPILE_SSL"
+	export CFLAGS="-I/opt/openssl3/include $CFLAGS"
+	export LD_LIBRARY_PATH="/opt/openssl3/lib64:/opt/openssl3/lib:$HOME/src/ima-evm-utils/src/.libs:$LD_LIBRARY_PATH"
+	export LDFLAGS="-L/opt/openssl3/lib64 -L/opt/openssl3/lib $LDFLAGS"
+	export PATH="/opt/openssl3/bin:$HOME/src/ima-evm-utils/src/.libs:$PATH"
+fi
+
 case "$VARIANT" in
 	i386)
 		echo "32-bit compilation"
diff --git a/src/evmctl.c b/src/evmctl.c
index 2fc11db77ec3..349215eeb632 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -2977,8 +2977,10 @@ int main(int argc, char *argv[])
 #if CONFIG_IMA_EVM_ENGINE
 		case 139: /* --engine e */
 			imaevm_params.eng = setup_engine(optarg);
-			if (!imaevm_params.eng)
+			if (!imaevm_params.eng) {
+				log_info("setup_engine failed\n");
 				goto error;
+			}
 			break;
 #endif
 		case 140: /* --xattr-user */
diff --git a/tests/install-openssl3.sh b/tests/install-openssl3.sh
index 1b634681a760..911c32bcf87c 100755
--- a/tests/install-openssl3.sh
+++ b/tests/install-openssl3.sh
@@ -13,7 +13,14 @@ wget --no-check-certificate https://github.com/openssl/openssl/archive/refs/tags
 tar --no-same-owner -xzf ${version}.tar.gz
 cd openssl-${version}
 
-./Configure --prefix=/opt/openssl3 --openssldir=/opt/openssl3/ssl
+if [ "$VARIANT" = "i386" ]; then
+	echo "32-bit compilation"
+	FLAGS="-m32 linux-generic32"
+fi
+
+./Configure $FLAGS no-engine no-dynamic-engine --prefix=/opt/openssl3 --openssldir=/opt/openssl3
+# Uncomment for debugging
+# perl configdata.pm --dump | grep engine
 make -j$(nproc)
 # only install apps and library
 sudo make install_sw
-- 
2.31.1

[PATCH ima-evm-utils v4 17/17] Fix d2i_x509_fp failure

[Mimi Zohar]

Before calling d2i_x509_fp(), make sure the keyfile is a regular file.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/libimaevm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index 8070ffd61a2c..e6fbec5bc17b 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -250,6 +250,7 @@ EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
 {
 	FILE *fp;
 	EVP_PKEY *pkey = NULL;
+	struct stat st;
 
 	if (!keyfile)
 		return NULL;
@@ -262,6 +263,16 @@ EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
 	}
 
 	if (x509) {
+		if (fstat(fileno(fp), &st) == -1)
+			goto out;
+
+		if ((st.st_mode & S_IFMT) != S_IFREG) {
+			if (imaevm_params.verbose > LOG_INFO)
+				log_err("Keyfile is not regular file: %s\n",
+					 keyfile);
+			goto out;
+		}
+
 		X509 *crt = d2i_X509_fp(fp, NULL);
 
 		if (!crt) {
-- 
2.31.1

Re: [PATCH ima-evm-utils v4 14/17] Base sm2/sm3 test on openssl version installed

[Stefan Berger]

On 11/1/22 16:18, Mimi Zohar wrote:
> Since the distros are now shipping with OpenSSL 3, no need
> to build it.  Limit the sm2/sm3 test to OpenSSL 3.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   tests/sign_verify.test | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/sign_verify.test b/tests/sign_verify.test
> index 3e9548e1aeb7..ed95f6955dc1 100755
> --- a/tests/sign_verify.test
> +++ b/tests/sign_verify.test
> @@ -416,8 +416,8 @@ sign_verify  prime256v1 sha384 0x030205:K:004[345678]
>   sign_verify  prime256v1 sha512 0x030206:K:004[345678]
>   
>   # If openssl 3.0 is installed, test the SM2/3 algorithm combination
> -if [ -x /opt/openssl3/bin/openssl ]; then
> -  PATH=/opt/openssl3/bin:$PATH LD_LIBRARY_PATH=/opt/openssl3/lib \
> +ssl_version=$(openssl version | sed -e 's/^OpenSSL //' | sed -e 's/ .*//')

This would give you the major number in one step by selecting all all characters after 'OpenSSL ' and before the first '.':

major=$(openssl version | sed -n 's/^OpenSSL \([^\.]\).*/\1/p')


> +if test "${ssl_version::1}" = "3"; then
>       sign_verify  sm2    sm3    0x030211:K:004[345678]
>   fi
>   

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Re: [PATCH ima-evm-utils v4 01/17] Revert "Reset 'errno' after failure to open or access a file"

[Stefan Berger]

On 11/1/22 16:17, Mimi Zohar wrote:
> This reverts commit acb19d1894a4a95471b8d2346cd6c3ecf3385110.

$ git show acb19d1
fatal: ambiguous argument 'acb19d1': unknown revision or path not in the working tree.

Are you reverting this from a public tree? or could you just drop this patch from your series?

Also after removing this patch from the mbox file I cannot apply these patches to my sourceforge checkout -- presumably next-testing branch:

$ git am ./v4_20221101_zohar_address_deprecated_warnings.mbx
Applying: log and reset 'errno' after failure to open non-critical files
Applying: Log and reset 'errno' on lsetxattr failure
Applying: travis: update dist=focal
Applying: Update configure.ac to address a couple of obsolete warnings
Applying: Deprecate IMA signature version 1
error: patch failed: src/libimaevm.c:684
error: src/libimaevm.c: patch does not apply
Patch failed at 0005 Deprecate IMA signature version 1
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

    Stefan

Re: [PATCH ima-evm-utils v4 01/17] Revert "Reset 'errno' after failure to open or access a file"

[Mimi Zohar]

Hi Stefan,

On Tue, 2022-11-01 at 17:46 -0400, Stefan Berger wrote:
> 
> On 11/1/22 16:17, Mimi Zohar wrote:
> > This reverts commit acb19d1894a4a95471b8d2346cd6c3ecf3385110.
> 
> $ git show acb19d1
> fatal: ambiguous argument 'acb19d1': unknown revision or path not in the working tree.
> 
> Are you reverting this from a public tree? or could you just drop this patch from your series?
> 
> Also after removing this patch from the mbox file I cannot apply these patches to my sourceforge checkout -- presumably next-testing branch:
> 
> $ git am ./v4_20221101_zohar_address_deprecated_warnings.mbx
> Applying: log and reset 'errno' after failure to open non-critical files
> Applying: Log and reset 'errno' on lsetxattr failure
> Applying: travis: update dist=focal
> Applying: Update configure.ac to address a couple of obsolete warnings
> Applying: Deprecate IMA signature version 1
> error: patch failed: src/libimaevm.c:684
> error: src/libimaevm.c: patch does not apply
> Patch failed at 0005 Deprecate IMA signature version 1
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".

Sorry, I should have used "--base=auto" when generating the patch set. 
Yes, the patch set is based on the github next-testing branch.

thanks,

Mimi

Re: [PATCH ima-evm-utils v4 01/17] Revert "Reset 'errno' after failure to open or access a file"

[Stefan Berger]

On 11/1/22 19:04, Mimi Zohar wrote:
> Hi Stefan,
> 
> On Tue, 2022-11-01 at 17:46 -0400, Stefan Berger wrote:
>>
>> On 11/1/22 16:17, Mimi Zohar wrote:
>>> This reverts commit acb19d1894a4a95471b8d2346cd6c3ecf3385110.
>>
>> $ git show acb19d1
>> fatal: ambiguous argument 'acb19d1': unknown revision or path not in the working tree.
>>
>> Are you reverting this from a public tree? or could you just drop this patch from your series?
>>
>> Also after removing this patch from the mbox file I cannot apply these patches to my sourceforge checkout -- presumably next-testing branch:
>>
>> $ git am ./v4_20221101_zohar_address_deprecated_warnings.mbx
>> Applying: log and reset 'errno' after failure to open non-critical files
>> Applying: Log and reset 'errno' on lsetxattr failure
>> Applying: travis: update dist=focal
>> Applying: Update configure.ac to address a couple of obsolete warnings
>> Applying: Deprecate IMA signature version 1
>> error: patch failed: src/libimaevm.c:684
>> error: src/libimaevm.c: patch does not apply
>> Patch failed at 0005 Deprecate IMA signature version 1
>> hint: Use 'git am --show-current-patch=diff' to see the failed patch
>> When you have resolved this problem, run "git am --continue".
>> If you prefer to skip this patch, run "git am --skip" instead.
>> To restore the original branch and stop patching, run "git am --abort".
> 
> Sorry, I should have used "--base=auto" when generating the patch set.
> Yes, the patch set is based on the github next-testing branch.
> 

Is the github repo now the main repo and sourceforge repo is dead?

> thanks,
> 
> Mimi
> 

Re: [PATCH ima-evm-utils v4 17/17] Fix d2i_x509_fp failure

[Stefan Berger]

On 11/1/22 16:18, Mimi Zohar wrote:
> Before calling d2i_x509_fp(), make sure the keyfile is a regular file.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/libimaevm.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 8070ffd61a2c..e6fbec5bc17b 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -250,6 +250,7 @@ EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
>   {
>   	FILE *fp;
>   	EVP_PKEY *pkey = NULL;
> +	struct stat st;
>   
>   	if (!keyfile)
>   		return NULL;
> @@ -262,6 +263,16 @@ EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
>   	}
>   
>   	if (x509) {
> +		if (fstat(fileno(fp), &st) == -1)
> +			goto out;
> +

If this was to ever happen evmctl may just terminate without an error message.

> +		if ((st.st_mode & S_IFMT) != S_IFREG) {

This function can also read plain public keys in the else branch. Should this test cover both cases?



> +			if (imaevm_params.verbose > LOG_INFO)
> +				log_err("Keyfile is not regular file: %s\n",
> +					 keyfile);
> +			goto out;
> +		}
> +
>   		X509 *crt = d2i_X509_fp(fp, NULL);
>   
>   		if (!crt) {

Re: [PATCH ima-evm-utils v4 03/17] Log and reset 'errno' on lsetxattr failure

[Stefan Berger]

On 11/1/22 16:17, Mimi Zohar wrote:
> Writing either security.ima hashes or security.evm hmacs from userspace
> will fail regardless of the IMA or EVM fix mode.  In fix mode, 'touch'
> will force security.ima and security.evm to be updated.
> 
> Make the setxattr error messages more explicit and clear errno.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/evmctl.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 54123bf20f03..b1dcd9b1c1ef 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -572,7 +572,7 @@ static int sign_evm(const char *file, const char *key)
>   	if (xattr) {
>   		err = lsetxattr(file, xattr_evm, sig, len, 0);
>   		if (err < 0) {
> -			log_err("setxattr failed: %s\n", file);
> +			log_errno_reset(LOG_ERR, "Set EVM xattr failed: %s", file);

Change 'Set' to 'Setting' (in other cases as well)?

>   			return err;
>   		}
>   	}
> @@ -615,7 +615,8 @@ static int hash_ima(const char *file)
>   	if (xattr) {
>   		err = lsetxattr(file, xattr_ima, hash, len, 0);
>   		if (err < 0) {
> -			log_err("setxattr failed: %s\n", file);
> +			log_errno_reset(LOG_ERR, "Set IMA hash xattr failed: %s",
> +					file);
>   			return err;
>   		}
>   	}
> @@ -652,7 +653,8 @@ static int sign_ima(const char *file, const char *key)
>   	if (xattr) {
>   		err = lsetxattr(file, xattr_ima, sig, len, 0);
>   		if (err < 0) {
> -			log_err("setxattr failed: %s\n", file);
> +			log_errno_reset(LOG_ERR, "Set IMA sig xattr failed: %s",
> +					file);
>   			return err;
>   		}
>   	}
> @@ -1125,7 +1127,7 @@ static int setxattr_ima(const char *file, char *sig_file)
>   
>   	err = lsetxattr(file, xattr_ima, sig, len, 0);
>   	if (err < 0)
> -		log_err("setxattr failed: %s\n", file);
> +		log_errno_reset(LOG_ERR, "Set IMA sig xattr failed: %s", file);
>   	free(sig);
>   	return err;
>   }
> @@ -1323,7 +1325,8 @@ static int hmac_evm(const char *file, const char *key)
>   		sig[0] = EVM_XATTR_HMAC;
>   		err = lsetxattr(file, xattr_evm, sig, len + 1, 0);
>   		if (err < 0) {
> -			log_err("setxattr failed: %s\n", file);
> +			log_errno_reset(LOG_ERR, "Set EVM hmac xattr failed: %s",
> +					file);
>   			return err;
>   		}
>   	}


Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Re: [PATCH ima-evm-utils v4 02/17] log and reset 'errno' after failure to open non-critical files

[Stefan Berger]

On 11/1/22 16:17, Mimi Zohar wrote:
> Define a log_errno_reset macro to emit the errno string at or near the
> time of error, similar to the existing log_errno macro, but also reset
> errno to avoid dangling or duplicate errno messages on exit.
> 
> The initial usage is for non-critical file open failures.

After looking just at the fopen() in evmctl.c at the end of this series there are some that are left over that show no error message (read_binary_bios_measurements) others that still use log_err() then. Should they not all be converted/extended and use log_errno_reset()?


> 
> Suggested-by: Vitaly Chikunov <vt@altlinux.org>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/evmctl.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 0412bc0ac2b0..54123bf20f03 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -166,6 +166,9 @@ struct tpm_bank_info {
>   static char *pcrfile[MAX_PCRFILE];
>   static unsigned npcrfile;
>   
> +#define log_errno_reset(level, fmt, args...) \
> +	{do_log(level, fmt " (errno: %s)\n", ##args, strerror(errno)); errno = 0; }
> +
>   static int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
>   {
>   	FILE *fp;
> @@ -1911,8 +1914,10 @@ static int read_sysfs_pcrs(int num_banks, struct tpm_bank_info *tpm_banks)
>   	fp = fopen(pcrs, "r");
>   	if (!fp)
>   		fp = fopen(misc_pcrs, "r");
> -	if (!fp)
> +	if (!fp) {
> +		log_errno_reset(LOG_DEBUG, "Failed to read TPM 1.2 PCRs");
>   		return -1;
> +	}
>   
>   	result = read_one_bank(&tpm_banks[0], fp);
>   	fclose(fp);
> @@ -2055,7 +2060,6 @@ static int ima_measurement(const char *file)
>   	int err_padded = -1;
>   	int err = -1;
>   
> -	errno = 0;
>   	memset(zero, 0, MAX_DIGEST_SIZE);
>   
>   	pseudo_padded_banks = init_tpm_banks(&num_banks);
> @@ -2072,6 +2076,8 @@ static int ima_measurement(const char *file)
>   		init_public_keys(imaevm_params.keyfile);
>   	else				/* assume read pubkey from x509 cert */
>   		init_public_keys("/etc/keys/x509_evm.der");
> +	if (errno)
> +		log_errno_reset(LOG_DEBUG, "Failed to initialize public keys");
>   
>   	/*
>   	 * Reading the PCRs before walking the IMA measurement list
> @@ -2746,6 +2752,8 @@ int main(int argc, char *argv[])
>   	unsigned long keyid;
>   	char *eptr;
>   
> +	errno = 0;	/* initialize global errno */
> +
>   #if !(OPENSSL_VERSION_NUMBER < 0x10100000)
>   	OPENSSL_init_crypto(
>   #ifndef DISABLE_OPENSSL_CONF

Re: [PATCH ima-evm-utils v4 02/17] log and reset 'errno' after failure to open non-critical files

[Mimi Zohar]

On Wed, 2022-11-02 at 17:02 -0400, Stefan Berger wrote:
> 
> On 11/1/22 16:17, Mimi Zohar wrote:
> > Define a log_errno_reset macro to emit the errno string at or near the
> > time of error, similar to the existing log_errno macro, but also reset
> > errno to avoid dangling or duplicate errno messages on exit.
> > 
> > The initial usage is for non-critical file open failures.
> 
> After looking just at the fopen() in evmctl.c at the end of this
> series there are some that are left over that show no error message
> (read_binary_bios_measurements) others that still use log_err() then.
> Should they not all be converted/extended and use log_errno_reset()?
> 

No, log_errno_reset() is meant for the specific case where the program
continues to execute, but the errno message is delayed and emitted on
program exit.  In the case of read_binary_bios_measurements(), the
caller emits an error message and immediately exits.  There's no delay
between the error and the errno message.

Calling log_err() to emit an error mesage and then exiting seems to be
fine.

-- 
thanks,

Mimi

Re: [PATCH ima-evm-utils v4 01/17] Revert "Reset 'errno' after failure to open or access a file"

[Mimi Zohar]

On Tue, 2022-11-01 at 20:25 -0400, Stefan Berger wrote:
> 
> On 11/1/22 19:04, Mimi Zohar wrote:
> > Hi Stefan,
> > 
> > On Tue, 2022-11-01 at 17:46 -0400, Stefan Berger wrote:
> >>
> >> On 11/1/22 16:17, Mimi Zohar wrote:
> >>> This reverts commit acb19d1894a4a95471b8d2346cd6c3ecf3385110.
> >>
> >> $ git show acb19d1
> >> fatal: ambiguous argument 'acb19d1': unknown revision or path not in the working tree.
> >>
> >> Are you reverting this from a public tree? or could you just drop this patch from your series?
> >>
> >> Also after removing this patch from the mbox file I cannot apply these patches to my sourceforge checkout -- presumably next-testing branch:
> >>
> >> $ git am ./v4_20221101_zohar_address_deprecated_warnings.mbx
> >> Applying: log and reset 'errno' after failure to open non-critical files
> >> Applying: Log and reset 'errno' on lsetxattr failure
> >> Applying: travis: update dist=focal
> >> Applying: Update configure.ac to address a couple of obsolete warnings
> >> Applying: Deprecate IMA signature version 1
> >> error: patch failed: src/libimaevm.c:684
> >> error: src/libimaevm.c: patch does not apply
> >> Patch failed at 0005 Deprecate IMA signature version 1
> >> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> >> When you have resolved this problem, run "git am --continue".
> >> If you prefer to skip this patch, run "git am --skip" instead.
> >> To restore the original branch and stop patching, run "git am --abort".
> > 
> > Sorry, I should have used "--base=auto" when generating the patch set.
> > Yes, the patch set is based on the github next-testing branch.
> > 
> 
> Is the github repo now the main repo and sourceforge repo is dead?

The "next" branch in both repo's are the same.  Before posting patches,
I verify that github Actions works.   As a result, the next-testing
branch on github is rebased frequently.  Once a patch set is ready, the
"next" branch in both repo's is updated.

To answer your question the github repo is primary.

Mimi

Re: [PATCH ima-evm-utils v4 01/17] Revert "Reset 'errno' after failure to open or access a file"

[Petr Vorel]

> On Tue, 2022-11-01 at 20:25 -0400, Stefan Berger wrote:

> > On 11/1/22 19:04, Mimi Zohar wrote:
> > > Hi Stefan,

> > > On Tue, 2022-11-01 at 17:46 -0400, Stefan Berger wrote:

> > >> On 11/1/22 16:17, Mimi Zohar wrote:
> > >>> This reverts commit acb19d1894a4a95471b8d2346cd6c3ecf3385110.

> > >> $ git show acb19d1
> > >> fatal: ambiguous argument 'acb19d1': unknown revision or path not in the working tree.

> > >> Are you reverting this from a public tree? or could you just drop this patch from your series?

> > >> Also after removing this patch from the mbox file I cannot apply these patches to my sourceforge checkout -- presumably next-testing branch:

> > >> $ git am ./v4_20221101_zohar_address_deprecated_warnings.mbx
> > >> Applying: log and reset 'errno' after failure to open non-critical files
> > >> Applying: Log and reset 'errno' on lsetxattr failure
> > >> Applying: travis: update dist=focal
> > >> Applying: Update configure.ac to address a couple of obsolete warnings
> > >> Applying: Deprecate IMA signature version 1
> > >> error: patch failed: src/libimaevm.c:684
> > >> error: src/libimaevm.c: patch does not apply
> > >> Patch failed at 0005 Deprecate IMA signature version 1
> > >> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> > >> When you have resolved this problem, run "git am --continue".
> > >> If you prefer to skip this patch, run "git am --skip" instead.
> > >> To restore the original branch and stop patching, run "git am --abort".

> > > Sorry, I should have used "--base=auto" when generating the patch set.
> > > Yes, the patch set is based on the github next-testing branch.


> > Is the github repo now the main repo and sourceforge repo is dead?

> The "next" branch in both repo's are the same.  Before posting patches,
> I verify that github Actions works.   As a result, the next-testing
> branch on github is rebased frequently.  Once a patch set is ready, the
> "next" branch in both repo's is updated.

> To answer your question the github repo is primary.
Maybe deleting everything in sourceforge and ad put single file with link to
github.com would save you work (having master, next and next-testing branches
with this file).

Kind regards,
Petr

> Mimi

Re: [PATCH ima-evm-utils v4 01/17] Revert "Reset 'errno' after failure to open or access a file"

[Mimi Zohar]

Hi Petr,

> > > Is the github repo now the main repo and sourceforge repo is dead?
> 
> > The "next" branch in both repo's are the same.  Before posting patches,
> > I verify that github Actions works.   As a result, the next-testing
> > branch on github is rebased frequently.  Once a patch set is ready, the
> > "next" branch in both repo's is updated.
> 
> > To answer your question the github repo is primary.
> Maybe deleting everything in sourceforge and ad put single file with link to
> github.com would save you work (having master, next and next-testing branches
> with this file).

Thanks, definitely appreciate time saving tips!  Vitaly suggested
saving the sourceforge wiki info in ima-evm-utils and updating the
file(s) like any other file.  Before removing "everything"h from
sourceforge, that still needs to be done.

-- 
thanks,

Mimi

Re: [PATCH ima-evm-utils v4 01/17] Revert "Reset 'errno' after failure to open or access a file"

[Vitaly Chikunov]

On Thu, Nov 03, 2022 at 05:35:40PM -0400, Mimi Zohar wrote:
> Hi Petr,
> 
> > > > Is the github repo now the main repo and sourceforge repo is dead?
> > 
> > > The "next" branch in both repo's are the same.  Before posting patches,
> > > I verify that github Actions works.   As a result, the next-testing
> > > branch on github is rebased frequently.  Once a patch set is ready, the
> > > "next" branch in both repo's is updated.
> > 
> > > To answer your question the github repo is primary.
> > Maybe deleting everything in sourceforge and ad put single file with link to
> > github.com would save you work (having master, next and next-testing branches
> > with this file).
> 
> Thanks, definitely appreciate time saving tips!  Vitaly suggested
> saving the sourceforge wiki info in ima-evm-utils and updating the
> file(s) like any other file.  Before removing "everything"h from
> sourceforge, that still needs to be done.

I thought markdown should be downloaded from Edit interface, which is not
available on SF for non-admins. But I found recently how to download it
in Json format via Allura API:

  curl -s https://sourceforge.net/rest/p/linux-ima/wiki/Home  | jq -r .text

Then only end-of-lines need to be fixed.

Thanks,

> 
> -- 
> thanks,
> 
> Mimi

Re: [PATCH ima-evm-utils v4 01/17] Revert "Reset 'errno' after failure to open or access a file"

[Mimi Zohar]

On Fri, 2022-11-04 at 01:50 +0300, Vitaly Chikunov wrote:
> On Thu, Nov 03, 2022 at 05:35:40PM -0400, Mimi Zohar wrote:
> > Hi Petr,
> > 
> > > > > Is the github repo now the main repo and sourceforge repo is dead?
> > > 
> > > > The "next" branch in both repo's are the same.  Before posting patches,
> > > > I verify that github Actions works.   As a result, the next-testing
> > > > branch on github is rebased frequently.  Once a patch set is ready, the
> > > > "next" branch in both repo's is updated.
> > > 
> > > > To answer your question the github repo is primary.
> > > Maybe deleting everything in sourceforge and ad put single file with link to
> > > github.com would save you work (having master, next and next-testing branches
> > > with this file).
> > 
> > Thanks, definitely appreciate time saving tips!  Vitaly suggested
> > saving the sourceforge wiki info in ima-evm-utils and updating the
> > file(s) like any other file.  Before removing "everything"h from
> > sourceforge, that still needs to be done.
> 
> I thought markdown should be downloaded from Edit interface, which is not
> available on SF for non-admins. But I found recently hbow to download it
> in Json format via Allura API:
> 
>   curl -s https://sourceforge.net/rest/p/linux-ima/wiki/Home  | jq -r .text
> 
> Then only end-of-lines need to be fixed.

There's an admin option to export the wiki, but this is much better.
-- 
thanks,

Mimi