Merge remote-tracking branch 'origin/pr/113'

* origin/pr/113:
  Don't allow trailing slashes in paths being unpacked
  Allow disabling symbolic link checking
  Add flag to allow non-canonical symbolic links
  Use for loop instead of do-while
  Improve symlink tests
  Allow disabling character set filtering
  Better error message for forbidden symbolic links
  Avoid redundantly validating symbolic link names
  Fix -Wmissing-declarations and -Wmissing-prototypes warnings
  Avoid casting away const when not needed
  Document the pretty-printer in the code generator
  Move path validation tests before charset tests
  Test that symbolic links can end in '/'
  Allow symlinks ending in ".."
  Document algorithm used to sanitize paths

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

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

qrexec-lib/ioall.c

@@ -24,8 +24,9 @@
 #include <stdlib.h>
 #include <fcntl.h>
 #include <errno.h>
+#include "libqubes-rpc-filecopy.h"
 
-void perror_wrapper(const char * msg)
+static void perror_wrapper(const char * msg)
 {
     int prev=errno;
     perror(msg);

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 */

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

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

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;