[PATCH ima-evm-utils] Check for tsspcrread in runtime

[PATCH ima-evm-utils] Check for tsspcrread in runtime

[Petr Vorel]

instead of checking in build time as it's runtime dependency.
Also log when tsspcrread not found to make debugging easier.

We search for tsspcrread unless there is tss2-esys with Esys_PCR_Read(),
thus pcr_none.c was dropped as unneeded.

file_exist(), file_exist() and MIN() taken from LTP project.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Hi Mimi,

small improvement based on the current next-testing branch
(9638068aff2476b567185d7eb94126449ad89ca7).

I'm sorry I don't have the required setup, thus didn't test this patch.

Kind regards,
Petr

 configure.ac         |  7 -----
 src/Makefile.am      |  4 ---
 src/evmctl.c         |  5 +--
 src/pcr.h            |  2 +-
 src/pcr_none.c       | 52 ------------------------------
 src/pcr_tss.c        |  2 +-
 src/pcr_tsspcrread.c | 22 ++++++++++---
 src/utils.c          | 75 ++++++++++++++++++++++++++++++++++++++++++++
 src/utils.h          |  1 +
 9 files changed, 99 insertions(+), 71 deletions(-)
 delete mode 100644 src/pcr_none.c

diff --git a/configure.ac b/configure.ac
index f246182..e7df7cd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -30,12 +30,6 @@ AC_SUBST(KERNEL_HEADERS)
 AC_CHECK_HEADER(unistd.h)
 AC_CHECK_HEADERS(openssl/conf.h)
 
-AC_CHECK_PROG(TSSPCRREAD, [tsspcrread], yes, no)
-if test "x$TSSPCRREAD" = "xyes"; then
-	AC_DEFINE(HAVE_TSSPCRREAD, 1, [Define to 1 if you have tsspcrread binary installed])
-fi
-AM_CONDITIONAL([USE_PCRTSSPCRREAD], [test "x$TSSPCRREAD" = "xyes"])
-
 AC_CHECK_LIB([tss2-esys], [Esys_PCR_Read])
 AC_CHECK_LIB([tss2-rc], [Tss2_RC_Decode])
 AM_CONDITIONAL([USE_PCRTSS], [test "x$ac_cv_lib_tss2_esys_Esys_PCR_Read" = "xyes"])
@@ -83,7 +77,6 @@ echo
 echo	"Configuration:"
 echo	"          debug: $pkg_cv_enable_debug"
 echo	"   openssl-conf: $enable_openssl_conf"
-echo	"     tsspcrread: $TSSPCRREAD"
 echo	"      tss2-esys: $ac_cv_lib_tss2_esys_Esys_PCR_Read"
 echo	" tss2-rc-decode: $ac_cv_lib_tss2_rc_Tss2_RC_Decode"
 echo
diff --git a/src/Makefile.am b/src/Makefile.am
index 9bbff50..ba9719b 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -25,11 +25,7 @@ evmctl_LDADD =  $(LIBCRYPTO_LIBS) -lkeyutils libimaevm.la
 if USE_PCRTSS
 evmctl_SOURCES += pcr_tss.c
 else
-if USE_PCRTSSPCRREAD
 evmctl_SOURCES += pcr_tsspcrread.c
-else
-evmctl_SOURCES += pcr_none.c
-endif
 endif
 
 AM_CPPFLAGS = -I$(top_srcdir) -include config.h
diff --git a/src/evmctl.c b/src/evmctl.c
index 90a3eeb..1d75e58 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1844,8 +1844,9 @@ static int read_tpm_banks(int num_banks, struct tpm_bank_info *bank)
 		return 0;
 
 	/* Any userspace applications available for reading TPM 2.0 PCRs? */
-	if (!tpm2_pcr_supported()) {
-		log_debug("Failed to read TPM 2.0 PCRs\n");
+	if (!tpm2_pcr_supported(&errmsg)) {
+		log_debug("Failed to read TPM 2.0 PCRs: (%s)\n", errmsg);
+		free(errmsg);
 		return 1;
 	}
 
diff --git a/src/pcr.h b/src/pcr.h
index 79547bd..0284f1c 100644
--- a/src/pcr.h
+++ b/src/pcr.h
@@ -1,3 +1,3 @@
-int tpm2_pcr_supported(void);
+int tpm2_pcr_supported(char **errmsg);
 int tpm2_pcr_read(const char *algo_name, int idx, uint8_t *hwpcr,
 		 int len, char **errmsg);
diff --git a/src/pcr_none.c b/src/pcr_none.c
deleted file mode 100644
index 43d053d..0000000
--- a/src/pcr_none.c
+++ /dev/null
@@ -1,52 +0,0 @@
-/*
- * ima-evm-utils - IMA/EVM support utilities
- *
- * Copyright (C) 2011 Nokia Corporation
- * Copyright (C) 2011,2012,2013 Intel Corporation
- * Copyright (C) 2013,2014 Samsung Electronics
- *
- * Authors:
- * Dmitry Kasatkin <dmitry.kasatkin@nokia.com>
- *                 <dmitry.kasatkin@intel.com>
- *                 <d.kasatkin@samsung.com>
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- *
- * As a special exception, the copyright holders give permission to link the
- * code of portions of this program with the OpenSSL library under certain
- * conditions as described in each individual source file and distribute
- * linked combinations including the program with the OpenSSL library. You
- * must comply with the GNU General Public License in all respects
- * for all of the code used other than as permitted herein. If you modify
- * file(s) with this exception, you may extend this exception to your
- * version of the file(s), but you are not obligated to do so. If you do not
- * wish to do so, delete this exception statement from your version. If you
- * delete this exception statement from all source files in the program,
- * then also delete it in the license file.
- *
- * File: pcr_none.c
- *	 PCR reading implementation that always fails
- */
-
-#include <stdint.h>
-
-int tpm2_pcr_supported(void)
-{
-	return 0;
-}
-
-int tpm2_pcr_read(const char *algo_name, int idx, uint8_t *hwpcr,
-		 int len, char **errmsg)
-{
-	return -1;
-}
diff --git a/src/pcr_tss.c b/src/pcr_tss.c
index da7be2e..87b8df3 100644
--- a/src/pcr_tss.c
+++ b/src/pcr_tss.c
@@ -51,7 +51,7 @@
 #endif
 #endif
 
-int tpm2_pcr_supported(void)
+int tpm2_pcr_supported(char **errmsg  __attribute__((unused)))
 {
 	return 1;
 }
diff --git a/src/pcr_tsspcrread.c b/src/pcr_tsspcrread.c
index 9c58dcb..3164a5d 100644
--- a/src/pcr_tsspcrread.c
+++ b/src/pcr_tsspcrread.c
@@ -39,6 +39,7 @@
  */
 
 #include <errno.h>
+#include <limits.h>
 #include <stdio.h>
 #include <string.h>
 #include <stdint.h>
@@ -47,8 +48,21 @@
 
 #include "utils.h"
 
-int tpm2_pcr_supported(void)
+#define CMD "tsspcrread"
+
+static char path[PATH_MAX];
+
+int tpm2_pcr_supported(char **errmsg)
 {
+	int ret;
+
+	if (get_cmd_path(CMD, path, sizeof(path))) {
+		ret = asprintf(errmsg, "Couldn't find '%s' in $PATH", CMD);
+		if (ret == -1)	/* the contents of errmsg is undefined */
+			*errmsg = NULL;
+		return 0;
+	}
+
 	return 1;
 }
 
@@ -57,11 +71,11 @@ int tpm2_pcr_read(const char *algo_name, int idx, uint8_t *hwpcr,
 {
 	FILE *fp;
 	char pcr[100];	/* may contain an error */
-	char cmd[50];
+	char cmd[PATH_MAX + 50];
 	int ret;
 
-	sprintf(cmd, "tsspcrread -halg %s -ha %d -ns 2> /dev/null",
-		algo_name, idx);
+	sprintf(cmd, "%s -halg %s -ha %d -ns 2> /dev/null",
+		path, algo_name, idx);
 	fp = popen(cmd, "r");
 	if (!fp) {
 		ret = asprintf(errmsg, "popen failed: %s", strerror(errno));
diff --git a/src/utils.c b/src/utils.c
index 22702ed..416a88c 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -1,7 +1,82 @@
 #include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
 
 #include "utils.h"
 
+#ifndef MIN
+# define MIN(a, b) ({ \
+	typeof(a) _a = (a); \
+	typeof(b) _b = (b); \
+	_a < _b ? _a : _b; \
+})
+#endif /* MIN */
+
+static int file_exist(const char *path)
+{
+	struct stat st;
+
+	if (!access(path, R_OK) && !stat(path, &st) && S_ISREG(st.st_mode))
+		return 1;
+
+	return 0;
+}
+
+int get_cmd_path(const char *prog_name, char *buf, size_t buf_len)
+{
+	const char *path = (const char *)getenv("PATH");
+	const char *start = path;
+	const char *end;
+	size_t size, ret;
+
+	if (path == NULL)
+		return -1;
+
+	do {
+		end = strchr(start, ':');
+
+		if (end != NULL)
+			snprintf(buf, MIN(buf_len, (size_t) (end - start + 1)),
+				 "%s", start);
+		else
+			snprintf(buf, buf_len, "%s", start);
+
+		size = strlen(buf);
+
+		/*
+		 * "::" inside $PATH, $PATH ending with ':' or $PATH starting
+		 * with ':' should be expanded into current working directory.
+		 */
+		if (size == 0) {
+			snprintf(buf, buf_len, ".");
+			size = strlen(buf);
+		}
+
+		/*
+		 * If there is no '/' ad the end of path from $PATH add it.
+		 */
+		if (buf[size - 1] != '/')
+			ret =
+			    snprintf(buf + size, buf_len - size, "/%s",
+				     prog_name);
+		else
+			ret =
+			    snprintf(buf + size, buf_len - size, "%s",
+				     prog_name);
+
+		if (buf_len - size > ret && file_exist(buf))
+			return 0;
+
+		start = end + 1;
+
+	} while (end != NULL);
+
+	return -1;
+}
+
 int hex_to_bin(char ch)
 {
 	if ((ch >= '0') && (ch <= '9'))
diff --git a/src/utils.h b/src/utils.h
index 7c0ce15..9ea179f 100644
--- a/src/utils.h
+++ b/src/utils.h
@@ -1,5 +1,6 @@
 #include <ctype.h>
 #include <sys/types.h>
 
+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);
-- 
2.27.0

Re: [PATCH ima-evm-utils] Check for tsspcrread in runtime

[Mimi Zohar]

On Tue, 2020-07-14 at 17:46 +0200, Petr Vorel wrote:
> instead of checking in build time as it's runtime dependency.
> Also log when tsspcrread not found to make debugging easier.
> 
> We search for tsspcrread unless there is tss2-esys with Esys_PCR_Read(),
> thus pcr_none.c was dropped as unneeded.
> 
> file_exist(), file_exist() and MIN() taken from LTP project.

One of these "file_exists" I assume is suppose to be "tst_get_path".

> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Hi Mimi,
> 
> small improvement based on the current next-testing branch
> (9638068aff2476b567185d7eb94126449ad89ca7).
> 
> I'm sorry I don't have the required setup, thus didn't test this patch.
> 
> Kind regards,
> Petr

Nice!  It works.

> diff --git a/src/pcr_tsspcrread.c b/src/pcr_tsspcrread.c
> @@ -47,8 +48,21 @@
> 
>  #include "utils.h"
> 
> -int tpm2_pcr_supported(void)
> +#define CMD "tsspcrread"
> +
> +static char path[PATH_MAX];
> +
> +int tpm2_pcr_supported(char **errmsg)
>  {
> +	int ret;
> +
> +	if (get_cmd_path(CMD, path, sizeof(path))) {
> +		ret = asprintf(errmsg, "Couldn't find '%s' in $PATH", CMD);
> +		if (ret == -1)	/* the contents of errmsg is undefined */
> +			*errmsg = NULL;
> +		return 0;
> +	}
> +

Any chance you could also emit the pathname on success as well?

>  	return 1;
>  }

Re: [PATCH ima-evm-utils] Check for tsspcrread in runtime

[Petr Vorel]

> On Tue, 2020-07-14 at 17:46 +0200, Petr Vorel wrote:
> > instead of checking in build time as it's runtime dependency.
> > Also log when tsspcrread not found to make debugging easier.

> > We search for tsspcrread unless there is tss2-esys with Esys_PCR_Read(),
> > thus pcr_none.c was dropped as unneeded.

> > file_exist(), file_exist() and MIN() taken from LTP project.

> One of these "file_exists" I assume is suppose to be "tst_get_path".
Yes. I'm sorry, thanks for catching it.


> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> > Hi Mimi,

> > small improvement based on the current next-testing branch
> > (9638068aff2476b567185d7eb94126449ad89ca7).

> > I'm sorry I don't have the required setup, thus didn't test this patch.

> > Kind regards,
> > Petr

> Nice!  It works.
Thanks a lot for a testing?

> > diff --git a/src/pcr_tsspcrread.c b/src/pcr_tsspcrread.c
> > @@ -47,8 +48,21 @@

> >  #include "utils.h"

> > -int tpm2_pcr_supported(void)
> > +#define CMD "tsspcrread"
> > +
> > +static char path[PATH_MAX];
> > +
> > +int tpm2_pcr_supported(char **errmsg)
> >  {
> > +	int ret;
> > +
> > +	if (get_cmd_path(CMD, path, sizeof(path))) {
> > +		ret = asprintf(errmsg, "Couldn't find '%s' in $PATH", CMD);
> > +		if (ret == -1)	/* the contents of errmsg is undefined */
> > +			*errmsg = NULL;
> > +		return 0;
> > +	}
> > +

> Any chance you could also emit the pathname on success as well?

Do you mean to print it into stderr:

int tpm2_pcr_supported(char **errmsg)
{
	int ret;

	if (get_cmd_path(CMD, path, sizeof(path))) {
		ret = asprintf(errmsg, "Couldn't find '%s' in $PATH", CMD);
		if (ret == -1)	/* the contents of errmsg is undefined */
			*errmsg = NULL;
		return 0;
	}

	ret = asprintf(errmsg, "Found '%s' in $PATH", CMD);
	if (ret == -1)	/* the contents of errmsg is undefined */
		*errmsg = NULL;
	return 1;
}

Shell I post v2 or you amend my patch?

BTW I was thinking to create custom function / macro for handling errmsg to
reduce duplicity.

+ there is minor warning on newer gcc, I'm not sure how to fix that:

evmctl.c: In function ‘read_tpm_banks’:
evmctl.c:1404:25: warning: ‘%2.2d’ directive writing between 2 and 10 bytes into a region of size 3 [-Wformat-overflow=]
 1404 |   sprintf(pcr_str, "PCR-%2.2d", i);
      |                         ^~~~~
evmctl.c:1404:20: note: directive argument in the range [0, 2147483647]
 1404 |   sprintf(pcr_str, "PCR-%2.2d", i);
      |                    ^~~~~~~~~~~
evmctl.c:1404:3: note: ‘sprintf’ output between 7 and 15 bytes into a destination of size 7
 1404 |   sprintf(pcr_str, "PCR-%2.2d", i);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Kind regards,
Petr

Re: [PATCH ima-evm-utils] Check for tsspcrread in runtime

[Mimi Zohar]

On Wed, 2020-07-15 at 08:21 +0200, Petr Vorel wrote:
> > On Tue, 2020-07-14 at 17:46 +0200, Petr Vorel wrote:
> > > instead of checking in build time as it's runtime dependency.
> > > Also log when tsspcrread not found to make debugging easier.
> 
> > > We search for tsspcrread unless there is tss2-esys with Esys_PCR_Read(),
> > > thus pcr_none.c was dropped as unneeded.
> 
> > > file_exist(), file_exist() and MIN() taken from LTP project.
> 
> > One of these "file_exists" I assume is suppose to be "tst_get_path".
> Yes. I'm sorry, thanks for catching it.
> 
> 
> > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > ---
> > > Hi Mimi,
> 
> > > small improvement based on the current next-testing branch
> > > (9638068aff2476b567185d7eb94126449ad89ca7).
> 
> > > I'm sorry I don't have the required setup, thus didn't test this patch.
> 
> > > Kind regards,
> > > Petr
> 
> > Nice!  It works.
> Thanks a lot for a testing?

Yes, reviewed/tested together.

diff --git a/src/pcr_tsspcrread.c b/src/pcr_tsspcrread.c
> > > @@ -47,8 +48,21 @@
> 
> > >  #include "utils.h"
> 
> > > -int tpm2_pcr_supported(void)
> > > +#define CMD "tsspcrread"
> > > +
> > > +static char path[PATH_MAX];
> > > +
> > > +int tpm2_pcr_supported(char **errmsg)
> > >  {
> > > +	int ret;
> > > +
> > > +	if (get_cmd_path(CMD, path, sizeof(path))) {
> > > +		ret = asprintf(errmsg, "Couldn't find '%s' in $PATH", CMD);
> > > +		if (ret == -1)	/* the contents of errmsg is undefined */
> > > +			*errmsg = NULL;
> > > +		return 0;
> > > +	}
> > > +
> 
> > Any chance you could also emit the pathname on success as well?
> 
> Do you mean to print it into stderr:
> 
> int tpm2_pcr_supported(char **errmsg)
> {
> 	int ret;
> 
> 	if (get_cmd_path(CMD, path, sizeof(path))) {
> 		ret = asprintf(errmsg, "Couldn't find '%s' in $PATH", CMD);
> 		if (ret == -1)	/* the contents of errmsg is undefined */
> 			*errmsg = NULL;
> 		return 0;
> 	}
> 
> 	ret = asprintf(errmsg, "Found '%s' in $PATH", CMD);
> 	if (ret == -1)	/* the contents of errmsg is undefined */
> 		*errmsg = NULL;
> 	return 1;
> }
> 

When running these tests remotely, it helps to know which method of
reading the PCRs is used.  How about adding something like this to
both instances of tpm2_pcr_supported()?

        if (imaevm_params.verbose > LOG_INFO)
                log_info("Using %s to read PCRs.\n", CMD);

> Shell I post v2 or you amend my patch?

Either way is fine. 

> BTW I was thinking to create custom function / macro for handling errmsg to
> reduce duplicity.

Sure, I assume that would be in addition to log_err() and log_errno().

> 
> + there is minor warning on newer gcc, I'm not sure how to fix that:
> 
> evmctl.c: In function ‘read_tpm_banks’:
> evmctl.c:1404:25: warning: ‘%2.2d’ directive writing between 2 and 10 bytes into a region of size 3 [-Wformat-overflow=]
>  1404 |   sprintf(pcr_str, "PCR-%2.2d", i);
>       |                         ^~~~~
> evmctl.c:1404:20: note: directive argument in the range [0, 2147483647]
>  1404 |   sprintf(pcr_str, "PCR-%2.2d", i);
>       |                    ^~~~~~~~~~~
> evmctl.c:1404:3: note: ‘sprintf’ output between 7 and 15 bytes into a destination of size 7
>  1404 |   sprintf(pcr_str, "PCR-%2.2d", i);
>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Interesting.  Checking that "i" isn't greater than 99 solves this
warning.  Changing pcr_str size from 7 to 8 solves the other warning.

Mimi

Re: [PATCH ima-evm-utils] Check for tsspcrread in runtime

[Petr Vorel]

Hi Mimi,

> > > Nice!  It works.
> > Thanks a lot for a testing?

> Yes, reviewed/tested together.
Sorry, I put question mark by accident, but thanks for confirmation anyway.

...
> When running these tests remotely, it helps to know which method of
> reading the PCRs is used.  How about adding something like this to
> both instances of tpm2_pcr_supported()?

>         if (imaevm_params.verbose > LOG_INFO)
>                 log_info("Using %s to read PCRs.\n", CMD);

+1

> > Shell I post v2 or you amend my patch?

> Either way is fine. 
Sending v2 in a minute. Feel free to amend it to suit your needs.

> > BTW I was thinking to create custom function / macro for handling errmsg to
> > reduce duplicity.

> Sure, I assume that would be in addition to log_err() and log_errno().
I'll probably postpone this cleanup after this patchset is merged (unless you do
the cleanup yourself). It can even wait after the release, I don't want to block
release with minor cleanup.


> > + there is minor warning on newer gcc, I'm not sure how to fix that:

> > evmctl.c: In function ‘read_tpm_banks’:
> > evmctl.c:1404:25: warning: ‘%2.2d’ directive writing between 2 and 10 bytes into a region of size 3 [-Wformat-overflow=]
> >  1404 |   sprintf(pcr_str, "PCR-%2.2d", i);
> >       |                         ^~~~~
> > evmctl.c:1404:20: note: directive argument in the range [0, 2147483647]
> >  1404 |   sprintf(pcr_str, "PCR-%2.2d", i);
> >       |                    ^~~~~~~~~~~
> > evmctl.c:1404:3: note: ‘sprintf’ output between 7 and 15 bytes into a destination of size 7
> >  1404 |   sprintf(pcr_str, "PCR-%2.2d", i);
> >       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> Interesting.  Checking that "i" isn't greater than 99 solves this
> warning.  Changing pcr_str size from 7 to 8 solves the other warning.
Nice, how simple. I wasn't sure myself about changing of the array size.
Feel free to just fix it.

Kind regards,
Petr