[PATCH ima-evm-utils 0/4] misc bug and other fixes

[PATCH ima-evm-utils 0/4] misc bug and other fixes

[Mimi Zohar]

Fix coverity reported issues and the tpm2_pcr_supported() informational
message.

Mimi Zohar (4):
  Don't ignore number of items read
  Define and verify the template data length upper bounds
  Sanity check the template data field sizes
  Fix tpm2_pcr_supported() output messages

 src/evmctl.c         | 57 +++++++++++++++++++++++++++++++++++++-------
 src/imaevm.h         | 10 ++++++++
 src/pcr_ibmtss.c     | 12 +---------
 src/pcr_tsspcrread.c |  4 ++--
 4 files changed, 62 insertions(+), 21 deletions(-)

-- 
2.31.1

[PATCH ima-evm-utils 1/4] Don't ignore number of items read

[Mimi Zohar]

fread() either returns the number of bytes read or the number of items
of data read.  Check that it returns the requested number of items read.

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

diff --git a/src/evmctl.c b/src/evmctl.c
index 2e21da67c444..bcf724c828f7 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -2161,7 +2161,7 @@ static int ima_measurement(const char *file)
 		}
 
 		memset(entry.name, 0x00, sizeof(entry.name));
-		if (!fread(entry.name, entry.header.name_len, 1, fp)) {
+		if (fread(entry.name, entry.header.name_len, 1, fp) != 1) {
 			log_err("Unable to read template name\n");
 			goto out;
 		}
@@ -2184,8 +2184,8 @@ static int ima_measurement(const char *file)
 
 		/* The "ima" template data is not length prefixed.  Skip it. */
 		if (!is_ima_template) {
-			if (!fread(&entry.template_len,
-				   sizeof(entry.template_len), 1, fp)) {
+			if (fread(&entry.template_len,
+				  sizeof(entry.template_len), 1, fp) != 1) {
 				log_err("Unable to read template length\n");
 				goto out;
 			}
@@ -2205,7 +2205,8 @@ static int ima_measurement(const char *file)
 		}
 
 		if (!is_ima_template) {
-			if (!fread(entry.template, entry.template_len, 1, fp)) {
+			if (fread(entry.template, entry.template_len,
+				  1, fp) != 1) {
 				log_errno("Unable to read template\n");
 				goto out;
 			}
@@ -2217,7 +2218,8 @@ static int ima_measurement(const char *file)
 			 * The "ima" template data format is digest,
 			 * filename length, filename.
 			 */
-			if (!fread(entry.template, SHA_DIGEST_LENGTH, 1, fp)) {
+			if (fread(entry.template, SHA_DIGEST_LENGTH,
+				  1, fp) != 1) {
 				log_errno("Unable to read file data hash\n");
 				goto out;
 			}
-- 
2.31.1

[PATCH ima-evm-utils 2/4] Define and verify the template data length upper bounds

[Mimi Zohar]

The template data length is variable, based on the template format.
Define some sort of upper bounds.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c |  3 ++-
 src/imaevm.h | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index bcf724c828f7..9ab804fee37a 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -2189,7 +2189,8 @@ static int ima_measurement(const char *file)
 				log_err("Unable to read template length\n");
 				goto out;
 			}
-			if (entry.template_len == 0) {
+			if (entry.template_len == 0 ||
+			    entry.template_len > MAX_TEMPLATE_SIZE) {
 				log_err("Invalid template data len\n");
 				goto out;
 			}
diff --git a/src/imaevm.h b/src/imaevm.h
index 8114bd051514..c43312d01dec 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -91,6 +91,16 @@
 #define MAX_DIGEST_SIZE		64
 #define MAX_SIGNATURE_SIZE	1024
 
+/*
+ * The maximum template data size is dependent on the template format. For
+ * example the 'ima-modsig' template includes two signatures - one for the
+ * entire file, the other without the appended signature - and other fields
+ * (e.g. file digest, file name, file digest without the appended signature).
+ *
+ * Other template formats are much smaller.
+ */
+#define MAX_TEMPLATE_SIZE	(MAX_SIGNATURE_SIZE * 4)
+
 #define __packed __attribute__((packed))
 
 enum evm_ima_xattr_type {
-- 
2.31.1

[PATCH ima-evm-utils 3/4] Sanity check the template data field sizes

[Mimi Zohar]

The field sizes of the original "ima" template data are static, but
the other template data fields are not.  They're prefixed with a size.

Add some data field size sanity checks in ima_show_ng().

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

diff --git a/src/evmctl.c b/src/evmctl.c
index 9ab804fee37a..4a071143679e 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1591,8 +1591,9 @@ void ima_ng_show(struct template_entry *entry)
 {
 	uint8_t *fieldp = entry->template;
 	uint32_t field_len;
-	int total_len = entry->template_len, digest_len, len, sig_len, fbuf_len;
+	int total_len = entry->template_len, digest_len, len, fbuf_len;
 	uint8_t *digest, *sig = NULL, *fbuf = NULL;
+	int sig_len = 0;
 	char *algo, *path;
 	int found;
 	int err;
@@ -1601,33 +1602,65 @@ void ima_ng_show(struct template_entry *entry)
 	field_len = *(uint32_t *)fieldp;
 	fieldp += sizeof(field_len);
 	total_len -= sizeof(field_len);
+	if (total_len < 0) {
+		log_err("Template \"%s\" invalid template data\n", entry->name);
+		return;
+	}
 
 	algo = (char *)fieldp;
 	len = strnlen(algo, field_len - 1) + 1;
 	digest_len = field_len - len;
+	if (digest_len < SHA_DIGEST_LENGTH ||
+	    digest_len > MAX_DIGEST_SIZE) {
+		log_err("Template \"%s\" invalid digest length\n", entry->name);
+		return;
+	}
 	digest = fieldp + len;
 
 	/* move to next field */
 	fieldp += field_len;
 	total_len -= field_len;
+	if (total_len < 0) {
+		log_err("Template \"%s\" invalid template data\n", entry->name);
+		return;
+	}
 
 	/* get path */
 	field_len = *(uint32_t *)fieldp;
 	fieldp += sizeof(field_len);
 	total_len -= sizeof(field_len);
+	if (field_len == 0 || field_len > PATH_MAX || total_len < field_len) {
+		log_err("Template \"%s\" invalid file pathname\n", entry->name);
+		return;
+	}
 
 	path = (char *)fieldp;
 
 	/* move to next field */
 	fieldp += field_len;
 	total_len -= field_len;
+	if (total_len < 0) {
+		log_err("Template \"%s\" invalid template data\n", entry->name);
+		return;
+	}
 
 	if (!strcmp(entry->name, "ima-sig") ||
 	    !strcmp(entry->name, "ima-sigv2")) {
-		/* get signature */
+		/* get signature, if it exists */
 		field_len = *(uint32_t *)fieldp;
 		fieldp += sizeof(field_len);
+		if (field_len > MAX_SIGNATURE_SIZE) {
+			log_err("Template \"%s\" invalid file signature size\n",
+				entry->name);
+			return;
+		}
+
 		total_len -= sizeof(field_len);
+		if (total_len < 0) {
+			log_err("Template \"%s\" invalid template data\n",
+				entry->name);
+			return;
+		}
 
 		if (field_len) {
 			sig = fieldp;
@@ -1651,6 +1684,11 @@ void ima_ng_show(struct template_entry *entry)
 		}
 	}
 
+	if (total_len < 0) {
+		log_err("Template \"%s\" invalid template data\n", entry->name);
+		return;
+	}
+
 	/* ascii_runtime_measurements */
 	if (imaevm_params.verbose > LOG_INFO) {
 		log_info("%d ", entry->header.pcr);
-- 
2.31.1

[PATCH ima-evm-utils 4/4] Fix tpm2_pcr_supported() output messages

[Mimi Zohar]

Remove unnecessary path check in pcr_ibmtss.c and update the syntax
in the other.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/pcr_ibmtss.c     | 12 +-----------
 src/pcr_tsspcrread.c |  4 ++--
 2 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/src/pcr_ibmtss.c b/src/pcr_ibmtss.c
index b8700ddd5da8..6b38d280e80c 100644
--- a/src/pcr_ibmtss.c
+++ b/src/pcr_ibmtss.c
@@ -20,21 +20,11 @@
 #undef MAX_DIGEST_SIZE	/* imaevm uses a different value than the TSS */
 #include <ibmtss/tss.h>
 
-#define CMD "tsspcrread"
-
-static char path[PATH_MAX];
-
 int tpm2_pcr_supported(void)
 {
 	if (imaevm_params.verbose > LOG_INFO)
-		log_info("Using %s to read PCRs.\n", CMD);
-
-	if (get_cmd_path(CMD, path, sizeof(path))) {
-		log_debug("Couldn't find '%s' in $PATH\n", CMD);
-		return 0;
-	}
+		log_info("Using ibmtss to read PCRs.\n");
 
-	log_debug("Found '%s' in $PATH\n", CMD);
 	return 1;
 }
 
diff --git a/src/pcr_tsspcrread.c b/src/pcr_tsspcrread.c
index 95048f8a5469..39ff8f7fc14d 100644
--- a/src/pcr_tsspcrread.c
+++ b/src/pcr_tsspcrread.c
@@ -60,11 +60,11 @@ int tpm2_pcr_supported(void)
 		log_info("Using %s to read PCRs.\n", CMD);
 
 	if (get_cmd_path(CMD, path, sizeof(path))) {
-		log_debug("Couldn't find '%s' in $PATH\n", CMD);
+		log_info("Couldn't find '%s' in %s\n", CMD, path);
 		return 0;
 	}
 
-	log_debug("Found '%s' in $PATH\n", CMD);
+	log_debug("Found '%s' in %s\n", CMD, path);
 	return 1;
 }
 
-- 
2.31.1

Re: [PATCH ima-evm-utils 4/4] Fix tpm2_pcr_supported() output messages

[Stefan Berger]

On 9/14/22 10:22, Mimi Zohar wrote:
> Remove unnecessary path check in pcr_ibmtss.c and update the syntax
> in the other.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/pcr_ibmtss.c     | 12 +-----------
>   src/pcr_tsspcrread.c |  4 ++--
>   2 files changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/src/pcr_ibmtss.c b/src/pcr_ibmtss.c
> index b8700ddd5da8..6b38d280e80c 100644
> --- a/src/pcr_ibmtss.c
> +++ b/src/pcr_ibmtss.c
> @@ -20,21 +20,11 @@
>   #undef MAX_DIGEST_SIZE	/* imaevm uses a different value than the TSS */
>   #include <ibmtss/tss.h>
>   
> -#define CMD "tsspcrread"
> -
> -static char path[PATH_MAX];
> -
>   int tpm2_pcr_supported(void)
>   {
>   	if (imaevm_params.verbose > LOG_INFO)
> -		log_info("Using %s to read PCRs.\n", CMD);
> -
> -	if (get_cmd_path(CMD, path, sizeof(path))) {
> -		log_debug("Couldn't find '%s' in $PATH\n", CMD);
> -		return 0;
> -	}
> +		log_info("Using ibmtss to read PCRs.\n");
>   
> -	log_debug("Found '%s' in $PATH\n", CMD);
>   	return 1;
>   }
>   
> diff --git a/src/pcr_tsspcrread.c b/src/pcr_tsspcrread.c
> index 95048f8a5469..39ff8f7fc14d 100644
> --- a/src/pcr_tsspcrread.c
> +++ b/src/pcr_tsspcrread.c
> @@ -60,11 +60,11 @@ int tpm2_pcr_supported(void)
>   		log_info("Using %s to read PCRs.\n", CMD);
>   
>   	if (get_cmd_path(CMD, path, sizeof(path))) {
> -		log_debug("Couldn't find '%s' in $PATH\n", CMD);
> +		log_info("Couldn't find '%s' in %s\n", CMD, path);
>   		return 0;
>   	}
>   
> -	log_debug("Found '%s' in $PATH\n", CMD);
> +	log_debug("Found '%s' in %s\n", CMD, path);
>   	return 1;
>   }
>   

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

Re: [PATCH ima-evm-utils 3/4] Sanity check the template data field sizes

[Stefan Berger]

On 9/14/22 10:22, Mimi Zohar wrote:
> The field sizes of the original "ima" template data are static, but
> the other template data fields are not.  They're prefixed with a size.
> 
> Add some data field size sanity checks in ima_show_ng().
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/evmctl.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 9ab804fee37a..4a071143679e 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -1591,8 +1591,9 @@ void ima_ng_show(struct template_entry *entry)
>   {
>   	uint8_t *fieldp = entry->template;
>   	uint32_t field_len;
> -	int total_len = entry->template_len, digest_len, len, sig_len, fbuf_len;
> +	int total_len = entry->template_len, digest_len, len, fbuf_len;
>   	uint8_t *digest, *sig = NULL, *fbuf = NULL;
> +	int sig_len = 0;
>   	char *algo, *path;
>   	int found;
>   	int err;
> @@ -1601,33 +1602,65 @@ void ima_ng_show(struct template_entry *entry)
>   	field_len = *(uint32_t *)fieldp;
>   	fieldp += sizeof(field_len);
>   	total_len -= sizeof(field_len);
> +	if (total_len < 0) {
> +		log_err("Template \"%s\" invalid template data\n", entry->name);
> +		return;
> +	}
>   
>   	algo = (char *)fieldp;
>   	len = strnlen(algo, field_len - 1) + 1;
>   	digest_len = field_len - len;
> +	if (digest_len < SHA_DIGEST_LENGTH ||
> +	    digest_len > MAX_DIGEST_SIZE) {
> +		log_err("Template \"%s\" invalid digest length\n", entry->name);
> +		return;
> +	}
>   	digest = fieldp + len;
>   
>   	/* move to next field */
>   	fieldp += field_len;
>   	total_len -= field_len;
> +	if (total_len < 0) {
> +		log_err("Template \"%s\" invalid template data\n", entry->name);
> +		return;
> +	}
>   
>   	/* get path */
>   	field_len = *(uint32_t *)fieldp;
>   	fieldp += sizeof(field_len);
>   	total_len -= sizeof(field_len);
> +	if (field_len == 0 || field_len > PATH_MAX || total_len < field_len) {
> +		log_err("Template \"%s\" invalid file pathname\n", entry->name);
> +		return;
> +	}
>   
>   	path = (char *)fieldp;
>   
>   	/* move to next field */
>   	fieldp += field_len;
>   	total_len -= field_len;
> +	if (total_len < 0) {
> +		log_err("Template \"%s\" invalid template data\n", entry->name);
> +		return;
> +	}
>   
>   	if (!strcmp(entry->name, "ima-sig") ||
>   	    !strcmp(entry->name, "ima-sigv2")) {
> -		/* get signature */
> +		/* get signature, if it exists */
>   		field_len = *(uint32_t *)fieldp;
>   		fieldp += sizeof(field_len);
> +		if (field_len > MAX_SIGNATURE_SIZE) {
> +			log_err("Template \"%s\" invalid file signature size\n",
> +				entry->name);
> +			return;
> +		}
> +
>   		total_len -= sizeof(field_len);
> +		if (total_len < 0) {
> +			log_err("Template \"%s\" invalid template data\n",
> +				entry->name);
> +			return;
> +		}
>   
>   		if (field_len) {
>   			sig = fieldp;
> @@ -1651,6 +1684,11 @@ void ima_ng_show(struct template_entry *entry)
>   		}
>   	}
>   
> +	if (total_len < 0) {
> +		log_err("Template \"%s\" invalid template data\n", entry->name);
> +		return;
> +	}
> +
>   	/* ascii_runtime_measurements */
>   	if (imaevm_params.verbose > LOG_INFO) {
>   		log_info("%d ", entry->header.pcr);

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

Re: [PATCH ima-evm-utils 2/4] Define and verify the template data length upper bounds

[Stefan Berger]

On 9/14/22 10:22, Mimi Zohar wrote:
> The template data length is variable, based on the template format.
> Define some sort of upper bounds.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/evmctl.c |  3 ++-
>   src/imaevm.h | 10 ++++++++++
>   2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index bcf724c828f7..9ab804fee37a 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -2189,7 +2189,8 @@ static int ima_measurement(const char *file)
>   				log_err("Unable to read template length\n");
>   				goto out;
>   			}
> -			if (entry.template_len == 0) {
> +			if (entry.template_len == 0 ||
> +			    entry.template_len > MAX_TEMPLATE_SIZE) {
>   				log_err("Invalid template data len\n");
>   				goto out;
>   			}
> diff --git a/src/imaevm.h b/src/imaevm.h
> index 8114bd051514..c43312d01dec 100644
> --- a/src/imaevm.h
> +++ b/src/imaevm.h
> @@ -91,6 +91,16 @@
>   #define MAX_DIGEST_SIZE		64
>   #define MAX_SIGNATURE_SIZE	1024
>   
> +/*
> + * The maximum template data size is dependent on the template format. For
> + * example the 'ima-modsig' template includes two signatures - one for the
> + * entire file, the other without the appended signature - and other fields
> + * (e.g. file digest, file name, file digest without the appended signature).
> + *
> + * Other template formats are much smaller.
> + */
> +#define MAX_TEMPLATE_SIZE	(MAX_SIGNATURE_SIZE * 4)

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

> +
>   #define __packed __attribute__((packed))
>   
>   enum evm_ima_xattr_type {

Re: [PATCH ima-evm-utils 1/4] Don't ignore number of items read

[Stefan Berger]

On 9/14/22 10:22, Mimi Zohar wrote:
> fread() either returns the number of bytes read or the number of items
> of data read.  Check that it returns the requested number of items read.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/evmctl.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 2e21da67c444..bcf724c828f7 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -2161,7 +2161,7 @@ static int ima_measurement(const char *file)
>   		}
>   
>   		memset(entry.name, 0x00, sizeof(entry.name));
> -		if (!fread(entry.name, entry.header.name_len, 1, fp)) {
> +		if (fread(entry.name, entry.header.name_len, 1, fp) != 1) {
>   			log_err("Unable to read template name\n");
>   			goto out;
>   		}
> @@ -2184,8 +2184,8 @@ static int ima_measurement(const char *file)
>   
>   		/* The "ima" template data is not length prefixed.  Skip it. */
>   		if (!is_ima_template) {
> -			if (!fread(&entry.template_len,
> -				   sizeof(entry.template_len), 1, fp)) {
> +			if (fread(&entry.template_len,
> +				  sizeof(entry.template_len), 1, fp) != 1) {
>   				log_err("Unable to read template length\n");
>   				goto out;
>   			}
> @@ -2205,7 +2205,8 @@ static int ima_measurement(const char *file)
>   		}
>   
>   		if (!is_ima_template) {
> -			if (!fread(entry.template, entry.template_len, 1, fp)) {
> +			if (fread(entry.template, entry.template_len,
> +				  1, fp) != 1) {
>   				log_errno("Unable to read template\n");
>   				goto out;
>   			}
> @@ -2217,7 +2218,8 @@ static int ima_measurement(const char *file)
>   			 * The "ima" template data format is digest,
>   			 * filename length, filename.
>   			 */
> -			if (!fread(entry.template, SHA_DIGEST_LENGTH, 1, fp)) {
> +			if (fread(entry.template, SHA_DIGEST_LENGTH,
> +				  1, fp) != 1) {
>   				log_errno("Unable to read file data hash\n");
>   				goto out;
>   			}

It was correct before as well ...

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

Re: [PATCH ima-evm-utils 1/4] Don't ignore number of items read

[Mimi Zohar]

Hi Stefan,

Thank you for this and the other reviews.

On Wed, 2022-09-14 at 17:30 -0400, Stefan Berger wrote:
> 
> On 9/14/22 10:22, Mimi Zohar wrote:
> > fread() either returns the number of bytes read or the number of items
> > of data read.  Check that it returns the requested number of items read.
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>


> 
> It was correct before as well ...
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Yeah, I'll update the patch description explaining that this stops the
static analysis complaints

-- 
thanks,

Mimi