[PATCH koji] added koji-helper setuid program

[Enrico Scholz]

This patch adds a 'koji-helper' setuid program which implements the
following methods:

* koji-helper rmrf <dir>
    removes everything under <dir>, inclusive <dir>.  It does not cross
    filesystem borders

* koji-helper rmtree <dir>
    removes everything under <dir>, but not <dir> itself. It does not cross
    filesystem borders


Methods above are implemented to replace the python 'safe_rmtree()' method
which was never safe, nor will work when 'kojid' is running as non-root.

Signed-off-by: Enrico Scholz <enrico.scholz(a)informatik.tu-chemnitz.de&gt;
---

 Makefile          |   15 ++-
 builder/kojid     |   53 +++--------
 koji.spec         |    3 -
 src/koji-helper.c |  260 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 286 insertions(+), 45 deletions(-)

diff --git a/Makefile b/Makefile
index cd88d4b..2a239fd 100644
--- a/Makefile
+++ b/Makefile
@@ -2,6 +2,8 @@ NAME=koji
 SPECFILE = $(firstword $(wildcard *.spec))
 SUBDIRS = hub builder koji cli docs util www
 
+sbindir = /usr/sbin
+
 ifdef DIST
 DIST_DEFINES := --define "dist $(DIST)"
 endif
@@ -52,11 +54,14 @@ ifndef TAG
 TAG=$(NAME)-$(VERSION)-$(RELEASE)
 endif
 
-_default:
-	@echo "read the makefile"
+all:	src/koji-helper
+
+src/koji-helper:	src/koji-helper.c
+	$(CC) $(CFLAGS) $< -o $@
 
 clean:
 	rm -f *.o *.so *.pyc *~ koji*.bz2 koji*.src.rpm
+	rm -f src/koji-helper
 	rm -rf koji-$(VERSION)
 	for d in $(SUBDIRS); do make -s -C $$d clean; done
 
@@ -100,14 +105,16 @@ force-tag::
 #	@$(MAKE) tag TAG_OPTS="-F $(TAG_OPTS)"
 
 DESTDIR ?= /
-install:
+install:	all
 	@if [ "$(DESTDIR)" = "" ]; then \
 		echo " "; \
 		echo "ERROR: A destdir is required"; \
 		exit 1; \
 	fi
 
-	mkdir -p $(DESTDIR)
+	mkdir -p $(DESTDIR)$(sbindir)
+
+	install -p -m0710 src/koji-helper $(DESTDIR)$(sbindir)
 
 	for d in $(SUBDIRS); do make DESTDIR=`cd $(DESTDIR); pwd` \
 		-C $$d install; [ $$? = 0 ] || exit 1; done
diff --git a/builder/kojid b/builder/kojid
index 6e973fe..5940954 100755
--- a/builder/kojid
+++ b/builder/kojid
@@ -150,35 +150,17 @@ def log_output(path, args, outfile, uploadpath, cwd=None,
logerror=0, append=0,
                     outfd.close()
                 return status[1]
 
-def safe_rmtree(path, unmount=False, strict=True):
+def safe_rmtree(path, strict=True, op='rmrf'):
     logger = logging.getLogger("koji.build")
-    #safe remove: with -xdev the find cmd will not cross filesystems
-    #             (though it will cross bind mounts from the same filesystem)
-    if not os.path.exists(path):
-        logger.debug("No such path: %s" % path)
-        return
-    if unmount:
-        umount_all(path)
-    #first rm -f non-directories
-    logger.debug('Scrubbing files in %s' % path)
-    rv = os.system("find '%s' -xdev \\! -type d -print0 |xargs -0 rm
-f" % path)
-    msg = 'file removal failed (code %r) for %s' % (rv,path)
-    if rv != 0:
-        logger.warn(msg)
-        if strict:
-            raise koji.GenericError, msg
-        else:
-            return rv
-    #them rmdir directories
-    #with -depth, we start at the bottom and work up
-    logger.debug('Scrubbing directories in %s' % path)
-    rv = os.system("find '%s' -xdev -depth -type d -print0 |xargs -0
rmdir" % path)
-    msg = 'dir removal failed (code %r) for %s' % (rv,path)
-    if rv != 0:
-        logger.warn(msg)
-        if strict:
-            raise koji.GenericError, msg
-    return rv
+    rc     = os.spawnvp(os.P_WAIT, '/usr/sbin/koji-helper',
['/usr/sbin/koji-helper', op, path])
+    if rc!=0:
+	msg = "directory removal failed (code %r) for %s" % (rc,path)
+	logger.warn(msg)
+	if strict:
+		raise koji.GenericError, msg
+	else:
+		return rc
+    return rc
 
 def umount_all(topdir):
     "Unmount every mount under topdir"
@@ -635,7 +617,7 @@ class TaskManager(object):
                 if age > 3600*24:
                     #dir untouched for a day
                     self.logger.info("Removing buildroot: %s" % desc)
-                    if topdir and safe_rmtree(topdir, unmount=True, strict=False) != 0:
+                    if topdir and safe_rmtree(topdir, strict=False) != 0:
                         continue
                     #also remove the config
                     try:
@@ -644,15 +626,7 @@ class TaskManager(object):
                         self.logger.warn("%s: can't remove config: %s" %
(desc, e))
                 elif age > 120:
                     if rootdir:
-                        try:
-                            flist = os.listdir(rootdir)
-                        except OSError, e:
-                            self.logger.warn("%s: can't list rootdir: %s" %
(desc, e))
-                            continue
-                        if flist:
-                            self.logger.info("%s: clearing rootdir" % desc)
-                        for fn in flist:
-                            safe_rmtree("%s/%s" % (rootdir,fn), unmount=True,
strict=False)
+                        safe_rmtree(rootdir, strict=False, op='rmtree')
                 else:
                     self.logger.debug("Recent buildroot: %s: %i seconds" %
(desc,age))
         self.logger.debug("Local buildroots: %d" % len(local_br))
@@ -1211,8 +1185,7 @@ class BaseTaskHandler(object):
     def removeWorkdir(self):
         if self.workdir is None:
             return
-        safe_rmtree(self.workdir, unmount=False, strict=True)
-        #os.spawnvp(os.P_WAIT, 'rm', ['rm', '-rf',
self.workdir])
+        os.spawnvp(os.P_WAIT, 'rm', ['rm', '-rf', self.workdir])
 
     def wait(self, subtasks=None, all=False, failany=False):
         """Wait on subtasks
diff --git a/koji.spec b/koji.spec
index 13d3bf0..f14bb6e 100644
--- a/koji.spec
+++ b/koji.spec
@@ -16,7 +16,6 @@ Group: Applications/System
 URL: http://hosted.fedoraproject.org/projects/koji
 Source: %{name}-%{PACKAGE_VERSION}.tar.bz2
 BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
-BuildArch: noarch
 Requires: python-krbV >= 1.0.13
 Requires: rpm-python
 Requires: pyOpenSSL
@@ -89,6 +88,7 @@ koji-web is a web UI to the Koji system.
 %setup -q
 
 %build
+make CFLAGS="$CFLAGS" CC="%__cc" all
 
 %install
 rm -rf $RPM_BUILD_ROOT
@@ -125,6 +125,7 @@ rm -rf $RPM_BUILD_ROOT
 
 %files builder
 %defattr(-,root,root)
+%attr(4710,root,kojibuilder) %_sbindir/koji-helper
 %{_sbindir}/kojid
 %{_initrddir}/kojid
 %config(noreplace) %{_sysconfdir}/sysconfig/kojid
diff --git a/src/koji-helper.c b/src/koji-helper.c
new file mode 100644
index 0000000..a3d0921
--- /dev/null
+++ b/src/koji-helper.c
@@ -0,0 +1,260 @@
+/*	--*- c -*--
+ * Copyright (C) 2007 Enrico Scholz <enrico.scholz(a)informatik.tu-chemnitz.de&gt;
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>;.
+ */
+
+#define _GNU_SOURCE
+
+#ifndef MOCK_ROOT
+#  define MOCK_ROOT	"/var/lib/mock"
+#endif
+
+#include <sys/stat.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <unistd.h>
+#include <dirent.h>
+#include <fcntl.h>
+#include <stdbool.h>
+
+static int __attribute__((__nonnull__(1, 2)))
+safe_chdir(char const *path, struct stat const *exp_st) 
+{
+	struct stat		cur_st;
+	
+	if (strchr(path, '/')) {
+		fprintf(stderr, "safe_chdir(): invalid char in path '%s'\n", path);
+		return -1;
+	}
+
+	if (strcmp(path, "..")==0) {
+		fprintf(stderr, "safe_chdir(): parent dir referred\n");
+		return -1;
+	}
+
+	if (chdir(path) < 0) {
+		fprintf(stderr, "chdir(%s): %s\n", path, strerror(errno));
+		return -1;
+	}
+
+	if (stat(".", &cur_st) < 0) {
+		fprintf(stderr, "stat(%s): %s\n", path, strerror(errno));
+		return -2;
+	}
+
+	if (cur_st.st_dev != exp_st->st_dev ||
+	    cur_st.st_ino != exp_st->st_ino) {
+		fprintf(stderr, "RACE: path '%s' changed before chdir()\n", path);
+		return -2;
+	}
+
+	return 0;
+}
+
+static int
+rmrf_cwd(struct stat *cwd_st)
+{
+	DIR		*cwd = opendir(".");
+	int		rc   = -1;
+
+	if (!cwd) {
+		perror("opendir()");
+		return -1;
+	}
+
+	for (;;) {
+		struct dirent	*ent = readdir(cwd);
+		struct stat	st;
+
+		if (!ent)
+			break;
+
+		if (ent->d_name[0] == '.' &&
+		    (ent->d_name[1] == '\0'||
+		     (ent->d_name[1] == '.' && ent->d_name[2] == '\0')))
+			continue;	/* skip '.' and '..' */
+
+		if (lstat(ent->d_name, &st) < 0) {
+			fprintf(stderr, "rmrf_cwd: lstat(%s): %s\n",
+				ent->d_name, strerror(errno));
+			continue;
+		}
+
+		if (cwd_st && cwd_st->st_dev != st.st_dev)
+			continue;	/* do not cross devices */
+		else if (S_ISDIR(st.st_mode)) {
+			switch (safe_chdir(ent->d_name, &st)) {
+			case -1: continue;
+			case -2: break;
+			default: rmrf_cwd(&st); break;
+			}
+
+			if (fchdir(dirfd(cwd)) < 0) {
+				perror("rmrf_cwd: fchdir()");
+				goto err;
+			}
+
+			if (rmdir(ent->d_name) < 0) {
+				fprintf(stderr, "rmrf_cwd: rmdir(%s): %s\n",
+					ent->d_name, strerror(errno));
+				continue;
+			}
+		} else if (unlink(ent->d_name) < 0) {
+			fprintf(stderr, "rmrf_cwd: unlink(%s): %s\n",
+				ent->d_name, strerror(errno));
+			continue;
+		}
+	}
+
+	rc = 0;
+err:
+	closedir(cwd);
+	return rc;
+}
+
+static int
+safe_chdir_subpath(char const *path_c, size_t path_len)
+{
+	char		path[path_len+1];
+	char		*ptr = path;
+	int		rc = 0;
+
+	if (path_len == 0)
+		return 0;
+
+	strncpy(path, path_c, path_len);
+	path[path_len] = '\0';
+
+	while (ptr) {
+		char		*new_ptr = strsep(&ptr, "/");
+		struct stat	st;
+
+		if (*new_ptr == '\0')
+			continue;	/* skip empty path components
+					 * (e.g. double /) */
+		
+		if (lstat(new_ptr, &st) < 0) {
+			fprintf(stderr, "stat(%s): %s\n",
+				new_ptr, strerror(errno));
+			rc = -1;
+		} else if (!S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode)) {
+			fprintf(stderr, "safe_chdir_subpath(): invalid mode of '%s':
%04x\n",
+				new_ptr, st.st_mode);
+			rc = -1;
+		} else
+			rc = safe_chdir(new_ptr, &st);
+
+		if (rc < 0)
+			break;
+	}
+
+	return rc;
+}
+
+/* Usage: rmrf <dir> */
+static int
+do_rmrf(int argc, char *argv[], bool remove_parent_dir)
+{
+	char const	*dir;
+	size_t		dir_len;
+	char const	*last_path;
+	int		parent_fd;
+	
+	if (argc != 2) {
+		fprintf(stderr, "wrong number of parameters for 'rmrf'
operation\n");
+		return EXIT_FAILURE;
+	}
+
+	dir       = argv[1];
+
+	/* strip leading MOCK_ROOT; it's a little bit hacky but required to
+	 * keep backward compatibility */
+	if (strncmp(dir, MOCK_ROOT, sizeof(MOCK_ROOT)-1) == 0)
+		dir += sizeof(MOCK_ROOT)-1;
+
+	while (*dir == '/')
+		++dir;			/* strip leading '/' */
+	
+	dir_len   = strlen(dir);
+	while (dir_len>0 && dir[dir_len-1] == '/')
+		--dir_len;		/* strip trailing '/' */
+
+	last_path = dir + dir_len;
+	while (last_path > dir && last_path[-1] != '/')
+		--last_path;
+
+	if (dir_len == 0) {
+		fprintf(stderr, "do_rmrf(): empty path\n");
+		return EXIT_FAILURE;
+	}
+	if (dir_len > 255) {
+		fprintf(stderr, "pathname too long\n");
+		return EXIT_FAILURE;
+	}
+
+	
+	/* real work begins here... */
+
+	if (chdir(MOCK_ROOT) < 0) {
+		perror("chdir(<MOCK_ROOT>)");
+		return EXIT_FAILURE;
+	}
+
+	if (last_path > dir &&		/* else, it would be a noop */
+	    safe_chdir_subpath(dir, last_path - dir) < 0)
+		return EXIT_FAILURE;
+
+	parent_fd = open(".", O_RDONLY|O_DIRECTORY);
+	if (parent_fd < 0) {
+		perror("open(<MOCK_ROOT>)");
+		return EXIT_FAILURE;
+	}
+
+	if (safe_chdir_subpath(last_path, dir+dir_len - last_path + 1) < 0)
+		return EXIT_FAILURE;
+
+	/* we are now *in* the given path */
+	if (rmrf_cwd(NULL) < 0)
+		return EXIT_FAILURE;
+
+	if (fchdir(parent_fd) < 0) {
+		perror("fchdir(<parent>)");
+		return EXIT_FAILURE;
+	}
+
+	if (remove_parent_dir &&
+	    rmdir(last_path) < 0)
+		return EXIT_FAILURE;
+
+	return EXIT_SUCCESS;
+}
+
+int main(int argc, char *argv[])
+{
+	if (argc < 2) {
+		fprintf(stderr, "not enough parameters\n");
+		return EXIT_FAILURE;
+	}
+
+	if (strcmp(argv[1], "rmrf") == 0)
+		return do_rmrf(argc-1, argv+1, true);
+	else if (strcmp(argv[1], "rmtree") == 0)
+		return do_rmrf(argc-1, argv+1, false);
+	else
+		fprintf(stderr, "unknown argument\n");
+
+	return EXIT_FAILURE;
+}

[Mike McLean]

Enrico Scholz wrote:
>  This patch adds a 'koji-helper' setuid program which
> implements the
>  following methods: 
>  Methods above are implemented to replace the python
> 'safe_rmtree()' method
>  which was never safe, nor will work when 'kojid' is running as non-root. 
It all depends on what you mean by safe, I suppose. The safe_rmtree 
function protects against the destruction of stray mounts underneath the 
buildroot. This is a serious risk, though perhaps some folks will not 
appreciate how serious until they are debugging a buildroot, add a 
mount, and accidentally delete its contents when the buildroot is cleaned.

Your patch seems to remove this protection.

I designed kojid to run as root, and I don't see that as a problem. Many 
daemons run as root and kojid has more need of it than most. I do not 
like the old mock security model and I consider it flawed. I have no 
desire to emulate it in koji.

[Enrico Scholz]

Mike McLean <mikem(a)redhat.com&gt; writes:

> > This patch adds a 'koji-helper' setuid program which
> implements the
> > following methods:
>
> > Methods above are implemented to replace the python 'safe_rmtree()' method
> > which was never safe, nor will work when 'kojid' is running as non-root.
>
>  It all depends on what you mean by safe 
Definitively not the racy

| find ... | xargs rm

...


>  The safe_rmtree function protects against the destruction of stray
>  mounts underneath the buildroot. This is a serious risk, though perhaps
>  some folks will not appreciate how serious until they are debugging a
>  buildroot, add a mount, and accidentally delete its contents when the
>  buildroot is cleaned.
>
>  Your patch seems to remove this protection. 
no; it does not cross filesystem borders.


>  I designed kojid to run as root, and I don't see that as a
> problem. Many
>  daemons run as root and kojid has more need of it than most. 
What are these needs? 'kojid' runs perfectly as non-root.


>  I do not like the old mock security model and I consider it flawed.
> I
>  have no desire to emulate it in koji. 
Yes; mock's helper binary is full of races and broken constraints :(



Enrico