* [PATCH 0/2] ima-evm-utils: Fix use of sign_hash via API
@ 2021-01-06 9:43 Patrick Uiterwijk
2021-01-06 9:43 ` [PATCH 1/2] Fix sign_hash not observing the hashalgo argument Patrick Uiterwijk
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Patrick Uiterwijk @ 2021-01-06 9:43 UTC (permalink / raw)
To: linux-integrity, zohar; +Cc: pbrobinson, Patrick Uiterwijk
When using sign_hash, the resulting signature is incorrect if any hash
algorithm other than sha1 is used.
This is because while the sign_hash function has a hashalgo argument,
the sign_hash_v2 function does not actually use this argument for
anything except setting the hash_algo value in the header.
This patch makes sure it uses the algo variable consistently.
Patrick Uiterwijk (2):
Fix sign_hash not observing the hashalgo argument
Add test for using sign_hash API
src/evmctl.c | 23 ----------------
src/libimaevm.c | 4 +--
src/utils.c | 20 ++++++++++++++
src/utils.h | 1 +
tests/.gitignore | 2 ++
tests/Makefile.am | 5 ++++
tests/sign_verify.apitest.c | 55 +++++++++++++++++++++++++++++++++++++
tests/sign_verify.test | 30 ++++++++++++++++----
8 files changed, 109 insertions(+), 31 deletions(-)
create mode 100644 tests/sign_verify.apitest.c
--
2.26.2
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/2] Fix sign_hash not observing the hashalgo argument 2021-01-06 9:43 [PATCH 0/2] ima-evm-utils: Fix use of sign_hash via API Patrick Uiterwijk @ 2021-01-06 9:43 ` Patrick Uiterwijk 2021-01-07 12:24 ` Mimi Zohar 2021-01-06 9:43 ` [PATCH 2/2] Add test for using sign_hash API Patrick Uiterwijk 2021-07-05 15:49 ` [PATCH ima-evm-utils v2 0/2] Fix use of sign_hash via API Patrick Uiterwijk 2 siblings, 1 reply; 15+ messages in thread From: Patrick Uiterwijk @ 2021-01-06 9:43 UTC (permalink / raw) To: linux-integrity, zohar; +Cc: pbrobinson, Patrick Uiterwijk This fixes sign_hash not using the correct algorithm for creating the signature, by ensuring it uses the passed in variable value. Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> --- src/libimaevm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libimaevm.c b/src/libimaevm.c index fa6c278..72d5e67 100644 --- a/src/libimaevm.c +++ b/src/libimaevm.c @@ -916,7 +916,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); @@ -942,7 +942,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.26.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix sign_hash not observing the hashalgo argument 2021-01-06 9:43 ` [PATCH 1/2] Fix sign_hash not observing the hashalgo argument Patrick Uiterwijk @ 2021-01-07 12:24 ` Mimi Zohar 2021-01-07 13:08 ` Vitaly Chikunov 0 siblings, 1 reply; 15+ messages in thread From: Mimi Zohar @ 2021-01-07 12:24 UTC (permalink / raw) To: Patrick Uiterwijk, linux-integrity; +Cc: pbrobinson, Vitaly Chikunov Hi Patrick, On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote: > This fixes sign_hash not using the correct algorithm for creating the > signature, by ensuring it uses the passed in variable value. > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> Thank you. This is a regression first introduced in commit 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API"). Mimi ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix sign_hash not observing the hashalgo argument 2021-01-07 12:24 ` Mimi Zohar @ 2021-01-07 13:08 ` Vitaly Chikunov 2021-01-07 13:15 ` Vitaly Chikunov 0 siblings, 1 reply; 15+ messages in thread From: Vitaly Chikunov @ 2021-01-07 13:08 UTC (permalink / raw) To: Mimi Zohar; +Cc: Patrick Uiterwijk, linux-integrity, pbrobinson Patrick, Mimi, On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote: > On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote: > > This fixes sign_hash not using the correct algorithm for creating the > > signature, by ensuring it uses the passed in variable value. > > > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> > > Thank you. This is a regression first introduced in commit > 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API"). Ah, when sign_hash() is used not via evmctl, but as a library imaevm_params may be not set. Thanks! ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix sign_hash not observing the hashalgo argument 2021-01-07 13:08 ` Vitaly Chikunov @ 2021-01-07 13:15 ` Vitaly Chikunov 2021-01-07 14:55 ` Mimi Zohar 2021-01-07 15:13 ` Patrick Uiterwijk 0 siblings, 2 replies; 15+ messages in thread From: Vitaly Chikunov @ 2021-01-07 13:15 UTC (permalink / raw) To: Mimi Zohar; +Cc: Patrick Uiterwijk, linux-integrity, pbrobinson On Thu, Jan 07, 2021 at 04:08:16PM +0300, Vitaly Chikunov wrote: > Patrick, Mimi, > > On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote: > > On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote: > > > This fixes sign_hash not using the correct algorithm for creating the > > > signature, by ensuring it uses the passed in variable value. > > > > > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> > > > > Thank you. This is a regression first introduced in commit > > 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API"). > > Ah, when sign_hash() is used not via evmctl, but as a library > imaevm_params may be not set. Thinking about imaevm_params -- it's still belong to a library and extensively used in libimaevm.c, so I wonder if the fix is perfect. Since, now there is hash_algo and algo duplication which one to prefer and why? Maybe, it should be [also] set like keypass in sign_hash? int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig) { if (keypass) imaevm_params.keypass = keypass; + if (hashalgo) + imaevm_params.hash_algo = hashalgo; return imaevm_params.x509 ? sign_hash_v2(hashalgo, hash, size, keyfile, sig) : sign_hash_v1(hashalgo, hash, size, keyfile, sig); } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix sign_hash not observing the hashalgo argument 2021-01-07 13:15 ` Vitaly Chikunov @ 2021-01-07 14:55 ` Mimi Zohar 2021-01-07 15:13 ` Patrick Uiterwijk 1 sibling, 0 replies; 15+ messages in thread From: Mimi Zohar @ 2021-01-07 14:55 UTC (permalink / raw) To: Vitaly Chikunov; +Cc: Patrick Uiterwijk, linux-integrity, pbrobinson On Thu, 2021-01-07 at 16:15 +0300, Vitaly Chikunov wrote: > On Thu, Jan 07, 2021 at 04:08:16PM +0300, Vitaly Chikunov wrote: > > Patrick, Mimi, > > > > On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote: > > > On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote: > > > > This fixes sign_hash not using the correct algorithm for creating the > > > > signature, by ensuring it uses the passed in variable value. > > > > > > > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> > > > > > > Thank you. This is a regression first introduced in commit > > > 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API"). > > > > Ah, when sign_hash() is used not via evmctl, but as a library > > imaevm_params may be not set. > > Thinking about imaevm_params -- it's still belong to a library and > extensively used in libimaevm.c, so I wonder if the fix is perfect. > Since, now there is hash_algo and algo duplication which one to prefer > and why? > > Maybe, it should be [also] set like keypass in sign_hash? > > int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig) > { > if (keypass) > imaevm_params.keypass = keypass; > > + if (hashalgo) > + imaevm_params.hash_algo = hashalgo; > > return imaevm_params.x509 ? > sign_hash_v2(hashalgo, hash, size, keyfile, sig) : > sign_hash_v1(hashalgo, hash, size, keyfile, sig); > } > > As seen above, the main difference between keypass and hashalgo is that hashalgo is being passed as an argument, while keypass isn't. Instead of changing the function arguments, it looks like keypass was stored as global variable. For this reason, the priority should be the function argument, not the global variable. thanks, Mimi ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Fix sign_hash not observing the hashalgo argument 2021-01-07 13:15 ` Vitaly Chikunov 2021-01-07 14:55 ` Mimi Zohar @ 2021-01-07 15:13 ` Patrick Uiterwijk 1 sibling, 0 replies; 15+ messages in thread From: Patrick Uiterwijk @ 2021-01-07 15:13 UTC (permalink / raw) To: Vitaly Chikunov; +Cc: Mimi Zohar, linux-integrity, pbrobinson On Thu, Jan 7, 2021 at 2:15 PM Vitaly Chikunov <vt@altlinux.org> wrote: > > On Thu, Jan 07, 2021 at 04:08:16PM +0300, Vitaly Chikunov wrote: > > Patrick, Mimi, > > > > On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote: > > > On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote: > > > > This fixes sign_hash not using the correct algorithm for creating the > > > > signature, by ensuring it uses the passed in variable value. > > > > > > > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> > > > > > > Thank you. This is a regression first introduced in commit > > > 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API"). > > > > Ah, when sign_hash() is used not via evmctl, but as a library > > imaevm_params may be not set. > > Thinking about imaevm_params -- it's still belong to a library and > extensively used in libimaevm.c, so I wonder if the fix is perfect. > Since, now there is hash_algo and algo duplication which one to prefer > and why? > > Maybe, it should be [also] set like keypass in sign_hash? I had this exact diff as an initial version of this patch, but then personally thought that because "hashalgo" is passed to sign_hash_v2 anyway, and sign_hash_v1 already prefers the argument (and ignores imaevm_params.hash_algo), keeping this behaviour in sync is more consistent. With my patch, imaevm_params.hash_algo is only used in verify_hash_v2 and ima_calc_hash, both of which are primarily called by ima_verify_signature which sets it, as a way to pass the argument without having it explicit everywhere. > > int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig) > { > if (keypass) > imaevm_params.keypass = keypass; > > + if (hashalgo) > + imaevm_params.hash_algo = hashalgo; > > return imaevm_params.x509 ? > sign_hash_v2(hashalgo, hash, size, keyfile, sig) : > sign_hash_v1(hashalgo, hash, size, keyfile, sig); > } > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] Add test for using sign_hash API 2021-01-06 9:43 [PATCH 0/2] ima-evm-utils: Fix use of sign_hash via API Patrick Uiterwijk 2021-01-06 9:43 ` [PATCH 1/2] Fix sign_hash not observing the hashalgo argument Patrick Uiterwijk @ 2021-01-06 9:43 ` Patrick Uiterwijk 2021-01-07 12:25 ` Mimi Zohar 2021-07-05 15:49 ` [PATCH ima-evm-utils v2 0/2] Fix use of sign_hash via API Patrick Uiterwijk 2 siblings, 1 reply; 15+ messages in thread From: Patrick Uiterwijk @ 2021-01-06 9:43 UTC (permalink / raw) To: linux-integrity, zohar; +Cc: pbrobinson, Patrick Uiterwijk This adds a test with a small program that calls the sign_hash API, to ensure that that codepath works. Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> --- src/evmctl.c | 23 ---------------- src/utils.c | 20 ++++++++++++++ src/utils.h | 1 + tests/.gitignore | 2 ++ tests/Makefile.am | 5 ++++ tests/sign_verify.apitest.c | 55 +++++++++++++++++++++++++++++++++++++ tests/sign_verify.test | 30 ++++++++++++++++---- 7 files changed, 107 insertions(+), 29 deletions(-) create mode 100644 tests/sign_verify.apitest.c diff --git a/src/evmctl.c b/src/evmctl.c index 1815f55..bb51688 100644 --- a/src/evmctl.c +++ b/src/evmctl.c @@ -165,29 +165,6 @@ struct tpm_bank_info { static char *pcrfile[MAX_PCRFILE]; static unsigned npcrfile; -static int bin2file(const char *file, const char *ext, const unsigned char *data, int len) -{ - FILE *fp; - char name[strlen(file) + (ext ? strlen(ext) : 0) + 2]; - int err; - - if (ext) - sprintf(name, "%s.%s", file, ext); - else - sprintf(name, "%s", file); - - log_info("Writing to %s\n", name); - - fp = fopen(name, "w"); - if (!fp) { - log_err("Failed to open: %s\n", name); - return -1; - } - err = fwrite(data, len, 1, fp); - fclose(fp); - return err; -} - static unsigned char *file2bin(const char *file, const char *ext, int *size) { FILE *fp; diff --git a/src/utils.c b/src/utils.c index fbb6a4b..6b99e78 100644 --- a/src/utils.c +++ b/src/utils.c @@ -112,3 +112,23 @@ int hex2bin(void *dst, const char *src, size_t count) } return 0; } + +int bin2file(const char *file, const char *ext, const unsigned char *data, int len) +{ + FILE *fp; + char name[strlen(file) + (ext ? strlen(ext) : 0) + 2]; + int err; + + if (ext) + sprintf(name, "%s.%s", file, ext); + else + sprintf(name, "%s", file); + + fp = fopen(name, "w"); + if (!fp) { + return -1; + } + err = fwrite(data, len, 1, fp); + fclose(fp); + return err; +} diff --git a/src/utils.h b/src/utils.h index 9ea179f..081997a 100644 --- a/src/utils.h +++ b/src/utils.h @@ -4,3 +4,4 @@ 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); +int bin2file(const char *file, const char *ext, const unsigned char *data, int len); diff --git a/tests/.gitignore b/tests/.gitignore index 9ecc984..c40735d 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -14,3 +14,5 @@ *.key *.conf +# Compiled version of apitest +sign_verify_apitest diff --git a/tests/Makefile.am b/tests/Makefile.am index ff928e1..74f6125 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -10,3 +10,8 @@ distclean: distclean-keys .PHONY: distclean-keys distclean-keys: ./gen-keys.sh clean + +AUTOMAKE_OPTIONS = subdir-objects +bin_PROGRAMS = sign_verify_apitest +sign_verify_apitest_SOURCES = sign_verify.apitest.c ../src/utils.c +sign_verify_apitest_LDFLAGS = -limaevm -L../src/.libs diff --git a/tests/sign_verify.apitest.c b/tests/sign_verify.apitest.c new file mode 100644 index 0000000..20e2160 --- /dev/null +++ b/tests/sign_verify.apitest.c @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * sign_verify.apitest: Test program for verifying sign_hash + * + * Copyright (C) 2020 Patrick Uiterwijk <patrick@puiterwijk.org> + * Copyright (C) 2013,2014 Samsung Electronics + * Copyright (C) 2011,2012,2013 Intel Corporation + * Copyright (C) 2011 Nokia Corporation + * Copyright (C) 2010 Cyril Hrubis <chrubis@suse.cz> + */ + +#include <assert.h> +#include <stdio.h> +#include <string.h> +#include <sys/types.h> +#include <attr/xattr.h> + +#include "../src/imaevm.h" +#include "../src/utils.h" + +int main(int argc, char **argv) { + unsigned char hash[MAX_DIGEST_SIZE]; + unsigned char sig[MAX_SIGNATURE_SIZE]; + int len, err; + char *file = argv[1]; + char *key = argv[2]; + char *algo = argv[3]; + char *digest = argv[4]; + + len = strlen(digest) / 2; + if (hex2bin(hash, digest, len) != 0) { + fprintf(stderr, "Error during hex2bin\n"); + return 1; + } + + len = sign_hash(algo, hash, len, key, NULL, sig + 1); + if (len <= 1) { + fprintf(stderr, "Error signing\n"); + return 1; + } + + /* add header */ + len++; + sig[0] = EVM_IMA_XATTR_DIGSIG; + + bin2file(file, "sig", sig, len); + + err = lsetxattr(file, "user.ima", sig, len, 0); + if (err < 0) { + log_err("setxattr failed: %s\n", file); + return 1; + } + + return 0; +} diff --git a/tests/sign_verify.test b/tests/sign_verify.test index 288e133..e909d01 100755 --- a/tests/sign_verify.test +++ b/tests/sign_verify.test @@ -65,14 +65,14 @@ _keyid_from_cert() { # Convert test $type into evmctl op prefix _op() { - if [ "$1" = ima ]; then + if [ "$1" = ima -o "$1" = ima_api ]; then echo ima_ fi } # Convert test $type into xattr name _xattr() { - if [ "$1" = ima ]; then + if [ "$1" = ima -o "$1" = ima_api ]; then echo user.ima else echo user.evm @@ -112,11 +112,13 @@ _evmctl_sign() { [ "$type" = ima ] && opts+=" --sigfile" # shellcheck disable=SC2086 - ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \ + [ "$type" = ima -o "$type" = evm ] && (ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \ _evmctl_run "$(_op "$type")sign" $opts \ - --hashalgo "$alg" --key "$key" --xattr-user "$file" || return + --hashalgo "$alg" --key "$key" --xattr-user "$file" || return) + [ "$type" = ima_api ] && ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \ + ./sign_verify_apitest "$file" "$key" "$alg" "$(openssl dgst $OPENSSL_ENGINE -$ALG -hex -r $FILE | awk '{print $1}')" - if [ "$type" = ima ]; then + if [ "$type" = ima -o "$type" = ima_api ]; then _test_sigfile "$file" "$(_xattr "$type")" "$file.sig" "$file.sig2" fi } @@ -124,12 +126,14 @@ _evmctl_sign() { # Run and test {ima_,}sign operation check_sign() { # Arguments are passed via global vars: - # TYPE (ima or evm), + # TYPE (ima, ima_api or evm), # KEY, # ALG (hash algo), # PREFIX (signature header prefix in hex), # OPTS (additional options for evmctl), # FILE (working file to sign). + [ "$TYPE" = ima_api ] && [[ "$OPTS" =~ --rsa ]] && return "$SKIP" + local "$@" local KEY=${KEY%.*}.key local FILE=${FILE:-$ALG.txt} @@ -267,6 +271,20 @@ sign_verify() { # Multiple files and some don't verify expect_fail check_verify FILE="/dev/null $file" + setfattr -x user.ima "$FILE" + rm "$FILE.sig" + fi + + TYPE=ima_api + if expect_pass check_sign; then + + # Normal verify with proper key should pass + expect_pass check_verify + expect_pass check_verify OPTS="--sigfile" + + # Multiple files and some don't verify + expect_fail check_verify FILE="/dev/null $file" + rm "$FILE.sig" fi -- 2.26.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] Add test for using sign_hash API 2021-01-06 9:43 ` [PATCH 2/2] Add test for using sign_hash API Patrick Uiterwijk @ 2021-01-07 12:25 ` Mimi Zohar 2021-01-07 12:53 ` Vitaly Chikunov 0 siblings, 1 reply; 15+ messages in thread From: Mimi Zohar @ 2021-01-07 12:25 UTC (permalink / raw) To: Patrick Uiterwijk, linux-integrity; +Cc: pbrobinson, Vitaly Chikunov Hi Patrick, On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote: > This adds a test with a small program that calls the sign_hash API, to > ensure that that codepath works. > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> Thank you for the regression test. Other than the couple of comments below, Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> > --- > diff --git a/tests/Makefile.am b/tests/Makefile.am > index ff928e1..74f6125 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -10,3 +10,8 @@ distclean: distclean-keys > .PHONY: distclean-keys > distclean-keys: > ./gen-keys.sh clean > + > +AUTOMAKE_OPTIONS = subdir-objects > +bin_PROGRAMS = sign_verify_apitest > +sign_verify_apitest_SOURCES = sign_verify.apitest.c ../src/utils.c > +sign_verify_apitest_LDFLAGS = -limaevm -L../src/.libs > diff --git a/tests/sign_verify.apitest.c b/tests/sign_verify.apitest.c > new file mode 100644 > index 0000000..20e2160 > --- /dev/null > +++ b/tests/sign_verify.apitest.c > @@ -0,0 +1,55 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * sign_verify.apitest: Test program for verifying sign_hash > + * > + * Copyright (C) 2020 Patrick Uiterwijk <patrick@puiterwijk.org> > + * Copyright (C) 2013,2014 Samsung Electronics > + * Copyright (C) 2011,2012,2013 Intel Corporation > + * Copyright (C) 2011 Nokia Corporation > + * Copyright (C) 2010 Cyril Hrubis <chrubis@suse.cz> > + */ Looking at the history, Dmitry Kasatkin must have been involved. From which software package is this from? > + > +#include <assert.h> > +#include <stdio.h> > +#include <string.h> > +#include <sys/types.h> > +#include <attr/xattr.h> This include fails on a couple of distros (e.g. alpine, tumbleweed, and RHEL 8.3). src/evmctl.c is using sys/xattr.h. Mimi > + > +#include "../src/imaevm.h" > +#include "../src/utils.h" ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] Add test for using sign_hash API 2021-01-07 12:25 ` Mimi Zohar @ 2021-01-07 12:53 ` Vitaly Chikunov 2021-01-07 15:08 ` Patrick Uiterwijk 0 siblings, 1 reply; 15+ messages in thread From: Vitaly Chikunov @ 2021-01-07 12:53 UTC (permalink / raw) To: Mimi Zohar; +Cc: Patrick Uiterwijk, linux-integrity, pbrobinson On Thu, Jan 07, 2021 at 07:25:03AM -0500, Mimi Zohar wrote: > > diff --git a/tests/sign_verify.apitest.c b/tests/sign_verify.apitest.c > > new file mode 100644 > > index 0000000..20e2160 > > --- /dev/null > > +++ b/tests/sign_verify.apitest.c > > @@ -0,0 +1,55 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * sign_verify.apitest: Test program for verifying sign_hash > > + * > > + * Copyright (C) 2020 Patrick Uiterwijk <patrick@puiterwijk.org> > > + * Copyright (C) 2013,2014 Samsung Electronics > > + * Copyright (C) 2011,2012,2013 Intel Corporation > > + * Copyright (C) 2011 Nokia Corporation > > + * Copyright (C) 2010 Cyril Hrubis <chrubis@suse.cz> > > + */ Woah, so much copyright for a simple sign_hash() call? > > Looking at the history, Dmitry Kasatkin must have been involved. From > which software package is this from? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] Add test for using sign_hash API 2021-01-07 12:53 ` Vitaly Chikunov @ 2021-01-07 15:08 ` Patrick Uiterwijk 0 siblings, 0 replies; 15+ messages in thread From: Patrick Uiterwijk @ 2021-01-07 15:08 UTC (permalink / raw) To: Vitaly Chikunov; +Cc: Mimi Zohar, linux-integrity, pbrobinson On Thu, Jan 7, 2021 at 1:54 PM Vitaly Chikunov <vt@altlinux.org> wrote: > > On Thu, Jan 07, 2021 at 07:25:03AM -0500, Mimi Zohar wrote: > > > diff --git a/tests/sign_verify.apitest.c b/tests/sign_verify.apitest.c > > > new file mode 100644 > > > index 0000000..20e2160 > > > --- /dev/null > > > +++ b/tests/sign_verify.apitest.c > > > @@ -0,0 +1,55 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * sign_verify.apitest: Test program for verifying sign_hash > > > + * > > > + * Copyright (C) 2020 Patrick Uiterwijk <patrick@puiterwijk.org> > > > + * Copyright (C) 2013,2014 Samsung Electronics > > > + * Copyright (C) 2011,2012,2013 Intel Corporation > > > + * Copyright (C) 2011 Nokia Corporation > > > + * Copyright (C) 2010 Cyril Hrubis <chrubis@suse.cz> > > > + */ > > Woah, so much copyright for a simple sign_hash() call? I basically copied the copyright statements from evmctl.c (and put them in the reversed order as per the new syntax as Mimi did in utils.c as a fix to my previous patchset). This was because the "int main" function started out as a copy of "sign_ima" from evmctl.c. The Samsung, Intel and Nokia entries are all from Dmitry Kasatkin. Cyril's entry is there because I initially copied hex2bin from utils.c before just linking against it, I'll remove that one in a next version. > > > > > Looking at the history, Dmitry Kasatkin must have been involved. From > > which software package is this from? ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH ima-evm-utils v2 0/2] Fix use of sign_hash via API 2021-01-06 9:43 [PATCH 0/2] ima-evm-utils: Fix use of sign_hash via API Patrick Uiterwijk 2021-01-06 9:43 ` [PATCH 1/2] Fix sign_hash not observing the hashalgo argument Patrick Uiterwijk 2021-01-06 9:43 ` [PATCH 2/2] Add test for using sign_hash API Patrick Uiterwijk @ 2021-07-05 15:49 ` Patrick Uiterwijk 2021-07-05 15:49 ` [PATCH ima-evm-utils v2 1/2] Fix sign_hash not observing the hashalgo argument Patrick Uiterwijk 2021-07-05 15:49 ` [PATCH ima-evm-utils v2 2/2] Add test for using sign_hash API Patrick Uiterwijk 2 siblings, 2 replies; 15+ messages in thread From: Patrick Uiterwijk @ 2021-07-05 15:49 UTC (permalink / raw) To: linux-integrity, zohar; +Cc: pbrobinson, patrick When using sign_hash, the resulting signature is incorrect if any hash algorithm other than sha1 is used. This is because while the sign_hash function has a hashalgo argument, the sign_hash_v2 function does not actually use this argument for anything except setting the hash_algo value in the header. This patch makes sure it uses the algo variable consistently. Changes since v1: - Using sys/xattr.h - Removed copyright line for sign_ima Patrick Uiterwijk (2): Fix sign_hash not observing the hashalgo argument Add test for using sign_hash API src/evmctl.c | 23 ---------------- src/libimaevm.c | 4 +-- src/utils.c | 20 ++++++++++++++ src/utils.h | 1 + tests/.gitignore | 2 ++ tests/Makefile.am | 5 ++++ tests/sign_verify.apitest.c | 55 +++++++++++++++++++++++++++++++++++++ tests/sign_verify.test | 30 ++++++++++++++++---- 8 files changed, 109 insertions(+), 31 deletions(-) create mode 100644 tests/sign_verify.apitest.c -- 2.31.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH ima-evm-utils v2 1/2] Fix sign_hash not observing the hashalgo argument 2021-07-05 15:49 ` [PATCH ima-evm-utils v2 0/2] Fix use of sign_hash via API Patrick Uiterwijk @ 2021-07-05 15:49 ` Patrick Uiterwijk 2021-07-05 15:49 ` [PATCH ima-evm-utils v2 2/2] Add test for using sign_hash API Patrick Uiterwijk 1 sibling, 0 replies; 15+ messages in thread From: Patrick Uiterwijk @ 2021-07-05 15:49 UTC (permalink / raw) To: linux-integrity, zohar; +Cc: pbrobinson, patrick This fixes sign_hash not using the correct algorithm for creating the signature, by ensuring it uses the passed in variable value. Fixes: 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API"). Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> --- 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.31.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH ima-evm-utils v2 2/2] Add test for using sign_hash API 2021-07-05 15:49 ` [PATCH ima-evm-utils v2 0/2] Fix use of sign_hash via API Patrick Uiterwijk 2021-07-05 15:49 ` [PATCH ima-evm-utils v2 1/2] Fix sign_hash not observing the hashalgo argument Patrick Uiterwijk @ 2021-07-05 15:49 ` Patrick Uiterwijk 2021-07-06 15:53 ` Mimi Zohar 1 sibling, 1 reply; 15+ messages in thread From: Patrick Uiterwijk @ 2021-07-05 15:49 UTC (permalink / raw) To: linux-integrity, zohar; +Cc: pbrobinson, patrick This adds a test with a small program that calls the sign_hash API, to ensure that that codepath works. Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> --- src/evmctl.c | 23 ---------------- src/utils.c | 20 ++++++++++++++ src/utils.h | 1 + tests/.gitignore | 2 ++ tests/Makefile.am | 5 ++++ tests/sign_verify.apitest.c | 55 +++++++++++++++++++++++++++++++++++++ tests/sign_verify.test | 30 ++++++++++++++++---- 7 files changed, 107 insertions(+), 29 deletions(-) create mode 100644 tests/sign_verify.apitest.c diff --git a/src/evmctl.c b/src/evmctl.c index 7a6f202..867b142 100644 --- a/src/evmctl.c +++ b/src/evmctl.c @@ -166,29 +166,6 @@ struct tpm_bank_info { static char *pcrfile[MAX_PCRFILE]; static unsigned npcrfile; -static int bin2file(const char *file, const char *ext, const unsigned char *data, int len) -{ - FILE *fp; - char name[strlen(file) + (ext ? strlen(ext) : 0) + 2]; - int err; - - if (ext) - sprintf(name, "%s.%s", file, ext); - else - sprintf(name, "%s", file); - - log_info("Writing to %s\n", name); - - fp = fopen(name, "w"); - if (!fp) { - log_err("Failed to open: %s\n", name); - return -1; - } - err = fwrite(data, len, 1, fp); - fclose(fp); - return err; -} - static unsigned char *file2bin(const char *file, const char *ext, int *size) { FILE *fp; diff --git a/src/utils.c b/src/utils.c index fbb6a4b..ce2cc28 100644 --- a/src/utils.c +++ b/src/utils.c @@ -112,3 +112,23 @@ int hex2bin(void *dst, const char *src, size_t count) } return 0; } + +int bin2file(const char *file, const char *ext, const unsigned char *data, int len) +{ + FILE *fp; + char name[strlen(file) + (ext ? strlen(ext) : 0) + 2]; + int err; + + if (ext) + sprintf(name, "%s.%s", file, ext); + else + sprintf(name, "%s", file); + + fp = fopen(name, "w"); + if (!fp) + return -1; + + err = fwrite(data, len, 1, fp); + fclose(fp); + return err; +} diff --git a/src/utils.h b/src/utils.h index 9ea179f..081997a 100644 --- a/src/utils.h +++ b/src/utils.h @@ -4,3 +4,4 @@ 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); +int bin2file(const char *file, const char *ext, const unsigned char *data, int len); diff --git a/tests/.gitignore b/tests/.gitignore index 9ecc984..c40735d 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -14,3 +14,5 @@ *.key *.conf +# Compiled version of apitest +sign_verify_apitest diff --git a/tests/Makefile.am b/tests/Makefile.am index ff928e1..74f6125 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -10,3 +10,8 @@ distclean: distclean-keys .PHONY: distclean-keys distclean-keys: ./gen-keys.sh clean + +AUTOMAKE_OPTIONS = subdir-objects +bin_PROGRAMS = sign_verify_apitest +sign_verify_apitest_SOURCES = sign_verify.apitest.c ../src/utils.c +sign_verify_apitest_LDFLAGS = -limaevm -L../src/.libs diff --git a/tests/sign_verify.apitest.c b/tests/sign_verify.apitest.c new file mode 100644 index 0000000..3fcd043 --- /dev/null +++ b/tests/sign_verify.apitest.c @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * sign_verify.apitest: Test program for verifying sign_hash + * + * Copyright (C) 2021 Patrick Uiterwijk <patrick@puiterwijk.org> + * Copyright (C) 2013,2014 Samsung Electronics + * Copyright (C) 2011,2012,2013 Intel Corporation + * Copyright (C) 2011 Nokia Corporation + */ + +#include <assert.h> +#include <stdio.h> +#include <string.h> +#include <sys/types.h> +#include <sys/xattr.h> + +#include "../src/imaevm.h" +#include "../src/utils.h" + +int main(int argc, char **argv) +{ + unsigned char hash[MAX_DIGEST_SIZE]; + unsigned char sig[MAX_SIGNATURE_SIZE]; + int len, err; + char *file = argv[1]; + char *key = argv[2]; + char *algo = argv[3]; + char *digest = argv[4]; + + len = strlen(digest) / 2; + if (hex2bin(hash, digest, len) != 0) { + fprintf(stderr, "Error during hex2bin\n"); + return 1; + } + + len = sign_hash(algo, hash, len, key, NULL, sig + 1); + if (len <= 1) { + fprintf(stderr, "Error signing\n"); + return 1; + } + + /* add header */ + len++; + sig[0] = EVM_IMA_XATTR_DIGSIG; + + bin2file(file, "sig", sig, len); + + err = lsetxattr(file, "user.ima", sig, len, 0); + if (err < 0) { + log_err("setxattr failed: %s\n", file); + return 1; + } + + return 0; +} diff --git a/tests/sign_verify.test b/tests/sign_verify.test index 3d7aa51..6f92801 100755 --- a/tests/sign_verify.test +++ b/tests/sign_verify.test @@ -66,14 +66,14 @@ _keyid_from_cert() { # Convert test $type into evmctl op prefix _op() { - if [ "$1" = ima ]; then + if [ "$1" = ima -o "$1" = ima_api ]; then echo ima_ fi } # Convert test $type into xattr name _xattr() { - if [ "$1" = ima ]; then + if [ "$1" = ima -o "$1" = ima_api ]; then echo user.ima else echo user.evm @@ -113,11 +113,13 @@ _evmctl_sign() { [ "$type" = ima ] && opts+=" --sigfile" # shellcheck disable=SC2086 - ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \ + [ "$type" = ima -o "$type" = evm ] && (ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \ _evmctl_run "$(_op "$type")sign" $opts \ - --hashalgo "$alg" --key "$key" --xattr-user "$file" || return + --hashalgo "$alg" --key "$key" --xattr-user "$file" || return) + [ "$type" = ima_api ] && ADD_TEXT_FOR="$alg ($key)" ADD_DEL=$file \ + ./sign_verify_apitest "$file" "$key" "$alg" "$(openssl dgst $OPENSSL_ENGINE -$ALG -hex -r $FILE | awk '{print $1}')" - if [ "$type" = ima ]; then + if [ "$type" = ima -o "$type" = ima_api ]; then _test_sigfile "$file" "$(_xattr "$type")" "$file.sig" "$file.sig2" fi } @@ -125,12 +127,14 @@ _evmctl_sign() { # Run and test {ima_,}sign operation check_sign() { # Arguments are passed via global vars: - # TYPE (ima or evm), + # TYPE (ima, ima_api or evm), # KEY, # ALG (hash algo), # PREFIX (signature header prefix in hex), # OPTS (additional options for evmctl), # FILE (working file to sign). + [ "$TYPE" = ima_api ] && [[ "$OPTS" =~ --rsa ]] && return "$SKIP" + local "$@" local KEY=${KEY%.*}.key local FILE=${FILE:-$ALG.txt} @@ -268,6 +272,20 @@ sign_verify() { # Multiple files and some don't verify expect_fail check_verify FILE="/dev/null $file" + setfattr -x user.ima "$FILE" + rm "$FILE.sig" + fi + + TYPE=ima_api + if expect_pass check_sign; then + + # Normal verify with proper key should pass + expect_pass check_verify + expect_pass check_verify OPTS="--sigfile" + + # Multiple files and some don't verify + expect_fail check_verify FILE="/dev/null $file" + rm "$FILE.sig" fi -- 2.31.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH ima-evm-utils v2 2/2] Add test for using sign_hash API 2021-07-05 15:49 ` [PATCH ima-evm-utils v2 2/2] Add test for using sign_hash API Patrick Uiterwijk @ 2021-07-06 15:53 ` Mimi Zohar 0 siblings, 0 replies; 15+ messages in thread From: Mimi Zohar @ 2021-07-06 15:53 UTC (permalink / raw) To: Patrick Uiterwijk, linux-integrity; +Cc: pbrobinson, Vitaly Chikunov Hi Patrick, On Mon, 2021-07-05 at 17:49 +0200, Patrick Uiterwijk wrote: > This adds a test with a small program that calls the sign_hash API, to > ensure that that codepath works. > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> Somehow I missed that running the test without this patch the summary "SKIP" info matches the "skipped" messages, but running with the patch there's a discrepancy. $ ./sign_verify.test &> /tmp/sign_verify.log $ tail -5 /tmp/sign_verify.log evmctl ima_sign failed properly with (1) EVP_get_digestbyname(md_gost12_512) failed errno: No such file or directory (2) PASS: 127 SKIP: 20 FAIL: 0 $ grep "skipped" /tmp/sign_verify.log | wc -l 20 $ tail -5 /tmp/sign_verify.log-api evmctl ima_sign: no detached signature md_gost12_512.txt~.sig rm: cannot remove 'md_gost12_512.txt~': No such file or directory PASS: 175 SKIP: 32 FAIL: 0 $ grep "skipped" /tmp/sign_verify.log-api | wc -l 30 > diff --git a/tests/sign_verify.apitest.c b/tests/sign_verify.apitest.c > new file mode 100644 > index 0000000..3fcd043 > --- /dev/null > +++ b/tests/sign_verify.apitest.c > @@ -0,0 +1,55 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * sign_verify.apitest: Test program for verifying sign_hash > + * > + * Copyright (C) 2021 Patrick Uiterwijk <patrick@puiterwijk.org> > + * Copyright (C) 2013,2014 Samsung Electronics > + * Copyright (C) 2011,2012,2013 Intel Corporation > + * Copyright (C) 2011 Nokia Corporation > + */ As this is a new file and test, the copyrights other than your own are unnecessary. > + > +#include <assert.h> > +#include <stdio.h> > +#include <string.h> > +#include <sys/types.h> > +#include <sys/xattr.h> > + > +#include "../src/imaevm.h" > +#include "../src/utils.h" > + > +int main(int argc, char **argv) > +{ > + unsigned char hash[MAX_DIGEST_SIZE]; > + unsigned char sig[MAX_SIGNATURE_SIZE]; > + int len, err; > + char *file = argv[1]; > + char *key = argv[2]; > + char *algo = argv[3]; > + char *digest = argv[4]; > + How about testing 'argc' before continuing? > + len = strlen(digest) / 2; > + if (hex2bin(hash, digest, len) != 0) { > + fprintf(stderr, "Error during hex2bin\n"); > + return 1; > + } > + > + len = sign_hash(algo, hash, len, key, NULL, sig + 1); > + if (len <= 1) { > + fprintf(stderr, "Error signing\n"); > + return 1; > + } > + > + /* add header */ > + len++; > + sig[0] = EVM_IMA_XATTR_DIGSIG; > + > + bin2file(file, "sig", sig, len); > + > + err = lsetxattr(file, "user.ima", sig, len, 0); > + if (err < 0) { > + log_err("setxattr failed: %s\n", file); > + return 1; > + } > + > + return 0; > +} > diff --git a/tests/sign_verify.test b/tests/sign_verify.test > index 3d7aa51..6f92801 100755 > --- a/tests/sign_verify.test > +++ b/tests/sign_verify.test > @@ -125,12 +127,14 @@ _evmctl_sign() { > # Run and test {ima_,}sign operation > check_sign() { > # Arguments are passed via global vars: > - # TYPE (ima or evm), > + # TYPE (ima, ima_api or evm), Similarly TYPE should be updated in verify_sign as well. thanks, Mimi > # KEY, > # ALG (hash algo), > # PREFIX (signature header prefix in hex), > # OPTS (additional options for evmctl), > # FILE (working file to sign). > + [ "$TYPE" = ima_api ] && [[ "$OPTS" =~ --rsa ]] && return "$SKIP" > + > local "$@" > local KEY=${KEY%.*}.key > local FILE=${FILE:-$ALG.txt} > @@ -268,6 +272,20 @@ sign_verify() { > # Multiple files and some don't verify > expect_fail check_verify FILE="/dev/null $file" > > + setfattr -x user.ima "$FILE" > + rm "$FILE.sig" > + fi > + > + TYPE=ima_api > + if expect_pass check_sign; then > + > + # Normal verify with proper key should pass > + expect_pass check_verify > + expect_pass check_verify OPTS="--sigfile" > + > + # Multiple files and some don't verify > + expect_fail check_verify FILE="/dev/null $file" > + > rm "$FILE.sig" > fi > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-07-06 15:53 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-06 9:43 [PATCH 0/2] ima-evm-utils: Fix use of sign_hash via API Patrick Uiterwijk 2021-01-06 9:43 ` [PATCH 1/2] Fix sign_hash not observing the hashalgo argument Patrick Uiterwijk 2021-01-07 12:24 ` Mimi Zohar 2021-01-07 13:08 ` Vitaly Chikunov 2021-01-07 13:15 ` Vitaly Chikunov 2021-01-07 14:55 ` Mimi Zohar 2021-01-07 15:13 ` Patrick Uiterwijk 2021-01-06 9:43 ` [PATCH 2/2] Add test for using sign_hash API Patrick Uiterwijk 2021-01-07 12:25 ` Mimi Zohar 2021-01-07 12:53 ` Vitaly Chikunov 2021-01-07 15:08 ` Patrick Uiterwijk 2021-07-05 15:49 ` [PATCH ima-evm-utils v2 0/2] Fix use of sign_hash via API Patrick Uiterwijk 2021-07-05 15:49 ` [PATCH ima-evm-utils v2 1/2] Fix sign_hash not observing the hashalgo argument Patrick Uiterwijk 2021-07-05 15:49 ` [PATCH ima-evm-utils v2 2/2] Add test for using sign_hash API Patrick Uiterwijk 2021-07-06 15:53 ` Mimi Zohar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).