diff --git a/qrexec-lib/Makefile b/qrexec-lib/Makefile index 3c918220..399664c1 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 @@ -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/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 #include #include +#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/libqubes-rpc-filecopy.h b/qrexec-lib/libqubes-rpc-filecopy.h index c9ae6fc1..608a4237 100644 --- a/qrexec-lib/libqubes-rpc-filecopy.h +++ b/qrexec-lib/libqubes-rpc-filecopy.h @@ -67,6 +67,9 @@ enum copy_flags { COPY_DEFAULT = 0, COPY_ALLOW_SYMLINKS = (1 << 0), 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/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 diff --git a/qrexec-lib/pure.h b/qrexec-lib/pure.h index 7fabb896..60222c84 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,22 @@ 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), + /// 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), + /// 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), + /// Allow trailing slash. + QUBES_PURE_ALLOW_TRAILING_SLASH = (1 << 4), +}; + #ifdef __cplusplus } #endif 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; diff --git a/qrexec-lib/unicode.c b/qrexec-lib/unicode.c index 4cd42edb..a79c13ae 100644 --- a/qrexec-lib/unicode.c +++ b/qrexec-lib/unicode.c @@ -5,6 +5,7 @@ #include #include #include +#include #include QUBES_PURE_PUBLIC bool @@ -101,25 +102,97 @@ static int validate_utf8_char(const uint8_t *untrusted_c) { return qubes_pure_code_point_safe_for_display(code_point) ? total_size : 0; } -static size_t validate_path(const uint8_t *const untrusted_name, size_t allowed_leading_dotdot) +// 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). +// +// 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. 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 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) { - size_t non_dotdot_components = 0, i = 0; - do { + // 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; + bool const allow_non_canonical = (flags & QUBES_PURE_ALLOW_NON_CANONICAL_PATHS); + if (untrusted_name[0] == '\0') + return allow_non_canonical ? 0 : -ENOLINK; // empty path + if (untrusted_name[0] == '/') + return -ENOLINK; // absolute path + 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]) { - case '/': // repeated or initial slash - case '\0': // trailing slash or empty string - return 0; + case '\0': // impossible, loop exit condition & if statement before + // loop check this + COMPILETIME_UNREACHABLE; + case '/': // repeated slash + if (allow_non_canonical) + continue; + return -EILSEQ; case '.': - if (untrusted_name[i + 1] == '\0' || untrusted_name[i + 1] == '/') - return 0; + 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] == '.') && (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 -ENOLINK; allowed_leading_dotdot--; - i += 2; // advance past ".." + i++; // loop will advance past second "." continue; } __attribute__((fallthrough)); @@ -129,40 +202,99 @@ static size_t validate_path(const uint8_t *const untrusted_name, size_t allowed_ break; } } - if (untrusted_name[i] >= 0x20 && untrusted_name[i] <= 0x7E) { - i++; + 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) { + /* 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 0; + return -EILSEQ; } } - } 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; } -QUBES_PURE_PUBLIC bool -qubes_pure_validate_file_name(const uint8_t *untrusted_filename) +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; +} + +QUBES_PURE_PUBLIC int +qubes_pure_validate_file_name_v2(const uint8_t *const untrusted_filename, + const uint32_t flags) { - return validate_path(untrusted_filename, 0) > 0; + 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); + // Always return -EILSEQ, since -ENOLINK only makes sense for symlinks + return res > 0 ? 0 : -EILSEQ; } 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) { - size_t depth = validate_path(untrusted_name, 0); + return qubes_pure_validate_file_name_v2(untrusted_filename, + QUBES_PURE_ALLOW_TRAILING_SLASH) == 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 + 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 - // 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; + 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" cannot point to "../c", and "a/b/c" can point to "../d" - // but not "../../d". - return validate_path(untrusted_target, depth - 2) > 0; + // "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. 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, + QUBES_PURE_ALLOW_TRAILING_SLASH) == 0; } QUBES_PURE_PUBLIC bool diff --git a/qrexec-lib/unpack.c b/qrexec-lib/unpack.c index b3c3f456..6b68c733 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,16 +196,18 @@ 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, + uint32_t flags) { int ret; int fdout = -1, safe_dirfd; const char *last_segment; char *path_dup; - if (!qubes_pure_validate_file_name((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); @@ -260,14 +262,16 @@ 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, + uint32_t flags) { int safe_dirfd; const char *last_segment; char *path_dup; - if (!qubes_pure_validate_file_name((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); @@ -291,15 +295,14 @@ 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, + uint32_t flags) { char untrusted_content[MAX_PATH_LENGTH]; const char *last_segment; char *path_dup; unsigned int filelen; - if (!qubes_pure_validate_file_name((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,12 +315,15 @@ 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. */ - if (!qubes_pure_validate_symbolic_link((uint8_t *)untrusted_name, - (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); @@ -331,21 +337,26 @@ 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) do_exit(ENAMETOOLONG, NULL); /* filename too long so not received at all */ namelen = untrusted_hdr->namelen; /* sanitized above */ + // 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_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 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 0adf55b9..c68d1992 100644 --- a/qrexec-lib/validator-test.c +++ b/qrexec-lib/validator-test.c @@ -1,6 +1,8 @@ #include #include #include +#include + #include "pure.h" #include #ifdef NDEBUG @@ -65,12 +67,161 @@ 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) { (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((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((const uint8_t *)"a//b")); + + // No "." as a path component + 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((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")); + 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)); + } + + const struct symlink_test 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), + // ...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. + 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])) + 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); + // 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,47 +325,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")); }