[RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Matthew Garrett]

Systems in lockdown mode should block the kexec of untrusted kernels.
For x86 and ARM we can ensure that a kernel is trustworthy by validating
a PE signature, but this isn't possible on other architectures. On those
platforms we can use IMA instead, either with native IMA digital
signatures or EVM-protected IMA hashes. Add a function to determine
whether IMA will verify signatures on kexec files, and if so permit
kexec_file() even if the kernel is otherwise locked down. This is
restricted to cases where CONFIG_INTEGRITY_TRUSTED_KEYRING is set in
order to prevent an attacker from loading additional keys at runtime.

Signed-off-by: Matthew Garrett <mjg59@google.com>
---
 include/linux/evm.h                 |  6 +++++
 include/linux/ima.h                 |  9 ++++++++
 kernel/kexec_file.c                 |  9 ++++++--
 security/integrity/evm/evm_main.c   |  2 +-
 security/integrity/ima/ima_policy.c | 35 +++++++++++++++++++++++++++++
 5 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/include/linux/evm.h b/include/linux/evm.h
index 8302bc29bb35..6e89d046b716 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -15,6 +15,7 @@
 struct integrity_iint_cache;
 
 #ifdef CONFIG_EVM
+extern bool evm_key_loaded(void);
 extern int evm_set_key(void *key, size_t keylen);
 extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
 					     const char *xattr_name,
@@ -45,6 +46,11 @@ static inline int posix_xattr_acl(const char *xattrname)
 #endif
 #else
 
+static inline bool evm_key_loaded(void)
+{
+	return false;
+}
+
 static inline int evm_set_key(void *key, size_t keylen)
 {
 	return -EOPNOTSUPP;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index dc12fbcf484c..2ec593537c9b 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -132,4 +132,13 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
 	return 0;
 }
 #endif /* CONFIG_IMA_APPRAISE */
+
+#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
+extern bool ima_appraise_kexec_signature(void);
+#else
+static inline bool ima_appraise_kexec_signature(void)
+{
+	return false;
+}
+#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
 #endif /* _LINUX_IMA_H */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 0cfe4f6f7f85..8ca607f1b515 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -20,11 +20,11 @@
 #include <linux/mutex.h>
 #include <linux/list.h>
 #include <linux/fs.h>
-#include <linux/ima.h>
 #include <crypto/hash.h>
 #include <crypto/sha.h>
 #include <linux/elf.h>
 #include <linux/elfcore.h>
+#include <linux/ima.h>
 #include <linux/kernel.h>
 #include <linux/syscalls.h>
 #include <linux/vmalloc.h>
@@ -240,7 +240,12 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 
 		ret = 0;
 
-		if (kernel_is_locked_down(reason)) {
+		/* If IMA is guaranteed to appraise a signature on the kexec
+		 * image, permit it even if the kernel is otherwise locked
+		 * down.
+		 */
+		if (!ima_appraise_kexec_signature() &&
+		    kernel_is_locked_down(reason)) {
 			ret = -EPERM;
 			goto out;
 		}
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index b6d9f14bc234..aad61bc0f774 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -87,7 +87,7 @@ static void __init evm_init_config(void)
 	pr_info("HMAC attrs: 0x%x\n", evm_hmac_attrs);
 }
 
-static bool evm_key_loaded(void)
+bool evm_key_loaded(void)
 {
 	return (bool)(evm_initialized & EVM_KEY_MASK);
 }
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8bc8a1c8cb3f..c06b1a6b3528 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -21,6 +21,7 @@
 #include <linux/genhd.h>
 #include <linux/seq_file.h>
 #include <linux/ima.h>
+#include <linux/evm.h>
 
 #include "ima.h"
 
@@ -1336,4 +1337,38 @@ int ima_policy_show(struct seq_file *m, void *v)
 	seq_puts(m, "\n");
 	return 0;
 }
+
 #endif	/* CONFIG_IMA_READ_POLICY */
+
+#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
+/*
+ * ima_appraise_kexec: whether IMA will appraise a kexec image, either via
+ * IMA digital signatures or with a hash and EVM validation
+ */
+bool ima_appraise_kexec_signature(void)
+{
+	struct ima_rule_entry *entry;
+	bool found = false;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(entry, ima_rules, list) {
+		if (entry->func != KEXEC_KERNEL_CHECK ||
+		    entry->action != APPRAISE)
+			continue;
+
+		/*
+		 * We require this to be a digital signature, not a raw IMA
+		 * hash. An IMA hash is acceptable as long as it's covered
+		 * by an EVM signature.
+		 */
+		if (entry->flags & IMA_DIGSIG_REQUIRED ||
+		    evm_key_loaded()) {
+			found = true;
+			break;
+		}
+	}
+
+	rcu_read_unlock();
+	return found;
+}
+#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
-- 
2.21.0.360.g471c308f928-goog

Re: [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Mimi Zohar]

On Tue, 2019-03-12 at 12:57 -0700, Matthew Garrett wrote:
> Systems in lockdown mode should block the kexec of untrusted kernels.
> For x86 and ARM we can ensure that a kernel is trustworthy by validating
> a PE signature, but this isn't possible on other architectures. On those
> platforms we can use IMA instead, either with native IMA digital
> signatures or EVM-protected IMA hashes. Add a function to determine
> whether IMA will verify signatures on kexec files, and if so permit
> kexec_file() even if the kernel is otherwise locked down. This is
> restricted to cases where CONFIG_INTEGRITY_TRUSTED_KEYRING is set in
> order to prevent an attacker from loading additional keys at runtime.

Thank you for working on this!  With the changes suggested below, it
might work.  :)

> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> ---
>  include/linux/evm.h                 |  6 +++++
>  include/linux/ima.h                 |  9 ++++++++
>  kernel/kexec_file.c                 |  9 ++++++--
>  security/integrity/evm/evm_main.c   |  2 +-
>  security/integrity/ima/ima_policy.c | 35 +++++++++++++++++++++++++++++
>  5 files changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index 8302bc29bb35..6e89d046b716 100644
> --- a/include/linux/evm.h
> +++ b/include/linux/evm.h
> @@ -15,6 +15,7 @@
>  struct integrity_iint_cache;
>  
>  #ifdef CONFIG_EVM
> +extern bool evm_key_loaded(void);
>  extern int evm_set_key(void *key, size_t keylen);
>  extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
>  					     const char *xattr_name,
> @@ -45,6 +46,11 @@ static inline int posix_xattr_acl(const char *xattrname)
>  #endif
>  #else
>  
> +static inline bool evm_key_loaded(void)
> +{
> +	return false;
> +}
> +
>  static inline int evm_set_key(void *key, size_t keylen)
>  {
>  	return -EOPNOTSUPP;
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index dc12fbcf484c..2ec593537c9b 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -132,4 +132,13 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
>  	return 0;
>  }
>  #endif /* CONFIG_IMA_APPRAISE */
> +
> +#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> +extern bool ima_appraise_kexec_signature(void);
> +#else
> +static inline bool ima_appraise_kexec_signature(void)
> +{
> +	return false;
> +}
> +#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
>  #endif /* _LINUX_IMA_H */
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 0cfe4f6f7f85..8ca607f1b515 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -20,11 +20,11 @@
>  #include <linux/mutex.h>
>  #include <linux/list.h>
>  #include <linux/fs.h>
> -#include <linux/ima.h>
>  #include <crypto/hash.h>
>  #include <crypto/sha.h>
>  #include <linux/elf.h>
>  #include <linux/elfcore.h>
> +#include <linux/ima.h>
>  #include <linux/kernel.h>
>  #include <linux/syscalls.h>
>  #include <linux/vmalloc.h>
> @@ -240,7 +240,12 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  
>  		ret = 0;
>  
> -		if (kernel_is_locked_down(reason)) {
> +		/* If IMA is guaranteed to appraise a signature on the kexec
> +		 * image, permit it even if the kernel is otherwise locked
> +		 * down.
> +		 */
> +		if (!ima_appraise_kexec_signature() &&
> +		    kernel_is_locked_down(reason)) {
>  			ret = -EPERM;
>  			goto out;
>  		}
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index b6d9f14bc234..aad61bc0f774 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -87,7 +87,7 @@ static void __init evm_init_config(void)
>  	pr_info("HMAC attrs: 0x%x\n", evm_hmac_attrs);
>  }
>  
> -static bool evm_key_loaded(void)
> +bool evm_key_loaded(void)
>  {
>  	return (bool)(evm_initialized & EVM_KEY_MASK);
>  }

This might be sufficient for your environment, but in general it
isn't.


> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 8bc8a1c8cb3f..c06b1a6b3528 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -21,6 +21,7 @@
>  #include <linux/genhd.h>
>  #include <linux/seq_file.h>
>  #include <linux/ima.h>
> +#include <linux/evm.h>
>  
>  #include "ima.h"
>  
> @@ -1336,4 +1337,38 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	seq_puts(m, "\n");
>  	return 0;
>  }
> +
>  #endif	/* CONFIG_IMA_READ_POLICY */
> +
> +#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)

With these defines, the function isn't limited to just "lockdown".
 Either fix the defines or the patch description.

> +/*
> + * ima_appraise_kexec: whether IMA will appraise a kexec image, either via
> + * IMA digital signatures or with a hash and EVM validation
> + */
> +bool ima_appraise_kexec_signature(void)

Instead of making this specific to kexec, how about naming the
function something like ima_require_appraise_signature() and pass the
kernel_read_file_id (eg. READING_KEXEC_IMAGE, READING_MODULE).

> +{
> +	struct ima_rule_entry *entry;
> +	bool found = false;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(entry, ima_rules, list) {
> +		if (entry->func != KEXEC_KERNEL_CHECK ||
> +		    entry->action != APPRAISE)
> +			continue;
> +
> +		/*
> +		 * We require this to be a digital signature, not a raw IMA
> +		 * hash. An IMA hash is acceptable as long as it's covered
> +		 * by an EVM signature.
> +		 */
> +		if (entry->flags & IMA_DIGSIG_REQUIRED ||
> +		    evm_key_loaded()) {

evm_key_loaded() is a problem.

> +			found = true;
> +			break;
> +		}

The first matching rule dictates the policy.  Move the "break" here.

Walking the list looking for a specific rule might not be a true
indication of the policy.  For example, with a generic rule prior to a
specific rule, the generic rule might take precedence.

As long as the generic rules require a signature, there isn't a
problem.  I would stop walking the policy rules after the first
"appraise" rule that doesn't require a signature.  This will prevent
returning a false positive.

The builtin and arch policy rules are by design forced to be first.

Mimi


> +	}
> +
> +	rcu_read_unlock();
> +	return found;
> +}
> +#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */

Re: [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Matthew Garrett]

On Wed, Mar 13, 2019 at 4:58 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Tue, 2019-03-12 at 12:57 -0700, Matthew Garrett wrote:
> > Systems in lockdown mode should block the kexec of untrusted kernels.
> > For x86 and ARM we can ensure that a kernel is trustworthy by validating
> > a PE signature, but this isn't possible on other architectures. On those
> > platforms we can use IMA instead, either with native IMA digital
> > signatures or EVM-protected IMA hashes. Add a function to determine
> > whether IMA will verify signatures on kexec files, and if so permit
> > kexec_file() even if the kernel is otherwise locked down. This is
> > restricted to cases where CONFIG_INTEGRITY_TRUSTED_KEYRING is set in
> > order to prevent an attacker from loading additional keys at runtime.
>
> Thank you for working on this!  With the changes suggested below, it
> might work.  :)

Ok, I'll incorporate them - just one question:

> > +bool evm_key_loaded(void)
> >  {
> >       return (bool)(evm_initialized & EVM_KEY_MASK);
> >  }
>
> This might be sufficient for your environment, but in general it
> isn't.

Oh hm. The only case I can see where this isn't sufficient is if the
filesystem returns EOPNOTSUPP for the EVM xattr, but in that case we
should already have failed to get the IMA xattr and will fail
appraisal as a result?

> > +#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
>
> With these defines, the function isn't limited to just "lockdown".
>  Either fix the defines or the patch description.

The function will be called even when lockdown isn't enabled, but it
won't have any impact on the logic flow.

Re: [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Mimi Zohar]

On Wed, 2019-03-13 at 13:36 -0700, Matthew Garrett wrote:
> On Wed, Mar 13, 2019 at 4:58 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Tue, 2019-03-12 at 12:57 -0700, Matthew Garrett wrote:
> > > Systems in lockdown mode should block the kexec of untrusted kernels.
> > > For x86 and ARM we can ensure that a kernel is trustworthy by validating
> > > a PE signature, but this isn't possible on other architectures. On those
> > > platforms we can use IMA instead, either with native IMA digital
> > > signatures or EVM-protected IMA hashes. Add a function to determine
> > > whether IMA will verify signatures on kexec files, and if so permit
> > > kexec_file() even if the kernel is otherwise locked down. This is
> > > restricted to cases where CONFIG_INTEGRITY_TRUSTED_KEYRING is set in
> > > order to prevent an attacker from loading additional keys at runtime.
> >
> > Thank you for working on this!  With the changes suggested below, it
> > might work.  :)
> 
> Ok, I'll incorporate them - just one question:
> 
> > > +bool evm_key_loaded(void)
> > >  {
> > >       return (bool)(evm_initialized & EVM_KEY_MASK);
> > >  }
> >
> > This might be sufficient for your environment, but in general it
> > isn't.
> 
> Oh hm. The only case I can see where this isn't sufficient is if the
> filesystem returns EOPNOTSUPP for the EVM xattr, but in that case we
> should already have failed to get the IMA xattr and will fail
> appraisal as a result?

The evm_initialized flag is an indication that EVM has been
initialized on the system.  Both hmac and signatures could be
supported.  Even checking for EVM_INIT_X509 doesn't provide any
guarantees that the particular file has an EVM signature.

(The hmac can be updated (eg. change in security xattrs,
remove/additional of protected xattr), so we can't rely on them.)

> 
> > > +#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> >
> > With these defines, the function isn't limited to just "lockdown".
> >  Either fix the defines or the patch description.
> 
> The function will be called even when lockdown isn't enabled, but it
> won't have any impact on the logic flow.

Ok, so inverting the test order should prevent unnecessarily calling
ima_apprase_kexec_signature().

+               if (!ima_appraise_kexec_signature() &&
+                   kernel_is_locked_down(reason)) {

Mimi

Re: [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Matthew Garrett]

On Wed, Mar 13, 2019 at 2:29 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2019-03-13 at 13:36 -0700, Matthew Garrett wrote:
> > Oh hm. The only case I can see where this isn't sufficient is if the
> > filesystem returns EOPNOTSUPP for the EVM xattr, but in that case we
> > should already have failed to get the IMA xattr and will fail
> > appraisal as a result?
>
> The evm_initialized flag is an indication that EVM has been
> initialized on the system.  Both hmac and signatures could be
> supported.  Even checking for EVM_INIT_X509 doesn't provide any
> guarantees that the particular file has an EVM signature.
>
> (The hmac can be updated (eg. change in security xattrs,
> remove/additional of protected xattr), so we can't rely on them.)

So having IMA appraisal of the hash and hmac-based EVM validation of
the xattr security isn't sufficient? Is this just because of the
offline attack case?

> > > > +#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> > >
> > > With these defines, the function isn't limited to just "lockdown".
> > >  Either fix the defines or the patch description.
> >
> > The function will be called even when lockdown isn't enabled, but it
> > won't have any impact on the logic flow.
>
> Ok, so inverting the test order should prevent unnecessarily calling
> ima_apprase_kexec_signature().

Unfortunately kernel_is_locked_down(reason) will print a message
telling us that something was blocked, even if
ima_appraise_signature() then permits it, so we need to do it in this
order to shortcut that.

Re: [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Mimi Zohar]

On Wed, 2019-03-13 at 14:59 -0700, Matthew Garrett wrote:
> On Wed, Mar 13, 2019 at 2:29 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Wed, 2019-03-13 at 13:36 -0700, Matthew Garrett wrote:
> > > Oh hm. The only case I can see where this isn't sufficient is if the
> > > filesystem returns EOPNOTSUPP for the EVM xattr, but in that case we
> > > should already have failed to get the IMA xattr and will fail
> > > appraisal as a result?
> >
> > The evm_initialized flag is an indication that EVM has been
> > initialized on the system.  Both hmac and signatures could be
> > supported.  Even checking for EVM_INIT_X509 doesn't provide any
> > guarantees that the particular file has an EVM signature.
> >
> > (The hmac can be updated (eg. change in security xattrs,
> > remove/additional of protected xattr), so we can't rely on them.)
> 
> So having IMA appraisal of the hash and hmac-based EVM validation of
> the xattr security isn't sufficient? Is this just because of the
> offline attack case?

The IMA hash and EVM hmac combination is fine for offline protection.
It's used for mutable files.  For immutable files, there must be
either an IMA or EVM signature.

Mimi

Re: [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Matthew Garrett]

On Wed, Mar 13, 2019 at 6:08 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> The IMA hash and EVM hmac combination is fine for offline protection.
> It's used for mutable files.  For immutable files, there must be
> either an IMA or EVM signature.

Ok. Is the correct way to handle this to check that the file has a
signature, or to extend IMA policy to allow it to provide a
requirement that EVM verify a signature rather than an HMAC and have
the arch policy set that?

Re: [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Mimi Zohar]

On Thu, 2019-03-14 at 14:08 -0700, Matthew Garrett wrote:
> On Wed, Mar 13, 2019 at 6:08 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > The IMA hash and EVM hmac combination is fine for offline protection.
> > It's used for mutable files.  For immutable files, there must be
> > either an IMA or EVM signature.
> 
> Ok. Is the correct way to handle this to check that the file has a
> signature, or to extend IMA policy to allow it to provide a
> requirement that EVM verify a signature rather than an HMAC and have
> the arch policy set that?

I'm not sure what you mean by "check that the file has a signature".

EVM and IMA are separate subsystems with a defined interface for
interaction between them. evm_verifyxattr() isn't, but could be called
by LSMs.  So evm_verifyxattr() would need to be extended to return the
EVM xattr type.  The IMA policy could then require a specific evmxattr
type.  Possible.

Perhaps for now require IMA signatures and defer supporting EVM
signatures?

Mimi

Re: [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Matthew Garrett]

On Thu, Mar 14, 2019 at 3:31 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Thu, 2019-03-14 at 14:08 -0700, Matthew Garrett wrote:
> > On Wed, Mar 13, 2019 at 6:08 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > The IMA hash and EVM hmac combination is fine for offline protection.
> > > It's used for mutable files.  For immutable files, there must be
> > > either an IMA or EVM signature.
> >
> > Ok. Is the correct way to handle this to check that the file has a
> > signature, or to extend IMA policy to allow it to provide a
> > requirement that EVM verify a signature rather than an HMAC and have
> > the arch policy set that?
>
> I'm not sure what you mean by "check that the file has a signature".

Call getxattr(XATTR_SECURITY_EVM) and parse the type to determine
whether it's an hmac or a signature.

> EVM and IMA are separate subsystems with a defined interface for
> interaction between them. evm_verifyxattr() isn't, but could be called
> by LSMs.  So evm_verifyxattr() would need to be extended to return the
> EVM xattr type.  The IMA policy could then require a specific evmxattr
> type.  Possible.

I'd been thinking of doing it the other way (ie, pass the set of
permitted EVM xattr types to evm_verifyxattr()), but yes.

> Perhaps for now require IMA signatures and defer supporting EVM
> signatures?

If that's sufficient for you, happy to do that.

Re: [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Mimi Zohar]

On Thu, 2019-03-14 at 15:54 -0700, Matthew Garrett wrote:
> On Thu, Mar 14, 2019 at 3:31 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Thu, 2019-03-14 at 14:08 -0700, Matthew Garrett wrote:
> > > On Wed, Mar 13, 2019 at 6:08 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > The IMA hash and EVM hmac combination is fine for offline protection.
> > > > It's used for mutable files.  For immutable files, there must be
> > > > either an IMA or EVM signature.
> > >
> > > Ok. Is the correct way to handle this to check that the file has a
> > > signature, or to extend IMA policy to allow it to provide a
> > > requirement that EVM verify a signature rather than an HMAC and have
> > > the arch policy set that?
> >
> > I'm not sure what you mean by "check that the file has a signature".
> 
> Call getxattr(XATTR_SECURITY_EVM) and parse the type to determine
> whether it's an hmac or a signature.

The other option is preferable.

> 
> > EVM and IMA are separate subsystems with a defined interface for
> > interaction between them. evm_verifyxattr() isn't, but could be called
> > by LSMs.  So evm_verifyxattr() would need to be extended to return the
> > EVM xattr type.  The IMA policy could then require a specific evmxattr
> > type.  Possible.
> 
> I'd been thinking of doing it the other way (ie, pass the set of
> permitted EVM xattr types to evm_verifyxattr()), but yes.

Just as IMA decides how it wants to handle "no xattrs", let IMA decide
how it wants to handle the different EVM xattr types.

> 
> > Perhaps for now require IMA signatures and defer supporting EVM
> > signatures?
> 
> If that's sufficient for you, happy to do that.

Definitely

Mimi

[RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Matthew Garrett]

Systems in lockdown mode should block the kexec of untrusted kernels.
For x86 and ARM we can ensure that a kernel is trustworthy by validating
a PE signature, but this isn't possible on other architectures. On those
platforms we can use IMA digital signatures instead. Add a function to
determine whether IMA will verify signatures for a given event type, and
if so permit kexec_file() even if the kernel is otherwise locked down.
This is restricted to cases where CONFIG_INTEGRITY_TRUSTED_KEYRING is set
in order to prevent an attacker from loading additional keys at runtime.

Signed-off-by: Matthew Garrett <mjg59@google.com>
---
 include/linux/evm.h                 |  6 ++++
 include/linux/ima.h                 | 28 +++++++++++++++++++
 kernel/kexec_file.c                 |  9 ++++--
 security/integrity/evm/evm_main.c   |  2 +-
 security/integrity/ima/ima.h        | 20 +-------------
 security/integrity/ima/ima_policy.c | 43 +++++++++++++++++++++++++++++
 6 files changed, 86 insertions(+), 22 deletions(-)

diff --git a/include/linux/evm.h b/include/linux/evm.h
index 8302bc29bb35..6e89d046b716 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -15,6 +15,7 @@
 struct integrity_iint_cache;
 
 #ifdef CONFIG_EVM
+extern bool evm_key_loaded(void);
 extern int evm_set_key(void *key, size_t keylen);
 extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
 					     const char *xattr_name,
@@ -45,6 +46,11 @@ static inline int posix_xattr_acl(const char *xattrname)
 #endif
 #else
 
+static inline bool evm_key_loaded(void)
+{
+	return false;
+}
+
 static inline int evm_set_key(void *key, size_t keylen)
 {
 	return -EOPNOTSUPP;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index dc12fbcf484c..a42e2a9a08b7 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -27,6 +27,25 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 			      enum kernel_read_file_id id);
 extern void ima_post_path_mknod(struct dentry *dentry);
 
+#define __ima_hooks(hook)		\
+	hook(NONE)			\
+	hook(FILE_CHECK)		\
+	hook(MMAP_CHECK)		\
+	hook(BPRM_CHECK)		\
+	hook(CREDS_CHECK)		\
+	hook(POST_SETATTR)		\
+	hook(MODULE_CHECK)		\
+	hook(FIRMWARE_CHECK)		\
+	hook(KEXEC_KERNEL_CHECK)	\
+	hook(KEXEC_INITRAMFS_CHECK)	\
+	hook(POLICY_CHECK)		\
+	hook(MAX_CHECK)
+#define __ima_hook_enumify(ENUM)	ENUM,
+
+enum ima_hooks {
+	__ima_hooks(__ima_hook_enumify)
+};
+
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
@@ -132,4 +151,13 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
 	return 0;
 }
 #endif /* CONFIG_IMA_APPRAISE */
+
+#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
+extern bool ima_appraise_signature(enum ima_hooks func);
+#else
+static inline bool ima_appraise_kexec_signature(enum ima_hooks func)
+{
+	return false;
+}
+#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
 #endif /* _LINUX_IMA_H */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 0cfe4f6f7f85..3e04506a00a2 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -20,11 +20,11 @@
 #include <linux/mutex.h>
 #include <linux/list.h>
 #include <linux/fs.h>
-#include <linux/ima.h>
 #include <crypto/hash.h>
 #include <crypto/sha.h>
 #include <linux/elf.h>
 #include <linux/elfcore.h>
+#include <linux/ima.h>
 #include <linux/kernel.h>
 #include <linux/syscalls.h>
 #include <linux/vmalloc.h>
@@ -240,7 +240,12 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 
 		ret = 0;
 
-		if (kernel_is_locked_down(reason)) {
+		/* If IMA is guaranteed to appraise a signature on the kexec
+		 * image, permit it even if the kernel is otherwise locked
+		 * down.
+		 */
+		if (!ima_appraise_signature(KEXEC_KERNEL_CHECK) &&
+		    kernel_is_locked_down(reason)) {
 			ret = -EPERM;
 			goto out;
 		}
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index b6d9f14bc234..aad61bc0f774 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -87,7 +87,7 @@ static void __init evm_init_config(void)
 	pr_info("HMAC attrs: 0x%x\n", evm_hmac_attrs);
 }
 
-static bool evm_key_loaded(void)
+bool evm_key_loaded(void)
 {
 	return (bool)(evm_initialized & EVM_KEY_MASK);
 }
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index cc12f3449a72..71614a8ed2aa 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -20,6 +20,7 @@
 #include <linux/types.h>
 #include <linux/crypto.h>
 #include <linux/fs.h>
+#include <linux/ima.h>
 #include <linux/security.h>
 #include <linux/hash.h>
 #include <linux/tpm.h>
@@ -171,25 +172,6 @@ static inline unsigned long ima_hash_key(u8 *digest)
 	return hash_long(*digest, IMA_HASH_BITS);
 }
 
-#define __ima_hooks(hook)		\
-	hook(NONE)			\
-	hook(FILE_CHECK)		\
-	hook(MMAP_CHECK)		\
-	hook(BPRM_CHECK)		\
-	hook(CREDS_CHECK)		\
-	hook(POST_SETATTR)		\
-	hook(MODULE_CHECK)		\
-	hook(FIRMWARE_CHECK)		\
-	hook(KEXEC_KERNEL_CHECK)	\
-	hook(KEXEC_INITRAMFS_CHECK)	\
-	hook(POLICY_CHECK)		\
-	hook(MAX_CHECK)
-#define __ima_hook_enumify(ENUM)	ENUM,
-
-enum ima_hooks {
-	__ima_hooks(__ima_hook_enumify)
-};
-
 /* LIM API function definitions */
 int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
 		   int mask, enum ima_hooks func, int *pcr);
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8bc8a1c8cb3f..adeae1ab9ee9 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -21,6 +21,7 @@
 #include <linux/genhd.h>
 #include <linux/seq_file.h>
 #include <linux/ima.h>
+#include <linux/evm.h>
 
 #include "ima.h"
 
@@ -1336,4 +1337,46 @@ int ima_policy_show(struct seq_file *m, void *v)
 	seq_puts(m, "\n");
 	return 0;
 }
+
 #endif	/* CONFIG_IMA_READ_POLICY */
+
+#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
+/*
+ * ima_appraise_signature: whether IMA will appraise a given function using
+ * an IMA digital signature. This is restricted to cases where the kernel
+ * has a set of built-in trusted keys in order to avoid an attacker simply
+ * loading additional keys.
+ */
+bool ima_appraise_signature(enum ima_hooks func)
+{
+	struct ima_rule_entry *entry;
+	bool found = false;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(entry, ima_rules, list) {
+		if (entry->action != APPRAISE)
+			continue;
+
+		/* A generic entry will match, but otherwise require that it
+		 * match the func we're looking for
+		 */
+		if (entry->func && entry->func != func)
+			continue;
+
+		/* We require this to be a digital signature, not a raw IMA
+		 * hash.
+		 */
+		if (entry->flags & IMA_DIGSIG_REQUIRED)
+			found = true;
+
+		/* We've found a rule that matches, so break now even if it
+		 * didn't require a digital signature - a later rule that does
+		 * won't override it, so would be a false positive.
+		 */
+		break;
+	}
+
+	rcu_read_unlock();
+	return found;
+}
+#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
-- 
2.21.0.225.g810b269d1ac-goog

Re: [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Mimi Zohar]

On Fri, 2019-03-15 at 15:03 -0700, Matthew Garrett wrote:
> Systems in lockdown mode should block the kexec of untrusted kernels.
> For x86 and ARM we can ensure that a kernel is trustworthy by validating
> a PE signature, but this isn't possible on other architectures. On those
> platforms we can use IMA digital signatures instead. Add a function to
> determine whether IMA will verify signatures for a given event type,

In both the kexec and kernel modules cases, this should be in the past
tense.  Perhaps change it to something like, "whether IMA has already
or will verify signatures ...".

>  and
> if so permit kexec_file() even if the kernel is otherwise locked down.
> This is restricted to cases where CONFIG_INTEGRITY_TRUSTED_KEYRING is set
> in order to prevent an attacker from loading additional keys at runtime.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> ---
>  include/linux/evm.h                 |  6 ++++
>  include/linux/ima.h                 | 28 +++++++++++++++++++
>  kernel/kexec_file.c                 |  9 ++++--
>  security/integrity/evm/evm_main.c   |  2 +-
>  security/integrity/ima/ima.h        | 20 +-------------
>  security/integrity/ima/ima_policy.c | 43 +++++++++++++++++++++++++++++
>  6 files changed, 86 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index 8302bc29bb35..6e89d046b716 100644
> --- a/include/linux/evm.h
> +++ b/include/linux/evm.h
> @@ -15,6 +15,7 @@
>  struct integrity_iint_cache;
>  
>  #ifdef CONFIG_EVM
> +extern bool evm_key_loaded(void);
>  extern int evm_set_key(void *key, size_t keylen);
>  extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
>  					     const char *xattr_name,
> @@ -45,6 +46,11 @@ static inline int posix_xattr_acl(const char *xattrname)
>  #endif
>  #else
>  
> +static inline bool evm_key_loaded(void)
> +{
> +	return false;
> +}
> +

Remove remaining EVM fragment.

>  static inline int evm_set_key(void *key, size_t keylen)
>  {
>  	return -EOPNOTSUPP;
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index dc12fbcf484c..a42e2a9a08b7 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -27,6 +27,25 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
>  			      enum kernel_read_file_id id);
>  extern void ima_post_path_mknod(struct dentry *dentry);
>  
> +#define __ima_hooks(hook)		\
> +	hook(NONE)			\
> +	hook(FILE_CHECK)		\
> +	hook(MMAP_CHECK)		\
> +	hook(BPRM_CHECK)		\
> +	hook(CREDS_CHECK)		\
> +	hook(POST_SETATTR)		\
> +	hook(MODULE_CHECK)		\
> +	hook(FIRMWARE_CHECK)		\
> +	hook(KEXEC_KERNEL_CHECK)	\
> +	hook(KEXEC_INITRAMFS_CHECK)	\
> +	hook(POLICY_CHECK)		\
> +	hook(MAX_CHECK)
> +#define __ima_hook_enumify(ENUM)	ENUM,
> +
> +enum ima_hooks {
> +	__ima_hooks(__ima_hook_enumify)
> +};
> +
>  #ifdef CONFIG_IMA_KEXEC
>  extern void ima_add_kexec_buffer(struct kimage *image);
>  #endif
> @@ -132,4 +151,13 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
>  	return 0;
>  }
>  #endif /* CONFIG_IMA_APPRAISE */
> +
> +#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> +extern bool ima_appraise_signature(enum ima_hooks func);
> +#else
> +static inline bool ima_appraise_kexec_signature(enum ima_hooks func)
> +{
> +	return false;
> +}
> +#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
>  #endif /* _LINUX_IMA_H */
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 0cfe4f6f7f85..3e04506a00a2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -20,11 +20,11 @@
>  #include <linux/mutex.h>
>  #include <linux/list.h>
>  #include <linux/fs.h>
> -#include <linux/ima.h>
>  #include <crypto/hash.h>
>  #include <crypto/sha.h>
>  #include <linux/elf.h>
>  #include <linux/elfcore.h>
> +#include <linux/ima.h>
>  #include <linux/kernel.h>
>  #include <linux/syscalls.h>
>  #include <linux/vmalloc.h>
> @@ -240,7 +240,12 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  
>  		ret = 0;
>  
> -		if (kernel_is_locked_down(reason)) {
> +		/* If IMA is guaranteed to appraise a signature on the kexec
> +		 * image, permit it even if the kernel is otherwise locked
> +		 * down.
> +		 */
> +		if (!ima_appraise_signature(KEXEC_KERNEL_CHECK) &&
> +		    kernel_is_locked_down(reason)) {
>  			ret = -EPERM;
>  			goto out;

[Cc'ing Dave Young, Eric Biederman, kexec mailing list]

There was a discussion about using KEXEC_KERNEL_CHECK as an argument
when replacing copy_file_from_fd() with kernel_read_file_from_fd().
There was a subsequent discussion when adding a security call in
kexec_load_check.  The end result was defining two enumerations named
kernel_read_file_id and kernel_load_data_id with READING_KEXEC_IMAGE
and LOADING_KECEC_IMAGE respectively.

Instead of making the ima_hooks enumeration global, as we're already
relying on READING_KEXEC_IMAGE, use it.

>  		}
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index b6d9f14bc234..aad61bc0f774 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -87,7 +87,7 @@ static void __init evm_init_config(void)
>  	pr_info("HMAC attrs: 0x%x\n", evm_hmac_attrs);
>  }
>  
> -static bool evm_key_loaded(void)
> +bool evm_key_loaded(void)
>  {
>  	return (bool)(evm_initialized & EVM_KEY_MASK);
>  }

Remove remaining EVM fragment.

> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index cc12f3449a72..71614a8ed2aa 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -20,6 +20,7 @@
>  #include <linux/types.h>
>  #include <linux/crypto.h>
>  #include <linux/fs.h>
> +#include <linux/ima.h>
>  #include <linux/security.h>
>  #include <linux/hash.h>
>  #include <linux/tpm.h>
> @@ -171,25 +172,6 @@ static inline unsigned long ima_hash_key(u8 *digest)
>  	return hash_long(*digest, IMA_HASH_BITS);
>  }
>  
> -#define __ima_hooks(hook)		\
> -	hook(NONE)			\
> -	hook(FILE_CHECK)		\
> -	hook(MMAP_CHECK)		\
> -	hook(BPRM_CHECK)		\
> -	hook(CREDS_CHECK)		\
> -	hook(POST_SETATTR)		\
> -	hook(MODULE_CHECK)		\
> -	hook(FIRMWARE_CHECK)		\
> -	hook(KEXEC_KERNEL_CHECK)	\
> -	hook(KEXEC_INITRAMFS_CHECK)	\
> -	hook(POLICY_CHECK)		\
> -	hook(MAX_CHECK)
> -#define __ima_hook_enumify(ENUM)	ENUM,
> -
> -enum ima_hooks {
> -	__ima_hooks(__ima_hook_enumify)
> -};
> -
>  /* LIM API function definitions */
>  int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
>  		   int mask, enum ima_hooks func, int *pcr);
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 8bc8a1c8cb3f..adeae1ab9ee9 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -21,6 +21,7 @@
>  #include <linux/genhd.h>
>  #include <linux/seq_file.h>
>  #include <linux/ima.h>
> +#include <linux/evm.h>
>  
>  #include "ima.h"
>  
> @@ -1336,4 +1337,46 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	seq_puts(m, "\n");
>  	return 0;
>  }
> +
>  #endif	/* CONFIG_IMA_READ_POLICY */
> +
> +#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> +/*
> + * ima_appraise_signature: whether IMA will appraise a given function using
> + * an IMA digital signature. This is restricted to cases where the kernel
> + * has a set of built-in trusted keys in order to avoid an attacker simply
> + * loading additional keys.
> + */
> +bool ima_appraise_signature(enum ima_hooks func)
> +{
> +	struct ima_rule_entry *entry;
> +	bool found = false;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(entry, ima_rules, list) {
> +		if (entry->action != APPRAISE)
> +			continue;
> +
> +		/* A generic entry will match, but otherwise require that it
> +		 * match the func we're looking for
> +		 */
> +		if (entry->func && entry->func != func)
> +			continue;
> +
> +		/* We require this to be a digital signature, not a raw IMA
> +		 * hash.
> +		 */

Comments should either be a single line or "/*" on a separate line.

> +		if (entry->flags & IMA_DIGSIG_REQUIRED)
> +			found = true;
> +
> +		/* We've found a rule that matches, so break now even if it
> +		 * didn't require a digital signature - a later rule that does
> +		 * won't override it, so would be a false positive.
> +		 */
> +		break;
> +	}
> +

Much better.

thanks,

Mimi

> +	rcu_read_unlock();
> +	return found;
> +}
> +#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */

Re: [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Mimi Zohar]

On Fri, 2019-03-15 at 15:03 -0700, Matthew Garrett wrote:
> Systems in lockdown mode should block the kexec of untrusted kernels.
> For x86 and ARM we can ensure that a kernel is trustworthy by validating
> a PE signature, but this isn't possible on other architectures. On those
> platforms we can use IMA digital signatures instead. Add a function to
> determine whether IMA will verify signatures for a given event type,

In both the kexec and kernel modules cases, this should be in the past
tense.  Perhaps change it to something like, "whether IMA has already
or will verify signatures ...".

>  and
> if so permit kexec_file() even if the kernel is otherwise locked down.
> This is restricted to cases where CONFIG_INTEGRITY_TRUSTED_KEYRING is set
> in order to prevent an attacker from loading additional keys at runtime.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> ---
>  include/linux/evm.h                 |  6 ++++
>  include/linux/ima.h                 | 28 +++++++++++++++++++
>  kernel/kexec_file.c                 |  9 ++++--
>  security/integrity/evm/evm_main.c   |  2 +-
>  security/integrity/ima/ima.h        | 20 +-------------
>  security/integrity/ima/ima_policy.c | 43 +++++++++++++++++++++++++++++
>  6 files changed, 86 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index 8302bc29bb35..6e89d046b716 100644
> --- a/include/linux/evm.h
> +++ b/include/linux/evm.h
> @@ -15,6 +15,7 @@
>  struct integrity_iint_cache;
>  
>  #ifdef CONFIG_EVM
> +extern bool evm_key_loaded(void);
>  extern int evm_set_key(void *key, size_t keylen);
>  extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
>  					     const char *xattr_name,
> @@ -45,6 +46,11 @@ static inline int posix_xattr_acl(const char *xattrname)
>  #endif
>  #else
>  
> +static inline bool evm_key_loaded(void)
> +{
> +	return false;
> +}
> +

Remove remaining EVM fragment.

>  static inline int evm_set_key(void *key, size_t keylen)
>  {
>  	return -EOPNOTSUPP;
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index dc12fbcf484c..a42e2a9a08b7 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -27,6 +27,25 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
>  			      enum kernel_read_file_id id);
>  extern void ima_post_path_mknod(struct dentry *dentry);
>  
> +#define __ima_hooks(hook)		\
> +	hook(NONE)			\
> +	hook(FILE_CHECK)		\
> +	hook(MMAP_CHECK)		\
> +	hook(BPRM_CHECK)		\
> +	hook(CREDS_CHECK)		\
> +	hook(POST_SETATTR)		\
> +	hook(MODULE_CHECK)		\
> +	hook(FIRMWARE_CHECK)		\
> +	hook(KEXEC_KERNEL_CHECK)	\
> +	hook(KEXEC_INITRAMFS_CHECK)	\
> +	hook(POLICY_CHECK)		\
> +	hook(MAX_CHECK)
> +#define __ima_hook_enumify(ENUM)	ENUM,
> +
> +enum ima_hooks {
> +	__ima_hooks(__ima_hook_enumify)
> +};
> +
>  #ifdef CONFIG_IMA_KEXEC
>  extern void ima_add_kexec_buffer(struct kimage *image);
>  #endif
> @@ -132,4 +151,13 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
>  	return 0;
>  }
>  #endif /* CONFIG_IMA_APPRAISE */
> +
> +#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> +extern bool ima_appraise_signature(enum ima_hooks func);
> +#else
> +static inline bool ima_appraise_kexec_signature(enum ima_hooks func)
> +{
> +	return false;
> +}
> +#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
>  #endif /* _LINUX_IMA_H */
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 0cfe4f6f7f85..3e04506a00a2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -20,11 +20,11 @@
>  #include <linux/mutex.h>
>  #include <linux/list.h>
>  #include <linux/fs.h>
> -#include <linux/ima.h>
>  #include <crypto/hash.h>
>  #include <crypto/sha.h>
>  #include <linux/elf.h>
>  #include <linux/elfcore.h>
> +#include <linux/ima.h>
>  #include <linux/kernel.h>
>  #include <linux/syscalls.h>
>  #include <linux/vmalloc.h>
> @@ -240,7 +240,12 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  
>  		ret = 0;
>  
> -		if (kernel_is_locked_down(reason)) {
> +		/* If IMA is guaranteed to appraise a signature on the kexec
> +		 * image, permit it even if the kernel is otherwise locked
> +		 * down.
> +		 */
> +		if (!ima_appraise_signature(KEXEC_KERNEL_CHECK) &&
> +		    kernel_is_locked_down(reason)) {
>  			ret = -EPERM;
>  			goto out;

[Cc'ing Dave Young, Eric Biederman, kexec mailing list]

There was a discussion about using KEXEC_KERNEL_CHECK as an argument
when replacing copy_file_from_fd() with kernel_read_file_from_fd().
There was a subsequent discussion when adding a security call in
kexec_load_check.  The end result was defining two enumerations named
kernel_read_file_id and kernel_load_data_id with READING_KEXEC_IMAGE
and LOADING_KECEC_IMAGE respectively.

Instead of making the ima_hooks enumeration global, as we're already
relying on READING_KEXEC_IMAGE, use it.

>  		}
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index b6d9f14bc234..aad61bc0f774 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -87,7 +87,7 @@ static void __init evm_init_config(void)
>  	pr_info("HMAC attrs: 0x%x\n", evm_hmac_attrs);
>  }
>  
> -static bool evm_key_loaded(void)
> +bool evm_key_loaded(void)
>  {
>  	return (bool)(evm_initialized & EVM_KEY_MASK);
>  }

Remove remaining EVM fragment.

> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index cc12f3449a72..71614a8ed2aa 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -20,6 +20,7 @@
>  #include <linux/types.h>
>  #include <linux/crypto.h>
>  #include <linux/fs.h>
> +#include <linux/ima.h>
>  #include <linux/security.h>
>  #include <linux/hash.h>
>  #include <linux/tpm.h>
> @@ -171,25 +172,6 @@ static inline unsigned long ima_hash_key(u8 *digest)
>  	return hash_long(*digest, IMA_HASH_BITS);
>  }
>  
> -#define __ima_hooks(hook)		\
> -	hook(NONE)			\
> -	hook(FILE_CHECK)		\
> -	hook(MMAP_CHECK)		\
> -	hook(BPRM_CHECK)		\
> -	hook(CREDS_CHECK)		\
> -	hook(POST_SETATTR)		\
> -	hook(MODULE_CHECK)		\
> -	hook(FIRMWARE_CHECK)		\
> -	hook(KEXEC_KERNEL_CHECK)	\
> -	hook(KEXEC_INITRAMFS_CHECK)	\
> -	hook(POLICY_CHECK)		\
> -	hook(MAX_CHECK)
> -#define __ima_hook_enumify(ENUM)	ENUM,
> -
> -enum ima_hooks {
> -	__ima_hooks(__ima_hook_enumify)
> -};
> -
>  /* LIM API function definitions */
>  int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
>  		   int mask, enum ima_hooks func, int *pcr);
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 8bc8a1c8cb3f..adeae1ab9ee9 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -21,6 +21,7 @@
>  #include <linux/genhd.h>
>  #include <linux/seq_file.h>
>  #include <linux/ima.h>
> +#include <linux/evm.h>
>  
>  #include "ima.h"
>  
> @@ -1336,4 +1337,46 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	seq_puts(m, "\n");
>  	return 0;
>  }
> +
>  #endif	/* CONFIG_IMA_READ_POLICY */
> +
> +#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> +/*
> + * ima_appraise_signature: whether IMA will appraise a given function using
> + * an IMA digital signature. This is restricted to cases where the kernel
> + * has a set of built-in trusted keys in order to avoid an attacker simply
> + * loading additional keys.
> + */
> +bool ima_appraise_signature(enum ima_hooks func)
> +{
> +	struct ima_rule_entry *entry;
> +	bool found = false;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(entry, ima_rules, list) {
> +		if (entry->action != APPRAISE)
> +			continue;
> +
> +		/* A generic entry will match, but otherwise require that it
> +		 * match the func we're looking for
> +		 */
> +		if (entry->func && entry->func != func)
> +			continue;
> +
> +		/* We require this to be a digital signature, not a raw IMA
> +		 * hash.
> +		 */

Comments should either be a single line or "/*" on a separate line.

> +		if (entry->flags & IMA_DIGSIG_REQUIRED)
> +			found = true;
> +
> +		/* We've found a rule that matches, so break now even if it
> +		 * didn't require a digital signature - a later rule that does
> +		 * won't override it, so would be a false positive.
> +		 */
> +		break;
> +	}
> +

Much better.

thanks,

Mimi

> +	rcu_read_unlock();
> +	return found;
> +}
> +#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Matthew Garrett]

Systems in lockdown mode should block the kexec of untrusted kernels.
For x86 and ARM we can ensure that a kernel is trustworthy by validating
a PE signature, but this isn't possible on other architectures. On those
platforms we can use IMA digital signatures instead. Add a function to
determine whether IMA has or will verify signatures for a given event type,
and if so permit kexec_file() even if the kernel is otherwise locked down.
This is restricted to cases where CONFIG_INTEGRITY_TRUSTED_KEYRING is set
in order to prevent an attacker from loading additional keys at runtime.

Signed-off-by: Matthew Garrett <mjg59@google.com>
---
 include/linux/ima.h                 |  9 +++++
 kernel/kexec_file.c                 |  9 +++--
 security/integrity/ima/ima.h        |  3 ++
 security/integrity/ima/ima_main.c   |  2 +-
 security/integrity/ima/ima_policy.c | 52 +++++++++++++++++++++++++++++
 5 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index dc12fbcf484c..a78c04580a3c 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -132,4 +132,13 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
 	return 0;
 }
 #endif /* CONFIG_IMA_APPRAISE */
+
+#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
+extern bool ima_appraise_signature(enum kernel_read_file_id func);
+#else
+static inline bool ima_appraise_kexec_signature(enum kernel_read_file_id func)
+{
+	return false;
+}
+#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
 #endif /* _LINUX_IMA_H */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 0cfe4f6f7f85..3598f93a731a 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -20,11 +20,11 @@
 #include <linux/mutex.h>
 #include <linux/list.h>
 #include <linux/fs.h>
-#include <linux/ima.h>
 #include <crypto/hash.h>
 #include <crypto/sha.h>
 #include <linux/elf.h>
 #include <linux/elfcore.h>
+#include <linux/ima.h>
 #include <linux/kernel.h>
 #include <linux/syscalls.h>
 #include <linux/vmalloc.h>
@@ -240,7 +240,12 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 
 		ret = 0;
 
-		if (kernel_is_locked_down(reason)) {
+		/* If IMA is guaranteed to appraise a signature on the kexec
+		 * image, permit it even if the kernel is otherwise locked
+		 * down.
+		 */
+		if (!ima_appraise_signature(READING_KEXEC_IMAGE) &&
+		    kernel_is_locked_down(reason)) {
 			ret = -EPERM;
 			goto out;
 		}
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index cc12f3449a72..93a6825bfcaf 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -20,6 +20,7 @@
 #include <linux/types.h>
 #include <linux/crypto.h>
 #include <linux/fs.h>
+#include <linux/ima.h>
 #include <linux/security.h>
 #include <linux/hash.h>
 #include <linux/tpm.h>
@@ -115,6 +116,8 @@ struct ima_kexec_hdr {
 	u64 count;
 };
 
+extern const int read_idmap[];
+
 #ifdef CONFIG_HAVE_IMA_KEXEC
 void ima_load_kexec_buffer(void);
 #else
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 357edd140c09..927fe889201a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -473,7 +473,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 	return 0;
 }
 
-static const int read_idmap[READING_MAX_ID] = {
+const int read_idmap[READING_MAX_ID] = {
 	[READING_FIRMWARE] = FIRMWARE_CHECK,
 	[READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
 	[READING_MODULE] = MODULE_CHECK,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8bc8a1c8cb3f..bbd1d5b3d45a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -21,6 +21,7 @@
 #include <linux/genhd.h>
 #include <linux/seq_file.h>
 #include <linux/ima.h>
+#include <linux/evm.h>
 
 #include "ima.h"
 
@@ -1336,4 +1337,55 @@ int ima_policy_show(struct seq_file *m, void *v)
 	seq_puts(m, "\n");
 	return 0;
 }
+
 #endif	/* CONFIG_IMA_READ_POLICY */
+
+#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
+/*
+ * ima_appraise_signature: whether IMA will appraise a given function using
+ * an IMA digital signature. This is restricted to cases where the kernel
+ * has a set of built-in trusted keys in order to avoid an attacker simply
+ * loading additional keys.
+ */
+bool ima_appraise_signature(enum kernel_read_file_id id)
+{
+	struct ima_rule_entry *entry;
+	bool found = false;
+	enum ima_hooks func;
+
+	if (id >= READING_MAX_ID)
+		return false;
+
+	func = read_idmap[id] ?: FILE_CHECK;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(entry, ima_rules, list) {
+		if (entry->action != APPRAISE)
+			continue;
+
+		/*
+		 * A generic entry will match, but otherwise require that it
+		 * match the func we're looking for
+		 */
+		if (entry->func && entry->func != func)
+			continue;
+
+		/*
+		 * We require this to be a digital signature, not a raw IMA
+		 * hash.
+		 */
+		if (entry->flags & IMA_DIGSIG_REQUIRED)
+			found = true;
+
+		/*
+		 * We've found a rule that matches, so break now even if it
+		 * didn't require a digital signature - a later rule that does
+		 * won't override it, so would be a false positive.
+		 */
+		break;
+	}
+
+	rcu_read_unlock();
+	return found;
+}
+#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
-- 
2.21.0.225.g810b269d1ac-goog

[RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Matthew Garrett]

Systems in lockdown mode should block the kexec of untrusted kernels.
For x86 and ARM we can ensure that a kernel is trustworthy by validating
a PE signature, but this isn't possible on other architectures. On those
platforms we can use IMA digital signatures instead. Add a function to
determine whether IMA has or will verify signatures for a given event type,
and if so permit kexec_file() even if the kernel is otherwise locked down.
This is restricted to cases where CONFIG_INTEGRITY_TRUSTED_KEYRING is set
in order to prevent an attacker from loading additional keys at runtime.

Signed-off-by: Matthew Garrett <mjg59@google.com>
---
 include/linux/ima.h                 |  9 +++++
 kernel/kexec_file.c                 |  9 +++--
 security/integrity/ima/ima.h        |  3 ++
 security/integrity/ima/ima_main.c   |  2 +-
 security/integrity/ima/ima_policy.c | 52 +++++++++++++++++++++++++++++
 5 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index dc12fbcf484c..a78c04580a3c 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -132,4 +132,13 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
 	return 0;
 }
 #endif /* CONFIG_IMA_APPRAISE */
+
+#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
+extern bool ima_appraise_signature(enum kernel_read_file_id func);
+#else
+static inline bool ima_appraise_kexec_signature(enum kernel_read_file_id func)
+{
+	return false;
+}
+#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
 #endif /* _LINUX_IMA_H */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 0cfe4f6f7f85..3598f93a731a 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -20,11 +20,11 @@
 #include <linux/mutex.h>
 #include <linux/list.h>
 #include <linux/fs.h>
-#include <linux/ima.h>
 #include <crypto/hash.h>
 #include <crypto/sha.h>
 #include <linux/elf.h>
 #include <linux/elfcore.h>
+#include <linux/ima.h>
 #include <linux/kernel.h>
 #include <linux/syscalls.h>
 #include <linux/vmalloc.h>
@@ -240,7 +240,12 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 
 		ret = 0;
 
-		if (kernel_is_locked_down(reason)) {
+		/* If IMA is guaranteed to appraise a signature on the kexec
+		 * image, permit it even if the kernel is otherwise locked
+		 * down.
+		 */
+		if (!ima_appraise_signature(READING_KEXEC_IMAGE) &&
+		    kernel_is_locked_down(reason)) {
 			ret = -EPERM;
 			goto out;
 		}
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index cc12f3449a72..93a6825bfcaf 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -20,6 +20,7 @@
 #include <linux/types.h>
 #include <linux/crypto.h>
 #include <linux/fs.h>
+#include <linux/ima.h>
 #include <linux/security.h>
 #include <linux/hash.h>
 #include <linux/tpm.h>
@@ -115,6 +116,8 @@ struct ima_kexec_hdr {
 	u64 count;
 };
 
+extern const int read_idmap[];
+
 #ifdef CONFIG_HAVE_IMA_KEXEC
 void ima_load_kexec_buffer(void);
 #else
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 357edd140c09..927fe889201a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -473,7 +473,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 	return 0;
 }
 
-static const int read_idmap[READING_MAX_ID] = {
+const int read_idmap[READING_MAX_ID] = {
 	[READING_FIRMWARE] = FIRMWARE_CHECK,
 	[READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
 	[READING_MODULE] = MODULE_CHECK,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8bc8a1c8cb3f..bbd1d5b3d45a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -21,6 +21,7 @@
 #include <linux/genhd.h>
 #include <linux/seq_file.h>
 #include <linux/ima.h>
+#include <linux/evm.h>
 
 #include "ima.h"
 
@@ -1336,4 +1337,55 @@ int ima_policy_show(struct seq_file *m, void *v)
 	seq_puts(m, "\n");
 	return 0;
 }
+
 #endif	/* CONFIG_IMA_READ_POLICY */
+
+#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
+/*
+ * ima_appraise_signature: whether IMA will appraise a given function using
+ * an IMA digital signature. This is restricted to cases where the kernel
+ * has a set of built-in trusted keys in order to avoid an attacker simply
+ * loading additional keys.
+ */
+bool ima_appraise_signature(enum kernel_read_file_id id)
+{
+	struct ima_rule_entry *entry;
+	bool found = false;
+	enum ima_hooks func;
+
+	if (id >= READING_MAX_ID)
+		return false;
+
+	func = read_idmap[id] ?: FILE_CHECK;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(entry, ima_rules, list) {
+		if (entry->action != APPRAISE)
+			continue;
+
+		/*
+		 * A generic entry will match, but otherwise require that it
+		 * match the func we're looking for
+		 */
+		if (entry->func && entry->func != func)
+			continue;
+
+		/*
+		 * We require this to be a digital signature, not a raw IMA
+		 * hash.
+		 */
+		if (entry->flags & IMA_DIGSIG_REQUIRED)
+			found = true;
+
+		/*
+		 * We've found a rule that matches, so break now even if it
+		 * didn't require a digital signature - a later rule that does
+		 * won't override it, so would be a false positive.
+		 */
+		break;
+	}
+
+	rcu_read_unlock();
+	return found;
+}
+#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
-- 
2.21.0.225.g810b269d1ac-goog


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec