| Message ID | 20190312195715.101995-1-matthewgarrett@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down | expand |
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 */
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.
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
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.
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
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?
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
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.
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
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 */
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(-)