Extended restricted filesystem to support relative paths.

src/trusted/service_runtime/sel_ldr_filename.c

Index: src/trusted/service_runtime/sel_ldr_filename.c
diff --git a/src/trusted/service_runtime/sel_ldr_filename.c b/src/trusted/service_runtime/sel_ldr_filename.c
index 97556b653420b7513dffa02a1c543b70ea1f68da..30aa0b62c2bf6cec6cff4f8b56e9d0a4383bfc16 100644
--- a/src/trusted/service_runtime/sel_ldr_filename.c
+++ b/src/trusted/service_runtime/sel_ldr_filename.c
@@ -44,8 +44,6 @@ static int PathContainsRootPrefix(const char *path, size_t path_len) {
  * 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.
@@ -54,20 +52,52 @@ static int PathContainsRootPrefix(const char *path, size_t path_len) {
 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;
 }
 
@@ -134,17 +164,17 @@ static uint32_t CopyHostPathMounted(char *dest, size_t dest_max_size) {
   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;