Allow unsafe characters if invoked as qubes.UnsafeFileCopy

debian/control

@@ -5,6 +5,7 @@ Maintainer: unman <[email protected]>
 Build-Depends:
     debhelper,
     libpam0g-dev,
+    libqubes-pure-dev,
     libqubes-rpc-filecopy-dev (>= 3.1.3),
     libvchan-xen-dev,
     python3,

debian/qubes-core-agent.install

@@ -154,6 +154,7 @@ usr/lib/qubes/init/setup-rwdev.sh
 usr/lib/qubes/prepare-suspend
 usr/lib/qubes/qfile-agent
 usr/lib/qubes/qfile-unpacker
+usr/lib/qubes/qubes-fs-tree-check
 usr/lib/qubes/qopen-in-vm
 usr/lib/qubes/qubes-sync-clock
 usr/lib/qubes/qrun-in-vm

qubes-rpc/Makefile

@@ -13,20 +13,30 @@ endif
 CPPFLAGS := -I.
 CFLAGS := $(DEBUG_FLAGS) -O2 -Wall -Wextra -Werror -fPIC -pie $(CFLAGS)
 LDFLAGS := $(DEBUG_FLAGS) -pie $(LDFLAGS)
-LDLIBS := -lqubes-rpc-filecopy
+LDLIBS := -lqubes-rpc-filecopy -lqubes-pure
 
 .PHONY: all clean install
 
-all:	vm-file-editor qopen-in-vm qfile-agent qfile-unpacker tar2qfile
+all:	vm-file-editor qopen-in-vm qfile-agent qfile-unpacker tar2qfile qubes-fs-tree-check bin-qfile-unpacker
 
+# Ensure that these programs can find their shared libraries,
+# even when installed in e.g. a TemplateBasedVM to somewhere other
+# than /usr.
+vm-file-editor qopen-in-vm qfile-agent qfile-unpacker tar2qfile qubes-fs-tree-check: LDFLAGS += '-Wl,-rpath,$$ORIGIN/../../$$LIB'
+# This is installed in /usr/bin, not /usr/lib/qubes, so it needs a different rpath.
+bin-qfile-unpacker: LDFLAGS += '-Wl,-rpath,$$ORIGIN/../$$LIB'
+bin-qfile-unpacker: qfile-unpacker.o gui-fatal.o
+	$(CC) $(LDFLAGS) -o $@ $^ $(LDLIBS)
+qubes-fs-tree-check: LDLIBS := -lqubes-pure
+qubes-fs-tree-check: qubes-fs-tree-check.o
 vm-file-editor: vm-file-editor.o
 qopen-in-vm: qopen-in-vm.o gui-fatal.o
 qfile-agent: qfile-agent.o gui-fatal.o
 qfile-unpacker: qfile-unpacker.o gui-fatal.o
 tar2qfile: tar2qfile.o gui-fatal.o
 
 clean:
-	-$(RM) -- qopen-in-vm qfile-agent qfile-unpacker tar2qfile vm-file-editor *.o
+	-$(RM) -- qopen-in-vm qfile-agent qfile-unpacker tar2qfile vm-file-editor qubes-fs-tree-check bin-qfile-unpacker *.o
 
 install:
 	install -d $(DESTDIR)$(BINDIR)
@@ -46,13 +56,14 @@ install:
 	install -t $(DESTDIR)$(QUBESLIBDIR) \
 		prepare-suspend resize-rootfs \
 		qfile-agent qopen-in-vm qrun-in-vm qubes-sync-clock \
-		tar2qfile vm-file-editor xdg-icon qvm-template-repo-query
+		tar2qfile vm-file-editor xdg-icon qvm-template-repo-query \
+		qubes-fs-tree-check
 	# Install qfile-unpacker as SUID, because it will fail to receive
 	# files from other vm.
 	install -t $(DESTDIR)$(QUBESLIBDIR) -m 4755 qfile-unpacker
 	# This version isn't confined by SELinux, so it supports other
 	# home directories.
-	install -t $(DESTDIR)$(BINDIR) -m 4755 qfile-unpacker
+	install -m 4755 bin-qfile-unpacker $(DESTDIR)$(BINDIR)/qfile-unpacker
 	install -d $(DESTDIR)$(QUBESRPCCMDDIR)
 	install -t $(DESTDIR)$(QUBESRPCCMDDIR) \
 		qubes.Filecopy qubes.OpenInVM qubes.VMShell \

qubes-rpc/qfile-unpacker.c

@@ -11,16 +11,20 @@
 #include <string.h>
 #include <unistd.h>
 #include <sys/fsuid.h>
-#include <gui-fatal.h>
 #include <errno.h>
+#include <assert.h>
+#include <limits.h>
+#include <getopt.h>
+#include <err.h>
+
+#include <gui-fatal.h>
 #include <libqubes-rpc-filecopy.h>
 
 #define INCOMING_DIR_NAME "QubesIncoming"
 
-char *prepare_creds_return_dir(int uid)
+static char *prepare_creds_return_dir(uid_t uid, uid_t myuid)
 {
     const struct passwd *pwd;
-    uid_t myuid = getuid();
     if (myuid != 0 && myuid != (uid_t)uid)
         gui_fatal("Refusing to change to UID other than the caller's UID");
     pwd = getpwuid(uid);
@@ -30,37 +34,156 @@ char *prepare_creds_return_dir(int uid)
     }
     setenv("HOME", pwd->pw_dir, 1);
     setenv("USER", pwd->pw_name, 1);
+    if (pwd->pw_uid != uid)
+        gui_fatal("getpwuid() returned entry for wrong user");
     if (setgid(pwd->pw_gid) < 0)
         gui_fatal("Error setting group permissions");
     if (initgroups(pwd->pw_name, pwd->pw_gid) < 0)
         gui_fatal("Error initializing groups");
-    if (setfsuid(pwd->pw_uid) < 0)
+    setfsuid(pwd->pw_uid);
+    if ((uid_t)setfsuid(-1) != uid)
         gui_fatal("Error setting filesystem level permissions");
     return pwd->pw_dir;
 }
 
+static void set_wait_for_space_str(const char *str)
+{
+    if (strcmp(str, "0") == 0) {
+        set_wait_for_space(0);
+        return;
+    }
+    if (str[0] >= '1' && str[0] <= '9') {
+        errno = 0;
+        char *endp;
+        long res = strtol(str, &endp, 10);
+        if (errno == 0 && *endp == '\0' && res > 0 && res <= INT_MAX) {
+            set_wait_for_space((int)res);
+            return;
+        }
+    }
+    errx(1, "Space amount %s is invalid or exceeds %d bytes", str, INT_MAX);
+}
+
+enum {
+    opt_allow_unsafe_characters = 256,
+    opt_allow_unsafe_symlinks,
+    opt_no_allow_unsafe_characters,
+    opt_no_allow_unsafe_symlinks,
+};
+
+const struct option opts[] = {
+    { "no-allow-unsafe-characters", no_argument, NULL, opt_no_allow_unsafe_characters },
+    { "allow-unsafe-characters", no_argument, NULL, opt_allow_unsafe_characters },
+    { "no-allow-unsafe-symlinks", no_argument, NULL, opt_no_allow_unsafe_symlinks },
+    { "allow-unsafe-symlinks", no_argument, NULL, opt_allow_unsafe_symlinks },
+    { "verbose", no_argument, NULL, 'v' },
+    { "wait-for-space", required_argument, NULL, 'w' },
+    { NULL, 0, NULL, 0 },
+};
+
+uid_t parse_uid(const char *user)
+{
+    if (strcmp(user, "0") == 0)
+        return 0;
+    errno = 0;
+    char *end = NULL;
+    unsigned long long u = strtoull(user, &end, 10);
+    uid_t uid = (uid_t)u;
+    if (user[0] < '1' || user[0] > '9' ||
+            errno != 0 || *end != '\0' || uid != u)
+        gui_fatal("Invalid user ID argument");
+    return uid;
+}
+
 int main(int argc, char ** argv)
 {
     char *home_dir;
     char *incoming_dir_root;
     char *incoming_dir;
-    int uid, ret;
+    uid_t caller_uid = getuid(), uid = caller_uid;
     pid_t pid;
     const char *remote_domain;
     char *procdir_path;
     int procfs_fd;
-    int i;
-
-    if (argc >= 3) {
-        errno = 0;
-        uid = strtol(argv[1], NULL, 10);
-        if (errno)
-            gui_fatal("Invalid user ID argument");
-        home_dir = prepare_creds_return_dir(uid);
+    int i, ret;
+    int flags = COPY_ALLOW_SYMLINKS | COPY_ALLOW_DIRECTORIES;
+    if (argc < 1)
+        errx(EXIT_FAILURE, "NULL argv[0] passed to execve()");
+    if (argc >= 3 && argv[1][0] >= '0' && argv[1][0] <= '9') {
+        // Legacy case: parse options by hand
+        uid = parse_uid(argv[1]);
         incoming_dir = argv[2];
+
+        for (i = 3; i < argc; i++) {
+            if (strcmp(argv[i], "-v") == 0)
+                set_verbose(1);
+            else if (strcmp(argv[i], "-w") == 0) {
+                const char *next = argv[i + 1];
+                if (next != NULL && next[0] != '-') {
+                    set_wait_for_space_str(next);
+                    i++;
+                } else {
+                    set_wait_for_space(1);
+                }
+            } else {
+                gui_fatal("Invalid option %s", argv[i]);
+            }
+        }
     } else {
-        uid = getuid();
-        home_dir = prepare_creds_return_dir(uid);
+        incoming_dir = NULL;
+        // Modern case: use getopt(3)
+        for (;;) {
+            if (optind < 1 || optind > argc) {
+                // FIXME: is this actually impossible?
+                assert(!"invalid optind() value?");
+                abort();
+            }
+            int longindex = -1;
+            const char *const last = argv[optind];
+            int opt = getopt_long(argc, argv, "+vw:", opts, &longindex);
+            if (opt == -1) {
+                if (argc <= optind)
+                    break;
+                if (argc - optind > 2)
+                    errx(1, "Wrong number of non-option arguments (expected no more than 2, got %d)",
+                         argc - optind);
+                if (argv[optind][0] != '\0')
+                    uid = parse_uid(argv[optind]);
+                // might be NULL
+                incoming_dir = argv[optind + 1];
+                break;
+            }
+            if (opt == '?' || opt == ':')
+                return EXIT_FAILURE;
+            if (longindex != -1) {
+                const char *expected = opts[longindex].name;
+                if (strncmp(expected, last + 2, strlen(expected)) != 0)
+                    errx(1, "Option %s must be passed as --%s", last, expected);
+            }
+            switch (opt) {
+            case 'v':
+                set_verbose(1);
+                break;
+            case opt_allow_unsafe_characters:
+                flags |= COPY_ALLOW_UNSAFE_CHARACTERS;
+                break;
+            case opt_allow_unsafe_symlinks:
+                flags |= COPY_ALLOW_UNSAFE_SYMLINKS;
+                break;
+            case opt_no_allow_unsafe_characters:
+                flags &= ~COPY_ALLOW_UNSAFE_CHARACTERS;
+                break;
+            case opt_no_allow_unsafe_symlinks:
+                flags &= ~COPY_ALLOW_UNSAFE_SYMLINKS;
+                break;
+            case 'w':
+                set_wait_for_space_str(optarg);
+                break;
+            }
+        }
+    }
+    home_dir = prepare_creds_return_dir(uid, caller_uid);
+    if (incoming_dir == NULL) {
         remote_domain = getenv("QREXEC_REMOTE_DOMAIN");
         if (!remote_domain) {
             gui_fatal("Cannot get remote domain name");
@@ -69,25 +192,14 @@ int main(int argc, char ** argv)
         if (asprintf(&incoming_dir_root, "%s/%s", home_dir, INCOMING_DIR_NAME) < 0) {
             gui_fatal("Error allocating memory");
         }
+        // mkdir() failing is harmless.  If the directory doesn't exist after
+        // the call, the subsequent chdir() will fail.
         mkdir(incoming_dir_root, 0700);
         if (asprintf(&incoming_dir, "%s/%s", incoming_dir_root, remote_domain) < 0)
             gui_fatal("Error allocating memory");
         mkdir(incoming_dir, 0700);
     }
 
-    for (i = 3; i < argc; i++) {
-        if (strcmp(argv[i], "-v") == 0)
-            set_verbose(1);
-        else if (strcmp(argv[i], "-w") == 0)
-            if (i+1 < argc && argv[i+1][0] != '-') {
-                set_wait_for_space(atoi(argv[i+1]));
-                i++;
-            } else
-                set_wait_for_space(1);
-        else
-            gui_fatal("Invalid option %s", argv[i]);
-    }
-
     if (chdir(incoming_dir))
         gui_fatal("Error chdir to %s", incoming_dir);
 
@@ -103,7 +215,7 @@ int main(int argc, char ** argv)
             if (asprintf(&procdir_path, "/proc/%d/fd", getpid()) < 0) {
                 gui_fatal("Error allocating memory");
             }
-            procfs_fd = open(procdir_path, O_DIRECTORY | O_RDONLY);
+            procfs_fd = open(procdir_path, O_DIRECTORY | O_RDONLY | O_NOCTTY | O_CLOEXEC);
             if (procfs_fd < 0)
                 perror("Failed to open /proc");
             else
@@ -117,7 +229,7 @@ int main(int argc, char ** argv)
                 perror("setuid");
                 exit(1);
             }
-            return do_unpack();
+            return do_unpack_ext(flags);
     }
     if (waitpid(pid, &ret, 0) < 0) {
         gui_fatal("Failed to wait for child process");