| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <37460a6a-da6c-3fdc-7460-0cec024e49db@schaufler-ca.com>
Date: Thu, 22 Apr 2021 08:46:20 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Roberto Sassu <roberto.sassu@...wei.com>,
"zohar@...ux.ibm.com" <zohar@...ux.ibm.com>,
"jmorris@...ei.org" <jmorris@...ei.org>,
"paul@...l-moore.com" <paul@...l-moore.com>
Cc: "linux-integrity@...r.kernel.org" <linux-integrity@...r.kernel.org>,
"linux-security-module@...r.kernel.org"
<linux-security-module@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"selinux@...r.kernel.org" <selinux@...r.kernel.org>,
"reiserfs-devel@...r.kernel.org" <reiserfs-devel@...r.kernel.org>,
Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v2 3/6] security: Pass xattrs allocated by LSMs to the
inode_init_security hook
On 4/22/2021 6:46 AM, Roberto Sassu wrote:
>> From: Casey Schaufler [mailto:casey@...aufler-ca.com]
>> Sent: Thursday, April 22, 2021 12:44 AM
>> On 4/21/2021 9:19 AM, Roberto Sassu wrote:
>>> In preparation for moving EVM to the LSM infrastructure, this patch
>>> replaces the name, value, len triple with the xattr array pointer provided
>>> by security_inode_init_security(). LSMs are expected to call the new
>>> function lsm_find_xattr_slot() to find the first unused slot of the array
>>> where the xattr should be written.
>>>
>>> This patch modifies also SELinux and Smack to search for an unused slot, to
>>> have a consistent behavior across LSMs (the unmodified version would
>>> overwrite the xattr set by the first LSM in the chain). It is also
>>> desirable to have the modification in those LSMs, as they are likely used
>>> as a reference for the development of new LSMs.
>> This looks better than V1. One safety comment below.
>>
>>> Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
>>> ---
>>> include/linux/lsm_hook_defs.h | 4 ++--
>>> include/linux/lsm_hooks.h | 18 +++++++++++++++---
>>> security/security.c | 13 +++++++------
>>> security/selinux/hooks.c | 13 ++++++-------
>>> security/smack/smack_lsm.c | 20 +++++++++-----------
>>> 5 files changed, 39 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>>> index 477a597db013..afb9dd122f60 100644
>>> --- a/include/linux/lsm_hook_defs.h
>>> +++ b/include/linux/lsm_hook_defs.h
>>> @@ -111,8 +111,8 @@ LSM_HOOK(int, 0, path_notify, const struct path
>> *path, u64 mask,
>>> LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
>>> LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
>>> LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
>>> - struct inode *dir, const struct qstr *qstr, const char **name,
>>> - void **value, size_t *len)
>>> + struct inode *dir, const struct qstr *qstr, struct xattr *xattrs,
>>> + void *fs_data)
>>> LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
>>> const struct qstr *name, const struct inode *context_inode)
>>> LSM_HOOK(int, 0, inode_create, struct inode *dir, struct dentry *dentry,
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index c5498f5174ce..e8c9bac29b9d 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -27,6 +27,7 @@
>>>
>>> #include <linux/security.h>
>>> #include <linux/init.h>
>>> +#include <linux/xattr.h>
>>> #include <linux/rculist.h>
>>>
>>> /**
>>> @@ -227,9 +228,11 @@
>>> * @inode contains the inode structure of the newly created inode.
>>> * @dir contains the inode structure of the parent directory.
>>> * @qstr contains the last path component of the new object
>>> - * @name will be set to the allocated name suffix (e.g. selinux).
>>> - * @value will be set to the allocated attribute value.
>>> - * @len will be set to the length of the value.
>>> + * @xattrs contains the full array of xattrs allocated by LSMs where
>>> + * ->name will be set to the allocated name suffix (e.g. selinux).
>>> + * ->value will be set to the allocated attribute value.
>>> + * ->len will be set to the length of the value.
>>> + * @fs_data contains filesystem-specific data.
>>> * Returns 0 if @name and @value have been successfully set,
>>> * -EOPNOTSUPP if no security attribute is needed, or
>>> * -ENOMEM on memory allocation failure.
>>> @@ -1661,4 +1664,13 @@ static inline void security_delete_hooks(struct
>> security_hook_list *hooks,
>>> extern int lsm_inode_alloc(struct inode *inode);
>>>
>> Some "security researcher" with a fuzz tester is going to manage to dump junk
>> into the slots and ruin your week. I suggest a simple change to make bounds
>> checking
>> possible. It should never happen, but if that was sufficient people would
>> love C
>> string processing better.
>>
>>> +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs)
>> +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs,
int available)
> Ok. I looked at how I should do that. Initially, I thought that I could
> use a global variable storing the number of inode_init_security
> implementations, determined at LSM registration time. Then,
> I realized that this would not work, as the number of array elements
> when security_old_inode_init_security() is called is 1.
You can address that by expanding the call_int_hook MACRO in
security_old_inode_init_security() in place and change it to stop
after the first call. The two callers of security_old_inode_init_security()
are going to need to be converted to security_inode_init_security()
when the "complete" stacking (i.e. SELinux + Smack) anyway, so I don't
see that as an issue.
Is anyone concerned that ocfs2 and reiserfs aren't EVM capable?
>
> I modified the patch set to pass also the number of array elements.
>
> Roberto
>
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli
>
>>> +{
>>> + struct xattr *slot;
>>> +
>>> + for (slot = xattrs; slot && slot->name != NULL; slot++)
>> + for (slot = xattrs; slot && slot->name != NULL; slot++)
>> if (WARN_ON(slot > xattrs[available]))
>> return NULL;
>>
>>> + ;
>>> +
>>> + return slot;
>>> +}
>>> #endif /* ! __LINUX_LSM_HOOKS_H */
>>> diff --git a/security/security.c b/security/security.c
>>> index 7f14e59c4f8e..2c1fe1496069 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -1037,18 +1037,16 @@ int security_inode_init_security(struct inode
>> *inode, struct inode *dir,
>>> if (!initxattrs)
>>> return call_int_hook(inode_init_security, -EOPNOTSUPP,
>> inode,
>>> - dir, qstr, NULL, NULL, NULL);
>>> + dir, qstr, NULL, fs_data);
>>> memset(new_xattrs, 0, sizeof(new_xattrs));
>>> lsm_xattr = new_xattrs;
>>> ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
qstr,
>>> - &lsm_xattr->name,
>>> - &lsm_xattr->value,
>>> - &lsm_xattr->value_len);
>>> + lsm_xattr, fs_data);
>>> if (ret)
>>> goto out;
>>>
>>> evm_xattr = lsm_xattr + 1;
>>> - ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
>>> + ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);
>>> if (ret)
>>> goto out;
>>> ret = initxattrs(inode, new_xattrs, fs_data);
>>> @@ -1071,10 +1069,13 @@ int security_old_inode_init_security(struct inode
>> *inode, struct inode *dir,
>>> const struct qstr *qstr, const char **name,
>>> void **value, size_t *len)
>>> {
>>> + struct xattr xattr = { .name = NULL, .value = NULL, .value_len = 0 };
>>> + struct xattr *lsm_xattr = (name && value && len) ? &xattr : NULL;
>>> +
>>> if (unlikely(IS_PRIVATE(inode)))
>>> return -EOPNOTSUPP;
>>> return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
>>> - qstr, name, value, len);
>>> + qstr, lsm_xattr, NULL);
>>> }
>>> EXPORT_SYMBOL(security_old_inode_init_security);
>>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index ddd097790d47..806827eb132a 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -2916,11 +2916,11 @@ static int selinux_dentry_create_files_as(struct
>> dentry *dentry, int mode,
>>> static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
>>> const struct qstr *qstr,
>>> - const char **name,
>>> - void **value, size_t *len)
>>> + struct xattr *xattrs, void *fs_data)
>>> {
>>> const struct task_security_struct *tsec = selinux_cred(current_cred());
>>> struct superblock_security_struct *sbsec;
>>> + struct xattr *xattr = lsm_find_xattr_slot(xattrs);
>>> u32 newsid, clen;
>>> int rc;
>>> char *context;
>>> @@ -2947,16 +2947,15 @@ static int selinux_inode_init_security(struct
>> inode *inode, struct inode *dir,
>>> !(sbsec->flags & SBLABEL_MNT))
>>> return -EOPNOTSUPP;
>>>
>>> - if (name)
>>> - *name = XATTR_SELINUX_SUFFIX;
>>> + if (xattr) {
>>> + xattr->name = XATTR_SELINUX_SUFFIX;
>>>
>>> - if (value && len) {
>>> rc = security_sid_to_context_force(&selinux_state, newsid,
>>> &context, &clen);
>>> if (rc)
>>> return rc;
>>> - *value = context;
>>> - *len = clen;
>>> + xattr->value = context;
>>> + xattr->value_len = clen;
>>> }
>>>
>>> return 0;
>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>> index 12a45e61c1a5..af7eee0fee52 100644
>>> --- a/security/smack/smack_lsm.c
>>> +++ b/security/smack/smack_lsm.c
>>> @@ -962,26 +962,24 @@ static int smack_inode_alloc_security(struct inode
>> *inode)
>>> * @inode: the newly created inode
>>> * @dir: containing directory object
>>> * @qstr: unused
>>> - * @name: where to put the attribute name
>>> - * @value: where to put the attribute value
>>> - * @len: where to put the length of the attribute
>>> + * @xattrs: where to put the attribute
>>> *
>>> * Returns 0 if it all works out, -ENOMEM if there's no memory
>>> */
>>> static int smack_inode_init_security(struct inode *inode, struct inode
>> *dir,
>>> - const struct qstr *qstr, const char **name,
>>> - void **value, size_t *len)
>>> + const struct qstr *qstr,
>>> + struct xattr *xattrs, void *fs_data)
>>> {
>>> struct inode_smack *issp = smack_inode(inode);
>>> struct smack_known *skp = smk_of_current();
>>> struct smack_known *isp = smk_of_inode(inode);
>>> struct smack_known *dsp = smk_of_inode(dir);
>>> + struct xattr *xattr = lsm_find_xattr_slot(xattrs);
>>> int may;
>>>
>>> - if (name)
>>> - *name = XATTR_SMACK_SUFFIX;
>>> + if (xattr) {
>>> + xattr->name = XATTR_SMACK_SUFFIX;
>>>
>>> - if (value && len) {
>>> rcu_read_lock();
>>> may = smk_access_entry(skp->smk_known, dsp->smk_known,
>>> &skp->smk_rules);
>>> @@ -999,11 +997,11 @@ static int smack_inode_init_security(struct inode
>> *inode, struct inode *dir,
>>> issp->smk_flags |= SMK_INODE_CHANGED;
>>> }
>>>
>>> - *value = kstrdup(isp->smk_known, GFP_NOFS);
>>> - if (*value == NULL)
>>> + xattr->value = kstrdup(isp->smk_known, GFP_NOFS);
>>> + if (xattr->value == NULL)
>>> return -ENOMEM;
>>>
>>> - *len = strlen(isp->smk_known);
>>> + xattr->value_len = strlen(isp->smk_known);
>>> }
>>>
>>> return 0;
Powered by blists - more mailing lists