[PATCH 0/5 v2] Using the hash in MOKx to blacklist kernel module

[PATCH 0/5 v2] Using the hash in MOKx to blacklist kernel module

[Lee, Chun-Yi]

This patch set is base on the efi-lock-down and keys-uefi branchs in
David Howells's linux-fs git tree.
    https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-uefi

The main purpose is using the MOKx to blacklist kernel module.

As the MOK (Machine Owner Key), MOKx is a EFI boot time variable which
is maintained by shim boot loader. We can enroll the hash of blacklisted
kernel module (with or without signature) to MOKx by mokutil. Kernel loads
the hash from MOKx to blacklist keyring when booting. Kernel will prevent
to load the kernel module when its hash be found in blacklist.

This function is useful to revoke a kernel module that it has exploit. Or
revoking a kernel module that it was signed by a unsecure key.

Except MOKx, this patch set fixs another two issues: The MOK/MOKx should
not be loaded when secure boot is disabled. And, modified error message
prints out appropriate status string for reading by human being.

v2:
Chekcikng the attributes of db and mok before loading certificates.

Lee, Chun-Yi (5):
  MODSIGN: do not load mok when secure boot disabled
  MODSIGN: print appropriate status message when getting UEFI
    certificates list
  MODSIGN: load blacklist from MOKx
  MODSIGN: checking the blacklisted hash before loading a kernel module
  MODSIGN: check the attributes of db and mok

 certs/load_uefi.c       | 92 +++++++++++++++++++++++++++++++++++--------------
 include/linux/efi.h     | 25 ++++++++++++++
 kernel/module_signing.c | 62 +++++++++++++++++++++++++++++++--
 3 files changed, 152 insertions(+), 27 deletions(-)

-- 
2.10.2

[PATCH 1/5] MODSIGN: do not load mok when secure boot disabled

[Lee, Chun-Yi]

The mok can not be trusted when the secure boot is disabled. Which
means that the kernel embedded certificate is the only trusted key.

Due to db/dbx are authenticated variables, they needs manufacturer's
KEK for update. So db/dbx are secure when secureboot disabled.

Cc: David Howells <dhowells@redhat.com>
Cc: Josh Boyer <jwboyer@fedoraproject.org>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
 certs/load_uefi.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/certs/load_uefi.c b/certs/load_uefi.c
index 3d88459..d6de4d0 100644
--- a/certs/load_uefi.c
+++ b/certs/load_uefi.c
@@ -164,17 +164,6 @@ static int __init load_uefi_certs(void)
 		}
 	}
 
-	mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
-	if (!mok) {
-		pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
-	} else {
-		rc = parse_efi_signature_list("UEFI:MokListRT",
-					      mok, moksize, get_handler_for_db);
-		if (rc)
-			pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
-		kfree(mok);
-	}
-
 	dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
 	if (!dbx) {
 		pr_info("MODSIGN: Couldn't get UEFI dbx list\n");
@@ -187,6 +176,21 @@ static int __init load_uefi_certs(void)
 		kfree(dbx);
 	}
 
+	/* the MOK can not be trusted when secure boot is disabled */
+	if (!efi_enabled(EFI_SECURE_BOOT))
+		return 0;
+
+	mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
+	if (!mok) {
+		pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
+	} else {
+		rc = parse_efi_signature_list("UEFI:MokListRT",
+					      mok, moksize, get_handler_for_db);
+		if (rc)
+			pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
+		kfree(mok);
+	}
+
 	return rc;
 }
 late_initcall(load_uefi_certs);
-- 
2.10.2

Re: [PATCH 1/5] MODSIGN: do not load mok when secure boot disabled

[Ard Biesheuvel]

On 13 March 2018 at 10:37, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> The mok can not be trusted when the secure boot is disabled. Which
> means that the kernel embedded certificate is the only trusted key.
>
> Due to db/dbx are authenticated variables, they needs manufacturer's
> KEK for update. So db/dbx are secure when secureboot disabled.
>

Did you consider the case where secure boot is not implemented? I
don't think db/dbx are secure in that case, although perhaps it may
not matter (a bit more information on the purpose of these patches and
all the shim lingo etc would be appreciated)

> Cc: David Howells <dhowells@redhat.com>
> Cc: Josh Boyer <jwboyer@fedoraproject.org>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> ---
>  certs/load_uefi.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/certs/load_uefi.c b/certs/load_uefi.c
> index 3d88459..d6de4d0 100644
> --- a/certs/load_uefi.c
> +++ b/certs/load_uefi.c
> @@ -164,17 +164,6 @@ static int __init load_uefi_certs(void)
>                 }
>         }
>
> -       mok = get_cert_list(L"MokListRT", &mok_var, &moksize);

Which tree does this apply to? My tree doesn't have get_cert_list()

> -       if (!mok) {
> -               pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
> -       } else {
> -               rc = parse_efi_signature_list("UEFI:MokListRT",
> -                                             mok, moksize, get_handler_for_db);
> -               if (rc)
> -                       pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
> -               kfree(mok);
> -       }
> -
>         dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
>         if (!dbx) {
>                 pr_info("MODSIGN: Couldn't get UEFI dbx list\n");
> @@ -187,6 +176,21 @@ static int __init load_uefi_certs(void)
>                 kfree(dbx);
>         }
>
> +       /* the MOK can not be trusted when secure boot is disabled */
> +       if (!efi_enabled(EFI_SECURE_BOOT))
> +               return 0;
> +
> +       mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
> +       if (!mok) {
> +               pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
> +       } else {
> +               rc = parse_efi_signature_list("UEFI:MokListRT",
> +                                             mok, moksize, get_handler_for_db);
> +               if (rc)
> +                       pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
> +               kfree(mok);
> +       }
> +
>         return rc;
>  }
>  late_initcall(load_uefi_certs);
> --
> 2.10.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] MODSIGN: do not load mok when secure boot disabled

[joeyli]

Hi Ard, 

First! Thanks for your review!

On Tue, Mar 13, 2018 at 05:25:30PM +0000, Ard Biesheuvel wrote:
> On 13 March 2018 at 10:37, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > The mok can not be trusted when the secure boot is disabled. Which
> > means that the kernel embedded certificate is the only trusted key.
> >
> > Due to db/dbx are authenticated variables, they needs manufacturer's
> > KEK for update. So db/dbx are secure when secureboot disabled.
> >
> 
> Did you consider the case where secure boot is not implemented? I
> don't think db/dbx are secure in that case, although perhaps it may
> not matter (a bit more information on the purpose of these patches and
> all the shim lingo etc would be appreciated)
> 

The patch 5 in this series checks that the db/dbx must have
EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS attribute. But I agree
with you that kernel should checks the SecureBoot variable must
exist in system. I will add patch to detect it.

> > Cc: David Howells <dhowells@redhat.com>
> > Cc: Josh Boyer <jwboyer@fedoraproject.org>
> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> > ---
> >  certs/load_uefi.c | 26 +++++++++++++++-----------
> >  1 file changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/certs/load_uefi.c b/certs/load_uefi.c
> > index 3d88459..d6de4d0 100644
> > --- a/certs/load_uefi.c
> > +++ b/certs/load_uefi.c
> > @@ -164,17 +164,6 @@ static int __init load_uefi_certs(void)
> >                 }
> >         }
> >
> > -       mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
> 
> Which tree does this apply to? My tree doesn't have get_cert_list()
>

This patch set is base on the efi-lock-down and keys-uefi branchs in
David Howells's linux-fs git tree.
    https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-uefi

Thanks a lot!
Joey Lee 

[PATCH 2/5] MODSIGN: print appropriate status message when getting UEFI certificates list

[Lee, Chun-Yi]

When getting certificates list from UEFI variable, the original error
message shows the state number from UEFI firmware. It's hard to be read
by human. This patch changed the error message to show the appropriate
string.

The message will be showed as:

[    0.788529] MODSIGN: Couldn't get UEFI MokListRT: EFI_NOT_FOUND
[    0.788537] MODSIGN: Couldn't get UEFI MokListXRT: EFI_NOT_FOUND

Cc: David Howells <dhowells@redhat.com>
Cc: Josh Boyer <jwboyer@fedoraproject.org>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
 certs/load_uefi.c   | 43 ++++++++++++++++++++++++++++++-------------
 include/linux/efi.h | 25 +++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 13 deletions(-)

diff --git a/certs/load_uefi.c b/certs/load_uefi.c
index d6de4d0..f2f372b 100644
--- a/certs/load_uefi.c
+++ b/certs/load_uefi.c
@@ -4,6 +4,7 @@
 #include <linux/err.h>
 #include <linux/efi.h>
 #include <linux/slab.h>
+#include <linux/ucs2_string.h>
 #include <keys/asymmetric-type.h>
 #include <keys/system_keyring.h>
 #include "internal.h"
@@ -32,6 +33,24 @@ static __init bool uefi_check_ignore_db(void)
 	return status == EFI_SUCCESS;
 }
 
+static __init void print_get_fail(efi_char16_t *char16_str, efi_status_t status)
+{
+	char *utf8_str;
+	unsigned long utf8_size;
+
+	if (!char16_str)
+		return;
+	utf8_size = ucs2_utf8size(char16_str) + 1;
+	utf8_str = kmalloc(utf8_size, GFP_KERNEL);
+	if (!utf8_str)
+		return;
+	ucs2_as_utf8(utf8_str, char16_str, utf8_size);
+
+	pr_info("MODSIGN: Couldn't get UEFI %s: %s\n",
+		utf8_str, efi_status_to_str(status));
+	kfree(utf8_str);
+}
+
 /*
  * Get a certificate list blob from the named EFI variable.
  */
@@ -45,25 +64,29 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
 
 	status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
 	if (status != EFI_BUFFER_TOO_SMALL) {
-		pr_err("Couldn't get size: 0x%lx\n", status);
-		return NULL;
+		if (status != EFI_NOT_FOUND)
+			pr_err("Couldn't get size: 0x%lx\n", status);
+		goto err;
 	}
 
 	db = kmalloc(lsize, GFP_KERNEL);
 	if (!db) {
 		pr_err("Couldn't allocate memory for uefi cert list\n");
-		return NULL;
+		goto err;
 	}
 
 	status = efi.get_variable(name, guid, NULL, &lsize, db);
 	if (status != EFI_SUCCESS) {
 		kfree(db);
 		pr_err("Error reading db var: 0x%lx\n", status);
-		return NULL;
+		goto err;
 	}
 
 	*size = lsize;
 	return db;
+err:
+	print_get_fail(name, status);
+	return NULL;
 }
 
 /*
@@ -153,9 +176,7 @@ static int __init load_uefi_certs(void)
 	 */
 	if (!uefi_check_ignore_db()) {
 		db = get_cert_list(L"db", &secure_var, &dbsize);
-		if (!db) {
-			pr_err("MODSIGN: Couldn't get UEFI db list\n");
-		} else {
+		if (db) {
 			rc = parse_efi_signature_list("UEFI:db",
 						      db, dbsize, get_handler_for_db);
 			if (rc)
@@ -165,9 +186,7 @@ static int __init load_uefi_certs(void)
 	}
 
 	dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
-	if (!dbx) {
-		pr_info("MODSIGN: Couldn't get UEFI dbx list\n");
-	} else {
+	if (dbx) {
 		rc = parse_efi_signature_list("UEFI:dbx",
 					      dbx, dbxsize,
 					      get_handler_for_dbx);
@@ -181,9 +200,7 @@ static int __init load_uefi_certs(void)
 		return 0;
 
 	mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
-	if (!mok) {
-		pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
-	} else {
+	if (mok) {
 		rc = parse_efi_signature_list("UEFI:MokListRT",
 					      mok, moksize, get_handler_for_db);
 		if (rc)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2729d6f..c44946c 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1600,4 +1600,29 @@ struct linux_efi_random_seed {
 	u8	bits[];
 };
 
+#define EFI_STATUS_STR(_status)				\
+	case EFI_##_status:				\
+		return "EFI_" __stringify(_status);	\
+
+static inline char *
+efi_status_to_str(efi_status_t status)
+{
+	switch (status) {
+	EFI_STATUS_STR(SUCCESS)
+	EFI_STATUS_STR(LOAD_ERROR)
+	EFI_STATUS_STR(INVALID_PARAMETER)
+	EFI_STATUS_STR(UNSUPPORTED)
+	EFI_STATUS_STR(BAD_BUFFER_SIZE)
+	EFI_STATUS_STR(BUFFER_TOO_SMALL)
+	EFI_STATUS_STR(NOT_READY)
+	EFI_STATUS_STR(DEVICE_ERROR)
+	EFI_STATUS_STR(WRITE_PROTECTED)
+	EFI_STATUS_STR(OUT_OF_RESOURCES)
+	EFI_STATUS_STR(NOT_FOUND)
+	EFI_STATUS_STR(ABORTED)
+	EFI_STATUS_STR(SECURITY_VIOLATION)
+	}
+
+	return "";
+}
 #endif /* _LINUX_EFI_H */
-- 
2.10.2

[PATCH 3/5] MODSIGN: load blacklist from MOKx

[Lee, Chun-Yi]

This patch adds the logic to load the blacklisted hash and
certificates from MOKx which is maintained by shim bootloader.

Cc: David Howells <dhowells@redhat.com>
Cc: Josh Boyer <jwboyer@fedoraproject.org>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
 certs/load_uefi.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/certs/load_uefi.c b/certs/load_uefi.c
index f2f372b..dc66a79 100644
--- a/certs/load_uefi.c
+++ b/certs/load_uefi.c
@@ -164,8 +164,8 @@ static int __init load_uefi_certs(void)
 {
 	efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
 	efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
-	void *db = NULL, *dbx = NULL, *mok = NULL;
-	unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
+	void *db = NULL, *dbx = NULL, *mok = NULL, *mokx = NULL;
+	unsigned long dbsize = 0, dbxsize = 0, moksize = 0, mokxsize = 0;
 	int rc = 0;
 
 	if (!efi.get_variable)
@@ -195,7 +195,7 @@ static int __init load_uefi_certs(void)
 		kfree(dbx);
 	}
 
-	/* the MOK can not be trusted when secure boot is disabled */
+	/* the MOK and MOKx can not be trusted when secure boot is disabled */
 	if (!efi_enabled(EFI_SECURE_BOOT))
 		return 0;
 
@@ -208,6 +208,16 @@ static int __init load_uefi_certs(void)
 		kfree(mok);
 	}
 
+	mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize);
+	if (mokx) {
+		rc = parse_efi_signature_list("UEFI:mokx",
+					      mokx, mokxsize,
+					      get_handler_for_dbx);
+		if (rc)
+			pr_err("Couldn't parse MokListXRT signatures: %d\n", rc);
+		kfree(mokx);
+	}
+
 	return rc;
 }
 late_initcall(load_uefi_certs);
-- 
2.10.2

[PATCH 4/5] MODSIGN: checking the blacklisted hash before loading a kernel module

[Lee, Chun-Yi]

This patch adds the logic for checking the kernel module's hash
base on blacklist. The hash must be generated by sha256 and enrolled
to dbx/mokx.

For example:
	sha256sum sample.ko
	mokutil --mokx --import-hash $HASH_RESULT

Whether the signature on ko file is stripped or not, the hash can be
compared by kernel.

Cc: David Howells <dhowells@redhat.com>
Cc: Josh Boyer <jwboyer@fedoraproject.org>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
 kernel/module_signing.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index d3d6f95..d30ac74 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -11,9 +11,12 @@
 
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/module.h>
 #include <linux/string.h>
 #include <linux/verification.h>
 #include <crypto/public_key.h>
+#include <crypto/hash.h>
+#include <keys/system_keyring.h>
 #include "module-internal.h"
 
 enum pkey_id_type {
@@ -42,19 +45,67 @@ struct module_signature {
 	__be32	sig_len;	/* Length of signature data */
 };
 
+static int mod_is_hash_blacklisted(const void *mod, size_t verifylen)
+{
+	struct crypto_shash *tfm;
+	struct shash_desc *desc;
+	size_t digest_size, desc_size;
+	u8 *digest;
+	int ret = 0;
+
+	tfm = crypto_alloc_shash("sha256", 0, 0);
+	if (IS_ERR(tfm))
+		goto error_return;
+
+	desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
+	digest_size = crypto_shash_digestsize(tfm);
+	digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
+	if (!digest) {
+		pr_err("digest memory buffer allocate fail\n");
+		ret = -ENOMEM;
+		goto error_digest;
+	}
+	desc = (void *)digest + digest_size;
+	desc->tfm = tfm;
+	desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+	ret = crypto_shash_init(desc);
+	if (ret < 0)
+		goto error_shash;
+
+	ret = crypto_shash_finup(desc, mod, verifylen, digest);
+	if (ret < 0)
+		goto error_shash;
+
+	pr_debug("%ld digest: %*phN\n", verifylen, (int) digest_size, digest);
+
+	ret = is_hash_blacklisted(digest, digest_size, "bin");
+	if (ret == -EKEYREJECTED)
+		pr_err("Module hash %*phN is blacklisted\n",
+		       (int) digest_size, digest);
+
+error_shash:
+	kfree(digest);
+error_digest:
+	crypto_free_shash(tfm);
+error_return:
+	return ret;
+}
+
 /*
  * Verify the signature on a module.
  */
 int mod_verify_sig(const void *mod, unsigned long *_modlen)
 {
 	struct module_signature ms;
-	size_t modlen = *_modlen, sig_len;
+	size_t modlen = *_modlen, sig_len, wholelen;
+	int ret;
 
 	pr_devel("==>%s(,%zu)\n", __func__, modlen);
 
 	if (modlen <= sizeof(ms))
 		return -EBADMSG;
 
+	wholelen = modlen + sizeof(MODULE_SIG_STRING) - 1;
 	memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
 	modlen -= sizeof(ms);
 
@@ -80,7 +131,14 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
 		return -EBADMSG;
 	}
 
-	return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
+	ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
 				      (void *)1UL, VERIFYING_MODULE_SIGNATURE,
 				      NULL, NULL);
+	pr_devel("verify_pkcs7_signature() = %d\n", ret);
+
+	/* checking hash of module is in blacklist */
+	if (!ret)
+		ret = mod_is_hash_blacklisted(mod, wholelen);
+
+	return ret;
 }
-- 
2.10.2

Re: [PATCH 4/5] MODSIGN: checking the blacklisted hash before loading a kernel module

[James Bottomley]

On Tue, 2018-03-13 at 18:38 +0800, Lee, Chun-Yi wrote:
> This patch adds the logic for checking the kernel module's hash
> base on blacklist. The hash must be generated by sha256 and enrolled
> to dbx/mokx.
> 
> For example:
> 	sha256sum sample.ko
> 	mokutil --mokx --import-hash $HASH_RESULT
> 
> Whether the signature on ko file is stripped or not, the hash can be
> compared by kernel.

What's the use case for this?  We're already in trouble from the ODMs
for the size of dbx and its consumption of the extremely limited
variable space, so do we really have a use case for adding module
blacklist hashes to the UEFI variables given the space constraints (as
in one we can't do any other way)?

James

Re: [PATCH 4/5] MODSIGN: checking the blacklisted hash before loading a kernel module

[joeyli]

On Tue, Mar 13, 2018 at 10:18:35AM -0700, James Bottomley wrote:
> On Tue, 2018-03-13 at 18:38 +0800, Lee, Chun-Yi wrote:
> > This patch adds the logic for checking the kernel module's hash
> > base on blacklist. The hash must be generated by sha256 and enrolled
> > to dbx/mokx.
> > 
> > For example:
> > 	sha256sum sample.ko
> > 	mokutil --mokx --import-hash $HASH_RESULT
> > 
> > Whether the signature on ko file is stripped or not, the hash can be
> > compared by kernel.
> 
> What's the use case for this?  We're already in trouble from the ODMs
> for the size of dbx and its consumption of the extremely limited
> variable space, so do we really have a use case for adding module
> blacklist hashes to the UEFI variables given the space constraints (as
> in one we can't do any other way)?
>

The dbx is a authenticated variable that it can only be updated by
manufacturer. The mokx gives a flexible way for distro to revoke a key
or a signed module. Then we don't need to touch shim or bother
manufacturer to deliver new db. Currently it doesn't have real use
case yet. 

I knew that the NVRAM has limited space. But distro needs a backup
solution for emergency.

Thanks a lot!
Joey Lee 

Re: [PATCH 4/5] MODSIGN: checking the blacklisted hash before loading a kernel module

[James Bottomley]

On Wed, 2018-03-14 at 14:08 +0800, joeyli wrote:
> On Tue, Mar 13, 2018 at 10:18:35AM -0700, James Bottomley wrote:
> > 
> > On Tue, 2018-03-13 at 18:38 +0800, Lee, Chun-Yi wrote:
> > > 
> > > This patch adds the logic for checking the kernel module's hash
> > > base on blacklist. The hash must be generated by sha256 and
> > > enrolled
> > > to dbx/mokx.
> > > 
> > > For example:
> > > 	sha256sum sample.ko
> > > 	mokutil --mokx --import-hash $HASH_RESULT
> > > 
> > > Whether the signature on ko file is stripped or not, the hash can
> > > be
> > > compared by kernel.
> > 
> > What's the use case for this?  We're already in trouble from the
> > ODMs for the size of dbx and its consumption of the extremely
> > limited variable space, so do we really have a use case for adding
> > module blacklist hashes to the UEFI variables given the space
> > constraints (as in one we can't do any other way)?
> > 
> 
> The dbx is a authenticated variable that it can only be updated by
> manufacturer. The mokx gives a flexible way for distro to revoke a
> key or a signed module. Then we don't need to touch shim or bother
> manufacturer to deliver new db. Currently it doesn't have real use
> case yet. 
> 
> I knew that the NVRAM has limited space. But distro needs a backup
> solution for emergency.

I wasn't asking why the variable, I was asking why the mechanism.

OK, let me try to ask the question in a different way:

Why would the distribution need to blacklist a module in this way?  For
the distro to execute the script to add this blacklist, means the
system is getting automated or manual updates ... can't those updates
just remove the module?

The point is that module sha sums are pretty ephemeral in our model
(they change with every kernel), so it seems to be a mismatch to place
them in a permanent blacklist, particularly when we have very limited
space for that list.

James

Re: [PATCH 4/5] MODSIGN: checking the blacklisted hash before loading a kernel module

[joeyli]

On Wed, Mar 14, 2018 at 07:19:25AM -0700, James Bottomley wrote:
> On Wed, 2018-03-14 at 14:08 +0800, joeyli wrote:
> > On Tue, Mar 13, 2018 at 10:18:35AM -0700, James Bottomley wrote:
> > > 
> > > On Tue, 2018-03-13 at 18:38 +0800, Lee, Chun-Yi wrote:
> > > > 
> > > > This patch adds the logic for checking the kernel module's hash
> > > > base on blacklist. The hash must be generated by sha256 and
> > > > enrolled
> > > > to dbx/mokx.
> > > > 
> > > > For example:
> > > > 	sha256sum sample.ko
> > > > 	mokutil --mokx --import-hash $HASH_RESULT
> > > > 
> > > > Whether the signature on ko file is stripped or not, the hash can
> > > > be
> > > > compared by kernel.
> > > 
> > > What's the use case for this?  We're already in trouble from the
> > > ODMs for the size of dbx and its consumption of the extremely
> > > limited variable space, so do we really have a use case for adding
> > > module blacklist hashes to the UEFI variables given the space
> > > constraints (as in one we can't do any other way)?
> > > 
> > 
> > The dbx is a authenticated variable that it can only be updated by
> > manufacturer. The mokx gives a flexible way for distro to revoke a
> > key or a signed module. Then we don't need to touch shim or bother
> > manufacturer to deliver new db. Currently it doesn't have real use
> > case yet. 
> > 
> > I knew that the NVRAM has limited space. But distro needs a backup
> > solution for emergency.
> 
> I wasn't asking why the variable, I was asking why the mechanism.
> 
> OK, let me try to ask the question in a different way:
> 
> Why would the distribution need to blacklist a module in this way?  For

This way is a new option for user to blacklist a module but not the only
way. MOK has this ability because shim implements the mokx by signature
database format (EFI_SIGNATURE_DATA in UEFI spec). This format supports
both hash signature and x.509 certificate.

> the distro to execute the script to add this blacklist, means the
> system is getting automated or manual updates ... can't those updates
> just remove the module?
>
Yes, we can just remove or update the module in kernel rpm or kmp. But
user may re-install distro with old kernel or install a old kmp. If
the blacklist hash was stored in variable, then kernel can prevent
to load the module.

On the other hand, for enrolling mokx, user must reboots system and
deals with shim-mokmanager UI. It's more secure because user should
really know what he does. And user can choice not to enroll the hash
if they still want to use the module.

> The point is that module sha sums are pretty ephemeral in our model
> (they change with every kernel), so it seems to be a mismatch to place
> them in a permanent blacklist, particularly when we have very limited
> space for that list.
>
Normally we run a serious process for signing a kernel module before
shipping it to customer. The SUSE's "Partner Linux Driver Program” (PLDP)
is an example. So the module sha sums are not too ephemeral.

I agree with you for the space is limit. But the mokx gives a option to
distro or user to blacklist kernel module. They can choice to use this
mechanism or not. 

Thanks a lot!
Joey Lee

Re: [PATCH 4/5] MODSIGN: checking the blacklisted hash before loading a kernel module

[James Bottomley]

On Thu, 2018-03-15 at 14:16 +0800, joeyli wrote:
> On Wed, Mar 14, 2018 at 07:19:25AM -0700, James Bottomley wrote:
> > 
> > On Wed, 2018-03-14 at 14:08 +0800, joeyli wrote:
> > > 
> > > On Tue, Mar 13, 2018 at 10:18:35AM -0700, James Bottomley wrote:
> > > > 
> > > > 
> > > > On Tue, 2018-03-13 at 18:38 +0800, Lee, Chun-Yi wrote:
> > > > > 
> > > > > 
> > > > > This patch adds the logic for checking the kernel module's
> > > > > hash base on blacklist. The hash must be generated by sha256
> > > > > and enrolled to dbx/mokx.
> > > > > 
> > > > > For example:
> > > > > 	sha256sum sample.ko
> > > > > 	mokutil --mokx --import-hash $HASH_RESULT
> > > > > 
> > > > > Whether the signature on ko file is stripped or not, the hash
> > > > > can be compared by kernel.
> > > > 
> > > > What's the use case for this?  We're already in trouble from
> > > > the ODMs for the size of dbx and its consumption of the
> > > > extremely limited variable space, so do we really have a use
> > > > case for adding module blacklist hashes to the UEFI variables
> > > > given the space constraints (as in one we can't do any other
> > > > way)?
> > > > 
> > > 
> > > The dbx is a authenticated variable that it can only be updated
> > > by manufacturer. The mokx gives a flexible way for distro to
> > > revoke a key or a signed module. Then we don't need to touch shim
> > > or bother manufacturer to deliver new db. Currently it doesn't
> > > have real use case yet. 
> > > 
> > > I knew that the NVRAM has limited space. But distro needs a
> > > backup solution for emergency.
> > 
> > I wasn't asking why the variable, I was asking why the mechanism.
> > 
> > OK, let me try to ask the question in a different way:
> > 
> > Why would the distribution need to blacklist a module in this way?
> >  For
> 
> This way is a new option for user to blacklist a module but not the
> only way.

So this is for the *user* not the distribution?

>  MOK has this ability because shim implements the mokx by signature
> database format (EFI_SIGNATURE_DATA in UEFI spec). This format
> supports both hash signature and x.509 certificate.
> 
> > 
> > the distro to execute the script to add this blacklist, means the
> > system is getting automated or manual updates ... can't those
> > updates just remove the module?
> > 
> Yes, we can just remove or update the module in kernel rpm or kmp.
> But user may re-install distro with old kernel or install a old kmp.
> If the blacklist hash was stored in variable, then kernel can prevent
> to load the module.
> 
> On the other hand, for enrolling mokx, user must reboots system and
> deals with shim-mokmanager UI. It's more secure because user should
> really know what he does. And user can choice not to enroll the hash
> if they still want to use the module.

OK, so now the use case is the user needs to roll back but doesn't want
a module to load ... I've got to say that in that case I'd just remove
it before reload.

> > The point is that module sha sums are pretty ephemeral in our model
> > (they change with every kernel), so it seems to be a mismatch to
> > place them in a permanent blacklist, particularly when we have very
> > limited space for that list.
> > 
> Normally we run a serious process for signing a kernel module before
> shipping it to customer. The SUSE's "Partner Linux Driver Program”
> (PLDP) is an example. So the module sha sums are not too ephemeral.

Ephemeral isn't about the signing process it means that the sum is
short lived because every time you create a module for a specific
kernel its sum changes (because of the interface versioning) so your
blacklist only applies to one module and specific kernel combination.
 Once you compile it for a different kernel you need a different
blacklist sum for it.

James

Re: [PATCH 4/5] MODSIGN: checking the blacklisted hash before loading a kernel module

[joeyli]

On Thu, Mar 15, 2018 at 07:30:26AM -0700, James Bottomley wrote:
> On Thu, 2018-03-15 at 14:16 +0800, joeyli wrote:
> > On Wed, Mar 14, 2018 at 07:19:25AM -0700, James Bottomley wrote:
> > > 
> > > On Wed, 2018-03-14 at 14:08 +0800, joeyli wrote:
> > > > 
> > > > On Tue, Mar 13, 2018 at 10:18:35AM -0700, James Bottomley wrote:
> > > > > 
> > > > > 
> > > > > On Tue, 2018-03-13 at 18:38 +0800, Lee, Chun-Yi wrote:
> > > > > > 
> > > > > > 
> > > > > > This patch adds the logic for checking the kernel module's
> > > > > > hash base on blacklist. The hash must be generated by sha256
> > > > > > and enrolled to dbx/mokx.
> > > > > > 
> > > > > > For example:
> > > > > > 	sha256sum sample.ko
> > > > > > 	mokutil --mokx --import-hash $HASH_RESULT
> > > > > > 
> > > > > > Whether the signature on ko file is stripped or not, the hash
> > > > > > can be compared by kernel.
> > > > > 
> > > > > What's the use case for this?  We're already in trouble from
> > > > > the ODMs for the size of dbx and its consumption of the
> > > > > extremely limited variable space, so do we really have a use
> > > > > case for adding module blacklist hashes to the UEFI variables
> > > > > given the space constraints (as in one we can't do any other
> > > > > way)?
> > > > > 
> > > > 
> > > > The dbx is a authenticated variable that it can only be updated
> > > > by manufacturer. The mokx gives a flexible way for distro to
> > > > revoke a key or a signed module. Then we don't need to touch shim
> > > > or bother manufacturer to deliver new db. Currently it doesn't
> > > > have real use case yet. 
> > > > 
> > > > I knew that the NVRAM has limited space. But distro needs a
> > > > backup solution for emergency.
> > > 
> > > I wasn't asking why the variable, I was asking why the mechanism.
> > > 
> > > OK, let me try to ask the question in a different way:
> > > 
> > > Why would the distribution need to blacklist a module in this way?
> > >  For
> > 
> > This way is a new option for user to blacklist a module but not the
> > only way.
> 
> So this is for the *user* not the distribution?
> 
> >  MOK has this ability because shim implements the mokx by signature
> > database format (EFI_SIGNATURE_DATA in UEFI spec). This format
> > supports both hash signature and x.509 certificate.
> > 
> > > 
> > > the distro to execute the script to add this blacklist, means the
> > > system is getting automated or manual updates ... can't those
> > > updates just remove the module?
> > > 
> > Yes, we can just remove or update the module in kernel rpm or kmp.
> > But user may re-install distro with old kernel or install a old kmp.
> > If the blacklist hash was stored in variable, then kernel can prevent
> > to load the module.
> > 
> > On the other hand, for enrolling mokx, user must reboots system and
> > deals with shim-mokmanager UI. It's more secure because user should
> > really know what he does. And user can choice not to enroll the hash
> > if they still want to use the module.
> 
> OK, so now the use case is the user needs to roll back but doesn't want
> a module to load ... I've got to say that in that case I'd just remove
> it before reload.
> 
> > > The point is that module sha sums are pretty ephemeral in our model
> > > (they change with every kernel), so it seems to be a mismatch to
> > > place them in a permanent blacklist, particularly when we have very
> > > limited space for that list.
> > > 
> > Normally we run a serious process for signing a kernel module before
> > shipping it to customer. The SUSE's "Partner Linux Driver Program”
> > (PLDP) is an example. So the module sha sums are not too ephemeral.
> 
> Ephemeral isn't about the signing process it means that the sum is
> short lived because every time you create a module for a specific
> kernel its sum changes (because of the interface versioning) so your
> blacklist only applies to one module and specific kernel combination.
>  Once you compile it for a different kernel you need a different
> blacklist sum for it.
>

I agree with you that the sum is ephemeral. I will remove this patch
from the mokx series. The certificate in mokx will be loaded to
blacklist keyring. Which means that we still can use mokx to revoke
public key. But kernel will not check the blacklisted hash before
loading kernel module.

Thanks a lot!
Joey Lee

[PATCH 5/5] MODSIGN: check the attributes of db and mok

[Lee, Chun-Yi]

That's better for checking the attributes of db and mok variables
before loading certificates to kernel keyring.

For db and dbx, both of them are authenticated variables. Which
means that they can only be modified by manufacturer's key. So
the kernel should checks EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS
attribute before we trust it.

For mok-rt and mokx-rt, both of them are created by shim boot loader
to forward the mok/mokx content to runtime. They must be runtime-volatile
variables. So kernel should checks that the attributes map did not set
EFI_VARIABLE_NON_VOLATILE bit before we trust it.

Cc: David Howells <dhowells@redhat.com>
Cc: Josh Boyer <jwboyer@fedoraproject.org>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
 certs/load_uefi.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/certs/load_uefi.c b/certs/load_uefi.c
index dc66a79..52526bd 100644
--- a/certs/load_uefi.c
+++ b/certs/load_uefi.c
@@ -33,7 +33,8 @@ static __init bool uefi_check_ignore_db(void)
 	return status == EFI_SUCCESS;
 }
 
-static __init void print_get_fail(efi_char16_t *char16_str, efi_status_t status)
+static __init void print_get_fail(efi_char16_t *char16_str, efi_status_t status,
+				  u32 attr)
 {
 	char *utf8_str;
 	unsigned long utf8_size;
@@ -46,8 +47,8 @@ static __init void print_get_fail(efi_char16_t *char16_str, efi_status_t status)
 		return;
 	ucs2_as_utf8(utf8_str, char16_str, utf8_size);
 
-	pr_info("MODSIGN: Couldn't get UEFI %s: %s\n",
-		utf8_str, efi_status_to_str(status));
+	pr_info("MODSIGN: Couldn't get UEFI %s: %s    Attributes: 0x%016x\n",
+		utf8_str, efi_status_to_str(status), attr);
 	kfree(utf8_str);
 }
 
@@ -55,12 +56,13 @@ static __init void print_get_fail(efi_char16_t *char16_str, efi_status_t status)
  * Get a certificate list blob from the named EFI variable.
  */
 static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
-				  unsigned long *size)
+				  unsigned long *size, u32 pos_attr, u32 neg_attr)
 {
 	efi_status_t status;
 	unsigned long lsize = 4;
 	unsigned long tmpdb[4];
 	void *db;
+	u32 attr = 0;
 
 	status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
 	if (status != EFI_BUFFER_TOO_SMALL) {
@@ -75,17 +77,22 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
 		goto err;
 	}
 
-	status = efi.get_variable(name, guid, NULL, &lsize, db);
+	status = efi.get_variable(name, guid, &attr, &lsize, db);
 	if (status != EFI_SUCCESS) {
-		kfree(db);
 		pr_err("Error reading db var: 0x%lx\n", status);
-		goto err;
+		goto free;
 	}
+	/* must have positive attributes and no negative attributes */
+	if ((pos_attr && !(attr & pos_attr)) ||
+	    (neg_attr && (attr & neg_attr)))
+		goto free;
 
 	*size = lsize;
 	return db;
+free:
+	kfree(db);
 err:
-	print_get_fail(name, status);
+	print_get_fail(name, status, attr);
 	return NULL;
 }
 
@@ -175,7 +182,8 @@ static int __init load_uefi_certs(void)
 	 * an error if we can't get them.
 	 */
 	if (!uefi_check_ignore_db()) {
-		db = get_cert_list(L"db", &secure_var, &dbsize);
+		db = get_cert_list(L"db", &secure_var, &dbsize,
+			EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, 0);
 		if (db) {
 			rc = parse_efi_signature_list("UEFI:db",
 						      db, dbsize, get_handler_for_db);
@@ -185,7 +193,8 @@ static int __init load_uefi_certs(void)
 		}
 	}
 
-	dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
+	dbx = get_cert_list(L"dbx", &secure_var, &dbxsize,
+		EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, 0);
 	if (dbx) {
 		rc = parse_efi_signature_list("UEFI:dbx",
 					      dbx, dbxsize,
@@ -199,7 +208,8 @@ static int __init load_uefi_certs(void)
 	if (!efi_enabled(EFI_SECURE_BOOT))
 		return 0;
 
-	mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
+	mok = get_cert_list(L"MokListRT", &mok_var, &moksize,
+				0, EFI_VARIABLE_NON_VOLATILE);
 	if (mok) {
 		rc = parse_efi_signature_list("UEFI:MokListRT",
 					      mok, moksize, get_handler_for_db);
@@ -208,7 +218,8 @@ static int __init load_uefi_certs(void)
 		kfree(mok);
 	}
 
-	mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize);
+	mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize,
+				0, EFI_VARIABLE_NON_VOLATILE);
 	if (mokx) {
 		rc = parse_efi_signature_list("UEFI:mokx",
 					      mokx, mokxsize,
-- 
2.10.2