| Message ID | 1563460194-21569-2-git-send-email-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] ima_evm_utils: erroneous "verification failed: 0 (invalid padding)" message | expand |
Mimi, On Thu, Jul 18, 2019 at 10:29:54AM -0400, Mimi Zohar wrote: > Each tima a new unknown key is encountered, emit a message of the format > "key #: <keyid> unknown". The individual files using unknown keys are > then only logged in verbose mode. > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > --- > src/libimaevm.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/src/libimaevm.c b/src/libimaevm.c > index 43eb4ef2412c..d2194a6ca0f8 100644 > --- a/src/libimaevm.c > +++ b/src/libimaevm.c > @@ -402,13 +402,26 @@ static struct public_key_entry *public_keys = NULL; > > static EVP_PKEY *find_keyid(uint32_t keyid) > { > - struct public_key_entry *entry; > + struct public_key_entry *entry, *tail = public_keys; If user specified in `-k` filename that does not exist no key is added into public_keys and it remains NULL. > + int i = 1; > > for (entry = public_keys; entry != NULL; entry = entry->next) { > if (entry->keyid == keyid) > return entry->key; > + i++; > + tail = entry; > } > - return NULL; > + > + /* add unknown keys to list */ > + entry = calloc(1, sizeof(struct public_key_entry)); > + if (!entry) { > + perror("calloc"); > + return 0; > + } > + entry->keyid = keyid; > + tail->next = entry; In that case here is SIGSEGV when user try to ima_verify. > + log_err("key %d: %x unknown\n", i, __be32_to_cpup(&keyid)); > + return 0; > } > > void init_public_keys(const char *keyfiles) > @@ -470,8 +483,8 @@ static int verify_hash_v2(const char *file, const unsigned char *hash, int size, > > pkey = find_keyid(keyid); > if (!pkey) { > - log_err("%s: unknown keyid: %x\n", > - file, __be32_to_cpup(&keyid)); > + log_info("%s: unknown keyid: %x\n", > + file, __be32_to_cpup(&keyid)); > return -1; > } >
On Wed, 2019-07-24 at 01:18 +0300, Vitaly Chikunov wrote: > Mimi, > > On Thu, Jul 18, 2019 at 10:29:54AM -0400, Mimi Zohar wrote: > > Each tima a new unknown key is encountered, emit a message of the format > > "key #: <keyid> unknown". The individual files using unknown keys are > > then only logged in verbose mode. > > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > --- > > src/libimaevm.c | 21 +++++++++++++++++---- > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/src/libimaevm.c b/src/libimaevm.c > > index 43eb4ef2412c..d2194a6ca0f8 100644 > > --- a/src/libimaevm.c > > +++ b/src/libimaevm.c > > @@ -402,13 +402,26 @@ static struct public_key_entry *public_keys = NULL; > > > > static EVP_PKEY *find_keyid(uint32_t keyid) > > { > > - struct public_key_entry *entry; > > + struct public_key_entry *entry, *tail = public_keys; > > If user specified in `-k` filename that does not exist no key is added > into public_keys and it remains NULL. Right, just the keyid is added and an error message is emitted. > > > + int i = 1; > > > > for (entry = public_keys; entry != NULL; entry = entry->next) { > > if (entry->keyid == keyid) > > return entry->key; > > + i++; > > + tail = entry; > > } > > - return NULL; > > + > > + /* add unknown keys to list */ > > + entry = calloc(1, sizeof(struct public_key_entry)); > > + if (!entry) { > > + perror("calloc"); > > + return 0; > > + } > > + entry->keyid = keyid; > > + tail->next = entry; > > In that case here is SIGSEGV when user try to ima_verify. find_keyid() returns NULL, which is checked before being used. There's only one caller of this function. Do you have a test case to reproduce this bug? Mimi > > > + log_err("key %d: %x unknown\n", i, __be32_to_cpup(&keyid)); > > + return 0; > > } > > > > void init_public_keys(const char *keyfiles) > > @@ -470,8 +483,8 @@ static int verify_hash_v2(const char *file, const unsigned char *hash, int size, > > > > pkey = find_keyid(keyid); > > if (!pkey) { > > - log_err("%s: unknown keyid: %x\n", > > - file, __be32_to_cpup(&keyid)); > > + log_info("%s: unknown keyid: %x\n", > > + file, __be32_to_cpup(&keyid)); > > return -1; > > } > > >
Mimi, On Tue, Jul 23, 2019 at 06:59:16PM -0400, Mimi Zohar wrote: > On Wed, 2019-07-24 at 01:18 +0300, Vitaly Chikunov wrote: > > On Thu, Jul 18, 2019 at 10:29:54AM -0400, Mimi Zohar wrote: > > > Each tima a new unknown key is encountered, emit a message of the format > > > "key #: <keyid> unknown". The individual files using unknown keys are > > > then only logged in verbose mode. > > > > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > > --- > > > src/libimaevm.c | 21 +++++++++++++++++---- > > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/libimaevm.c b/src/libimaevm.c > > > index 43eb4ef2412c..d2194a6ca0f8 100644 > > > --- a/src/libimaevm.c > > > +++ b/src/libimaevm.c > > > @@ -402,13 +402,26 @@ static struct public_key_entry *public_keys = NULL; > > > > > > static EVP_PKEY *find_keyid(uint32_t keyid) > > > { > > > - struct public_key_entry *entry; > > > + struct public_key_entry *entry, *tail = public_keys; > > > > If user specified in `-k` filename that does not exist no key is added > > into public_keys and it remains NULL. > > Right, just the keyid is added and an error message is emitted. > > > > > > + int i = 1; > > > > > > for (entry = public_keys; entry != NULL; entry = entry->next) { > > > if (entry->keyid == keyid) > > > return entry->key; > > > + i++; > > > + tail = entry; > > > } > > > - return NULL; > > > + > > > + /* add unknown keys to list */ > > > + entry = calloc(1, sizeof(struct public_key_entry)); > > > + if (!entry) { > > > + perror("calloc"); > > > + return 0; > > > + } > > > + entry->keyid = keyid; > > > + tail->next = entry; > > > > In that case here is SIGSEGV when user try to ima_verify. > > find_keyid() returns NULL, which is checked before being used. > There's only one caller of this function. find_keyid does not return since tail is NULL, thus there is null dereference in `tail->next`. > Do you have a test case to reproduce this bug? ima-evm-utils/tests ((31b5f50...))$ rm sha1.txt ima-evm-utils/tests ((31b5f50...))$ touch sha1.txt ima-evm-utils/tests ((31b5f50...))$ evmctl ima_sign -k test-rsa1024.key -a sha1 --xattr-user sha1.txt ima-evm-utils/tests ((31b5f50...))$ evmctl ima_verify -k /absent --xattr-user sha1.txt Failed to open keyfile: /absent Segmentation fault 31b5f50 is head of your git next branch. I temporary fixed like this: - tail->next = entry; + if (tail) + tail->next = entry; + else + public_keys = entry;
On Wed, 2019-07-24 at 02:13 +0300, Vitaly Chikunov wrote: > Mimi, > > On Tue, Jul 23, 2019 at 06:59:16PM -0400, Mimi Zohar wrote: > > On Wed, 2019-07-24 at 01:18 +0300, Vitaly Chikunov wrote: > > > On Thu, Jul 18, 2019 at 10:29:54AM -0400, Mimi Zohar wrote: > > > > Each tima a new unknown key is encountered, emit a message of the format > > > > "key #: <keyid> unknown". The individual files using unknown keys are > > > > then only logged in verbose mode. > > > > > > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > > > --- > > > > src/libimaevm.c | 21 +++++++++++++++++---- > > > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/src/libimaevm.c b/src/libimaevm.c > > > > index 43eb4ef2412c..d2194a6ca0f8 100644 > > > > --- a/src/libimaevm.c > > > > +++ b/src/libimaevm.c > > > > @@ -402,13 +402,26 @@ static struct public_key_entry *public_keys = NULL; > > > > > > > > static EVP_PKEY *find_keyid(uint32_t keyid) > > > > { > > > > - struct public_key_entry *entry; > > > > + struct public_key_entry *entry, *tail = public_keys; > > > > > > If user specified in `-k` filename that does not exist no key is added > > > into public_keys and it remains NULL. > > > > Right, just the keyid is added and an error message is emitted. > > > > > > > > > + int i = 1; > > > > > > > > for (entry = public_keys; entry != NULL; entry = entry->next) { > > > > if (entry->keyid == keyid) > > > > return entry->key; > > > > + i++; > > > > + tail = entry; > > > > } > > > > - return NULL; > > > > + > > > > + /* add unknown keys to list */ > > > > + entry = calloc(1, sizeof(struct public_key_entry)); > > > > + if (!entry) { > > > > + perror("calloc"); > > > > + return 0; > > > > + } > > > > + entry->keyid = keyid; > > > > + tail->next = entry; > > > > > > In that case here is SIGSEGV when user try to ima_verify. > > > > find_keyid() returns NULL, which is checked before being used. > > There's only one caller of this function. > > find_keyid does not return since tail is NULL, thus there is null > dereference in `tail->next`. > > > Do you have a test case to reproduce this bug? > > ima-evm-utils/tests ((31b5f50...))$ rm sha1.txt > ima-evm-utils/tests ((31b5f50...))$ touch sha1.txt > ima-evm-utils/tests ((31b5f50...))$ evmctl ima_sign -k test-rsa1024.key -a sha1 --xattr-user sha1.txt > ima-evm-utils/tests ((31b5f50...))$ evmctl ima_verify -k /absent --xattr-user sha1.txt > Failed to open keyfile: /absent > Segmentation fault > > 31b5f50 is head of your git next branch. > > I temporary fixed like this: > > - tail->next = entry; > + if (tail) > + tail->next = entry; > + else > + public_keys = entry; I see. If you don't object, I'll fold this into the original patch. Mimi
Mimi, On Tue, Jul 23, 2019 at 08:19:46PM -0400, Mimi Zohar wrote: > On Wed, 2019-07-24 at 02:13 +0300, Vitaly Chikunov wrote: > > > > > + tail->next = entry; > > > > > > > > In that case here is SIGSEGV when user try to ima_verify. > > > > > > find_keyid() returns NULL, which is checked before being used. > > > There's only one caller of this function. > > > > find_keyid does not return since tail is NULL, thus there is null > > dereference in `tail->next`. > > > > > Do you have a test case to reproduce this bug? > > > > ima-evm-utils/tests ((31b5f50...))$ rm sha1.txt > > ima-evm-utils/tests ((31b5f50...))$ touch sha1.txt > > ima-evm-utils/tests ((31b5f50...))$ evmctl ima_sign -k test-rsa1024.key -a sha1 --xattr-user sha1.txt > > ima-evm-utils/tests ((31b5f50...))$ evmctl ima_verify -k /absent --xattr-user sha1.txt > > Failed to open keyfile: /absent > > Segmentation fault > > > > 31b5f50 is head of your git next branch. > > > > I temporary fixed like this: > > > > - tail->next = entry; > > + if (tail) > > + tail->next = entry; > > + else > > + public_keys = entry; > > I see. If you don't object, I'll fold this into the original patch. Of course. Thanks,
diff --git a/src/libimaevm.c b/src/libimaevm.c index 43eb4ef2412c..d2194a6ca0f8 100644 --- a/src/libimaevm.c +++ b/src/libimaevm.c @@ -402,13 +402,26 @@ static struct public_key_entry *public_keys = NULL; static EVP_PKEY *find_keyid(uint32_t keyid) { - struct public_key_entry *entry; + struct public_key_entry *entry, *tail = public_keys; + int i = 1; for (entry = public_keys; entry != NULL; entry = entry->next) { if (entry->keyid == keyid) return entry->key; + i++; + tail = entry; } - return NULL; + + /* add unknown keys to list */ + entry = calloc(1, sizeof(struct public_key_entry)); + if (!entry) { + perror("calloc"); + return 0; + } + entry->keyid = keyid; + tail->next = entry; + log_err("key %d: %x unknown\n", i, __be32_to_cpup(&keyid)); + return 0; } void init_public_keys(const char *keyfiles) @@ -470,8 +483,8 @@ static int verify_hash_v2(const char *file, const unsigned char *hash, int size, pkey = find_keyid(keyid); if (!pkey) { - log_err("%s: unknown keyid: %x\n", - file, __be32_to_cpup(&keyid)); + log_info("%s: unknown keyid: %x\n", + file, __be32_to_cpup(&keyid)); return -1; }
Each tima a new unknown key is encountered, emit a message of the format "key #: <keyid> unknown". The individual files using unknown keys are then only logged in verbose mode. Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- src/libimaevm.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)