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 e5e19cd8..f2a601b6 100644 --- a/qrexec-lib/unicode.c +++ b/qrexec-lib/unicode.c @@ -169,7 +169,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]) { @@ -218,6 +219,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; } @@ -226,6 +235,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; } @@ -245,7 +255,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 @@ -273,8 +284,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; } @@ -282,7 +295,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 088c9e2e..30ea79f8 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), }; bool failed = false; for (size_t i = 0; i < sizeof(symlink_checks)/sizeof(symlink_checks[0]); ++i) {