Browse Source

[Pal/Linux-SGX] Check ECALL_ENCLAVE_START parameters

With this change we check all parameters from the urts which we can
verify (This is only true if the other PRs we depend on are merged, see
code comments).

We can't verify some parameters like our 'uid' or 'manifest_name'. Here
we have to take the value the urts has passed to us and need to be
careful wherever we use them.

To keep the verification code simple we change the 'arguments' and
'environments' parameters from argv style arrays with pointers to
strings to a continuous NUL-delimited string.
Simon Gaiser 6 years ago
parent
commit
1b3b3487b5

+ 150 - 4
Pal/src/host/Linux-SGX/db_main.c

@@ -129,15 +129,91 @@ static int loader_filter (const char * key, int len)
     return 1;
 }
 
+/*
+ * Takes a pointer+size to an untrusted memory region containing a
+ * NUL-separated list of strings. It builds a argv-style list in trusted memory
+ * with those strings.
+ *
+ * It is responsible for handling the access to untrusted memory safely
+ * (returns NULL on error) and ensures that all strings are properly
+ * terminated. The content of the strings is NOT further sanitized.
+ *
+ * The argv-style list is allocated on the heap and the caller is responsible
+ * to free it (For argv and envp we rely on auto free on termination in
+ * practice).
+ */
+static const char** make_argv_list(void * uptr_src, uint64_t src_size) {
+    const char **argv;
+
+    if (src_size == 0) {
+        argv = malloc(sizeof(char *));
+        argv[0] = NULL;
+        return argv;
+    }
+
+    char * data = malloc(src_size);
+    if (!data) {
+        return NULL;
+    }
+
+    if (sgx_copy_to_enclave(data, uptr_src, src_size) != 0) {
+        goto free_and_err;
+    }
+    data[src_size - 1] = '\0';
+
+    uint64_t argc = 0;
+    for (uint64_t i = 0; i < src_size; i++) {
+        if (data[i] == '\0') {
+            argc++;
+        }
+    }
+
+    size_t argv_size;
+    if (__builtin_mul_overflow(argc + 1, sizeof(char *), &argv_size)) {
+        goto free_and_err;
+    }
+    argv = malloc(argv_size);
+    if (!argv) {
+        goto free_and_err;
+    }
+    argv[argc] = NULL;
+
+    uint64_t data_i = 0;
+    for (uint64_t arg_i = 0; arg_i < argc; arg_i++) {
+        argv[arg_i] = &data[data_i];
+        while (data[data_i] != '\0') {
+            data_i++;
+        }
+        data_i++;
+    }
+
+    return argv;
+
+free_and_err:
+    free(data);
+    return NULL;
+}
+
 extern void * enclave_base;
 
-void pal_linux_main(const char ** arguments, const char ** environments,
-                    struct pal_sec * sec_info)
+void pal_linux_main(char * uptr_args, uint64_t args_size,
+                    char * uptr_env, uint64_t env_size,
+                    struct pal_sec * uptr_sec_info)
 {
+    /*
+     * Our arguments are comming directly from the urts. We are responsible to
+     * check them.
+     */
+
     PAL_HANDLE parent = NULL;
     unsigned long start_time = _DkSystemTimeQuery();
     int rv;
 
+    struct pal_sec sec_info;
+    if (sgx_copy_to_enclave(&sec_info, uptr_sec_info, sizeof(sec_info)) != 0) {
+        return;
+    }
+
     /* relocate PAL itself */
     pal_map.l_addr = elf_machine_load_address();
     pal_map.l_name = ENCLAVE_FILENAME;
@@ -146,7 +222,65 @@ void pal_linux_main(const char ** arguments, const char ** environments,
 
     ELF_DYNAMIC_RELOCATE(&pal_map);
 
-    memcpy(&pal_sec, sec_info, sizeof(struct pal_sec));
+    /*
+     * We can't verify the following arguments from the urts. So we copy
+     * them directly but need to be careful when we use them.
+     */
+
+    pal_sec.instance_id = sec_info.instance_id;
+
+    COPY_ARRAY(pal_sec.exec_name, sec_info.exec_name);
+    pal_sec.exec_name[sizeof(pal_sec.exec_name) - 1] = '\0';
+
+    COPY_ARRAY(pal_sec.manifest_name, sec_info.manifest_name);
+    pal_sec.manifest_name[sizeof(pal_sec.manifest_name) - 1] = '\0';
+
+    COPY_ARRAY(pal_sec.proc_fds, sec_info.proc_fds);
+    COPY_ARRAY(pal_sec.pipe_prefix, sec_info.pipe_prefix);
+    pal_sec.mcast_port = sec_info.mcast_port;
+    pal_sec.mcast_srv = sec_info.mcast_srv;
+    pal_sec.mcast_cli = sec_info.mcast_cli;
+#ifdef DEBUG
+    pal_sec.in_gdb = sec_info.in_gdb;
+#endif
+#if PRINT_ENCLAVE_STAT == 1
+    pal_sec.start_time = sec_info.start_time;
+#endif
+
+    /* For {p,u,g}ids we can at least do some minimal checking. */
+
+    /* ppid should be positive when interpreted as signed. It's 0 if we don't
+     * have a graphene parent process. */
+    if (sec_info.ppid > INT32_MAX) {
+        return;
+    }
+    pal_sec.ppid = sec_info.ppid;
+
+    /* As ppid but we always have a pid, so 0 is invalid. */
+    if (sec_info.pid > INT32_MAX || sec_info.pid == 0) {
+        return;
+    }
+    pal_sec.pid = sec_info.pid;
+
+    /* -1 is treated as special value for example by chown. */
+    if (sec_info.uid == (PAL_IDX)-1 || sec_info.gid == (PAL_IDX)-1) {
+        return;
+    }
+    pal_sec.uid = sec_info.uid;
+    pal_sec.gid = sec_info.gid;
+
+
+    /* TODO: remove with PR #589 */
+    pal_sec.heap_min = sec_info.heap_min;
+    pal_sec.heap_max = sec_info.heap_max;
+
+    /* TODO: remove with PR #588 */
+    pal_sec.exec_addr = sec_info.exec_addr;
+    pal_sec.exec_size = sec_info.exec_size;
+
+    /* TODO: remove with PR #580 */
+    pal_sec.manifest_addr = sec_info.manifest_addr;
+    pal_sec.manifest_size = sec_info.manifest_size;
 
     /* set up page allocator and slab manager */
     init_slab_mgr(pagesz);
@@ -164,6 +298,18 @@ void pal_linux_main(const char ** arguments, const char ** environments,
         ocall_exit(rv);
     }
 
+    if (args_size > MAX_ARGS_SIZE || env_size > MAX_ENV_SIZE) {
+        return;
+    }
+    const char ** arguments = make_argv_list(uptr_args, args_size);
+    if (!arguments) {
+        return;
+    }
+    const char ** environments = make_argv_list(uptr_env, env_size);
+    if (!environments) {
+        return;
+    }
+
     pal_state.start_time = start_time;
 
     /* if there is a parent, create parent handle */
@@ -224,7 +370,7 @@ void pal_linux_main(const char ** arguments, const char ** environments,
 #if PRINT_ENCLAVE_STAT == 1
     printf("                >>>>>>>> "
            "Enclave loading time =      %10ld milliseconds\n",
-           _DkSystemTimeQuery() - sec_info->start_time);
+           _DkSystemTimeQuery() - pal_sec.start_time);
 #endif
 
     /* set up thread handle */

+ 4 - 2
Pal/src/host/Linux-SGX/ecall_types.h

@@ -10,7 +10,9 @@ enum {
 struct pal_sec;
 
 typedef struct {
-    const char ** ms_arguments;
-    const char ** ms_environments;
+    char * ms_args;
+    uint64_t ms_args_size;
+    char * ms_env;
+    uint64_t ms_env_size;
     struct pal_sec * ms_sec_info;
 } ms_ecall_enclave_start_t;

+ 9 - 4
Pal/src/host/Linux-SGX/enclave_ecalls.c

@@ -10,8 +10,9 @@
 
 #define SGX_CAST(type, item) ((type) (item))
 
-void pal_linux_main (const char ** arguments, const char ** environments,
-                     struct pal_sec * sec_info);
+void pal_linux_main(char * uptr_args, uint64_t args_size,
+                    char * uptr_env, uint64_t env_size,
+                    struct pal_sec * uptr_sec_info);
 
 void pal_start_thread (void);
 
@@ -73,9 +74,13 @@ void handle_ecall (long ecall_index, void * ecall_args, void * exit_target,
         ms_ecall_enclave_start_t * ms =
                 (ms_ecall_enclave_start_t *) ecall_args;
 
-        if (!ms) return;
+        if (!ms || !sgx_is_completely_outside_enclave(ms, sizeof(*ms))) {
+            return;
+        }
 
-        pal_linux_main(ms->ms_arguments, ms->ms_environments,
+        /* pal_linux_main is responsible to check the passed arguments */
+        pal_linux_main(ms->ms_args, ms->ms_args_size,
+                       ms->ms_env, ms->ms_env_size,
                        ms->ms_sec_info);
     } else {
         // ENCLAVE_START already called (maybe successfully, maybe not), so

+ 12 - 0
Pal/src/host/Linux-SGX/enclave_framework.c

@@ -37,6 +37,18 @@ bool sgx_is_completely_outside_enclave(const void* addr, uint64_t size) {
     return enclave_base >= addr + size || enclave_top <= addr;
 }
 
+__attribute__((warn_unused_result))
+int sgx_copy_to_enclave(void * dst, void * uptr_src, uint64_t size) {
+    if (!sgx_is_completely_outside_enclave(uptr_src, size) ||
+        !sgx_is_completely_within_enclave(dst, size)) {
+        return -PAL_ERROR_DENIED;
+    }
+
+    memcpy(dst, uptr_src, size);
+
+    return 0;
+}
+
 void * sgx_ocalloc (uint64_t size)
 {
     void * ustack = GET_ENCLAVE_TLS(ustack) - size;

+ 3 - 0
Pal/src/host/Linux-SGX/pal_linux_defs.h

@@ -23,4 +23,7 @@
 
 #define PRINT_ENCLAVE_STAT  (0)
 
+#define MAX_ARGS_SIZE       (10000000)
+#define MAX_ENV_SIZE        (10000000)
+
 #endif /* PAL_LINUX_DEFS_H */

+ 2 - 0
Pal/src/host/Linux-SGX/sgx_api.h

@@ -30,6 +30,8 @@ void sgx_ocfree (void);
 bool sgx_is_completely_within_enclave (const void * addr, uint64_t size);
 bool sgx_is_completely_outside_enclave(const void * addr, uint64_t size);
 
+int sgx_copy_to_enclave(void * dst, void * uptr_src, uint64_t size);
+
 int sgx_report (sgx_arch_targetinfo_t * targetinfo,
                 void * reportdata, sgx_arch_report_t * report);
 

+ 5 - 3
Pal/src/host/Linux-SGX/sgx_enclave.c

@@ -711,11 +711,13 @@ sgx_ocall_fn_t ocall_table[OCALL_NR] = {
 
 #define EDEBUG(code, ms) do {} while (0)
 
-int ecall_enclave_start (const char ** arguments, const char ** environments)
+int ecall_enclave_start (char * args, uint64_t args_size, char * env, uint64_t env_size)
 {
     ms_ecall_enclave_start_t ms;
-    ms.ms_arguments = arguments;
-    ms.ms_environments = environments;
+    ms.ms_args = args;
+    ms.ms_args_size = args_size;
+    ms.ms_env = env;
+    ms.ms_env_size = env_size;
     ms.ms_sec_info = PAL_SEC();
     EDEBUG(ECALL_ENCLAVE_START, &ms);
     return sgx_ecall(ECALL_ENCLAVE_START, &ms);

+ 1 - 1
Pal/src/host/Linux-SGX/sgx_enclave.h

@@ -4,6 +4,6 @@
 #include "pal_linux.h"
 #include "pal_security.h"
 
-int ecall_enclave_start (const char ** arguments, const char ** environments);
+int ecall_enclave_start (char * args, uint64_t args_size, char * env, uint64_t env_size);
 
 int ecall_thread_start (void);

+ 34 - 8
Pal/src/host/Linux-SGX/sgx_main.c

@@ -640,7 +640,8 @@ finalize:
 static int load_enclave (struct pal_enclave * enclave,
                          const char * manifest_uri,
                          const char * exec_uri,
-                         const char ** arguments, const char ** environments,
+                         char * args, uint64_t args_size,
+                         char * env, uint64_t env_size,
                          bool exec_uri_inferred)
 {
     struct pal_sec * pal_sec = &enclave->pal_sec;
@@ -668,14 +669,21 @@ static int load_enclave (struct pal_enclave * enclave,
     pal_sec->gid = INLINE_SYSCALL(getgid, 0);
 
 #ifdef DEBUG
-    for (const char ** e = environments ; *e ; e++) {
-        if (strcmp_static(*e, "IN_GDB=1")) {
+    int env_i = 0;
+    while (env_i < env_size) {
+        if (strcmp_static(&env[env_i], "IN_GDB=1")) {
             SGX_DBG(DBG_I, "being GDB'ed!!!\n");
             pal_sec->in_gdb = true;
         }
 
-        if (strcmp_static(*e, "LD_PRELOAD="))
-            *e = "\0";
+        if (strcmp_static(&env[env_i], "LD_PRELOAD=")) {
+            uint64_t env_i_size = strnlen(&env[env_i], env_size - env_i) + 1;
+            memmove(&env[env_i], &env[env_i + env_i_size], env_size - env_i - env_i_size);
+            env_size -= env_i_size;
+            continue;
+        }
+
+        env_i += strnlen(&env[env_i], env_size - env_i) + 1;
     }
 #endif
 
@@ -803,7 +811,7 @@ static int load_enclave (struct pal_enclave * enclave,
     map_tcs(INLINE_SYSCALL(gettid, 0));
 
     /* start running trusted PAL */
-    ecall_enclave_start(arguments, environments);
+    ecall_enclave_start(args, args_size, env, env_size);
 
 #if PRINT_ENCLAVE_STAT == 1
     PAL_NUM exit_time = 0;
@@ -816,7 +824,7 @@ static int load_enclave (struct pal_enclave * enclave,
     return 0;
 }
 
-int main (int argc, const char ** argv, const char ** envp)
+int main (int argc, char ** argv, char ** envp)
 {
     const char * manifest_uri = NULL;
     char * exec_uri = NULL;
@@ -917,7 +925,25 @@ int main (int argc, const char ** argv, const char ** envp)
     else
         SGX_DBG(DBG_I, "executable file not found\n");
 
-    return load_enclave(enclave, manifest_uri, exec_uri, argv, envp, exec_uri_inferred);
+    /*
+     * While C does not guarantee that the argv[i] and envp[i] strings are
+     * continuous we know that we are running on Linux, which does this. This
+     * saves us creating a copy of all argv and envp strings.
+     */
+    char * args = argv[0];
+    uint64_t args_size = argc > 0 ? (argv[argc - 1] - argv[0]) + strlen(argv[argc - 1]) + 1: 0;
+
+    int envc = 0;
+    while (envp[envc] != NULL) {
+        envc++;
+    }
+    char * env = envp[0];
+    uint64_t env_size = envc > 0 ? (envp[envc - 1] - envp[0]) + strlen(envp[envc - 1]) + 1: 0;
+
+
+    return load_enclave(enclave, manifest_uri, exec_uri,
+                        args, args_size, env, env_size,
+                        exec_uri_inferred);
 
 usage:
     SGX_DBG(DBG_E, "USAGE: %s [executable|manifest] args ...\n", pal_loader);