Extended restricted filesystem to support relative paths.

src/trusted/service_runtime/sel_ldr_filename.c

 /*
  * Copyright (c) 2016 The Native Client Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "native_client/src/trusted/service_runtime/sel_ldr_filename.h"
 
 #include <errno.h>
 #include <string.h>
@@ skipping 26 matching lines @@
     return 0;
   return 1;
 }
 
 #if !NACL_WINDOWS
 /*
  * Given a |virtual_path| (a path supplied by the user with no knowledge of
  * the mounted directory) transform it into an |absolute_path|, which is an
  * absolute path prefixed by the root mount directory.
  *
- * TODO(smklein): The virtual_path is assumed to be absolute. Change this.
- *
  * @param[in] virtual_path Virtual path supplied by user.
  * @param[out] absolute_path The absolute path referenced by the |virtual_path|.
  * @param[in] absolute_path_size The size of the |absolute_path| buffer.
  * @return 0 on success, else a negated NaCl errno.
  */
 static uint32_t VirtualToAbsolutePath(const char *virtual_path,
                                       char       *absolute_path,
                                       size_t     absolute_path_max_size) {
+  size_t cwd_path_len;
   size_t virtual_path_len = strlen(virtual_path);
-  /* Check that we have enough room to prepend the prefix (absolute case). */
-  if (virtual_path_len + NaClRootDirLen + 1 > absolute_path_max_size) {
-    NaClLog(LOG_ERROR, "Pathname too long: %s\n", virtual_path);
-    return -NACL_ABI_ENAMETOOLONG;
+  absolute_path[0] = '\0'; /* Required to strcat from start of path. */
+  if (virtual_path[0] == '/') {
+    /* Absolute Path = Prefix + Absolute Virtual Path + '\0' */
+    if (virtual_path_len + NaClRootDirLen + 1 > absolute_path_max_size) {
+      NaClLog(LOG_ERROR, "Pathname too long: %s\n", virtual_path);
+      return -NACL_ABI_ENAMETOOLONG;
+    }
+    /* Prefix */
+    strcat(absolute_path, NaClRootDir);
+    /* Prefix + Virtual Path */
+    strcat(absolute_path, virtual_path);
+  } else {
+    /* Absolute Path = Cwd + '/' + Relative Virtual Path + '\0' */

[Mark Seaborn, 2016/02/17 22:13:23]

You don't actually need to do this concatenation.  You can just use the relative
path directly.  That would be equivalent and simpler -- there's less to go
wrong.

The only difference is that then the ValidateAbsolutePath() check won't pass. 
But that check is redundant -- how about removing it or making it conditional?

[Sean Klein, 2016/02/17 22:42:39]

Although I know it's technically not necessary now, conversion from relative to
absolute paths will be once ".." is added. As you mentioned in a previous code
review, race conditions would be possible with
a) Relative paths containing "..", and
b) Background threads changing the PWD.

This string concatenation will need to be done either now or later. Would you
prefer I cut it out of this CL and paste it into the next one?

(personally I would prefer for these changes to be done more incrementally, but
it's your call! I can remove it for now.)

[Mark Seaborn, 2016/02/18 19:32:02]

So you plan to handle paths containing "..", but without allowing symlinks?
I'd prefer to cut it out of this CL.  I'd prefer to review it only when the need
for it is made apparent by other logic that really needs it. :-)

Also, as I've mentioned before, I really don't want to be in the business of
reviewing C code for manipulating strings in fixed-length buffers, when we could
just change this to a .cc file and use C++'s std::string instead.

Adding a one-off piece of C string manipulation code is one thing, but
incrementally adding more is a sign that we should really be using something
safer.

[Sean Klein, 2016/02/19 18:49:06]

That is the short-term plan, yes.
Got it! I'll leave it out.

I'll start working on translating this file to C++ in a separate CL.

+    int retval = NaClHostDescGetcwd(absolute_path, absolute_path_max_size);
+    if (retval != 0) {
+      NaClLog(LOG_ERROR, "NaClHostDescGetcwd failed\n");
+      return retval;
+    }
+    cwd_path_len = strlen(absolute_path);
+    /*
+     * The prefix cannot be mounted at the root, so we can safely assume that
+     * the Cwd consists of some path component after "/", such as "/foo". This
+     * means that before inserting the relative path, we must insert an
+     * additional "/" at the end of the Cwd.
+     */
+    CHECK(NaClRootDirLen > 1);
+    CHECK(strncmp(absolute_path, NaClRootDir, NaClRootDirLen) == 0);
+    /*
+     * While verifying that the Cwd is inside a root (such as "/root"), ensure
+     * that the Cwd is not only matching the prefix of the root (such as
+     * "/root_but_not_really").
+     */
+    CHECK((absolute_path[NaClRootDirLen] == '/') ||
+          (absolute_path[NaClRootDirLen] == '\0'));
+    if (cwd_path_len + 1 + virtual_path_len + 1 > absolute_path_max_size) {
+      NaClLog(LOG_ERROR, "Pathname too long: %s\n", virtual_path);
+      return -NACL_ABI_ENAMETOOLONG;
+    }
+    /* Cwd + '/' */
+    strcat(absolute_path, "/");
+    /* Cwd + '/' + Relative Path */
+    strcat(absolute_path, virtual_path);
   }
 
-  /* Prefix */
-  memcpy(absolute_path, NaClRootDir, NaClRootDirLen);
-  /* Prefix + Virtual Path */
-  memcpy(absolute_path + NaClRootDirLen, virtual_path, virtual_path_len);
-  /* Prefix + Virtual Path + Terminator */
-  absolute_path[virtual_path_len + NaClRootDirLen] = '\0';
-
   return 0;
 }
 
 /*
  * Determine if |path| points to a symbolic link.
  *
  * @param[in] path Path of file to be checked.
  * @return Nonzero if path is symbolic link.
  */
 static int IsSymbolicLink(const char *path) {
@@ skipping 46 matching lines @@
  * @param[in] dest_max_size The size of the buffer holding dest.
  * @return 0 on success, else a NaCl errno.
  */
 static uint32_t CopyHostPathMounted(char *dest, size_t dest_max_size) {
   uint32_t retval;
   char     raw_path[NACL_CONFIG_PATH_MAX];
 
   if (dest_max_size <= 0 || dest[0] == '\0') {
     NaClLog(LOG_ERROR, "Dest cannot be empty path\n");
     return -NACL_ABI_ENOENT;
-  } else if (dest[0] != '/') {
-    /* TODO(smklein): Allow usage of relative paths. */
-    NaClLog(LOG_ERROR, "Pathname is not absolute: %s\n", dest);
-    return -NACL_ABI_EACCES;
   }
 
   CHECK(dest_max_size == NACL_CONFIG_PATH_MAX);
   CHECK(strlen(dest) < NACL_CONFIG_PATH_MAX);
   strcpy(raw_path, dest);
 
-  /* Transform the user's raw path into an absolute path. */
+  /*
+   * Transform the user's raw path into an absolute path.
+   * The path may be either absolute or relative here -- but it will
+   * be absolute once VirtualToAbsolutePath returns successfully.
+   */
   retval = VirtualToAbsolutePath(raw_path, dest, dest_max_size);
   if (retval != 0)
     return retval;
 
   /* Verify that the path cannot escape root. */
   return ValidateAbsolutePath(dest);
 }
 #endif /* !NACL_WINDOWS */
 
 uint32_t CopyHostPathInFromUser(struct NaClApp *nap,
@@ skipping 60 matching lines @@
       return (uint32_t) -NACL_ABI_EFAULT;
     return 0;
   }
 
   /* Copy out everything after the root dir (including the slash). */
   if (!NaClCopyOutToUser(nap, dst_usr_addr, path + NaClRootDirLen,
                          path_len - NaClRootDirLen + 1))
     return (uint32_t) -NACL_ABI_EFAULT;
   return 0;
 }