cmd/libsnap: add helper for dropping permissions

[zyga]

This patch adds a helper function for dropping elevated privileges.

Since snap-confine runs as root via the setuid-root mechanism we always
start with elevated permissions. For added security and usability,
before exiting the program in case of a specific error, we can drop the
extra permissions and then run any extra helper / debug support code,
such as a string manipulation routine.

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

[zyga]

The test failure is related to some breakage in ubuntu-image and is not related to this pull request.

[jdstrand]

I think with the suggested changes below, you can reduce this to ruid and rgid with getuid() and getgid().

[jdstrand]

Since only setgroups needs privs (ie, CAP_SETGID), because snap-confine may be installed on systems with fscaps and since this is a library function, let's do this a bit differently.

Eg, add something like this:

#include <sys/capability.h>

bool has_capability(const char *s) {
	int i, res;
	bool rc = false;

	cap_t caps;
	cap_value_t cap;
	cap_flag_value_t cap_flags_value;

	if (cap_from_name(s, &cap) < 0) {
		die("could not cap_from_name()");
	}

	if ((caps = cap_get_proc()) == NULL) {
		die("could not cap_get_proc()");
	}

	res = cap_get_flag(caps, cap, CAP_EFFECTIVE, &cap_flags_value);
	cap_free(caps);
	if (res < 0) {
		die("could not cap_get_flag()");
	}

	if (cap_flags_value == CAP_SET) {
		rc = true;
	}

	return rc;
}

Then, check if we have the capability of using setgroups, and only then call it:

// Drop extra group memberships when we can and use our primary
// real gid for portability.
gid_t gid_list[1];
gid_list[0] = rgid;
if (has_capability("cap_setgid") && setgroups(1, gid_list) < 0) {
    die("cannot set supplementary group identifiers");
}
// Switch to real group ID
...

[jdstrand]

Note, for the above, need libcap-dev and compile with -lcap.

[zyga]

Done, something like that but with security-irrelevant details tweaked.

[jdstrand]

This is where you could put a comment about being paranoid (but see below):

// With the above, this should never happen but be paranoid to help
// future-proof code changes. Specifically, if our real gid was not
// root, but one of uid/euid still are root, die(). Same for if our
// real uid was not root, but one of gid/egid are root, die().

[zyga]

Done

[jdstrand]

This should say 'uid and/or euid still elevated'.

[jdstrand]

This should say 'gid and/or egid still elevated'.

[jdstrand]

In considering this more, as a library function, I don't think we need the if (rgid != 0 && (getuid() == 0 || geteuid() == 0)) and if (ruid != 0 && (getgid() == 0 || getegid() == 0)) extra checks here, or a setgroups check because this function is self-contained and intended for one use-- to drop everything. Ie, you call sc_privs_drop() and we have a very clear line of permanent privilege dropping steps where if anything fails along the way, die() with a simple error. In other parts of the codebase, we drop, raise, drop, permanently drop, etc, each with a different context (eg, sometimes only raise euid) so the extra future-proofing checks in those places are worthwhile (imo).

[zyga]

Have a look at the state now and tell me if you feel we should drop the extra checks. I don't mind having them and I would like to get back on the main update-ns track.

PS: Thanks a lot for reminding me about capabilities, I didn't consider that root permissions can be partitioned while writing this.

[jdstrand]

You still want to cap_free() if cap_get_flag() fails which is why it was where it was in my suggestion.

[zyga]

Wait, even if we die? EDIT: done just in case it matters

[jdstrand]

It isn't strictly required but is good practice, sure.

[zyga]

Done now :-)

[jdstrand]

Well, I used 'res' in my suggested function so that you could run cap_get_flag and store result in 'res', cap_free(), then check 'res' so you only needed to have cap_free() once. This is not a blocker if you prefer it this way.

[jdstrand]

As mentioned, please get rid of this conditional. If non-root, the setgid() and setuid() calls are harmless. Consider if euid != 0 but egid == 0 for some reason; we still want to drop.

[zyga]

Ack, this is why the first version of the branch had separate gid / uid checks but I see why it is wrong to check for just one here. Removed.

[jdstrand]

Well, again, IMO you can drop these checks. See previous comments.

[zyga]

Removed

[jdstrand]

You don't need euid, suid, egid or sgid here (after you remove the next line).

[zyga]

Removed

[zyga]

libcap-dev on 14.04 doesn't ship the .pc file, eh ...

[zyga]

I'll change this so that we don't need to depend on libcap but only use it if it is available. This will match the expectations in particular distributions better and will avoid the issue with apt that makes it difficult to add a new dependency to an existing package.

[jdstrand]

Looks fine codewise, thanks for the updates.

[jdstrand]

Note, the gid test here isn't testing a real situation since snap-confine is either 4755 or has fscaps. It will never be 2755 or 6755 (unless you maybe do that in a spread test).

If you are concerned with these situations, perhaps add a comment:

// Practically, we can only test priv dropping when installed 4755 (setuid
// root) because the test can't fix up the uids/gids when this test runs as
// root (eg, under sudo or as root). This is because while the uid/gid are
// root, we can put euid/egid where we want, but glibc will helpfully and
// correctly set ruid/rgid, euid/egid and suid/sgid to non-root when using
// setuid/setgid from root to non-root. (man 2 setuid)

[jdstrand]

You still want to cap_free() if cap_get_flag() fails which is why it was where it was in my suggestion.

[zyga]

Wait, even if we die? EDIT: done just in case it matters

[jdstrand]

It isn't strictly required but is good practice, sure.

[zyga]

Done now :-)

[jdstrand]

Well, I used 'res' in my suggested function so that you could run cap_get_flag and store result in 'res', cap_free(), then check 'res' so you only needed to have cap_free() once. This is not a blocker if you prefer it this way.

[jdstrand]

You had a debug() statement here that you dropped. That is fine to keep.

[zyga]

We don't need to remove libcap as the particular issue with apt doesn't affect it :-)

[tyhicks]

Nitpick: this is an unnecessary translation. sc_has_capability() could easily take a cap_value_t instead of a string and not lose any readability.

[jdstrand]

You're right but I thought as a library it might be handy to take as a string ('cap_chown' looks nice as a security policy writer after all :). cap_from_name() also returns -1 for unknown so we didn't need to do CAP_IS_SUPPORTED which if we didn't use cap_from_name(), we'd want to add.

[tyhicks]

This uid variable is only used in an error message. Is that intended?