cmd/snap-confine: discard stale mount namespaces #4324

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

zyga commented Nov 29, 2017

This patch enables snap-confine to discard stale mount namespaces. The
code already contained to logic to detect a stale namespace. The patch
introduces an additional check. Once we know of a stale namespace we
check if it would be safe to discard it by looking at the processes that
inhabit it. This can be done reliably by enumerating the freezer group.

If we find any process we consider it unsafe for the mount namespace to
be discarded but we log a diagnostic message that system administrators
can see (it can be an important security fact that a particular snap is
using an older revision of the base snap).

The code is made a little bit more generic so that we can also filter by
user identifier. This is likely to be used by the upcoming per-user
mount namespace feature.

The apparmor profile is extended slightly to be able to read the
cgroup.procs file and to unmount existing namespaces. That last thing is
a bit of a magic / broken rule as apparmor has some issues that we don't
fully understand whenever setns is called.

< jjohansen> uh, setns is a known problem, double setns doesn't
change that
< jjohansen> hey zyga-ubuntu, setns won't necessarily break
everything dependent on several things. Partly to due with how mounts
are shared or whether they are cloned ..

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

Just a bit of self-review for explaining one aspect.

}
-bool sc_should_populate_ns_group(struct sc_ns_group *group)
+bool sc_should_populate_ns_group(struct sc_ns_group * group)
@zyga

zyga Nov 29, 2017

Contributor

Oddly this is coming out of my machine's make fmt. If indent changed rules again I will call for a flag day and propose that we move to clang-format that at least has a very readable and sane output without 80 column constraint.

@jdstrand

jdstrand Nov 29, 2017

Contributor

/me happens to like the 80 column constraint. ;)

@zyga zyga requested a review from jdstrand Nov 29, 2017

We had a chat on IRC about this and we decided this needs a bit of reorganizing (but not much) which would put the PR inline with the kernel design/implementation for setns and reduce what snap-confine is required to do.

Contributor

zyga commented Nov 30, 2017

FYI

-				if (umount2(mnt_fname, MNT_DETACH|UMOUNT_NOFOLLOW) < 0) {
+				if (umount2
+				    (mnt_fname,
+				     MNT_DETACH | UMOUNT_NOFOLLOW) < 0) {

This is indent being inconsistent with itself. :/

Contributor

zyga commented Nov 30, 2017

I've implemented the version that calls setns only once now. I'll close this PR and open a 2nd one when tests are happy.

cmd/snap-confine: discard stale mount namespaces
This patch enables snap-confine to discard stale mount namespaces. The
code already contained to logic to detect a stale namespace. The patch
introduces an additional check. Once we know of a stale namespace we
check if it would be safe to discard it by looking at the processes that
inhabit it. This can be done reliably by enumerating the freezer group.

If we find any process we consider it unsafe for the mount namespace to
be discarded but we log a diagnostic message that system administrators
can see (it can be an important security fact that a particular snap is
using an older revision of the base snap).

The code is made a little bit more generic so that we can also filter by
user identifier. This is likely to be used by the upcoming per-user
mount namespace feature.

The apparmor profile is extended slightly to be able to read the
cgroup.procs file and to unmount existing namespaces. That last thing is
a bit of a magic / broken rule as apparmor has some issues that we don't
fully understand whenever setns is called.

< jjohansen> uh, setns is a known problem, double setns doesn't
change that
< jjohansen> hey zyga-ubuntu, setns won't necessarily break
everything dependent on several things. Partly to due with how mounts
are shared or whether they are cloned ..

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Contributor

zyga commented Dec 4, 2017

I'm closing this PR in favour of the v2 approach.

@zyga zyga closed this Dec 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment