Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
overlord/ifacestate: setup seccomp security on startup #2821
Conversation
All checks have passed
zyga
added
the
Critical
label
Feb 9, 2017
| + securityBackends := m.repo.Backends() | ||
| + | ||
| + // Get all the snap infos | ||
| + snaps, err := snapstate.ActiveInfos(m.state) |
pedronis
Feb 9, 2017
Contributor
this gives you only active snaps (you need All to get all of them), I know we recently changed enable/disable to remove profiles os inactive ones don't have them, but not sure if it means there is something to do for old inactive stuff
zyga
Feb 9, 2017
Contributor
For inactive stuff there's nothing to do ATM: first of all profiles are not versioned (just the current revision can exist) and second of all once activated those profiles will get generated.
| +# FIXME: restore detailed parameter filtering for quota control once | ||
| +# snap-confine can read this syntax. | ||
| +# See LP:#1662489 for context. | ||
| +quotactl |
jdstrand
Feb 9, 2017
Contributor
NAK. quotactl was never granted before and this grants more privilege than mount-observe is meant to have.
| -ioctl - !TIOCSTI | ||
| +# FIXME: replace this with the filter of TIOCSTI once snap-confine can read this syntax | ||
| +# See LP:#1662489 for context. | ||
| +ioctl |
jdstrand
Feb 9, 2017
Contributor
This seems like the wrong approach. We will never be able to adjust seccomp policy that requires changes to snap-confine if we wait for 'snap-confine can read this syntax' since a package might be stuck for a long time. What is wrong with the exec'ing snap-confine from core approach?
zyga
Feb 9, 2017
Contributor
We've discussed this on a hangout so I'll just reply with a quick summary:
- this is a temporary fix to unbreak the world
- we will use snap-confine from the core
- the patch a09abe7 should be reverted as a part of that work
jdstrand
Feb 9, 2017
Contributor
Similar as with quota, can you do this:
# TIOCSTI allows for faking input (man tty_ioctl)
# TODO: this should be scaled back even more
#ioctl - !TIOCSTI
# FIXME: replace this with the filter of TIOCSTI once snap-confine can read this syntax
# See LP:#1662489 for context.
ioctl
| + continue | ||
| + } | ||
| + // Refresh security of this snap and backend | ||
| + if err := backend.Setup(snapInfo, opts, m.repo); err != nil { |
jdstrand
Feb 9, 2017
Contributor
How atomic is this operation? Since this happens outside of install/refresh/revert it seems possible for a snap to try to start while this is happening. Is there a race here?
zyga
Feb 9, 2017
Contributor
It runs before anything else can run, during setup of the snapd process.
jdstrand
Feb 9, 2017
Contributor
Ok, but OnClassic, imagine a core refresh comes in at the same time that the user is trying to execute the command who's profile is being refreshed. What will happen?
jdstrand
reviewed
Feb 9, 2017
•
| -quotactl Q_XGETQUOTA - - - | ||
| -quotactl Q_XGETQSTAT - - - | ||
| +# FIXME: restore quotactl with parameter filtering once # snap-confine can read | ||
| +this syntax. # See LP:#1662489 for context. |
jdstrand
Feb 9, 2017
•
Edited 1 time
-
jdstrand
Feb 9, 2017
Contributor
-
Feb 9, 2017
You have invalid syntax here ('this' wrapped around without a comment). Also, rather than removing the rules, can you just comment them out? Eg:
# FIXME: restore quotactl with parameter filtering once snap-confine can read
# this syntax. See LP:#1662489 for context.
#quotactl Q_GETQUOTA - - -
#quotactl Q_GETINFO - - -
#quotactl Q_GETFMT - - -
#quotactl Q_XGETQUOTA - - -
#quotactl Q_XGETQSTAT - - -
| -ioctl - !TIOCSTI | ||
| +# FIXME: replace this with the filter of TIOCSTI once snap-confine can read this syntax | ||
| +# See LP:#1662489 for context. | ||
| +ioctl |
jdstrand
Feb 9, 2017
Contributor
This seems like the wrong approach. We will never be able to adjust seccomp policy that requires changes to snap-confine if we wait for 'snap-confine can read this syntax' since a package might be stuck for a long time. What is wrong with the exec'ing snap-confine from core approach?
zyga
Feb 9, 2017
Contributor
We've discussed this on a hangout so I'll just reply with a quick summary:
- this is a temporary fix to unbreak the world
- we will use snap-confine from the core
- the patch a09abe7 should be reverted as a part of that work
jdstrand
Feb 9, 2017
Contributor
Similar as with quota, can you do this:
# TIOCSTI allows for faking input (man tty_ioctl)
# TODO: this should be scaled back even more
#ioctl - !TIOCSTI
# FIXME: replace this with the filter of TIOCSTI once snap-confine can read this syntax
# See LP:#1662489 for context.
ioctl
| + continue | ||
| + } | ||
| + // Refresh security of this snap and backend | ||
| + if err := backend.Setup(snapInfo, opts, m.repo); err != nil { |
jdstrand
Feb 9, 2017
Contributor
How atomic is this operation? Since this happens outside of install/refresh/revert it seems possible for a snap to try to start while this is happening. Is there a race here?
zyga
Feb 9, 2017
Contributor
It runs before anything else can run, during setup of the snapd process.
jdstrand
Feb 9, 2017
Contributor
Ok, but OnClassic, imagine a core refresh comes in at the same time that the user is trying to execute the command who's profile is being refreshed. What will happen?
zyga commentedFeb 9, 2017
This patch attempts to unbreak the world by automatically refreshing the
seccomp profiles on startp of snapd. Seccomp profiles are not expensive
to compute and don't require compilation or loading.
This lets us fix the problem that surfaced when snapd started generating
a syntax, in the seccomp profile, that only new snap-confine
understands. Because snap-confine does not conceptually re-execute yet
everyone that is on the edge channel is now experiencing problems.
This patch should be applied along the next patch that generates
compatible seccomp profiles, without the new syntax.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com