From 1bc5d7cad042989e328bedd919f9b94ed001e329 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 8 May 2024 19:27:10 -0400 Subject: [PATCH] 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. (cherry picked from commit 044c4d433732c6b501c2b26e636cb0cc8622c27a) --- 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]))