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

[PATCH 0/4] 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. 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.

Lee, Chun-Yi (4):
  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

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

-- 
2.10.2

[PATCH 1/4] 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>
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/4] MODSIGN: do not load mok when secure boot disabled

[James Bottomley]

On Wed, 2017-11-29 at 22:11 +0800, Lee, Chun-Yi 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.
> 
> Cc: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 
> Cc: Josh Boyer <jwboyer-rxtnV0ftBwyoClj4AeEUq9i2O/JbrIOy@public.gmane.org>
> Signed-off-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>
> ---
>  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);

This isn't really a criticism of your patch but the underlying: all of
the *RT variables, like MokListRT are insecure runtime variables and
can be tampered with.  I can agree that I can't see a tamper attack
between exit boot services and pulling this in to the key list, but I'd
feel a lot happier if the values were obtained directly from the BS
variable before exit boot services because that's means the path for
getting the values is directly within the secure envelope and doesn't
rely on passing via an insecure element.

James

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

[joeyli]

Hi James, 

First, thank you for reviewing and comment!

On Thu, Nov 30, 2017 at 07:51:03AM -0800, James Bottomley wrote:
> On Wed, 2017-11-29 at 22:11 +0800, Lee, Chun-Yi 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.
> > 
> > Cc: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 
> > Cc: Josh Boyer <jwboyer-rxtnV0ftBwyoClj4AeEUq9i2O/JbrIOy@public.gmane.org>
> > Signed-off-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>
> > ---
> >  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);
> 
> This isn't really a criticism of your patch but the underlying: all of
> the *RT variables, like MokListRT are insecure runtime variables and
> can be tampered with.  I can agree that I can't see a tamper attack
> between exit boot services and pulling this in to the key list, but I'd
> feel a lot happier if the values were obtained directly from the BS
> variable before exit boot services because that's means the path for
> getting the values is directly within the secure envelope and doesn't
> rely on passing via an insecure element.
>

The shim creates MokListRT as a Runtime-Volatile variable then copies
MokList to MokListRT at boot time. According to spec, the Runtime-Volatile
variable is read only after ExitBootService:

UEFI Sepc V2.7, P.281
...
Once ExitBootServices() is performed, only variables that have
EFI_VARIABLE_RUNTIME_ACCESS and EFI_VARIABLE_NON_VOLATILE set can be
set with SetVariable().
Variables that have runtime access but that are not nonvolatile
are read-only data variables once ExitBootServices() is performed.
...

On the other hand, the kernel code is signed and verified by shim. So
all codes in initial kernel path are secure. 

But, your suggestion reminds me that the code in get_cert_list() must
checks MOK/MOKx's attribute to make sure they are Runtime-Volatile
variables. I will update this patch.

Thanks a lot!
Joey Lee

[PATCH 2/4] 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>
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/4] 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Josh Boyer <jwboyer-rxtnV0ftBwyoClj4AeEUq9i2O/JbrIOy@public.gmane.org>
Signed-off-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>
---
 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/4] 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>
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