diff mbox series

[v3,04/10] certs: Fix blacklist flag type confusion

Message ID 20210114151909.2344974-5-mic@digikod.net (mailing list archive)
State New
Headers show
Series Enable root to update the blacklist keyring | expand

Commit Message

Mickaël Salaün Jan. 14, 2021, 3:19 p.m. UTC 
From: David Howells <dhowells@redhat.com>

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")
cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Reported-by: Mickaël Salaün <mic@linux.microsoft.com>
Signed-off-by: David Howells <dhowells@redhat.com>
[mic@linux.microsoft.com: fix ima_mok_init()]
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
---

Changes since v2:
* Cherry-pick rewritten v1 patch from
  https://lore.kernel.org/lkml/2659836.1607940186@warthog.procyon.org.uk/
  to rebase on v5.11-rc3 and fix ima_mok_init().
---
 certs/blacklist.c                | 2 +-
 include/linux/key.h              | 1 +
 security/integrity/ima/ima_mok.c | 4 +---
 security/keys/key.c              | 2 ++
 4 files changed, 5 insertions(+), 4 deletions(-)

Comments

Jarkko Sakkinen Jan. 20, 2021, 3:55 a.m. UTC | #1 
On Thu, Jan 14, 2021 at 04:19:03PM +0100, Mickaël Salaün wrote:
> From: David Howells <dhowells@redhat.com>
> 
> 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().

OK, so thing work by luck now, but given the new patches which allow
to append new keys they would break, right?

> We can also use this in ima_mok_init() rather than setting the flag
> manually.

What does ima_mok_init() do?
> 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")
> cc: Mimi Zohar <zohar@linux.vnet.ibm.com>

Nit: Cc

> Cc: David Woodhouse <dwmw2@infradead.org>
> Reported-by: Mickaël Salaün <mic@linux.microsoft.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> [mic@linux.microsoft.com: fix ima_mok_init()]
> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> ---
> 
> Changes since v2:
> * Cherry-pick rewritten v1 patch from
>   https://lore.kernel.org/lkml/2659836.1607940186@warthog.procyon.org.uk/
>   to rebase on v5.11-rc3 and fix ima_mok_init().
> ---
>  certs/blacklist.c                | 2 +-
>  include/linux/key.h              | 1 +
>  security/integrity/ima/ima_mok.c | 4 +---
>  security/keys/key.c              | 2 ++
>  4 files changed, 5 insertions(+), 4 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 0f2e24f13c2b..eed3ce139a32 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..5594dd38ab04 100644
> --- a/security/integrity/ima/ima_mok.c
> +++ b/security/integrity/ima/ima_mok.c
> @@ -38,13 +38,11 @@ __init int ima_mok_init(void)
>  				(KEY_POS_ALL & ~KEY_POS_SETATTR) |
>  				KEY_USR_VIEW | KEY_USR_READ |
>  				KEY_USR_WRITE | KEY_USR_SEARCH,
> -				KEY_ALLOC_NOT_IN_QUOTA,
> +				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 ebe752b137aa..c45afdd1dfbb 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;
> -- 
> 2.30.0
> 

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkko
Mickaël Salaün Jan. 20, 2021, 11:15 a.m. UTC | #2 
On 20/01/2021 04:55, Jarkko Sakkinen wrote:
> On Thu, Jan 14, 2021 at 04:19:03PM +0100, Mickaël Salaün wrote:
>> From: David Howells <dhowells@redhat.com>
>>
>> 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().
> 
> OK, so thing work by luck now, but given the new patches which allow
> to append new keys they would break, right?

Without this fix, patch 9/10 would allow to remove and modify keys from
the blacklist keyring.

> 
>> We can also use this in ima_mok_init() rather than setting the flag
>> manually.
> 
> What does ima_mok_init() do?

This was initially an addition from David Howells, I only fixed the
argument bit-ORing. ima_mok_init() allocates a blacklist keyring (with
different properties) dedicated to IMA.

>> 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")
>> cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> 
> Nit: Cc

OK

> 
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Reported-by: Mickaël Salaün <mic@linux.microsoft.com>
>> Signed-off-by: David Howells <dhowells@redhat.com>
>> [mic@linux.microsoft.com: fix ima_mok_init()]
>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
>> ---
>>
>> Changes since v2:
>> * Cherry-pick rewritten v1 patch from
>>   https://lore.kernel.org/lkml/2659836.1607940186@warthog.procyon.org.uk/
>>   to rebase on v5.11-rc3 and fix ima_mok_init().
>> ---
>>  certs/blacklist.c                | 2 +-
>>  include/linux/key.h              | 1 +
>>  security/integrity/ima/ima_mok.c | 4 +---
>>  security/keys/key.c              | 2 ++
>>  4 files changed, 5 insertions(+), 4 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 0f2e24f13c2b..eed3ce139a32 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..5594dd38ab04 100644
>> --- a/security/integrity/ima/ima_mok.c
>> +++ b/security/integrity/ima/ima_mok.c
>> @@ -38,13 +38,11 @@ __init int ima_mok_init(void)
>>  				(KEY_POS_ALL & ~KEY_POS_SETATTR) |
>>  				KEY_USR_VIEW | KEY_USR_READ |
>>  				KEY_USR_WRITE | KEY_USR_SEARCH,
>> -				KEY_ALLOC_NOT_IN_QUOTA,
>> +				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 ebe752b137aa..c45afdd1dfbb 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;
>> -- 
>> 2.30.0
>>
> 
> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> /Jarkko
>
Jarkko Sakkinen Jan. 20, 2021, 11:45 p.m. UTC | #3 
On Wed, Jan 20, 2021 at 12:15:10PM +0100, Mickaël Salaün wrote:
> 
> On 20/01/2021 04:55, Jarkko Sakkinen wrote:
> > On Thu, Jan 14, 2021 at 04:19:03PM +0100, Mickaël Salaün wrote:
> >> From: David Howells <dhowells@redhat.com>
> >>
> >> 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().
> > 
> > OK, so thing work by luck now, but given the new patches which allow
> > to append new keys they would break, right?
> 
> Without this fix, patch 9/10 would allow to remove and modify keys from
> the blacklist keyring.
> 
> > 
> >> We can also use this in ima_mok_init() rather than setting the flag
> >> manually.
> > 
> > What does ima_mok_init() do?
> 
> This was initially an addition from David Howells, I only fixed the
> argument bit-ORing. ima_mok_init() allocates a blacklist keyring (with
> different properties) dedicated to IMA.
 
Please add this to the commit message.

/Jarkko
diff mbox series

Patch

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 0f2e24f13c2b..eed3ce139a32 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..5594dd38ab04 100644
--- a/security/integrity/ima/ima_mok.c
+++ b/security/integrity/ima/ima_mok.c
@@ -38,13 +38,11 @@  __init int ima_mok_init(void)
 				(KEY_POS_ALL & ~KEY_POS_SETATTR) |
 				KEY_USR_VIEW | KEY_USR_READ |
 				KEY_USR_WRITE | KEY_USR_SEARCH,
-				KEY_ALLOC_NOT_IN_QUOTA,
+				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 ebe752b137aa..c45afdd1dfbb 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;