[PATCH v2] KEYS: Make use of platform keyring for module signature verify

[PATCH v2] KEYS: Make use of platform keyring for module signature verify

[Robert Holmes]

This patch completes commit 278311e417be ("kexec, KEYS: Make use of
platform keyring for signature verify") which, while adding the
platform keyring for bzImage verification, neglected to also add
this keyring for module verification.

As such, kernel modules signed with keys from the MokList variable
were not successfully verified.

Signed-off-by: Robert Holmes <robeholmes@gmail.com>
Cc: linux-integrity@vger.kernel.org
Cc: keyrings@vger.kernel.org
Cc: stable@vger.kernel.org
---
 kernel/module_signing.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 6b9a926fd86b..cf94220e9154 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -49,6 +49,7 @@ int mod_verify_sig(const void *mod, struct load_info *info)
 {
 	struct module_signature ms;
 	size_t sig_len, modlen = info->len;
+	int ret;
 
 	pr_devel("==>%s(,%zu)\n", __func__, modlen);
 
@@ -82,8 +83,15 @@ int mod_verify_sig(const void *mod, struct load_info *info)
 		return -EBADMSG;
 	}
 
-	return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
-				      VERIFY_USE_SECONDARY_KEYRING,
-				      VERIFYING_MODULE_SIGNATURE,
-				      NULL, NULL);
+	ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
+				     VERIFY_USE_SECONDARY_KEYRING,
+				     VERIFYING_MODULE_SIGNATURE,
+				     NULL, NULL);
+	if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING)) {
+		ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
+					     VERIFY_USE_PLATFORM_KEYRING,
+					     VERIFYING_MODULE_SIGNATURE,
+					     NULL, NULL);
+	}
+	return ret;
 }
-- 
2.21.0

Re: [PATCH v2] KEYS: Make use of platform keyring for module signature verify

[Robert Holmes]

In the v5.0.9 stable tree we also require also cherry-picking commits

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=219a3e8676f3132d27b530c7d2d6bcab89536b57
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=278311e417be60f7caef6fcb12bda4da2711ceff

which, arguably, should be on stable anyway, since it has already picked up

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-5.0.y&id=9dc92c45177ab70e20ae94baa2f2e558da63a9c7
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-5.0.y&id=60740accf78494e166ec76bdc39b7d75fc2fe1c7

Robert.

On Wed, Apr 24, 2019 at 5:06 PM Sasha Levin <sashal@kernel.org> wrote:
>
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
>
> The bot has tested the following trees: v5.0.9, v4.19.36, v4.14.113, v4.9.170, v4.4.178, v3.18.138.
>
> v5.0.9: Build failed! Errors:
>     kernel/module_signing.c:92:11: error: ‘VERIFY_USE_PLATFORM_KEYRING’ undeclared (first use in this function); did you mean ‘VERIFY_USE_SECONDARY_KEYRING’?
>
> v4.19.36: Failed to apply! Possible dependencies:
>     e84cd7ee630e ("modsign: use all trusted keys to verify module signature")
>
> v4.14.113: Failed to apply! Possible dependencies:
>     81a0abd9f213 ("module: make it clear when we're handling the module copy in info->hdr")
>     e84cd7ee630e ("modsign: use all trusted keys to verify module signature")
>     f314dfea16a0 ("modsign: log module name in the event of an error")
>
> v4.9.170: Failed to apply! Possible dependencies:
>     3e2e857f9c3a ("module: Add module name to modinfo")
>     490194269665 ("module: Pass struct load_info into symbol checks")
>     71810db27c1c ("modversions: treat symbol CRCs as 32 bit quantities")
>     71d9f5079358 ("module: Fix a comment above strong_try_module_get()")
>     81a0abd9f213 ("module: make it clear when we're handling the module copy in info->hdr")
>     96b5b19459b3 ("module: make the modinfo name const")
>     e84cd7ee630e ("modsign: use all trusted keys to verify module signature")
>     f314dfea16a0 ("modsign: log module name in the event of an error")
>
> v4.4.178: Failed to apply! Possible dependencies:
>     136cd3450af8 ("powerpc/module: Only try to generate the ftrace_caller() stub once")
>     20ef10c1b306 ("module: Use the same logic for setting and unsetting RO/NX")
>     3e2e857f9c3a ("module: Add module name to modinfo")
>     490194269665 ("module: Pass struct load_info into symbol checks")
>     4c91bd6eeabb ("powerpc: Merge the RELOCATABLE config entries for ppc32 and ppc64")
>     71810db27c1c ("modversions: treat symbol CRCs as 32 bit quantities")
>     7523e4dc5057 ("module: use a structure to encapsulate layout.")
>     81a0abd9f213 ("module: make it clear when we're handling the module copy in info->hdr")
>     96b5b19459b3 ("module: make the modinfo name const")
>     a5967db9af51 ("kbuild: allow architectures to use thin archives instead of ld -r")
>     b67067f1176d ("kbuild: allow archs to select link dead code/data elimination")
>     be7de5f91fdc ("modules: Add kernel parameter to blacklist modules")
>     cd3caefb4663 ("Fix subtle CONFIG_MODVERSIONS problems")
>     da4230714662 ("powerpc/32/booke: Fix the build error when CRASH_DUMP is enabled")
>     f314dfea16a0 ("modsign: log module name in the event of an error")
>     faaae2a58143 ("Re-enable CONFIG_MODVERSIONS in a slightly weaker form")
>
> v3.18.138: Failed to apply! Possible dependencies:
>     136cd3450af8 ("powerpc/module: Only try to generate the ftrace_caller() stub once")
>     3e2e857f9c3a ("module: Add module name to modinfo")
>     490194269665 ("module: Pass struct load_info into symbol checks")
>     4c91bd6eeabb ("powerpc: Merge the RELOCATABLE config entries for ppc32 and ppc64")
>     6da0b565150b ("kernel:module Fix coding style errors and warnings.")
>     71810db27c1c ("modversions: treat symbol CRCs as 32 bit quantities")
>     7523e4dc5057 ("module: use a structure to encapsulate layout.")
>     7d485f647c1f ("ARM: 8220/1: allow modules outside of bl range")
>     81a0abd9f213 ("module: make it clear when we're handling the module copy in info->hdr")
>     926a59b1dfe2 ("module: Annotate module version magic")
>     96b5b19459b3 ("module: make the modinfo name const")
>     be7de5f91fdc ("modules: Add kernel parameter to blacklist modules")
>     cb9e3c292d01 ("mm: vmalloc: pass additional vm_flags to __vmalloc_node_range()")
>     cd3caefb4663 ("Fix subtle CONFIG_MODVERSIONS problems")
>     da4230714662 ("powerpc/32/booke: Fix the build error when CRASH_DUMP is enabled")
>     f314dfea16a0 ("modsign: log module name in the event of an error")
>     faaae2a58143 ("Re-enable CONFIG_MODVERSIONS in a slightly weaker form")
>
>
> How should we proceed with this patch?
>
> --
> Thanks,
> Sasha

Re: [PATCH v2] KEYS: Make use of platform keyring for module signature verify

[Mimi Zohar]

On Wed, 2019-04-24 at 14:33 +0000, Robert Holmes wrote:
> This patch completes commit 278311e417be ("kexec, KEYS: Make use of
> platform keyring for signature verify") which, while adding the
> platform keyring for bzImage verification, neglected to also add
> this keyring for module verification.
> 
> As such, kernel modules signed with keys from the MokList variable
> were not successfully verified.

Using the platform keyring keys for verifying kernel modules was not
neglected, but rather intentional.  This patch description should
clearly explain the reason for needing to verify kernel module
signatures based on the pre-boot keys.  (Hint: verifying kernel
modules based on the pre-boot keys was previously rejected.)

Mimi

> 
> Signed-off-by: Robert Holmes <robeholmes@gmail.com>
> Cc: linux-integrity@vger.kernel.org
> Cc: keyrings@vger.kernel.org
> Cc: stable@vger.kernel.org
> ---
>  kernel/module_signing.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> index 6b9a926fd86b..cf94220e9154 100644
> --- a/kernel/module_signing.c
> +++ b/kernel/module_signing.c
> @@ -49,6 +49,7 @@ int mod_verify_sig(const void *mod, struct load_info *info)
>  {
>  	struct module_signature ms;
>  	size_t sig_len, modlen = info->len;
> +	int ret;
> 
>  	pr_devel("==>%s(,%zu)\n", __func__, modlen);
> 
> @@ -82,8 +83,15 @@ int mod_verify_sig(const void *mod, struct load_info *info)
>  		return -EBADMSG;
>  	}
> 
> -	return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> -				      VERIFY_USE_SECONDARY_KEYRING,
> -				      VERIFYING_MODULE_SIGNATURE,
> -				      NULL, NULL);
> +	ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> +				     VERIFY_USE_SECONDARY_KEYRING,
> +				     VERIFYING_MODULE_SIGNATURE,
> +				     NULL, NULL);
> +	if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING)) {
> +		ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> +					     VERIFY_USE_PLATFORM_KEYRING,
> +					     VERIFYING_MODULE_SIGNATURE,
> +					     NULL, NULL);
> +	}
> +	return ret;
>  }

Re: [PATCH v2] KEYS: Make use of platform keyring for module signature verify

[Jeremy Cline]

Hi,

On Thu, Apr 25, 2019 at 07:55:50AM -0400, Mimi Zohar wrote:
> On Wed, 2019-04-24 at 14:33 +0000, Robert Holmes wrote:
> > This patch completes commit 278311e417be ("kexec, KEYS: Make use of
> > platform keyring for signature verify") which, while adding the
> > platform keyring for bzImage verification, neglected to also add
> > this keyring for module verification.
> > 
> > As such, kernel modules signed with keys from the MokList variable
> > were not successfully verified.
> 
> Using the platform keyring keys for verifying kernel modules was not
> neglected, but rather intentional.  This patch description should
> clearly explain the reason for needing to verify kernel module
> signatures based on the pre-boot keys.  (Hint: verifying kernel
> modules based on the pre-boot keys was previously rejected.)

So the background for this patch is that Fedora, which carries the
lockdown patch set, recently regressed[0] with respect to user-signed
modules. Previously, we carried a patch that added all the pre-boot keys
to the secondary keyring. That way users could add a machine owner key
and use secure boot and lockdown with their self-signed 3rd party modules.

Since the pre-boot keys are now loaded into the platform keyring, I
suggested that Robert submit the patch upstream, but since the lockdown
patches aren't upstream perhaps it doesn't make much sense to pick this
up and Fedora should continue carrying it.

[0] https://bugzilla.redhat.com/show_bug.cgi?id=1701096

Regards,
Jeremy

> > 
> > Signed-off-by: Robert Holmes <robeholmes@gmail.com>
> > Cc: linux-integrity@vger.kernel.org
> > Cc: keyrings@vger.kernel.org
> > Cc: stable@vger.kernel.org
> > ---
> >  kernel/module_signing.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> > index 6b9a926fd86b..cf94220e9154 100644
> > --- a/kernel/module_signing.c
> > +++ b/kernel/module_signing.c
> > @@ -49,6 +49,7 @@ int mod_verify_sig(const void *mod, struct load_info *info)
> >  {
> >  	struct module_signature ms;
> >  	size_t sig_len, modlen = info->len;
> > +	int ret;
> > 
> >  	pr_devel("==>%s(,%zu)\n", __func__, modlen);
> > 
> > @@ -82,8 +83,15 @@ int mod_verify_sig(const void *mod, struct load_info *info)
> >  		return -EBADMSG;
> >  	}
> > 
> > -	return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> > -				      VERIFY_USE_SECONDARY_KEYRING,
> > -				      VERIFYING_MODULE_SIGNATURE,
> > -				      NULL, NULL);
> > +	ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> > +				     VERIFY_USE_SECONDARY_KEYRING,
> > +				     VERIFYING_MODULE_SIGNATURE,
> > +				     NULL, NULL);
> > +	if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING)) {
> > +		ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> > +					     VERIFY_USE_PLATFORM_KEYRING,
> > +					     VERIFYING_MODULE_SIGNATURE,
> > +					     NULL, NULL);
> > +	}
> > +	return ret;
> >  }
> 

Re: [PATCH v2] KEYS: Make use of platform keyring for module signature verify

[James Bottomley]

On Thu, 2019-04-25 at 07:55 -0400, Mimi Zohar wrote:
> On Wed, 2019-04-24 at 14:33 +0000, Robert Holmes wrote:
> > This patch completes commit 278311e417be ("kexec, KEYS: Make use of
> > platform keyring for signature verify") which, while adding the
> > platform keyring for bzImage verification, neglected to also add
> > this keyring for module verification.
> > 
> > As such, kernel modules signed with keys from the MokList variable
> > were not successfully verified.
> 
> Using the platform keyring keys for verifying kernel modules was not
> neglected, but rather intentional.  This patch description should
> clearly explain the reason for needing to verify kernel module
> signatures based on the pre-boot keys.  (Hint: verifying kernel
> modules based on the pre-boot keys was previously rejected.)

To clarify here: most Linux systems use shim/mok to pivot the root of
trust away from the Secure Boot db variable to the new MokList/shim
built in keys.  This makes the actual secure boot db outside the
expected Linux Kernel trust boundary *unless* the user has taken
ownership of the system and is actually using db for their own trusted
keys.  This makes the policy for what pre-boot keys to trust within the
Linux boundary very complex, which is why we default to not using the
pre-boot keys at all.

James