[Pal] Fix bugs in path normalization

Previous implementation had a few bugs in path-normalization functions,
e.g., in get_norm_path(): "../..a/" -> "....a" and "/../a"   -> "../a",
and in get_base_name(): "/" -> garbage (buffer overflow).

Pal/lib/api.h

@@ -166,9 +166,8 @@ extern const char * const * sys_errlist_internal;
 
 /* Graphene functions */
 
-int get_norm_path (const char * path, char * buf, int offset, int size);
-
-int get_base_name (const char * path, char * buf, int size);
+int get_norm_path(const char* path, char* buf, size_t* size);
+int get_base_name(const char* path, char* buf, size_t* size);
 
 /* Loading configs / manifests */
 

Pal/lib/graphene/path.c

@@ -24,115 +24,143 @@
 #include <api.h>
 #include <pal_error.h>
 
-int get_norm_path (const char * path, char * buf, int offset, int size)
-{
-    int head = offset;
-    char c, c1;
-    const char * p = path;
-
-    while (head) { /* find the real head, not interrupted by dot-dot */
-        if (head > 1 && buf[head - 1] == '.' && buf[head - 2] == '.')
-            break;
-        head--;
+/*
+ * Finds next '/' in `path`.
+ * Returns a pointer to it or to the nullbyte ending the string if no '/' has been found.
+ */
+static inline const char* find_next_slash(const char* path) {
+    while (*path && *path != '/') {
+        path++;
+    }
+    return path;
+}
+
+/*
+ * Finds previous '/' in `path` (starting from `size` - 1) and returns offset to it.
+ * If the last character is '/', then it is skipped (as a token can end with '/').
+ */
+static inline size_t find_prev_slash_offset(const char* path, size_t size) {
+    if (size && path[size - 1] == '/') {
+        size--;
+    }
+    while (size && path[size - 1] != '/') {
+        size--;
+    }
+    return size;
+}
+
+/*
+ * Before calling this function *size_ptr should hold the size of buf.
+ * After returning it holds number of bytes actually written to it
+ * (excluding the ending '\0').
+ */
+int get_norm_path(const char* path, char* buf, size_t* size_ptr) {
+    if (!path || !buf || !size_ptr) {
+        return -PAL_ERROR_INVAL;
     }
 
-    for (c = '/' ; c ; c = c1, p++) {
-        c1 = *p;
-        if (c == '/') {     /* find a slash, or the beginning of the path */
-            if (c1 == 0)    /* no more path */
-                break;
-            if (c1 == '/')  /* consequential slashes */
-                continue;
-            if (c1 == '.') {    /* find a dot, can be dot-dot or a file */
-                c1 = *(++p);
-                if (c1 == 0)    /* no more path */
-                    break;
-                if (c1 == '/')  /* a dot, skip it */
-                    continue;
-                if (c1 == '.') {    /* must be dot-dot */
-                    c1 = *(++p);
-                    if (c1 != 0 && c1 != '/') { /* Paths can start with a dot
-                                                 * dot: ..xyz is ok */
-                        if (offset >= size - 2)
-                            return -PAL_ERROR_TOOLONG;
-                        buf[offset++] = '.';
-                        buf[offset++] = '.';
-                        continue;
-                    }
-                    if (offset > head) {    /* remove the last token */
-                        while (offset > head && buf[--offset] != '/');
-                    } else {
-                        if (offset) {   /* add a slash */
-                            if (offset >= size - 1)
-                                return -PAL_ERROR_TOOLONG;
-                            buf[offset++] = '/';
-                        }               /* add a dot-dot */
-                        if (offset >= size - 2)
-                            return -PAL_ERROR_TOOLONG;
-                        buf[offset++] = '.';
-                        buf[offset++] = '.';
-                        head = offset;
-                    }
-                } else { /* it's a file */
-                    if (offset) {   /* add a slash */
-                        if (offset >= size - 1)
-                            return -PAL_ERROR_TOOLONG;
-                        buf[offset++] = '/';
-                    }
-                    if (offset >= size - 1)
-                        return -PAL_ERROR_TOOLONG;
-                    buf[offset++] = '.';
+    size_t size = *size_ptr;
+    if (!size) {
+        return -PAL_ERROR_ZEROSIZE;
+    }
+    /* reserve 1 byte for ending '\0' */
+    size--;
+
+    size_t offset = 0,
+           ret_size = 0; /* accounts for undiscardable bytes written to `buf`
+                          * i.e. `buf - ret_size` points to original `buf` */
+    unsigned char need_slash = 0; // is '/' needed before next token
+    bool is_absolute_path = *path == '/';
+
+    /* handle an absolute path */
+    if (is_absolute_path) {
+        if (size < 1) {
+            return -PAL_ERROR_TOOLONG;
+        }
+        *buf++ = '/';
+        size--;
+        ret_size++;
+        path++;
+    }
+
+    while (1) {
+        /* handle next token */
+        const char* end = find_next_slash(path);
+        if (end - path == 2 && path[0] == '.' && path[1] == '.') {
+            /* ".." */
+            if (offset) {
+                /* eat up previously written token */
+                offset = find_prev_slash_offset(buf, offset);
+                need_slash = 0;
+            } else if (!is_absolute_path) {
+                /* append undiscardable ".." since there is no previous token
+                 * but only if the path is not absolute */
+                if (need_slash + 2u > size) {
+                    return -PAL_ERROR_TOOLONG;
+                }
+                if (need_slash) {
+                    *buf++ = '/';
                 }
-                continue;
+                *buf++ = '.';
+                *buf++ = '.';
+                size -= need_slash + 2u;
+                ret_size += need_slash + 2u;
+                need_slash = 1;
+            } else {
+                /* remaining case: offset == 0, path is absolute and ".." was just seen,
+                 * i.e. "/..", which is collapsed to "/", hence nothing needs to be done */
             }
-        }
-        if (offset || c != '/' || *path == '/') {
-            if (offset >= size - 1)
+        } else if ((end == path) || (end - path == 1 && path[0] == '.')) {
+            /* ignore "//" and "." */
+        } else {
+            size_t len = (size_t)(end - path);
+            if (need_slash + len > size - offset) {
                 return -PAL_ERROR_TOOLONG;
-            buf[offset++] = c;
+            }
+            if (need_slash) {
+                buf[offset++] = '/';
+            }
+            memcpy(buf + offset, path, len);
+            offset += len;
+            need_slash = 1;
         }
+        if (!*end) {
+            break;
+        }
+        path = end + 1;
     }
 
-    buf[offset] = 0;
-    return offset;
-}
+    buf[offset] = '\0';
 
-int get_base_name (const char * path, char * buf, int size)
-{
-    const char * p = path;
-
-    for (; *p ; p++) {
-        if (*p == '/')
-            continue;
-        if (*p == '.') {
-            if (*(p + 1) == '/' || !*(p + 1)) {
-                p++;
-                continue;
-            }
-            if (*(p + 1) == '.') {
-                if (*(p + 2) == '/' || !*(p + 2)) {
-                    p += 2;
-                    continue;
-                }
-            }
-        }
+    *size_ptr = ret_size + offset;
 
-        const char * e = p + 1;
-        for (; *e && *e != '/' ; e++);
-        if (*e) {
-            p = e - 1;
-            continue;
-        }
+    return 0;
+}
 
-        if (e - p > size - 1)
-            return -PAL_ERROR_TOOLONG;
+/*
+ * Before calling this function *size should hold the size of buf.
+ * After returning it holds number of bytes actually written to it
+ * (excluding the trailing '\0').
+ */
+int get_base_name(const char* path, char* buf, size_t* size) {
+    if (!path || !buf || !size) {
+        return -PAL_ERROR_INVAL;
+    }
+
+    const char* end;
+    while (*(end = find_next_slash(path))) {
+        path = end + 1;
+    }
 
-        int offset = 0;
-        for (; p < e ; p++, offset++)
-            buf[offset] = *p;
-        buf[offset] = 0;
-        return offset;
+    size_t result = (size_t)(end - path);
+    if (result + 1 > *size) {
+        return -PAL_ERROR_TOOLONG;
     }
 
+    memcpy(buf, path, result);
+    buf[result] = '\0';
+
+    *size = result;
+
     return 0;
 }

Pal/src/db_main.c

@@ -274,9 +274,13 @@ noreturn void pal_main (
 
         /* The rule is to only find the manifest in the current directory */
         /* try open "<execname>.manifest" */
-        ret = get_base_name(exec_uri, uri_buf, URI_MAX);
+        size_t len = sizeof(uri_buf);
+        ret = get_base_name(exec_uri, uri_buf, &len);
+        if (ret < 0) {
+            INIT_FAIL(-ret, "cannot normalize exec_uri");
+        }
 
-        strcpy_static(uri_buf + ret, ".manifest", URI_MAX - (size_t)ret);
+        strcpy_static(uri_buf + len, ".manifest", sizeof(uri_buf) - len);
         ret = _DkStreamOpen(&manifest_handle, uri_buf, PAL_ACCESS_RDONLY, 0, 0, 0);
         if (ret) {
             /* try open "file:manifest" */

Pal/src/host/FreeBSD/db_main.c

@@ -198,7 +198,7 @@ void pal_bsd_main (void * args)
     const char * pal_name = NULL;
     PAL_HANDLE parent = NULL, exec = NULL, manifest = NULL;
     const char ** argv, ** envp;
-    int argc, ret;
+    int argc;
 
     struct timeval time;
     INLINE_SYSCALL(gettimeofday, 2, &time, NULL);
@@ -234,13 +234,18 @@ void pal_bsd_main (void * args)
         goto done_init;
 
 
-    int len = strlen(argv[0]);
-    PAL_HANDLE file = malloc(HANDLE_SIZE(file) + len + 1);
+    size_t len = strlen(argv[0]) + 1;
+    PAL_HANDLE file = malloc(HANDLE_SIZE(file) + len);
     SET_HANDLE_TYPE(file, file);
     file->hdr.flags |= RFD(0)|WFD(0)|WRITABLE(0);
     file->file.fd = fd;
+
     char * path = (void *) file + HANDLE_SIZE(file);
-    get_norm_path(argv[0], path, 0, len + 1);
+    int ret = get_norm_path(argv[0], path, &len);
+    if (ret < 0) {
+        printf("Could not normalize path (%s): %s\n", argv[0], PAL_STRERROR(ret));
+        goto done_init;
+    }
     file->file.realpath = path;
 
     if (!check_elf_object(file)) {

Pal/src/host/Linux-SGX/db_files.c

@@ -54,24 +54,29 @@ static int file_open (PAL_HANDLE * handle, const char * type, const char * uri,
         return unix_to_pal_error(ERRNO(fd));
 
     /* if try_create_path succeeded, prepare for the file handle */
-    int len = strlen(uri);
-    PAL_HANDLE hdl = malloc(HANDLE_SIZE(file) + len + 1);
+    size_t len = strlen(uri) + 1;
+    PAL_HANDLE hdl = malloc(HANDLE_SIZE(file) + len);
     SET_HANDLE_TYPE(hdl, file);
     HANDLE_HDR(hdl)->flags |= RFD(0)|WFD(0)|WRITABLE(0);
     hdl->file.fd = fd;
     hdl->file.append = 0;
     hdl->file.pass = 0;
     char * path = (void *) hdl + HANDLE_SIZE(file);
-    get_norm_path(uri, path, 0, len + 1);
+    int ret;
+    if ((ret = get_norm_path(uri, path, &len)) < 0) {
+        SGX_DBG(DBG_E, "Could not normalize path (%s): %s\n", uri, PAL_STRERROR(ret));
+        free(hdl);
+        return ret;
+    }
     hdl->file.realpath = (PAL_STR) path;
 
     sgx_stub_t * stubs;
     uint64_t total;
-    int ret = load_trusted_file(hdl, &stubs, &total, create);
+    ret = load_trusted_file(hdl, &stubs, &total, create);
     if (ret < 0) {
         SGX_DBG(DBG_E, "Accessing file:%s is denied. (%s) "
                 "This file is not trusted or allowed.\n", hdl->file.realpath,
-                PAL_STRERROR(-ret));
+                PAL_STRERROR(ret));
         free(hdl);
         return ret;
     }

Pal/src/host/Linux-SGX/db_main.c

@@ -96,8 +96,8 @@ static PAL_HANDLE setup_dummy_file_handle (const char * name)
         return NULL;
 
     name += static_strlen("file:");
-    int len = strlen(name);
-    PAL_HANDLE handle = malloc(HANDLE_SIZE(file) + len + 1);
+    size_t len = strlen(name) + 1;
+    PAL_HANDLE handle = malloc(HANDLE_SIZE(file) + len);
     SET_HANDLE_TYPE(handle, file);
     HANDLE_HDR(handle)->flags |= RFD(0);
     handle->file.fd = PAL_IDX_POISON;
@@ -105,7 +105,12 @@ static PAL_HANDLE setup_dummy_file_handle (const char * name)
     handle->file.pass = 0;
 
     char * path = (void *) handle + HANDLE_SIZE(file);
-    get_norm_path(name, path, 0, len + 1);
+    int ret = get_norm_path(name, path, &len);
+    if (ret < 0) {
+        SGX_DBG(DBG_E, "Could not normalize path (%s): %s\n", name, PAL_STRERROR(ret));
+        free(handle);
+        return NULL;
+    }
     handle->file.realpath = path;
 
     handle->file.total = 0;

Pal/src/host/Linux-SGX/enclave_framework.c

@@ -2,6 +2,7 @@
 #include <pal_linux_error.h>
 #include <pal_internal.h>
 #include <pal_debug.h>
+#include <pal_error.h>
 #include <pal_security.h>
 #include <pal_crypto.h>
 #include <api.h>
@@ -234,16 +235,15 @@ int load_trusted_file (PAL_HANDLE file, sgx_stub_t ** stubptr,
     struct trusted_file * tf = NULL, * tmp;
     char uri[URI_MAX];
     char normpath[URI_MAX];
-    int ret, fd = file->file.fd, len;
-    size_t uri_len;
+    int ret, fd = file->file.fd;
 
     if (!(HANDLE_HDR(file)->flags & RFD(0)))
         return -PAL_ERROR_DENIED;
 
-    len = _DkStreamGetName(file, uri, URI_MAX);
-    if (len < 0)
-        return len;
-    uri_len = (size_t)len;
+    ret = _DkStreamGetName(file, uri, URI_MAX);
+    if (ret < 0) {
+        return ret;
+    }
 
     /* Allow to create the file when allow_file_creation is turned on;
        The created file is added to allowed_file list for later access */
@@ -263,24 +263,26 @@ int load_trusted_file (PAL_HANDLE file, sgx_stub_t ** stubptr,
     normpath [2] = 'l';
     normpath [3] = 'e';
     normpath [4] = ':';
-    len = get_norm_path(uri + 5, normpath + 5, 0, sizeof(normpath) - 5);
-    if (len < 0) {
-        return len;
+    size_t len = sizeof(normpath) - 5;
+    ret = get_norm_path(uri + 5, normpath + 5, &len);
+    if (ret < 0) {
+        SGX_DBG(DBG_E, "Path (%s) normalizaion failed: %s\n", uri + 5, PAL_STRERROR(ret));
+        return ret;
     }
-    uri_len = (size_t)len + 5;
+    len += 5;
 
     _DkSpinLock(&trusted_file_lock);
 
     LISTP_FOR_EACH_ENTRY(tmp, &trusted_file_list, list) {
         if (tmp->stubs) {
             /* trusted files: must be exactly the same URI */
-            if (tmp->uri_len == uri_len && !memcmp(tmp->uri, normpath, uri_len + 1)) {
+            if (tmp->uri_len == len && !memcmp(tmp->uri, normpath, len + 1)) {
                 tf = tmp;
                 break;
             }
         } else {
             /* allowed files: must be a subfolder or file */
-            if (path_is_equal_or_subpath(tmp, normpath, uri_len)) {
+            if (path_is_equal_or_subpath(tmp, normpath, len)) {
                 tf = tmp;
                 break;
             }
@@ -677,8 +679,8 @@ static int init_trusted_file (const char * key, const char * uri)
     tmp = strcpy_static(cskey, "sgx.trusted_checksum.", URI_MAX);
     memcpy(tmp, key, strlen(key) + 1);
 
-    ssize_t len = get_config(pal_state.root_config, cskey, checksum, CONFIG_MAX);
-    if (len < 0)
+    ssize_t ret = get_config(pal_state.root_config, cskey, checksum, CONFIG_MAX);
+    if (ret < 0)
         return 0;
 
     /* Normalize the uri */
@@ -691,7 +693,12 @@ static int init_trusted_file (const char * key, const char * uri)
     normpath [2] = 'l';
     normpath [3] = 'e';
     normpath [4] = ':';
-    len = get_norm_path(uri + 5, normpath + 5, 0, URI_MAX);
+    size_t len = sizeof(normpath) - 5;
+    ret = get_norm_path(uri + 5, normpath + 5, &len);
+    if (ret < 0) {
+        SGX_DBG(DBG_E, "Path (%s) normalizaion failed: %s\n", uri + 5, PAL_STRERROR(ret));
+        return ret;
+    }
 
     return register_trusted_file(normpath, checksum);
 }

Pal/src/host/Linux-SGX/sgx_main.c

@@ -84,8 +84,9 @@ static char * resolve_uri (const char * uri, const char ** errstring)
     }
 
     char path_buf[URI_MAX];
-    int len = get_norm_path(uri + 5, path_buf, 0, URI_MAX);
-    if (len < 0) {
+    size_t len = URI_MAX;
+    int ret = get_norm_path(uri + 5, path_buf, &len);
+    if (ret < 0) {
         *errstring = "Invalid URI";
         return NULL;
     }
@@ -952,18 +953,17 @@ int main (int argc, char ** argv, char ** envp)
     INLINE_SYSCALL(close, 1, fd);
 
     char sgx_manifest[URI_MAX];
-    int len = get_base_name(exec_uri + static_strlen("file:"), sgx_manifest,
-                            URI_MAX);
-    if (len < 0) {
-        ret = len;
+    size_t len = sizeof(sgx_manifest);
+    ret = get_base_name(exec_uri + static_strlen("file:"), sgx_manifest, &len);
+    if (ret < 0) {
         goto out;
     }
 
     if (strcmp_static(sgx_manifest + len - strlen(".manifest"), ".manifest")) {
-        strcpy_static(sgx_manifest + len, ".sgx", URI_MAX - (size_t)len);
+        strcpy_static(sgx_manifest + len, ".sgx", sizeof(sgx_manifest) - len);
     } else if (!strcmp_static(sgx_manifest + len - strlen(".manifest.sgx"),
                               ".manifest.sgx")) {
-        strcpy_static(sgx_manifest + len, ".manifest.sgx", URI_MAX - (size_t)len);
+        strcpy_static(sgx_manifest + len, ".manifest.sgx", sizeof(sgx_manifest) - len);
     }
 
     if (memcmp(filebuf, "\177ELF", 4)) {

Pal/src/host/Linux/db_main.c

@@ -281,8 +281,8 @@ void pal_linux_main (void * args)
         goto done_init;
     }
 
-    int len = strlen(argv[0]);
-    PAL_HANDLE file = malloc(HANDLE_SIZE(file) + len + 1);
+    size_t len = strlen(argv[0]) + 1;
+    PAL_HANDLE file = malloc(HANDLE_SIZE(file) + len);
     SET_HANDLE_TYPE(file, file);
     HANDLE_HDR(file)->flags |= RFD(0)|WFD(0)|WRITABLE(0);
     file->file.fd = fd;
@@ -290,8 +290,13 @@ void pal_linux_main (void * args)
     file->file.append = false;
     file->file.pass = false;
     file->file.map_start = NULL;
+
     char * path = (void *) file + HANDLE_SIZE(file);
-    get_norm_path(argv[0], path, 0, len + 1);
+    int ret = get_norm_path(argv[0], path, &len);
+    if (ret < 0) {
+        printf("Could not normalize path (%s): %s\n", argv[0], PAL_STRERROR(ret));
+        goto done_init;
+    }
     file->file.realpath = path;
 
     if (!check_elf_object(file)) {