[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-security-module
Subject: Re: RFC: Seccomp as extended attribute
From: Michael Cassaniti <m.cassaniti () gmail ! com>
Date: 2012-10-31 4:43:38
Message-ID: 5090AC7A.9050106 () gmail ! com
[Download RAW message or body]
On 21/09/2012 17:57, Michael Cassaniti wrote:
> On 21/09/2012 3:34 PM, Michael Cassaniti wrote:
> > On 20/09/2012 5:59 AM, Will Drewry wrote:
> > > On Thu, Sep 13, 2012 at 8:53 PM, Eric Paris <eparis@parisplace.org>
> > > wrote:
> > > > On Thu, Sep 13, 2012 at 7:39 PM, Michael Cassaniti
> > > > <m.cassaniti@gmail.com> wrote:
> > > >
> > > > > What I don't understand is how does the system call number get
> > > > > populated.
> > > > > Even the current seccomp_run_filters() function that takes syscall
> > > > > as an
> > > > > argument doesn't seem to use it. Only think I can think of is the BPF
> > > > > evaluator getting it directly from the call stack.
> > > > Been too long and I don't really remember how the seccomp bpf stuff
> > > > works. But I this it comes somehow somewhere from the call to
> > > > seccomp_bpf_load() in sk_run_filter(). The syscall argument in
> > > > seccomp_run_filters() looks like a bug and should probably be
> > > > dropped... It was probably some vestige of auditing denials or
> > > > something. Not certain...
> > > Yeah it happens in seccomp_bpf_load() via asm/syscall.h helpers.
> > > There were checks that used it in the run_filters call initially, but
> > > those faded. I think I hesitated in removing it because, in theory,
> > > it would be cheaper to have it around than ask asm/syscall.h for.
> > > However, cleanliness of code and avoidance of insubstantiated
> > > micro-optimizations steered me away and so the syscall number is still
> > > passed in and unused. Seems like good starter patch ;)
> > So finally I have written the finished draft patches. Both are
> > different to my original patches. There are a few choices I've made
> > which might need debate:
> > - I haven't validated the architecture since the main function
> > definition is wrapped in CONFIG_SECCOMP_FILTER and will simply return
> > 0 if this isn't defined.
> > - I have broken this up into two patches just to show where the main
> > function hooks in.
> > - I have modified the seccomp_attach_filter function to switch
> > between a kernel or user space copy. I don't know how clean that
> > change is.
> > - I have tried my best to break the main loop out into smaller
> > functions so that there isn't one 100 line function. It is still
> > quite long though.
> > - The x86_64 architecture has around 560 system calls defined. I
> > have set the length to 128 bytes (1024 bits) which will allow for
> > 1021 system calls in the bitmap.
> >
> > Now that I understand how the filtering is processed, I worked out
> > why it wasn't necessary (or even a good idea) to replace the current
> > filter list in full. What I don't understand is why the
> > CAP_SYS_ADMIN/no_new_privs restrictions. If a process decides to
> > allow a system call that the current filter list denies, the final
> > result is to still deny the system call. Why restrict the ability to
> > add filters if a process can't gain access to any additional system
> > calls?
> >
> > === PATCH 1 ===
> > From Michael Cassaniti <m.cassaniti@gmail.com>
> >
> > Superficial patch showing hooks for seccomp extended attribute filter
> > code
> >
> > Signed-off-by: Michael Cassaniti <m.cassaniti@gmail.com>
> > ---
> >
> > diff -uprN -X linux-3.5-rp1/Documentation/dontdiff
> > linux-3.5/fs/exec.c linux-3.5-rp1/fs/exec.c
> > --- linux-3.5/fs/exec.c 2012-07-22 06:58:29.000000000 +1000
> > +++ linux-3.5-rp1/fs/exec.c 2012-09-13 12:27:14.966076904 +1000
> > @@ -1537,6 +1537,10 @@ static int do_execve_common(const char *
> > if (retval < 0)
> > goto out;
> >
> > + retval = security_seccomp_from_vfs(bprm);
> > + if (retval < 0)
> > + goto out;
> > +
> > retval = copy_strings_kernel(1, &bprm->filename, bprm);
> > if (retval < 0)
> > goto out;
> > diff -uprN -X linux-3.5-rp1/Documentation/dontdiff
> > linux-3.5/include/linux/seccomp.h linux-3.5-rp1/include/linux/seccomp.h
> > --- linux-3.5/include/linux/seccomp.h 2012-07-22
> > 06:58:29.000000000 +1000
> > +++ linux-3.5-rp1/include/linux/seccomp.h 2012-09-21
> > 12:43:28.215772113 +1000
> > @@ -119,6 +119,14 @@ static inline int seccomp_mode(struct se
> > extern void put_seccomp_filter(struct task_struct *tsk);
> > extern void get_seccomp_filter(struct task_struct *tsk);
> > extern u32 seccomp_bpf_load(int off);
> > +
> > +#define SECCOMP_XATTR_NAME "security.seccomp"
> > +#define SECCOMP_XATTR_LEN 128
> > +#define SECCOMP_XATTR_BIT_EN 0
> > +#define SECCOMP_XATTR_BIT_DEF_ACTION 1
> > +#define SECCOMP_XATTR_BIT_DEF_RETURN 2
> > +#define SECCOMP_XATTR_BITMAP_START 3
> > +
> > #else /* CONFIG_SECCOMP_FILTER */
> > static inline void put_seccomp_filter(struct task_struct *tsk)
> > {
> > diff -uprN -X linux-3.5-rp1/Documentation/dontdiff
> > linux-3.5/include/linux/security.h
> > linux-3.5-rp1/include/linux/security.h
> > --- linux-3.5/include/linux/security.h 2012-07-22
> > 06:58:29.000000000 +1000
> > +++ linux-3.5-rp1/include/linux/security.h 2012-09-21
> > 12:00:24.026007169 +1000
> > @@ -3023,5 +3023,23 @@ static inline void free_secdata(void *se
> > { }
> > #endif /* CONFIG_SECURITY */
> >
> > +#ifdef CONFIG_SECCOMP_FILTER
> > +
> > +extern int append_seccomp_from_vfs(struct linux_binprm *bprm);
> > +
> > +static inline int security_seccomp_from_vfs(struct linux_binprm *bprm)
> > +{
> > + return append_seccomp_from_vfs(bprm);
> > +}
> > +
> > +#else
> > +
> > +static inline int security_seccomp_from_vfs(struct linux_binprm *bprm)
> > +{
> > + return 0;
> > +}
> > +
> > +#endif /* CONFIG_SECCOMP_FILTER */
> > +
> > #endif /* ! __LINUX_SECURITY_H */
> >
> > diff -uprN -X linux-3.5-rp1/Documentation/dontdiff
> > linux-3.5/kernel/seccomp.c linux-3.5-rp1/kernel/seccomp.c
> > --- linux-3.5/kernel/seccomp.c 2012-07-22 06:58:29.000000000 +1000
> > +++ linux-3.5-rp1/kernel/seccomp.c 2012-09-21 13:23:52.254969072
> > +1000
> > @@ -502,3 +502,9 @@ long prctl_set_seccomp(unsigned long sec
> > out:
> > return ret;
> > }
> > +
> > +int append_seccomp_from_vfs(struct linux_binprm *bprm)
> > +{
> > + pr_debug("Entered stub %s\n", __func__);
> > + return 0;
> > +}
> >
> > === PATCH 2 ===
> > From Michael Cassaniti <m.cassaniti@gmail.com>
> >
> > Add seccomp filter via extended attribute
> >
> > Signed-off-by: Michael Cassaniti <m.cassaniti@gmail.com>
> > ---
> > diff -uprN -X linux-3.5-rp1/Documentation/dontdiff
> > linux-3.5-rp1/include/linux/seccomp.h
> > linux-3.5-rp2/include/linux/seccomp.h
> > --- linux-3.5-rp1/include/linux/seccomp.h 2012-09-21
> > 12:43:28.215772113 +1000
> > +++ linux-3.5-rp2/include/linux/seccomp.h 2012-09-21
> > 12:44:09.915410558 +1000
> > @@ -127,6 +127,11 @@ extern u32 seccomp_bpf_load(int off);
> > #define SECCOMP_XATTR_BIT_DEF_RETURN 2
> > #define SECCOMP_XATTR_BITMAP_START 3
> >
> > +#define SECCOMP_FILTER_LOAD_SYSCALL BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
> > BPF_DATA(nr))
> > +#define SECCOMP_FILTER_ALLOW BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW)
> > +#define SECCOMP_FILTER_KILL BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL)
> > +#define SECCOMP_FILTER_DENY BPF_STMT(BPF_RET|BPF_K,
> > (SECCOMP_RET_ERRNO|EPERM))
> > +
> > #else /* CONFIG_SECCOMP_FILTER */
> > static inline void put_seccomp_filter(struct task_struct *tsk)
> > {
> > diff -uprN -X linux-3.5-rp1/Documentation/dontdiff
> > linux-3.5-rp1/kernel/seccomp.c linux-3.5-rp2/kernel/seccomp.c
> > --- linux-3.5-rp1/kernel/seccomp.c 2012-09-21 13:23:52.254969072
> > +1000
> > +++ linux-3.5-rp2/kernel/seccomp.c 2012-09-21 14:43:27.687445601
> > +1000
> > @@ -18,6 +18,9 @@
> > #include <linux/compat.h>
> > #include <linux/sched.h>
> > #include <linux/seccomp.h>
> > +#include <linux/dcache.h> /* for dget() and struct dentry */
> > +#include <linux/binfmts.h> /* for struct binprm */
> > +#include <linux/fs.h> /* for struct inode */
> >
> > /* #define SECCOMP_DEBUG 1 */
> >
> > @@ -224,7 +227,7 @@ static u32 seccomp_run_filters(int sysca
> > *
> > * Returns 0 on success or an errno on failure.
> > */
> > -static long seccomp_attach_filter(struct sock_fprog *fprog)
> > +static long seccomp_attach_filter(struct sock_fprog *fprog, int
> > copy_mode)
> > {
> > struct seccomp_filter *filter;
> > unsigned long fp_size = fprog->len * sizeof(struct sock_filter);
> > @@ -252,16 +255,22 @@ static long seccomp_attach_filter(struct
> >
> > /* Allocate a new seccomp_filter */
> > filter = kzalloc(sizeof(struct seccomp_filter) + fp_size,
> > - GFP_KERNEL|__GFP_NOWARN);
> > + GFP_KERNEL|__GFP_NOWARN);
> > if (!filter)
> > return -ENOMEM;
> > +
> > atomic_set(&filter->usage, 1);
> > filter->len = fprog->len;
> >
> > /* Copy the instructions from fprog. */
> > ret = -EFAULT;
> > - if (copy_from_user(filter->insns, fprog->filter, fp_size))
> > - goto fail;
> > + if (copy_mode) {
> > + if (memcpy(filter->insns, fprog->filter, fp_size))
> > + goto fail;
> > + } else {
> > + if (copy_from_user(filter->insns, fprog->filter, fp_size))
> > + goto fail;
> > + }
> >
> > /* Check and rewrite the fprog via the skb checker */
> > ret = sk_chk_filter(filter->insns, filter->len);
> > @@ -307,7 +316,7 @@ long seccomp_attach_user_filter(char __u
> > #endif
> > if (copy_from_user(&fprog, user_filter, sizeof(fprog)))
> > goto out;
> > - ret = seccomp_attach_filter(&fprog);
> > + ret = seccomp_attach_filter(&fprog, 0);
> > out:
> > return ret;
> > }
> > @@ -503,8 +512,168 @@ out:
> > return ret;
> > }
> >
> > -int append_seccomp_from_vfs(struct linux_binprm *bprm)
> > +int get_seccomp_xattr_from_vfs(struct linux_binprm *bprm,
> > + unsigned char *bitmap)
> > {
> > - pr_debug("Entered stub %s\n", __func__);
> > + int size;
> > + struct dentry *dentry;
> > + struct inode *inode;
> > +
> > + dentry = dget(bprm->file->f_dentry);
> > + if (!dentry)
> > + return -EINVAL;
> > +
> > + inode = dentry->d_inode;
> > + if (!inode || !inode->i_op->getxattr)
> > + return -EOPNOTSUPP;
> > +
> > + size = inode->i_op->getxattr((struct dentry *)dentry,
> > + SECCOMP_XATTR_NAME, bitmap,
> > + SECCOMP_XATTR_LEN);
> > +
> > + if (size == -ENODATA || size == -EOPNOTSUPP)
> > + return -EOPNOTSUPP;
> > +
> > + if (size != SECCOMP_XATTR_LEN) {
> > + pr_notice("%s: Got invalid seccomp xattr for %s\n",
> > + __func__, bprm->filename);
> > + return -EINVAL;
> > + }
> > +
> > return 0;
> > }
> > +
> > +void seccomp_add_bpf_rule(struct sock_filter *filter, unsigned int
> > syscall,
> > + unsigned int rule, unsigned int rules)
> > +{
> > +
> > + /* Adds the appropriate rule to the filter list
> > + * Second last rule is default action
> > + * Last rule is opposite of default action
> > + */
> > +
> > + unsigned int opp_rule = rules - rule - 2;
> > +
> > + /* If syscall matches then jump to opposing rule, else continue */
> > + filter[rule] = (struct sock_filter)
> > + BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, syscall, opp_rule, 0);
> > +}
> > +
> > +void append_seccomp_default_vfs_filters(struct sock_filter *filter,
> > + unsigned int rule,
> > + unsigned int def_action,
> > + unsigned int def_return)
> > +{
> > + /* Add default rule at index rule, opposite rule at index rule+1 */
> > + rule++;
> > + if (def_action) {
> > + filter[rule] = (struct sock_filter)SECCOMP_FILTER_ALLOW;
> > + if (def_return)
> > + filter[rule+1] = (struct sock_filter)
> > + SECCOMP_FILTER_KILL;
> > + else
> > + filter[rule+1] = (struct sock_filter)
> > + SECCOMP_FILTER_DENY;
> > + } else {
> > + if (def_return)
> > + filter[rule] = (struct sock_filter)SECCOMP_FILTER_KILL;
> > + else
> > + filter[rule] = (struct sock_filter)SECCOMP_FILTER_DENY;
> > + filter[rule+1] = (struct sock_filter)SECCOMP_FILTER_ALLOW;
> > + }
> > +}
> > +
> > +int append_seccomp_from_vfs(struct linux_binprm *bprm)
> > +{
> > + unsigned int i = (unsigned int)SECCOMP_XATTR_BITMAP_START;
> > + unsigned int rules = 1; /* There is 1 starting rule */
> > + unsigned int rule = 0;
> > + unsigned int bit;
> > + unsigned int byte;
> > + unsigned char curr_byte;
> > + unsigned int def_action;
> > + unsigned int def_return;
> > + unsigned int curr_action;
> > + unsigned int loop_mode = 1;
> > + int rc = 0;
> > + struct sock_filter *filter;
> > + struct sock_fprog fprog;
> > + unsigned char *bitmap = kmalloc(SECCOMP_XATTR_LEN *
> > sizeof(*bitmap),
> > + GFP_KERNEL);
> > + if (!bitmap)
> > + return -ENOMEM;
> > + if (get_seccomp_xattr_from_vfs(bprm, bitmap)) {
> > + /* Doesn't matter if nothing returned */
> > + goto out;
> > + }
> > +
> > + if (!(bitmap[0] \
> > <http://marc.info/?l=linux-security-module&m=134914287007710&w=2#0> & (1 << \
> > SECCOMP_XATTR_BIT_EN))) + /* Seccomp extended attribute is disabled */
> > + goto out;
> > +
> > + /* Isolate default and return bits */
> > + def_action = (unsigned int)((bitmap[0] \
> > <http://marc.info/?l=linux-security-module&m=134914287007710&w=2#0> & + \
> > (1 << SECCOMP_XATTR_BIT_DEF_ACTION)) + >> \
> > SECCOMP_XATTR_BIT_DEF_ACTION); + def_return = (unsigned int)((bitmap[0] \
> > <http://marc.info/?l=linux-security-module&m=134914287007710&w=2#0> & + \
> > (1 << SECCOMP_XATTR_BIT_DEF_RETURN)) + >> \
> > SECCOMP_XATTR_BIT_DEF_RETURN); +
> > + while (i != SECCOMP_XATTR_LEN * 8) {
> > + bit = i % 8;
> > + byte = i / 8;
> > + curr_byte = bitmap[byte];
> > + curr_action = (unsigned int) ((curr_byte >> bit) & 0x01);
> > +
> > + if ((bit == 0) && (
> > + (def_action && ((0xFF ^ curr_byte) == 0)) ||
> > + ((def_action == 0) && (curr_byte == 0))
> > + )) {
> > + /* All byte aligned bits match default action */
> > + /* Skip checking these bits */
> > + i = i + 8;
> > + }
> > +
> > + if (curr_action != def_action) {
> > + if (loop_mode) {
> > + /* Count the number of set bits */
> > + rules++;
> > + } else {
> > + rule++;
> > + seccomp_add_bpf_rule(filter,
> > + i-SECCOMP_XATTR_BITMAP_START, rule, rules);
> > + }
> > + }
> > +
> > + i++;
> > + if (i >= SECCOMP_XATTR_LEN * 8) {
> > + if (loop_mode) {
> > + /* Go through the loop again, adding filters */
> > + loop_mode = 0;
> > + i = (unsigned int)SECCOMP_XATTR_BITMAP_START;
> > + /* Add allow/deny rules */
> > + rules = rules + 2;
> > + filter = kmalloc(rules * sizeof(*filter),
> > + GFP_KERNEL);
> > + if (!filter) {
> > + rc = -ENOMEM;
> > + goto out;
> > + }
> > + filter[0] \
> > <http://marc.info/?l=linux-security-module&m=134914287007710&w=2#0> = (struct \
> > sock_filter) + SECCOMP_FILTER_LOAD_SYSCALL;
> > + } else {
> > + break;
> > + }
> > + }
> > + }
> > +
> > + append_seccomp_default_vfs_filters(filter, rule, def_action,
> > + def_return);
> > + fprog.len = rules;
> > + fprog.filter = filter;
> > + rc = seccomp_attach_filter(&fprog, 1);
> > + kfree(filter);
> > +out:
> > + kfree(bitmap);
> > + return rc;
> > +}
> >
> >
> > === END PATCHES ===
> >
> > I haven't completed everything in the SubmitChecklist as of yet, but
> > that is in progress.
> >
> > Regards,
> > Michael Cassaniti
>
> Worked my way through the current SubmitChecklist and have passed the
> following steps: 1,2,5,8,10,1213(only CONFIG_PREEMPT),20,22(in similar
> userspace code)
> Currently irrelevant options: 6,7,11,14,15,16,17,18,19,24,25,26
> Couldn't test or unsure how to test: 2c,3,4,9,23
> Not done yet:2b,21
>
> There is a slight modification to remove a compiler warning. I am
> specifically setting the filter pointer (struct sock_filter *filter)
> in append_seccomp_from_vfs() to be 0 initially. The complaint is that
> it may be used unassigned, but that doesn't happen. I believe that is
> the only code change so far.
Hi All,
Just wanted to know what my next steps should be to get these patches looked over. \
Not quite sure on the etiquette from here.
Regards,
Michael Cassaniti
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic