Hi,
This new patch series is a rebase on v5.14-rc1 . David or Jarkko, if
it's still OK with you, could you please push this to linux-next?
I successfully tested this patch series with the 211 entries from
https://uefi.org/sites/default/files/resources/dbxupdate_x64.bin
The goal of these patches is to add a new configuration option to enable the
root user to load signed keys in the blacklist keyring. This keyring is useful
to "untrust" certificates or files. Enabling to safely update this keyring
without recompiling the kernel makes it more usable.
Previous patch series:
https://lore.kernel.org/lkml/[email protected]/
Regards,
Mickaël Salaün (5):
tools/certs: Add print-cert-tbs-hash.sh
certs: Check that builtin blacklist hashes are valid
certs: Make blacklist_vet_description() more strict
certs: Factor out the blacklist hash creation
certs: Allow root user to append signed hashes to the blacklist
keyring
MAINTAINERS | 2 +
certs/.gitignore | 1 +
certs/Kconfig | 17 +-
certs/Makefile | 17 +-
certs/blacklist.c | 218 ++++++++++++++----
crypto/asymmetric_keys/x509_public_key.c | 3 +-
include/keys/system_keyring.h | 14 +-
scripts/check-blacklist-hashes.awk | 37 +++
.../platform_certs/keyring_handler.c | 26 +--
tools/certs/print-cert-tbs-hash.sh | 91 ++++++++
10 files changed, 346 insertions(+), 80 deletions(-)
create mode 100755 scripts/check-blacklist-hashes.awk
create mode 100755 tools/certs/print-cert-tbs-hash.sh
base-commit: e73f0f0ee7541171d89f2e2491130c7771ba58d3
--
2.32.0
From: Mickaël Salaün <[email protected]>
Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
to dynamically add new keys to the blacklist keyring. This enables to
invalidate new certificates, either from being loaded in a keyring, or
from being trusted in a PKCS#7 certificate chain. This also enables to
add new file hashes to be denied by the integrity infrastructure.
Being able to untrust a certificate which could have normaly been
trusted is a sensitive operation. This is why adding new hashes to the
blacklist keyring is only allowed when these hashes are signed and
vouched by the builtin trusted keyring. A blacklist hash is stored as a
key description. The PKCS#7 signature of this description must be
provided as the key payload.
Marking a certificate as untrusted should be enforced while the system
is running. It is then forbiden to remove such blacklist keys.
Update blacklist keyring, blacklist key and revoked certificate access rights:
* allows the root user to search for a specific blacklisted hash, which
make sense because the descriptions are already viewable;
* forbids key update (blacklist and asymmetric ones);
* restricts kernel rights on the blacklist keyring to align with the
root user rights.
See help in tools/certs/print-cert-tbs-hash.sh .
Cc: David Howells <[email protected]>
Cc: David Woodhouse <[email protected]>
Cc: Eric Snowberg <[email protected]>
Cc: Jarkko Sakkinen <[email protected]>
Signed-off-by: Mickaël Salaün <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
Changes since v6:
* Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
Load mokx variables into the blacklist keyring").
Changes since v5:
* Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
key rights added to the blacklist keyring by the new
add_key_to_revocation_list(): align with blacklist key rights by
removing KEY_POS_WRITE as a safeguard, and add
KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
restrict_link_for_blacklist() that only allows blacklist key types to
be added to the keyring.
* Change the return code for restrict_link_for_blacklist() from -EPERM
to -EOPNOTSUPP to align with asymmetric key keyrings.
Changes since v3:
* Update commit message for print-cert-tbs-hash.sh .
Changes since v2:
* Add comment for blacklist_key_instantiate().
---
certs/Kconfig | 10 +++++
certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
2 files changed, 85 insertions(+), 21 deletions(-)
diff --git a/certs/Kconfig b/certs/Kconfig
index 0fbe184ceca5..e0e524b7eff9 100644
--- a/certs/Kconfig
+++ b/certs/Kconfig
@@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
containing X.509 certificates to be included in the default blacklist
keyring.
+config SYSTEM_BLACKLIST_AUTH_UPDATE
+ bool "Allow root to add signed blacklist keys"
+ depends on SYSTEM_BLACKLIST_KEYRING
+ depends on SYSTEM_DATA_VERIFICATION
+ help
+ If set, provide the ability to load new blacklist keys at run time if
+ they are signed and vouched by a certificate from the builtin trusted
+ keyring. The PKCS#7 signature of the description is set in the key
+ payload. Blacklist keys cannot be removed.
+
endmenu
diff --git a/certs/blacklist.c b/certs/blacklist.c
index b254c87ceb3a..486ce0dd8e9c 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -15,6 +15,7 @@
#include <linux/err.h>
#include <linux/seq_file.h>
#include <linux/uidgid.h>
+#include <linux/verification.h>
#include <keys/system_keyring.h>
#include "blacklist.h"
#include "common.h"
@@ -26,6 +27,9 @@
*/
#define MAX_HASH_LEN 128
+#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
+ KEY_USR_SEARCH | KEY_USR_VIEW)
+
static const char tbs_prefix[] = "tbs";
static const char bin_prefix[] = "bin";
@@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
return 0;
}
-/*
- * The hash to be blacklisted is expected to be in the description. There will
- * be no payload.
- */
-static int blacklist_preparse(struct key_preparsed_payload *prep)
+static int blacklist_key_instantiate(struct key *key,
+ struct key_preparsed_payload *prep)
{
- if (prep->datalen > 0)
- return -EINVAL;
- return 0;
+#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
+ int err;
+#endif
+
+ /* Sets safe default permissions for keys loaded by user space. */
+ key->perm = BLACKLIST_KEY_PERM;
+
+ /*
+ * Skips the authentication step for builtin hashes, they are not
+ * signed but still trusted.
+ */
+ if (key->flags & (1 << KEY_FLAG_BUILTIN))
+ goto out;
+
+#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
+ /*
+ * Verifies the description's PKCS#7 signature against the builtin
+ * trusted keyring.
+ */
+ err = verify_pkcs7_signature(key->description,
+ strlen(key->description), prep->data, prep->datalen,
+ NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
+ if (err)
+ return err;
+#else
+ /*
+ * It should not be possible to come here because the keyring doesn't
+ * have KEY_USR_WRITE and the only other way to call this function is
+ * for builtin hashes.
+ */
+ WARN_ON_ONCE(1);
+ return -EPERM;
+#endif
+
+out:
+ return generic_key_instantiate(key, prep);
}
-static void blacklist_free_preparse(struct key_preparsed_payload *prep)
+static int blacklist_key_update(struct key *key,
+ struct key_preparsed_payload *prep)
{
+ return -EPERM;
}
static void blacklist_describe(const struct key *key, struct seq_file *m)
@@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
static struct key_type key_type_blacklist = {
.name = "blacklist",
.vet_description = blacklist_vet_description,
- .preparse = blacklist_preparse,
- .free_preparse = blacklist_free_preparse,
- .instantiate = generic_key_instantiate,
+ .instantiate = blacklist_key_instantiate,
+ .update = blacklist_key_update,
.describe = blacklist_describe,
};
@@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
hash,
NULL,
0,
- ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
- KEY_USR_VIEW),
+ BLACKLIST_KEY_PERM,
KEY_ALLOC_NOT_IN_QUOTA |
KEY_ALLOC_BUILT_IN);
if (IS_ERR(key)) {
@@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
NULL,
data,
size,
- ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
- KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
+ KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
+ | KEY_USR_VIEW,
+ KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
+ | KEY_ALLOC_BYPASS_RESTRICTION);
if (IS_ERR(key)) {
pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
@@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
}
#endif
+static int restrict_link_for_blacklist(struct key *dest_keyring,
+ const struct key_type *type, const union key_payload *payload,
+ struct key *restrict_key)
+{
+ if (type == &key_type_blacklist)
+ return 0;
+ return -EOPNOTSUPP;
+}
+
/*
* Initialise the blacklist
*/
static int __init blacklist_init(void)
{
const char *const *bl;
+ struct key_restriction *restriction;
if (register_key_type(&key_type_blacklist) < 0)
panic("Can't allocate system blacklist key type\n");
+ restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
+ if (!restriction)
+ panic("Can't allocate blacklist keyring restriction\n");
+ restriction->check = restrict_link_for_blacklist;
+
blacklist_keyring =
keyring_alloc(".blacklist",
GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(),
- (KEY_POS_ALL & ~KEY_POS_SETATTR) |
- KEY_USR_VIEW | KEY_USR_READ |
- KEY_USR_SEARCH,
- KEY_ALLOC_NOT_IN_QUOTA |
+ KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH |
+ KEY_POS_WRITE |
+ KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH
+#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
+ | KEY_USR_WRITE
+#endif
+ , KEY_ALLOC_NOT_IN_QUOTA |
KEY_ALLOC_SET_KEEP,
- NULL, NULL);
+ restriction, NULL);
if (IS_ERR(blacklist_keyring))
panic("Can't allocate system blacklist keyring\n");
--
2.32.0
Hi Jarkko,
Since everyone seems OK with this and had plenty of time to complain,
could you please take this patch series in your tree? It still applies
on v5.16-rc5 and it is really important to us. Please let me know if you
need something more.
Regards,
Mickaël
On 12/07/2021 19:03, Mickaël Salaün wrote:
> Hi,
>
> This new patch series is a rebase on v5.14-rc1 . David or Jarkko, if
> it's still OK with you, could you please push this to linux-next?
>
> I successfully tested this patch series with the 211 entries from
> https://uefi.org/sites/default/files/resources/dbxupdate_x64.bin
>
> The goal of these patches is to add a new configuration option to enable the
> root user to load signed keys in the blacklist keyring. This keyring is useful
> to "untrust" certificates or files. Enabling to safely update this keyring
> without recompiling the kernel makes it more usable.
>
> Previous patch series:
> https://lore.kernel.org/lkml/[email protected]/
>
> Regards,
>
> Mickaël Salaün (5):
> tools/certs: Add print-cert-tbs-hash.sh
> certs: Check that builtin blacklist hashes are valid
> certs: Make blacklist_vet_description() more strict
> certs: Factor out the blacklist hash creation
> certs: Allow root user to append signed hashes to the blacklist
> keyring
>
> MAINTAINERS | 2 +
> certs/.gitignore | 1 +
> certs/Kconfig | 17 +-
> certs/Makefile | 17 +-
> certs/blacklist.c | 218 ++++++++++++++----
> crypto/asymmetric_keys/x509_public_key.c | 3 +-
> include/keys/system_keyring.h | 14 +-
> scripts/check-blacklist-hashes.awk | 37 +++
> .../platform_certs/keyring_handler.c | 26 +--
> tools/certs/print-cert-tbs-hash.sh | 91 ++++++++
> 10 files changed, 346 insertions(+), 80 deletions(-)
> create mode 100755 scripts/check-blacklist-hashes.awk
> create mode 100755 tools/certs/print-cert-tbs-hash.sh
>
>
> base-commit: e73f0f0ee7541171d89f2e2491130c7771ba58d3
>
On Mon, Dec 13, 2021 at 04:30:29PM +0100, Micka?l Sala?n wrote:
> Hi Jarkko,
>
> Since everyone seems OK with this and had plenty of time to complain, could
> you please take this patch series in your tree? It still applies on
> v5.16-rc5 and it is really important to us. Please let me know if you need
> something more.
>
> Regards,
> Micka?l
I'm off-work up until end of the year, i.e. I will address only important
bug fixes and v5.16 up until that.
If any of the patches is yet missing my ack, feel free to
Acked-by: Jarkko Sakkinen <[email protected]>
/Jarkko
On 21/12/2021 09:50, Jarkko Sakkinen wrote:
> On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
>> Hi Jarkko,
>>
>> Since everyone seems OK with this and had plenty of time to complain, could
>> you please take this patch series in your tree? It still applies on
>> v5.16-rc5 and it is really important to us. Please let me know if you need
>> something more.
>>
>> Regards,
>> Mickaël
>
> I'm off-work up until end of the year, i.e. I will address only important
> bug fixes and v5.16 up until that.
>
> If any of the patches is yet missing my ack, feel free to
>
> Acked-by: Jarkko Sakkinen <[email protected]>
Thanks Jarkko. Can you please take it into your tree?
Regards,
Mickaël
On Tue, Jan 04, 2022 at 04:56:36PM +0100, Micka?l Sala?n wrote:
>
> On 21/12/2021 09:50, Jarkko Sakkinen wrote:
> > On Mon, Dec 13, 2021 at 04:30:29PM +0100, Micka?l Sala?n wrote:
> > > Hi Jarkko,
> > >
> > > Since everyone seems OK with this and had plenty of time to complain, could
> > > you please take this patch series in your tree? It still applies on
> > > v5.16-rc5 and it is really important to us. Please let me know if you need
> > > something more.
> > >
> > > Regards,
> > > Micka?l
> >
> > I'm off-work up until end of the year, i.e. I will address only important
> > bug fixes and v5.16 up until that.
> >
> > If any of the patches is yet missing my ack, feel free to
> >
> > Acked-by: Jarkko Sakkinen <[email protected]>
>
> Thanks Jarkko. Can you please take it into your tree?
I can yes, as I need to anyway do a revised PR for v5.17, as one commit
in my first trial had a truncated fixes tag.
/Jarkko
On Thu, Jan 06, 2022 at 09:12:22PM +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 04, 2022 at 04:56:36PM +0100, Micka?l Sala?n wrote:
> >
> > On 21/12/2021 09:50, Jarkko Sakkinen wrote:
> > > On Mon, Dec 13, 2021 at 04:30:29PM +0100, Micka?l Sala?n wrote:
> > > > Hi Jarkko,
> > > >
> > > > Since everyone seems OK with this and had plenty of time to complain, could
> > > > you please take this patch series in your tree? It still applies on
> > > > v5.16-rc5 and it is really important to us. Please let me know if you need
> > > > something more.
> > > >
> > > > Regards,
> > > > Micka?l
> > >
> > > I'm off-work up until end of the year, i.e. I will address only important
> > > bug fixes and v5.16 up until that.
> > >
> > > If any of the patches is yet missing my ack, feel free to
> > >
> > > Acked-by: Jarkko Sakkinen <[email protected]>
> >
> > Thanks Jarkko. Can you please take it into your tree?
>
> I can yes, as I need to anyway do a revised PR for v5.17, as one commit
> in my first trial had a truncated fixes tag.
Please check:
git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git
/Jarkko
On 06/01/2022 20:16, Jarkko Sakkinen wrote:
> On Thu, Jan 06, 2022 at 09:12:22PM +0200, Jarkko Sakkinen wrote:
>> On Tue, Jan 04, 2022 at 04:56:36PM +0100, Mickaël Salaün wrote:
>>>
>>> On 21/12/2021 09:50, Jarkko Sakkinen wrote:
>>>> On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
>>>>> Hi Jarkko,
>>>>>
>>>>> Since everyone seems OK with this and had plenty of time to complain, could
>>>>> you please take this patch series in your tree? It still applies on
>>>>> v5.16-rc5 and it is really important to us. Please let me know if you need
>>>>> something more.
>>>>>
>>>>> Regards,
>>>>> Mickaël
>>>>
>>>> I'm off-work up until end of the year, i.e. I will address only important
>>>> bug fixes and v5.16 up until that.
>>>>
>>>> If any of the patches is yet missing my ack, feel free to
>>>>
>>>> Acked-by: Jarkko Sakkinen <[email protected]>
>>>
>>> Thanks Jarkko. Can you please take it into your tree?
>>
>> I can yes, as I need to anyway do a revised PR for v5.17, as one commit
>> in my first trial had a truncated fixes tag.
>
> Please check:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git
>
> /Jarkko
Great, thanks!
On 09/03/2022 17:01, Jarkko Sakkinen wrote:
> On Tue, Mar 08, 2022 at 05:02:23PM +0100, Mickaël Salaün wrote:
>>
>> On 08/03/2022 14:19, Jarkko Sakkinen wrote:
>>> On Tue, Mar 08, 2022 at 01:18:28PM +0100, Mickaël Salaün wrote:
>>>>
>>>> On 08/03/2022 12:53, Jarkko Sakkinen wrote:
>>>>> On Mon, Jul 12, 2021 at 07:03:13PM +0200, Mickaël Salaün wrote:
>>>>>> From: Mickaël Salaün <[email protected]>
>>>>>>
>>>>>> Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
>>>>>> to dynamically add new keys to the blacklist keyring. This enables to
>>>>>> invalidate new certificates, either from being loaded in a keyring, or
>>>>>> from being trusted in a PKCS#7 certificate chain. This also enables to
>>>>>> add new file hashes to be denied by the integrity infrastructure.
>>>>>>
>>>>>> Being able to untrust a certificate which could have normaly been
>>>>>> trusted is a sensitive operation. This is why adding new hashes to the
>>>>>> blacklist keyring is only allowed when these hashes are signed and
>>>>>> vouched by the builtin trusted keyring. A blacklist hash is stored as a
>>>>>> key description. The PKCS#7 signature of this description must be
>>>>>> provided as the key payload.
>>>>>>
>>>>>> Marking a certificate as untrusted should be enforced while the system
>>>>>> is running. It is then forbiden to remove such blacklist keys.
>>>>>>
>>>>>> Update blacklist keyring, blacklist key and revoked certificate access rights:
>>>>>> * allows the root user to search for a specific blacklisted hash, which
>>>>>> make sense because the descriptions are already viewable;
>>>>>> * forbids key update (blacklist and asymmetric ones);
>>>>>> * restricts kernel rights on the blacklist keyring to align with the
>>>>>> root user rights.
>>>>>>
>>>>>> See help in tools/certs/print-cert-tbs-hash.sh .
>>>>>>
>>>>>> Cc: David Howells <[email protected]>
>>>>>> Cc: David Woodhouse <[email protected]>
>>>>>> Cc: Eric Snowberg <[email protected]>
>>>>>> Cc: Jarkko Sakkinen <[email protected]>
>>>>>> Signed-off-by: Mickaël Salaün <[email protected]>
>>>>>> Link: https://lore.kernel.org/r/[email protected]
>>>>>> ---
>>>>>>
>>>>>> Changes since v6:
>>>>>> * Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
>>>>>> Load mokx variables into the blacklist keyring").
>>>>>>
>>>>>> Changes since v5:
>>>>>> * Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
>>>>>> key rights added to the blacklist keyring by the new
>>>>>> add_key_to_revocation_list(): align with blacklist key rights by
>>>>>> removing KEY_POS_WRITE as a safeguard, and add
>>>>>> KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
>>>>>> restrict_link_for_blacklist() that only allows blacklist key types to
>>>>>> be added to the keyring.
>>>>>> * Change the return code for restrict_link_for_blacklist() from -EPERM
>>>>>> to -EOPNOTSUPP to align with asymmetric key keyrings.
>>>>>>
>>>>>> Changes since v3:
>>>>>> * Update commit message for print-cert-tbs-hash.sh .
>>>>>>
>>>>>> Changes since v2:
>>>>>> * Add comment for blacklist_key_instantiate().
>>>>>> ---
>>>>>> certs/Kconfig | 10 +++++
>>>>>> certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
>>>>>> 2 files changed, 85 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/certs/Kconfig b/certs/Kconfig
>>>>>> index 0fbe184ceca5..e0e524b7eff9 100644
>>>>>> --- a/certs/Kconfig
>>>>>> +++ b/certs/Kconfig
>>>>>> @@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
>>>>>> containing X.509 certificates to be included in the default blacklist
>>>>>> keyring.
>>>>>> +config SYSTEM_BLACKLIST_AUTH_UPDATE
>>>>>> + bool "Allow root to add signed blacklist keys"
>>>>>> + depends on SYSTEM_BLACKLIST_KEYRING
>>>>>> + depends on SYSTEM_DATA_VERIFICATION
>>>>>> + help
>>>>>> + If set, provide the ability to load new blacklist keys at run time if
>>>>>> + they are signed and vouched by a certificate from the builtin trusted
>>>>>> + keyring. The PKCS#7 signature of the description is set in the key
>>>>>> + payload. Blacklist keys cannot be removed.
>>>>>> +
>>>>>> endmenu
>>>>>> diff --git a/certs/blacklist.c b/certs/blacklist.c
>>>>>> index b254c87ceb3a..486ce0dd8e9c 100644
>>>>>> --- a/certs/blacklist.c
>>>>>> +++ b/certs/blacklist.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>> #include <linux/err.h>
>>>>>> #include <linux/seq_file.h>
>>>>>> #include <linux/uidgid.h>
>>>>>> +#include <linux/verification.h>
>>>>>> #include <keys/system_keyring.h>
>>>>>> #include "blacklist.h"
>>>>>> #include "common.h"
>>>>>> @@ -26,6 +27,9 @@
>>>>>> */
>>>>>> #define MAX_HASH_LEN 128
>>>>>> +#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
>>>>>> + KEY_USR_SEARCH | KEY_USR_VIEW)
>>>>>> +
>>>>>> static const char tbs_prefix[] = "tbs";
>>>>>> static const char bin_prefix[] = "bin";
>>>>>> @@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
>>>>>> return 0;
>>>>>> }
>>>>>> -/*
>>>>>> - * The hash to be blacklisted is expected to be in the description. There will
>>>>>> - * be no payload.
>>>>>> - */
>>>>>> -static int blacklist_preparse(struct key_preparsed_payload *prep)
>>>>>> +static int blacklist_key_instantiate(struct key *key,
>>>>>> + struct key_preparsed_payload *prep)
>>>>>> {
>>>>>> - if (prep->datalen > 0)
>>>>>> - return -EINVAL;
>>>>>> - return 0;
>>>>>> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
>>>>>> + int err;
>>>>>> +#endif
>>>>>> +
>>>>>> + /* Sets safe default permissions for keys loaded by user space. */
>>>>>> + key->perm = BLACKLIST_KEY_PERM;
>>>>>> +
>>>>>> + /*
>>>>>> + * Skips the authentication step for builtin hashes, they are not
>>>>>> + * signed but still trusted.
>>>>>> + */
>>>>>> + if (key->flags & (1 << KEY_FLAG_BUILTIN))
>>>>>> + goto out;
>>>>>> +
>>>>>> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
>>>>>> + /*
>>>>>> + * Verifies the description's PKCS#7 signature against the builtin
>>>>>> + * trusted keyring.
>>>>>> + */
>>>>>> + err = verify_pkcs7_signature(key->description,
>>>>>> + strlen(key->description), prep->data, prep->datalen,
>>>>>> + NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
>>>>>> + if (err)
>>>>>> + return err;
>>>>>> +#else
>>>>>> + /*
>>>>>> + * It should not be possible to come here because the keyring doesn't
>>>>>> + * have KEY_USR_WRITE and the only other way to call this function is
>>>>>> + * for builtin hashes.
>>>>>> + */
>>>>>> + WARN_ON_ONCE(1);
>>>>>> + return -EPERM;
>>>>>> +#endif
>>>>>> +
>>>>>> +out:
>>>>>> + return generic_key_instantiate(key, prep);
>>>>>> }
>>>>>> -static void blacklist_free_preparse(struct key_preparsed_payload *prep)
>>>>>> +static int blacklist_key_update(struct key *key,
>>>>>> + struct key_preparsed_payload *prep)
>>>>>> {
>>>>>> + return -EPERM;
>>>>>> }
>>>>>> static void blacklist_describe(const struct key *key, struct seq_file *m)
>>>>>> @@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
>>>>>> static struct key_type key_type_blacklist = {
>>>>>> .name = "blacklist",
>>>>>> .vet_description = blacklist_vet_description,
>>>>>> - .preparse = blacklist_preparse,
>>>>>> - .free_preparse = blacklist_free_preparse,
>>>>>> - .instantiate = generic_key_instantiate,
>>>>>> + .instantiate = blacklist_key_instantiate,
>>>>>> + .update = blacklist_key_update,
>>>>>> .describe = blacklist_describe,
>>>>>> };
>>>>>> @@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
>>>>>> hash,
>>>>>> NULL,
>>>>>> 0,
>>>>>> - ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
>>>>>> - KEY_USR_VIEW),
>>>>>> + BLACKLIST_KEY_PERM,
>>>>>> KEY_ALLOC_NOT_IN_QUOTA |
>>>>>> KEY_ALLOC_BUILT_IN);
>>>>>> if (IS_ERR(key)) {
>>>>>> @@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
>>>>>> NULL,
>>>>>> data,
>>>>>> size,
>>>>>> - ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
>>>>>> - KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
>>>>>> + KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
>>>>>> + | KEY_USR_VIEW,
>>>>>> + KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
>>>>>> + | KEY_ALLOC_BYPASS_RESTRICTION);
>>>>>> if (IS_ERR(key)) {
>>>>>> pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
>>>>>> @@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
>>>>>> }
>>>>>> #endif
>>>>>> +static int restrict_link_for_blacklist(struct key *dest_keyring,
>>>>>> + const struct key_type *type, const union key_payload *payload,
>>>>>> + struct key *restrict_key)
>>>>>> +{
>>>>>> + if (type == &key_type_blacklist)
>>>>>> + return 0;
>>>>>> + return -EOPNOTSUPP;
>>>>>> +}
>>>>>> +
>>>>>> /*
>>>>>> * Initialise the blacklist
>>>>>> */
>>>>>> static int __init blacklist_init(void)
>>>>>> {
>>>>>> const char *const *bl;
>>>>>> + struct key_restriction *restriction;
>>>>>> if (register_key_type(&key_type_blacklist) < 0)
>>>>>> panic("Can't allocate system blacklist key type\n");
>>>>>> + restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
>>>>>> + if (!restriction)
>>>>>> + panic("Can't allocate blacklist keyring restriction\n");
>>>>>
>>>>>
>>>>> This prevents me from taking this to my pull request. In moderns standards,
>>>>> no new BUG_ON(), panic() etc. should never added to the kernel.
>>>>>
>>>>> I missed this in my review.
>>>>>
>>>>> This should rather be e.g.
>>>>>
>>>>> restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
>>>>> if (!restriction) {
>>>>> pr_err("Can't allocate blacklist keyring restriction\n");
>>>>> return 0;
>>>>> }
>>>>>
>>>>> Unfortunately I need to drop this patch set, because adding new panic()
>>>>> is simply a no-go.
>>>>
>>>> I agree that panic() is not great in general, but I followed the other part
>>>> of the code (just above) that do the same. This part of the kernel should
>>>> failed if critical memory allocation failed at boot time (only). It doesn't
>>>> impact the kernel once it is running. I don't think that just ignoring this
>>>> error with return 0 is fine, after all it's a critical error right?
>>>
>>> It's not good reason enough to crash the whole kernel, even if it is a
>>> critical error (e.g. run-time foresincs). Even WARN() is not recommended
>>> these days [*].
>>
>> I think that what Greg said in this email is that WARN*() should only be
>> used for cases that should never happen, it is definitely not deprecated,
>> but WARN_ON_ONCE() may be a better idea though. WARN*() helps detect such
>> thought-to-be-impossible cases, that can happen e.g. with code refactoring.
>>
>> A lot of initialization/boot code (e.g. without user space nor external
>> interactions, mostly __init functions) do panic if there is unexpected and
>> unrecoverable errors like failed memory allocations. I think handling such
>> errors otherwise would be more complex for no benefit. Moreover, delegating
>> such error handling to user space could create new (silent) issues.
>
> To crash the whole kernel, you should be able to clearly explain why it
> makes sense in the situation.
If there is no enough memory to allocate 24 bytes (struct
key_restriction) during early boot, I really doubt the kernel can do much.
>
>>>
>>> For the existing panic()-statements: I'm happy to review patches that
>>> render them out. >
>>> Not sure tho, if this fails should it be then "everything blacklisted".
>>> Just one thing to consider.
>>
>> Well, if it fail it will be "nothing will work afterwards". Do you have a
>> working and useful scenario for this kind of error?
>
> So you have zero chances to get a shell without blacklist just to do
> kernel forensics?
Right, I don't think the kernel can launch any process (nor continue
early boot) if it cannot allocate 24 bytes.
On Wed, Mar 09, 2022 at 07:36:50PM +0100, Micka?l Sala?n wrote:
>
> On 09/03/2022 17:01, Jarkko Sakkinen wrote:
> > On Tue, Mar 08, 2022 at 05:02:23PM +0100, Micka?l Sala?n wrote:
> > >
> > > On 08/03/2022 14:19, Jarkko Sakkinen wrote:
> > > > On Tue, Mar 08, 2022 at 01:18:28PM +0100, Micka?l Sala?n wrote:
> > > > >
> > > > > On 08/03/2022 12:53, Jarkko Sakkinen wrote:
> > > > > > On Mon, Jul 12, 2021 at 07:03:13PM +0200, Micka?l Sala?n wrote:
> > > > > > > From: Micka?l Sala?n <[email protected]>
> > > > > > >
> > > > > > > Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
> > > > > > > to dynamically add new keys to the blacklist keyring. This enables to
> > > > > > > invalidate new certificates, either from being loaded in a keyring, or
> > > > > > > from being trusted in a PKCS#7 certificate chain. This also enables to
> > > > > > > add new file hashes to be denied by the integrity infrastructure.
> > > > > > >
> > > > > > > Being able to untrust a certificate which could have normaly been
> > > > > > > trusted is a sensitive operation. This is why adding new hashes to the
> > > > > > > blacklist keyring is only allowed when these hashes are signed and
> > > > > > > vouched by the builtin trusted keyring. A blacklist hash is stored as a
> > > > > > > key description. The PKCS#7 signature of this description must be
> > > > > > > provided as the key payload.
> > > > > > >
> > > > > > > Marking a certificate as untrusted should be enforced while the system
> > > > > > > is running. It is then forbiden to remove such blacklist keys.
> > > > > > >
> > > > > > > Update blacklist keyring, blacklist key and revoked certificate access rights:
> > > > > > > * allows the root user to search for a specific blacklisted hash, which
> > > > > > > make sense because the descriptions are already viewable;
> > > > > > > * forbids key update (blacklist and asymmetric ones);
> > > > > > > * restricts kernel rights on the blacklist keyring to align with the
> > > > > > > root user rights.
> > > > > > >
> > > > > > > See help in tools/certs/print-cert-tbs-hash.sh .
> > > > > > >
> > > > > > > Cc: David Howells <[email protected]>
> > > > > > > Cc: David Woodhouse <[email protected]>
> > > > > > > Cc: Eric Snowberg <[email protected]>
> > > > > > > Cc: Jarkko Sakkinen <[email protected]>
> > > > > > > Signed-off-by: Micka?l Sala?n <[email protected]>
> > > > > > > Link: https://lore.kernel.org/r/[email protected]
> > > > > > > ---
> > > > > > >
> > > > > > > Changes since v6:
> > > > > > > * Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
> > > > > > > Load mokx variables into the blacklist keyring").
> > > > > > >
> > > > > > > Changes since v5:
> > > > > > > * Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
> > > > > > > key rights added to the blacklist keyring by the new
> > > > > > > add_key_to_revocation_list(): align with blacklist key rights by
> > > > > > > removing KEY_POS_WRITE as a safeguard, and add
> > > > > > > KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
> > > > > > > restrict_link_for_blacklist() that only allows blacklist key types to
> > > > > > > be added to the keyring.
> > > > > > > * Change the return code for restrict_link_for_blacklist() from -EPERM
> > > > > > > to -EOPNOTSUPP to align with asymmetric key keyrings.
> > > > > > >
> > > > > > > Changes since v3:
> > > > > > > * Update commit message for print-cert-tbs-hash.sh .
> > > > > > >
> > > > > > > Changes since v2:
> > > > > > > * Add comment for blacklist_key_instantiate().
> > > > > > > ---
> > > > > > > certs/Kconfig | 10 +++++
> > > > > > > certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
> > > > > > > 2 files changed, 85 insertions(+), 21 deletions(-)
> > > > > > >
> > > > > > > diff --git a/certs/Kconfig b/certs/Kconfig
> > > > > > > index 0fbe184ceca5..e0e524b7eff9 100644
> > > > > > > --- a/certs/Kconfig
> > > > > > > +++ b/certs/Kconfig
> > > > > > > @@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
> > > > > > > containing X.509 certificates to be included in the default blacklist
> > > > > > > keyring.
> > > > > > > +config SYSTEM_BLACKLIST_AUTH_UPDATE
> > > > > > > + bool "Allow root to add signed blacklist keys"
> > > > > > > + depends on SYSTEM_BLACKLIST_KEYRING
> > > > > > > + depends on SYSTEM_DATA_VERIFICATION
> > > > > > > + help
> > > > > > > + If set, provide the ability to load new blacklist keys at run time if
> > > > > > > + they are signed and vouched by a certificate from the builtin trusted
> > > > > > > + keyring. The PKCS#7 signature of the description is set in the key
> > > > > > > + payload. Blacklist keys cannot be removed.
> > > > > > > +
> > > > > > > endmenu
> > > > > > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > > > > > index b254c87ceb3a..486ce0dd8e9c 100644
> > > > > > > --- a/certs/blacklist.c
> > > > > > > +++ b/certs/blacklist.c
> > > > > > > @@ -15,6 +15,7 @@
> > > > > > > #include <linux/err.h>
> > > > > > > #include <linux/seq_file.h>
> > > > > > > #include <linux/uidgid.h>
> > > > > > > +#include <linux/verification.h>
> > > > > > > #include <keys/system_keyring.h>
> > > > > > > #include "blacklist.h"
> > > > > > > #include "common.h"
> > > > > > > @@ -26,6 +27,9 @@
> > > > > > > */
> > > > > > > #define MAX_HASH_LEN 128
> > > > > > > +#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
> > > > > > > + KEY_USR_SEARCH | KEY_USR_VIEW)
> > > > > > > +
> > > > > > > static const char tbs_prefix[] = "tbs";
> > > > > > > static const char bin_prefix[] = "bin";
> > > > > > > @@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
> > > > > > > return 0;
> > > > > > > }
> > > > > > > -/*
> > > > > > > - * The hash to be blacklisted is expected to be in the description. There will
> > > > > > > - * be no payload.
> > > > > > > - */
> > > > > > > -static int blacklist_preparse(struct key_preparsed_payload *prep)
> > > > > > > +static int blacklist_key_instantiate(struct key *key,
> > > > > > > + struct key_preparsed_payload *prep)
> > > > > > > {
> > > > > > > - if (prep->datalen > 0)
> > > > > > > - return -EINVAL;
> > > > > > > - return 0;
> > > > > > > +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> > > > > > > + int err;
> > > > > > > +#endif
> > > > > > > +
> > > > > > > + /* Sets safe default permissions for keys loaded by user space. */
> > > > > > > + key->perm = BLACKLIST_KEY_PERM;
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * Skips the authentication step for builtin hashes, they are not
> > > > > > > + * signed but still trusted.
> > > > > > > + */
> > > > > > > + if (key->flags & (1 << KEY_FLAG_BUILTIN))
> > > > > > > + goto out;
> > > > > > > +
> > > > > > > +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> > > > > > > + /*
> > > > > > > + * Verifies the description's PKCS#7 signature against the builtin
> > > > > > > + * trusted keyring.
> > > > > > > + */
> > > > > > > + err = verify_pkcs7_signature(key->description,
> > > > > > > + strlen(key->description), prep->data, prep->datalen,
> > > > > > > + NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> > > > > > > + if (err)
> > > > > > > + return err;
> > > > > > > +#else
> > > > > > > + /*
> > > > > > > + * It should not be possible to come here because the keyring doesn't
> > > > > > > + * have KEY_USR_WRITE and the only other way to call this function is
> > > > > > > + * for builtin hashes.
> > > > > > > + */
> > > > > > > + WARN_ON_ONCE(1);
> > > > > > > + return -EPERM;
> > > > > > > +#endif
> > > > > > > +
> > > > > > > +out:
> > > > > > > + return generic_key_instantiate(key, prep);
> > > > > > > }
> > > > > > > -static void blacklist_free_preparse(struct key_preparsed_payload *prep)
> > > > > > > +static int blacklist_key_update(struct key *key,
> > > > > > > + struct key_preparsed_payload *prep)
> > > > > > > {
> > > > > > > + return -EPERM;
> > > > > > > }
> > > > > > > static void blacklist_describe(const struct key *key, struct seq_file *m)
> > > > > > > @@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
> > > > > > > static struct key_type key_type_blacklist = {
> > > > > > > .name = "blacklist",
> > > > > > > .vet_description = blacklist_vet_description,
> > > > > > > - .preparse = blacklist_preparse,
> > > > > > > - .free_preparse = blacklist_free_preparse,
> > > > > > > - .instantiate = generic_key_instantiate,
> > > > > > > + .instantiate = blacklist_key_instantiate,
> > > > > > > + .update = blacklist_key_update,
> > > > > > > .describe = blacklist_describe,
> > > > > > > };
> > > > > > > @@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
> > > > > > > hash,
> > > > > > > NULL,
> > > > > > > 0,
> > > > > > > - ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> > > > > > > - KEY_USR_VIEW),
> > > > > > > + BLACKLIST_KEY_PERM,
> > > > > > > KEY_ALLOC_NOT_IN_QUOTA |
> > > > > > > KEY_ALLOC_BUILT_IN);
> > > > > > > if (IS_ERR(key)) {
> > > > > > > @@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
> > > > > > > NULL,
> > > > > > > data,
> > > > > > > size,
> > > > > > > - ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
> > > > > > > - KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
> > > > > > > + KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
> > > > > > > + | KEY_USR_VIEW,
> > > > > > > + KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
> > > > > > > + | KEY_ALLOC_BYPASS_RESTRICTION);
> > > > > > > if (IS_ERR(key)) {
> > > > > > > pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
> > > > > > > @@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
> > > > > > > }
> > > > > > > #endif
> > > > > > > +static int restrict_link_for_blacklist(struct key *dest_keyring,
> > > > > > > + const struct key_type *type, const union key_payload *payload,
> > > > > > > + struct key *restrict_key)
> > > > > > > +{
> > > > > > > + if (type == &key_type_blacklist)
> > > > > > > + return 0;
> > > > > > > + return -EOPNOTSUPP;
> > > > > > > +}
> > > > > > > +
> > > > > > > /*
> > > > > > > * Initialise the blacklist
> > > > > > > */
> > > > > > > static int __init blacklist_init(void)
> > > > > > > {
> > > > > > > const char *const *bl;
> > > > > > > + struct key_restriction *restriction;
> > > > > > > if (register_key_type(&key_type_blacklist) < 0)
> > > > > > > panic("Can't allocate system blacklist key type\n");
> > > > > > > + restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > > > > > > + if (!restriction)
> > > > > > > + panic("Can't allocate blacklist keyring restriction\n");
> > > > > >
> > > > > >
> > > > > > This prevents me from taking this to my pull request. In moderns standards,
> > > > > > no new BUG_ON(), panic() etc. should never added to the kernel.
> > > > > >
> > > > > > I missed this in my review.
> > > > > >
> > > > > > This should rather be e.g.
> > > > > >
> > > > > > restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > > > > > if (!restriction) {
> > > > > > pr_err("Can't allocate blacklist keyring restriction\n");
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > Unfortunately I need to drop this patch set, because adding new panic()
> > > > > > is simply a no-go.
> > > > >
> > > > > I agree that panic() is not great in general, but I followed the other part
> > > > > of the code (just above) that do the same. This part of the kernel should
> > > > > failed if critical memory allocation failed at boot time (only). It doesn't
> > > > > impact the kernel once it is running. I don't think that just ignoring this
> > > > > error with return 0 is fine, after all it's a critical error right?
> > > >
> > > > It's not good reason enough to crash the whole kernel, even if it is a
> > > > critical error (e.g. run-time foresincs). Even WARN() is not recommended
> > > > these days [*].
> > >
> > > I think that what Greg said in this email is that WARN*() should only be
> > > used for cases that should never happen, it is definitely not deprecated,
> > > but WARN_ON_ONCE() may be a better idea though. WARN*() helps detect such
> > > thought-to-be-impossible cases, that can happen e.g. with code refactoring.
> > >
> > > A lot of initialization/boot code (e.g. without user space nor external
> > > interactions, mostly __init functions) do panic if there is unexpected and
> > > unrecoverable errors like failed memory allocations. I think handling such
> > > errors otherwise would be more complex for no benefit. Moreover, delegating
> > > such error handling to user space could create new (silent) issues.
> >
> > To crash the whole kernel, you should be able to clearly explain why it
> > makes sense in the situation.
>
> If there is no enough memory to allocate 24 bytes (struct key_restriction)
> during early boot, I really doubt the kernel can do much.
>
> >
> > > >
> > > > For the existing panic()-statements: I'm happy to review patches that
> > > > render them out. >
> > > > Not sure tho, if this fails should it be then "everything blacklisted".
> > > > Just one thing to consider.
> > >
> > > Well, if it fail it will be "nothing will work afterwards". Do you have a
> > > working and useful scenario for this kind of error?
> >
> > So you have zero chances to get a shell without blacklist just to do
> > kernel forensics?
>
> Right, I don't think the kernel can launch any process (nor continue early
> boot) if it cannot allocate 24 bytes.
initcall is just wrong layer to choose to crash the kernel. It should be
either do_initcall_level() or do_one_initcall() that should care about
this (or not care). You can print error message and return -ENODEV;
BR, Jarkko