tests: add test to run snap inside lxd as a user #4230

Closed
wants to merge 47 commits into
from
Commits
Jump to file or symbol
Failed to load files and symbols.
+34 −7
Split
Viewing a subset of changes. View all

cmd,packaging: make snap-confine setgid root

This patch makes snap-confine also setgid root (after being setuid-root
since forever). This is required to manipulate cgroups inside LXD
containers.

To limit the scope of the change, snap-confine hides the setgid aspect
for most of the code and only restores it for the cgroup manipulation.

Forum: https://forum.snapcraft.io/t/snapcraft-adt-failures-with-the-new-core-release/2850
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
  • Loading branch information...
commit 7722c0404b97fa0ac119acb495caa62c3f5ab321 @zyga zyga committed Nov 16, 2017
View
@@ -52,7 +52,7 @@ fmt: $(foreach dir,$(subdirs),$(wildcard $(srcdir)/$(dir)/*.[ch]))
# installing a fresh copy of snap confine and the appropriate apparmor profile.
.PHONY: hack
hack: snap-confine/snap-confine snap-confine/snap-confine.apparmor snap-update-ns/snap-update-ns snap-seccomp/snap-seccomp
- sudo install -D -m 4755 snap-confine/snap-confine $(DESTDIR)$(libexecdir)/snap-confine
+ sudo install -D -m 6755 snap-confine/snap-confine $(DESTDIR)$(libexecdir)/snap-confine
sudo install -m 644 snap-confine/snap-confine.apparmor $(DESTDIR)/etc/apparmor.d/$(patsubst .%,%,$(subst /,.,$(libexecdir))).snap-confine.real
sudo install -d -m 755 $(DESTDIR)/var/lib/snapd/apparmor/snap-confine/
sudo apparmor_parser -r snap-confine/snap-confine.apparmor
@@ -321,8 +321,8 @@ if CAPS_OVER_SETUID
# Ensure that snap-confine has CAP_SYS_ADMIN capability
setcap cap_sys_admin=pe $(DESTDIR)$(libexecdir)/snap-confine
else
-# Ensure that snap-confine is +s (setuid)
- chmod 4755 $(DESTDIR)$(libexecdir)/snap-confine
+# Ensure that snap-confine is u+s,g+s (setuid and setgid)
+ chmod 6755 $(DESTDIR)$(libexecdir)/snap-confine
endif
##
@@ -131,8 +131,28 @@ int main(int argc, char **argv)
debug("base snap: %s", base_snap_name);
// Who are we?
- uid_t real_uid = getuid();
- gid_t real_gid = getgid();
+ uid_t ruid, euid, suid;
+ gid_t rgid, egid, sgid;
+ getresuid(&ruid, &euid, &suid);
+ getresgid(&rgid, &egid, &sgid);
+ debug("ruid: %d, euid: %d, suid: %d", ruid, euid, suid);
+ debug("rgid: %d, egid: %d, sgid: %d", rgid, egid, sgid);
@jdstrand

jdstrand Nov 17, 2017

Contributor

These extra variables to real_uid and real_gid reduce clarity. I suggest coding the whole thing like this instead (untested):

	// Who are we?
	uid_t real_uid, effective_uid, saved_uid;
	git_t real_gid, effective_gid, saved_gid;
	getresuid(&real_uid, &effective_uid, &saved_uid);
	getresgid(&real_gid, &effective_gid, &saved_gid);
	debug("ruid: %d, euid: %d, suid: %d", real_uid, effective_uid, saved_uid);
	debug("rgid: %d, egid: %d, sgid: %d", real_gid, effective_gid, saved_gid);

	// snap-confine runs as both setuid root and setgid root.
	// Temporarily drop group privileges here and reraise later
	// as needed.
	if (effective_gid == 0 && real_gid != 0) {
		if (setegid(real_gid) != 0) {
			die("cannot set effective group id to %d", real_gid);
		}
	}
...
			// Temporarily raise egid so we can chown the freezer cgroup
			// under LXD
			if (getegid() != 0 && saved_gid == 0) {
				if (setegid(0) != 0) {
			        	die("cannot set effective group id to root");
				}
			}
			sc_cgroup_freezer_join(snap_name, getpid());
			if (geteuid() == 0) {
				if (setegid(real_gid) != 0) {
					die("cannot set effective group id to %d",
					    real_gid);
				}
			}
...
+
+ // If we are running as group root but the real group id is not zero then
+ // the setgid root permission is in effect. To limit the scope of the
+ // change this is causing for the code temporairly set the effective group
+ // id to the real group id. We will change that again below when we
+ // manipulate the freezer cgroup.
+ // Because LXD uses particular permissions for cgroups we need to be
+ // group-root for that operation to succeed.
+ if (euid == 0 && ruid != 0) {
+ if (setegid(ruid) != 0) {
+ die("cannot set effective group id to %d", ruid);
+ }
+ }
@jdstrand

jdstrand Nov 17, 2017

Contributor

You are calling setegid() but checking for, commenting on and setting to uid. See above for suggestion.

@zyga

zyga Nov 17, 2017

Contributor

Oh, man, thanks!

@zyga

zyga Nov 17, 2017

Contributor

In retrospective the non abbreviated names are really really good.

+
+ uid_t real_uid = ruid;
+ gid_t real_gid = rgid;
#ifndef CAPS_OVER_SETUID
// this code always needs to run as root for the cgroup/udev setup,
@@ -226,7 +246,14 @@ int main(int argc, char **argv)
// control group. This simplifies testing if any processes
// belonging to a given snap are still alive.
// See the documentation of the function for details.
+ if (setegid(0) != 0) {
@jdstrand

jdstrand Nov 17, 2017

Contributor

You're going to want to do this:

if (getegid() != 0 && sgid == 0) {
    if (setegid(0) != 0) {
        ...

See above for suggestion.

+ die("cannot set effective group id to root");
+ }
sc_cgroup_freezer_join(snap_name, getpid());
+ if (setegid(rgid) != 0) {
@jdstrand

jdstrand Nov 17, 2017

Contributor

You're going to want to do this:

if (geteuid() == 0) {
    if (setegid(rgid) != 0) {
        ...

See above for suggestion.

+ die("cannot set effective group id to %d",
+ rgid);
+ }
sc_unlock(snap_name, snap_lock_fd);
// Reset path as we cannot rely on the path from the host OS to
@@ -622,7 +622,7 @@ popd
%dir %{_libexecdir}/snapd
# For now, we can't use caps
# FIXME: Switch to "%%attr(0755,root,root) %%caps(cap_sys_admin=pe)" asap!
-%attr(4755,root,root) %{_libexecdir}/snapd/snap-confine
+%attr(6755,root,root) %{_libexecdir}/snapd/snap-confine
%{_libexecdir}/snapd/snap-discard-ns
%{_libexecdir}/snapd/snap-seccomp
%{_libexecdir}/snapd/snap-update-ns
@@ -279,7 +279,7 @@ fi
%dir /var/lib/snapd/seccomp/bpf
%dir /var/lib/snapd/snaps
%dir /var/cache/snapd
-%verify(not user group mode) %attr(04755,root,root) %{_libexecdir}/snapd/snap-confine
+%verify(not user group mode) %attr(06755,root,root) %{_libexecdir}/snapd/snap-confine
%{_mandir}/man1/snap-confine.1.gz
%{_mandir}/man5/snap-discard-ns.5.gz
%{_udevrulesdir}/80-snappy-assign.rules