Don't allow trailing slashes in paths being unpacked

They would just result in a confusing ENOENT error.
QubesOS · May 9, 2024 · f40aeb8 · f40aeb8
1 parent 6c6923e
commit f40aeb8
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 7 deletions.
diff --git a/qrexec-lib/pure.h b/qrexec-lib/pure.h
@@ -253,8 +253,12 @@ 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 character set filtering.
     QUBES_PURE_NO_UNICODE_FILTERING = (1 << 0),
+    /// Allow trailing slash.
+    QUBES_PURE_ALLOW_TRAILING_SLASH = (1 << 1),
 };
 
 #ifdef __cplusplus

diff --git a/qrexec-lib/unicode.c b/qrexec-lib/unicode.c
@@ -168,7 +168,7 @@ static ssize_t validate_path(const uint8_t *const untrusted_name,
     if (untrusted_name[0] == '\0')
         return -ENOLINK; // empty path
     do {
-        if (i == 0 || untrusted_name[i - 1] == '/') {
+        if (i <= 0 || untrusted_name[i - 1] == '/') {
             switch (untrusted_name[i]) {
             case '\0': // impossible, loop exit condition & if statement before
                        // loop check this
@@ -211,12 +211,21 @@ static ssize_t validate_path(const uint8_t *const untrusted_name,
             }
         }
     } while (untrusted_name[i]);
+    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;
 }
 
 static bool flag_check(const uint32_t flags)
 {
-    return (flags & ~(__typeof__(flags))(QUBES_PURE_NO_UNICODE_FILTERING)) == 0;
+    return (flags & ~(__typeof__(flags))(QUBES_PURE_NO_UNICODE_FILTERING|
+                                         QUBES_PURE_ALLOW_TRAILING_SLASH)) == 0;
 }
 
 QUBES_PURE_PUBLIC int
@@ -235,7 +244,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
@@ -259,16 +269,19 @@ 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;
 }
 
 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
@@ -132,8 +132,18 @@ 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 "/".
+    // Symlinks may end in "/"...
     assert(qubes_pure_validate_symbolic_link((const uint8_t *)"a/b/c", (const uint8_t *)"a/"));
+    // ...even without QUBES_PURE_ALLOW_TRAILING_SLASH.
+    assert(qubes_pure_validate_symbolic_link_v2((const uint8_t *)"a/b/c",
+                                                (const uint8_t *)"a/", 0) == 0);
+    // but the path cannot...
+    assert(qubes_pure_validate_symbolic_link_v2((const uint8_t *)"a/b/c/",
+                                                (const uint8_t *)"a/", 0) == -EILSEQ);
+    // ...unless QUBES_PURE_ALLOW_TRAILING_SLASH is passed.
+    assert(qubes_pure_validate_symbolic_link_v2((const uint8_t *)"a/b/c/",
+                                                (const uint8_t *)"a/",
+                                                QUBES_PURE_ALLOW_TRAILING_SLASH) == 0);
     // Symlinks reject invalid paths.
     assert(!qubes_pure_validate_symbolic_link((const uint8_t *)"..", (const uint8_t *)"a/"));