[PATCH v2 0/5] Updates to use IBM TSS C API rather than command line tools

[PATCH v2 0/5] Updates to use IBM TSS C API rather than command line tools

[Ken Goldman]

This patch set replaces calls to command line tools with C code.  This
should be more stable and easier to maintain.  INSTALL instructions
are updated.

While the code is being touched, it changes the PCR handle from
a signed int to a uint32_t, removing the need for error checking.

Finally, it fixes an environment variable issue and some spelling
errors.

Ken Goldman (5):
  ima-evm-utils: Change env variable TPM_SERVER_TYPE for tpm_server
  ima-evm-utils: Change PCR iterater from int to uint32_t
  ima-evm-utils: Change tpm2_pcr_read() to use C code
  ima-evm-utils: Correct spelling errors
  ima-evm-utils: Expand the INSTALL instructions.

 INSTALL                   |  25 +++++-
 src/Makefile.am           |   1 +
 src/evmctl.c              |   9 ++-
 src/pcr.h                 |   2 +-
 src/pcr_tss.c             |   4 +-
 src/pcr_tsspcrread.c      | 156 +++++++++++++++++++++++++++++---------
 tests/boot_aggregate.test |   7 +-
 7 files changed, 159 insertions(+), 45 deletions(-)

-- 
2.25.1

[PATCH v2 1/5] ima-evm-utils: Change env variable TPM_SERVER_TYPE for tpm_server

[Ken Goldman]

The default value raw is appropriate for 'swtpm'.  tpm_server
uses the Microsoft packet encapsulation, so the env variable
must have the value mssim.

Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>
---
 tests/boot_aggregate.test | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/boot_aggregate.test b/tests/boot_aggregate.test
index 1c7b1f2..b109a32 100755
--- a/tests/boot_aggregate.test
+++ b/tests/boot_aggregate.test
@@ -35,6 +35,7 @@ else
 	export TPM_COMMAND_PORT=2321
 	export TPM_PLATFORM_PORT=2322
 	export TPM_SERVER_NAME="localhost"
+	# swtpm uses the raw, unencapsulated packet format
 	export TPM_SERVER_TYPE="raw"
 
 fi
@@ -73,6 +74,8 @@ swtpm_start() {
 			SWTPM_PPID=$!
 		fi
 	elif [ -n "${swtpm}" ]; then
+	        # tpm_server uses the Microsoft simulator encapsulated packet format
+                export TPM_SERVER_TYPE="mssim"
 		pgrep swtpm
 		if [ $? -eq 0 ]; then
 			echo "INFO: Software TPM (tpm_server) already running"
-- 
2.25.1

[PATCH v2 2/5] ima-evm-utils: Change PCR iterater from int to uint32_t

[Ken Goldman]

PCR numbers are naturally unsigned values.  Further, they are
32 bits, even on 64-bit machines. This change eliminates the
need for negative value and overflow tests.

The parameter name is changed from j and idx to pcrHandle,
which is more decriptive and is the parameter name used in
the TPM 2.0 specification.

Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>
---
 src/evmctl.c         | 9 +++++----
 src/pcr.h            | 2 +-
 src/pcr_tss.c        | 4 ++--
 src/pcr_tsspcrread.c | 4 ++--
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 1815f55..b056a1e 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1895,7 +1895,8 @@ static int read_tpm_banks(int num_banks, struct tpm_bank_info *bank)
 {
 	int tpm_enabled = 0;
 	char *errmsg = NULL;
-	int i, j;
+	int i;
+	uint32_t pcrHandle;
 	int err;
 
 	/* If --pcrs was specified, read only from the specified file(s) */
@@ -1915,9 +1916,9 @@ static int read_tpm_banks(int num_banks, struct tpm_bank_info *bank)
 	/* Read PCRs from multiple TPM 2.0 banks */
 	for (i = 0; i < num_banks; i++) {
 		err = 0;
-		for (j = 0; j < NUM_PCRS && !err; j++) {
-			err = tpm2_pcr_read(bank[i].algo_name, j,
-					    bank[i].pcr[j], bank[i].digest_size,
+		for (pcrHandle = 0; pcrHandle < NUM_PCRS && !err; pcrHandle++) {
+			err = tpm2_pcr_read(bank[i].algo_name, pcrHandle,
+					    bank[i].pcr[pcrHandle], bank[i].digest_size,
 					    &errmsg);
 			if (err) {
 				log_debug("Failed to read %s PCRs: (%s)\n",
diff --git a/src/pcr.h b/src/pcr.h
index 79547bd..dd8311a 100644
--- a/src/pcr.h
+++ b/src/pcr.h
@@ -1,3 +1,3 @@
 int tpm2_pcr_supported(void);
-int tpm2_pcr_read(const char *algo_name, int idx, uint8_t *hwpcr,
+int tpm2_pcr_read(const char *algo_name, uint32_t pcrHandle, uint8_t *hwpcr,
 		 int len, char **errmsg);
diff --git a/src/pcr_tss.c b/src/pcr_tss.c
index feb1ff7..5e00524 100644
--- a/src/pcr_tss.c
+++ b/src/pcr_tss.c
@@ -106,7 +106,7 @@ static TPM2_ALG_ID algo_to_tss2(const char *algo_name)
 	return TPM2_ALG_ERROR;
 }
 
-int tpm2_pcr_read(const char *algo_name, int idx, uint8_t *hwpcr,
+int tpm2_pcr_read(const char *algo_name, uint32_t pcrHandle, uint8_t *hwpcr,
 		 int len, char **errmsg)
 {
 	TSS2_ABI_VERSION abi_version = {
@@ -140,7 +140,7 @@ int tpm2_pcr_read(const char *algo_name, int idx, uint8_t *hwpcr,
 		}
 	};
 
-	pcr_select_in.pcrSelections[0].pcrSelect[idx / 8] = (1 << (idx % 8));
+	pcr_select_in.pcrSelections[0].pcrSelect[pcrHandle / 8] = (1 << (pcrHandle % 8));
 
 	ret = Esys_Initialize(&ctx, NULL, &abi_version);
 	if (ret != TPM2_RC_SUCCESS) {
diff --git a/src/pcr_tsspcrread.c b/src/pcr_tsspcrread.c
index 462f270..118c7d2 100644
--- a/src/pcr_tsspcrread.c
+++ b/src/pcr_tsspcrread.c
@@ -68,7 +68,7 @@ int tpm2_pcr_supported(void)
 	return 1;
 }
 
-int tpm2_pcr_read(const char *algo_name, int idx, uint8_t *hwpcr,
+int tpm2_pcr_read(const char *algo_name, uint32_t pcrHandle, uint8_t *hwpcr,
 		 int len, char **errmsg)
 {
 	FILE *fp;
@@ -77,7 +77,7 @@ int tpm2_pcr_read(const char *algo_name, int idx, uint8_t *hwpcr,
 	int ret;
 
 	sprintf(cmd, "%s -halg %s -ha %d -ns 2> /dev/null",
-		path, algo_name, idx);
+		path, algo_name, pcrHandle);
 	fp = popen(cmd, "r");
 	if (!fp) {
 		ret = asprintf(errmsg, "popen failed: %s", strerror(errno));
-- 
2.25.1

[PATCH v2 3/5] ima-evm-utils: Change tpm2_pcr_read() to use C code

[Ken Goldman]

Replace the call out to the command line tools with C functions.

The algorithm_string_to_algid() function supports only the digest
algorithms in use.  The table has place holders for other agorithms as
they are needed and the C strings are defined.

The table can also be used for an algrithm ID to string function if
it's ever needed.

When using the IBM TSS, link in its library.

Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>
---
 src/Makefile.am      |   1 +
 src/pcr_tsspcrread.c | 156 +++++++++++++++++++++++++++++++++----------
 2 files changed, 123 insertions(+), 34 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index d6c779f..bf18caf 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -26,6 +26,7 @@ if USE_PCRTSS
 evmctl_SOURCES += pcr_tss.c
 else
 evmctl_SOURCES += pcr_tsspcrread.c
+evmctl_LDADD += -libmtss
 endif
 
 AM_CPPFLAGS = -I$(top_srcdir) -include config.h
diff --git a/src/pcr_tsspcrread.c b/src/pcr_tsspcrread.c
index 118c7d2..eae68b7 100644
--- a/src/pcr_tsspcrread.c
+++ b/src/pcr_tsspcrread.c
@@ -50,6 +50,10 @@
 #include "utils.h"
 #include "imaevm.h"
 
+#define TPM_POSIX	/* use Posix, not Windows constructs in TSS */
+#undef MAX_DIGEST_SIZE	/* imaevm uses a different value than the TSS */
+#include <ibmtss/tss.h>
+
 #define CMD "tsspcrread"
 
 static char path[PATH_MAX];
@@ -68,44 +72,128 @@ int tpm2_pcr_supported(void)
 	return 1;
 }
 
-int tpm2_pcr_read(const char *algo_name, uint32_t pcrHandle, uint8_t *hwpcr,
-		 int len, char **errmsg)
-{
-	FILE *fp;
-	char pcr[100];	/* may contain an error */
-	char cmd[PATH_MAX + 50];
-	int ret;
-
-	sprintf(cmd, "%s -halg %s -ha %d -ns 2> /dev/null",
-		path, algo_name, pcrHandle);
-	fp = popen(cmd, "r");
-	if (!fp) {
-		ret = asprintf(errmsg, "popen failed: %s", strerror(errno));
-		if (ret == -1)	/* the contents of errmsg is undefined */
-			*errmsg = NULL;
-		return -1;
-	}
+/* Table mapping C strings to TCG algorithm identifiers */
+
+typedef struct tdAlgorithm_Map {
+    const char *algorithm_string;
+    TPMI_ALG_HASH algid;
+} Algorithm_Map;
 
-	if (fgets(pcr, sizeof(pcr), fp) == NULL) {
-		ret = asprintf(errmsg, "tsspcrread failed: %s",
-			       strerror(errno));
-		if (ret == -1)	/* the contents of errmsg is undefined */
-			*errmsg = NULL;
-		ret = pclose(fp);
-		return -1;
+Algorithm_Map algorithm_map[] = {
+				 { "sha1", TPM_ALG_SHA1},
+				 { "sha256", TPM_ALG_SHA256},
+#if 0	/* uncomment as these digest algorithms are supported */
+				 { "", TPM_ALG_SHA384},
+				 { "", TPM_ALG_SHA512},
+				 { "", TPM_ALG_SM3_256},
+				 { "", TPM_ALG_SHA3_256},
+				 { "", TPM_ALG_SHA3_384},
+				 { "", TPM_ALG_SHA3_512},
+#endif
+};
+
+/* algorithm_string_to_algid() converts a digest algorithm from a C string to a TCG algorithm
+   identifier as defined in the TCG Algorithm Regisrty..
+
+   Returns TPM_ALG_ERROR if the string has an unsupported value.
+*/
+
+static TPMI_ALG_HASH algorithm_string_to_algid(const char *algorithm_string)
+{
+    size_t 	i;
+    for (i=0 ; i < sizeof(algorithm_map)/sizeof(Algorithm_Map) ; i++) {
+	if (strcmp(algorithm_string, algorithm_map[i].algorithm_string) == 0) {
+	    return algorithm_map[i].algid; /* if match */
 	}
+    }
+    return TPM_ALG_ERROR;
+}
 
-	/* get the popen "cmd" return code */
-	ret = pclose(fp);
+/* tpm2_pcr_read() reads the PCR
 
-	/* Treat an unallocated bank as an error */
-	if (!ret && (strlen(pcr) < SHA_DIGEST_LENGTH))
-		ret = -1;
+   algo_name is the PCR digest algorithm (the PCR bank) as a C string
+   pcrHandle is the PCR number to read
+   hwpcr is a buffer for the PCR output in binary
+   len is the allocated size of hwpcr and should match the digest algorithm
+*/
 
-	if (!ret)
-		hex2bin(hwpcr, pcr, len);
-	else
-		*errmsg = strndup(pcr, strlen(pcr) - 1); /* remove newline */
+int tpm2_pcr_read(const char *algo_name, uint32_t pcrHandle, uint8_t *hwpcr,
+		 int len, char **errmsg)
+{
+        int 			ret = 0;	/* function return code */
+	TPM_RC			rc = 0;		/* TCG return code */
+	PCR_Read_In 		pcrReadIn;	/* command input */
+	PCR_Read_Out 		pcrReadOut;	/* response output */
+	TSS_CONTEXT		*tssContext = NULL;
+	TPMI_ALG_HASH 		alg_id;		/* PCR algorithm */
 
-	return ret;
+	if (rc == 0) {		/* map algorithm string to TCG value */
+	    alg_id = algorithm_string_to_algid(algo_name);
+	    if (alg_id == TPM_ALG_ERROR) {
+		ret = asprintf(errmsg, "tpm2_pcr_read: unknown algorithm %s", algo_name);
+		if (ret == -1) {	/* the contents of errmsg is undefined */
+		    *errmsg = NULL;
+		}
+		rc = 1;
+	    }
+	}
+	if (rc == 0) {
+	    rc = TSS_Create(&tssContext);
+	}
+	/* call TSS to execute the command */
+	if (rc == 0) {
+	    pcrReadIn.pcrSelectionIn.count = 1;
+	    pcrReadIn.pcrSelectionIn.pcrSelections[0].hash = alg_id;
+	    pcrReadIn.pcrSelectionIn.pcrSelections[0].sizeofSelect = 3;
+	    pcrReadIn.pcrSelectionIn.pcrSelections[0].pcrSelect[0] = 0;
+	    pcrReadIn.pcrSelectionIn.pcrSelections[0].pcrSelect[1] = 0;
+	    pcrReadIn.pcrSelectionIn.pcrSelections[0].pcrSelect[2] = 0;
+	    pcrReadIn.pcrSelectionIn.pcrSelections[0].pcrSelect[pcrHandle / 8] =
+		1 << (pcrHandle % 8);
+	    rc = TSS_Execute(tssContext,
+			     (RESPONSE_PARAMETERS *)&pcrReadOut,
+			     (COMMAND_PARAMETERS *)&pcrReadIn,
+			     NULL,
+			     TPM_CC_PCR_Read,
+			     TPM_RH_NULL, NULL, 0);
+	}
+	if (rc == 0) {
+	    /* nothing read, bank missing */
+	    if (pcrReadOut.pcrValues.count == 0) {
+		ret = asprintf(errmsg, "tpm2_pcr_read: returned count 0 for %s", algo_name);
+		if (ret == -1) {	/* the contents of errmsg is undefined */
+		    *errmsg = NULL;
+		}
+		rc = 1;
+	    }
+	    /* len parameter did not match the digest algorithm */
+	    else if (pcrReadOut.pcrValues.digests[0].t.size != len) {
+		ret = asprintf(errmsg,
+			       "tpm2_pcr_read: expected length %d actual %u for %s",
+			       len, pcrReadOut.pcrValues.digests[0].t.size, algo_name);
+		if (ret == -1) {	/* the contents of errmsg is undefined */
+		    *errmsg = NULL;
+		}
+		rc = 1;
+	    }
+	    else {
+		memcpy(hwpcr,
+		       pcrReadOut.pcrValues.digests[0].t.buffer,
+		       pcrReadOut.pcrValues.digests[0].t.size);
+	    }
+	}
+	{
+	    TPM_RC rc1 = TSS_Delete(tssContext);
+	    if (rc == 0) {
+		rc = rc1;
+	    }
+	}
+	/* map TCG return code to function return code */
+	if (rc == 0) {
+	    return 0;
+	}
+	else {
+	    return -1;
+	}
 }
+
-- 
2.25.1

[PATCH v2 4/5] ima-evm-utils: Correct spelling errors

[Ken Goldman]

In comments and error messages.  No impact to code.

Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>
---
 tests/boot_aggregate.test | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/boot_aggregate.test b/tests/boot_aggregate.test
index b109a32..9967de2 100755
--- a/tests/boot_aggregate.test
+++ b/tests/boot_aggregate.test
@@ -58,7 +58,7 @@ swtpm_start() {
 	swtpm="$(which tpm_server)"
 	swtpm1="$(which swtpm)"
 	if [ -z "${swtpm}" ] && [ -z "${swtpm1}" ]; then
-		echo "${CYAN}SKIP: Softare TPM (tpm_server and swtpm) not found${NORM}"
+		echo "${CYAN}SKIP: Software TPM (tpm_server and swtpm) not found${NORM}"
 		return "$SKIP"
 	fi
 
@@ -122,7 +122,7 @@ display_pcrs() {
 	done
 }
 
-# The first entry in the IMA measuremnet list is the "boot_aggregate".
+# The first entry in the IMA measurement list is the "boot_aggregate".
 # For each kexec, an additional "boot_aggregate" will appear in the
 # measurement list, assuming the previous measurement list is carried
 # across the kexec.
-- 
2.25.1

[PATCH v2 5/5] ima-evm-utils: Expand the INSTALL instructions.

[Ken Goldman]

Add some of the less obvious package, TPM, and TSS prerequisites.

autoreconf -i is required before ./configure

Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>
---
 INSTALL | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/INSTALL b/INSTALL
index 007e939..58a1f46 100644
--- a/INSTALL
+++ b/INSTALL
@@ -9,10 +9,31 @@ are permitted in any medium without royalty provided the copyright
 notice and this notice are preserved.  This file is offered as-is,
 without warranty of any kind.
 
+Prerequisites
+=============
+
+This project has the following prerequisites:
+
+(Ubuntu package names)
+	libkeyutils-dev
+	libtasn1-dev
+	libgmp-dev
+	libnspr4-dev
+	libnss3-dev
+
+These software TPMs are supported:
+      https://github.com/stefanberger/swtpm
+      https://sourceforge.net/projects/ibmswtpm2/
+      https://github.com/stefanberger/libtpms
+
+Supported TSSes include these.  Both are included in some distros.
+	  IBM TSS https://sourceforge.net/projects/ibmtpm20tss/
+	  Intel TSS
+
 Basic Installation
 ==================
 
-   Briefly, the shell commands `./configure; make; make install' should
+   Briefly, the shell commands `autoreconf -i; ./configure; make; make install' should
 configure, build, and install this package.  The following
 more-detailed instructions are generic; see the `README' file for
 instructions specific to this package.  Some packages provide this
@@ -51,7 +72,7 @@ of `autoconf'.
    The simplest way to compile this package is:
 
   1. `cd' to the directory containing the package's source code and type
-     `./configure' to configure the package for your system.
+     `autoreconf -i' and then `./configure' to configure the package for your system.
 
      Running `configure' might take a while.  While running, it prints
      some messages telling which features it is checking for.
-- 
2.25.1

Re: [PATCH v2 1/5] ima-evm-utils: Change env variable TPM_SERVER_TYPE for tpm_server

[Mimi Zohar]

Hi Ken,

On Mon, 2020-10-12 at 19:44 -0400, Ken Goldman wrote:
> The default value raw is appropriate for 'swtpm'.  tpm_server
> uses the Microsoft packet encapsulation, so the env variable
> must have the value mssim.
> 
> Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>

Thank you for noticing this regression.

> ---
>  tests/boot_aggregate.test | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/boot_aggregate.test b/tests/boot_aggregate.test
> index 1c7b1f2..b109a32 100755
> --- a/tests/boot_aggregate.test
> +++ b/tests/boot_aggregate.test
> @@ -35,6 +35,7 @@ else
>  	export TPM_COMMAND_PORT=2321
>  	export TPM_PLATFORM_PORT=2322
>  	export TPM_SERVER_NAME="localhost"
> +	# swtpm uses the raw, unencapsulated packet format
>  	export TPM_SERVER_TYPE="raw"

Instead of adding a comment here, how about only exporting
TPM_SERVER_TYPE for "swtpm".

>  
>  fi
> @@ -73,6 +74,8 @@ swtpm_start() {
>  			SWTPM_PPID=$!
>  		fi
>  	elif [ -n "${swtpm}" ]; then
> +	        # tpm_server uses the Microsoft simulator encapsulated packet format
> +                export TPM_SERVER_TYPE="mssim"

Exporting TPM_SERVER_TYPE like this is causing openssl/tumbleweed to
fail.

thanks,

Mimi

>  		pgrep swtpm
>  		if [ $? -eq 0 ]; then
>  			echo "INFO: Software TPM (tpm_server) already running"

Re: [PATCH v2 1/5] ima-evm-utils: Change env variable TPM_SERVER_TYPE for tpm_server

[Ken Goldman]

On 10/14/2020 6:04 PM, Mimi Zohar wrote:
> Hi Ken,
> 
> On Mon, 2020-10-12 at 19:44 -0400, Ken Goldman wrote:
>> The default value raw is appropriate for 'swtpm'.  tpm_server
>> uses the Microsoft packet encapsulation, so the env variable
>> must have the value mssim.
>>
>> Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>
> 
> Thank you for noticing this regression.
> 
>> ---
>>   tests/boot_aggregate.test | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/tests/boot_aggregate.test b/tests/boot_aggregate.test
>> index 1c7b1f2..b109a32 100755
>> --- a/tests/boot_aggregate.test
>> +++ b/tests/boot_aggregate.test
>> @@ -35,6 +35,7 @@ else
>>   	export TPM_COMMAND_PORT=2321
>>   	export TPM_PLATFORM_PORT=2322
>>   	export TPM_SERVER_NAME="localhost"
>> +	# swtpm uses the raw, unencapsulated packet format
>>   	export TPM_SERVER_TYPE="raw"
> 
> Instead of adding a comment here, how about only exporting
> TPM_SERVER_TYPE for "swtpm".

That certainly works.  I thought the idea was, "Make the
smallest change that fixes the problem."   Moving that
line under swtpm is a reasonable alternative.

I'd leave the comment.  I suspect many people
don't know about the Microsoft TPM packet format,
so the line would otherwise be confusing.

> 
>>   
>>   fi
>> @@ -73,6 +74,8 @@ swtpm_start() {
>>   			SWTPM_PPID=$!
>>   		fi
>>   	elif [ -n "${swtpm}" ]; then
>> +	        # tpm_server uses the Microsoft simulator encapsulated packet format
>> +                export TPM_SERVER_TYPE="mssim"
> 
> Exporting TPM_SERVER_TYPE like this is causing openssl/tumbleweed to
> fail.
> 

That's odd.  Are you saying that openssl uses the env variable
TPM_SERVER_TYPE?  What in openssl fails?  What's the error
message.

Re: [PATCH v2 1/5] ima-evm-utils: Change env variable TPM_SERVER_TYPE for tpm_server

[Mimi Zohar]

On Wed, 2020-10-14 at 18:17 -0400, Ken Goldman wrote:
> On 10/14/2020 6:04 PM, Mimi Zohar wrote:
> > Hi Ken,
> > 
> > On Mon, 2020-10-12 at 19:44 -0400, Ken Goldman wrote:
> >> The default value raw is appropriate for 'swtpm'.  tpm_server
> >> uses the Microsoft packet encapsulation, so the env variable
> >> must have the value mssim.
> >>
> >> Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>
> > 
> > Thank you for noticing this regression.
> > 
> >> ---
> >>   tests/boot_aggregate.test | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/tests/boot_aggregate.test b/tests/boot_aggregate.test
> >> index 1c7b1f2..b109a32 100755
> >> --- a/tests/boot_aggregate.test
> >> +++ b/tests/boot_aggregate.test
> >> @@ -35,6 +35,7 @@ else
> >>   	export TPM_COMMAND_PORT=2321
> >>   	export TPM_PLATFORM_PORT=2322
> >>   	export TPM_SERVER_NAME="localhost"
> >> +	# swtpm uses the raw, unencapsulated packet format
> >>   	export TPM_SERVER_TYPE="raw"
> > 
> > Instead of adding a comment here, how about only exporting
> > TPM_SERVER_TYPE for "swtpm".
> 
> That certainly works.  I thought the idea was, "Make the
> smallest change that fixes the problem."   Moving that
> line under swtpm is a reasonable alternative.

In this case, moving the line and adding the comment is the smallest
change.  To indicate this is a bug fix, you would add "Fixes:
f831508297cd ("Install the swtpm package, if available") in addition to
your Signed-off-by tag.

> 
> I'd leave the comment.  I suspect many people
> don't know about the Microsoft TPM packet format,
> so the line would otherwise be confusing.
> 
> > 
> >>   
> >>   fi
> >> @@ -73,6 +74,8 @@ swtpm_start() {
> >>   			SWTPM_PPID=$!
> >>   		fi
> >>   	elif [ -n "${swtpm}" ]; then
> >> +	        # tpm_server uses the Microsoft simulator encapsulated packet format
> >> +                export TPM_SERVER_TYPE="mssim"
> > 
> > Exporting TPM_SERVER_TYPE like this is causing openssl/tumbleweed to
> > fail.
> > 
> 
> That's odd.  Are you saying that openssl uses the env variable
> TPM_SERVER_TYPE?  What in openssl fails?  What's the error
> message.

"make check" is showing:


TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
3: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
4: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
5: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
6: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
7: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
8: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
9: pcrread: failed, rc 00000100
TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
initialized
INFO: Calculating the boot_aggregate (PCRs 0 - 9) for multiple banks
Failed to read any TPM PCRs
errno: No such file or directory (2)
SKIP: evmctl ima_boot_aggregate: 

thanks,

Mimi

Re: [PATCH v2 1/5] ima-evm-utils: Change env variable TPM_SERVER_TYPE for tpm_server

[Ken Goldman]

On 10/14/2020 6:28 PM, Mimi Zohar wrote:
>>> Instead of adding a comment here, how about only exporting
>>> TPM_SERVER_TYPE for "swtpm".

>> That certainly works.  I thought the idea was, "Make the
>> smallest change that fixes the problem."   Moving that
>> line under swtpm is a reasonable alternative.

> In this case, moving the line and adding the comment is the smallest
> change.  To indicate this is a bug fix, you would add "Fixes:
> f831508297cd ("Install the swtpm package, if available") in addition to
> your Signed-off-by tag.
> 

The current patch adds one line.  This proposal adds one line and
moves another line.  It also changes the swtpm flow, which must
be tested.

It's OK with me.  Let me know.

Where should I add that "Fixes ..." text?  What is the exact format?

Re: [PATCH v2 1/5] ima-evm-utils: Change env variable TPM_SERVER_TYPE for tpm_server

[Ken Goldman]

On 10/14/2020 6:28 PM, Mimi Zohar wrote:
 >>>>
 >>>>    fi
 >>>> @@ -73,6 +74,8 @@ swtpm_start() {
 >>>>    			SWTPM_PPID=$!
 >>>>    		fi
 >>>>    	elif [ -n "${swtpm}" ]; then
 >>>> +	        # tpm_server uses the Microsoft simulator encapsulated packet format
 >>>> +                export TPM_SERVER_TYPE="mssim"
 >>> Exporting TPM_SERVER_TYPE like this is causing openssl/tumbleweed to
 >>> fail.
 >>>
 >> That's odd.  Are you saying that openssl uses the env variable
 >> TPM_SERVER_TYPE?  What in openssl fails?  What's the error
 >> message.
 > "make check" is showing:
 >
 >
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 3: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 4: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 5: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 6: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 7: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 8: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > 9: pcrread: failed, rc 00000100
 > TPM_RC_INITIALIZE - TPM not initialized by TPM2_Startup or already
 > initialized
 > INFO: Calculating the boot_aggregate (PCRs 0 - 9) for multiple banks
 > Failed to read any TPM PCRs
 > errno: No such file or directory (2)
 > SKIP: evmctl ima_boot_aggregate:

Are you sure that this failure is within openssl?  It doesn't look
that way to me.

Were there perhaps more of those errors?  I suspect that because
the messages are labeled 3-9, but PCR 0-9 are read.

I don't know the test code.  My guess is:

- If there were 10 errors, the startup command is missing,
causing each PCR read to fail.

- If there were 9 errors, startup is being sent before each PCR read,
but only one is permitted.

Re: [PATCH v2 1/5] ima-evm-utils: Change env variable TPM_SERVER_TYPE for tpm_server

[Mimi Zohar]

On Thu, 2020-10-15 at 08:54 -0400, Ken Goldman wrote:
> On 10/14/2020 6:28 PM, Mimi Zohar wrote:
> >>> Instead of adding a comment here, how about only exporting
> >>> TPM_SERVER_TYPE for "swtpm".
> 
> >> That certainly works.  I thought the idea was, "Make the
> >> smallest change that fixes the problem."   Moving that
> >> line under swtpm is a reasonable alternative.
> 
> > In this case, moving the line and adding the comment is the smallest
> > change.  To indicate this is a bug fix, you would add "Fixes:
> > f831508297cd ("Install the swtpm package, if available") in addition to
> > your Signed-off-by tag.
> > 
> 
> The current patch adds one line.  This proposal adds one line and
> moves another line.  It also changes the swtpm flow, which must
> be tested.
> 
> It's OK with me.  Let me know.
> 
> Where should I add that "Fixes ..." text?  What is the exact format?

The "Fixes" tag belongs in the patch description above your Signed-off-
by tag.  The format is:  Fixes: < commit number> < commit ttitle>

e.g. Fixes: f831508297cd ("Install the swtpm package, if available")

As this is a bug fix, please update the Subject line and post this
change independently of the other changes.  I've already tested the
suggested change.  Once the updated patch is posted, it will hopefully
be tested by the distros as well.

thanks,

Mimi

Re: [PATCH v2 3/5] ima-evm-utils: Change tpm2_pcr_read() to use C code

[Mimi Zohar]

Hi Ken,

On Mon, 2020-10-12 at 19:44 -0400, Ken Goldman wrote:
> Replace the call out to the command line tools with C functions.
> 
> The algorithm_string_to_algid() function supports only the digest
> algorithms in use.  The table has place holders for other agorithms as
> they are needed and the C strings are defined.
> 
> The table can also be used for an algrithm ID to string function if
> it's ever needed.
> 
> When using the IBM TSS, link in its library.
> 
> Signed-off-by: Ken Goldman <kgoldman@us.ibm.com>

The code seems to be working properly, but needs to be cleaned up. 
There are simple formatting changes and some style changes.

Simple formatting changes:
- Tabs should be 8 characters.
- Remove blank spaces before code.
- 80 characater maximum line length.
- Leave a blank line between variable definitions and code.
- Long comments should start with "/*" and end with "*/" on separate
lines:
/*
 * multiple line comments
 * continued
 */
- When defining function variables, please use the format:
term:
<definition>


Style changes:
- There are valid reasons for having a common function exit (e.g.
freeing memory), otherwise the function should exit early.  The "if (rc
== 0)" tests cause the code to unnecessarily be indented.  Instead the
test could be inverted "if (!rc)" followed by a "goto out".

- Please do not use camel or Hungarian variable naming conventions.  
The variable definition can reference the spec name.

thanks,

Mimi

> ---
>  src/Makefile.am      |   1 +
>  src/pcr_tsspcrread.c | 156 +++++++++++++++++++++++++++++++++----------
>  2 files changed, 123 insertions(+), 34 deletions(-)
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index d6c779f..bf18caf 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -26,6 +26,7 @@ if USE_PCRTSS
>  evmctl_SOURCES += pcr_tss.c
>  else
>  evmctl_SOURCES += pcr_tsspcrread.c
> +evmctl_LDADD += -libmtss
>  endif
>  
>  AM_CPPFLAGS = -I$(top_srcdir) -include config.h
> diff --git a/src/pcr_tsspcrread.c b/src/pcr_tsspcrread.c
> index 118c7d2..eae68b7 100644
> --- a/src/pcr_tsspcrread.c
> +++ b/src/pcr_tsspcrread.c
> @@ -50,6 +50,10 @@
>  #include "utils.h"
>  #include "imaevm.h"
>  
> +#define TPM_POSIX	/* use Posix, not Windows constructs in TSS */
> +#undef MAX_DIGEST_SIZE	/* imaevm uses a different value than the TSS */
> +#include <ibmtss/tss.h>
> +
>  #define CMD "tsspcrread"
>  
>  static char path[PATH_MAX];
> @@ -68,44 +72,128 @@ int tpm2_pcr_supported(void)
>  	return 1;
>  }
>  
> -int tpm2_pcr_read(const char *algo_name, uint32_t pcrHandle, uint8_t *hwpcr,
> -		 int len, char **errmsg)
> -{
> -	FILE *fp;
> -	char pcr[100];	/* may contain an error */
> -	char cmd[PATH_MAX + 50];
> -	int ret;
> -
> -	sprintf(cmd, "%s -halg %s -ha %d -ns 2> /dev/null",
> -		path, algo_name, pcrHandle);
> -	fp = popen(cmd, "r");
> -	if (!fp) {
> -		ret = asprintf(errmsg, "popen failed: %s", strerror(errno));
> -		if (ret == -1)	/* the contents of errmsg is undefined */
> -			*errmsg = NULL;
> -		return -1;
> -	}
> +/* Table mapping C strings to TCG algorithm identifiers */
> +
> +typedef struct tdAlgorithm_Map {
> +    const char *algorithm_string;
> +    TPMI_ALG_HASH algid;
> +} Algorithm_Map;
>  
> -	if (fgets(pcr, sizeof(pcr), fp) == NULL) {
> -		ret = asprintf(errmsg, "tsspcrread failed: %s",
> -			       strerror(errno));
> -		if (ret == -1)	/* the contents of errmsg is undefined */
> -			*errmsg = NULL;
> -		ret = pclose(fp);
> -		return -1;
> +Algorithm_Map algorithm_map[] = {
> +				 { "sha1", TPM_ALG_SHA1},
> +				 { "sha256", TPM_ALG_SHA256},
> +#if 0	/* uncomment as these digest algorithms are supported */
> +				 { "", TPM_ALG_SHA384},
> +				 { "", TPM_ALG_SHA512},
> +				 { "", TPM_ALG_SM3_256},
> +				 { "", TPM_ALG_SHA3_256},
> +				 { "", TPM_ALG_SHA3_384},
> +				 { "", TPM_ALG_SHA3_512},
> +#endif
> +};
> +
> +/* algorithm_string_to_algid() converts a digest algorithm from a C string to a TCG algorithm
> +   identifier as defined in the TCG Algorithm Regisrty..
> +
> +   Returns TPM_ALG_ERROR if the string has an unsupported value.
> +*/
> +
> +static TPMI_ALG_HASH algorithm_string_to_algid(const char *algorithm_string)
> +{
> +    size_t 	i;
> +    for (i=0 ; i < sizeof(algorithm_map)/sizeof(Algorithm_Map) ; i++) {
> +	if (strcmp(algorithm_string, algorithm_map[i].algorithm_string) == 0) {
> +	    return algorithm_map[i].algid; /* if match */
>  	}
> +    }
> +    return TPM_ALG_ERROR;
> +}
>  
> -	/* get the popen "cmd" return code */
> -	ret = pclose(fp);
> +/* tpm2_pcr_read() reads the PCR
>  
> -	/* Treat an unallocated bank as an error */
> -	if (!ret && (strlen(pcr) < SHA_DIGEST_LENGTH))
> -		ret = -1;
> +   algo_name is the PCR digest algorithm (the PCR bank) as a C string
> +   pcrHandle is the PCR number to read
> +   hwpcr is a buffer for the PCR output in binary
> +   len is the allocated size of hwpcr and should match the digest algorithm
> +*/
>  
> -	if (!ret)
> -		hex2bin(hwpcr, pcr, len);
> -	else
> -		*errmsg = strndup(pcr, strlen(pcr) - 1); /* remove newline */
> +int tpm2_pcr_read(const char *algo_name, uint32_t pcrHandle, uint8_t *hwpcr,
> +		 int len, char **errmsg)
> +{
> +        int 			ret = 0;	/* function return code */
> +	TPM_RC			rc = 0;		/* TCG return code */
> +	PCR_Read_In 		pcrReadIn;	/* command input */
> +	PCR_Read_Out 		pcrReadOut;	/* response output */
> +	TSS_CONTEXT		*tssContext = NULL;
> +	TPMI_ALG_HASH 		alg_id;		/* PCR algorithm */
>  
> -	return ret;
> +	if (rc == 0) {		/* map algorithm string to TCG value */
> +	    alg_id = algorithm_string_to_algid(algo_name);
> +	    if (alg_id == TPM_ALG_ERROR) {
> +		ret = asprintf(errmsg, "tpm2_pcr_read: unknown algorithm %s", algo_name);
> +		if (ret == -1) {	/* the contents of errmsg is undefined */
> +		    *errmsg = NULL;
> +		}
> +		rc = 1;
> +	    }
> +	}
> +	if (rc == 0) {
> +	    rc = TSS_Create(&tssContext);
> +	}
> +	/* call TSS to execute the command */
> +	if (rc == 0) {
> +	    pcrReadIn.pcrSelectionIn.count = 1;
> +	    pcrReadIn.pcrSelectionIn.pcrSelections[0].hash = alg_id;
> +	    pcrReadIn.pcrSelectionIn.pcrSelections[0].sizeofSelect = 3;
> +	    pcrReadIn.pcrSelectionIn.pcrSelections[0].pcrSelect[0] = 0;
> +	    pcrReadIn.pcrSelectionIn.pcrSelections[0].pcrSelect[1] = 0;
> +	    pcrReadIn.pcrSelectionIn.pcrSelections[0].pcrSelect[2] = 0;
> +	    pcrReadIn.pcrSelectionIn.pcrSelections[0].pcrSelect[pcrHandle / 8] =
> +		1 << (pcrHandle % 8);
> +	    rc = TSS_Execute(tssContext,
> +			     (RESPONSE_PARAMETERS *)&pcrReadOut,
> +			     (COMMAND_PARAMETERS *)&pcrReadIn,
> +			     NULL,
> +			     TPM_CC_PCR_Read,
> +			     TPM_RH_NULL, NULL, 0);
> +	}
> +	if (rc == 0) {
> +	    /* nothing read, bank missing */
> +	    if (pcrReadOut.pcrValues.count == 0) {
> +		ret = asprintf(errmsg, "tpm2_pcr_read: returned count 0 for %s", algo_name);
> +		if (ret == -1) {	/* the contents of errmsg is undefined */
> +		    *errmsg = NULL;
> +		}
> +		rc = 1;
> +	    }
> +	    /* len parameter did not match the digest algorithm */
> +	    else if (pcrReadOut.pcrValues.digests[0].t.size != len) {
> +		ret = asprintf(errmsg,
> +			       "tpm2_pcr_read: expected length %d actual %u for %s",
> +			       len, pcrReadOut.pcrValues.digests[0].t.size, algo_name);
> +		if (ret == -1) {	/* the contents of errmsg is undefined */
> +		    *errmsg = NULL;
> +		}
> +		rc = 1;
> +	    }
> +	    else {
> +		memcpy(hwpcr,
> +		       pcrReadOut.pcrValues.digests[0].t.buffer,
> +		       pcrReadOut.pcrValues.digests[0].t.size);
> +	    }
> +	}
> +	{
> +	    TPM_RC rc1 = TSS_Delete(tssContext);
> +	    if (rc == 0) {
> +		rc = rc1;
> +	    }
> +	}
> +	/* map TCG return code to function return code */
> +	if (rc == 0) {
> +	    return 0;
> +	}
> +	else {
> +	    return -1;
> +	}
>  }
> +