Allow disabling character set filtering

This allows copying paths with names that would otherwise be forbidden.
This requires adding new APIs, so take the opportunity to provide more
useful information about why a path was rejected.

To ensure that internal invariants are not violated, this uses a new
COMPILETIME_UNREACHABLE macro to validate that a statement can be proven
unreachable by the compiler.  This is superior to abort() because it is
checked at compile-time by the optimizer: if the compiler cannot prove
that the code is unreachable, the build will fail.  This technique is
also used by BUILD_BUG_ON() in the Linux kernel.  Since compilers do not
promise to always be able to prove a piece of code unreachable, static
checking is disabled by default.  It can be enabled by including
CHECK_UNREACHABLE=1 in the build environment.

This is part of QubesOS/qubes-issues#8332 (less restricted qfile-copy),
but that issue also requires changes to qfile-unpacker (part of
qubes-core-agent-linux) to fix.  The code is both backwards and forwards
compatible: Old qfile-unpacker versions will work fine with the new
library, and new qfile-unpacker versions will work fine with the old
library.  However, qubes.UnsafeFileCopy will behave like qubes.Filecopy
unless the library has been updated.

Initially, I decided to unconditionally disallow ASCII control
characters in filenames, even if other character set filtering is
disabled.  This is because they are very useful for exploits and not
very useful for other purposes.  However, they can still arise in
practice for completely legitimate reasons.  These include copying and
pasting into a file name in a GUI file manager and deliberately creating
strangely-named files for test purposes.  Therefore, disabling filtering
now disables _all_ character set filtering.  Restrictions to prevent
directory traversal are still enforced, though, because violations of
these are much more likely to be exploitable.  While symlinks that point
outside of the directory being copied might arise legitimately, they
will not be meaningful after being copied and _do_ allow an attacker to
cause mischief.  For instance, if a symlink points to
/home/user/.bashrc, "cp a b" in a directory copied by qvm-copy could
overwrite ~/.bashrc with attacker-controlled data.  The checks on
symlink paths prevent this.

(cherry picked from commit aa2466c)

qrexec-lib/Makefile

@@ -15,6 +15,9 @@ libqubes-rpc-filecopy.so.$(SO_VER): $(objs) ./$(pure_lib).$(pure_sover)
 validator-test: validator-test.o ./$(pure_lib).$(pure_sover)
 	libs=$$(pkg-config --libs icu-uc) && $(CC) '-Wl,-rpath,$$ORIGIN' $(LDFLAGS) -o $@ $^ $$libs
 $(pure_objs): CFLAGS += -fvisibility=hidden -DQUBES_PURE_IMPLEMENTATION
+ifeq ($(CHECK_UNREACHABLE),1)
+$(pure_objs): CFLAGS += -DCHECK_UNREACHABLE
+endif
 validator-test: CFLAGS += -UNDEBUG
 check: validator-test
 	LD_LIBRARY_PATH=. ./validator-test

qrexec-lib/libqubes-rpc-filecopy.h

@@ -67,6 +67,7 @@ enum copy_flags {
     COPY_DEFAULT = 0,
     COPY_ALLOW_SYMLINKS = (1 << 0),
     COPY_ALLOW_DIRECTORIES = (1 << 1),
+    COPY_ALLOW_UNSAFE_CHARACTERS = (1 << 2),
 };
 
 /* feedback handling */

qrexec-lib/pure.h

@@ -93,10 +93,30 @@ struct QubesMutableSlice {
  * filenames are also rejected, including invalid UTF-8 sequences and
  * all control characters.  The input must be NUL-terminated.
  *
- * Returns true on success and false on failure.
+ * \param[in] untrusted_path The path to be checked.
+ * \return \true on success and \false on failure.
+ *
+ * This is equivalent to passing zero for the flags parameter
+ * to qubes_pure_validate_file_name_v2() and checking that
+ * the result is zero.
  */
 QUBES_PURE_PUBLIC bool
-qubes_pure_validate_file_name(const uint8_t *untrusted_filename);
+qubes_pure_validate_file_name(const uint8_t *untrusted_path);
+
+/**
+ * Validate that a string is a valid path and will not result in
+ * directory traversal if used as such.  If flags does not include
+ * \ref QUBES_PURE_ALLOW_UNSAFE_CHARACTERS, characters that are unsafe for
+ * filenames are also rejected, including invalid UTF-8 sequences and
+ * all control characters.  The input must be NUL-terminated.
+ *
+ * \param[in] untrusted_path The path to be checked.
+ * \param flags 0 or QUBES_PURE_ALLOW_UNSAFE_CHARACTERS.
+ * \return 0 on success, negative errno value on failure.
+ */
+QUBES_PURE_PUBLIC int
+qubes_pure_validate_file_name_v2(const uint8_t *const untrusted_path,
+                                 const uint32_t flags);
 
 /**
  * Validate that `untrusted_name` is a valid symbolic link name
@@ -107,11 +127,34 @@ qubes_pure_validate_file_name(const uint8_t *untrusted_filename);
  * NUL-terminated.
  *
  * Returns true on success and false on failure.
+ *
+ * This is equivalent to passing zero for the flags parameter
+ * to qubes_pure_validate_symbolic_link_v2() and checking that
+ * the result is zero.
  */
 QUBES_PURE_PUBLIC bool
 qubes_pure_validate_symbolic_link(const uint8_t *untrusted_name,
                                   const uint8_t *untrusted_target);
 
+/**
+ * Validate that `untrusted_path` is a valid symbolic link name
+ * and that creating a symbolic link with that name and target
+ * `untrusted_target` is also safe.  If flags does not include
+ * \ref QUBES_PURE_ALLOW_UNSAFE_CHARACTERS, characters that are unsafe for
+ * filenames are also rejected, including invalid UTF-8 sequences and
+ * all control characters.  The input must be NUL-terminated.
+ *
+ * \param[in] untrusted_path The path to be checked.
+ * \param[in] untrusted_target The proposed target for the symbolic link.
+ * \param flags 0 or QUBES_PURE_ALLOW_UNSAFE_CHARACTERS.
+ * \return 0 on success, negative errno value on failure.
+ */
+QUBES_PURE_PUBLIC int
+qubes_pure_validate_symbolic_link_v2(const uint8_t *untrusted_path,
+                                     const uint8_t *untrusted_target,
+                                     uint32_t flags);
+
+
 /**
  * Validate that `code_point` is safe to display.  To be considered safe to
  * display, a code point must be valid and not be a control character.
@@ -210,6 +253,11 @@ enum QubeNameValidationError {
 QUBES_PURE_PUBLIC enum QubeNameValidationError
 qubes_pure_is_valid_qube_name(const struct QubesSlice untrusted_str);
 
+enum QubesFilenameValidationFlags {
+    /// Disable Unicode charset restrictions and UTF-8 validity checks.
+    QUBES_PURE_ALLOW_UNSAFE_CHARACTERS = (1 << 0),
+};
+
 #ifdef __cplusplus
 }
 #endif

qrexec-lib/unicode.c

@@ -5,6 +5,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <inttypes.h>
+#include <errno.h>
 #include <assert.h>
 
 QUBES_PURE_PUBLIC bool
@@ -101,8 +102,32 @@ static int validate_utf8_char(const uint8_t *untrusted_c) {
     return qubes_pure_code_point_safe_for_display(code_point) ? total_size : 0;
 }
 
-// This is one of the trickiest, security-critical functions in the whole
-// repository (opendir_safe() in unpack.c is the other).  It is critical
+// Statically assert that a statement is not reachable.
+//
+// At runtime, this is just abort(), but it comes with a neat trick:
+// if optimizations are on, CHECK_UNREACHABLE is defined, and the compiler
+// claims to be GNU-compatible, the compiler must prove that this is
+// unreachable.  Otherwise, it is a compile-time error.
+//
+// To enable static checking of this macro, pass CHECK_UNREACHABLE=1 to the
+// makefile or include it in the environment.
+#if defined __GNUC__ && defined __OPTIMIZE__ && defined CHECK_UNREACHABLE
+#define COMPILETIME_UNREACHABLE do {    \
+    extern void not_reachable(void)     \
+        __attribute__((                 \
+            error("Compiler could not prove that this statement is not reachable"), \
+            noreturn));                 \
+    not_reachable();                    \
+} while (0)
+#else
+#define COMPILETIME_UNREACHABLE do {    \
+    assert(0);                          \
+    abort();                            \
+} while (0)
+#endif
+
+// This is one of the trickiest, most security-critical functions in the
+// whole repository (opendir_safe() in unpack.c is the other).  It is critical
 // for preventing directory traversal attacks.  The code does use a chroot()
 // and a bind mount, but the bind mount is not always effective if mount
 // namespaces are in use, and the chroot can be bypassed (QSB-015).
@@ -116,37 +141,48 @@ static int validate_utf8_char(const uint8_t *untrusted_c) {
 // Algorithm:
 //
 // At the start of the loop and after '/', the code checks for '/' and '.'.
-// '/', "./", or ".\0" indicate a non-canonical path: the code skips past them
-// if allow_non_canonical is set, and fails otherwise.  "../" and "..\0" are
-// ".." components: the code checks that the limit on non-".." components has
-// not been exceeded, fails if it has, and otherwise decrements the limit.
+// '/', "./", or ".\0" indicate a non-canonical path.  These are currently
+// rejected, but they could safely be accepted in the future without allowing
+// directory traversal attacks.  "../" and "..\0" are ".." components: the code
+// checks that the limit on non-".." components has not been exceeded, fails if
+// it has, and otherwise decrements the limit.  This ensures that a directory
+// tree cannot contain symlinks that point outside of the tree itself.
 // Anything else is a normal path component: the limit on ".." components
 // is set to zero, and the number of non-".." components is incremented.
 //
 // The return value is the number of non-".." components on
-// success, or -1 on failure.
-static ssize_t validate_path(const uint8_t *const untrusted_name, size_t allowed_leading_dotdot)
+// success, or a negative errno value on failure.  The return value might be
+// zero.
+static ssize_t validate_path(const uint8_t *const untrusted_name,
+                             size_t allowed_leading_dotdot,
+                             const uint32_t flags)
 {
     // We assume that there are not SSIZE_MAX path components.
     // This cannot happen on hardware using a flat address space,
     // as this would require SIZE_MAX bytes in the path and leave
     // no space for the executable code.
     ssize_t non_dotdot_components = 0;
     size_t i = 0;
+    if (untrusted_name[0] == '\0')
+        return -ENOLINK; // empty path
     do {
         if (i == 0 || untrusted_name[i - 1] == '/') {
             switch (untrusted_name[i]) {
+            case '\0': // impossible, loop exit condition & if statement before
+                       // loop check this
+                COMPILETIME_UNREACHABLE;
             case '/': // repeated or initial slash
-            case '\0': // empty string
-                return -1;
+                return -EILSEQ;
             case '.':
-                if (untrusted_name[i + 1] == '\0' || untrusted_name[i + 1] == '/')
-                    return -1;
+                if (untrusted_name[i + 1] == '\0' || untrusted_name[i + 1] == '/') {
+                    // Path component is "."
+                    return -EILSEQ;
+                }
                 if ((untrusted_name[i + 1] == '.') &&
                     (untrusted_name[i + 2] == '\0' || untrusted_name[i + 2] == '/')) {
                     /* Check if the limit on leading ".." components has been exceeded */
                     if (allowed_leading_dotdot < 1)
-                        return -1;
+                        return -ENOLINK;
                     allowed_leading_dotdot--;
                     i += 2; // advance past ".."
                     continue;
@@ -158,44 +194,77 @@ static ssize_t validate_path(const uint8_t *const untrusted_name, size_t allowed
                 break;
             }
         }
-        if (untrusted_name[i] >= 0x20 && untrusted_name[i] <= 0x7E) {
+        if (untrusted_name[i] == 0) {
+            // If this is violated, the subsequent i++ will be out of bounds
+            COMPILETIME_UNREACHABLE;
+        } else if ((0x20 <= untrusted_name[i] && untrusted_name[i] <= 0x7E) ||
+                   (flags & QUBES_PURE_ALLOW_UNSAFE_CHARACTERS) != 0) {
             i++;
         } else {
             int utf8_ret = validate_utf8_char((const unsigned char *)(untrusted_name + i));
             if (utf8_ret > 0) {
                 i += utf8_ret;
             } else {
-                return -1;
+                return -EILSEQ;
             }
         }
     } while (untrusted_name[i]);
     return non_dotdot_components;
 }
 
-QUBES_PURE_PUBLIC bool
-qubes_pure_validate_file_name(const uint8_t *untrusted_filename)
+static bool flag_check(const uint32_t flags)
 {
-    return validate_path(untrusted_filename, 0) > 0;
+    return (flags & ~(__typeof__(flags))(QUBES_PURE_ALLOW_UNSAFE_CHARACTERS)) == 0;
+}
+
+QUBES_PURE_PUBLIC int
+qubes_pure_validate_file_name_v2(const uint8_t *const untrusted_filename,
+                                 const uint32_t flags)
+{
+    if (!flag_check(flags))
+        return -EINVAL;
+    // We require at least one non-".." component in the path.
+    ssize_t res = validate_path(untrusted_filename, 0, flags);
+    return res > 0 ? 0 : -EILSEQ; // -ENOLINK is only for symlinks
 }
 
 QUBES_PURE_PUBLIC bool
-qubes_pure_validate_symbolic_link(const uint8_t *untrusted_name,
-                                  const uint8_t *untrusted_target)
+qubes_pure_validate_file_name(const uint8_t *const untrusted_filename)
 {
-    ssize_t depth = validate_path(untrusted_name, 0);
+    return qubes_pure_validate_file_name_v2(untrusted_filename, 0) == 0;
+}
+
+QUBES_PURE_PUBLIC int
+qubes_pure_validate_symbolic_link_v2(const uint8_t *untrusted_name,
+                                     const uint8_t *untrusted_target,
+                                     uint32_t flags)
+{
+    if (!flag_check(flags))
+        return -EINVAL;
+    ssize_t depth = validate_path(untrusted_name, 0, flags);
+    if (depth < 0)
+        return -EILSEQ; // -ENOLINK is only for symlinks
     // Symlink paths must have at least 2 components: "a/b" is okay
     // but "a" is not.  This ensures that the toplevel "a" entry
     // is not a symbolic link.
     if (depth < 2)
-        return false;
+        return -ENOLINK;
     // Symlinks must have at least 2 more path components in the name
     // than the number of leading ".." path elements in the target.
     // "a/b" can point to "c" (which resolves to "a/c") but not "../c"
     // (which resolves to "c").  Similarly and "a/b/c" can point to "../d"
     // (which resolves to "a/d") but not "../../d" (which resolves to "d").
     // This ensures that ~/QubesIncoming/QUBENAME/a/b cannot point outside
     // of ~/QubesIncoming/QUBENAME/a.
-    return validate_path(untrusted_target, (size_t)(depth - 2)) >= 0;
+    ssize_t res = validate_path(untrusted_target, (size_t)(depth - 2), flags);
+    return res < 0 ? res : 0;
+}
+
+QUBES_PURE_PUBLIC bool
+qubes_pure_validate_symbolic_link(const uint8_t *untrusted_name,
+                                  const uint8_t *untrusted_target)
+{
+    return qubes_pure_validate_symbolic_link_v2(untrusted_name, untrusted_target, 0) == 0;
 }
 
 QUBES_PURE_PUBLIC bool

qrexec-lib/unpack.c

@@ -197,15 +197,17 @@ static int opendir_safe(int dirfd, char *path, const char **last_segment)
 }
 
 static void process_one_file_reg(struct file_header *untrusted_hdr,
-                                 const char *untrusted_name)
+                                 const char *untrusted_name,
+                                 uint32_t flags)
 {
     int ret;
     int fdout = -1, safe_dirfd;
     const char *last_segment;
     char *path_dup;
 
-    if (!qubes_pure_validate_file_name((const uint8_t *)untrusted_name))
-        do_exit(EILSEQ, untrusted_name); /* FIXME: better error message */
+    ret = qubes_pure_validate_file_name_v2((const uint8_t *)untrusted_name, flags);
+    if (ret != 0)
+        do_exit(-ret, untrusted_name); /* FIXME: better error message */
     if ((path_dup = strdup(untrusted_name)) == NULL)
         do_exit(ENOMEM, untrusted_name);
     safe_dirfd = opendir_safe(AT_FDCWD, path_dup, &last_segment);
@@ -261,13 +263,15 @@ static void process_one_file_reg(struct file_header *untrusted_hdr,
 
 
 static void process_one_file_dir(struct file_header *untrusted_hdr,
-                                 const char *untrusted_name)
+                                 const char *untrusted_name,
+                                 uint32_t flags)
 {
     int safe_dirfd;
     const char *last_segment;
     char *path_dup;
-    if (!qubes_pure_validate_file_name((const uint8_t *)untrusted_name))
-        do_exit(EILSEQ, untrusted_name); /* FIXME: better error message */
+    int rc = qubes_pure_validate_file_name_v2((const uint8_t *)untrusted_name, flags);
+    if (rc != 0)
+        do_exit(rc, untrusted_name); /* FIXME: better error message */
     if ((path_dup = strdup(untrusted_name)) == NULL)
         do_exit(ENOMEM, untrusted_name);
     safe_dirfd = opendir_safe(AT_FDCWD, path_dup, &last_segment);
@@ -292,7 +296,8 @@ static void process_one_file_dir(struct file_header *untrusted_hdr,
 }
 
 static void process_one_file_link(struct file_header *untrusted_hdr,
-                                  const char *untrusted_name)
+                                  const char *untrusted_name,
+                                  uint32_t flags)
 {
     char untrusted_content[MAX_PATH_LENGTH];
     const char *last_segment;
@@ -314,9 +319,11 @@ static void process_one_file_link(struct file_header *untrusted_hdr,
      * Ensure that no immediate subdirectory of ~/QubesIncoming/VMNAME
      * may have symlinks that point out of it.
      */
-    if (!qubes_pure_validate_symbolic_link((const uint8_t *)untrusted_name,
-                                           (const uint8_t *)untrusted_content))
-        do_exit(EILSEQ, untrusted_content);
+    int rc = qubes_pure_validate_symbolic_link_v2((const uint8_t *)untrusted_name,
+                                                  (const uint8_t *)untrusted_content,
+                                                  flags);
+    if (rc != 0)
+        do_exit(-rc, untrusted_content);
 
     if ((path_dup = strdup(untrusted_name)) == NULL)
         do_exit(ENOMEM, untrusted_name);
@@ -336,15 +343,16 @@ static void process_one_file(struct file_header *untrusted_hdr, int flags)
     if (untrusted_hdr->namelen > MAX_PATH_LENGTH - 1)
         do_exit(ENAMETOOLONG, NULL); /* filename too long so not received at all */
     namelen = untrusted_hdr->namelen; /* sanitized above */
+    uint32_t validate_flags = ((uint32_t)flags >> 2) & (QUBES_PURE_ALLOW_UNSAFE_CHARACTERS);
     if (!read_all_with_crc(0, untrusted_namebuf, namelen))
         do_exit(LEGAL_EOF, NULL); // hopefully remote has produced error message
     untrusted_namebuf[namelen] = 0;
     if (S_ISREG(untrusted_hdr->mode))
-        process_one_file_reg(untrusted_hdr, untrusted_namebuf);
+        process_one_file_reg(untrusted_hdr, untrusted_namebuf, validate_flags);
     else if (S_ISLNK(untrusted_hdr->mode) && (flags & COPY_ALLOW_SYMLINKS))
-        process_one_file_link(untrusted_hdr, untrusted_namebuf);
+        process_one_file_link(untrusted_hdr, untrusted_namebuf, validate_flags);
     else if (S_ISDIR(untrusted_hdr->mode) && (flags & COPY_ALLOW_DIRECTORIES))
-        process_one_file_dir(untrusted_hdr, untrusted_namebuf);
+        process_one_file_dir(untrusted_hdr, untrusted_namebuf, validate_flags);
     else
         do_exit(EINVAL, untrusted_namebuf);
     if (verbose && !S_ISDIR(untrusted_hdr->mode))