KEY_FLAG_KEEP is not meant to be passed to keyring_alloc() or key_alloc(),
as these only take KEY_ALLOC_* flags. KEY_FLAG_KEEP has the same value as
KEY_ALLOC_BYPASS_RESTRICTION, but fortunately only key_create_or_update()
uses it. LSMs using the key_alloc hook don't check that flag.
KEY_FLAG_KEEP is then ignored but fortunately (again) the root user cannot
write to the blacklist keyring, so it is not possible to remove a key/hash
from it.
Fix this by adding a KEY_ALLOC_SET_KEEP flag that tells key_alloc() to set
KEY_FLAG_KEEP on the new key. blacklist_init() can then, correctly, pass
this to keyring_alloc().
We can also use this in ima_mok_init() rather than setting the flag
manually.
Note that this doesn't fix an observable bug with the current
implementation but it is required to allow addition of new hashes to the
blacklist in the future without making it possible for them to be removed.
Fixes: 734114f8782f ("KEYS: Add a system blacklist keyring")
Reported-by: Mickaël Salaün <[email protected]>
Signed-off-by: David Howells <[email protected]>
cc: Mickaël Salaün <[email protected]>
cc: Petko Manolov <[email protected]>
cc: Mimi Zohar <[email protected]>
Cc: David Woodhouse <[email protected]>
---
certs/blacklist.c | 2 +-
include/linux/key.h | 1 +
security/integrity/ima/ima_mok.c | 3 +--
security/keys/key.c | 2 ++
4 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/certs/blacklist.c b/certs/blacklist.c
index a888b934a1cd..029471947838 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -162,7 +162,7 @@ static int __init blacklist_init(void)
KEY_USR_VIEW | KEY_USR_READ |
KEY_USR_SEARCH,
KEY_ALLOC_NOT_IN_QUOTA |
- KEY_FLAG_KEEP,
+ KEY_ALLOC_SET_KEEP,
NULL, NULL);
if (IS_ERR(blacklist_keyring))
panic("Can't allocate system blacklist keyring\n");
diff --git a/include/linux/key.h b/include/linux/key.h
index 1b0837c975b9..7febc4881363 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -289,6 +289,7 @@ extern struct key *key_alloc(struct key_type *type,
#define KEY_ALLOC_BUILT_IN 0x0004 /* Key is built into kernel */
#define KEY_ALLOC_BYPASS_RESTRICTION 0x0008 /* Override the check on restricted keyrings */
#define KEY_ALLOC_UID_KEYRING 0x0010 /* allocating a user or user session keyring */
+#define KEY_ALLOC_SET_KEEP 0x0020 /* Set the KEEP flag on the key/keyring */
extern void key_revoke(struct key *key);
extern void key_invalidate(struct key *key);
diff --git a/security/integrity/ima/ima_mok.c b/security/integrity/ima/ima_mok.c
index 36cadadbfba4..ce8871d96f12 100644
--- a/security/integrity/ima/ima_mok.c
+++ b/security/integrity/ima/ima_mok.c
@@ -39,12 +39,11 @@ __init int ima_mok_init(void)
KEY_USR_VIEW | KEY_USR_READ |
KEY_USR_WRITE | KEY_USR_SEARCH,
KEY_ALLOC_NOT_IN_QUOTA,
+ KEY_ALLOC_SET_KEEP,
restriction, NULL);
if (IS_ERR(ima_blacklist_keyring))
panic("Can't allocate IMA blacklist keyring.");
-
- set_bit(KEY_FLAG_KEEP, &ima_blacklist_keyring->flags);
return 0;
}
device_initcall(ima_mok_init);
diff --git a/security/keys/key.c b/security/keys/key.c
index e282c6179b21..151ff39b6803 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -303,6 +303,8 @@ struct key *key_alloc(struct key_type *type, const char *desc,
key->flags |= 1 << KEY_FLAG_BUILTIN;
if (flags & KEY_ALLOC_UID_KEYRING)
key->flags |= 1 << KEY_FLAG_UID_KEYRING;
+ if (flags & KEY_ALLOC_SET_KEEP)
+ key->flags |= 1 << KEY_FLAG_KEEP;
#ifdef KEY_DEBUGGING
key->magic = KEY_DEBUG_MAGIC;
Hi Mickaël, Jarkko,
Can I transfer your acks from:
https://lore.kernel.org/lkml/[email protected]/
to here?
David
On 18/02/2021 17:24, David Howells wrote:
>
> Hi Mickaël, Jarkko,
>
> Can I transfer your acks from:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> to here?
No, the current thread contains an old version with an error in the
patch for ima_mok_init(). Please take the last series (fixing this
patch) that I rebased on your next branch:
https://lore.kernel.org/keyrings/[email protected]/
Regards,
Mickaël
Mickaël Salaün <[email protected]> wrote:
> > Can I transfer your acks from:
> >
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > to here?
>
> No, the current thread contains an old version with an error in the
> patch for ima_mok_init(). Please take the last series (fixing this
> patch) that I rebased on your next branch:
> https://lore.kernel.org/keyrings/[email protected]/
Is there a quick fix for the error? If I replace your patches I'll probably
have to withdraw my pull request for this cycle.
David
On Thu, Feb 18, 2021 at 04:24:34PM +0000, David Howells wrote:
>
> Hi Micka?l, Jarkko,
>
> Can I transfer your acks from:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> to here?
>
> David
Yes, thanks.
/Jarkko
On 18/02/2021 23:55, David Howells wrote:
> Mickaël Salaün <[email protected]> wrote:
>
>>> Can I transfer your acks from:
>>>
>>> https://lore.kernel.org/lkml/[email protected]/
>>>
>>> to here?
>>
>> No, the current thread contains an old version with an error in the
>> patch for ima_mok_init(). Please take the last series (fixing this
>> patch) that I rebased on your next branch:
>> https://lore.kernel.org/keyrings/[email protected]/
>
> Is there a quick fix for the error? If I replace your patches I'll probably
> have to withdraw my pull request for this cycle.
I think you just replied to the wrong thread. You have the correct
commit(s) in your tree here:
-
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-fixes&id=a03da41508b177da59780d759af16207a00686bb
-
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next&id=4993e1f9479a4161fd7d93e2b8b30b438f00cb0f
-
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-misc&id=508f44ffefbf879fbb82fdbc8bf1e6023b85158a
For all of these you can add my Reviewed-by or Acked-by.
Thanks!
>
> David
>
Mickaël Salaün <[email protected]> wrote:
> No, the current thread contains an old version with an error in the
> patch for ima_mok_init(). Please take the last series (fixing this
> patch) that I rebased on your next branch:
> https://lore.kernel.org/keyrings/[email protected]/
Are you referring to the comma rather than a bar here?
KEY_ALLOC_NOT_IN_QUOTA,
+ KEY_ALLOC_SET_KEEP,
I fixed that in my branch:
- KEY_ALLOC_NOT_IN_QUOTA,
+ KEY_ALLOC_NOT_IN_QUOTA |
+ KEY_ALLOC_SET_KEEP,
I'll repost the series.
David
On 19/02/2021 10:45, David Howells wrote:
>
> Mickaël Salaün <[email protected]> wrote:
>
>> No, the current thread contains an old version with an error in the
>> patch for ima_mok_init(). Please take the last series (fixing this
>> patch) that I rebased on your next branch:
>> https://lore.kernel.org/keyrings/[email protected]/
>
> Are you referring to the comma rather than a bar here?
Yes, it is correct now, I just replied to the previous email. :)
>
> KEY_ALLOC_NOT_IN_QUOTA,
> + KEY_ALLOC_SET_KEEP,
>
> I fixed that in my branch:
>
> - KEY_ALLOC_NOT_IN_QUOTA,
> + KEY_ALLOC_NOT_IN_QUOTA |
> + KEY_ALLOC_SET_KEEP,
>
> I'll repost the series.
No need for that, I checked your commits, they're good:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-misc&id=508f44ffefbf879fbb82fdbc8bf1e6023b85158a
>
> David
>