Re: [PATCH v4 2/3] Create IMA machine owner keys (MOK) and blacklist keyrings;

[Petko Manolov]

On 15-10-19 14:20:48, Mimi Zohar wrote:
> On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> > This option creates IMA MOK and blacklist keyrings.  IMA MOK is an
> > intermediate keyring that sits between .system and .ima keyrings,
> > effectively forming a simple CA hierarchy.  To successfully import a key
> > into .ima_mok it must be signed by a key which CA is in .system keyring.
> > On turn any key that needs to go in .ima keyring must be signed by CA in
> > either .system or .ima_mok keyrings. IMA MOK is empty at kernel boot.
> > 
> > IMA blacklist keyring contains all revoked IMA keys.  It is consulted
> > before any other keyring.  If the search is successful the requested
> > operation is rejected and error is returned to the caller.
> 
> Why is the blacklist so closely tied to the .ima_mok keyring?   Is this
> keyring only used for blacklisting keys on the .ima_mok keyring or for
> blacklisting keys on the .ima keyring as well?

This is all we've been using the blacklist keyring so far.  By semantics it is 
system-wide keyring so maybe the commit message should be changed.  Nothing 
prevents others from using it.

The problem is that an IMA option enables the creation of this keyring, which 
makes it IMA specific for the moment.  If it is decided that the blacklist 
keyring should be detached from it's IMA bonds then i guess two things should 
happen: 1) broader discussion (perhaps involving David); and 2) modifying the 
patches;

BTW, same applies for the MOK keyring.  If you want these to be more generally 
used i assume a discussion is in order.


Petko


> > Signed-off-by: Petko Manolov 
> > ---
> >  crypto/asymmetric_keys/x509_public_key.c |  2 ++
> >  include/keys/system_keyring.h| 24 ++
> >  security/integrity/digsig_asymmetric.c   | 14 +
> >  security/integrity/ima/Kconfig   | 17 ++
> >  security/integrity/ima/Makefile  |  1 +
> >  security/integrity/ima/ima_mok.c | 54 
> > 
> >  6 files changed, 112 insertions(+)
> >  create mode 100644 security/integrity/ima/ima_mok.c
> > 
> > diff --git a/crypto/asymmetric_keys/x509_public_key.c 
> > b/crypto/asymmetric_keys/x509_public_key.c
> > index 1970966..66dcf30 100644
> > --- a/crypto/asymmetric_keys/x509_public_key.c
> > +++ b/crypto/asymmetric_keys/x509_public_key.c
> > @@ -319,6 +319,8 @@ static int x509_key_preparse(struct 
> > key_preparsed_payload *prep)
> > goto error_free_cert;
> > } else if (!prep->trusted) {
> > ret = x509_validate_trust(cert, get_system_trusted_keyring());
> > +   if (ret)
> > +   ret = x509_validate_trust(cert, get_ima_mok_keyring());
> > if (!ret)
> > prep->trusted = 1;
> > }
> > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> > index b20cd88..39fd38c 100644
> > --- a/include/keys/system_keyring.h
> > +++ b/include/keys/system_keyring.h
> > @@ -35,4 +35,28 @@ extern int system_verify_data(const void *data, unsigned 
> > long len,
> >   enum key_being_used_for usage);
> >  #endif
> > 
> > +#ifdef CONFIG_IMA_MOK_KEYRING
> > +extern struct key *ima_mok_keyring;
> > +extern struct key *ima_blacklist_keyring;
> > +
> > +static inline struct key *get_ima_mok_keyring(void)
> > +{
> > +   return ima_mok_keyring;
> > +}
> > +static inline struct key *get_ima_blacklist_keyring(void)
> > +{
> > +   return ima_blacklist_keyring;
> > +}
> > +#else
> > +static inline struct key *get_ima_mok_keyring(void)
> > +{
> > +   return NULL;
> > +}
> > +static inline struct key *get_ima_blacklist_keyring(void)
> > +{
> > +   return NULL;
> > +}
> > +#endif /* CONFIG_IMA_MOK_KEYRING */
> > +
> > +
> >  #endif /* _KEYS_SYSTEM_KEYRING_H */
> > diff --git a/security/integrity/digsig_asymmetric.c 
> > b/security/integrity/digsig_asymmetric.c
> > index 4fec181..5ade2a7 100644
> > --- a/security/integrity/digsig_asymmetric.c
> > +++ b/security/integrity/digsig_asymmetric.c
> > @@ -17,6 +17,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > 
> >  #include "integrity.h"
> > 
> > @@ -32,9 +33,22 @@ static struct key *request_asymmetric_key(struct key 
> > *keyring, uint32_t keyid)
> > 
> > pr_debug("key search: \"%s\"\n", name);
> > 
> > +   key = get_ima_blacklist_keyring();
> > +   if (key) {
> > +   key_ref_t kref;
> > +
> > +   kref = keyring_search(make_key_ref(key, 1),
> > +_type_asymmetric, name);
> > +   if (!IS_ERR(kref)) {
> > +   pr_err("Key '%s' is in ima_blacklist_keyring\n", name);
> > +   return ERR_PTR(-EKEYREJECTED);
> > +   }
> > +   }
> > +
> > if (keyring) {
> > /* search in specific keyring */
> > key_ref_t kref;
> > +
> > kref = keyring_search(make_key_ref(keyring, 1),
> > 

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Petko Manolov]

On 15-10-19 14:21:03, Mimi Zohar wrote:
> On Fri, 2015-10-16 at 22:31 +0300, 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.
> 
> There was no need for locking in the original version.  This comment should 
> be 
> included in a change log to reflect different versions of the patch.

Yep, a message change is in order.


Petko


> > 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(_write_mutex);
> > +   if (res)
> > +   return res;
> > 
> > if (datalen >= PAGE_SIZE)
> > datalen = PAGE_SIZE - 1;
> > @@ -286,6 +293,8 @@ out:
> > if (result < 0)
> > valid_policy = 0;
> > kfree(data);
> > +   mutex_unlock(_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, _fs_flags);
> > +#endif
> > return 0;
> >  }
> > 
> > diff --git a/security/integrity/ima/ima_policy.c 
> > b/security/integrity/ima/ima_policy.c
> > index 3997e20..7ace4e4 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -16,6 +16,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> > 
> >  #include "ima.h"
> > @@ -135,11 +136,11 @@ static struct ima_rule_entry default_appraise_rules[] 
> > = {
> > 
> >  static LIST_HEAD(ima_default_rules);
> >  static LIST_HEAD(ima_policy_rules);
> > +static LIST_HEAD(ima_temp_rules);
> >  static struct list_head *ima_rules;
> > 
> > -static DEFINE_MUTEX(ima_rules_mutex);
> > -
> >  static int ima_policy __initdata;
> > +
> >  static int __init default_measure_policy_setup(char *str)
> >  {
> > if (ima_policy)
> > @@ -171,9 +172,8 @@ static int __init default_appraise_policy_setup(char 
> > *str)
> >  __setup("ima_appraise_tcb", default_appraise_policy_setup);
> > 
> >  /*
> > - * Although the IMA policy does not change, the LSM policy can be
> > - * reloaded, leaving the IMA LSM based rules referring to the old,
> > - * stale LSM policy.
> > + * Blocking here is not legal as we hold an RCU lock.  ima_update_policy()
> > + * should make it safe to walk the list at any time.
> >   *
> >   * Update the IMA LSM based rules to reflect the reloaded LSM policy.
> >   * We assume the rules still exist; and BUG_ON() if they don't.
> > @@ -184,7 +184,6 @@ static void ima_lsm_update_rules(void)
> > int result;
> > int i;
> > 
> > -   mutex_lock(_rules_mutex);
> > list_for_each_entry_safe(entry, tmp, 

Re: [PATCH v4 1/3] Enable multiple writes to the IMA policy;

[Petko Manolov]

On 15-10-19 18:28:22, Mimi Zohar wrote:
> On Mon, 2015-10-19 at 14:21 -0400, Mimi Zohar wrote:
> > On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> 
> > > 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(_write_mutex);
> > > + if (res)
> > > + return res;
> > > 
> > >   if (datalen >= PAGE_SIZE)
> > >   datalen = PAGE_SIZE - 1;
> > > @@ -286,6 +293,8 @@ out:
> > >   if (result < 0)
> > >   valid_policy = 0;
> > >   kfree(data);
> > > + mutex_unlock(_write_mutex);
> > > +
> > >   return result;
> > >  }
> > > 
> > > @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode, 
> > > struct file *file)
> > >   return 0;
> > >   }
> > >   ima_update_policy();
> > > +#ifndef  CONFIG_IMA_WRITE_POLICY
> > >   securityfs_remove(ima_policy);
> > >   ima_policy = NULL;
> > > +#else
> > > + clear_bit(IMA_FS_BUSY, _fs_flags);
> > > +#endif
> > >   return 0;
> > >  }
> > > 
> 
> The IMA_FS_BUSY flag needs to be cleared, like here, above for !
> valid_policy.

Good catch.  Fixed.


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: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Petko Manolov]

On 15-10-16 22:31:31, Petko Manolov wrote:
> When in development it is useful to read back the IMA policy.  This patch
> provides the functionality.  However, this is a potential security hole so
> it should not be used in production-grade kernels.
> 
> Signed-off-by: Petko Manolov 
> ---
>  security/integrity/ima/Kconfig  |  13 +++
>  security/integrity/ima/ima.h|  13 ++-
>  security/integrity/ima/ima_fs.c |  29 +-
>  security/integrity/ima/ima_policy.c | 190 
> 
>  4 files changed, 239 insertions(+), 6 deletions(-)
> 
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 235b3c2..040778f 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -121,6 +121,19 @@ config IMA_WRITE_POLICY
>  
> If unsure, say N.
>  
> +config IMA_READ_POLICY
> + bool "Enable reading back the current IMA policy"
> + depends on IMA
> + default n
> + help
> +When developing and debugging an IMA enabled system it is often
> +useful to be able to read the IMA policy.  This patch allows
> +for doing so.  However, being able to read the IMA policy is
> +considered insecure and is strongly discouraged for
> +production-grade kernels.
> +
> +If unsure, say N.
> +
>  config IMA_APPRAISE
>   bool "Appraise integrity measurements"
>   depends on IMA
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index e2a60c3..ebc3554 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -166,6 +166,10 @@ void ima_update_policy(void);
>  void ima_update_policy_flag(void);
>  ssize_t ima_parse_add_rule(char *);
>  void ima_delete_rules(void);
> +void *ima_policy_start(struct seq_file *m, loff_t *pos);
> +void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> +void ima_policy_stop(struct seq_file *m, void *v);
> +int ima_policy_show(struct seq_file *m, void *v);
>  
>  /* Appraise integrity measurements */
>  #define IMA_APPRAISE_ENFORCE 0x01
> @@ -263,4 +267,11 @@ static inline int ima_init_keyring(const unsigned int id)
>   return 0;
>  }
>  #endif /* CONFIG_IMA_TRUSTED_KEYRING */
> -#endif
> +
> +#ifdef   CONFIG_IMA_READ_POLICY
> +#define  POLICY_FILE_FLAGS   (S_IWUSR | S_IRUSR)
> +#else
> +#define  POLICY_FILE_FLAGS   S_IWUSR
> +#endif /* CONFIG_IMA_WRITE_POLICY */
> +
> +#endif /* __LINUX_IMA_H */
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index a3cf5c0..85bd129 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -311,14 +311,29 @@ enum ima_fs_flags {
>  
>  static unsigned long ima_fs_flags;
>  
> +#ifdef   CONFIG_IMA_READ_POLICY
> +static const struct seq_operations ima_policy_seqops = {
> + .start = ima_policy_start,
> + .next = ima_policy_next,
> + .stop = ima_policy_stop,
> + .show = ima_policy_show,
> +};
> +#endif
> +
>  /*
>   * ima_open_policy: sequentialize access to the policy file
>   */
>  static int ima_open_policy(struct inode *inode, struct file *filp)
>  {
> - /* No point in being allowed to open it if you aren't going to write */
> - if (!(filp->f_flags & O_WRONLY))
> + if (!(filp->f_flags & O_WRONLY)) {
> +#ifndef  CONFIG_IMA_READ_POLICY
>   return -EACCES;
> +#else
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> + return seq_open(filp, _policy_seqops);
> +#endif
> + }
>   if (test_and_set_bit(IMA_FS_BUSY, _fs_flags))
>   return -EBUSY;
>   return 0;
> @@ -334,6 +349,10 @@ static int ima_open_policy(struct inode *inode, struct 
> file *filp)
>  static int ima_release_policy(struct inode *inode, struct file *file)
>  {
>   const char *cause = valid_policy ? "completed" : "failed";
> + int policy_write = (file->f_flags & O_WRONLY);
> +
> + if (!policy_write)
> + return 0;

The above check is bogus so these four lines should go away too.  Silently 
returning if 'policy' is readable is now what we mean.  I guess we should check 
the operation, not the file flags.

Petko


>   pr_info("IMA: policy update %s\n", cause);
>   integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> @@ -342,9 +361,9 @@ static int ima_release_policy(struct inode *inode, struct 
> file *file)
>   if (!valid_policy) {
>   ima_delete_rules();
>   valid_policy = 1;
> - clear_bit(IMA_FS_BUSY, _fs_flags);
>   return 0;
>   }
> +
>   ima_update_policy();
>  #ifndef  CONFIG_IMA_WRITE_POLICY
>   securityfs_remove(ima_policy);
> @@ -358,6 +377,7 @@ static int ima_release_policy(struct inode *inode, struct 
> file *file)
>  static const struct file_operations ima_measure_policy_ops = {
>   .open = 

Re: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Mimi Zohar]

On Tue, 2015-10-20 at 10:26 +0300, Petko Manolov wrote:
> On 15-10-19 14:21:42, Mimi Zohar wrote:
> > On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> > > When in development it is useful to read back the IMA policy.  This patch
> > > provides the functionality.  However, this is a potential security hole so
> > > it should not be used in production-grade kernels.
> >  
> > Like the other IMA securityfs files, only root would be able to read it.
> > Once we start allowing additional rules to be appended to the policy,
> > being able to view the resulting policy is important.  Is there a reason
> > for limiting this option to development?
> 
> I have not considered allowing non-root users to read the policy - i was 
> merely 
> cleaning up the Zbigniew's patch.  I guess it might be useful to be able to 
> read 
> the policy when in development mode.

I guess I wasn't clear.  I don't have a problem with the patch itself,
just with the patch description.  What is this "security hole" that the
option should ONLY be configured for development?Only privileged
users can view the policy.  I don't see the problem with configuring it
in general.  Please remove the comment.

Since responding, I've enabled this feature.  Very nice!

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: [GIT PULL] Keys bugfixes

[Linus Torvalds]

On Mon, Oct 19, 2015 at 7:50 PM, James Morris  wrote:
> Please pull these key susbystem fixes for 4.3, per the message from David
> Howells:

Oops. I already pulled David's tree.

Linus
--
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 3/3] Allows reading back the current IMA policy;

[Petko Manolov]

On 15-10-19 14:21:42, Mimi Zohar wrote:
> On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> > When in development it is useful to read back the IMA policy.  This patch
> > provides the functionality.  However, this is a potential security hole so
> > it should not be used in production-grade kernels.
>  
> Like the other IMA securityfs files, only root would be able to read it.
> Once we start allowing additional rules to be appended to the policy,
> being able to view the resulting policy is important.  Is there a reason
> for limiting this option to development?

I have not considered allowing non-root users to read the policy - i was merely 
cleaning up the Zbigniew's patch.  I guess it might be useful to be able to 
read 
the policy when in development mode.


Petko


> > Signed-off-by: Petko Manolov 
> > ---
> >  security/integrity/ima/Kconfig  |  13 +++
> >  security/integrity/ima/ima.h|  13 ++-
> >  security/integrity/ima/ima_fs.c |  29 +-
> >  security/integrity/ima/ima_policy.c | 190 
> > 
> >  4 files changed, 239 insertions(+), 6 deletions(-)
> > 
> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > index 235b3c2..040778f 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -121,6 +121,19 @@ config IMA_WRITE_POLICY
> > 
> >   If unsure, say N.
> > 
> > +config IMA_READ_POLICY
> > +   bool "Enable reading back the current IMA policy"
> > +   depends on IMA
> > +   default n
> > +   help
> > +  When developing and debugging an IMA enabled system it is often
> > +  useful to be able to read the IMA policy.  This patch allows
> > +  for doing so.  However, being able to read the IMA policy is
> > +  considered insecure and is strongly discouraged for
> > +  production-grade kernels.
> > +
> > +  If unsure, say N.
> > +
> >  config IMA_APPRAISE
> > bool "Appraise integrity measurements"
> > depends on IMA
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index e2a60c3..ebc3554 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -166,6 +166,10 @@ void ima_update_policy(void);
> >  void ima_update_policy_flag(void);
> >  ssize_t ima_parse_add_rule(char *);
> >  void ima_delete_rules(void);
> > +void *ima_policy_start(struct seq_file *m, loff_t *pos);
> > +void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> > +void ima_policy_stop(struct seq_file *m, void *v);
> > +int ima_policy_show(struct seq_file *m, void *v);
> > 
> >  /* Appraise integrity measurements */
> >  #define IMA_APPRAISE_ENFORCE   0x01
> > @@ -263,4 +267,11 @@ static inline int ima_init_keyring(const unsigned int 
> > id)
> > return 0;
> >  }
> >  #endif /* CONFIG_IMA_TRUSTED_KEYRING */
> > -#endif
> > +
> > +#ifdef CONFIG_IMA_READ_POLICY
> > +#definePOLICY_FILE_FLAGS   (S_IWUSR | S_IRUSR)
> > +#else
> > +#definePOLICY_FILE_FLAGS   S_IWUSR
> > +#endif /* CONFIG_IMA_WRITE_POLICY */
> > +
> > +#endif /* __LINUX_IMA_H */
> > diff --git a/security/integrity/ima/ima_fs.c 
> > b/security/integrity/ima/ima_fs.c
> > index a3cf5c0..85bd129 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -311,14 +311,29 @@ enum ima_fs_flags {
> > 
> >  static unsigned long ima_fs_flags;
> > 
> > +#ifdef CONFIG_IMA_READ_POLICY
> > +static const struct seq_operations ima_policy_seqops = {
> > +   .start = ima_policy_start,
> > +   .next = ima_policy_next,
> > +   .stop = ima_policy_stop,
> > +   .show = ima_policy_show,
> > +};
> > +#endif
> > +
> >  /*
> >   * ima_open_policy: sequentialize access to the policy file
> >   */
> >  static int ima_open_policy(struct inode *inode, struct file *filp)
> >  {
> > -   /* No point in being allowed to open it if you aren't going to write */
> > -   if (!(filp->f_flags & O_WRONLY))
> > +   if (!(filp->f_flags & O_WRONLY)) {
> > +#ifndefCONFIG_IMA_READ_POLICY
> > return -EACCES;
> > +#else
> > +   if (!capable(CAP_SYS_ADMIN))
> > +   return -EPERM;
> > +   return seq_open(filp, _policy_seqops);
> > +#endif
> > +   }
> > if (test_and_set_bit(IMA_FS_BUSY, _fs_flags))
> > return -EBUSY;
> > return 0;
> > @@ -334,6 +349,10 @@ static int ima_open_policy(struct inode *inode, struct 
> > file *filp)
> >  static int ima_release_policy(struct inode *inode, struct file *file)
> >  {
> > const char *cause = valid_policy ? "completed" : "failed";
> > +   int policy_write = (file->f_flags & O_WRONLY);
> > +
> > +   if (!policy_write)
> > +   return 0;
> > 
> > pr_info("IMA: policy update %s\n", cause);
> > integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> > @@ -342,9 +361,9 @@ static int ima_release_policy(struct inode *inode, 
> > struct 

Re: [PATCH v4 2/3] Create IMA machine owner keys (MOK) and blacklist keyrings;

[Petko Manolov]

On 15-10-20 10:13:34, Mimi Zohar wrote:
> On Tue, 2015-10-20 at 16:24 +0300, Petko Manolov wrote:
> > 
> > The code does not ties these keyrings around IMA.  The way i've done it, 
> > they are actually system wide keyrings and any other subsystem may use them.
> > 
> > Since there was no general discussion (hence agreement) that the Linux 
> > kernel need these keyrings i've put them within the IMA config options.  
> > For 
> > now only IMA users interested in creating CA hierarchy should use them.
> > 
> > > Basically, I'm trying to better understand the use case scenario.
> > 
> > To build CA hierarchy we basically need two things:
> > 
> > - system wide keyring writable at runtime, i.e. .ima_mok;
> > - system wide blacklist keyring writable at runtime, i.e. .blacklist;
> > 
> > .system is read-only and populated at kernel build time.  During the 
> > lifetime (as in runtime) of the machine we need to dynamically add tenant's 
> > certificates and IMA keys.  Without .ima_mok this can not be done easily 
> > because most of tenant's certificates are not available at krenel build 
> > time.  Most of these have one year of validity and must be replaced, while 
> > the router's uptime is typically much longer.
> > 
> > While the current kernel key code handles CA expiration it does not handle 
> > blacklisted certificates or keys.  The patch add checks to this keyring so 
> > any CA that has been compromised can't harm the system.
> > 
> > .ima_mok and .blacklist are IMA-enabled features for the moment not by 
> > semantics, but by policy.  I do think they should become system wide one 
> > day 
> > and there are good reasons to do so.
> 
> All very good!  As we discussed, this can and should be upstreamed initially 
> for IMA.
> 
> My question, however, has more to do with the last paragraph of the patch 
> description which says, "IMA blacklist keyring contains all revoked IMA keys. 
>  
> It is consulted before any other keyring.  If the search is successful the 
> requested operation is rejected and error is returned to the caller."
> 
> If the IMA blacklist keyring is for ALL IMA keys, those on the the original 
> .ima keyring and those on the .ima_mok keyring, then why is the Kconfig tied 
> to the .ima_mok keyring?  Does it make sense to have a blacklist keyring 
> without the .ima_mok keyring?  Should there be a separate Kconfig blacklist 
> keyring option?

Since having a proper CA hierarchy means access to both keyrings i never 
thought 
about separating them.  The blacklist keyring should be functional without it's 
counterpart so yes, i think it should be possible to have option for each of 
them, i.e. one for .ima_mok and one for .blacklist.

I am OK with finer granularity of the IMA options.  I wonder, though, whether 
the casual user will grasp the idea.

In short - do you want me to add separate options for the two keyrings in 
Kconfig?


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: [PATCH v4 2/3] Create IMA machine owner keys (MOK) and blacklist keyrings;

[Petko Manolov]

On 15-10-20 14:32:10, Mimi Zohar wrote:
> On Tue, 2015-10-20 at 18:33 +0300, Petko Manolov wrote:
> > 
> > As far as i know there is no concept of write-once to a keyring in the 
> > kernel.  David will correct me if i am wrong.  I wonder how hard would it 
> > be 
> > to add such functionality, in case it is missing?
> > 
> > Ideally a revoked key should stay in .blacklist until it expire or the 
> > system is rebooted.
> 
> Keys currently revoked return -EKEYREVOKED for a certain amount of time, 
> before being garbage collected.  Perhaps for trusted keys we could piggy back 
> on this option, returning -EKEYREVOKED, but prevent them from being garbage 
> collected?

I need to think about this.  Should -EKEYREVOKED be the same as -ENOKEY in this 
case?  I guess the end result is pretty much the same from IMA view point, but 
there may be a requirement to list all revoked keys...


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: [PATCH v4 2/3] Create IMA machine owner keys (MOK) and blacklist keyrings;

[Mimi Zohar]

On Tue, 2015-10-20 at 18:33 +0300, Petko Manolov wrote:
> On 15-10-20 11:21:43, Mimi Zohar wrote:
> > On Tue, 2015-10-20 at 17:43 +0300, Petko Manolov wrote:

> 
> > Thinking about the blacklist keyring some more...  My concern is more that 
> > keys can be added and removed at run time from either of the .ima or the 
> > ima_mok keyrings.  The need for a blacklist keyring is to prevent the key 
> > from 
> > being removed and at a later point re-added. Unfortunately, keys can be 
> > added 
> > and removed similarly from the blacklist keyring as well.  Unless keys can 
> > be 
> > added, without the ability of removing them, I'm not sure of the benefit of 
> > a 
> > blacklist keyring.  I assume adding and removing keys requires the same 
> > write 
> > privilege.  (cc'ing David Howells)
> 
> As far as i know there is no concept of write-once to a keyring in the 
> kernel.  
> David will correct me if i am wrong.  I wonder how hard would it be to add 
> such 
> functionality, in case it is missing?
> 
> Ideally a revoked key should stay in .blacklist until it expire or the system 
> is 
> rebooted.

Keys currently revoked return -EKEYREVOKED for a certain amount of time,
before being garbage collected.  Perhaps for trusted keys we could piggy
back on this option, returning -EKEYREVOKED, but prevent them from being
garbage collected?

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: [PATCH v4 2/3] Create IMA machine owner keys (MOK) and blacklist keyrings;

[Mimi Zohar]

On Tue, 2015-10-20 at 21:42 +0300, Petko Manolov wrote:
> On 15-10-20 14:32:10, Mimi Zohar wrote:
> > On Tue, 2015-10-20 at 18:33 +0300, Petko Manolov wrote:
> > > 
> > > As far as i know there is no concept of write-once to a keyring in the 
> > > kernel.  David will correct me if i am wrong.  I wonder how hard would it 
> > > be 
> > > to add such functionality, in case it is missing?
> > > 
> > > Ideally a revoked key should stay in .blacklist until it expire or the 
> > > system is rebooted.
> > 
> > Keys currently revoked return -EKEYREVOKED for a certain amount of time, 
> > before being garbage collected.  Perhaps for trusted keys we could piggy 
> > back 
> > on this option, returning -EKEYREVOKED, but prevent them from being garbage 
> > collected?
> 
> I need to think about this.  Should -EKEYREVOKED be the same as -ENOKEY in 
> this 
> case?  I guess the end result is pretty much the same from IMA view point, 
> but 
> there may be a requirement to list all revoked keys...

When checking the blacklist, getting -EKEYREVOKED is definitely
different than -ENOKEY.

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: [RFC PATCH v3 2/5] lsm: introduce hooks for kdbus

[Stephen Smalley]

On Mon, Oct 19, 2015 at 6:29 PM, Paul Moore  wrote:
> On Friday, October 09, 2015 10:56:12 AM Stephen Smalley wrote:
>> On 10/07/2015 07:08 PM, Paul Moore wrote:
>> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
>> > index ef63d65..1cb87b3 100644
>> > --- a/ipc/kdbus/connection.c
>> > +++ b/ipc/kdbus/connection.c
>> > @@ -108,6 +109,14 @@ static struct kdbus_conn *kdbus_conn_new(struct
>> > kdbus_ep *ep,>
>> > if (!owner && (creds || pids || seclabel))
>> >
>> > return ERR_PTR(-EPERM);
>> >
>> > +   ret = security_kdbus_conn_new(get_cred(file->f_cred),
>>
>> You only need to use get_cred() if saving a reference; otherwise, you'll
>> leak one here.
>
> Yes, that was a typo on my part, thanks.
>
>> Also, do we want file->f_cred here or ep->bus->node.creds (the latter is
>> what is used by their own checks; the former is typically the same as
>> current cred IIUC).  For that matter, what about ep->node.creds vs ep->bus-
>> node.creds vs. ep->bus->domain->node.creds?  Can they differ?  Do we care?
>
> We don't want file->f_cred, per our previous discussions.  I was working on
> this patchset in small chunks and while I added credential storing in the
> nodes, I forgot to update the hooks before I hit send, my apologies.
>
> My current thinking is to pass both the endpoint and bus credentials, as I
> believe they can differ.  Both the bus and the endpoint inherit their security
> labels from their creator and while I don't have any specifics, I think it is
> reasonable to imagine those two processes having different security labels.
> Assuming we pass both credentials down to the LSM, I'm currently thinking of
> the following SELinux access controls for this hook:
>
>   allow  bus_t:kdbus { connect };
>   allow  ep_t:kdbus { use privileged activator monitor policy };

I think it would be simpler to apply an associate check when the
endpoint is created between the endpoint label and the bus label
(which will typically be the same), and then only check based on
endpoint label for all subsequent permission checks involving that
endpoint.  Then you don't have to worry about which label to use for
all the other permission checks. And you get finer-grained control -
per-endpoint rather than only per-bus.  In the common case, the labels
will be the same and it won't matter, but if you want to support some
MAC form of their 'custom endpoints' with further restrictions, you
can do that by labeling the endpoints uniquely and using those labels
in your permission checks.

Otherwise, I have to ask whether you need two different classes above
(bus vs endpoint), and whether the privileged/activator/monitor/policy
permissions properly belong with the endpoint label or the bus label.
At least some of those are bus-wide properties, not endpoint-specific,
but that's ok as long as we have established a relationship between
the endpoint label and bus label.

> ... besides the additional label, I added the kdbus:use permission and dropped
> the kdbus:owner permission.  Considering that the endpoint label, ep_t, in the
> examples above, could be different from the current process, it seemed
> reasonable to want to control that interaction and I felt the fd:use
> permission was the closest existing control so I reused the permission name.
> I decided to drop the "owner" permission as it really wasn't the useful for
> anything anymore, it simply indicates that the current task is the DAC owner
> of the endpoint.

Can you 'use' an endpoint in any way other than to connect via it?
If not, I'd just call that connect (won't conflict if you get rid of
the separate bus check above), or distinguish it via separate classes
or as connectthrough vs connectto.

conn->owner is used to determine whether the caller can fake
credentials, skip kdbus policy checking, create an activator, monitor,
or policy holder connection, etc.  Our options are:
1. Apply a SELinux check when it is set to see if the caller is
allowed to own the bus based on MAC labels and policy, and if not,
refuse to create the connection (that's what checking the owner
permission was doing).

2. Separately apply MAC checks over each of those abilities (fake
creds, override policy, create an activator, monitor, or policy
holder, etc) when there is an attempt to exercise them (not all during
connection creation), and selectively deny that ability.  More
invasive, more potential for breakage for applications that don't
expect failure if they could create the connection in the first place.

3. Treat faking of DAC credentials and skipping of kdbus policy
checking as not of interest to MAC, leaving it only controlled by the
existing uid match or CAP_IPC_OWNER check.  Simple, but seems to lose
some of the potential benefit of using SELinux for confining processes
using kdbus.

privileged actually seems less interesting than owner these days,
although I haven't done a thorough analysis.

IIUC, creating a monitor or policy holder 

Re: [PULL] Smack - Changes for 4.4

[James Morris]

On Mon, 19 Oct 2015, Casey Schaufler wrote:

> The following changes since commit 049e6dde7e57f0054fdc49102e7ef4830c698b46:
> 
>   Linux 4.3-rc4 (2015-10-04 16:57:17 +0100)
> 
> are available in the git repository at:
> 
>   https://github.com/cschaufler/smack-next.git smack-for-4.4
> 

Pulled, thanks.

-- 
James Morris


--
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 3/3] Allows reading back the current IMA policy;

[Mimi Zohar]

On Tue, 2015-10-20 at 15:10 +0300, Petko Manolov wrote:
> On 15-10-20 08:00:29, Mimi Zohar wrote:
> > On Tue, 2015-10-20 at 10:26 +0300, Petko Manolov wrote:
> > > On 15-10-19 14:21:42, Mimi Zohar wrote:
> > > > On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov wrote:
> > > > > When in development it is useful to read back the IMA policy.  This 
> > > > > patch
> > > > > provides the functionality.  However, this is a potential security 
> > > > > hole so
> > > > > it should not be used in production-grade kernels.
> > > >  
> > > > Like the other IMA securityfs files, only root would be able to read it.
> > > > Once we start allowing additional rules to be appended to the policy,
> > > > being able to view the resulting policy is important.  Is there a reason
> > > > for limiting this option to development?
> > > 
> > > I have not considered allowing non-root users to read the policy - i was 
> > > merely 
> > > cleaning up the Zbigniew's patch.  I guess it might be useful to be able 
> > > to read 
> > > the policy when in development mode.
> > 
> > I guess I wasn't clear.  I don't have a problem with the patch itself, just 
> > with the patch description.  What is this "security hole" that the option 
> > should ONLY be configured for development?  Only privileged users can view 
> > the 
> > policy.  I don't see the problem with configuring it in general.  Please 
> > remove the comment.
> 
> By "security hole" i mean being able to read it at all.  Root or non-root.  
> Knowing what the IMA policy is may give the attacker an idea how to 
> circumvent 
> it.  I used stronger words in order to attract the user's attention and 
> consider 
> carefully what the implications are when enabling this option.
> 
> However, i do not insist on keeping this comment.  I will remove it or 
> re-word 
> it if you think it is nonsensical in it's present form.
> 
> BTW, i still think it is a good idea that only the root user have access to 
> the 
> IMA policy.  Unless i hear otherwise i am planning to keep the current 
> functionality.

Exactly!  Because only privileged users (eg. root) have access to
securityfs files, I don't see the security concern.

> > Since responding, I've enabled this feature.  Very nice!
> 
> Have you tried it?

Yes, being able to see the existing policy is nice.

BTW, I haven't compared this patch with the original one yet.  Unless
there were so many changes so that it isn't the same patch anymore, the
patch author should be Zbigniew Jasiński .  Any
changes you made would be listed in the change log.

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: [PATCH v4 2/3] Create IMA machine owner keys (MOK) and blacklist keyrings;

[Petko Manolov]

On 15-10-20 08:48:20, Mimi Zohar wrote:
> On Tue, 2015-10-20 at 10:22 +0300, Petko Manolov wrote:
> > 
> > This is all we've been using the blacklist keyring so far.  By semantics it 
> > is system-wide keyring so maybe the commit message should be changed.  
> > Nothing prevents others from using it.
> > 
> > The problem is that an IMA option enables the creation of this keyring, 
> > which makes it IMA specific for the moment.  If it is decided that the 
> > blacklist keyring should be detached from it's IMA bonds then i guess two 
> > things should happen: 1) broader discussion (perhaps involving David); and 
> > 2) modifying the patches;
> > 
> > BTW, same applies for the MOK keyring.  If you want these to be more 
> > generally used i assume a discussion is in order.
> 
> I understand these keyrings could be useful for the more general case, but 
> lets first discuss IMA.  I'm trying to understand why the blacklist is tied 
> so 
> closely to just the .ima_mok keyring in the Kconfig.  Is the need for 
> blacklisting keys any less if the key is on the .ima keyring vs. the .ima_mok 
> keyring?

The code does not ties these keyrings around IMA.  The way i've done it, they 
are actually system wide keyrings and any other subsystem may use them.

Since there was no general discussion (hence agreement) that the Linux kernel 
need these keyrings i've put them within the IMA config options.  For now only 
IMA users interested in creating CA hierarchy should use them.

> Basically, I'm trying to better understand the use case scenario.

To build CA hierarchy we basically need two things:

- system wide keyring writable at runtime, i.e. .ima_mok;
- system wide blacklist keyring writable at runtime, i.e. .blacklist;

.system is read-only and populated at kernel build time.  During the lifetime 
(as in runtime) of the machine we need to dynamically add tenant's certificates 
and IMA keys.  Without .ima_mok this can not be done easily because most of 
tenant's certificates are not available at krenel build time.  Most of these 
have one year of validity and must be replaced, while the router's uptime is 
typically much longer.

While the current kernel key code handles CA expiration it does not handle 
blacklisted certificates or keys.  The patch add checks to this keyring so any 
CA that has been compromised can't harm the system.

.ima_mok and .blacklist are IMA-enabled features for the moment not by 
semantics, but by policy.  I do think they should become system wide one day 
and 
there are good reasons to do so.


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: [PATCH v4 3/3] Allows reading back the current IMA policy;

[Petko Manolov]

On 15-10-20 09:03:19, Mimi Zohar wrote:
> On Tue, 2015-10-20 at 15:10 +0300, Petko Manolov wrote:
> > 
> > By "security hole" i mean being able to read it at all.  Root or non-root.  
> > Knowing what the IMA policy is may give the attacker an idea how to 
> > circumvent it.  I used stronger words in order to attract the user's 
> > attention and consider carefully what the implications are when enabling 
> > this option.
> > 
> > However, i do not insist on keeping this comment.  I will remove it or 
> > re-word it if you think it is nonsensical in it's present form.
> > 
> > BTW, i still think it is a good idea that only the root user have access to 
> > the IMA policy.  Unless i hear otherwise i am planning to keep the current 
> > functionality.
> 
> Exactly!  Because only privileged users (eg. root) have access to securityfs 
> files, I don't see the security concern.

OK, so i'll remove the scary warning. :)

> > > Since responding, I've enabled this feature.  Very nice!
> > 
> > Have you tried it?
> 
> Yes, being able to see the existing policy is nice.
> 
> BTW, I haven't compared this patch with the original one yet.  Unless there 
> were so many changes so that it isn't the same patch anymore, the patch 
> author 
> should be Zbigniew Jasiński .  Any changes you made 
> would be listed in the change log.

The patch is modified significantly, but not to the point to be a new one.  The 
changes mostly address your comments about using raw strings instead of those 
that already exist.  I feel that there's more to be done there, but will wait 
for you to review it first.

Unfortunately i never heard from Zbigniew even though he's copied in this 
thread 
since the beginning.  I would very much like to hear his comments.


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