ima-evm-utils: Some cleanups and bugfixes

PATCH In review Stefan Berger 2 participants 9 messages Apr 19, 2021 – Jun 25, 2021 Keys-trusted

lore

Share & tools

lore

Stefan Berger

author

2021-04-19 15:01 UTC #0 lore raw

cover ima-evm-utils: Some cleanups and bugfixes

This PR contains some cleanups and bugfixes for libimaevm of ima-evm-utils.

Stefan

Stefan Berger (6):
libimaevm: Properly check for error returned by EVP_DigestUpdate
libimaevm: Remove unused off variable
libimaevm: Rename variable returned from readlink to len
libimaevm: Rename variable from cr to newline
libimaevm: Report unsupported filetype using log_err
libimaevm: Use function parameter algo for name of hash

 src/libimaevm.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

Signature · 1 line

2.30.2

Stefan Berger

author

2021-04-19 15:02 UTC #1 lore raw

PATCH 1/6 libimaevm: Properly check for error returned by EVP_DigestUpdate

The error checking in add_dir_hash was wrong. EVP_DigestUpdate returns 1 on success and 0 on error, so we cannot just accumulate it using or'ing.

From the man page:
EVP_DigestInit_ex(), EVP_DigestUpdate(), EVP_DigestFinal_ex()
Returns 1 for success and 0 for failure.

Signed-off-by Stefan Berger <[email protected]>

---
 src/libimaevm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

src/libimaevm.c +4 -6

index fa6c278..d8fc0d4 100644
@@ -179,7 +179,6 @@ out:
 
 static int add_dir_hash(const char *file, EVP_MD_CTX *ctx)
 {
-	int err;
 	struct dirent *de;
 	DIR *dir;
 	unsigned long long ino, off;

@@ -198,11 +197,10 @@ static int add_dir_hash(const char *file, EVP_MD_CTX *ctx)
 		type = de->d_type;
 		log_debug("entry: %s, ino: %llu, type: %u, off: %llu, reclen: %hu\n",
 			  de->d_name, ino, type, off, de->d_reclen);
-		err = EVP_DigestUpdate(ctx, de->d_name, strlen(de->d_name));
-		/*err |= EVP_DigestUpdate(ctx, &off, sizeof(off));*/
-		err |= EVP_DigestUpdate(ctx, &ino, sizeof(ino));
-		err |= EVP_DigestUpdate(ctx, &type, sizeof(type));
-		if (!err) {
+		if (EVP_DigestUpdate(ctx, de->d_name, strlen(de->d_name)) != 1 ||
+		    /* EVP_DigestUpdate(ctx, &off, sizeof(off)) != 1 || */
+		    EVP_DigestUpdate(ctx, &ino, sizeof(ino)) != 1||
+		    EVP_DigestUpdate(ctx, &type, sizeof(type)) != 1) {
 			log_err("EVP_DigestUpdate() failed\n");
 			output_openssl_errors();
 			result = 1;

Signature · 1 line

2.30.2

Stefan Berger

author

2021-04-19 15:02 UTC #2 lore raw

PATCH 2/6 libimaevm: Remove unused off variable

In reply to Stefan Berger

The 'off' variable was unused in add_dir_hash(), so remove it.

Signed-off-by Stefan Berger <[email protected]>

---
 src/libimaevm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

src/libimaevm.c +3 -5

index d8fc0d4..0137884 100644
@@ -181,7 +181,7 @@ static int add_dir_hash(const char *file, EVP_MD_CTX *ctx)
 {
 	struct dirent *de;
 	DIR *dir;
-	unsigned long long ino, off;
+	unsigned long long ino;
 	unsigned int type;
 	int result = 0;

@@ -193,12 +193,10 @@ static int add_dir_hash(const char *file, EVP_MD_CTX *ctx)
 
 	while ((de = readdir(dir))) {
 		ino = de->d_ino;
-		off = de->d_off;
 		type = de->d_type;
-		log_debug("entry: %s, ino: %llu, type: %u, off: %llu, reclen: %hu\n",
-			  de->d_name, ino, type, off, de->d_reclen);
+		log_debug("entry: %s, ino: %llu, type: %u, reclen: %hu\n",
+			  de->d_name, ino, type, de->d_reclen);
 		if (EVP_DigestUpdate(ctx, de->d_name, strlen(de->d_name)) != 1 ||
-		    /* EVP_DigestUpdate(ctx, &off, sizeof(off)) != 1 || */
 		    EVP_DigestUpdate(ctx, &ino, sizeof(ino)) != 1||
 		    EVP_DigestUpdate(ctx, &type, sizeof(type)) != 1) {
 			log_err("EVP_DigestUpdate() failed\n");

Signature · 1 line

2.30.2

Stefan Berger

author

2021-04-19 15:02 UTC #3 lore raw

PATCH 3/6 libimaevm: Rename variable returned from readlink to len

In reply to Stefan Berger

The variable returned from readlink is a length indicator of the number of bytes placed into a buffer, not only an error. Leave a note in the code that a zero-length link is also treated as an error, besdies the usual -1.

Signed-off-by Stefan Berger <[email protected]>

---
 src/libimaevm.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

src/libimaevm.c +6 -5

index 0137884..9a6739b 100644
@@ -213,15 +213,16 @@ static int add_dir_hash(const char *file, EVP_MD_CTX *ctx)
 
 static int add_link_hash(const char *path, EVP_MD_CTX *ctx)
 {
-	int err;
+	int len;
 	char buf[1024];
 
-	err = readlink(path, buf, sizeof(buf));
-	if (err <= 0)
+	len = readlink(path, buf, sizeof(buf));
+	/* 0-length links are also an error */
+	if (len <= 0)
 		return -1;
 
-	log_info("link: %s -> %.*s\n", path, err, buf);
-	return !EVP_DigestUpdate(ctx, buf, err);
+	log_info("link: %s -> %.*s\n", path, len, buf);
+	return !EVP_DigestUpdate(ctx, buf, len);
 }
 
 static int add_dev_hash(struct stat *st, EVP_MD_CTX *ctx)

Signature · 1 line

2.30.2

Stefan Berger

author

2021-04-19 15:26 UTC #4 lore raw

Re: [PATCH 3/6] libimaevm: Rename variable returned from readlink to len

Quoted 4 lines · Stefan Berger

The variable returned from readlink is a length indicator of the number of bytes placed into a buffer, not only an error. Leave a note in the code that a zero-length link is also treated as an error, besdies the usual -1.

Is link signing supported by IMA in the kernel? Maybe I missed something when looking at the code in the Linux kernel, but I could not find that it was supported. What about directory signing and socket/device file signing?

Stefan

Stefan Berger

author

2021-04-19 15:02 UTC #5 lore raw

PATCH 4/6 libimaevm: Rename variable from cr to newline

In reply to Stefan Berger

Rename function variable from cr (carriage return, '\r') to newline, because this is what it is.

Signed-off-by Stefan Berger <[email protected]>

---
 src/libimaevm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

src/libimaevm.c +2 -2

index 9a6739b..b40b6d8 100644
@@ -90,14 +90,14 @@ struct libimaevm_params imaevm_params = {
 
 static void __attribute__ ((constructor)) libinit(void);
 
-void imaevm_do_hexdump(FILE *fp, const void *ptr, int len, bool cr)
+void imaevm_do_hexdump(FILE *fp, const void *ptr, int len, bool newline)
 {
 	int i;
 	uint8_t *data = (uint8_t *) ptr;
 
 	for (i = 0; i < len; i++)
 		fprintf(fp, "%02x", data[i]);
-	if (cr)
+	if (newline)
 		fprintf(fp, "\n");
 }

Signature · 1 line

2.30.2

Stefan Berger

author

2021-04-19 15:02 UTC #6 lore raw

PATCH 5/6 libimaevm: Report unsupported filetype using log_err

In reply to Stefan Berger

There's no errno set at this point so that using log_errno would display something useful. Instead use log_error().

Signed-off-by Stefan Berger <[email protected]>

---
 src/libimaevm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

src/libimaevm.c +1 -1

index b40b6d8..06f1063 100644
@@ -286,7 +286,7 @@ int ima_calc_hash(const char *file, uint8_t *hash)
 		err = add_dev_hash(&st, pctx);
 		break;
 	default:
-		log_errno("Unsupported file type");
+		log_err("Unsupported file type (0x%x)", st.st_mode & S_IFMT);
 		err = -1;
 		goto err;
 	}

Signature · 1 line

2.30.2

Stefan Berger

author

2021-04-19 15:02 UTC #7 lore raw

PATCH 6/6 libimaevm: Use function parameter algo for name of hash

In reply to Stefan Berger

Instead of using the global variable imaevm_params.hash_algo as the hash algo to use, use the algo parameter passed into the function. Existing code in this function already uses 'algo' for writing the has into the header:

        hdr->hash_algo = imaevm_get_hash_algo(algo);

Signed-off-by Stefan Berger <[email protected]>

---
 src/libimaevm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

src/libimaevm.c +2 -2

index 06f1063..2856270 100644
@@ -913,7 +913,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
 		return -1;
 	}
 
-	log_info("hash(%s): ", imaevm_params.hash_algo);
+	log_info("hash(%s): ", algo);
 	log_dump(hash, size);
 
 	pkey = read_priv_pkey(keyfile, imaevm_params.keypass);

@@ -939,7 +939,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
 	if (!EVP_PKEY_sign_init(ctx))
 		goto err;
 	st = "EVP_get_digestbyname";
-	if (!(md = EVP_get_digestbyname(imaevm_params.hash_algo)))
+	if (!(md = EVP_get_digestbyname(algo)))
 		goto err;
 	st = "EVP_PKEY_CTX_set_signature_md";
 	if (!EVP_PKEY_CTX_set_signature_md(ctx, md))

Signature · 1 line

2.30.2

Mimi Zohar

maintainer

2021-06-25 20:42 UTC #8 lore raw

In reply to Stefan Berger

Hi Stefan,

Quoted 1 line · Stefan Berger

This PR contains some cleanups and bugfixes for libimaevm of ima-evm-utils.

In the future, for ima-evm-utils patches, please prefix all patches with "ima-evm-utils", not just the cover letter (e.g. --subject- prefix="PATCH ima-evm-utils").

This patch set is now in next-testing.

thanks,

Mimi