[PATCH RFC 2/2] seccomp: implement SECCOMP_FILTER_FLAG_NO_INHERITANCE

[Dmitry V. Levin]

On Sun, Nov 17, 2019 at 06:52:04PM +0100, Paul Chaignon wrote:
> On Fri, Nov 15, 2019 at 05:56:50PM +0100, Paul Chaignon wrote:
> > On Fri, Nov 15, 2019 at 06:55:55PM +0300, Dmitry V. Levin wrote:
> > > On Fri, Nov 15, 2019 at 04:46:14PM +0100, Paul Chaignon wrote:
> > > > On Thu, Nov 14, 2019 at 07:06:20PM +0100, Paul Chaignon wrote:
> > > > > On Mon, Nov 11, 2019 at 11:44:50PM +0300, Dmitry V. Levin wrote:
> > > > > > On Mon, Nov 11, 2019 at 04:20:20PM +0100, Paul Chaignon wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > > >  /*
> > > > > > >   * All BPF programs must return a 32-bit value.
> > > > > > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > > > > > index 55af6931c6ec..1df5b058b067 100644
> > > > > > > --- a/kernel/fork.c
> > > > > > > +++ b/kernel/fork.c
> > > > > > > @@ -1605,6 +1605,11 @@ static void copy_seccomp(struct task_struct *p)
> > > > > > >  	 */
> > > > > > >  	assert_spin_locked(&current->sighand->siglock);
> > > > > > >  
> > > > > > > +	if (!inherited_seccomp_filter(current)) {
> > > > > > > +		clear_tsk_thread_flag(p, TIF_SECCOMP);
> > > > > > > +		return;
> > > > > > > +	}
> > > > > > 
> > > > > > Since the task can have many seccomp filters (chained by seccomp_filter.prev)
> > > > > > and SECCOMP_FILTER_FLAG_NO_INHERIT flag affects only those filters that were
> > > > > > created using this flag, this code should copy only those filters that are
> > > > > > excluded from the fork inheritance.
> > > > > 
> > > > > Hm, I had the semantics all wrong.  I'll send a v2 with a new patch and
> > > > > test.
> > > > 
> > > > Actually, I think we should just mandate that all seccomp filters in the
> > > > list have the same value for flag SECCOMP_FILTER_FLAG_NO_FORK_INHERIT.
> > > > This should be as simple as checking that the flag has the same value for
> > > > a new filter and its parent.  In case of TSYNC we should be fine since all
> > > > threads need to have the same ancestors as the calling thread.
> > > > 
> > > > If we don't do that, we'll have to inherit partial lists, which prevents
> > > > us from using references since we'll need to modify field .prev of some
> > > > filters.  I'm guessing making actual copies of filters will not be
> > > > acceptable upstream...
> > > > 
> > > > One big issue with this approach is if we want to strace an application
> > > > that itself uses seccomp-bpf.  In that case, the application will likely
> > > > fail to set up its own filter because flag NO_FORK_INHERIT differ.
> > > 
> > > Seccomp filters are ubiquitous nowadays, software like systemd-nspawn
> > > generates and applies seccomp filters by default.  If the proposed feature
> > > does not support this workflow, does it have a chance to be accepted
> > > into the kernel?
> > 
> > Yep, that was my concern too...  I'll have a second look after the weekend
> > in case I missed something, but I don't see any other clear solution at
> > the moment :(
> 
> Actually, we could relax the constraint: when adding a new filter with
> flag NO_FORK_INHERIT, we can simply mandate that all *previous* filters
> (again, we only need to check the previous one) have NO_FORK_INHERIT as
> well.  Then, forks inherit a sublist of filters starting with the first
> filter without NO_FORK_INHERIT set.

Would it be possible to add a traditional seccomp filter after
a NO_FORK_INHERIT one?  If yes, how is it going to work?
If no, is there any security implications?
Anyway, ...

> Since strace will typically install the first seccomp filter, we should be
> fine, right?  What do you think?

... this means that strace --seccomp-bpf would imply -f or not depending
on circumstances, e.g. whether it's invoked in a container or not.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20191118/0f8dfc22/attachment.bin>