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(-) -- 2.30.2
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 <stefanb@linux.ibm.com> --- src/libimaevm.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/libimaevm.c b/src/libimaevm.c index fa6c278..d8fc0d4 100644 --- a/src/libimaevm.c +++ b/src/libimaevm.c @@ -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; -- 2.30.2
The 'off' variable was unused in add_dir_hash(), so remove it. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/libimaevm.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/libimaevm.c b/src/libimaevm.c index d8fc0d4..0137884 100644 --- a/src/libimaevm.c +++ b/src/libimaevm.c @@ -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"); -- 2.30.2
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 <stefanb@linux.ibm.com> --- src/libimaevm.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/libimaevm.c b/src/libimaevm.c index 0137884..9a6739b 100644 --- a/src/libimaevm.c +++ b/src/libimaevm.c @@ -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) -- 2.30.2
Rename function variable from cr (carriage return, '\r') to newline, because this is what it is. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/libimaevm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libimaevm.c b/src/libimaevm.c index 9a6739b..b40b6d8 100644 --- a/src/libimaevm.c +++ b/src/libimaevm.c @@ -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"); } -- 2.30.2
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 <stefanb@linux.ibm.com> --- src/libimaevm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libimaevm.c b/src/libimaevm.c index b40b6d8..06f1063 100644 --- a/src/libimaevm.c +++ b/src/libimaevm.c @@ -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; } -- 2.30.2
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 <stefanb@linux.ibm.com> --- src/libimaevm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libimaevm.c b/src/libimaevm.c index 06f1063..2856270 100644 --- a/src/libimaevm.c +++ b/src/libimaevm.c @@ -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)) -- 2.30.2
On 4/19/21 11:01 AM, Stefan Berger wrote:
> 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
Hi Stefan,
On Mon, 2021-04-19 at 11:01 -0400, Stefan Berger wrote:
> 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