| 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
| ||
|
Date: Tue, 7 Apr 2020 17:39:09 +0200
From: Jann Horn <jannh@...gle.com>
To: deven.desai@...ux.microsoft.com
Cc: agk@...hat.com, Jens Axboe <axboe@...nel.dk>, snitzer@...hat.com,
James Morris <jmorris@...ei.org>,
"Serge E. Hallyn" <serge@...lyn.com>,
Mimi Zohar <zohar@...ux.ibm.com>,
linux-integrity@...r.kernel.org,
linux-security-module <linux-security-module@...r.kernel.org>,
dm-devel@...hat.com, linux-block@...r.kernel.org,
tyhicks@...ux.microsoft.com,
Pavel Tatashin <pasha.tatashin@...een.com>,
Sasha Levin <sashal@...nel.org>,
jaskarankhurana@...ux.microsoft.com, nramas@...ux.microsoft.com,
mdsakib@...ux.microsoft.com,
kernel list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v2 03/12] security: add ipe lsm policy parser and
policy loading
On Tue, Apr 7, 2020 at 12:14 AM <deven.desai@...ux.microsoft.com> wrote:
[...]
> Adds the policy parser and the policy loading to IPE, along with the
> related sysfs and securityfs entries.
[...]
> diff --git a/security/ipe/ipe-parse.c b/security/ipe/ipe-parse.c
[...]
> +/* Internal Type Definitions */
> +enum property_priority {
> + other = 0,
> + action = 1,
> + op = 2,
> + default_action = 3,
> + policy_ver = 4,
> + policy_name = 5,
> +};
> +
> +struct token {
> + struct list_head next_tok;
> + const char *key;
> + enum property_priority key_priority;
> + const char *val;
> +};
> +
[...]
> +/**
> + * ipe_free_policy: Deallocate an ipe_policy structure.
> + * @pol: Policy to free.
> + */
> +void ipe_free_policy(struct ipe_policy *pol)
> +{
> + size_t i;
> + struct ipe_rule *ptr;
> + struct ipe_rule_table *op;
> + struct list_head *l_ptr, *l_next;
> +
> + if (IS_ERR_OR_NULL(pol))
> + return;
> +
> + for (i = 0; i < ARRAY_SIZE(pol->ops); ++i) {
> + op = &pol->ops[i];
> +
> + list_for_each_safe(l_ptr, l_next, &op->rules) {
> + ptr = list_entry(l_ptr, struct ipe_rule, next);
> + list_del(l_ptr);
> + ipe_free_rule(ptr);
> + }
> + }
> +
> + kfree(pol->policy_name);
> + kfree(pol);
> + pol = NULL;
What is this assignment supposed to do?
> +}
[...]
> diff --git a/security/ipe/ipe-policy.c b/security/ipe/ipe-policy.c
[...]
> +/**
> + * ipe_is_active_policy: Determine if @policy is the currently active policy.
> + * @policy: Policy to check if it's the active policy.
> + *
> + * NOTE: If this attribute is needed to be consistent over a critical section,
> + * do not use this function, as it does not hold the read lock over the
> + * entirety of the critical section.
> + *
> + * Return:
> + * true - @policy is the active policy
> + * false - @policy is not the active policy
> + */
> +bool ipe_is_active_policy(const struct ipe_policy *policy)
> +{
> + bool result;
> +
> + rcu_read_lock();
> +
> + result = rcu_dereference(ipe_active_policy) == policy;
> +
> + rcu_read_unlock();
You're not actually accessing the pointer, so you can use rcu_access_pointer()
and get rid of the rcu_read_lock()/rcu_read_unlock(). Then this helper turns
into a one-liner.
> + return result;
> +}
> +
> +/**
> + * ipe_update_active_policy: Determine if @old is the active policy, and update
> + * the active policy if necessary.
> + * @old: The previous policy that the update is trying to replace.
> + * @new: The new policy attempting to replace @old.
> + *
> + * If @old is not the active policy, nothing will be done.
> + *
> + * Return:
> + * 0 - OK
> + * -EBADMSG - Invalid Policy
> + */
> +int ipe_update_active_policy(const struct ipe_policy *old,
> + const struct ipe_policy *new)
> +{
> + int rc = 0;
> + const struct ipe_policy *curr_policy = NULL;
> +
> + /* no active policy, safe to update */
> + if (!ipe_active_policy)
This should be rcu_access_pointer().
> + return 0;
[...]
> +}
[...]
> diff --git a/security/ipe/ipe-secfs.c b/security/ipe/ipe-secfs.c
[...]
> +/**
> + * alloc_callback: Callback given to verify_pkcs7_signature function to set
> + * the inner content reference and parse the policy.
> + * @ctx: "ipe_policy_node" to set inner content, size and parsed policy of.
> + * @data: Start of PKCS#7 inner content.
> + * @len: Length of @data.
> + * @asn1hdrlen: Unused.
> + *
> + * Return:
> + * 0 - OK
> + * ERR_PTR(-EBADMSG) - Invalid policy syntax
> + * ERR_PTR(-ENOMEM) - Out of memory
> + */
> +static int alloc_callback(void *ctx, const void *data, size_t len,
> + size_t asn1hdrlen)
> +{
> + char *cpy = NULL;
> + struct ipe_policy *pol = NULL;
> + struct ipe_policy_node *n = (struct ipe_policy_node *)ctx;
> +
> + n->content = (const u8 *)data;
> + n->content_size = len;
> +
> + if (len == 0)
> + return -EBADMSG;
> +
> + cpy = kzalloc(len + 1, GFP_KERNEL);
> + if (!cpy)
> + return -ENOMEM;
> +
> + (void)strncpy(cpy, data, len);
Shouldn't this just be memcpy()?
> + pol = ipe_parse_policy(cpy);
> + if (IS_ERR(pol)) {
> + kfree(cpy);
> + return PTR_ERR(pol);
> + }
> +
> + n->parsed = pol;
> + kfree(cpy);
> + return 0;
> +}
[...]
> +static ssize_t ipe_secfs_new_policy(struct file *f, const char __user *data,
> + size_t len, loff_t *offset)
> +{
> + ssize_t rc = 0;
> + u8 *cpy = NULL;
> + ssize_t written = 0;
> +
> + if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
> + return -EPERM;
Use file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN) instead, both here and
elsewhere.
> + cpy = kzalloc(len, GFP_KERNEL);
> + if (!cpy) {
> + rc = -ENOMEM;
> + goto err;
> + }
> +
> + written = simple_write_to_buffer(cpy, len, offset, data, len);
> + if (written < 0) {
> + rc = written;
> + goto err;
> + }
This should probably be memdup_user() instead of
kzalloc()+simple_write_to_buffer()?
> + rc = ipe_build_policy_secfs_node(cpy, written);
> +err:
> + kfree(cpy);
> + return rc < 0 ? rc : written;
> +}
> +
> +/**
> + * retrieve_backed_dentry: Retrieve a dentry with a backing inode, identified
> + * by @name, under @parent.
> + * @name: Name of the dentry under @parent.
> + * @parent: The parent dentry to search under for @name.
> + * @size: Length of @name.
> + *
> + * This takes a reference to the returned dentry. Caller needs to call dput
> + * to drop the reference.
> + *
> + * Return:
> + * valid dentry - OK
> + * ERR_PTR - Error, see lookup_one_len_unlocked
> + * NULL - No backing inode was found
> + */
> +static struct dentry *retrieve_backed_dentry(const char *name,
> + struct dentry *parent,
> + size_t size)
> +{
> + int rc = 0;
> + struct dentry *tmp = NULL;
> +
> + tmp = lookup_one_len_unlocked(name, parent, size);
> + if (IS_ERR(tmp)) {
> + rc = PTR_ERR(tmp);
> + goto out;
> + }
You could just "return tmp;" here.
> +
> + if (!d_really_is_positive(tmp))
> + goto out1;
And here "return NULL;".
> + return tmp;
> +out1:
> + tmp = NULL;
This just sets a variable that is never read again to NULL?
> +out:
> + return rc == 0 ? NULL : ERR_PTR(rc);
> +}
> +
> +/**
> + * ipe_secfs_del_policy: Delete a policy indicated by the name provided by
> + * @data
> + * @f: Unused.
> + * @data: Buffer containing the policy id to delete.
> + * @len: Length of @data.
> + * @offset: Offset into @data.
> + *
> + * NOTE: Newlines are treated as part of the name, if using echo to test,
> + * use -n to prohibit the silent addition of a newline.
> + *
> + * Return:
> + * > 0 - OK
> + * -ENOMEM - Out of memory
> + * -EPERM - Policy is active
> + * -ENOENT - Policy does not exist
> + * -EPERM - if a MAC subsystem is enabled, missing CAP_MAC_ADMIN
> + * Other - See retrieve_backed_dentry
> + */
> +static ssize_t ipe_secfs_del_policy(struct file *f, const char __user *data,
> + size_t len, loff_t *offset)
> +{
> + ssize_t rc = 0;
> + char *id = NULL;
> + ssize_t written = 0;
> + struct dentry *raw = NULL;
> + struct dentry *content = NULL;
> + struct inode *policy_i = NULL;
> + struct dentry *policy_root = NULL;
> + struct inode *policies_root = NULL;
> + const struct ipe_policy *target = NULL;
> +
> + if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
> + return -EPERM;
> +
> + id = kzalloc(len, GFP_KERNEL);
> + if (!id) {
> + rc = -ENOMEM;
> + goto out;
> + }
> +
> + written = simple_write_to_buffer(id, len, offset, data, len);
> + if (written < 0) {
> + rc = written;
> + goto out;
> + }
> +
> + policies_root = d_inode(ipe_policies_root);
> +
> + policy_root = retrieve_backed_dentry(id, ipe_policies_root, written);
> + if (IS_ERR_OR_NULL(policy_root)) {
> + rc = IS_ERR(policy_root) ? PTR_ERR(policy_root) : -ENOENT;
> + goto out;
> + }
> +
> + policy_i = d_inode(policy_root);
> +
> + /* if the found dentry matches boot policy, fail */
> + if (boot_policy_node == policy_root) {
> + rc = -EACCES;
> + goto out1;
> + }
> +
> + target = ((struct ipe_policy_node *)policy_i->i_private)->parsed;
> +
> + /* fail if it's the active policy */
> + if (ipe_is_active_policy(target)) {
> + rc = -EPERM;
> + goto out1;
> + }
Why can it not become the active policy after this check?
> + raw = retrieve_backed_dentry(IPE_FULL_CONTENT, policy_root,
> + strlen(IPE_FULL_CONTENT));
> + if (IS_ERR_OR_NULL(raw)) {
> + rc = IS_ERR(raw) ? PTR_ERR(raw) : -ENOENT;
> + goto out1;
> + }
> +
> + content = retrieve_backed_dentry(IPE_INNER_CONTENT, policy_root,
> + strlen(IPE_INNER_CONTENT));
> + if (IS_ERR_OR_NULL(content)) {
> + rc = IS_ERR(content) ? PTR_ERR(content) : -ENOENT;
> + goto out2;
> + }
> +
> + inode_lock(policies_root);
> + ipe_free_policy_node(policy_i->i_private);
> + policy_i->i_private = NULL;
> + inode_unlock(policies_root);
> +
> + dput(raw);
> + dput(content);
> + dput(policy_root);
> + securityfs_remove(raw);
> + securityfs_remove(content);
> + securityfs_remove(policy_root);
> +
> + kfree(id);
> + return written;
> +out2:
> + dput(raw);
> +out1:
> + dput(policy_root);
> +out:
> + kfree(id);
> + return rc;
> +}
> +
> +/**
> + * ipe_secfs_rd_policy: Read the raw content (full enveloped PKCS7) data of
> + * the policy stored within the file's parent inode.
> + * @f: File representing the securityfs entry.
> + * @data: User mode buffer to place the raw pkcs7.
> + * @len: Length of @data.
> + * @offset: Offset into @data.
> + *
> + * Return:
> + * > 0 - OK
> + * -ENOMEM - Out of memory
> + */
> +static ssize_t ipe_secfs_rd_policy(struct file *f, char __user *data,
> + size_t size, loff_t *offset)
> +{
> + ssize_t rc = 0;
> + size_t avail = 0;
> + u8 *buffer = NULL;
> + struct inode *root = NULL;
> + const struct ipe_policy_node *node = NULL;
> +
> + root = d_inode(f->f_path.dentry->d_parent);
> +
> + inode_lock_shared(root);
> + node = (const struct ipe_policy_node *)root->i_private;
> +
> + avail = node->data_len;
> + buffer = kmemdup(node->data, avail, GFP_KERNEL);
> + if (!buffer) {
> + rc = -ENOMEM;
> + goto cleanup;
> + }
> +
> + rc = simple_read_from_buffer(data, size, offset, buffer, avail);
> +cleanup:
> + inode_unlock_shared(root);
Same thing as in ipe_secfs_rd_content(): simple_read_from_buffer() needlessly
within locked section, buffer not freed.
> +
> + return rc;
> +}
> +
> +/**
> + * ipe_secfs_ud_policy: Update a policy in place with a new PKCS7 policy.
> + * @f: File representing the securityfs entry.
> + * @data: Buffer user mode to place the raw pkcs7.
> + * @len: Length of @data.
> + * @offset: Offset into @data.
> + *
> + * Return:
> + * 0 - OK
> + * -EBADMSG - Invalid policy format
> + * -ENOMEM - Out of memory
> + * -EPERM - if a MAC subsystem is enabled, missing CAP_MAC_ADMIN
> + * -EINVAL - Incorrect policy name for this node, or version is < current
> + */
> +static ssize_t ipe_secfs_ud_policy(struct file *f, const char __user *data,
> + size_t len, loff_t *offset)
> +{
> + ssize_t rc = 0;
> + u8 *cpy = NULL;
> + ssize_t written = 0;
> + struct inode *root = NULL;
> + struct crypto_shash *tfm = NULL;
> + struct ipe_policy_node *new = NULL;
> + struct ipe_policy_node *old = NULL;
> +
> + if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
> + return -EPERM;
> +
> + cpy = kzalloc(len, GFP_KERNEL);
> + if (!cpy) {
> + rc = -ENOMEM;
> + goto out;
> + }
> +
> + written = simple_write_to_buffer(cpy, len, offset, data, len);
> + if (written < 0) {
> + rc = written;
> + goto out;
> + }
You'd probably be better off just doing memdup_user() here.
simple_write_to_buffer() only makes sense if you have a buffer that can be
continuously updated with multiple writes.
> + new = ipe_alloc_policy_node(cpy, len);
> + if (IS_ERR(new)) {
> + rc = PTR_ERR(new);
> + goto out;
> + }
> +
> + tfm = crypto_alloc_shash("sha1", 0, 0);
> + if (IS_ERR(tfm))
> + goto out2;
> +
> + root = d_inode(f->f_path.dentry->d_parent);
> + inode_lock(root);
> +
> + old = (struct ipe_policy_node *)root->i_private;
> +
> + if (strcmp(old->parsed->policy_name, new->parsed->policy_name)) {
> + rc = -EINVAL;
> + goto out3;
> + }
> +
> + if (!ipe_is_valid_policy(old->parsed, new->parsed)) {
> + rc = -EINVAL;
> + goto out3;
> + }
> +
> + rc = ipe_update_active_policy(old->parsed, new->parsed);
> + if (rc != 0)
> + goto out3;
> +
> + ipe_audit_policy_load(new->parsed, new->data, new->data_len, tfm);
> + swap(root->i_private, new);
> +
> + inode_unlock(root);
> + kfree(cpy);
> + ipe_free_policy_node(new);
> + crypto_free_shash(tfm);
> +
> + return written;
> +out3:
> + inode_unlock(root);
> + ipe_free_policy_node(new);
> +out2:
> + crypto_free_shash(tfm);
> +out:
> + kfree(cpy);
> + return rc;
> +}
[...]
> +static ssize_t ipe_secfs_rd_content(struct file *f, char __user *data,
> + size_t size, loff_t *offset)
> +{
> + ssize_t rc = 0;
> + size_t avail = 0;
> + u8 *buffer = NULL;
> + struct inode *root = NULL;
> + const struct ipe_policy_node *node = NULL;
> +
> + root = d_inode(f->f_path.dentry->d_parent);
> +
> + inode_lock(root);
> + node = (const struct ipe_policy_node *)root->i_private;
> +
> + avail = node->content_size;
> + buffer = kmemdup(node->content, avail, GFP_KERNEL);
> + if (!buffer) {
> + rc = -ENOMEM;
> + goto cleanup;
> + }
> +
> + rc = simple_read_from_buffer(data, size, offset, buffer, avail);
> +cleanup:
> + inode_unlock(root);
Why are you nod doing the simple_read_from_buffer() after inode_unlock()?
The way you're doing it now, there isn't really a point in the kmemdup() at
all...
Also, you'll have to free the buffer before returning.
> + return rc;
> +}
[...]
> diff --git a/security/ipe/ipe-sysfs.c b/security/ipe/ipe-sysfs.c
[...]
> +static int ipe_switch_active_policy(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp,
> + loff_t *ppos)
> +{
> + int rc = 0;
> + char *id = NULL;
> + size_t size = 0;
> +
> + if (write) {
> + id = kzalloc((*lenp) + 1, GFP_KERNEL);
> + if (!id)
> + return -ENOMEM;
> +
> + table->data = id;
> + table->maxlen = (*lenp) + 1;
> +
> + rc = proc_dostring(table, write, buffer, lenp, ppos);
> + if (rc != 0)
> + goto out;
> +
> + rc = ipe_set_active_policy(id, strlen(id));
> + } else {
> + rcu_read_lock();
> + size = strlen(rcu_dereference(ipe_active_policy)->policy_name);
Can't `ipe_active_policy` be NULL here?
> + rcu_read_unlock();
> +
> + id = kzalloc(size + 1, GFP_KERNEL);
The `+ 1` seems unnecessary.
> + if (!id)
> + return -ENOMEM;
> +
> + rcu_read_lock();
> + strncpy(id, rcu_dereference(ipe_active_policy)->policy_name,
> + size);
> + rcu_read_unlock();
> +
> + table->data = id;
> + table->maxlen = size;
> +
> + rc = proc_dostring(table, write, buffer, lenp, ppos);
> + }
> +out:
> + kfree(id);
> + return rc;
> +}
> +
> +#endif /* CONFIG_SECURITYFS */
[...]
> diff --git a/security/ipe/ipe.c b/security/ipe/ipe.c
[...]
> +
> +/**
> + * strict_parse: Kernel command line parameter to enable strict parsing of
> + * IPE policies - causing unrecognized properties to fail
> + * parsing. This breaks backwards compatibility of IPE policies,
> + * when enabled.
I guess the backwards compatibility stuff is referring to an out-of-tree version
of this series that you've already shipped?
> + * This is also controlled by the sysctl, "ipe.strict_parse".
> + */
(Also, same thing as in the other patch re sysctls and kernel command line
parameters for the same feature.)
Powered by blists - more mailing lists