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);