Re: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring
On Fri, 2015-10-23 at 16:05 +0300, Petko Manolov wrote: > On 15-10-22 21:49:25, Dmitry Kasatkin wrote: > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > > index df30334..a292b88 100644 > > --- a/security/integrity/ima/Kconfig > > +++ b/security/integrity/ima/Kconfig > > @@ -123,14 +123,17 @@ config IMA_APPRAISE > > If unsure, say N. > > > > config IMA_TRUSTED_KEYRING > > - bool "Require all keys on the .ima keyring be signed" > > + bool "Require all keys on the .ima keyring be signed (deprecated)" > > depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING > > depends on INTEGRITY_ASYMMETRIC_KEYS > > + select INTEGRITY_TRUSTED_KEYRING > > default y > > help > >This option requires that all keys added to the .ima > >keyring be signed by a key on the system trusted keyring. > > > > + This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING > > + > > config IMA_LOAD_X509 > > bool "Load X509 certificate onto the '.ima' trusted keyring" > > depends on IMA_TRUSTED_KEYRING > > > I guess we may as well remove this switch. Otherwise somebody have to > remember > to post a patch that does so a few kernel releases after this one goes > mainline. There's no harm in leaving the "IMA_TRUSTED_KEYRING" Kconfig option for a couple of releases (or perhaps until it is enabled in a long term release), so that the INTEGRITY_TRUSTED_KEYRING Kconfig option is enabled automatically. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/6] evm: provide a function to set EVM key from the kernel
On Thu, 2015-10-22 at 21:49 +0300, Dmitry Kasatkin wrote:
> Crypto HW kernel module can possibly initialize EVM key from the
> kernel __init code to enable EVM before calling 'init' process.
> This patch provide a function evm_set_key() which can be used to
> set custom key directly to EVM without using KEY subsystem.
Thanks, Dmitry. There's a minor comment inline.
>
> Changes in v3:
> * error reporting moved to evm_set_key
> * EVM_INIT_HMAC moved to evm_set_key
> * added bitop to prevent key setting race
>
> Changes in v2:
> * use size_t for key size instead of signed int
> * provide EVM_MAX_KEY_SIZE macro in
> * provide EVM_MIN_KEY_SIZE macro in
>
> Signed-off-by: Dmitry Kasatkin
> ---
> include/linux/evm.h | 7 ++
> security/integrity/evm/evm_crypto.c | 47
> +++--
> security/integrity/evm/evm_secfs.c | 10 +++-
> 3 files changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index 1fcb88c..35ed9a8 100644
> --- a/include/linux/evm.h
> +++ b/include/linux/evm.h
> @@ -14,6 +14,7 @@
> struct integrity_iint_cache;
>
> #ifdef CONFIG_EVM
> +extern int evm_set_key(void *key, size_t keylen);
> extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
>const char *xattr_name,
>void *xattr_value,
> @@ -42,6 +43,12 @@ static inline int posix_xattr_acl(const char *xattrname)
> }
> #endif
> #else
> +
> +static inline int evm_set_key(void *key, size_t keylen)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> #ifdef CONFIG_INTEGRITY
> static inline enum integrity_status evm_verifyxattr(struct dentry *dentry,
> const char *xattr_name,
> diff --git a/security/integrity/evm/evm_crypto.c
> b/security/integrity/evm/evm_crypto.c
> index 34e1a6f..7aec93e 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include "evm.h"
> @@ -32,6 +33,41 @@ struct crypto_shash *hash_tfm;
>
> static DEFINE_MUTEX(mutex);
>
> +#define EVM_SET_KEY_BUSY 0
> +
> +static unsigned long evm_set_key_flags;
> +
> +/* evm_set_key - set EVM HMAC key from the kernel
> + *
For exported functions this should be "kernel-doc" format.
Mimi
> + * This function allows to set EVM HMAC key from the kernel
> + * without using key subsystem 'encrypted' keys. It can be used
> + * by the crypto HW kernel module which has own way of managing
> + * keys.
> + *
> + * key length should be between 32 and 128 bytes long
> + */
> +int evm_set_key(void *key, size_t keylen)
> +{
> + int rc;
> +
> + rc = -EBUSY;
> + if (test_and_set_bit(EVM_SET_KEY_BUSY, &evm_set_key_flags))
> + goto busy;
> + rc = -EINVAL;
> + if (keylen > MAX_KEY_SIZE)
> + goto inval;
> + memcpy(evmkey, key, keylen);
> + evm_initialized |= EVM_INIT_HMAC;
> + pr_info("key initialized\n");
> + return 0;
> +inval:
> + clear_bit(EVM_SET_KEY_BUSY, &evm_set_key_flags);
> +busy:
> + pr_err("key initialization failed\n");
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(evm_set_key);
> +
> static struct shash_desc *init_desc(char type)
> {
> long rc;
> @@ -242,7 +278,7 @@ int evm_init_key(void)
> {
> struct key *evm_key;
> struct encrypted_key_payload *ekp;
> - int rc = 0;
> + int rc;
>
> evm_key = request_key(&key_type_encrypted, EVMKEY, NULL);
> if (IS_ERR(evm_key))
> @@ -250,12 +286,9 @@ int evm_init_key(void)
>
> down_read(&evm_key->sem);
> ekp = evm_key->payload.data;
> - if (ekp->decrypted_datalen > MAX_KEY_SIZE) {
> - rc = -EINVAL;
> - goto out;
> - }
> - memcpy(evmkey, ekp->decrypted_data, ekp->decrypted_datalen);
> -out:
> +
> + rc = evm_set_key(ekp->decrypted_data, ekp->decrypted_datalen);
> +
> /* burn the original key contents */
> memset(ekp->decrypted_data, 0, ekp->decrypted_datalen);
> up_read(&evm_key->sem);
> diff --git a/security/integrity/evm/evm_secfs.c
> b/security/integrity/evm/evm_secfs.c
> index 3f775df..c8dccd5 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -62,7 +62,7 @@ static ssize_t evm_write_key(struct file *file, const char
> __user *buf,
>size_t count, loff_t *ppos)
> {
> char temp[80];
> - int i, error;
> + int i;
>
> if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_INIT_HMAC))
> return -EPERM;
> @@ -78,12 +78,8 @@ static ssize_t evm_write_key(struct file *file, const char
> __user *buf,
> if ((sscanf(temp, "%d", &i) != 1) || (i != 1))
> return -EINVAL;
>
> - error = evm_init_key();
> - if (!error) {
> - evm_initialized |= EVM_INIT_HM
Re: [PATCHv3 3/6] evm: enable EVM when X509 certificate is loaded
On Thu, 2015-10-22 at 21:49 +0300, Dmitry Kasatkin wrote:
> In order to enable EVM before starting 'init' process,
> evm_initialized needs to be non-zero. Before it was
> indicating that HMAC key is loaded. When EVM loads
> X509 before calling 'init', it is possible to enable
> EVM to start signature based verification.
>
> This patch defines bits to enable EVM if key of any type
> is loaded.
Thanks, Dmitry. There's one comment inline.
> Changes in v2:
> * EVM_STATE_KEY_SET replaced by EVM_INIT_HMAC
> * EVM_STATE_X509_SET replaced by EVM_INIT_X509
>
> Signed-off-by: Dmitry Kasatkin
> ---
> security/integrity/evm/evm.h| 3 +++
> security/integrity/evm/evm_crypto.c | 2 ++
> security/integrity/evm/evm_main.c | 6 +-
> security/integrity/evm/evm_secfs.c | 4 ++--
> 4 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> index 88bfe77..f5f1272 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -21,6 +21,9 @@
>
> #include "../integrity.h"
>
> +#define EVM_INIT_HMAC0x0001
> +#define EVM_INIT_X5090x0002
> +
> extern int evm_initialized;
> extern char *evm_hmac;
> extern char *evm_hash;
> diff --git a/security/integrity/evm/evm_crypto.c
> b/security/integrity/evm/evm_crypto.c
> index 159ef3e..34e1a6f 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -40,6 +40,8 @@ static struct shash_desc *init_desc(char type)
> struct shash_desc *desc;
>
> if (type == EVM_XATTR_HMAC) {
> + if (!(evm_initialized & EVM_INIT_HMAC))
> + return ERR_PTR(-ENOKEY);
init_desc() is called from a couple of different places. In some
instances, like when converting from a signature to an hmac, if
init_desc() fails, the xattr isn't converted to an HMAC. No big deal.
But there are other cases, like when a protected xattr is modified,
failing the write will make the file inaccessible. Does there need to
be an error msg of some sort or an audit msg?
Mimi
> tfm = &hmac_tfm;
> algo = evm_hmac;
> } else {
> diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> index 519de0a..420d94d 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -475,7 +475,11 @@ EXPORT_SYMBOL_GPL(evm_inode_init_security);
> #ifdef CONFIG_EVM_LOAD_X509
> void __init evm_load_x509(void)
> {
> - integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH);
> + int rc;
> +
> + rc = integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH);
> + if (!rc)
> + evm_initialized |= EVM_INIT_X509;
> }
> #endif
>
> diff --git a/security/integrity/evm/evm_secfs.c
> b/security/integrity/evm/evm_secfs.c
> index cf12a04..3f775df 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -64,7 +64,7 @@ static ssize_t evm_write_key(struct file *file, const char
> __user *buf,
> char temp[80];
> int i, error;
>
> - if (!capable(CAP_SYS_ADMIN) || evm_initialized)
> + if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_INIT_HMAC))
> return -EPERM;
>
> if (count >= sizeof(temp) || count == 0)
> @@ -80,7 +80,7 @@ static ssize_t evm_write_key(struct file *file, const char
> __user *buf,
>
> error = evm_init_key();
> if (!error) {
> - evm_initialized = 1;
> + evm_initialized |= EVM_INIT_HMAC;
> pr_info("initialized\n");
> } else
> pr_err("initialization failed\n");
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;
On Fri, Oct 23, 2015 at 3:29 PM, Petko Manolov wrote:
> On 15-10-22 22:15:30, Dmitry Kasatkin wrote:
>> Hi Petko,
>>
>> I have a question
>>
>> On Fri, Oct 16, 2015 at 10:31 PM, Petko Manolov wrote:
>> > IMA policy can now be updated multiple times. The new rules get appended
>> > to the original policy. Have in mind that the rules are scanned in FIFO
>> > order so be careful when you add new ones.
>> >
>> > The mutex locks are replaced with RCU, which should lead to faster policy
>> > traversals. The new rules are first appended to a temporary list, which
>> > on error gets released without disturbing the normal IMA operations.
>> >
>> > Signed-off-by: Petko Manolov
>> > ---
>> > security/integrity/ima/Kconfig | 14
>> > security/integrity/ima/ima_fs.c | 13 +++
>> > security/integrity/ima/ima_policy.c | 70
>> > +
>> > 3 files changed, 74 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/security/integrity/ima/Kconfig
>> > b/security/integrity/ima/Kconfig
>> > index df30334..15264b7 100644
>> > --- a/security/integrity/ima/Kconfig
>> > +++ b/security/integrity/ima/Kconfig
>> > @@ -107,6 +107,20 @@ config IMA_DEFAULT_HASH
>> > default "sha512" if IMA_DEFAULT_HASH_SHA512
>> > default "wp512" if IMA_DEFAULT_HASH_WP512
>> >
>> > +config IMA_WRITE_POLICY
>> > + bool "Enable multiple writes to the IMA policy"
>> > + depends on IMA
>> > + default n
>> > + help
>> > + IMA policy can now be updated multiple times. The new rules get
>> > + appended to the original policy. Have in mind that the rules are
>> > + scanned in FIFO order so be careful when you add new ones.
>> > +
>> > + WARNING: Potential security hole - should be used with care in
>> > + production-grade kernels!
>> > +
>> > + If unsure, say N.
>> > +
>> > config IMA_APPRAISE
>> > bool "Appraise integrity measurements"
>> > depends on IMA
>> > diff --git a/security/integrity/ima/ima_fs.c
>> > b/security/integrity/ima/ima_fs.c
>> > index 816d175..a3cf5c0 100644
>> > --- a/security/integrity/ima/ima_fs.c
>> > +++ b/security/integrity/ima/ima_fs.c
>> > @@ -25,6 +25,8 @@
>> >
>> > #include "ima.h"
>> >
>> > +static DEFINE_MUTEX(ima_write_mutex);
>> > +
>> > static int valid_policy = 1;
>> > #define TMPBUFLEN 12
>> > static ssize_t ima_show_htable_value(char __user *buf, size_t count,
>> > @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file,
>> > const char __user *buf,
>> > {
>> > char *data = NULL;
>> > ssize_t result;
>> > + int res;
>> > +
>> > + res = mutex_lock_interruptible(&ima_write_mutex);
>> > + if (res)
>> > + return res;
>> >
>>
>> You got rid of "ima_rules_mutex" in favor of RCU.
>>
>> I wonder why 'ima_write_mutex" is needed here?
>> I do not see any other uses.
>>
>> ima_open_policy() provide "exclusive" access
>>
>> if (test_and_set_bit(IMA_FS_BUSY, &ima_fs_flags))
>>return -EBUSY;
>
> I was actually going to get rid of IMA_FS_BUSY. It is less flexible with
> respect to user-space tools. If the flag is up then the policy upload will
> fail. The user script or program must check the error and repeat the
> operation
> after some time. Seems kind of pointless to me, unless i am missing
> something.
>
> With a semaphore the second process will block and write the policy once the
> first writer leave the operation. No need to repeat it unless there's some
> real
> error.
>
> I was trying to be careful and not break the code in case the new
> functionality
> is not selected in Kconfig. However, with your most recent patch-set i guess
> we'll have to revisit a few things. :)
Obviously in original situation it will be only a "single" policy
writer - which IMA init script or binary.
If some other script tries to write policy at the same time I would
consider it as someone exploit would trying to do nasty things.
With possibility to update policy I also do not see any need for
multiple writers, when second waits first to finish update and it is
not aware about coming another update. It would be some integrity
manager doing policy updates sequentially from time to time.
But with "reading" the policy file, I could imaging process blocking
to wait when update/read completes.
Dmtry
>
>> > if (datalen >= PAGE_SIZE)
>> > datalen = PAGE_SIZE - 1;
>> > @@ -286,6 +293,8 @@ out:
>> > if (result < 0)
>> > valid_policy = 0;
>> > kfree(data);
>> > + mutex_unlock(&ima_write_mutex);
>> > +
>> > return result;
>> > }
>> >
>> > @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode,
>> > struct file *file)
>> > return 0;
>> > }
>> > ima_update_policy();
>> > +#ifndefCONFIG_IMA_WRITE_POLICY
>> > securityfs_remove(ima_policy);
>> > ima_policy = NULL;
>
RE: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring
From: Petko Manolov [pet...@mip-labs.com]
Sent: Friday, October 23, 2015 4:05 PM
To: Dmitry Kasatkin
Cc: zo...@linux.vnet.ibm.com; linux-ima-de...@lists.sourceforge.net;
linux-security-module@vger.kernel.org; linux-ker...@vger.kernel.org; Dmitry
Kasatkin
Subject: Re: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted'
keyring
On 15-10-22 21:49:25, Dmitry Kasatkin wrote:
> Require all keys added to the EVM keyring be signed by an
> existing trusted key on the system trusted keyring.
>
> This patch also switches IMA to use integrity_init_keyring().
>
> Changes in v3:
> * Added 'init_keyring' config based variable to skip initializing
> keyring instead of using __integrity_init_keyring() wrapper.
> * Added dependency back to CONFIG_IMA_TRUSTED_KEYRING
>
> Changes in v2:
> * Replace CONFIG_EVM_TRUSTED_KEYRING with IMA and EVM common
> CONFIG_INTEGRITY_TRUSTED_KEYRING configuration option
> * Deprecate CONFIG_IMA_TRUSTED_KEYRING but keep it for config
> file compatibility. (Mimi Zohar)
>
> Signed-off-by: Dmitry Kasatkin
> ---
> security/integrity/Kconfig| 11 +++
> security/integrity/digsig.c | 14 --
> security/integrity/evm/evm_main.c | 8 +---
> security/integrity/ima/Kconfig| 5 -
> security/integrity/ima/ima.h | 12
> security/integrity/ima/ima_init.c | 2 +-
> security/integrity/integrity.h| 5 ++---
> 7 files changed, 35 insertions(+), 22 deletions(-)
>
> diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
> index 73c457b..21d7568 100644
> --- a/security/integrity/Kconfig
> +++ b/security/integrity/Kconfig
> @@ -41,6 +41,17 @@ config INTEGRITY_ASYMMETRIC_KEYS
> This option enables digital signature verification using
> asymmetric keys.
>
> +config INTEGRITY_TRUSTED_KEYRING
> + bool "Require all keys on the integrity keyrings be signed"
> + depends on SYSTEM_TRUSTED_KEYRING
> + depends on INTEGRITY_ASYMMETRIC_KEYS
> + select KEYS_DEBUG_PROC_KEYS
> + default y
> + help
> +This option requires that all keys added to the .ima and
> +.evm keyrings be signed by a key on the system trusted
> +keyring.
> +
> config INTEGRITY_AUDIT
> bool "Enables integrity auditing support "
> depends on AUDIT
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 5be9ffb..8ef1511 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -24,15 +24,22 @@
> static struct key *keyring[INTEGRITY_KEYRING_MAX];
>
> static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
> +#ifndef CONFIG_INTEGRITY_TRUSTED_KEYRING
> "_evm",
> - "_module",
> -#ifndef CONFIG_IMA_TRUSTED_KEYRING
> "_ima",
> #else
> + ".evm",
> ".ima",
> #endif
> + "_module",
> };
>
> +#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING
> +static bool init_keyring __initdata = true;
> +#else
> +static bool init_keyring __initdata;
> +#endif
> +
> int integrity_digsig_verify(const unsigned int id, const char *sig, int
> siglen,
> const char *digest, int digestlen)
> {
> @@ -68,6 +75,9 @@ int __init integrity_init_keyring(const unsigned int id)
> const struct cred *cred = current_cred();
> int err = 0;
>
> + if (!init_keyring)
> + return 0;
> +
> keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
> KGIDT_INIT(0), cred,
> ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> index 1334e02..75b7e30 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -478,15 +478,17 @@ static int __init init_evm(void)
>
> evm_init_config();
>
> + error = integrity_init_keyring(INTEGRITY_KEYRING_EVM);
> + if (error)
> + return error;
> +
> error = evm_init_secfs();
> if (error < 0) {
> pr_info("Error registering secfs\n");
> - goto err;
> + return error;
> }
>
> return 0;
> -err:
> - return error;
> }
>
> /*
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index df30334..a292b88 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -123,14 +123,17 @@ config IMA_APPRAISE
> If unsure, say N.
>
> config IMA_TRUSTED_KEYRING
> - bool "Require all keys on the .ima keyring be signed"
> + bool "Require all keys on the .ima keyring be signed (deprecated)"
> depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
> depends on INTEGRITY_ASYMMETRIC_KEYS
> + select INTEGRITY_TRUSTED_KEYRING
> default y
> help
> This option requires that all keys added to the .ima
> keyring be signed by a key on the system trusted keyring.
>
> +Th
Re: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring
On 15-10-22 21:49:25, Dmitry Kasatkin wrote:
> Require all keys added to the EVM keyring be signed by an
> existing trusted key on the system trusted keyring.
>
> This patch also switches IMA to use integrity_init_keyring().
>
> Changes in v3:
> * Added 'init_keyring' config based variable to skip initializing
> keyring instead of using __integrity_init_keyring() wrapper.
> * Added dependency back to CONFIG_IMA_TRUSTED_KEYRING
>
> Changes in v2:
> * Replace CONFIG_EVM_TRUSTED_KEYRING with IMA and EVM common
> CONFIG_INTEGRITY_TRUSTED_KEYRING configuration option
> * Deprecate CONFIG_IMA_TRUSTED_KEYRING but keep it for config
> file compatibility. (Mimi Zohar)
>
> Signed-off-by: Dmitry Kasatkin
> ---
> security/integrity/Kconfig| 11 +++
> security/integrity/digsig.c | 14 --
> security/integrity/evm/evm_main.c | 8 +---
> security/integrity/ima/Kconfig| 5 -
> security/integrity/ima/ima.h | 12
> security/integrity/ima/ima_init.c | 2 +-
> security/integrity/integrity.h| 5 ++---
> 7 files changed, 35 insertions(+), 22 deletions(-)
>
> diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
> index 73c457b..21d7568 100644
> --- a/security/integrity/Kconfig
> +++ b/security/integrity/Kconfig
> @@ -41,6 +41,17 @@ config INTEGRITY_ASYMMETRIC_KEYS
> This option enables digital signature verification using
> asymmetric keys.
>
> +config INTEGRITY_TRUSTED_KEYRING
> + bool "Require all keys on the integrity keyrings be signed"
> + depends on SYSTEM_TRUSTED_KEYRING
> + depends on INTEGRITY_ASYMMETRIC_KEYS
> + select KEYS_DEBUG_PROC_KEYS
> + default y
> + help
> +This option requires that all keys added to the .ima and
> +.evm keyrings be signed by a key on the system trusted
> +keyring.
> +
> config INTEGRITY_AUDIT
> bool "Enables integrity auditing support "
> depends on AUDIT
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 5be9ffb..8ef1511 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -24,15 +24,22 @@
> static struct key *keyring[INTEGRITY_KEYRING_MAX];
>
> static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
> +#ifndef CONFIG_INTEGRITY_TRUSTED_KEYRING
> "_evm",
> - "_module",
> -#ifndef CONFIG_IMA_TRUSTED_KEYRING
> "_ima",
> #else
> + ".evm",
> ".ima",
> #endif
> + "_module",
> };
>
> +#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING
> +static bool init_keyring __initdata = true;
> +#else
> +static bool init_keyring __initdata;
> +#endif
> +
> int integrity_digsig_verify(const unsigned int id, const char *sig, int
> siglen,
> const char *digest, int digestlen)
> {
> @@ -68,6 +75,9 @@ int __init integrity_init_keyring(const unsigned int id)
> const struct cred *cred = current_cred();
> int err = 0;
>
> + if (!init_keyring)
> + return 0;
> +
> keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
> KGIDT_INIT(0), cred,
> ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> index 1334e02..75b7e30 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -478,15 +478,17 @@ static int __init init_evm(void)
>
> evm_init_config();
>
> + error = integrity_init_keyring(INTEGRITY_KEYRING_EVM);
> + if (error)
> + return error;
> +
> error = evm_init_secfs();
> if (error < 0) {
> pr_info("Error registering secfs\n");
> - goto err;
> + return error;
> }
>
> return 0;
> -err:
> - return error;
> }
>
> /*
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index df30334..a292b88 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -123,14 +123,17 @@ config IMA_APPRAISE
> If unsure, say N.
>
> config IMA_TRUSTED_KEYRING
> - bool "Require all keys on the .ima keyring be signed"
> + bool "Require all keys on the .ima keyring be signed (deprecated)"
> depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
> depends on INTEGRITY_ASYMMETRIC_KEYS
> + select INTEGRITY_TRUSTED_KEYRING
> default y
> help
> This option requires that all keys added to the .ima
> keyring be signed by a key on the system trusted keyring.
>
> +This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING
> +
> config IMA_LOAD_X509
> bool "Load X509 certificate onto the '.ima' trusted keyring"
> depends on IMA_TRUSTED_KEYRING
I guess we may as well remove this switch. Otherwise somebody have to remember
to post a patch that does so a few kernel releases after this one goes mainlin
Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;
On 15-10-22 22:15:30, Dmitry Kasatkin wrote:
> Hi Petko,
>
> I have a question
>
> On Fri, Oct 16, 2015 at 10:31 PM, Petko Manolov wrote:
> > IMA policy can now be updated multiple times. The new rules get appended
> > to the original policy. Have in mind that the rules are scanned in FIFO
> > order so be careful when you add new ones.
> >
> > The mutex locks are replaced with RCU, which should lead to faster policy
> > traversals. The new rules are first appended to a temporary list, which
> > on error gets released without disturbing the normal IMA operations.
> >
> > Signed-off-by: Petko Manolov
> > ---
> > security/integrity/ima/Kconfig | 14
> > security/integrity/ima/ima_fs.c | 13 +++
> > security/integrity/ima/ima_policy.c | 70
> > +
> > 3 files changed, 74 insertions(+), 23 deletions(-)
> >
> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > index df30334..15264b7 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -107,6 +107,20 @@ config IMA_DEFAULT_HASH
> > default "sha512" if IMA_DEFAULT_HASH_SHA512
> > default "wp512" if IMA_DEFAULT_HASH_WP512
> >
> > +config IMA_WRITE_POLICY
> > + bool "Enable multiple writes to the IMA policy"
> > + depends on IMA
> > + default n
> > + help
> > + IMA policy can now be updated multiple times. The new rules get
> > + appended to the original policy. Have in mind that the rules are
> > + scanned in FIFO order so be careful when you add new ones.
> > +
> > + WARNING: Potential security hole - should be used with care in
> > + production-grade kernels!
> > +
> > + If unsure, say N.
> > +
> > config IMA_APPRAISE
> > bool "Appraise integrity measurements"
> > depends on IMA
> > diff --git a/security/integrity/ima/ima_fs.c
> > b/security/integrity/ima/ima_fs.c
> > index 816d175..a3cf5c0 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -25,6 +25,8 @@
> >
> > #include "ima.h"
> >
> > +static DEFINE_MUTEX(ima_write_mutex);
> > +
> > static int valid_policy = 1;
> > #define TMPBUFLEN 12
> > static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> > @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file,
> > const char __user *buf,
> > {
> > char *data = NULL;
> > ssize_t result;
> > + int res;
> > +
> > + res = mutex_lock_interruptible(&ima_write_mutex);
> > + if (res)
> > + return res;
> >
>
> You got rid of "ima_rules_mutex" in favor of RCU.
>
> I wonder why 'ima_write_mutex" is needed here?
> I do not see any other uses.
>
> ima_open_policy() provide "exclusive" access
>
> if (test_and_set_bit(IMA_FS_BUSY, &ima_fs_flags))
>return -EBUSY;
I was actually going to get rid of IMA_FS_BUSY. It is less flexible with
respect to user-space tools. If the flag is up then the policy upload will
fail. The user script or program must check the error and repeat the operation
after some time. Seems kind of pointless to me, unless i am missing something.
With a semaphore the second process will block and write the policy once the
first writer leave the operation. No need to repeat it unless there's some
real
error.
I was trying to be careful and not break the code in case the new functionality
is not selected in Kconfig. However, with your most recent patch-set i guess
we'll have to revisit a few things. :)
> > if (datalen >= PAGE_SIZE)
> > datalen = PAGE_SIZE - 1;
> > @@ -286,6 +293,8 @@ out:
> > if (result < 0)
> > valid_policy = 0;
> > kfree(data);
> > + mutex_unlock(&ima_write_mutex);
> > +
> > return result;
> > }
> >
> > @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode,
> > struct file *file)
> > return 0;
> > }
> > ima_update_policy();
> > +#ifndefCONFIG_IMA_WRITE_POLICY
> > securityfs_remove(ima_policy);
> > ima_policy = NULL;
> > +#else
> > + clear_bit(IMA_FS_BUSY, &ima_fs_flags);
> > +#endif
> > return 0;
> > }
> >
>
> May be could actually just clear a flag and leave sysfs entry anyway
>
> #ifdefCONFIG_IMA_WRITE_POLICY
>clear_bit(IMA_FS_BUSY, &ima_fs_flags);
> #endif
>
> otherwise flag will be busy and next open fails as well...
As noted above, i'd rather be rid of this flag.
Petko
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] KEYS: Miscellaneous patches for next
James Morris wrote: > Have these been in next yet? No. David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
