Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces/seccomp: add bind() syscall for forced-devmode systems #3394
Conversation
All checks have passed
| @@ -128,6 +129,10 @@ func addContent(securityTag string, opts interfaces.ConfinementOptions, snippetF | ||
| buffer.Write(defaultTemplate) | ||
| buffer.WriteString(snippetForTag) | ||
| + if release.ReleaseInfo.ForceDevMode() { |
All checks have passed
mvo5
added this to the 2.27 milestone
May 24, 2017
| +// from go uses bind for IPv4/IPv6 support detection when loaded. This | ||
| +// gets a problem for all hooks when they use snapctl. On systems which | ||
| +// use AppArmor the bind() call is properly mediated and the caller gets | ||
| +// an error back. |
niemeyer
May 24, 2017
Contributor
Not sure I understand the problem given this description. If we get an error when apparmor is enabled, that would mean the bind always fails. If that's the case, why are systems without apparmor special?
Also, can we please stop using the term "force devmode" in that context? This sounds like a misnomer for "no apparmor support". We should slowly move all terminology towards that, both in our use and in the code. Some of the misleading conversations and proposals we had recently around that seems to come from that (for example, the idea of disabling seccomp improperly).
morphis
May 24, 2017
Contributor
Not sure I understand the problem given this description. If we get an error when apparmor is enabled, that would mean the bind always fails. If that's the case, why are systems without apparmor special?
The difference is that AppArmor gives back an error to the caller of bind() where Seccomp doesn`t. So in scenarios where you have both AppArmor takes first and returns an error to the caller and Seccomp is never reached. When AppArmor isn't present the check goes directly to Seccomp which doesn't return and error but kills the caller. That makes the difference we're trying to fix here.
Also, can we please stop using the term "force devmode" in that context? This sounds like a misnomer for "no apparmor support".
I am open to that. Just reused "forced devmode" as it was already there and used elsewhere. What we should keep in mind is that we a term which refers to a mode which misses a LSM rather than calling out explicitly AppArmor as this could be SELinux in the future too.
jdstrand
approved these changes
May 25, 2017
•
The conditional policy looks fine, though I'd like to see a clearer comment myself. I'm approving based on the logic of the code and the conditions where it is applied and not on the conversations surrounding the terminology of 'forced devmode'.
| +// from go uses bind for IPv4/IPv6 support detection when loaded. This | ||
| +// gets a problem for all hooks when they use snapctl. On systems which | ||
| +// use AppArmor the bind() call is properly mediated and the caller gets | ||
| +// an error back. |
niemeyer
May 24, 2017
Contributor
Not sure I understand the problem given this description. If we get an error when apparmor is enabled, that would mean the bind always fails. If that's the case, why are systems without apparmor special?
Also, can we please stop using the term "force devmode" in that context? This sounds like a misnomer for "no apparmor support". We should slowly move all terminology towards that, both in our use and in the code. Some of the misleading conversations and proposals we had recently around that seems to come from that (for example, the idea of disabling seccomp improperly).
morphis
May 24, 2017
Contributor
Not sure I understand the problem given this description. If we get an error when apparmor is enabled, that would mean the bind always fails. If that's the case, why are systems without apparmor special?
The difference is that AppArmor gives back an error to the caller of bind() where Seccomp doesn`t. So in scenarios where you have both AppArmor takes first and returns an error to the caller and Seccomp is never reached. When AppArmor isn't present the check goes directly to Seccomp which doesn't return and error but kills the caller. That makes the difference we're trying to fix here.
Also, can we please stop using the term "force devmode" in that context? This sounds like a misnomer for "no apparmor support".
I am open to that. Just reused "forced devmode" as it was already there and used elsewhere. What we should keep in mind is that we a term which refers to a mode which misses a LSM rather than calling out explicitly AppArmor as this could be SELinux in the future too.
added some commits
May 29, 2017
Some checks were not successful
codecov-io
commented
May 29, 2017
•
Edited 1 time
-
codecov-io
May 30, 2017
-
May 30, 2017
Codecov Report
@@ Coverage Diff @@
## master #3394 +/- ##
=========================================
+ Coverage 77.49% 77.5% +<.01%
=========================================
Files 369 369
Lines 25472 25474 +2
=========================================
+ Hits 19740 19744 +4
+ Misses 3980 3979 -1
+ Partials 1752 1751 -1
Continue to review full report at Codecov.
|
| +// snapctl working on these systems. | ||
| +// This will only get added to the seccomp default policy for a snap | ||
| +// if the system we're running on doesn't support AppArmor. In all | ||
| +// other cases bind() isn't added to the seccomp policy. |
| @@ -549,3 +549,23 @@ pwritev | ||
| # of socket(), bind(), connect(), etc individually. | ||
| socketcall | ||
| `) | ||
| + | ||
| +// bindSyscallWorkaround specifies the bind() syscall for systems which | ||
| +// don't use AppArmor but only seccomp. In those cases the net package |
niemeyer
May 29, 2017
Contributor
Sorry for being perhaps pedantic, but still doesn't sound quite right. What Go uses in the net package doesn't depend on whether apparmor is enabled or not. The behavior of Go is always the same.
Please let me give it a try:
// Go's net package attempts to bind early to check whether IPv6 is available or not.
// For systems with apparmor enabled, this will be mediated and cause an error to be
// returned. Without apparmor, the call goes through to seccomp and the process is
// killed instead of just getting the error.
//
// For that reason once apparmor is disabled the seccomp profile is given access
// to bind, so that these processes are not improperly killed. There is on going
// work to make seccomp return an error in those cases as well and log the error.
// Once that's in place we can drop this hack.
How does that sound? @jdstrand, is this right?
This explanation actually makes me wonder about the point from @mvo5 a bit more. I wonder if we shouldn't simply disable seccomp altogether if apparmor is not around, until this is fixed. We're special casing one specific call because it hurts us, but my guess is that there will be other snaps with the exact same kind of problem on different syscalls out there.
jdstrand
May 29, 2017
Contributor
@niemeyer - your text is indeed better.
As for only supporting both enabled or both disabled, this was always the plan until more recently when people argued that there may be some security benefit to enabling seccomp without apparmor. I agree that while the seccomp-only mediation is not complete by any means, there is security benefit to having only it enabled. We discussed how bind() may not be the only syscall that is problematic but I feel that there shouldn't be too many of these based on the history of the snappy security policy. I'm fine with going back to the original 'all or nothing' design, but assuming people still want seccomp-only support, I think a wait and see approach is ok; we can always revisit this if/when needed.
morphis
May 30, 2017
Contributor
I am ok with either way. The only thing we need to ensure, if we decide to only allow seccomp+apparmor, is that we need to enforce that. Right now we offer our users to build with --disable-apparmor --enable-seccomp. We should not allow this anymore then to prevent these cases as we can't control how anyone is using our software.
morphis commentedMay 24, 2017
This fixes LP #1644573
Also see https://forum.snapcraft.io/t/hooks-calling-snapctl-are-broken-with-just-seccomp-enabled/658/7 for the discussion.