idmap improvements #1509

brauner · 2017-04-15T11:33:13Z

Signed-off-by: Christian Brauner christian.brauner@ubuntu.com

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

hallyn · 2017-04-15T15:05:03Z

src/lxc/conf.c

+		if (!has_sufficient_privilege &&
+		    lxc_file_cap_is_set(newuidmap_path, CAP_SETUID, CAP_EFFECTIVE) &&
+		    lxc_file_cap_is_set(newuidmap_path, CAP_SETUID, CAP_PERMITTED) &&
+		    sb.st_uid == 0)


File does not need to be owned by root for filecaps to take effect. The sb.st_uid check can be removed afaics.

You would still be able to write to new_{g,u}idmap even if euid > 0?

Write to it? Aren't you going to execute it?

I mean will new{g,u}idmap be able to write to /proc/<pid>/uid_map when it is not uid 0?

Unless of course, uid_map is owned by another the unpriv user.

hallyn · 2017-04-15T15:08:25Z

src/lxc/conf.c

-		free(cmdpath);
+	newuidmap_path = on_path("newuidmap", NULL);
+	if (newuidmap_path) {
+		bool has_sufficient_privilege = false;


This block sure seems like a good candidate for a helper function :) Not requisite for this patch, just a comment.

Yeah, thought about it as well. Might just write it now.

So if you turn that block into a helper then it goes away? :)

In either case, the check here to break out of the loop needs to be for fret right? or not?

Oops, that last comment was supposed to go with the block below.

hallyn · 2017-04-15T15:11:14Z

src/lxc/conf.c

 		} else {
 			left = LXC_IDMAPLEN - (pos - buf);
 			fill = snprintf(pos, left, "\n");
 			if (fill <= 0 || fill >= left)
 				SYSERROR("Too many {g,u}id mappings defined.");
 			pos += fill;
-			ret = system(buf);
+			fret = system(buf);
 		}
 		if (ret)


Should this check still be on ret, or on fret? What was the reason for switching from ret to fret?

ret is used in the new{g,u}idmap block before and might get initialized to a non-zero value but the for-loop, as unlikely as this is, might not be run and then you'd exit with error.

brauner · 2017-04-15T16:20:55Z

src/lxc/conf.c

+		return -1;
+	}
+
+	if (has_privileged_newuidmap && !has_privileged_newgidmap) {


I think it makes sense to error out when we find only one of the two binaries.

brauner · 2017-04-15T16:21:46Z

src/lxc/utils.c

+	/* Check if it has file capabilities. */
+	if (lxc_file_cap_is_set(path, CAP_SETUID, CAP_EFFECTIVE) &&
+	    lxc_file_cap_is_set(path, CAP_SETUID, CAP_PERMITTED) &&
+	    st.st_uid == uid) {


I left this in for now. But if we're sure I'll remove it before the merge.

brauner · 2017-04-15T16:22:53Z

src/lxc/conf.c

-	if (cmdpath) {
-		use_shadow = 1;
-		free(cmdpath);
+	newuidmap_path = on_path("newuidmap", NULL);


I didn't move the on_path() part into the setuid checks. But I probably could. Should we do that, what do you think?

hallyn · 2017-04-15T16:47:22Z

Quoting Christian Brauner (notifications@github.com):

brauner commented on this pull request. > + newgidmap_path = on_path("newgidmap", NULL); + if (newgidmap_path) { + has_privileged_newgidmap = is_setuid_binary(newgidmap_path, 0); + if (!has_privileged_newgidmap) { + ERROR("The newgidmap binary lacks sufficient privilege."); + free(newgidmap_path); + return -1; + } + free(newgidmap_path); + use_shadow++; + } else if (euid > 0) { + ERROR("The newgidmap binary is missing and euid is %d.", euid); + return -1; + } + + if (has_privileged_newuidmap && !has_privileged_newgidmap) { I think it makes sense to error out when we find only one of the two binaries.

Yes. I'm only suggesting using a helper to check if the binary is there and sufficiently privileged, since you have around 30 lines of code duplicated.

hallyn · 2017-04-15T16:48:57Z

Quoting Christian Brauner (notifications@github.com):

brauner commented on this pull request. > + if (ret < 0) { + SYSERROR("Failed to stat \"%s\".", newuidmap_path); + free(newuidmap_path); + return -1; + } + + /* Check if the newuidmap binary is setuid. */ + if ((sb.st_mode & S_ISUID) && sb.st_uid == 0) + has_sufficient_privilege = true; + + #if HAVE_LIBCAP + /* Check if it has file capabilities. */ + if (!has_sufficient_privilege && + lxc_file_cap_is_set(newuidmap_path, CAP_SETUID, CAP_EFFECTIVE) && + lxc_file_cap_is_set(newuidmap_path, CAP_SETUID, CAP_PERMITTED) && + sb.st_uid == 0) I mean will `new{g,u}idmap` be able to write to `/proc/<pid>/uid_map` when it is not uid `0`?

Yes, you only need CAP_SETUID for uid_map and CAP_SETGID for gid_map.

hallyn · 2017-04-15T16:50:09Z

Quoting Christian Brauner (notifications@github.com):

brauner commented on this pull request. > * will protected it by preventing another user from being handed the * range by shadow. */ - cmdpath = on_path("newuidmap", NULL); - if (cmdpath) { - use_shadow = 1; - free(cmdpath); + newuidmap_path = on_path("newuidmap", NULL); I didn't move the `on_path()` part into the setuid checks. But I probably could. Should we do that, what do you think?

I don't know, depends on whether the privilege-only check is more reusable. I suspect just a on_path_and_privileged("newuidmap", CAP_SETUID) and on_path_privileged("newgidmap", CAP_SETGID | CAP_SETUID) works out nicely.

Add two new helpers that allow to determine whether a given proc or file has a capability in the given set and move lxc_cap_is_set() to static function that both call internally. Closes lxc#296. Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

brauner · 2017-04-15T21:04:33Z

@hallyn, updated.

hallyn · 2017-04-15T22:05:28Z

Hi,

looks good. Just two more comments for now:

unfortunately (if i read this right) writing to map_file also requires cap_sys_admin against the user_ns, in addition to cap_setuid/cap_setgid.
newgidmap is actually setuid-root, not setgid-root. So the filecaps check should check for CAP_SETGID and CAP_SYS_ADMIN, but the 'check if the binary is setuid' check should check for setuid-root.

The new{g,u}idmap binaries where a source of trouble for users when they lacked sufficient privileges. This commit adds code to check for sufficient privilege. It checks whether new{g,u}idmap is root owned and has the setuid bit set and if it doesn't it checks whether new{g,u}idmap is root owned and has CAP_SETUID in its CAP_PERMITTED and CAP_EFFECTIVE set. Closes lxc#296. Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

brauner · 2017-04-15T23:03:32Z

Hi,

looks good. Just two more comments for now:

unfortunately (if i read this right) writing to map_file also requires cap_sys_admin against the user_ns, in addition to cap_setuid/cap_setgid.

newgidmap is actually setuid-root, not setgid-root. So the filecaps check should check for CAP_SETGID and CAP_SYS_ADMIN, but the 'check if the binary is setuid' check should check for setuid-root.

As discussed on IRC CAP_SYS_ADMIN is actually not required. What we need to check is:

1. newuidmap has the setuid bit set --> good to go
1. newuidmap has CAP_SETUID file capability in CAP_EFFECTIVE and CAP_PERMITTED --> good to go
1. newgidmap has the setuid bit set --> good to go
1. newgidmap has CAP_SETGID file capability in CAP_EFFECTIVE and CAP_PERMITTED --> good to go.
  Anything else we don't consider for now.

brauner · 2017-04-15T23:42:36Z

@hallyn, should be ready to merge.

hallyn · 2017-04-16T15:20:48Z

Awesome, thanks.

Signed-off-by: Evgeni Golov <evgeni@debian.org>

fix typo introduced in #1509

Signed-off-by: Evgeni Golov <evgeni@debian.org>

conf: lxc_map_ids() non-functional changes

4bc3b75

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

brauner force-pushed the 2017-04-15/improve_lxc_id_map branch 2 times, most recently from e78ea55 to 208a0da Compare April 15, 2017 13:13

brauner changed the title ~~conf: lxc_map_ids() non-functional changes~~ idmap improvements Apr 15, 2017

brauner requested a review from hallyn April 15, 2017 14:09

hallyn reviewed Apr 15, 2017

View reviewed changes

brauner force-pushed the 2017-04-15/improve_lxc_id_map branch from 208a0da to 238ab1f Compare April 15, 2017 16:19

brauner commented Apr 15, 2017

View reviewed changes

brauner force-pushed the 2017-04-15/improve_lxc_id_map branch 3 times, most recently from f5f350c to 608d9d0 Compare April 15, 2017 19:07

brauner changed the title ~~idmap improvements~~ [WIP] idmap improvements Apr 15, 2017

brauner force-pushed the 2017-04-15/improve_lxc_id_map branch from 608d9d0 to ac5f143 Compare April 15, 2017 20:34

brauner changed the title ~~[WIP] idmap improvements~~ idmap improvements Apr 15, 2017

brauner force-pushed the 2017-04-15/improve_lxc_id_map branch from ac5f143 to 22f5365 Compare April 15, 2017 20:49

brauner force-pushed the 2017-04-15/improve_lxc_id_map branch from 22f5365 to 032768a Compare April 15, 2017 20:50

brauner force-pushed the 2017-04-15/improve_lxc_id_map branch from 032768a to 2c45c2c Compare April 15, 2017 21:24

Christian Brauner added 2 commits April 16, 2017 00:59

conf: improve log when mounting rootfs

91c3e28

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

brauner force-pushed the 2017-04-15/improve_lxc_id_map branch from 2c45c2c to 91c3e28 Compare April 15, 2017 23:00

hallyn merged commit 93caf97 into lxc:master Apr 16, 2017

evgeni added a commit to evgeni/lxc that referenced this pull request Apr 17, 2017

fix typo introduced in lxc#1509

d26582c

Signed-off-by: Evgeni Golov <evgeni@debian.org>

brauner pushed a commit that referenced this pull request Apr 17, 2017

Merge pull request #1511 from evgeni/typo

e6ceab4

fix typo introduced in #1509

stgraber pushed a commit that referenced this pull request Apr 18, 2017

fix typo introduced in #1509

c2fc203

Signed-off-by: Evgeni Golov <evgeni@debian.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

idmap improvements #1509

idmap improvements #1509

brauner commented Apr 15, 2017

hallyn Apr 15, 2017

brauner Apr 15, 2017

hallyn Apr 15, 2017

brauner Apr 15, 2017

brauner Apr 15, 2017

hallyn Apr 15, 2017

brauner Apr 15, 2017

hallyn Apr 15, 2017

hallyn Apr 15, 2017

hallyn Apr 15, 2017

brauner Apr 15, 2017

brauner Apr 15, 2017

brauner Apr 15, 2017

brauner Apr 15, 2017

hallyn commented Apr 15, 2017 via email

hallyn commented Apr 15, 2017 via email

hallyn commented Apr 15, 2017 via email

brauner commented Apr 15, 2017

hallyn commented Apr 15, 2017

brauner commented Apr 15, 2017

brauner commented Apr 15, 2017

hallyn commented Apr 16, 2017

idmap improvements #1509

idmap improvements #1509

Conversation

brauner commented Apr 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallyn commented Apr 15, 2017 via email

hallyn commented Apr 15, 2017 via email

hallyn commented Apr 15, 2017 via email

brauner commented Apr 15, 2017

hallyn commented Apr 15, 2017

brauner commented Apr 15, 2017

brauner commented Apr 15, 2017

hallyn commented Apr 16, 2017