date:20151026

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

2015-10-26 Thread Mimi Zohar

On Sat, 2015-10-24 at 17:04 +0300, Dmitry Kasatkin wrote:
> On Sat, Oct 24, 2015 at 3:28 PM, Petko Manolov  wrote:
> > On 15-10-23 20:13:41, Dmitry Kasatkin wrote:
> >> On Fri, Oct 23, 2015 at 3:29 PM, Petko Manolov  wrote:
> >> >
> >> > 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.
> >
> > You've got a point here.  If we get rid of the busy flag we'll have to 
> > block at
> > open() instead of write() in order to keep the "original" semantics.
> >
> 
> No, that was not my point.
> The point was that there is no another/simultaneous  policy writer in reality.
> 
> >> 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.
> >
> > Nope, that's not the only scenario.  Imagine a machine with multiple 
> > tenants and
> > huge uptime.  Imagine also that these tenants want to run their own 
> > software on
> > this machine.  Policy write may occur at any time.
> >
> 
> No, I cannot imagine that unrelated processes can write/update policy
> simultaneously at any time.
> I can imagine that some device/system management software does policy update.
> 
> May be you could give more exact use case.

His usage of the term "multi-tenant" is not the traditional one of
having to protect one tenant from another tenant, but where all tenants
are equally trusted.  Each tenant is allowed to update their own keys on
the .ima_mok/.ima keyrings system and append new rules to the policy.
The tenants are protecting against a file of unknown origin from being
executed, or possibly accessed.

Think of a system owned by a company/university with different
departments all wanting to run code on it.  Not only does the system
owner not want to get involved each time a new piece of code has to be
signed, but wants to easily be able to associate the code on the system
with a dept.  So the system owner delegates authority to the different
departments by signing their certificate.

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 1/3] Enable multiple writes to the IMA policy;

2015-10-26 Thread Mimi Zohar

On Tue, 2015-10-27 at 00:03 +0200, Petko Manolov wrote:
> On 15-10-26 22:39:28, Dmitry Kasatkin wrote:

> > Can you please still explain when multiple policy writers can content? I 
> > 100% 
> > understand the role of mutex
> 
> Ignore the high level requirements for the moment.  Every time you have a 
> contended resource you need to protect it from concurrent writers.  IMA 
> policy 
> is read way more frequently than it is been written.  Just once in the past, 
> now 
> a few times more.

Right.  We all agree that only one process can append new rules at a
time.  The open currently fails with -EBUSY.  If the policy isn't being
updated frequently and there isn't any contention for writing the
policy, the question is why change the existing behavior (by defining a
new mutex)?

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

2015-10-26 Thread Dmitry Kasatkin

Hi,

Updated in the patch.

http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/log/?h=ima-next

Dmitry

On Fri, Oct 23, 2015 at 9:30 PM, Mimi Zohar  wrote:
> 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, _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, _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(_type_encrypted, EVMKEY, NULL);
>>   if (IS_ERR(evm_key))
>> @@ -250,12 +286,9 @@ int evm_init_key(void)
>>
>>   down_read(_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(_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) ||

Re: [PATCHv3 3/6] evm: enable EVM when X509 certificate is loaded

2015-10-26 Thread Dmitry Kasatkin

Hi,

I added error printing to the patch

http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/log/?h=ima-next

Dmitry


On Fri, Oct 23, 2015 at 9:31 PM, Mimi Zohar  wrote:
> 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 = _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");
>
>



-- 
Thanks,
Dmitry
--
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

[PATCH v3 2/7] selinux: Add accessor functions for inode->i_security

2015-10-26 Thread Andreas Gruenbacher

Add functions dentry_security and inode_security for accessing
inode->i_security.  These functions initially don't do much, but they
will later be used to revalidate the security labels when necessary.

Signed-off-by: Andreas Gruenbacher 
---
 security/selinux/hooks.c | 101 ++-
 1 file changed, 57 insertions(+), 44 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index fc8f626..65e8689 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -241,6 +241,24 @@ static int inode_alloc_security(struct inode *inode)
return 0;
 }
 
+/*
+ * Get the security label of a dentry's inode.
+ */
+static struct inode_security_struct *dentry_security(struct dentry *dentry)
+{
+   struct inode *inode = d_backing_inode(dentry);
+
+   return inode->i_security;
+}
+
+/*
+ * Get the security label of an inode.
+ */
+static struct inode_security_struct *inode_security(struct inode *inode)
+{
+   return inode->i_security;
+}
+
 static void inode_free_rcu(struct rcu_head *head)
 {
struct inode_security_struct *isec;
@@ -564,8 +582,8 @@ static int selinux_get_mnt_opts(const struct super_block 
*sb,
opts->mnt_opts_flags[i++] = DEFCONTEXT_MNT;
}
if (sbsec->flags & ROOTCONTEXT_MNT) {
-   struct inode *root = d_backing_inode(sbsec->sb->s_root);
-   struct inode_security_struct *isec = root->i_security;
+   struct dentry *root = sbsec->sb->s_root;
+   struct inode_security_struct *isec = dentry_security(root);
 
rc = security_sid_to_context(isec->sid, , );
if (rc)
@@ -620,8 +638,8 @@ static int selinux_set_mnt_opts(struct super_block *sb,
int rc = 0, i;
struct superblock_security_struct *sbsec = sb->s_security;
const char *name = sb->s_type->name;
-   struct inode *inode = d_backing_inode(sbsec->sb->s_root);
-   struct inode_security_struct *root_isec = inode->i_security;
+   struct dentry *root = sbsec->sb->s_root;
+   struct inode_security_struct *root_isec = dentry_security(root);
u32 fscontext_sid = 0, context_sid = 0, rootcontext_sid = 0;
u32 defcontext_sid = 0;
char **mount_options = opts->mnt_opts;
@@ -852,8 +870,8 @@ static int selinux_cmp_sb_context(const struct super_block 
*oldsb,
if ((oldflags & DEFCONTEXT_MNT) && old->def_sid != new->def_sid)
goto mismatch;
if (oldflags & ROOTCONTEXT_MNT) {
-   struct inode_security_struct *oldroot = 
d_backing_inode(oldsb->s_root)->i_security;
-   struct inode_security_struct *newroot = 
d_backing_inode(newsb->s_root)->i_security;
+   struct inode_security_struct *oldroot = 
dentry_security(oldsb->s_root);
+   struct inode_security_struct *newroot = 
dentry_security(newsb->s_root);
if (oldroot->sid != newroot->sid)
goto mismatch;
}
@@ -903,17 +921,14 @@ static int selinux_sb_clone_mnt_opts(const struct 
super_block *oldsb,
if (!set_fscontext)
newsbsec->sid = sid;
if (!set_rootcontext) {
-   struct inode *newinode = d_backing_inode(newsb->s_root);
-   struct inode_security_struct *newisec = 
newinode->i_security;
+   struct inode_security_struct *newisec = 
dentry_security(newsb->s_root);
newisec->sid = sid;
}
newsbsec->mntpoint_sid = sid;
}
if (set_rootcontext) {
-   const struct inode *oldinode = d_backing_inode(oldsb->s_root);
-   const struct inode_security_struct *oldisec = 
oldinode->i_security;
-   struct inode *newinode = d_backing_inode(newsb->s_root);
-   struct inode_security_struct *newisec = newinode->i_security;
+   const struct inode_security_struct *oldisec = 
dentry_security(oldsb->s_root);
+   struct inode_security_struct *newisec = 
dentry_security(newsb->s_root);
 
newisec->sid = oldisec->sid;
}
@@ -1623,7 +1638,7 @@ static int inode_has_perm(const struct cred *cred,
return 0;
 
sid = cred_sid(cred);
-   isec = inode->i_security;
+   isec = inode_security(inode);
 
return avc_has_perm(sid, isec->sid, isec->sclass, perms, adp);
 }
@@ -1712,13 +1727,13 @@ out:
 /*
  * Determine the label for an inode that might be unioned.
  */
-static int selinux_determine_inode_label(const struct inode *dir,
+static int selinux_determine_inode_label(struct inode *dir,
 const struct qstr *name,
 u16 tclass,
 u32 *_new_isid)
 {
const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
-

[PATCH v3 1/7] selinux: Remove unused variable in selinux_inode_init_security

2015-10-26 Thread Andreas Gruenbacher

Signed-off-by: Andreas Gruenbacher 
---
 security/selinux/hooks.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e4369d8..fc8f626 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2756,13 +2756,11 @@ static int selinux_inode_init_security(struct inode 
*inode, struct inode *dir,
   void **value, size_t *len)
 {
const struct task_security_struct *tsec = current_security();
-   struct inode_security_struct *dsec;
struct superblock_security_struct *sbsec;
u32 sid, newsid, clen;
int rc;
char *context;
 
-   dsec = dir->i_security;
sbsec = dir->i_sb->s_security;
 
sid = tsec->sid;
-- 
2.5.0

--
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

[PATCH v3 3/7] selinux: Get rid of file_path_has_perm

2015-10-26 Thread Andreas Gruenbacher

Use path_has_perm directly instead.

Signed-off-by: Andreas Gruenbacher 
---
 security/selinux/hooks.c | 18 +++---
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 65e8689..d6b4dc9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1673,18 +1673,6 @@ static inline int path_has_perm(const struct cred *cred,
return inode_has_perm(cred, inode, av, );
 }
 
-/* Same as path_has_perm, but uses the inode from the file struct. */
-static inline int file_path_has_perm(const struct cred *cred,
-struct file *file,
-u32 av)
-{
-   struct common_audit_data ad;
-
-   ad.type = LSM_AUDIT_DATA_PATH;
-   ad.u.path = file->f_path;
-   return inode_has_perm(cred, file_inode(file), av, );
-}
-
 /* Check whether a task can use an open file descriptor to
access an inode in a given way.  Check access to the
descriptor itself, and then use dentry_has_perm to
@@ -2371,14 +2359,14 @@ static inline void flush_unauthorized_files(const 
struct cred *cred,
struct tty_file_private *file_priv;
 
/* Revalidate access to controlling tty.
-  Use file_path_has_perm on the tty path directly
+  Use path_has_perm on the tty path directly
   rather than using file_has_perm, as this particular
   open file may belong to another process and we are
   only interested in the inode-based check here. */
file_priv = list_first_entry(>tty_files,
struct tty_file_private, list);
file = file_priv->file;
-   if (file_path_has_perm(cred, file, FILE__READ | 
FILE__WRITE))
+   if (path_has_perm(cred, >f_path, FILE__READ | 
FILE__WRITE))
drop_tty = 1;
}
spin_unlock(_files_lock);
@@ -3537,7 +3525,7 @@ static int selinux_file_open(struct file *file, const 
struct cred *cred)
 * new inode label or new policy.
 * This check is not redundant - do not remove.
 */
-   return file_path_has_perm(cred, file, open_file_to_av(file));
+   return path_has_perm(cred, >f_path, open_file_to_av(file));
 }
 
 /* task security operations */
-- 
2.5.0

--
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

[PATCH v3 5/7] security: Add hook to invalidate inode security labels

2015-10-26 Thread Andreas Gruenbacher

Add a hook to invalidate an inode's security label when the cached
information becomes invalid.

Implement the new hook in selinux: set a flag when a security label becomes
invalid.  When hitting a security label which has been marked as invalid in
inode_has_perm, try reloading the label.

If an inode does not have any dentries attached, we cannot reload its
security label because we cannot use the getxattr inode operation.  In that
case, continue using the old, invalid label until a dentry becomes
available.

Signed-off-by: Andreas Gruenbacher 
---
 include/linux/lsm_hooks.h |  6 ++
 include/linux/security.h  |  5 +
 security/security.c   |  8 
 security/selinux/hooks.c  | 30 --
 security/selinux/include/objsec.h |  6 ++
 5 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ec3a6ba..945ae1d 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1261,6 +1261,10 @@
  * audit_rule_init.
  * @rule contains the allocated rule
  *
+ * @inode_invalidate_secctx:
+ * Notify the security module that it must revalidate the security context
+ * of an inode.
+ *
  * @inode_notifysecctx:
  * Notify the security module of what the security context of an inode
  * should be.  Initializes the incore security context managed by the
@@ -1516,6 +1520,7 @@ union security_list_options {
int (*secctx_to_secid)(const char *secdata, u32 seclen, u32 *secid);
void (*release_secctx)(char *secdata, u32 seclen);
 
+   void (*inode_invalidate_secctx)(struct inode *inode);
int (*inode_notifysecctx)(struct inode *inode, void *ctx, u32 ctxlen);
int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32 ctxlen);
int (*inode_getsecctx)(struct inode *inode, void **ctx, u32 *ctxlen);
@@ -1757,6 +1762,7 @@ struct security_hook_heads {
struct list_head secid_to_secctx;
struct list_head secctx_to_secid;
struct list_head release_secctx;
+   struct list_head inode_invalidate_secctx;
struct list_head inode_notifysecctx;
struct list_head inode_setsecctx;
struct list_head inode_getsecctx;
diff --git a/include/linux/security.h b/include/linux/security.h
index 2f4c1f7..9692571 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -353,6 +353,7 @@ int security_secid_to_secctx(u32 secid, char **secdata, u32 
*seclen);
 int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
 void security_release_secctx(char *secdata, u32 seclen);
 
+void security_inode_invalidate_secctx(struct inode *inode);
 int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
 int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
 int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
@@ -1093,6 +1094,10 @@ static inline void security_release_secctx(char 
*secdata, u32 seclen)
 {
 }
 
+static inline void security_inode_invalidate_secctx(struct inode *inode)
+{
+}
+
 static inline int security_inode_notifysecctx(struct inode *inode, void *ctx, 
u32 ctxlen)
 {
return -EOPNOTSUPP;
diff --git a/security/security.c b/security/security.c
index 46f405c..e4371cd 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1161,6 +1161,12 @@ void security_release_secctx(char *secdata, u32 seclen)
 }
 EXPORT_SYMBOL(security_release_secctx);
 
+void security_inode_invalidate_secctx(struct inode *inode)
+{
+   call_void_hook(inode_invalidate_secctx, inode);
+}
+EXPORT_SYMBOL(security_inode_invalidate_secctx);
+
 int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
 {
return call_int_hook(inode_notifysecctx, 0, inode, ctx, ctxlen);
@@ -1763,6 +1769,8 @@ struct security_hook_heads security_hook_heads = {
LIST_HEAD_INIT(security_hook_heads.secctx_to_secid),
.release_secctx =
LIST_HEAD_INIT(security_hook_heads.release_secctx),
+   .inode_invalidate_secctx =
+   LIST_HEAD_INIT(security_hook_heads.inode_invalidate_secctx),
.inode_notifysecctx =
LIST_HEAD_INIT(security_hook_heads.inode_notifysecctx),
.inode_setsecctx =
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2a04729..f93dafd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -820,7 +820,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
goto out;
 
root_isec->sid = rootcontext_sid;
-   root_isec->initialized = 1;
+   root_isec->initialized = LABEL_INITIALIZED;
}
 
if (defcontext_sid) {
@@ -1308,11 +1308,11 @@ static int inode_doinit_with_dentry(struct inode 
*inode, struct dentry *opt_dent
unsigned len = 0;
int rc = 0;
 
-   if (isec->initialized)

[PATCH v3 0/7] Inode security label invalidation

2015-10-26 Thread Andreas Gruenbacher

Here is another version of the patch queue to make gfs2 and similar file
systems work with SELinux.  As suggested by Stephen Smalley [*], the relevant
uses of inode->security are wrapped in function calls that try to revalidate
invalid labels.

  [*] http://marc.info/?l=linux-kernel=144416710207686=2

The patches are looking good from my point of view; is there anything else that
needs addressing?

Does SELinux have test suites that these patches could be tested agains?

Thanks,
Andreas

Andreas Gruenbacher (7):
  selinux: Remove unused variable in selinux_inode_init_security
  selinux: Add accessor functions for inode->i_security
  selinux: Get rid of file_path_has_perm
  selinux: Push dentry down from {dentry,path,file}_has_perm
  security: Add hook to invalidate inode security labels
  selinux: Revalidate invalid inode security labels
  gfs2: Invalide security labels of inodes when they go invalid

 fs/gfs2/glops.c   |   2 +
 include/linux/lsm_hooks.h |   6 ++
 include/linux/security.h  |   5 +
 security/security.c   |   8 ++
 security/selinux/hooks.c  | 213 ++
 security/selinux/include/objsec.h |   6 ++
 6 files changed, 152 insertions(+), 88 deletions(-)

-- 
2.5.0

--
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

[PATCH v3 4/7] selinux: Push dentry down from {dentry,path,file}_has_perm

2015-10-26 Thread Andreas Gruenbacher

In dentry_has_perm, path_has_perm, and file_has_perm, push the dentry down
to before avc_has_perm so that dentry_security can be used instead of
inode_security.  Since inode_has_perm now takes a dentry, rename it to
__dentry_has_perm.

Signed-off-by: Andreas Gruenbacher 
---
 security/selinux/hooks.c | 29 +
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d6b4dc9..2a04729 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1621,56 +1621,54 @@ static int task_has_system(struct task_struct *tsk,
SECCLASS_SYSTEM, perms, NULL);
 }
 
-/* Check whether a task has a particular permission to an inode.
-   The 'adp' parameter is optional and allows other audit
+/* Check whether a task has a particular permission to a dentry's
+   inode.  The 'adp' parameter is optional and allows other audit
data to be passed (e.g. the dentry). */
-static int inode_has_perm(const struct cred *cred,
- struct inode *inode,
- u32 perms,
- struct common_audit_data *adp)
+static int __dentry_has_perm(const struct cred *cred,
+struct dentry *dentry,
+u32 perms,
+struct common_audit_data *adp)
 {
struct inode_security_struct *isec;
u32 sid;
 
validate_creds(cred);
 
-   if (unlikely(IS_PRIVATE(inode)))
+   if (unlikely(IS_PRIVATE(d_backing_inode(dentry
return 0;
 
sid = cred_sid(cred);
-   isec = inode_security(inode);
+   isec = dentry_security(dentry);
 
return avc_has_perm(sid, isec->sid, isec->sclass, perms, adp);
 }
 
-/* Same as inode_has_perm, but pass explicit audit data containing
+/* Same as __dentry_has_perm, but pass explicit audit data containing
the dentry to help the auditing code to more easily generate the
pathname if needed. */
 static inline int dentry_has_perm(const struct cred *cred,
  struct dentry *dentry,
  u32 av)
 {
-   struct inode *inode = d_backing_inode(dentry);
struct common_audit_data ad;
 
ad.type = LSM_AUDIT_DATA_DENTRY;
ad.u.dentry = dentry;
-   return inode_has_perm(cred, inode, av, );
+   return __dentry_has_perm(cred, dentry, av, );
 }
 
-/* Same as inode_has_perm, but pass explicit audit data containing
+/* Same as __dentry_has_perm, but pass explicit audit data containing
the path to help the auditing code to more easily generate the
pathname if needed. */
 static inline int path_has_perm(const struct cred *cred,
const struct path *path,
u32 av)
 {
-   struct inode *inode = d_backing_inode(path->dentry);
struct common_audit_data ad;
 
ad.type = LSM_AUDIT_DATA_PATH;
ad.u.path = *path;
-   return inode_has_perm(cred, inode, av, );
+   return __dentry_has_perm(cred, path->dentry, av, );
 }
 
 /* Check whether a task can use an open file descriptor to
@@ -1686,7 +1684,6 @@ static int file_has_perm(const struct cred *cred,
 u32 av)
 {
struct file_security_struct *fsec = file->f_security;
-   struct inode *inode = file_inode(file);
struct common_audit_data ad;
u32 sid = cred_sid(cred);
int rc;
@@ -1706,7 +1703,7 @@ static int file_has_perm(const struct cred *cred,
/* av is zero if only checking access to the descriptor. */
rc = 0;
if (av)
-   rc = inode_has_perm(cred, inode, av, );
+   rc = __dentry_has_perm(cred, file->f_path.dentry, av, );
 
 out:
return rc;
-- 
2.5.0

--
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

[PATCH v3 6/7] selinux: Revalidate invalid inode security labels

2015-10-26 Thread Andreas Gruenbacher

When fetching inode's security label, check if they are still valid, and try
reloading invalid labels.  Reloading will fail when we are in RCU context which
doesn't allow sleeping, or when we can't find a dentry for the inode.
(Reloading happens via iop->getxattr which takes a dentry parameter.)

Signed-off-by: Andreas Gruenbacher 
---
 security/selinux/hooks.c | 47 +++
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f93dafd..61aead9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -241,22 +241,51 @@ static int inode_alloc_security(struct inode *inode)
return 0;
 }
 
+static int inode_doinit_with_dentry(struct inode *inode, struct dentry 
*opt_dentry);
+
 /*
- * Get the security label of a dentry's inode.
+ * Get an inode's security label.  When the label has been marked as invalid,
+ * try to reload it.  The @opt_dentry parameter should be set to a dentry of
+ * the inode; when no dentry is available, set it to NULL instead.  The @rcu
+ * parameter indicates when sleeping and thus reloading labels is not allowed;
+ * in that case, this function will return ERR_PTR(-ECHILD).
+ */
+static struct inode_security_struct *__inode_security(struct inode *inode,
+ struct dentry *opt_dentry,
+ bool rcu)
+{
+   struct inode_security_struct *isec = inode->i_security;
+
+   if (isec->initialized == LABEL_INVALID) {
+   if (rcu)
+   return ERR_PTR(-ECHILD);
+
+   /*
+* Try reloading the inode security label.  This will fail if
+* @opt_dentry is NULL and no dentry for this inode can be
+* found; in that case, we will continue using the old label.
+*/
+   inode_doinit_with_dentry(inode, opt_dentry);
+   }
+   return isec;
+}
+
+/*
+ * Get the security label of a dentry's inode.  Function may block.
  */
 static struct inode_security_struct *dentry_security(struct dentry *dentry)
 {
struct inode *inode = d_backing_inode(dentry);
 
-   return inode->i_security;
+   return __inode_security(inode, dentry, false);
 }
 
 /*
- * Get the security label of an inode.
+ * Get the security label of an inode.  Function may block.
  */
 static struct inode_security_struct *inode_security(struct inode *inode)
 {
-   return inode->i_security;
+   return __inode_security(inode, NULL, false);
 }
 
 static void inode_free_rcu(struct rcu_head *head)
@@ -362,8 +391,6 @@ static const char *labeling_behaviors[7] = {
"uses native labeling",
 };
 
-static int inode_doinit_with_dentry(struct inode *inode, struct dentry 
*opt_dentry);
-
 static inline int inode_doinit(struct inode *inode)
 {
return inode_doinit_with_dentry(inode, NULL);
@@ -2858,7 +2885,9 @@ static int selinux_inode_follow_link(struct dentry 
*dentry, struct inode *inode,
ad.type = LSM_AUDIT_DATA_DENTRY;
ad.u.dentry = dentry;
sid = cred_sid(cred);
-   isec = inode_security(inode);
+   isec = __inode_security(inode, NULL, rcu);
+   if (IS_ERR(isec))
+   return PTR_ERR(isec);
 
return avc_has_perm_flags(sid, isec->sid, isec->sclass, FILE__READ, ,
  rcu ? MAY_NOT_BLOCK : 0);
@@ -2910,7 +2939,9 @@ static int selinux_inode_permission(struct inode *inode, 
int mask)
perms = file_mask_to_av(inode->i_mode, mask);
 
sid = cred_sid(cred);
-   isec = inode_security(inode);
+   isec = __inode_security(inode, NULL, flags & MAY_NOT_BLOCK);
+   if (IS_ERR(isec))
+   return PTR_ERR(isec);
 
rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, );
audited = avc_audit_required(perms, , rc,
-- 
2.5.0

--
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;

2015-10-26 Thread Mimi Zohar

On Mon, 2015-10-26 at 16:01 +0200, Petko Manolov wrote:
> On 15-10-25 07:50:32, Mimi Zohar wrote:
> > On Sat, 2015-10-24 at 17:06 +0300, Dmitry Kasatkin wrote:
> > 
> > > > @@ -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.
> > 
> > Please don't remove this comment.
> 
> OK, i will leave it, but the beginning of the first sentence should be 
> changed 
> to reflect that the IMA policy _may_ change.

Right, please start the sentence with "The LSM policy can be ...".

> 
> > > > + * 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.
> > > >   
> > 
> > I'm not sure this comment is needed.  If it is needed, this should be moved 
> > to 
> > below the function description.
> 
> From personal experience, i like it when i see such comments, especially at 
> critical code section.  I'll move it down below if you don't like it there.

I prefer to start with the reason for the function.

> > > >   * 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, _policy_rules, list) {
> > > 
> > > 
> > > Use of "list_for_each_entry_safe" makes me having a doubt.
> > > 
> > > "safe" versions are used when entries are removed while walk.
> > > 
> > > If it is so, then entire RCU case is impossible?
> > 
> > Taking the lock here was to prevent modifying the LSM rule number multiple 
> > times.  Using the "safe" version was never needed.  The worst that can 
> > happen 
> > here is that the LSM rule number is updated multiple times.
> 
> "safe" is a remnant from the original version of the code.  I don't think it 
> is 
> necessary, but didn't want to make too many changes in one go.  In the worst 
> case the list traversal is a bit slower, which should not happen too often 
> anyway.

Right, this was my mistake.  Please feel free to change it.

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 1/3] Enable multiple writes to the IMA policy;

2015-10-26 Thread Petko Manolov

On 15-10-25 07:50:32, Mimi Zohar wrote:
> On Sat, 2015-10-24 at 17:06 +0300, Dmitry Kasatkin wrote:
> 
> > > @@ -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.
> 
> Please don't remove this comment.

OK, i will leave it, but the beginning of the first sentence should be changed 
to reflect that the IMA policy _may_ change.

> > > + * 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.
> > >   
> 
> I'm not sure this comment is needed.  If it is needed, this should be moved 
> to 
> below the function description.

>From personal experience, i like it when i see such comments, especially at 
critical code section.  I'll move it down below if you don't like it there.

> > >   * 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, _policy_rules, list) {
> > 
> > 
> > Use of "list_for_each_entry_safe" makes me having a doubt.
> > 
> > "safe" versions are used when entries are removed while walk.
> > 
> > If it is so, then entire RCU case is impossible?
> 
> Taking the lock here was to prevent modifying the LSM rule number multiple 
> times.  Using the "safe" version was never needed.  The worst that can happen 
> here is that the LSM rule number is updated multiple times.

"safe" is a remnant from the original version of the code.  I don't think it is 
necessary, but didn't want to make too many changes in one go.  In the worst 
case the list traversal is a bit slower, which should not happen too often 
anyway.



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 1/3] Enable multiple writes to the IMA policy;

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

Re: [PATCHv3 4/6] evm: provide a function to set EVM key from the kernel

Re: [PATCHv3 3/6] evm: enable EVM when X509 certificate is loaded

[PATCH v3 2/7] selinux: Add accessor functions for inode->i_security

[PATCH v3 1/7] selinux: Remove unused variable in selinux_inode_init_security

[PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[PATCH v3 5/7] security: Add hook to invalidate inode security labels

[PATCH v3 0/7] Inode security label invalidation

[PATCH v3 4/7] selinux: Push dentry down from {dentry,path,file}_has_perm

[PATCH v3 6/7] selinux: Revalidate invalid inode security labels

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

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

13 matches

Site Navigation

Mail list logo

Footer information