Debian Bug report logs - #777649
unblock: cgmanager/0.33-2+deb8u1

Package: release.debian.org; Maintainer for release.debian.org is Debian Release Team <debian-release@lists.debian.org>;

Reported by: Serge Hallyn <serge.hallyn@ubuntu.com>

Date: Wed, 11 Feb 2015 04:39:02 UTC

Severity: normal

Tags: confirmed

Done: Niels Thykier <niels@thykier.net>

Bug is archived. No further changes may be made.

Toggle useless messages

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to debian-bugs-dist@lists.debian.org, Debian Release Team <debian-release@lists.debian.org>:
Bug#777649; Package release.debian.org. (Wed, 11 Feb 2015 04:39:07 GMT) (full text, mbox, link).


Acknowledgement sent to Serge Hallyn <serge.hallyn@ubuntu.com>:
New Bug report received and forwarded. Copy sent to Debian Release Team <debian-release@lists.debian.org>. (Wed, 11 Feb 2015 04:39:07 GMT) (full text, mbox, link).


Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: Serge Hallyn <serge.hallyn@ubuntu.com>
To: submit@bugs.debian.org
Subject: cgmanager security update for jessie
Date: Wed, 11 Feb 2015 04:36:59 +0000
Package: release.debian.org
Usertags: jessie-pu

A security issue was found in cgmanager, allowing root-owned privileged
containers to fully administer cgroups on the host.  Two other issues
were found which allow cgmanager to be crashed by unprivileged users.
These have all been fixed in sid. The debdiff below, against the current
jessie package, fixes them for jessie.

debdiff:

diff -Nru cgmanager-0.33/debian/changelog cgmanager-0.33/debian/changelog
--- cgmanager-0.33/debian/changelog	2014-10-13 18:35:43.000000000 -0500
+++ cgmanager-0.33/debian/changelog	2015-01-26 09:15:49.000000000 -0600
@@ -1,3 +1,16 @@
+cgmanager (0.33-3) testing; urgency=medium
+
+  * SECURITY UPDATE: Cross-cgroup resource control bypass.
+    - debian/patches/0003-make-sure-to-check-cgroup-hierarchy.patch, modify
+      cgmanager.c to verify that requests are allowed under the caller's
+      cgroup.
+    - CVE-2014-1425
+  * 0004-chown-stop-cgmanager-crash-on-chown-of-bad-file.patch and
+    0005-prevent-some-cgmanager-asserts.patch: prevent cgmanager
+    crashing on unhandled asserts or dbus error (LP: #1407787)
+
+ -- Serge Hallyn <serge.hallyn@ubuntu.com>  Mon, 26 Jan 2015 09:12:02 -0600
+
 cgmanager (0.33-2) unstable; urgency=medium
 
   * Cherrypick two upstream patches to ensure that 'movepid all' continues
diff -Nru cgmanager-0.33/debian/patches/0003-make-sure-to-check-cgroup-hierarchy.patch cgmanager-0.33/debian/patches/0003-make-sure-to-check-cgroup-hierarchy.patch
--- cgmanager-0.33/debian/patches/0003-make-sure-to-check-cgroup-hierarchy.patch	1969-12-31 18:00:00.000000000 -0600
+++ cgmanager-0.33/debian/patches/0003-make-sure-to-check-cgroup-hierarchy.patch	2015-01-26 09:15:58.000000000 -0600
@@ -0,0 +1,201 @@
+From 6267916d4ea939794e0583cd8b08bd0b9594a6e2 Mon Sep 17 00:00:00 2001
+From: Serge Hallyn <serge.hallyn@ubuntu.com>
+Date: Wed, 26 Nov 2014 01:00:10 -0600
+Subject: [PATCH 1/1] make sure to check cgroup hierarchy
+
+Some cases weren't doing that, although at least those were still
+checking for proper ownership.
+
+Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
+---
+ cgmanager.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
+ 1 file changed, 80 insertions(+), 5 deletions(-)
+
+Index: cgmanager-0.33/cgmanager.c
+===================================================================
+--- cgmanager-0.33.orig/cgmanager.c
++++ cgmanager-0.33/cgmanager.c
+@@ -558,13 +558,20 @@ next:
+ int get_value_main(void *parent, const char *controller, const char *cgroup,
+ 		const char *key, struct ucred p, struct ucred r, char **value)
+ {
+-	char path[MAXPATHLEN];
++	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
+ 
+ 	if (!sane_cgroup(cgroup)) {
+ 		nih_error("%s: unsafe cgroup", __func__);
+ 		return -1;
+ 	}
+ 
++	// Get p's current cgroup in pcgpath
++	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
++		nih_error("%s: Could not determine the proxy's cgroup for %s",
++				__func__, controller);
++		return -1;
++	}
++
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+@@ -577,6 +584,14 @@ int get_value_main(void *parent, const c
+ 		return -1;
+ 	}
+ 
++	// Make sure target cgroup is under proxy's
++	int plen = strlen(pcgpath);
++	if (strncmp(pcgpath, path, plen) != 0) {
++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
++			r.pid);
++		return -1;
++	}
++
+ 	/* append the filename */
+ 	if (strlen(path) + strlen(key) + 2 > MAXPATHLEN) {
+ 		nih_error("%s: filename too long for cgroup %s key %s", __func__, path, key);
+@@ -608,19 +623,34 @@ int set_value_main(const char *controlle
+ 		struct ucred r)
+ 
+ {
+-	char path[MAXPATHLEN];
++	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
+ 
+ 	if (!sane_cgroup(cgroup)) {
+ 		nih_error("%s: unsafe cgroup", __func__);
+ 		return -1;
+ 	}
+ 
++	// Get p's current cgroup in pcgpath
++	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
++		nih_error("%s: Could not determine the proxy's cgroup for %s",
++				__func__, controller);
++		return -1;
++	}
++
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+ 		return -1;
+ 	}
+ 
++	// Make sure target cgroup is under proxy's
++	int plen = strlen(pcgpath);
++	if (strncmp(pcgpath, path, plen) != 0) {
++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
++			r.pid);
++		return -1;
++	}
++
+ 	/* Check access rights to the cgroup directory */
+ 	if (!may_access(r.pid, r.uid, r.gid, path, O_RDONLY)) {
+ 		nih_error("%s: Pid %d may not access %s\n", __func__, r.pid, path);
+@@ -823,7 +853,7 @@ next:
+ int get_tasks_main(void *parent, const char *controller, const char *cgroup,
+ 			struct ucred p, struct ucred r, int32_t **pids)
+ {
+-	char path[MAXPATHLEN];
++	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
+ 	const char *key = "tasks";
+ 	int alloced_pids = 0, nrpids = 0;
+ 
+@@ -832,12 +862,27 @@ int get_tasks_main(void *parent, const c
+ 		return -1;
+ 	}
+ 
++	// Get p's current cgroup in pcgpath
++	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
++		nih_error("%s: Could not determine the proxy's cgroup for %s",
++				__func__, controller);
++		return -1;
++	}
++
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+ 		return -1;
+ 	}
+ 
++	// Make sure target cgroup is under proxy's
++	int plen = strlen(pcgpath);
++	if (strncmp(pcgpath, path, plen) != 0) {
++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
++			r.pid);
++		return -1;
++	}
++
+ 	/* Check access rights to the cgroup directory */
+ 	if (!may_access(r.pid, r.uid, r.gid, path, O_RDONLY)) {
+ 		nih_error("%s: Pid %d may not access %s\n", __func__, r.pid, path);
+@@ -906,7 +951,7 @@ int collect_tasks(void *parent, const ch
+ 		struct ucred p, struct ucred r, int32_t **pids,
+ 		int *alloced_pids, int *nrpids)
+ {
+-	char path[MAXPATHLEN];
++	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
+ 	nih_local char *rpath = NULL;
+ 
+ 	if (!sane_cgroup(cgroup)) {
+@@ -914,12 +959,27 @@ int collect_tasks(void *parent, const ch
+ 		return -1;
+ 	}
+ 
++	// Get p's current cgroup in pcgpath
++	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
++		nih_error("%s: Could not determine the proxy's cgroup for %s",
++				__func__, controller);
++		return -1;
++	}
++
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+ 		return -2;
+ 	}
+ 
++	// Make sure target cgroup is under proxy's
++	int plen = strlen(pcgpath);
++	if (strncmp(pcgpath, path, plen) != 0) {
++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
++			r.pid);
++		return -1;
++	}
++
+ 	/* Check access rights to the cgroup directory */
+ 	if (!may_access(r.pid, r.uid, r.gid, path, O_RDONLY)) {
+ 		nih_error("%s: Pid %d may not access %s\n", __func__, r.pid, path);
+@@ -982,7 +1042,7 @@ next:
+ int list_children_main(void *parent, const char *controller, const char *cgroup,
+ 			struct ucred p, struct ucred r, char ***output)
+ {
+-	char path[MAXPATHLEN];
++	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
+ 
+ 	*output = NULL;
+ 	if (!sane_cgroup(cgroup)) {
+@@ -990,12 +1050,27 @@ int list_children_main(void *parent, con
+ 		return -1;
+ 	}
+ 
++	// Get p's current cgroup in pcgpath
++	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
++		nih_error("%s: Could not determine the proxy's cgroup for %s",
++				__func__, controller);
++		return -1;
++	}
++
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+ 		return -1;
+ 	}
+ 
++	// Make sure target cgroup is under proxy's
++	int plen = strlen(pcgpath);
++	if (strncmp(pcgpath, path, plen) != 0) {
++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
++			r.pid);
++		return -1;
++	}
++
+ 	/* Check access rights to the cgroup directory */
+ 	if (!may_access(r.pid, r.uid, r.gid, path, O_RDONLY)) {
+ 		nih_error("%s: Pid %d may not access %s\n", __func__, r.pid, path);
diff -Nru cgmanager-0.33/debian/patches/0004-chown-stop-cgmanager-crash-on-chown-of-bad-file.patch cgmanager-0.33/debian/patches/0004-chown-stop-cgmanager-crash-on-chown-of-bad-file.patch
--- cgmanager-0.33/debian/patches/0004-chown-stop-cgmanager-crash-on-chown-of-bad-file.patch	1969-12-31 18:00:00.000000000 -0600
+++ cgmanager-0.33/debian/patches/0004-chown-stop-cgmanager-crash-on-chown-of-bad-file.patch	2015-01-26 09:16:00.000000000 -0600
@@ -0,0 +1,26 @@
+From a7fddc8078e7a1fc1d8e33516c90d1a37b8bfc8c Mon Sep 17 00:00:00 2001
+From: Serge Hallyn <serge.hallyn@ubuntu.com>
+Date: Mon, 15 Dec 2014 18:34:27 -0600
+Subject: [PATCH 1/4] chown: stop cgmanager crash on chown of bad file
+
+Wrong error function was being used.
+
+Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
+---
+ cgmanager.c | 3 +--
+ 1 file changed, 1 insertion(+), 2 deletions(-)
+
+Index: cgmanager-0.33/cgmanager.c
+===================================================================
+--- cgmanager-0.33.orig/cgmanager.c
++++ cgmanager-0.33/cgmanager.c
+@@ -525,8 +525,7 @@ int chmod_main(const char *controller, c
+ 	}
+ 
+ 	if (file && ( strchr(file, '/') || strchr(file, '\\')) ) {
+-		nih_dbus_error_raise_printf (DBUS_ERROR_INVALID_ARGS,
+-				"invalid file");
++		nih_error("%s: invalid file", __func__);
+ 		return -1;
+ 	}
+ 
diff -Nru cgmanager-0.33/debian/patches/0005-prevent-some-cgmanager-asserts.patch cgmanager-0.33/debian/patches/0005-prevent-some-cgmanager-asserts.patch
--- cgmanager-0.33/debian/patches/0005-prevent-some-cgmanager-asserts.patch	1969-12-31 18:00:00.000000000 -0600
+++ cgmanager-0.33/debian/patches/0005-prevent-some-cgmanager-asserts.patch	2015-01-26 09:16:02.000000000 -0600
@@ -0,0 +1,44 @@
+From c56249afbccfb66f4896c1fc3380e7c1459c1afc Mon Sep 17 00:00:00 2001
+From: Serge Hallyn <serge.hallyn@ubuntu.com>
+Date: Mon, 29 Dec 2014 01:19:56 -0600
+Subject: [PATCH 4/4] prevent some cgmanager asserts
+
+Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
+---
+ cgmanager.c | 9 ++++++---
+ 1 file changed, 6 insertions(+), 3 deletions(-)
+
+Index: cgmanager-0.33/cgmanager.c
+===================================================================
+--- cgmanager-0.33.orig/cgmanager.c
++++ cgmanager-0.33/cgmanager.c
+@@ -899,7 +899,8 @@ int get_tasks_main(void *parent, const c
+ 
+ 	*pids = NULL;
+ 	if (file_read_pids(parent, path, pids, &alloced_pids, &nrpids) < 0) {
+-		nih_free(*pids);
++		if (*pids)
++			nih_free(*pids);
+ 		return -1;
+ 	}
+ 	return nrpids;
+@@ -1007,7 +1008,8 @@ int get_tasks_recursive_main(void *paren
+ 	if (strcmp(controller, "all") != 0 && !strchr(controller, ',')) {
+ 		if (collect_tasks(parent, controller, cgroup, p, r, pids,
+ 				&alloced_pids, &nrpids) < 0) {
+-			nih_free(*pids);
++			if (*pids)
++				nih_free(*pids);
+ 			return -1;
+ 		}
+ 		return nrpids;
+@@ -1028,7 +1030,8 @@ int get_tasks_recursive_main(void *paren
+ 		if (ret == -2)  // permission denied - ignore
+ 			goto next;
+ 		if (ret != 0) {
+-			nih_free(*pids);
++			if (*pids)
++				nih_free(*pids);
+ 			return -1;
+ 		}
+ next:
diff -Nru cgmanager-0.33/debian/patches/series cgmanager-0.33/debian/patches/series
--- cgmanager-0.33/debian/patches/series	2014-10-13 18:32:58.000000000 -0500
+++ cgmanager-0.33/debian/patches/series	2015-01-26 09:11:59.000000000 -0600
@@ -1,2 +1,5 @@
 0001-do_move_pid_main-don-t-break-out-of-while-loop-on-er.patch
 0002-check-enabled-column-when-parsing-proc-cgroups.patch
+0003-make-sure-to-check-cgroup-hierarchy.patch
+0004-chown-stop-cgmanager-crash-on-chown-of-bad-file.patch
+0005-prevent-some-cgmanager-asserts.patch



Information forwarded to debian-bugs-dist@lists.debian.org, Debian Release Team <debian-release@lists.debian.org>:
Bug#777649; Package release.debian.org. (Wed, 11 Feb 2015 06:45:08 GMT) (full text, mbox, link).


Acknowledgement sent to Niels Thykier <niels@thykier.net>:
Extra info received and forwarded to list. Copy sent to Debian Release Team <debian-release@lists.debian.org>. (Wed, 11 Feb 2015 06:45:08 GMT) (full text, mbox, link).


Message #10 received at 777649@bugs.debian.org (full text, mbox, reply):

From: Niels Thykier <niels@thykier.net>
To: Serge Hallyn <serge.hallyn@ubuntu.com>, 777649@bugs.debian.org
Subject: Re: Bug#777649: cgmanager security update for jessie
Date: Wed, 11 Feb 2015 07:40:52 +0100
Control: tags -1 moreinfo

On 2015-02-11 05:36, Serge Hallyn wrote:
> Package: release.debian.org
> Usertags: jessie-pu
> 
> A security issue was found in cgmanager, allowing root-owned privileged
> containers to fully administer cgroups on the host.  Two other issues
> were found which allow cgmanager to be crashed by unprivileged users.
> These have all been fixed in sid. The debdiff below, against the current
> jessie package, fixes them for jessie.
> 
> debdiff:
> 
> [...]
> + 
> ++	// Make sure target cgroup is under proxy's
> ++	int plen = strlen(pcgpath);
> ++	if (strncmp(pcgpath, path, plen) != 0) {
> ++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
> ++			r.pid);
> ++		return -1;
> ++	}
> ++
> [...]

Hi,

Is this truly a sufficient test?  The above only tests that pcgpath is a
prefix of path.  I do not know exactly what these paths are, so I have
to ask.

Consider:

  pcgpath = "root"
  pcpgpath = "root-acually-not-really"
  plen = strlen(pcgpath) (= 4)

So if only the first plen characters match, they will be considered
equal.  If you know, cases like this cannot happen, then it is fine.  I
just wanted to double check.

Thanks,
~Niels







Added tag(s) moreinfo. Request was from Niels Thykier <niels@thykier.net> to 777649-submit@bugs.debian.org. (Wed, 11 Feb 2015 06:45:09 GMT) (full text, mbox, link).


Information forwarded to debian-bugs-dist@lists.debian.org, Debian Release Team <debian-release@lists.debian.org>:
Bug#777649; Package release.debian.org. (Wed, 11 Feb 2015 19:00:13 GMT) (full text, mbox, link).


Acknowledgement sent to Serge Hallyn <serge.hallyn@ubuntu.com>:
Extra info received and forwarded to list. Copy sent to Debian Release Team <debian-release@lists.debian.org>. (Wed, 11 Feb 2015 19:00:13 GMT) (full text, mbox, link).


Message #17 received at 777649@bugs.debian.org (full text, mbox, reply):

From: Serge Hallyn <serge.hallyn@ubuntu.com>
To: Niels Thykier <niels@thykier.net>
Cc: 777649@bugs.debian.org, Serge Hallyn <serge.hallyn@ubuntu.com>
Subject: Re: Bug#777649: cgmanager security update for jessie
Date: Wed, 11 Feb 2015 18:57:35 +0000
Quoting Niels Thykier (niels@thykier.net):
> Control: tags -1 moreinfo
> 
> On 2015-02-11 05:36, Serge Hallyn wrote:
> > Package: release.debian.org
> > Usertags: jessie-pu
> > 
> > A security issue was found in cgmanager, allowing root-owned privileged
> > containers to fully administer cgroups on the host.  Two other issues
> > were found which allow cgmanager to be crashed by unprivileged users.
> > These have all been fixed in sid. The debdiff below, against the current
> > jessie package, fixes them for jessie.
> > 
> > debdiff:
> > 
> > [...]
> > + 
> > ++	// Make sure target cgroup is under proxy's
> > ++	int plen = strlen(pcgpath);
> > ++	if (strncmp(pcgpath, path, plen) != 0) {
> > ++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
> > ++			r.pid);
> > ++		return -1;
> > ++	}
> > ++
> > [...]
> 
> Hi,
> 
> Is this truly a sufficient test?  The above only tests that pcgpath is a
> prefix of path.  I do not know exactly what these paths are, so I have
> to ask.
> 
> Consider:
> 
>   pcgpath = "root"
>   pcpgpath = "root-acually-not-really"
>   plen = strlen(pcgpath) (= 4)
> 
> So if only the first plen characters match, they will be considered
> equal.  If you know, cases like this cannot happen, then it is fine.  I
> just wanted to double check.

Thanks, I appreciate the extra set of eyes.

The situation is that the task making the request (or proxying the request)
is supposed to be locked under its current cgroup, say /a/b/c.  It's making
a request pertaining to some cgroup X.  We want to make sure that X is
under /a/b/c.  Hence the path prefix test.

thanks,
-serge



Information forwarded to debian-bugs-dist@lists.debian.org, Debian Release Team <debian-release@lists.debian.org>:
Bug#777649; Package release.debian.org. (Wed, 11 Feb 2015 21:03:05 GMT) (full text, mbox, link).


Acknowledgement sent to Niels Thykier <niels@thykier.net>:
Extra info received and forwarded to list. Copy sent to Debian Release Team <debian-release@lists.debian.org>. (Wed, 11 Feb 2015 21:03:05 GMT) (full text, mbox, link).


Message #22 received at 777649@bugs.debian.org (full text, mbox, reply):

From: Niels Thykier <niels@thykier.net>
To: Serge Hallyn <serge.hallyn@ubuntu.com>
Cc: 777649@bugs.debian.org
Subject: Re: Bug#777649: cgmanager security update for jessie
Date: Wed, 11 Feb 2015 22:01:32 +0100
On 2015-02-11 19:57, Serge Hallyn wrote:
> Quoting Niels Thykier (niels@thykier.net):
>> Control: tags -1 moreinfo
>>
>> On 2015-02-11 05:36, Serge Hallyn wrote:
>>> Package: release.debian.org
>>> Usertags: jessie-pu
>>>
>>> A security issue was found in cgmanager, allowing root-owned privileged
>>> containers to fully administer cgroups on the host.  Two other issues
>>> were found which allow cgmanager to be crashed by unprivileged users.
>>> These have all been fixed in sid. The debdiff below, against the current
>>> jessie package, fixes them for jessie.
>>>
>>> debdiff:
>>>
>>> [...]
>>> + 
>>> ++	// Make sure target cgroup is under proxy's
>>> ++	int plen = strlen(pcgpath);
>>> ++	if (strncmp(pcgpath, path, plen) != 0) {
>>> ++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
>>> ++			r.pid);
>>> ++		return -1;
>>> ++	}
>>> ++
>>> [...]
>>
>> Hi,
>>
>> Is this truly a sufficient test?  The above only tests that pcgpath is a
>> prefix of path.  I do not know exactly what these paths are, so I have
>> to ask.
>>
>> Consider:
>>
>>   pcgpath = "root"
>>   pcpgpath = "root-acually-not-really"
>>   plen = strlen(pcgpath) (= 4)
>>
>> So if only the first plen characters match, they will be considered
>> equal.  If you know, cases like this cannot happen, then it is fine.  I
>> just wanted to double check.
> 
> Thanks, I appreciate the extra set of eyes.
> 
> The situation is that the task making the request (or proxying the request)
> is supposed to be locked under its current cgroup, say /a/b/c.  It's making
> a request pertaining to some cgroup X.  We want to make sure that X is
> under /a/b/c.  Hence the path prefix test.
> 
> thanks,
> -serge
> 

Ok, are we guaranteed that pcgpath ends with the path separator?  Consider:

  "/foo/bar"
  "/foo/bar2/somewhere-else"

Unless the path separator is included in the end (i.e. it always uses
"/foo/bar/" instead of "/foo/bar"), then it might still be possible to
by-pass the prefix test.

~Niels





Information forwarded to debian-bugs-dist@lists.debian.org, Debian Release Team <debian-release@lists.debian.org>:
Bug#777649; Package release.debian.org. (Thu, 12 Feb 2015 00:51:04 GMT) (full text, mbox, link).


Acknowledgement sent to Serge Hallyn <serge.hallyn@ubuntu.com>:
Extra info received and forwarded to list. Copy sent to Debian Release Team <debian-release@lists.debian.org>. (Thu, 12 Feb 2015 00:51:04 GMT) (full text, mbox, link).


Message #27 received at 777649@bugs.debian.org (full text, mbox, reply):

From: Serge Hallyn <serge.hallyn@ubuntu.com>
To: Niels Thykier <niels@thykier.net>
Cc: 777649@bugs.debian.org, Serge Hallyn <serge.hallyn@ubuntu.com>
Subject: Re: Bug#777649: cgmanager security update for jessie
Date: Thu, 12 Feb 2015 00:48:10 +0000
Quoting Niels Thykier (niels@thykier.net):
> Ok, are we guaranteed that pcgpath ends with the path separator?  Consider:

No in fact I think we're guaranteed it won't.

>   "/foo/bar"
>   "/foo/bar2/somewhere-else"
> 
> Unless the path separator is included in the end (i.e. it always uses
> "/foo/bar/" instead of "/foo/bar"), then it might still be possible to
> by-pass the prefix test.

Indeed it will, thanks!  I'm going to write a patch which commonizes
the checks and takes care of this case.  I'll get it into the next
release and send a patch for jessie tonight or tomorrow.

Note that ownership checks still apply, so the task in /foo/bar
could only affect /foo/bar2  if it owns /foo/bar2.  Or if it is
root, but root in a privileged container will be locked under
/lxc/$container.  So this should be less urgent than the larger
fix already addressed.



Information forwarded to debian-bugs-dist@lists.debian.org, Debian Release Team <debian-release@lists.debian.org>:
Bug#777649; Package release.debian.org. (Thu, 12 Feb 2015 04:36:04 GMT) (full text, mbox, link).


Acknowledgement sent to Serge Hallyn <serge.hallyn@ubuntu.com>:
Extra info received and forwarded to list. Copy sent to Debian Release Team <debian-release@lists.debian.org>. (Thu, 12 Feb 2015 04:36:04 GMT) (full text, mbox, link).


Message #32 received at 777649@bugs.debian.org (full text, mbox, reply):

From: Serge Hallyn <serge.hallyn@ubuntu.com>
To: Niels Thykier <niels@thykier.net>, 777649@bugs.debian.org
Subject: Re: Bug#777649: cgmanager security update for jessie
Date: Thu, 12 Feb 2015 04:32:21 +0000
Here is a new debdiff.  (tested in its original upstream version
in v0.36)   Maybe it would've been easier to squash the two patches,
but this way it's easier to tell whether the patches match what is
upstream.

diff -Nru cgmanager-0.33/debian/changelog cgmanager-0.33/debian/changelog
--- cgmanager-0.33/debian/changelog	2014-10-13 18:35:43.000000000 -0500
+++ cgmanager-0.33/debian/changelog	2015-02-11 22:28:11.000000000 -0600
@@ -1,3 +1,18 @@
+cgmanager (0.33-3) testing; urgency=medium
+
+  * SECURITY UPDATE: Cross-cgroup resource control bypass.
+    - debian/patches/0003-make-sure-to-check-cgroup-hierarchy.patch, modify
+      cgmanager.c to verify that requests are allowed under the caller's
+      cgroup.
+    - CVE-2014-1425
+  * 0004-chown-stop-cgmanager-crash-on-chown-of-bad-file.patch and
+    0005-prevent-some-cgmanager-asserts.patch: prevent cgmanager
+    crashing on unhandled asserts or dbus error (LP: #1407787)
+  * 0006-fix-subdirectory-check: further fix to the previous patch for
+    CVE-2014-1425.
+
+ -- Serge Hallyn <serge.hallyn@ubuntu.com>  Mon, 26 Jan 2015 09:12:02 -0600
+
 cgmanager (0.33-2) unstable; urgency=medium
 
   * Cherrypick two upstream patches to ensure that 'movepid all' continues
diff -Nru cgmanager-0.33/debian/patches/0003-make-sure-to-check-cgroup-hierarchy.patch cgmanager-0.33/debian/patches/0003-make-sure-to-check-cgroup-hierarchy.patch
--- cgmanager-0.33/debian/patches/0003-make-sure-to-check-cgroup-hierarchy.patch	1969-12-31 18:00:00.000000000 -0600
+++ cgmanager-0.33/debian/patches/0003-make-sure-to-check-cgroup-hierarchy.patch	2015-01-26 09:15:58.000000000 -0600
@@ -0,0 +1,201 @@
+From 6267916d4ea939794e0583cd8b08bd0b9594a6e2 Mon Sep 17 00:00:00 2001
+From: Serge Hallyn <serge.hallyn@ubuntu.com>
+Date: Wed, 26 Nov 2014 01:00:10 -0600
+Subject: [PATCH 1/1] make sure to check cgroup hierarchy
+
+Some cases weren't doing that, although at least those were still
+checking for proper ownership.
+
+Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
+---
+ cgmanager.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
+ 1 file changed, 80 insertions(+), 5 deletions(-)
+
+Index: cgmanager-0.33/cgmanager.c
+===================================================================
+--- cgmanager-0.33.orig/cgmanager.c
++++ cgmanager-0.33/cgmanager.c
+@@ -558,13 +558,20 @@ next:
+ int get_value_main(void *parent, const char *controller, const char *cgroup,
+ 		const char *key, struct ucred p, struct ucred r, char **value)
+ {
+-	char path[MAXPATHLEN];
++	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
+ 
+ 	if (!sane_cgroup(cgroup)) {
+ 		nih_error("%s: unsafe cgroup", __func__);
+ 		return -1;
+ 	}
+ 
++	// Get p's current cgroup in pcgpath
++	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
++		nih_error("%s: Could not determine the proxy's cgroup for %s",
++				__func__, controller);
++		return -1;
++	}
++
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+@@ -577,6 +584,14 @@ int get_value_main(void *parent, const c
+ 		return -1;
+ 	}
+ 
++	// Make sure target cgroup is under proxy's
++	int plen = strlen(pcgpath);
++	if (strncmp(pcgpath, path, plen) != 0) {
++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
++			r.pid);
++		return -1;
++	}
++
+ 	/* append the filename */
+ 	if (strlen(path) + strlen(key) + 2 > MAXPATHLEN) {
+ 		nih_error("%s: filename too long for cgroup %s key %s", __func__, path, key);
+@@ -608,19 +623,34 @@ int set_value_main(const char *controlle
+ 		struct ucred r)
+ 
+ {
+-	char path[MAXPATHLEN];
++	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
+ 
+ 	if (!sane_cgroup(cgroup)) {
+ 		nih_error("%s: unsafe cgroup", __func__);
+ 		return -1;
+ 	}
+ 
++	// Get p's current cgroup in pcgpath
++	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
++		nih_error("%s: Could not determine the proxy's cgroup for %s",
++				__func__, controller);
++		return -1;
++	}
++
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+ 		return -1;
+ 	}
+ 
++	// Make sure target cgroup is under proxy's
++	int plen = strlen(pcgpath);
++	if (strncmp(pcgpath, path, plen) != 0) {
++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
++			r.pid);
++		return -1;
++	}
++
+ 	/* Check access rights to the cgroup directory */
+ 	if (!may_access(r.pid, r.uid, r.gid, path, O_RDONLY)) {
+ 		nih_error("%s: Pid %d may not access %s\n", __func__, r.pid, path);
+@@ -823,7 +853,7 @@ next:
+ int get_tasks_main(void *parent, const char *controller, const char *cgroup,
+ 			struct ucred p, struct ucred r, int32_t **pids)
+ {
+-	char path[MAXPATHLEN];
++	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
+ 	const char *key = "tasks";
+ 	int alloced_pids = 0, nrpids = 0;
+ 
+@@ -832,12 +862,27 @@ int get_tasks_main(void *parent, const c
+ 		return -1;
+ 	}
+ 
++	// Get p's current cgroup in pcgpath
++	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
++		nih_error("%s: Could not determine the proxy's cgroup for %s",
++				__func__, controller);
++		return -1;
++	}
++
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+ 		return -1;
+ 	}
+ 
++	// Make sure target cgroup is under proxy's
++	int plen = strlen(pcgpath);
++	if (strncmp(pcgpath, path, plen) != 0) {
++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
++			r.pid);
++		return -1;
++	}
++
+ 	/* Check access rights to the cgroup directory */
+ 	if (!may_access(r.pid, r.uid, r.gid, path, O_RDONLY)) {
+ 		nih_error("%s: Pid %d may not access %s\n", __func__, r.pid, path);
+@@ -906,7 +951,7 @@ int collect_tasks(void *parent, const ch
+ 		struct ucred p, struct ucred r, int32_t **pids,
+ 		int *alloced_pids, int *nrpids)
+ {
+-	char path[MAXPATHLEN];
++	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
+ 	nih_local char *rpath = NULL;
+ 
+ 	if (!sane_cgroup(cgroup)) {
+@@ -914,12 +959,27 @@ int collect_tasks(void *parent, const ch
+ 		return -1;
+ 	}
+ 
++	// Get p's current cgroup in pcgpath
++	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
++		nih_error("%s: Could not determine the proxy's cgroup for %s",
++				__func__, controller);
++		return -1;
++	}
++
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+ 		return -2;
+ 	}
+ 
++	// Make sure target cgroup is under proxy's
++	int plen = strlen(pcgpath);
++	if (strncmp(pcgpath, path, plen) != 0) {
++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
++			r.pid);
++		return -1;
++	}
++
+ 	/* Check access rights to the cgroup directory */
+ 	if (!may_access(r.pid, r.uid, r.gid, path, O_RDONLY)) {
+ 		nih_error("%s: Pid %d may not access %s\n", __func__, r.pid, path);
+@@ -982,7 +1042,7 @@ next:
+ int list_children_main(void *parent, const char *controller, const char *cgroup,
+ 			struct ucred p, struct ucred r, char ***output)
+ {
+-	char path[MAXPATHLEN];
++	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
+ 
+ 	*output = NULL;
+ 	if (!sane_cgroup(cgroup)) {
+@@ -990,12 +1050,27 @@ int list_children_main(void *parent, con
+ 		return -1;
+ 	}
+ 
++	// Get p's current cgroup in pcgpath
++	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
++		nih_error("%s: Could not determine the proxy's cgroup for %s",
++				__func__, controller);
++		return -1;
++	}
++
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+ 		return -1;
+ 	}
+ 
++	// Make sure target cgroup is under proxy's
++	int plen = strlen(pcgpath);
++	if (strncmp(pcgpath, path, plen) != 0) {
++		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
++			r.pid);
++		return -1;
++	}
++
+ 	/* Check access rights to the cgroup directory */
+ 	if (!may_access(r.pid, r.uid, r.gid, path, O_RDONLY)) {
+ 		nih_error("%s: Pid %d may not access %s\n", __func__, r.pid, path);
diff -Nru cgmanager-0.33/debian/patches/0004-chown-stop-cgmanager-crash-on-chown-of-bad-file.patch cgmanager-0.33/debian/patches/0004-chown-stop-cgmanager-crash-on-chown-of-bad-file.patch
--- cgmanager-0.33/debian/patches/0004-chown-stop-cgmanager-crash-on-chown-of-bad-file.patch	1969-12-31 18:00:00.000000000 -0600
+++ cgmanager-0.33/debian/patches/0004-chown-stop-cgmanager-crash-on-chown-of-bad-file.patch	2015-01-26 09:16:00.000000000 -0600
@@ -0,0 +1,26 @@
+From a7fddc8078e7a1fc1d8e33516c90d1a37b8bfc8c Mon Sep 17 00:00:00 2001
+From: Serge Hallyn <serge.hallyn@ubuntu.com>
+Date: Mon, 15 Dec 2014 18:34:27 -0600
+Subject: [PATCH 1/4] chown: stop cgmanager crash on chown of bad file
+
+Wrong error function was being used.
+
+Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
+---
+ cgmanager.c | 3 +--
+ 1 file changed, 1 insertion(+), 2 deletions(-)
+
+Index: cgmanager-0.33/cgmanager.c
+===================================================================
+--- cgmanager-0.33.orig/cgmanager.c
++++ cgmanager-0.33/cgmanager.c
+@@ -525,8 +525,7 @@ int chmod_main(const char *controller, c
+ 	}
+ 
+ 	if (file && ( strchr(file, '/') || strchr(file, '\\')) ) {
+-		nih_dbus_error_raise_printf (DBUS_ERROR_INVALID_ARGS,
+-				"invalid file");
++		nih_error("%s: invalid file", __func__);
+ 		return -1;
+ 	}
+ 
diff -Nru cgmanager-0.33/debian/patches/0005-prevent-some-cgmanager-asserts.patch cgmanager-0.33/debian/patches/0005-prevent-some-cgmanager-asserts.patch
--- cgmanager-0.33/debian/patches/0005-prevent-some-cgmanager-asserts.patch	1969-12-31 18:00:00.000000000 -0600
+++ cgmanager-0.33/debian/patches/0005-prevent-some-cgmanager-asserts.patch	2015-01-26 09:16:02.000000000 -0600
@@ -0,0 +1,44 @@
+From c56249afbccfb66f4896c1fc3380e7c1459c1afc Mon Sep 17 00:00:00 2001
+From: Serge Hallyn <serge.hallyn@ubuntu.com>
+Date: Mon, 29 Dec 2014 01:19:56 -0600
+Subject: [PATCH 4/4] prevent some cgmanager asserts
+
+Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
+---
+ cgmanager.c | 9 ++++++---
+ 1 file changed, 6 insertions(+), 3 deletions(-)
+
+Index: cgmanager-0.33/cgmanager.c
+===================================================================
+--- cgmanager-0.33.orig/cgmanager.c
++++ cgmanager-0.33/cgmanager.c
+@@ -899,7 +899,8 @@ int get_tasks_main(void *parent, const c
+ 
+ 	*pids = NULL;
+ 	if (file_read_pids(parent, path, pids, &alloced_pids, &nrpids) < 0) {
+-		nih_free(*pids);
++		if (*pids)
++			nih_free(*pids);
+ 		return -1;
+ 	}
+ 	return nrpids;
+@@ -1007,7 +1008,8 @@ int get_tasks_recursive_main(void *paren
+ 	if (strcmp(controller, "all") != 0 && !strchr(controller, ',')) {
+ 		if (collect_tasks(parent, controller, cgroup, p, r, pids,
+ 				&alloced_pids, &nrpids) < 0) {
+-			nih_free(*pids);
++			if (*pids)
++				nih_free(*pids);
+ 			return -1;
+ 		}
+ 		return nrpids;
+@@ -1028,7 +1030,8 @@ int get_tasks_recursive_main(void *paren
+ 		if (ret == -2)  // permission denied - ignore
+ 			goto next;
+ 		if (ret != 0) {
+-			nih_free(*pids);
++			if (*pids)
++				nih_free(*pids);
+ 			return -1;
+ 		}
+ next:
diff -Nru cgmanager-0.33/debian/patches/0006-fix-subdirectory-check cgmanager-0.33/debian/patches/0006-fix-subdirectory-check
--- cgmanager-0.33/debian/patches/0006-fix-subdirectory-check	1969-12-31 18:00:00.000000000 -0600
+++ cgmanager-0.33/debian/patches/0006-fix-subdirectory-check	2015-02-11 22:26:20.000000000 -0600
@@ -0,0 +1,229 @@
+commit 9c41660679e740eecc4831b4a0043a09197d44dd
+Author: Serge Hallyn <serge.hallyn@ubuntu.com>
+Date:   Wed Feb 11 19:07:38 2015 -0600
+
+    Fix subdirectory check
+    
+    Commit 6a30adc22cc05b5 was not quite complete.  If the task is in
+    /xxx then it may act on /xxx or /xxx/a, but not /xxx2.  This commit
+    fixes the check to disallow /xxx2.  It also commonizes the check,
+    cleaning up the code a bit.
+    
+    Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
+
+Index: cgmanager-0.33/cgmanager.c
+===================================================================
+--- cgmanager-0.33.orig/cgmanager.c
++++ cgmanager-0.33/cgmanager.c
+@@ -557,20 +557,13 @@ next:
+ int get_value_main(void *parent, const char *controller, const char *cgroup,
+ 		const char *key, struct ucred p, struct ucred r, char **value)
+ {
+-	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
++	char path[MAXPATHLEN];
+ 
+ 	if (!sane_cgroup(cgroup)) {
+ 		nih_error("%s: unsafe cgroup", __func__);
+ 		return -1;
+ 	}
+ 
+-	// Get p's current cgroup in pcgpath
+-	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
+-		nih_error("%s: Could not determine the proxy's cgroup for %s",
+-				__func__, controller);
+-		return -1;
+-	}
+-
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+@@ -583,9 +576,7 @@ int get_value_main(void *parent, const c
+ 		return -1;
+ 	}
+ 
+-	// Make sure target cgroup is under proxy's
+-	int plen = strlen(pcgpath);
+-	if (strncmp(pcgpath, path, plen) != 0) {
++	if (!path_is_under_taskcg(p.pid, controller, path)) {
+ 		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
+ 			r.pid);
+ 		return -1;
+@@ -622,29 +613,20 @@ int set_value_main(const char *controlle
+ 		struct ucred r)
+ 
+ {
+-	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
++	char path[MAXPATHLEN];
+ 
+ 	if (!sane_cgroup(cgroup)) {
+ 		nih_error("%s: unsafe cgroup", __func__);
+ 		return -1;
+ 	}
+ 
+-	// Get p's current cgroup in pcgpath
+-	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
+-		nih_error("%s: Could not determine the proxy's cgroup for %s",
+-				__func__, controller);
+-		return -1;
+-	}
+-
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+ 		return -1;
+ 	}
+ 
+-	// Make sure target cgroup is under proxy's
+-	int plen = strlen(pcgpath);
+-	if (strncmp(pcgpath, path, plen) != 0) {
++	if (!path_is_under_taskcg(p.pid, controller, path)) {
+ 		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
+ 			r.pid);
+ 		return -1;
+@@ -852,7 +834,7 @@ next:
+ int get_tasks_main(void *parent, const char *controller, const char *cgroup,
+ 			struct ucred p, struct ucred r, int32_t **pids)
+ {
+-	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
++	char path[MAXPATHLEN];
+ 	const char *key = "tasks";
+ 	int alloced_pids = 0, nrpids = 0;
+ 
+@@ -861,22 +843,13 @@ int get_tasks_main(void *parent, const c
+ 		return -1;
+ 	}
+ 
+-	// Get p's current cgroup in pcgpath
+-	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
+-		nih_error("%s: Could not determine the proxy's cgroup for %s",
+-				__func__, controller);
+-		return -1;
+-	}
+-
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+ 		return -1;
+ 	}
+ 
+-	// Make sure target cgroup is under proxy's
+-	int plen = strlen(pcgpath);
+-	if (strncmp(pcgpath, path, plen) != 0) {
++	if (!path_is_under_taskcg(p.pid, controller, path)) {
+ 		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
+ 			r.pid);
+ 		return -1;
+@@ -951,7 +924,7 @@ int collect_tasks(void *parent, const ch
+ 		struct ucred p, struct ucred r, int32_t **pids,
+ 		int *alloced_pids, int *nrpids)
+ {
+-	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
++	char path[MAXPATHLEN];
+ 	nih_local char *rpath = NULL;
+ 
+ 	if (!sane_cgroup(cgroup)) {
+@@ -959,22 +932,13 @@ int collect_tasks(void *parent, const ch
+ 		return -1;
+ 	}
+ 
+-	// Get p's current cgroup in pcgpath
+-	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
+-		nih_error("%s: Could not determine the proxy's cgroup for %s",
+-				__func__, controller);
+-		return -1;
+-	}
+-
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+ 		return -2;
+ 	}
+ 
+-	// Make sure target cgroup is under proxy's
+-	int plen = strlen(pcgpath);
+-	if (strncmp(pcgpath, path, plen) != 0) {
++	if (!path_is_under_taskcg(p.pid, controller, path)) {
+ 		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
+ 			r.pid);
+ 		return -1;
+@@ -1044,7 +1008,7 @@ next:
+ int list_children_main(void *parent, const char *controller, const char *cgroup,
+ 			struct ucred p, struct ucred r, char ***output)
+ {
+-	char pcgpath[MAXPATHLEN], path[MAXPATHLEN];
++	char path[MAXPATHLEN];
+ 
+ 	*output = NULL;
+ 	if (!sane_cgroup(cgroup)) {
+@@ -1052,22 +1016,13 @@ int list_children_main(void *parent, con
+ 		return -1;
+ 	}
+ 
+-	// Get p's current cgroup in pcgpath
+-	if (!compute_pid_cgroup(p.pid, controller, "", pcgpath, NULL)) {
+-		nih_error("%s: Could not determine the proxy's cgroup for %s",
+-				__func__, controller);
+-		return -1;
+-	}
+-
+ 	if (!compute_pid_cgroup(r.pid, controller, cgroup, path, NULL)) {
+ 		nih_error("%s: Could not determine the requested cgroup (%s:%s)",
+                 __func__, controller, cgroup);
+ 		return -1;
+ 	}
+ 
+-	// Make sure target cgroup is under proxy's
+-	int plen = strlen(pcgpath);
+-	if (strncmp(pcgpath, path, plen) != 0) {
++	if (!path_is_under_taskcg(p.pid, controller, path)) {
+ 		nih_error("%s: target cgroup is not below r (%d)'s", __func__,
+ 			r.pid);
+ 		return -1;
+Index: cgmanager-0.33/fs.c
+===================================================================
+--- cgmanager-0.33.orig/fs.c
++++ cgmanager-0.33/fs.c
+@@ -1707,3 +1707,34 @@ bool was_premounted(const char *controll
+ 		return false;
+ 	return all_mounts[i].premounted;
+ }
++
++/*
++ * Check that (absolute) @path is under @pid's cgroup for @contr
++ */
++bool path_is_under_taskcg(pid_t pid, const char *contr,const char *path)
++{
++	char pcgpath[MAXPATHLEN];
++	size_t plen;
++
++	// Get p's current cgroup in pcgpath
++	if (!compute_pid_cgroup(pid, contr, "", pcgpath, NULL)) {
++		nih_error("%s: Could not determine the proxy's cgroup for %s",
++				__func__, contr);
++		return false;
++	}
++	plen = strlen(pcgpath);
++	// path must start with pcgpath
++	if (strncmp(pcgpath, path, plen) != 0)
++		return false;
++	// If path is equal to pcgpath then that's ok
++	if (plen == strlen(path))
++		return true;
++	/*
++	 * if path is longer than pcpgath, then it must be a subdirectory
++	 * of pcpgpath. I.e. if pcgpath is /xxx then /xxx/a is ok, /xxx2 is
++	 * not.
++	 */
++	if (path[plen] == '/')
++		return true;
++	return false;
++}
+Index: cgmanager-0.33/fs.h
+===================================================================
+--- cgmanager-0.33.orig/fs.h
++++ cgmanager-0.33/fs.h
+@@ -57,3 +57,4 @@ bool was_premounted(const char *controll
+ void do_prune_comounts(char *controllers);
+ void do_list_controllers(void *parent, char ***output);
+ void convert_directory_contents(struct keys_return_type **keys, struct ucred r);
++bool path_is_under_taskcg(pid_t pid, const char *contr,const char *path);
diff -Nru cgmanager-0.33/debian/patches/series cgmanager-0.33/debian/patches/series
--- cgmanager-0.33/debian/patches/series	2014-10-13 18:32:58.000000000 -0500
+++ cgmanager-0.33/debian/patches/series	2015-02-11 22:26:16.000000000 -0600
@@ -1,2 +1,6 @@
 0001-do_move_pid_main-don-t-break-out-of-while-loop-on-er.patch
 0002-check-enabled-column-when-parsing-proc-cgroups.patch
+0003-make-sure-to-check-cgroup-hierarchy.patch
+0004-chown-stop-cgmanager-crash-on-chown-of-bad-file.patch
+0005-prevent-some-cgmanager-asserts.patch
+0006-fix-subdirectory-check



Information forwarded to debian-bugs-dist@lists.debian.org, Debian Release Team <debian-release@lists.debian.org>:
Bug#777649; Package release.debian.org. (Thu, 12 Feb 2015 06:42:14 GMT) (full text, mbox, link).


Acknowledgement sent to Niels Thykier <niels@thykier.net>:
Extra info received and forwarded to list. Copy sent to Debian Release Team <debian-release@lists.debian.org>. (Thu, 12 Feb 2015 06:42:14 GMT) (full text, mbox, link).


Message #37 received at 777649@bugs.debian.org (full text, mbox, reply):

From: Niels Thykier <niels@thykier.net>
To: Serge Hallyn <serge.hallyn@ubuntu.com>, 777649@bugs.debian.org
Subject: Re: Bug#777649: cgmanager security update for jessie
Date: Thu, 12 Feb 2015 07:40:31 +0100
Control: tags -1 confirmed moreinfo

On 2015-02-12 05:32, Serge Hallyn wrote:
> Here is a new debdiff.  (tested in its original upstream version
> in v0.36)   Maybe it would've been easier to squash the two patches,
> but this way it's easier to tell whether the patches match what is
> upstream.
> 
> [...]

Ack, looks better.  :)  Please add the (missing parts of this) patch to
unstable first and then upload the target fixes into testing with
version 0.33-2+deb8u1.

Please remove the moreinfo tag once the above have been done.

Thanks,
~Niels





Added tag(s) confirmed. Request was from Niels Thykier <niels@thykier.net> to 777649-submit@bugs.debian.org. (Thu, 12 Feb 2015 06:42:14 GMT) (full text, mbox, link).


Information forwarded to debian-bugs-dist@lists.debian.org, Debian Release Team <debian-release@lists.debian.org>:
Bug#777649; Package release.debian.org. (Fri, 13 Feb 2015 04:39:05 GMT) (full text, mbox, link).


Acknowledgement sent to Serge Hallyn <serge.hallyn@ubuntu.com>:
Extra info received and forwarded to list. Copy sent to Debian Release Team <debian-release@lists.debian.org>. (Fri, 13 Feb 2015 04:39:05 GMT) (full text, mbox, link).


Message #44 received at 777649@bugs.debian.org (full text, mbox, reply):

From: Serge Hallyn <serge.hallyn@ubuntu.com>
To: Niels Thykier <niels@thykier.net>
Cc: 777649@bugs.debian.org
Subject: Re: Bug#777649: cgmanager security update for jessie
Date: Fri, 13 Feb 2015 04:36:05 +0000
Quoting Niels Thykier (niels@thykier.net):
> Control: tags -1 confirmed moreinfo
> 
> On 2015-02-12 05:32, Serge Hallyn wrote:
> > Here is a new debdiff.  (tested in its original upstream version
> > in v0.36)   Maybe it would've been easier to squash the two patches,
> > but this way it's easier to tell whether the patches match what is
> > upstream.
> > 
> > [...]
> 
> Ack, looks better.  :)  Please add the (missing parts of this) patch to
> unstable first

That is now in sid,

> and then upload the target fixes into testing with
> version 0.33-2+deb8u1.

Sorry, I'm not sure what you mean.  I don't actually have upload rights.
Should I ask someone to sponsor such a package, or just post the debdiff
here?  (It could be the same as the last debdiff I posted, with the version
number changed, or I could squash the two patches as I mentioned before)

> Please remove the moreinfo tag once the above have been done.

thanks,
-serge



Information forwarded to debian-bugs-dist@lists.debian.org, Debian Release Team <debian-release@lists.debian.org>:
Bug#777649; Package release.debian.org. (Sat, 14 Feb 2015 03:45:04 GMT) (full text, mbox, link).


Acknowledgement sent to Michael Gilbert <mgilbert@debian.org>:
Extra info received and forwarded to list. Copy sent to Debian Release Team <debian-release@lists.debian.org>. (Sat, 14 Feb 2015 03:45:04 GMT) (full text, mbox, link).


Message #49 received at 777649@bugs.debian.org (full text, mbox, reply):

From: Michael Gilbert <mgilbert@debian.org>
To: 777649@bugs.debian.org
Subject: Re: Bug#777649: cgmanager security update for jessie
Date: Fri, 13 Feb 2015 22:40:14 -0500
control: retitle -1 unblock: cgmanager/0.33-2+deb8u1

On Thu, Feb 12, 2015 at 11:36 PM, Serge Hallyn wrote:
> Sorry, I'm not sure what you mean.  I don't actually have upload rights.
> Should I ask someone to sponsor such a package, or just post the debdiff
> here?  (It could be the same as the last debdiff I posted, with the version
> number changed, or I could squash the two patches as I mentioned before)

You can post the debdiff here and ask for sponsorship.  CCing -mentors
and your past sponsors may be wise also.

Best wishes,
Mike



Changed Bug title to 'unblock: cgmanager/0.33-2+deb8u1' from 'cgmanager security update for jessie' Request was from Michael Gilbert <mgilbert@debian.org> to 777649-submit@bugs.debian.org. (Sat, 14 Feb 2015 03:45:04 GMT) (full text, mbox, link).


Removed tag(s) moreinfo. Request was from Serge Hallyn <serge.hallyn@ubuntu.com> to control@bugs.debian.org. (Tue, 17 Feb 2015 14:33:14 GMT) (full text, mbox, link).


Reply sent to Niels Thykier <niels@thykier.net>:
You have taken responsibility. (Wed, 18 Feb 2015 19:09:13 GMT) (full text, mbox, link).


Notification sent to Serge Hallyn <serge.hallyn@ubuntu.com>:
Bug acknowledged by developer. (Wed, 18 Feb 2015 19:09:13 GMT) (full text, mbox, link).


Message #58 received at 777649-done@bugs.debian.org (full text, mbox, reply):

From: Niels Thykier <niels@thykier.net>
To: Serge Hallyn <serge.hallyn@ubuntu.com>, 777649-done@bugs.debian.org
Subject: Re: Bug#777649: cgmanager security update for jessie
Date: Wed, 18 Feb 2015 20:07:06 +0100
On 2015-02-13 05:36, Serge Hallyn wrote:
> [...]
> 
>> and then upload the target fixes into testing with
>> version 0.33-2+deb8u1.
> 
> Sorry, I'm not sure what you mean.  I don't actually have upload rights.
> Should I ask someone to sponsor such a package, or just post the debdiff
> here?  (It could be the same as the last debdiff I posted, with the version
> number changed, or I could squash the two patches as I mentioned before)
> 
>> Please remove the moreinfo tag once the above have been done.
> 
> thanks,
> -serge
> 
> 

It got uploaded and I have unblocked the t-p-u version.

Thanks,
~Niels




Message #59 received at 777649-done@bugs.debian.org (full text, mbox, reply):

From: Serge Hallyn <serge.hallyn@ubuntu.com>
To: Niels Thykier <niels@thykier.net>
Cc: 777649-done@bugs.debian.org
Subject: Re: Bug#777649: cgmanager security update for jessie
Date: Wed, 18 Feb 2015 19:34:08 +0000
> It got uploaded and I have unblocked the t-p-u version.

Great, thanks very much.



Bug archived. Request was from Debbugs Internal Request <owner@bugs.debian.org> to internal_control@bugs.debian.org. (Thu, 19 Mar 2015 07:28:47 GMT) (full text, mbox, link).


Send a report that this bug log contains spam.


Debian bug tracking system administrator <owner@bugs.debian.org>. Last modified: Wed Jan 3 23:13:29 2018; Machine Name: beach

Debian Bug tracking system

Debbugs is free software and licensed under the terms of the GNU Public License version 2. The current version can be obtained from https://bugs.debian.org/debbugs-source/.

Copyright © 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson, 2005-2017 Don Armstrong, and many other contributors.