mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilbert Song <songzihao1...@gmail.com>
Subject Re: Review Request 68018: Added `SeccompFilter` class.
Date Tue, 15 Jan 2019 02:29:58 GMT


> On Jan. 2, 2019, 5:58 p.m., Gilbert Song wrote:
> > src/linux/seccomp/seccomp.cpp
> > Lines 141-144 (patched)
> > <https://reviews.apache.org/r/68018/diff/11/?file=2116580#file2116580line141>
> >
> >     Instead of always set `SCMP_FLTATR_CTL_NNP`. Should we consider to check root
privileges first (e.g., `geteuid() != 0`)?
> 
> Andrei Budnik wrote:
>     By default, libseccomp sets `true` to the `SCMP_FLTATR_CTL_NNP` flag
>     https://github.com/seccomp/libseccomp/blob/1e64feb5f1a9ea02687228e3073e8b784a04ce46/src/db.c#L960
>     
>     Hence, all Seccomp test pass even after removing `seccomp_attr_set(ctx, SCMP_FLTATR_CTL_NNP,
1)`. Also, this means that Docker daemon launches its containers with this flag set by default
(as they also use libseccomp).
>     
>     Disabling `SCMP_FLTATR_CTL_NNP` flag for a `root` means that Seccomp filter can be
reverted anytime. So, disabling this flag is meaningless.

Gotcha. Does it imply that task launched by docker daemon with seccomp profile enabled cannot
setuid (assuming docker relies on execve/execvpe)?


> On Jan. 2, 2019, 5:58 p.m., Gilbert Song wrote:
> > src/linux/seccomp/seccomp.cpp
> > Lines 147 (patched)
> > <https://reviews.apache.org/r/68018/diff/11/?file=2116580#file2116580line147>
> >
> >     Could we use `foreach (const ContainerSeccompProfile::Architecture& arch,
profile.architectures())`?
> >     
> >     So that it avoids the implicit conversion to `int` and also avoid the `static_cast`
below?
> 
> Andrei Budnik wrote:
>     I've already replied to the same comment above from Qian.
>     
>     The return type of `architectures()` is `const ::google::protobuf::RepeatedField<int>&`,
so it won't compile:
>     
>     `../../src/linux/seccomp/seccomp.cpp:147:85: error: invalid conversion from ‘int’
to ‘mesos::slave::ContainerSeccompProfile::Architecture {aka mesos::slave::ContainerSeccompProfile_Architecture}’
[-fpermissive]`

Just checked the .pb.h file. Good to know, thanks!


- Gilbert


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68018/#review211623
-----------------------------------------------------------


On Nov. 8, 2018, 7:24 a.m., Andrei Budnik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68018/
> -----------------------------------------------------------
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9034
>     https://issues.apache.org/jira/browse/MESOS-9034
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> `SeccompFilter` class is a wrapper for `libseccomp` API. Its main
> purpose is to provide a translation of the `ContainerSeccompProfile`
> message into calls of `libseccomp` API.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf 
>   src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132 
>   src/linux/seccomp/seccomp.hpp PRE-CREATION 
>   src/linux/seccomp/seccomp.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68018/diff/15/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message