[apparmor] Apparmor rules for dconf confinement
John Johansen
john.johansen at canonical.com
Tue Jun 23 07:22:02 UTC 2015
> ---
> security/apparmor/apparmorfs.c | 115 +++++++++++++++++++++++++++++++++++--
> security/apparmor/include/policy.h | 20 ++++++-
> security/apparmor/policy.c | 20 +++++++
> security/apparmor/policy_unpack.c | 54 ++++++++++++++++-
> 4 files changed, 202 insertions(+), 7 deletions(-)
>
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index f86f56e..ab11bd1 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -189,6 +189,97 @@ static const struct file_operations aa_fs_profile_remove = {
> .llseek = default_llseek,
> };
>
> +static bool keycmp(void *p1, void *p2) {
> + const char **k1 = p1;
> + const char **k2 = p2;
> + return !strcmp(*k1, *k2);
> +}
> +
> +/**
> + * query_data - queries a policy and writes its data to buf
> + * @buf: the resulting data is stored here (NOT NULL)
> + * @buf_len: size of buf
> + * @query: query string used to retrieve data
> + * @query_len: size of query including second NUL byte
> + *
> + * The buffers pointed to by buf and query may overlap. The query buffer is
> + * parsed before buf is written to.
> + *
> + * The query should look like "<LABEL>\0<KEY>\0", where <LABEL> is the name of
> + * the security confinement context and <KEY> is the name of the data to
> + * retrieve. <LABEL> and <KEY> must not be NUL-terminated.
> + *
> + * Don't expect the contents of buf to be preserved on failure.
> + *
> + * Returns: number of characters written to buf or -errno on failure
> + */
> +static ssize_t query_data(char *buf, size_t buf_len,
> + char *query, size_t query_len)
> +{
> + char *out;
> + const char *key;
> + struct label_it i;
> + struct aa_label *label;
> + struct aa_profile *profile;
> + struct aa_data *data;
> + ssize_t blocks = 0;
> + ssize_t bytes;
> + u32 hash;
> +
> + if (!query_len)
> + return -EINVAL; /* need a query */
> +
> + key = query + strnlen(query, query_len) + 1;
> + if (key + 1 >= query + query_len)
> + return -EINVAL; /* not enough space for a non-empty key */
> + if (key + strnlen(key, query + query_len - key) >= query + query_len)
> + return -EINVAL; /* must end with NUL */
> +
> + if (buf_len < 2 * sizeof(ssize_t))
> + return -EINVAL; /* not enough space */
> +
> + label = aa_label_parse(aa_current_label(), query, GFP_KERNEL, false);
> + if (IS_ERR(label))
> + return PTR_ERR(label);
> +
> + /* We are going to leave space for two numbers. The first is the total
> + * number of bytes we are writing after the first number. This is so
> + * users can read the full output without reallocation.
> + *
> + * The second number is the number of data blocks we're writing. An
> + * application might be confined by multiple policies having data in
> + * the same key. */
> + memset(buf, 0, 2 * sizeof(ssize_t));
> + out = buf + 2 * sizeof(ssize_t);
use of size_t for an interface between kernel and userspace is problematic.
We run into some weird combinations, 32bit userspace on 64bit kernel (common).
And potentially other weird combos with endian switching. I am not opposed
to using binary data, but define it exactly
u32 or u64 and whether it is little or big endian, that way we avoid any
chance of an issue between user and kernel space communication.
> +
> + label_for_each_confined(i, label, profile) {
> + if (!profile->data)
> + continue;
> +
> + hash = rhashtable_hashfn(profile->data, &key, sizeof(key));
> + data = rhashtable_lookup_compare(profile->data, hash, keycmp, &key);
> +
> + if (data) {
> + if (out + sizeof(ssize_t) + data->size > buf + buf_len) {
> + aa_put_label(label);
> + return -EINVAL; /* not enough space */
> + }
> + memcpy(out, &data->size, sizeof(ssize_t));
> + out += sizeof(ssize_t);
same with the size written here
> + memcpy(out, data->data, data->size);
> + out += data->size;
> + blocks++;
> + }
> + }
> + aa_put_label(label);
> +
> + bytes = out - buf - sizeof(ssize_t);
> + memcpy(buf, &bytes, sizeof(ssize_t));
> + memcpy(buf + sizeof(ssize_t), &blocks, sizeof(ssize_t));
> +
> + return out - buf;
> +}
> +
> /**
> * query_label - queries a label and writes permissions to buf
> * @buf: the resulting permissions string is stored here (NOT NULL)
> @@ -274,18 +365,27 @@ static ssize_t query_label(char *buf, size_t buf_len,
> #define QUERY_CMD_LABEL_LEN 6
> #define QUERY_CMD_PROFILE "profile\0"
> #define QUERY_CMD_PROFILE_LEN 8
> +#define QUERY_CMD_DATA "data\0"
> +#define QUERY_CMD_DATA_LEN 5
>
> /**
> - * aa_write_access - generic permissions query
> + * aa_write_access - generic permissions and data query
> * @file: pointer to open apparmorfs/access file
> * @ubuf: user buffer containing the complete query string (NOT NULL)
> * @count: size of ubuf
> * @ppos: position in the file (MUST BE ZERO)
> *
> - * Allows for one permission query per open(), write(), and read() sequence.
> - * The only query currently supported is a label-based query. For this query
> - * ubuf must begin with "label\0", followed by the profile query specific
> - * format described in the query_label() function documentation.
> + * Allows for one permissions or data query per open(), write(), and read()
> + * sequence. The only queries currently supported are label-based queries for
> + * permissions or data.
> + *
> + * For permissions queries, ubuf must begin with "label\0", followed by the
> + * profile query specific format described in the query_label() function
> + * documentation.
> + *
> + * For data queries, ubuf must have the form "data\0<LABEL>\0<KEY>\0", where
> + * <LABEL> is the name of the security confinement context and <KEY> is the
> + * name of the data to retrieve.
> *
> * Returns: number of bytes written or -errno on failure
> */
> @@ -312,6 +412,11 @@ static ssize_t aa_write_access(struct file *file, const char __user *ubuf,
> len = query_label(buf, SIMPLE_TRANSACTION_LIMIT,
> buf + QUERY_CMD_LABEL_LEN,
> count - QUERY_CMD_LABEL_LEN);
> + } else if (count > QUERY_CMD_DATA_LEN &&
> + !memcmp(buf, QUERY_CMD_DATA, QUERY_CMD_DATA_LEN)) {
> + len = query_data(buf, SIMPLE_TRANSACTION_LIMIT,
> + buf + QUERY_CMD_DATA_LEN,
> + count - QUERY_CMD_DATA_LEN);
> } else
> len = -EINVAL;
>
> diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
> index da71e27f..48e90a7 100644
> --- a/security/apparmor/include/policy.h
> +++ b/security/apparmor/include/policy.h
> @@ -18,6 +18,7 @@
> #include <linux/capability.h>
> #include <linux/cred.h>
> #include <linux/kref.h>
> +#include <linux/rhashtable.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> #include <linux/socket.h>
> @@ -142,6 +143,19 @@ struct aa_policydb {
>
> };
>
> +/* struct aa_data - generic data structure
> + * key: name for retrieving this data
> + * size: size of data in bytes
> + * data: binary data
> + * head: reserved for rhashtable
> + */
> +struct aa_data {
> + const char *key;
> + size_t size;
> + char *data;
> + struct rhash_head head;
> +};
> +
> /* struct aa_profile - basic confinement data
> * @base - base components of the profile (name, refcount, lists, lock ...)
> * @label - label this profile is an extension of
> @@ -161,9 +175,10 @@ struct aa_policydb {
> * @caps: capabilities for the profile
> * @net: network controls for the profile
> * @rlimits: rlimits for the profile
> - *
> * @dents: dentries for the profiles file entries in apparmorfs
> * @dirname: name of the profile dir in apparmorfs
> + * @blob: a single binary string holding the allocated data
> + * @data: hashtable for free-form policy aa_data
> *
> * The AppArmor profile contains the basic confinement data. Each profile
> * has a name, and exists in a namespace. The @name and @exec_match are
> @@ -205,6 +220,9 @@ struct aa_profile {
> unsigned char *hash;
> char *dirname;
> struct dentry *dents[AAFS_PROF_SIZEOF];
> +
> + void *blob;
> + struct rhashtable *data;
> };
>
> extern struct aa_namespace *root_ns;
> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
> index 7a9d4c8..0d5c477 100644
> --- a/security/apparmor/policy.c
> +++ b/security/apparmor/policy.c
> @@ -630,6 +630,11 @@ void __init aa_free_root_ns(void)
> */
> void aa_free_profile(struct aa_profile *profile)
> {
> + struct rhashtable *rht;
> + struct aa_data *data;
> + struct aa_data *next;
> + size_t i;
> +
> AA_DEBUG("%s(%p)\n", __func__, profile);
>
> if (!profile)
> @@ -651,6 +656,21 @@ void aa_free_profile(struct aa_profile *profile)
> aa_put_dfa(profile->xmatch);
> aa_put_dfa(profile->policy.dfa);
>
> + if (profile->data) {
> + rht = profile->data;
> + profile->data = NULL;
> + for (i = 0; i < rht->tbl->size; i++) {
> + rht_for_each_entry_safe(data, next, rht->tbl->buckets[i], rht, head) {
this ends up leaking the data->key
> + kzfree(data);
> + }
> + }
> + rhashtable_destroy(rht);
> + kzfree(rht);
> + }
> +
> + if (profile->blob)
> + kzfree(profile->blob);
> +
> kzfree(profile->hash);
> kzfree(profile);
> }
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index 7f63b67..185bbe0 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -20,6 +20,7 @@
> #include <asm/unaligned.h>
> #include <linux/ctype.h>
> #include <linux/errno.h>
> +#include <linux/jhash.h>
>
> #include "include/apparmor.h"
> #include "include/audit.h"
> @@ -484,6 +485,12 @@ fail:
> return 0;
> }
>
> +static u32 shash(const void *data, u32 len, u32 seed)
> +{
> + const char * const *key = data;
> + return jhash(*key, strlen(*key), seed);
> +}
> +
> /**
> * unpack_profile - unpack a serialized profile
> * @e: serialized data extent information (NOT NULL)
> @@ -494,6 +501,11 @@ static struct aa_profile *unpack_profile(struct aa_ext *e)
> {
> struct aa_profile *profile = NULL;
> const char *name = NULL;
> + const char *key = NULL;
> + char *blob = NULL;
> + struct rhashtable_params params = { 0 };
> + struct aa_ext f;
> + struct aa_data *data;
> size_t size = 0;
> int i, error = -EPROTO;
> kernel_cap_t tmpcap;
> @@ -668,14 +680,54 @@ static struct aa_profile *unpack_profile(struct aa_ext *e)
> if (!unpack_trans_table(e, profile))
> goto fail;
>
> + size = unpack_blob(e, &blob, "data");
while you can get away with this, it forces you to mess with the sub "aa_ext f"
nor do you actually end up checking that your sub size consumed is what the
outer blob says it is.
Is there any reason we would want to allocate a single larger blob, instead
a blob per entry? That would keep the kmalloc smaller and make it more likely
to succeed with larger blobs. At a minimum we should move the blob allocation
to kvmalloc and kvfree which can handle larger blobs of data well.
I've appended a patch (totally untested and not even built) at the end that
converts to individual allocations, what do you think?
> + if (size) {
> + profile->blob = kmemdup(blob, size, GFP_KERNEL);
> + if (!profile->blob)
> + goto fail;
> +
> + profile->data = kzalloc(sizeof(*profile->data), GFP_KERNEL);
> + if (!profile->data)
> + goto fail;
> +
> + params.nelem_hint = 1;
> + params.key_len = sizeof(void *);
> + params.key_offset = offsetof(struct aa_data, key);
> + params.head_offset = offsetof(struct aa_data, head);
> + params.hashfn = shash;
> +
> + if (rhashtable_init(profile->data, ¶ms))
> + goto fail;
> +
> + f.start = profile->blob;
> + f.end = f.start + size;
> + f.pos = f.start;
> + f.version = e->version;
> +
> + while (unpack_str(&f, &key, NULL)) {
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + data->key = key;
> + data->size = unpack_blob(&f, &data->data, NULL);
> + if (!data->size) {
> + kzfree(data);
> + goto fail;
> + }
> + data->head.next = NULL;
> + rhashtable_insert(profile->data, &data->head);
> + }
> + }
> +
> if (!unpack_nameX(e, AA_STRUCTEND, NULL))
> goto fail;
>
> return profile;
>
> fail:
> - if (profile)
> + /* don't worry about profile->data, aa_free_profile should free it */
> + if (profile) {
> + kzfree(profile->blob);
free_profile also frees profile->blob so either don't worry about it here
either or assign NULL to it, so we don't get a double free
> name = NULL;
> + }
> else if (!name)
> name = "unknown";
> audit_iface(profile, name, "failed to unpack profile", e, error);
> -- 2.1.4
>
>
---
diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
index 48e90a7..14843f4 100644
--- a/security/apparmor/include/policy.h
+++ b/security/apparmor/include/policy.h
@@ -177,7 +177,6 @@ struct aa_data {
* @rlimits: rlimits for the profile
* @dents: dentries for the profiles file entries in apparmorfs
* @dirname: name of the profile dir in apparmorfs
- * @blob: a single binary string holding the allocated data
* @data: hashtable for free-form policy aa_data
*
* The AppArmor profile contains the basic confinement data. Each profile
@@ -221,7 +220,6 @@ struct aa_profile {
char *dirname;
struct dentry *dents[AAFS_PROF_SIZEOF];
- void *blob;
struct rhashtable *data;
};
diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index 0d5c477..483f41e 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -661,6 +661,8 @@ void aa_free_profile(struct aa_profile *profile)
profile->data = NULL;
for (i = 0; i < rht->tbl->size; i++) {
rht_for_each_entry_safe(data, next, rht->tbl->buckets[i], rht, head) {
+ kzfree(data->key);
+ kvfree(data->data);
kzfree(data);
}
}
@@ -668,9 +670,6 @@ void aa_free_profile(struct aa_profile *profile)
kzfree(rht);
}
- if (profile->blob)
- kzfree(profile->blob);
-
kzfree(profile->hash);
kzfree(profile);
}
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index fcfd018..078a1e5 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -491,6 +491,14 @@ static u32 shash(const void *data, u32 len, u32 seed)
return jhash(*key, strlen(*key), seed);
}
+void *kvmemdup(const void *src, size_t len)
+{
+ void *p = kvmalloc(len);
+ if (p)
+ memcpy(p, src, len);
+ return p;
+}
+
/**
* unpack_profile - unpack a serialized profile
* @e: serialized data extent information (NOT NULL)
@@ -501,11 +509,6 @@ static struct aa_profile *unpack_profile(struct aa_ext *e)
{
struct aa_profile *profile = NULL;
const char *name = NULL;
- const char *key = NULL;
- char *blob = NULL;
- struct rhashtable_params params = { 0 };
- struct aa_ext f;
- struct aa_data *data;
size_t size = 0;
int i, error = -EPROTO;
kernel_cap_t tmpcap;
@@ -680,12 +683,9 @@ static struct aa_profile *unpack_profile(struct aa_ext *e)
if (!unpack_trans_table(e, profile))
goto fail;
- size = unpack_blob(e, &blob, "data");
- if (size) {
- profile->blob = kmemdup(blob, size, GFP_KERNEL);
- if (!profile->blob)
- goto fail;
-
+ if (unpack_nameX(e, AA_STRUCT, "data")) {
+ struct rhashtable_params params = { 0 };
+ const char *key = NULL;
profile->data = kzalloc(sizeof(*profile->data), GFP_KERNEL);
if (!profile->data)
goto fail;
@@ -699,23 +699,28 @@ static struct aa_profile *unpack_profile(struct aa_ext *e)
if (rhashtable_init(profile->data, ¶ms))
goto fail;
- f.start = profile->blob;
- f.end = f.start + size;
- f.pos = f.start;
- f.version = e->version;
-
while (unpack_str(&f, &key, NULL)) {
+ struct aa_data *data;
+ char *blob;
data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data) {
+ kzfree(key);
+ goto fail;
+ }
+ data->size = unpack_blob(e, &blob, NULL);
+ data->data = kvmemdup(blob, blob->size, GFP_KERNEL);
data->key = key;
- data->size = unpack_blob(&f, &data->data, NULL);
if (!data->size) {
- kzfree(data);
+ kzfree(key);
+ kvfree(data);
goto fail;
}
data->head.next = NULL;
rhashtable_insert(profile->data, &data->head);
}
}
+ if (!unpack_nameX(e, AA_STRUCTEND, NULL))
+ goto fail;
if (!unpack_nameX(e, AA_STRUCTEND, NULL))
goto fail;
@@ -724,10 +729,9 @@ static struct aa_profile *unpack_profile(struct aa_ext *e)
fail:
/* don't worry about profile->data, aa_free_profile should free it */
- if (profile) {
- kzfree(profile->blob);
+ if (profile)
name = NULL;
- }
+
else if (!name)
name = "unknown";
audit_iface(profile, name, "failed to unpack profile", e, error);
More information about the AppArmor
mailing list