Add flag to allow non-canonical symbolic links

This will later be exposed by qfile-unpacker, and is a safer alternative
to disabling all symbolic link checking.

qrexec-lib/libqubes-rpc-filecopy.h

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

qrexec-lib/pure.h

@@ -256,6 +256,12 @@ 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),
+    /// Allow non-canonical symbolic links.  Paths are still checked
+    /// to be canonial, as they are assumed to come from a filesystem
+    /// traversal, which will always produce canonical paths.
+    QUBES_PURE_ALLOW_NON_CANONICAL_SYMLINKS = (1 << 1),
+    /// Allow all paths to be non-canonical, including symlinks.
+    QUBES_PURE_ALLOW_NON_CANONICAL_PATHS = (1 << 3),
 };
 
 #ifdef __cplusplus

qrexec-lib/unicode.c

@@ -162,19 +162,27 @@ static ssize_t validate_path(const uint8_t *const untrusted_name,
     // as this would require SIZE_MAX bytes in the path and leave
     // no space for the executable code.
     ssize_t non_dotdot_components = 0;
+    bool const allow_non_canonical = (flags & QUBES_PURE_ALLOW_NON_CANONICAL_PATHS);
     if (untrusted_name[0] == '\0')
-        return -ENOLINK; // empty path
+        return allow_non_canonical ? 0 : -ENOLINK; // empty path
+    if (untrusted_name[0] == '/')
+        return -ENOLINK; // absolute path
     for (size_t i = 0; untrusted_name[i]; i++) {
         if (i == 0 || untrusted_name[i - 1] == '/') {
+            // Start of a path component
             switch (untrusted_name[i]) {
             case '\0': // impossible, loop exit condition & if statement before
                        // loop check this
                 COMPILETIME_UNREACHABLE;
-            case '/': // repeated or initial slash
+            case '/': // repeated slash
+                if (allow_non_canonical)
+                    continue;
                 return -EILSEQ;
             case '.':
                 if (untrusted_name[i + 1] == '\0' || untrusted_name[i + 1] == '/') {
                     // Path component is "."
+                    if (allow_non_canonical)
+                        continue;
                     return -EILSEQ;
                 }
                 if ((untrusted_name[i + 1] == '.') &&
@@ -213,7 +221,10 @@ static ssize_t validate_path(const uint8_t *const untrusted_name,
 
 static bool flag_check(const uint32_t flags)
 {
-    return (flags & ~(__typeof__(flags))(QUBES_PURE_ALLOW_UNSAFE_CHARACTERS)) == 0;
+    int const allowed = (QUBES_PURE_ALLOW_UNSAFE_CHARACTERS |
+                         QUBES_PURE_ALLOW_NON_CANONICAL_SYMLINKS |
+                         QUBES_PURE_ALLOW_NON_CANONICAL_PATHS);
+    return (flags & ~(__typeof__(flags))allowed) == 0;
 }
 
 QUBES_PURE_PUBLIC int
@@ -224,7 +235,8 @@ qubes_pure_validate_file_name_v2(const uint8_t *const untrusted_filename,
         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
+    // Always return -EILSEQ, since -ENOLINK only makes sense for symlinks
+    return res > 0 ? 0 : -EILSEQ;
 }
 
 QUBES_PURE_PUBLIC bool
@@ -243,6 +255,8 @@ qubes_pure_validate_symbolic_link_v2(const uint8_t *untrusted_name,
     ssize_t depth = validate_path(untrusted_name, 0, flags);
     if (depth < 0)
         return -EILSEQ; // -ENOLINK is only for symlinks
+    if ((flags & QUBES_PURE_ALLOW_NON_CANONICAL_SYMLINKS) != 0)
+        flags |= QUBES_PURE_ALLOW_NON_CANONICAL_PATHS;
     // 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.

qrexec-lib/unpack.c

@@ -343,7 +343,11 @@ 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);
+    // Never set QUBES_PURE_ALLOW_NON_CANONICAL_PATHS -- paths from qfile-agent
+    // will always be canonical.
+    uint32_t validate_flags = ((uint32_t)flags >> 2) &
+        (QUBES_PURE_ALLOW_UNSAFE_CHARACTERS |
+         QUBES_PURE_ALLOW_NON_CANONICAL_SYMLINKS);
     if (!read_all_with_crc(0, untrusted_namebuf, namelen))
         do_exit(LEGAL_EOF, NULL); // hopefully remote has produced error message
     untrusted_namebuf[namelen] = 0;

qrexec-lib/validator-test.c

@@ -67,6 +67,27 @@ static void character_must_be_forbidden(UChar32 c)
         abort();
     }
 }
+struct symlink_test {
+    const char *const path, *const target, *const file;
+    int const line, flags;
+    bool const allowed;
+};
+
+// returns 0 on success and nonzero on failure
+static int symlink_test(const struct symlink_test symlink_checks[], size_t size)
+{
+    bool failed = false;
+    for (size_t i = 0; i < size; ++i) {
+        const struct symlink_test *p = symlink_checks + i;
+        if ((qubes_pure_validate_symbolic_link_v2((const unsigned char *)p->path,
+                        (const unsigned char *)p->target,
+                        p->flags) == 0) != p->allowed) {
+            failed = true;
+            fprintf(stderr, "%s:%d:Test failure\n", p->file, p->line);
+        }
+    }
+    return (int)failed;
+}
 
 int main(int argc, char **argv)
 {
@@ -119,11 +140,7 @@ int main(int argc, char **argv)
                (always_forbidden || bad_unicode ? -EILSEQ : 0));
     }
 
-    struct p {
-        const char *const path, *const target, *const file;
-        int const line, flags;
-        bool const allowed;
-    } symlink_checks[] = {
+    const struct symlink_test checks[] = {
 #define TEST(path_, target_, flags_, allowed_) \
         { .path = (path_)                      \
         , .target = (target_)                  \
@@ -153,18 +170,41 @@ int main(int argc, char **argv)
         TEST("a/b/c", "..", 0, true),
         // Symlinks may end in "/".
         TEST("a/b/c", "a/", 0, true),
-        // Invalid paths are rejected.
+        // Invalid paths are rejected...
         TEST("..", "a/", 0, false),
+        // QUBES_PURE_ALLOW_NON_CANONICAL_PATHS allows non-canonical symlinks...
+        TEST("a/b", "b//c", QUBES_PURE_ALLOW_NON_CANONICAL_PATHS, true),
+        // ...and non-canonical paths.
+        TEST("a//b", "b//c", QUBES_PURE_ALLOW_NON_CANONICAL_PATHS, true),
     };
-    bool failed = false;
-    for (size_t i = 0; i < sizeof(symlink_checks)/sizeof(symlink_checks[0]); ++i) {
-        const struct p *p = symlink_checks + i;
-        if ((qubes_pure_validate_symbolic_link_v2((const unsigned char *)p->path,
-                                                  (const unsigned char *)p->target,
-                                                  p->flags) == 0) != p->allowed) {
-            failed = true;
-            fprintf(stderr, "%s:%d:Test failure\n", p->file, p->line);
-        }
+    int failed = 0;
+#define SYMLINK_TEST(a) symlink_test(a, sizeof(a)/sizeof(a[0]))
+    failed |= SYMLINK_TEST(checks);
+
+    for (int i = 0; i <= QUBES_PURE_ALLOW_NON_CANONICAL_SYMLINKS;
+            i += QUBES_PURE_ALLOW_NON_CANONICAL_SYMLINKS) {
+        const struct symlink_test canonical_checks[] = {
+            // QUBES_PURE_ALLOW_NON_CANONICAL_SYMLINKS allows non-canonical symlinks...
+            TEST("a/b", "b//c", i, i != 0),
+            TEST("a/b", "b/./c", i, i != 0),
+            TEST("a/b", "./c", i, i != 0),
+            TEST("a/b", "././c", i, i != 0),
+            TEST("a/b", "././", i, i != 0),
+            TEST("a/b", "c/./", i, i != 0),
+            TEST("b/c", "", i, i != 0),
+            TEST("b/c", ".", i, i != 0),
+            // ...but not non-canonical paths...
+            TEST("a//b", "b", i, false),
+            TEST("a/./b", "b", i, false),
+            TEST("./b/c", "b", i, false),
+            // ...or unsafe symlinks
+            TEST("a/b", "..", i, false),
+            TEST("a/b", "../b", i, false),
+            TEST("a/b", "/c", i, false),
+            TEST("b", "c", i, false),
+            TEST("b", ".", i, false),
+        };
+        failed |= SYMLINK_TEST(canonical_checks);
     }
     assert(!failed);