From f0f23af3c60fd6a80b3dfae50d0ad66d3ff5a937 Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Thu, 25 Apr 2024 15:48:59 -0400
Subject: [PATCH 01/15] Document algorithm used to sanitize paths

It is highly critical and is not obvious.
---
 qrexec-lib/unicode.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/qrexec-lib/unicode.c b/qrexec-lib/unicode.c
index 4cd42edb..cada9483 100644
--- a/qrexec-lib/unicode.c
+++ b/qrexec-lib/unicode.c
@@ -101,6 +101,30 @@ 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
+// 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).
+//
+// Preconditions:
+//
+// - untrusted_name is NUL-terminated.
+// - allowed_leading_dotdot is the maximum number of leading "../" sequences
+//   allowed.  Might be 0.
+//
+// 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.
+// 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 0 on failure.
 static size_t validate_path(const uint8_t *const untrusted_name, size_t allowed_leading_dotdot)
 {
     size_t non_dotdot_components = 0, i = 0;
@@ -155,13 +179,17 @@ qubes_pure_validate_symbolic_link(const uint8_t *untrusted_name,
 {
     size_t depth = validate_path(untrusted_name, 0);
     // Symlink paths must have at least 2 components: "a/b" is okay
-    // but "a" is not
+    // but "a" is not.  This ensures that the toplevel "a" entry
+    // is not a symbolic link.
     if (depth < 2)
         return false;
     // Symlinks must have at least 2 more path components in the name
     // than the number of leading ".." path elements in the target.
-    // "a/b" cannot point to "../c", and "a/b/c" can point to "../d"
-    // but not "../../d".
+    // "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, depth - 2) > 0;
 }
 

From 34c92b51fbd37345cee3d3a9ddb7cad43d51fb8d Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Thu, 25 Apr 2024 15:53:09 -0400
Subject: [PATCH 02/15] Allow symlinks ending in ".."

They were rejected without a good reason.  They pose no danger not
already posed by symlinks of the form "../a".
---
 qrexec-lib/unicode.c        | 23 ++++++++++++++---------
 qrexec-lib/validator-test.c |  2 ++
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/qrexec-lib/unicode.c b/qrexec-lib/unicode.c
index cada9483..5f39668b 100644
--- a/qrexec-lib/unicode.c
+++ b/qrexec-lib/unicode.c
@@ -124,24 +124,29 @@ static int validate_utf8_char(const uint8_t *untrusted_c) {
 // is set to zero, and the number of non-".." components is incremented.
 //
 // The return value is the number of non-".." components on
-// success, or 0 on failure.
-static size_t validate_path(const uint8_t *const untrusted_name, size_t allowed_leading_dotdot)
+// success, or -1 on failure.
+static ssize_t validate_path(const uint8_t *const untrusted_name, size_t allowed_leading_dotdot)
 {
-    size_t non_dotdot_components = 0, i = 0;
+    // 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;
     do {
         if (i == 0 || untrusted_name[i - 1] == '/') {
             switch (untrusted_name[i]) {
             case '/': // repeated or initial slash
             case '\0': // trailing slash or empty string
-                return 0;
+                return -1;
             case '.':
                 if (untrusted_name[i + 1] == '\0' || untrusted_name[i + 1] == '/')
-                    return 0;
+                    return -1;
                 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 0;
+                        return -1;
                     allowed_leading_dotdot--;
                     i += 2; // advance past ".."
                     continue;
@@ -160,7 +165,7 @@ static size_t validate_path(const uint8_t *const untrusted_name, size_t allowed_
             if (utf8_ret > 0) {
                 i += utf8_ret;
             } else {
-                return 0;
+                return -1;
             }
         }
     } while (untrusted_name[i]);
@@ -177,7 +182,7 @@ QUBES_PURE_PUBLIC bool
 qubes_pure_validate_symbolic_link(const uint8_t *untrusted_name,
                                   const uint8_t *untrusted_target)
 {
-    size_t depth = validate_path(untrusted_name, 0);
+    ssize_t depth = validate_path(untrusted_name, 0);
     // 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.
@@ -190,7 +195,7 @@ qubes_pure_validate_symbolic_link(const uint8_t *untrusted_name,
     // (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, depth - 2) > 0;
+    return validate_path(untrusted_target, (size_t)(depth - 2)) >= 0;
 }
 
 QUBES_PURE_PUBLIC bool
diff --git a/qrexec-lib/validator-test.c b/qrexec-lib/validator-test.c
index 0adf55b9..372fb2d9 100644
--- a/qrexec-lib/validator-test.c
+++ b/qrexec-lib/validator-test.c
@@ -217,4 +217,6 @@ int main(int argc, char **argv)
     assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"../a"));
     // Absolute symlinks are rejected
     assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"/a"));
+    // Symlinks may end in "..".
+    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)".."));
 }

From 9f78bc465b370b8973f5ad19c181c3e007bc48f1 Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Thu, 25 Apr 2024 16:11:56 -0400
Subject: [PATCH 03/15] Test that symbolic links can end in '/'

This is obscure, but it is harmless.  It simply means that the target of
the symlink must be a directory (or symlink to a directory), otherwise
opening the symlink will fail with ENOTDIR.  Allowing non-symlink paths
to end in '/' is also harmless: opendir_safe() will set the last path
component to the empty string, and attempting to create a file, symlink,
or directory with the empty string as a name will fail with ENOENT.
---
 qrexec-lib/unicode.c        | 2 +-
 qrexec-lib/validator-test.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/qrexec-lib/unicode.c b/qrexec-lib/unicode.c
index 5f39668b..fc4c56c0 100644
--- a/qrexec-lib/unicode.c
+++ b/qrexec-lib/unicode.c
@@ -137,7 +137,7 @@ static ssize_t validate_path(const uint8_t *const untrusted_name, size_t allowed
         if (i == 0 || untrusted_name[i - 1] == '/') {
             switch (untrusted_name[i]) {
             case '/': // repeated or initial slash
-            case '\0': // trailing slash or empty string
+            case '\0': // empty string
                 return -1;
             case '.':
                 if (untrusted_name[i + 1] == '\0' || untrusted_name[i + 1] == '/')
diff --git a/qrexec-lib/validator-test.c b/qrexec-lib/validator-test.c
index 372fb2d9..e85319f9 100644
--- a/qrexec-lib/validator-test.c
+++ b/qrexec-lib/validator-test.c
@@ -219,4 +219,6 @@ int main(int argc, char **argv)
     assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"/a"));
     // Symlinks may end in "..".
     assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)".."));
+    // Symlinks may end in "/".
+    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"a/"));
 }

From a656698185d4654037387f28eb1be6c87ae97bf0 Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Thu, 25 Apr 2024 16:20:06 -0400
Subject: [PATCH 04/15] Move path validation tests before charset tests

The charset validation tests do exhaustive loops over all possible
codepoints.  This is very useful for ensuring correct behavior on all
inputs, but is slow enough (several seconds) to be annoying during
development.  Moving the path validation tests (which are very fast)
before the charset validation tests allows developers to conclude that
they have passed much sooner, since if they fail the code will crash
quickly.
---
 qrexec-lib/validator-test.c | 95 +++++++++++++++++++------------------
 1 file changed, 48 insertions(+), 47 deletions(-)

diff --git a/qrexec-lib/validator-test.c b/qrexec-lib/validator-test.c
index e85319f9..614ce083 100644
--- a/qrexec-lib/validator-test.c
+++ b/qrexec-lib/validator-test.c
@@ -71,6 +71,54 @@ int main(int argc, char **argv)
     (void)argc;
     (void)argv;
     assert(qubes_pure_validate_file_name((uint8_t *)u8"simple_safe_filename.txt"));
+
+    // Directory traversal checks
+    assert(!qubes_pure_validate_file_name((uint8_t *)".."));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"../.."));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"a/.."));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"a/../b"));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"/"));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"//"));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"///"));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"/a"));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"//a"));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"///a"));
+
+    // No repeated slashes
+    assert(!qubes_pure_validate_file_name((uint8_t *)"a//b"));
+
+    // No "." as a path component
+    assert(!qubes_pure_validate_file_name((uint8_t *)"."));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"a/."));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"./a"));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"a/./a"));
+
+    // No ".." as a path component
+    assert(!qubes_pure_validate_file_name((uint8_t *)".."));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"a/.."));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"../a"));
+    assert(!qubes_pure_validate_file_name((uint8_t *)"a/../a"));
+
+    // Looks like "." or ".." but is not
+    assert(qubes_pure_validate_file_name((const uint8_t *)".a"));
+    assert(qubes_pure_validate_file_name((const uint8_t *)"..a"));
+
+    // Symbolic links
+    // Top level cannot be symlink
+    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a", (const uint8_t *)"b"));
+    // Symbolic links cannot escape
+    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b", (const uint8_t *)"../a"));
+    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b", (const uint8_t *)"../a/b/c"));
+    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"../../a"));
+    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b", (const uint8_t *)"a"));
+    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"../a"));
+    // Absolute symlinks are rejected
+    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"/a"));
+    // Symlinks may end in "..".
+    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)".."));
+    // Symlinks may end in "/".
+    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"a/"));
+
     // Greek letters are safe
     assert(qubes_pure_validate_file_name((uint8_t *)u8"\u03b2.txt"));
     assert(qubes_pure_validate_file_name((uint8_t *)u8"\u03b1.txt"));
@@ -174,51 +222,4 @@ int main(int argc, char **argv)
             assert(j < 0x10FFFFE);
         }
     }
-
-    // Directory traversal checks
-    assert(!qubes_pure_validate_file_name((uint8_t *)".."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"../.."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a/.."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a/../b"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"/"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"//"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"///"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"/a"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"//a"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"///a"));
-
-    // No repeated slashes
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a//b"));
-
-    // No "." as a path component
-    assert(!qubes_pure_validate_file_name((uint8_t *)"."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a/."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"./a"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a/./a"));
-
-    // No ".." as a path component
-    assert(!qubes_pure_validate_file_name((uint8_t *)".."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a/.."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"../a"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a/../a"));
-
-    // Looks like "." or ".." but is not
-    assert(qubes_pure_validate_file_name((const uint8_t *)".a"));
-    assert(qubes_pure_validate_file_name((const uint8_t *)"..a"));
-
-    // Symbolic links
-    // Top level cannot be symlink
-    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a", (const uint8_t *)"b"));
-    // Symbolic links cannot escape
-    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b", (const uint8_t *)"../a"));
-    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b", (const uint8_t *)"../a/b/c"));
-    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"../../a"));
-    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b", (const uint8_t *)"a"));
-    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"../a"));
-    // Absolute symlinks are rejected
-    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"/a"));
-    // Symlinks may end in "..".
-    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)".."));
-    // Symlinks may end in "/".
-    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"a/"));
 }

From cb5c376bf91dd52263188a9d69bdb44221f66899 Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Thu, 25 Apr 2024 16:28:36 -0400
Subject: [PATCH 05/15] Document the pretty-printer in the code generator

No functional change.
---
 qrexec-lib/unicode-generator.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qrexec-lib/unicode-generator.c b/qrexec-lib/unicode-generator.c
index 9c96372c..c20120fe 100644
--- a/qrexec-lib/unicode-generator.c
+++ b/qrexec-lib/unicode-generator.c
@@ -283,6 +283,8 @@ static bool is_permitted_code_point(uint32_t const code_point)
     }
 }
 
+// Generate the table of allowed codepoints,
+// as a bunch of cases in a switch statement.
 static void print_code_point_list(FILE *out)
 {
     bool last_allowed = false;

From 0070e5500c07edadc2fc5b2b4766836eee3a3a27 Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Thu, 25 Apr 2024 21:06:13 -0400
Subject: [PATCH 06/15] Avoid casting away const when not needed

Silences a clang warning.

No functional change intended.
---
 qrexec-lib/unpack.c         | 10 +++++-----
 qrexec-lib/validator-test.c | 40 ++++++++++++++++++-------------------
 2 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/qrexec-lib/unpack.c b/qrexec-lib/unpack.c
index b3c3f456..3aa363c9 100644
--- a/qrexec-lib/unpack.c
+++ b/qrexec-lib/unpack.c
@@ -204,7 +204,7 @@ void process_one_file_reg(struct file_header *untrusted_hdr,
     const char *last_segment;
     char *path_dup;
 
-    if (!qubes_pure_validate_file_name((uint8_t *)untrusted_name))
+    if (!qubes_pure_validate_file_name((const uint8_t *)untrusted_name))
         do_exit(EILSEQ, untrusted_name); /* FIXME: better error message */
     if ((path_dup = strdup(untrusted_name)) == NULL)
         do_exit(ENOMEM, untrusted_name);
@@ -266,7 +266,7 @@ void process_one_file_dir(struct file_header *untrusted_hdr,
     int safe_dirfd;
     const char *last_segment;
     char *path_dup;
-    if (!qubes_pure_validate_file_name((uint8_t *)untrusted_name))
+    if (!qubes_pure_validate_file_name((const uint8_t *)untrusted_name))
         do_exit(EILSEQ, untrusted_name); /* FIXME: better error message */
     if ((path_dup = strdup(untrusted_name)) == NULL)
         do_exit(ENOMEM, untrusted_name);
@@ -298,7 +298,7 @@ void process_one_file_link(struct file_header *untrusted_hdr,
     const char *last_segment;
     char *path_dup;
     unsigned int filelen;
-    if (!qubes_pure_validate_file_name((uint8_t *)untrusted_name))
+    if (!qubes_pure_validate_file_name((const uint8_t *)untrusted_name))
         do_exit(EILSEQ, untrusted_name); /* FIXME: better error message */
     int safe_dirfd;
     if (untrusted_hdr->filelen > MAX_PATH_LENGTH - 1)
@@ -315,8 +315,8 @@ 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((uint8_t *)untrusted_name,
-                                           (uint8_t *)untrusted_content))
+    if (!qubes_pure_validate_symbolic_link((const uint8_t *)untrusted_name,
+                                           (const uint8_t *)untrusted_content))
         do_exit(EILSEQ, untrusted_content);
 
     if ((path_dup = strdup(untrusted_name)) == NULL)
diff --git a/qrexec-lib/validator-test.c b/qrexec-lib/validator-test.c
index 614ce083..f170bf27 100644
--- a/qrexec-lib/validator-test.c
+++ b/qrexec-lib/validator-test.c
@@ -70,34 +70,34 @@ int main(int argc, char **argv)
 {
     (void)argc;
     (void)argv;
-    assert(qubes_pure_validate_file_name((uint8_t *)u8"simple_safe_filename.txt"));
+    assert(qubes_pure_validate_file_name((const uint8_t *)u8"simple_safe_filename.txt"));
 
     // Directory traversal checks
-    assert(!qubes_pure_validate_file_name((uint8_t *)".."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"../.."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a/.."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a/../b"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"/"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"//"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"///"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"/a"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"//a"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"///a"));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)".."));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"../.."));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"a/.."));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"a/../b"));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"/"));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"//"));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"///"));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"/a"));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"//a"));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"///a"));
 
     // No repeated slashes
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a//b"));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"a//b"));
 
     // No "." as a path component
-    assert(!qubes_pure_validate_file_name((uint8_t *)"."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a/."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"./a"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a/./a"));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"."));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"a/."));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"./a"));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"a/./a"));
 
     // No ".." as a path component
-    assert(!qubes_pure_validate_file_name((uint8_t *)".."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a/.."));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"../a"));
-    assert(!qubes_pure_validate_file_name((uint8_t *)"a/../a"));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)".."));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"a/.."));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"../a"));
+    assert(!qubes_pure_validate_file_name((const uint8_t *)"a/../a"));
 
     // Looks like "." or ".." but is not
     assert(qubes_pure_validate_file_name((const uint8_t *)".a"));

From 188b608d850ba00e276bfe0f7a40683b666d4755 Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Wed, 8 May 2024 17:45:24 -0400
Subject: [PATCH 07/15] Fix -Wmissing-declarations and -Wmissing-prototypes
 warnings

This is a theoretical ABI change, but hopefully no third-party app is
linking against undocumented functions not part of any header file.
Also remove an unused function.
---
 qrexec-lib/Makefile |  2 +-
 qrexec-lib/crc32.c  | 40 ----------------------------------------
 qrexec-lib/ioall.c  |  3 ++-
 qrexec-lib/unpack.c | 20 ++++++++++----------
 4 files changed, 13 insertions(+), 52 deletions(-)

diff --git a/qrexec-lib/Makefile b/qrexec-lib/Makefile
index 3c918220..2b7eec4c 100644
--- a/qrexec-lib/Makefile
+++ b/qrexec-lib/Makefile
@@ -1,5 +1,5 @@
 CC=gcc
-CFLAGS := $(CFLAGS) -I. -g3 -O2 -Wall -Wextra -Werror -pie -fPIC
+CFLAGS := $(CFLAGS) -I. -g3 -O2 -Wall -Wextra -Werror -pie -fPIC -Wmissing-declarations -Wmissing-prototypes
 SO_VER=2
 LDFLAGS+=-Wl,--no-undefined,--as-needed,-Bsymbolic -L .
 .PHONY: all clean install check
diff --git a/qrexec-lib/crc32.c b/qrexec-lib/crc32.c
index 88b06277..8fb8c2af 100644
--- a/qrexec-lib/crc32.c
+++ b/qrexec-lib/crc32.c
@@ -29,46 +29,6 @@
 unsigned long Crc32_ComputeBuf( unsigned long inCrc32, const void *buf,
                                        size_t bufLen );
 
-/*----------------------------------------------------------------------------*\
- *  NAME:
- *     Crc32_ComputeFile() - compute CRC-32 value for a file
- *  DESCRIPTION:
- *     Computes the CRC-32 value for an opened file.
- *  ARGUMENTS:
- *     file - file pointer
- *     outCrc32 - (out) result CRC-32 value
- *  RETURNS:
- *     err - 0 on success or -1 on error
- *  ERRORS:
- *     - file errors
-\*----------------------------------------------------------------------------*/
-
-int Crc32_ComputeFile( FILE *file, unsigned long *outCrc32 )
-{
-#   define CRC_BUFFER_SIZE  8192
-    unsigned char buf[CRC_BUFFER_SIZE];
-    size_t bufLen;
-
-    /** accumulate crc32 from file **/
-    *outCrc32 = 0;
-    while (1) {
-        bufLen = fread( buf, 1, CRC_BUFFER_SIZE, file );
-        if (bufLen == 0) {
-            if (ferror(file)) {
-                fprintf( stderr, "error reading file\n" );
-                goto ERR_EXIT;
-            }
-            break;
-        }
-        *outCrc32 = Crc32_ComputeBuf( *outCrc32, buf, bufLen );
-    }
-    return( 0 );
-
-    /** error exit **/
-ERR_EXIT:
-    return( -1 );
-}
-
 /*----------------------------------------------------------------------------*\
  *  NAME:
  *     Crc32_ComputeBuf() - computes the CRC-32 value of a memory buffer
diff --git a/qrexec-lib/ioall.c b/qrexec-lib/ioall.c
index 7a3f249a..38619f2d 100644
--- a/qrexec-lib/ioall.c
+++ b/qrexec-lib/ioall.c
@@ -24,8 +24,9 @@
 #include <stdlib.h>
 #include <fcntl.h>
 #include <errno.h>
+#include "libqubes-rpc-filecopy.h"
 
-void perror_wrapper(const char * msg)
+static void perror_wrapper(const char * msg)
 {
     int prev=errno;
     perror(msg);
diff --git a/qrexec-lib/unpack.c b/qrexec-lib/unpack.c
index 3aa363c9..eb8b69fc 100644
--- a/qrexec-lib/unpack.c
+++ b/qrexec-lib/unpack.c
@@ -45,7 +45,7 @@ void send_status_and_crc(int code, const char *last_filename);
 #define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT)
 #endif
 
-_Noreturn void do_exit(int code, const char *last_filename)
+static _Noreturn void do_exit(int code, const char *last_filename)
 {
     close(0);
     send_status_and_crc(code, last_filename);
@@ -74,7 +74,7 @@ void set_procfs_fd(int value)
     use_tmpfile = 1;
 }
 
-int wait_for_space(int fd, unsigned long how_much) {
+static int wait_for_space(int fd, unsigned long how_much) {
     int counter = 0;
     struct statvfs fs_space;
     do {
@@ -91,7 +91,7 @@ int wait_for_space(int fd, unsigned long how_much) {
 }
 
 static unsigned long crc32_sum = 0;
-int read_all_with_crc(int fd, void *buf, int size) {
+static int read_all_with_crc(int fd, void *buf, int size) {
     int ret;
     ret = read_all(fd, buf, size);
     if (ret)
@@ -196,8 +196,8 @@ static int opendir_safe(int dirfd, char *path, const char **last_segment)
     }
 }
 
-void process_one_file_reg(struct file_header *untrusted_hdr,
-        const char *untrusted_name)
+static void process_one_file_reg(struct file_header *untrusted_hdr,
+                                 const char *untrusted_name)
 {
     int ret;
     int fdout = -1, safe_dirfd;
@@ -260,8 +260,8 @@ void process_one_file_reg(struct file_header *untrusted_hdr,
 }
 
 
-void process_one_file_dir(struct file_header *untrusted_hdr,
-        const char *untrusted_name)
+static void process_one_file_dir(struct file_header *untrusted_hdr,
+                                 const char *untrusted_name)
 {
     int safe_dirfd;
     const char *last_segment;
@@ -291,8 +291,8 @@ void process_one_file_dir(struct file_header *untrusted_hdr,
     free(path_dup);
 }
 
-void process_one_file_link(struct file_header *untrusted_hdr,
-        const char *untrusted_name)
+static void process_one_file_link(struct file_header *untrusted_hdr,
+                                  const char *untrusted_name)
 {
     char untrusted_content[MAX_PATH_LENGTH];
     const char *last_segment;
@@ -331,7 +331,7 @@ void process_one_file_link(struct file_header *untrusted_hdr,
     free(path_dup);
 }
 
-void process_one_file(struct file_header *untrusted_hdr, int flags)
+static void process_one_file(struct file_header *untrusted_hdr, int flags)
 {
     unsigned int namelen;
     if (untrusted_hdr->namelen > MAX_PATH_LENGTH - 1)

From 2a0da19738e51d93288c930667407c4b293e0060 Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Thu, 25 Apr 2024 21:12:07 -0400
Subject: [PATCH 08/15] Avoid redundantly validating symbolic link names

qubes_pure_validate_symbolic_link() validates both the symlink path and
target, so validating the path with a separate
qubes_pure_validate_file_name() call is unnecessary.
---
 qrexec-lib/unpack.c         | 3 +--
 qrexec-lib/validator-test.c | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/qrexec-lib/unpack.c b/qrexec-lib/unpack.c
index eb8b69fc..73659c71 100644
--- a/qrexec-lib/unpack.c
+++ b/qrexec-lib/unpack.c
@@ -298,8 +298,6 @@ static void process_one_file_link(struct file_header *untrusted_hdr,
     const char *last_segment;
     char *path_dup;
     unsigned int filelen;
-    if (!qubes_pure_validate_file_name((const uint8_t *)untrusted_name))
-        do_exit(EILSEQ, untrusted_name); /* FIXME: better error message */
     int safe_dirfd;
     if (untrusted_hdr->filelen > MAX_PATH_LENGTH - 1)
         do_exit(ENAMETOOLONG, untrusted_name);
@@ -312,6 +310,7 @@ static void process_one_file_link(struct file_header *untrusted_hdr,
         do_exit(LEGAL_EOF, untrusted_name); // hopefully remote has produced error message
     untrusted_content[filelen] = 0;
     /*
+     * Sanitize both the path of the symbolic link and its target.
      * Ensure that no immediate subdirectory of ~/QubesIncoming/VMNAME
      * may have symlinks that point out of it.
      */
diff --git a/qrexec-lib/validator-test.c b/qrexec-lib/validator-test.c
index f170bf27..1ecde91b 100644
--- a/qrexec-lib/validator-test.c
+++ b/qrexec-lib/validator-test.c
@@ -118,6 +118,8 @@ int main(int argc, char **argv)
     assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)".."));
     // Symlinks may end in "/".
     assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"a/"));
+    // Symlinks reject invalid paths.
+    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"..", (const uint8_t *)"a/"));
 
     // Greek letters are safe
     assert(qubes_pure_validate_file_name((uint8_t *)u8"\u03b2.txt"));

From 34172dfd7e6c9a67f57180996cd29c2b751e8103 Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Thu, 25 Apr 2024 21:19:07 -0400
Subject: [PATCH 09/15] Better error message for forbidden symbolic links

The error message is not great, but at least it is not actively
misleading anymore.

Will fix QubesOS/qubes-issues#8581 once this is used by qfile-unpacker
and qfile-agent.
---
 qrexec-lib/pack.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/qrexec-lib/pack.c b/qrexec-lib/pack.c
index 5481e73d..1d7fa42c 100644
--- a/qrexec-lib/pack.c
+++ b/qrexec-lib/pack.c
@@ -107,6 +107,16 @@ void wait_for_result(void)
             case EINVAL:
                 call_error_handler("File copy: Corrupted data from packer%s\"%s\"", last_filename_prefix, last_filename);
                 break;
+            case EILSEQ:
+                call_error_handler("Forbidden character in file or link target. Name might look somewhat like \"%s\"", last_filename);
+                break;
+            case ENOLINK:
+                // FIXME: the protocol only provides the name of the file, not what it points to.
+                // FIXME: This assumes that a symlink target was rejected, not a path.  However,
+                // this code should only produces valid paths, so if an invalid path gets sent,
+                // that's a bug.
+                call_error_handler("Cannot verify that link at \"%s\" would not be broken by copy", last_filename);
+                break;
             case EDQUOT:
                 if (ignore_quota_error) {
                     /* skip also CRC check as sender and receiver might be

From aa2466cf251129f20223503cb13cf1f60c4029dc Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Thu, 25 Apr 2024 21:08:20 -0400
Subject: [PATCH 10/15] 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.
---
 qrexec-lib/Makefile                |   3 +
 qrexec-lib/libqubes-rpc-filecopy.h |   1 +
 qrexec-lib/pure.h                  |  52 ++++++++++++-
 qrexec-lib/unicode.c               | 115 +++++++++++++++++++++++------
 qrexec-lib/unpack.c                |  34 +++++----
 qrexec-lib/validator-test.c        |  16 ++++
 6 files changed, 183 insertions(+), 38 deletions(-)

diff --git a/qrexec-lib/Makefile b/qrexec-lib/Makefile
index 2b7eec4c..399664c1 100644
--- a/qrexec-lib/Makefile
+++ b/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
diff --git a/qrexec-lib/libqubes-rpc-filecopy.h b/qrexec-lib/libqubes-rpc-filecopy.h
index c9ae6fc1..280dcab4 100644
--- a/qrexec-lib/libqubes-rpc-filecopy.h
+++ b/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 */
diff --git a/qrexec-lib/pure.h b/qrexec-lib/pure.h
index 7fabb896..3cc5c014 100644
--- a/qrexec-lib/pure.h
+++ b/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
diff --git a/qrexec-lib/unicode.c b/qrexec-lib/unicode.c
index fc4c56c0..15bd5b94 100644
--- a/qrexec-lib/unicode.c
+++ b/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,16 +141,21 @@ 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,
@@ -133,20 +163,26 @@ static ssize_t validate_path(const uint8_t *const untrusted_name, size_t allowed
     // 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,36 +194,61 @@ 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"
@@ -195,7 +256,15 @@ qubes_pure_validate_symbolic_link(const uint8_t *untrusted_name,
     // (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
diff --git a/qrexec-lib/unpack.c b/qrexec-lib/unpack.c
index 73659c71..4cd1bf59 100644
--- a/qrexec-lib/unpack.c
+++ b/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))
diff --git a/qrexec-lib/validator-test.c b/qrexec-lib/validator-test.c
index 1ecde91b..d7ab4b89 100644
--- a/qrexec-lib/validator-test.c
+++ b/qrexec-lib/validator-test.c
@@ -1,6 +1,8 @@
 #include <stdio.h>
 #include <inttypes.h>
 #include <stdlib.h>
+#include <errno.h>
+
 #include "pure.h"
 #include <unicode/utf8.h>
 #ifdef NDEBUG
@@ -103,6 +105,20 @@ int main(int argc, char **argv)
     assert(qubes_pure_validate_file_name((const uint8_t *)".a"));
     assert(qubes_pure_validate_file_name((const uint8_t *)"..a"));
 
+    // Charset validation checks
+    for (unsigned int i = 0; i < 256; ++i) {
+        // These bytes are forbidden even if Unicode filtering is off.
+        bool always_forbidden = i == '/' || i == '.' || i == 0;
+        // NUL is not a valid continuation byte, so if Unicode filtering
+        // is on, all non-ASCII bytes are forbidden.
+        bool bad_unicode = i < 0x20 || i > 0x7E;
+        unsigned char buf[2] = { i, 0 };
+        assert(qubes_pure_validate_file_name_v2(buf, QUBES_PURE_ALLOW_UNSAFE_CHARACTERS) ==
+               (always_forbidden ? -EILSEQ : 0));
+        assert(qubes_pure_validate_file_name_v2(buf, 0) ==
+               (always_forbidden || bad_unicode ? -EILSEQ : 0));
+    }
+
     // Symbolic links
     // Top level cannot be symlink
     assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a", (const uint8_t *)"b"));

From 5dcc596144e093775def1b6611279868539c0abd Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Sun, 12 May 2024 10:18:41 -0400
Subject: [PATCH 11/15] Improve symlink tests

Instead of crashing on the first failed test, log an error message for
each failed test that points to the location of the error.
---
 qrexec-lib/validator-test.c | 65 +++++++++++++++++++++++++++----------
 1 file changed, 48 insertions(+), 17 deletions(-)

diff --git a/qrexec-lib/validator-test.c b/qrexec-lib/validator-test.c
index d7ab4b89..a730e921 100644
--- a/qrexec-lib/validator-test.c
+++ b/qrexec-lib/validator-test.c
@@ -119,23 +119,54 @@ int main(int argc, char **argv)
                (always_forbidden || bad_unicode ? -EILSEQ : 0));
     }
 
-    // Symbolic links
-    // Top level cannot be symlink
-    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a", (const uint8_t *)"b"));
-    // Symbolic links cannot escape
-    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b", (const uint8_t *)"../a"));
-    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b", (const uint8_t *)"../a/b/c"));
-    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"../../a"));
-    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b", (const uint8_t *)"a"));
-    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"../a"));
-    // Absolute symlinks are rejected
-    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"/a"));
-    // Symlinks may end in "..".
-    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)".."));
-    // Symlinks may end in "/".
-    assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"a/"));
-    // Symlinks reject invalid paths.
-    assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"..", (const uint8_t *)"a/"));
+    struct p {
+        const char *const path, *const target, *const file;
+        int const line, flags;
+        bool const allowed;
+    } symlink_checks[] = {
+#define TEST(path_, target_, flags_, allowed_) \
+        { .path = (path_)                      \
+        , .target = (target_)                  \
+        , .file = __FILE__                     \
+        , .line = __LINE__                     \
+        , .flags = (flags_)                    \
+        , .allowed = (allowed_)                \
+        }
+        // Symbolic links
+        // Top level cannot be symlink
+        TEST("a", "b", 0, false),
+        TEST("a/b", "../a", 0, false),
+        TEST("a", "b", 0, false),
+        // Symbolic links cannot escape
+        TEST("a/b", "../a", 0, false),
+        TEST("a/b", "../a", 0, false),
+        TEST("a/b", "../a/b/c", 0, false),
+        TEST("a/b", "../a/b/c", 0, false),
+        TEST("a/b/c", "../../a", 0, false),
+        TEST("a/b/c", "../../a", 0, false),
+        TEST("a/b", "a", 0, true),
+        TEST("a/b/c", "../a", 0, true),
+        // Absolute symlinks are rejected
+        TEST("a/b/c", "/a", 0, false),
+        TEST("a/b/c", "/a", 0, false),
+        // Symlinks may end in "..".
+        TEST("a/b/c", "..", 0, true),
+        // Symlinks may end in "/".
+        TEST("a/b/c", "a/", 0, true),
+        // Invalid paths are rejected.
+        TEST("..", "a/", 0, false),
+    };
+    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);
+        }
+    }
+    assert(!failed);
 
     // Greek letters are safe
     assert(qubes_pure_validate_file_name((uint8_t *)u8"\u03b2.txt"));

From 17c27aca49f61976f0c54c5f257b75d6e74fab93 Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Sun, 12 May 2024 10:20:38 -0400
Subject: [PATCH 12/15] Use for loop instead of do-while

This will make subsequent changes easier.

No functional change intended.
---
 qrexec-lib/unicode.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/qrexec-lib/unicode.c b/qrexec-lib/unicode.c
index 15bd5b94..ade88a4b 100644
--- a/qrexec-lib/unicode.c
+++ b/qrexec-lib/unicode.c
@@ -162,10 +162,9 @@ 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;
-    size_t i = 0;
     if (untrusted_name[0] == '\0')
         return -ENOLINK; // empty path
-    do {
+    for (size_t i = 0; untrusted_name[i]; i++) {
         if (i == 0 || untrusted_name[i - 1] == '/') {
             switch (untrusted_name[i]) {
             case '\0': // impossible, loop exit condition & if statement before
@@ -184,7 +183,7 @@ static ssize_t validate_path(const uint8_t *const untrusted_name,
                     if (allowed_leading_dotdot < 1)
                         return -ENOLINK;
                     allowed_leading_dotdot--;
-                    i += 2; // advance past ".."
+                    i++; // loop will advance past second "."
                     continue;
                 }
                 __attribute__((fallthrough));
@@ -199,16 +198,16 @@ static ssize_t validate_path(const uint8_t *const untrusted_name,
             COMPILETIME_UNREACHABLE;
         } else if ((0x20 <= untrusted_name[i] && untrusted_name[i] <= 0x7E) ||
                    (flags & QUBES_PURE_ALLOW_UNSAFE_CHARACTERS) != 0) {
-            i++;
+            /* loop will advance past this */
         } else {
             int utf8_ret = validate_utf8_char((const unsigned char *)(untrusted_name + i));
             if (utf8_ret > 0) {
-                i += utf8_ret;
+                i += (size_t)(utf8_ret - 1); /* loop will do one more increment */
             } else {
                 return -EILSEQ;
             }
         }
-    } while (untrusted_name[i]);
+    }
     return non_dotdot_components;
 }
 

From af7ae9bd0cf72af29466d5d67f71e30d5a82ee7a Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Sun, 12 May 2024 10:29:33 -0400
Subject: [PATCH 13/15] 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 |  1 +
 qrexec-lib/pure.h                  |  6 +++
 qrexec-lib/unicode.c               | 22 ++++++++--
 qrexec-lib/unpack.c                |  6 ++-
 qrexec-lib/validator-test.c        | 70 +++++++++++++++++++++++-------
 5 files changed, 85 insertions(+), 20 deletions(-)

diff --git a/qrexec-lib/libqubes-rpc-filecopy.h b/qrexec-lib/libqubes-rpc-filecopy.h
index 280dcab4..d9d7a235 100644
--- a/qrexec-lib/libqubes-rpc-filecopy.h
+++ b/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 */
diff --git a/qrexec-lib/pure.h b/qrexec-lib/pure.h
index 3cc5c014..1a5bee47 100644
--- a/qrexec-lib/pure.h
+++ b/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
diff --git a/qrexec-lib/unicode.c b/qrexec-lib/unicode.c
index ade88a4b..24cbc762 100644
--- a/qrexec-lib/unicode.c
+++ b/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.
diff --git a/qrexec-lib/unpack.c b/qrexec-lib/unpack.c
index 4cd1bf59..91b78b09 100644
--- a/qrexec-lib/unpack.c
+++ b/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;
diff --git a/qrexec-lib/validator-test.c b/qrexec-lib/validator-test.c
index a730e921..71b59e73 100644
--- a/qrexec-lib/validator-test.c
+++ b/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);
 

From aa0b85e5a45f4c877da6fde0e6c489d2a77b5d82 Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Sun, 12 May 2024 10:30:35 -0400
Subject: [PATCH 14/15] Allow disabling symbolic link checking

This turns off all checks for symbolic links, returning to the R4.1
behavior.  This is unsafe, but it might be necessary in some cases.
However, it will not be included by either qubes.Filecopy or
qubes.UnsafeFileCopy.
---
 qrexec-lib/libqubes-rpc-filecopy.h | 1 +
 qrexec-lib/pure.h                  | 2 ++
 qrexec-lib/unicode.c               | 5 ++++-
 qrexec-lib/unpack.c                | 2 +-
 qrexec-lib/validator-test.c        | 6 ++++++
 5 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/qrexec-lib/libqubes-rpc-filecopy.h b/qrexec-lib/libqubes-rpc-filecopy.h
index d9d7a235..608a4237 100644
--- a/qrexec-lib/libqubes-rpc-filecopy.h
+++ b/qrexec-lib/libqubes-rpc-filecopy.h
@@ -69,6 +69,7 @@ enum copy_flags {
     COPY_ALLOW_DIRECTORIES = (1 << 1),
     COPY_ALLOW_UNSAFE_CHARACTERS = (1 << 2),
     COPY_ALLOW_NON_CANONICAL_SYMLINKS = (1 << 3),
+    COPY_ALLOW_UNSAFE_SYMLINKS = (1 << 4),
 };
 
 /* feedback handling */
diff --git a/qrexec-lib/pure.h b/qrexec-lib/pure.h
index 1a5bee47..811ee54d 100644
--- a/qrexec-lib/pure.h
+++ b/qrexec-lib/pure.h
@@ -260,6 +260,8 @@ enum QubesFilenameValidationFlags {
     /// 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),
+    /// Do not check symbolic links at all.
+    QUBES_PURE_ALLOW_UNSAFE_SYMLINKS = (1 << 2),
     /// Allow all paths to be non-canonical, including symlinks.
     QUBES_PURE_ALLOW_NON_CANONICAL_PATHS = (1 << 3),
 };
diff --git a/qrexec-lib/unicode.c b/qrexec-lib/unicode.c
index 24cbc762..1f2c748a 100644
--- a/qrexec-lib/unicode.c
+++ b/qrexec-lib/unicode.c
@@ -223,7 +223,8 @@ static bool flag_check(const uint32_t flags)
 {
     int const allowed = (QUBES_PURE_ALLOW_UNSAFE_CHARACTERS |
                          QUBES_PURE_ALLOW_NON_CANONICAL_SYMLINKS |
-                         QUBES_PURE_ALLOW_NON_CANONICAL_PATHS);
+                         QUBES_PURE_ALLOW_NON_CANONICAL_PATHS |
+                         QUBES_PURE_ALLOW_UNSAFE_SYMLINKS);
     return (flags & ~(__typeof__(flags))allowed) == 0;
 }
 
@@ -255,6 +256,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_UNSAFE_SYMLINKS) != 0)
+        return depth > 0 ? 0 : -ENOLINK;
     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
diff --git a/qrexec-lib/unpack.c b/qrexec-lib/unpack.c
index 91b78b09..6b68c733 100644
--- a/qrexec-lib/unpack.c
+++ b/qrexec-lib/unpack.c
@@ -346,7 +346,7 @@ static void process_one_file(struct file_header *untrusted_hdr, int flags)
     // 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_UNSAFE_CHARACTERS | QUBES_PURE_ALLOW_UNSAFE_SYMLINKS |
          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
diff --git a/qrexec-lib/validator-test.c b/qrexec-lib/validator-test.c
index 71b59e73..64c99142 100644
--- a/qrexec-lib/validator-test.c
+++ b/qrexec-lib/validator-test.c
@@ -172,6 +172,12 @@ int main(int argc, char **argv)
         TEST("a/b/c", "a/", 0, true),
         // Invalid paths are rejected...
         TEST("..", "a/", 0, false),
+        // ...even with QUBES_PURE_ALLOW_UNSAFE_SYMLINKS.
+        TEST("..", "a/", QUBES_PURE_ALLOW_UNSAFE_SYMLINKS, false),
+        // but QUBES_PURE_ALLOW_UNSAFE_SYMLINKS allows bad symlinks
+        TEST("a", "/home/user/.bashrc", QUBES_PURE_ALLOW_UNSAFE_SYMLINKS, true),
+        // that are otherwise rejected
+        TEST("a", "/home/user/.bashrc", 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.

From 044c4d433732c6b501c2b26e636cb0cc8622c27a Mon Sep 17 00:00:00 2001
From: Demi Marie Obenour <demi@invisiblethingslab.com>
Date: Wed, 8 May 2024 19:27:10 -0400
Subject: [PATCH 15/15] Don't allow trailing slashes in paths being unpacked

They would result in a misleading ENOENT error due to passing an empty
path to openat().  Instead, reject the path with EILSEQ to indicate a
malformed pathname.
---
 qrexec-lib/pure.h           |  3 +++
 qrexec-lib/unicode.c        | 24 +++++++++++++++++++-----
 qrexec-lib/validator-test.c |  8 ++++++++
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/qrexec-lib/pure.h b/qrexec-lib/pure.h
index 811ee54d..60222c84 100644
--- a/qrexec-lib/pure.h
+++ b/qrexec-lib/pure.h
@@ -253,6 +253,7 @@ enum QubeNameValidationError {
 QUBES_PURE_PUBLIC enum QubeNameValidationError
 qubes_pure_is_valid_qube_name(const struct QubesSlice untrusted_str);
 
+/// Flags for pathname validation functions.
 enum QubesFilenameValidationFlags {
     /// Disable Unicode charset restrictions and UTF-8 validity checks.
     QUBES_PURE_ALLOW_UNSAFE_CHARACTERS = (1 << 0),
@@ -264,6 +265,8 @@ enum QubesFilenameValidationFlags {
     QUBES_PURE_ALLOW_UNSAFE_SYMLINKS = (1 << 2),
     /// Allow all paths to be non-canonical, including symlinks.
     QUBES_PURE_ALLOW_NON_CANONICAL_PATHS = (1 << 3),
+    /// Allow trailing slash.
+    QUBES_PURE_ALLOW_TRAILING_SLASH = (1 << 4),
 };
 
 #ifdef __cplusplus
diff --git a/qrexec-lib/unicode.c b/qrexec-lib/unicode.c
index 1f2c748a..a79c13ae 100644
--- a/qrexec-lib/unicode.c
+++ b/qrexec-lib/unicode.c
@@ -167,7 +167,8 @@ static ssize_t validate_path(const uint8_t *const untrusted_name,
         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++) {
+    size_t i;
+    for (i = 0; untrusted_name[i]; i++) {
         if (i == 0 || untrusted_name[i - 1] == '/') {
             // Start of a path component
             switch (untrusted_name[i]) {
@@ -216,6 +217,14 @@ static ssize_t validate_path(const uint8_t *const untrusted_name,
             }
         }
     }
+    if (i < 1 || untrusted_name[i]) {
+        // ideally this would be COMPILETIME_UNREACHABLE but GCC can't prove this
+        assert(0);
+        return -EILSEQ;
+    }
+    if ((flags & QUBES_PURE_ALLOW_TRAILING_SLASH) == 0 &&
+            untrusted_name[i - 1] == '/')
+        return -EILSEQ;
     return non_dotdot_components;
 }
 
@@ -224,6 +233,7 @@ static bool flag_check(const uint32_t flags)
     int const allowed = (QUBES_PURE_ALLOW_UNSAFE_CHARACTERS |
                          QUBES_PURE_ALLOW_NON_CANONICAL_SYMLINKS |
                          QUBES_PURE_ALLOW_NON_CANONICAL_PATHS |
+                         QUBES_PURE_ALLOW_TRAILING_SLASH |
                          QUBES_PURE_ALLOW_UNSAFE_SYMLINKS);
     return (flags & ~(__typeof__(flags))allowed) == 0;
 }
@@ -243,7 +253,8 @@ qubes_pure_validate_file_name_v2(const uint8_t *const untrusted_filename,
 QUBES_PURE_PUBLIC bool
 qubes_pure_validate_file_name(const uint8_t *const untrusted_filename)
 {
-    return qubes_pure_validate_file_name_v2(untrusted_filename, 0) == 0;
+    return qubes_pure_validate_file_name_v2(untrusted_filename,
+                                            QUBES_PURE_ALLOW_TRAILING_SLASH) == 0;
 }
 
 QUBES_PURE_PUBLIC int
@@ -271,8 +282,10 @@ qubes_pure_validate_symbolic_link_v2(const uint8_t *untrusted_name,
     // (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.
-    ssize_t res = validate_path(untrusted_target, (size_t)(depth - 2), flags);
+    // of ~/QubesIncoming/QUBENAME/a.  Always allow trailing slash in the
+    // symbolic link target, whether or not they are allowed in the path.
+    ssize_t res = validate_path(untrusted_target, (size_t)(depth - 2),
+                                flags | QUBES_PURE_ALLOW_TRAILING_SLASH);
     return res < 0 ? res : 0;
 }
 
@@ -280,7 +293,8 @@ 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;
+    return qubes_pure_validate_symbolic_link_v2(untrusted_name, untrusted_target,
+                                                QUBES_PURE_ALLOW_TRAILING_SLASH) == 0;
 }
 
 QUBES_PURE_PUBLIC bool
diff --git a/qrexec-lib/validator-test.c b/qrexec-lib/validator-test.c
index 64c99142..c68d1992 100644
--- a/qrexec-lib/validator-test.c
+++ b/qrexec-lib/validator-test.c
@@ -182,6 +182,14 @@ int main(int argc, char **argv)
         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),
+        // Symlinks may end in "/"...
+        TEST("a/b/c", "a/", QUBES_PURE_ALLOW_TRAILING_SLASH, true),
+        // ...even without QUBES_PURE_ALLOW_TRAILING_SLASH.
+        TEST("a/b/c", "a/", 0, true),
+        // but the path cannot...
+        TEST("a/b/c/", "a/", 0, false),
+        // ...unless QUBES_PURE_ALLOW_TRAILING_SLASH is passed.
+        TEST("a/b/c/", "a/", QUBES_PURE_ALLOW_TRAILING_SLASH, true),
     };
     int failed = 0;
 #define SYMLINK_TEST(a) symlink_test(a, sizeof(a)/sizeof(a[0]))