You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gilbert Song <so...@gmail.com> on 2019/01/03 01:58:25 UTC
Re: Review Request 68018: Added `SeccompFilter` class.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68018/#review211623
-----------------------------------------------------------
src/linux/seccomp/seccomp.hpp
Lines 42-43 (patched)
<https://reviews.apache.org/r/68018/#comment296984>
Add parameters?
src/linux/seccomp/seccomp.hpp
Lines 43 (patched)
<https://reviews.apache.org/r/68018/#comment296986>
defaults to None()?
src/linux/seccomp/seccomp.cpp
Lines 138 (patched)
<https://reviews.apache.org/r/68018/#comment296997>
Do I understand correctly that this would not impact on the setuid/setgid after pivot_root in mesos/launch.cpp?
The side effect is on the task: the task cannot setuid/setgid and cannot change capabilities?
src/linux/seccomp/seccomp.cpp
Lines 141-144 (patched)
<https://reviews.apache.org/r/68018/#comment297000>
Instead of always set `SCMP_FLTATR_CTL_NNP`. Should we consider to check root privileges first (e.g., `geteuid() != 0`)?
src/linux/seccomp/seccomp.cpp
Lines 147 (patched)
<https://reviews.apache.org/r/68018/#comment297010>
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?
src/linux/seccomp/seccomp.cpp
Lines 183 (patched)
<https://reviews.apache.org/r/68018/#comment297011>
capabilities->get(capabilities::BOUNDING)
src/linux/seccomp/seccomp.cpp
Lines 185 (patched)
<https://reviews.apache.org/r/68018/#comment297013>
nits:
To be explicit on `syscall.includes().capabilities_size() > 0` ?
src/linux/seccomp/seccomp.cpp
Lines 187 (patched)
<https://reviews.apache.org/r/68018/#comment297014>
Ditto if we could leverage Enum, instead of `int`.
- Gilbert Song
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 bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/linux/seccomp/seccomp.hpp PRE-CREATION
> src/linux/seccomp/seccomp.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68018/diff/11/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 68018: Added `SeccompFilter` class.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Jan. 3, 2019, 1:58 a.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.
>
> Gilbert Song wrote:
> Gotcha. Does it imply that task launched by docker daemon with seccomp profile enabled cannot setuid (assuming docker relies on execve/execvpe)?
No, it doesn't. See my comment https://reviews.apache.org/r/68018/#comment297515 starting with `Docker daemon can not be used to run arbitrary programs...`.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68018/#review211623
-----------------------------------------------------------
On Nov. 8, 2018, 3:24 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68018/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2018, 3:24 p.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
>
>
Re: Review Request 68018: Added `SeccompFilter` class.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Jan. 3, 2019, 1:58 a.m., Gilbert Song wrote:
> > src/linux/seccomp/seccomp.cpp
> > Lines 138 (patched)
> > <https://reviews.apache.org/r/68018/diff/11/?file=2116580#file2116580line138>
> >
> > Do I understand correctly that this would not impact on the setuid/setgid after pivot_root in mesos/launch.cpp?
> >
> > The side effect is on the task: the task cannot setuid/setgid and cannot change capabilities?
> Do I understand correctly that this would not impact on the setuid/setgid after pivot_root in mesos/launch.cpp?
Yes, correct.
From https://www.kernel.org/doc/Documentation/prctl/no_new_privs.txt:
```
Note that no_new_privs does not prevent privilege changes that do not
involve execve. An appropriately privileged task can still call
setuid(2) and receive SCM_RIGHTS datagrams.
```
AFAIU, `NO_NEW_PRIVS` prevents changing current setuid/setgid/capabilities when calling `execve` with these bits set on executable.
> On Jan. 3, 2019, 1:58 a.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`)?
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.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68018/#review211623
-----------------------------------------------------------
On Nov. 8, 2018, 3:24 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68018/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2018, 3:24 p.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 bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/linux/seccomp/seccomp.hpp PRE-CREATION
> src/linux/seccomp/seccomp.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68018/diff/14/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 68018: Added `SeccompFilter` class.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Jan. 3, 2019, 1:58 a.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?
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]`
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68018/#review211623
-----------------------------------------------------------
On Nov. 8, 2018, 3:24 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68018/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2018, 3:24 p.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 bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/linux/seccomp/seccomp.hpp PRE-CREATION
> src/linux/seccomp/seccomp.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68018/diff/12/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 68018: Added `SeccompFilter` class.
Posted by Gilbert Song <so...@gmail.com>.
> 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
>
>