CVE-2018-20742: Out-of-bounds Write (CWE-787)
value: An issue was discovered in UC Berkeley RISE Opaque before 2018-12-01. There is no boundary check on ocall_malloc. The return value could be a pointer to enclave memory. It could cause an arbitrary enclave memory write.
CWE description: The product writes data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 7.5
Published: 2019-01-24T05:29Z
Repository: https://github.com/ucbrise/opaque
Language: C++
Files changed: src/enclave/App/App.cpp, src/enclave/Enclave/Enclave.cpp, src/enclave/Enclave/Enclave.edl, src/enclave/Enclave/util.cpp, src/enclave/Enclave/util.h
Commit date: 2018-12-01 01:11:22 -0800
Commit message: Check that ecall [user_check] pointers and ocall_malloc result pointer are outside enclave (#67)

This should reduce the enclave's attack surface by preventing an attacker from invoking ecalls on or triggering unexpected writes to arbitrary enclave memory, which could potentially leak information about that memory or lead to incorrect results.

Fixes #36. Fixes #66.
Diff: diff --git a/src/enclave/App/App.cpp b/src/enclave/App/App.cpp
index 66dcbbd4..5d680c2d 100644
--- a/src/enclave/App/App.cpp
+++ b/src/enclave/App/App.cpp
@@ -355,11 +355,11 @@ void ocall_print_string(const char *str)
    */
   printf("%s", str);
   fflush(stdout);
 }
 
-void ocall_malloc(size_t size, uint8_t **ret) {
+void unsafe_ocall_malloc(size_t size, uint8_t **ret) {
   *ret = static_cast<uint8_t *>(malloc(size));
 }
 
 void ocall_free(uint8_t *buf) {
   free(buf);
diff --git a/src/enclave/Enclave/Enclave.cpp b/src/enclave/Enclave/Enclave.cpp
index bca6918e..b78a2645 100644
--- a/src/enclave/Enclave/Enclave.cpp
+++ b/src/enclave/Enclave/Enclave.cpp
@@ -8,19 +8,25 @@
 #include "Filter.h"
 #include "Join.h"
 #include "Project.h"
 #include "Sort.h"
 #include "isv_enclave.h"
+#include "sgx_lfence.h"
 #include "util.h"
 
 // This file contains definitions of the ecalls declared in Enclave.edl. Errors originating within
 // these ecalls are signaled by throwing a std::runtime_error, which is caught at the top level of
 // the ecall (i.e., within these definitions), and are then rethrown as Java exceptions using
 // ocall_throw.
 
 void ecall_encrypt(uint8_t *plaintext, uint32_t plaintext_length,
                    uint8_t *ciphertext, uint32_t cipher_length) {
+  // Guard against encrypting or overwriting enclave memory
+  assert(sgx_is_outside_enclave(plaintext, plaintext_length) == 1);
+  assert(sgx_is_outside_enclave(ciphertext, cipher_length) == 1);
+  sgx_lfence();
+
   try {
     // IV (12 bytes) + ciphertext + mac (16 bytes)
     assert(cipher_length >= plaintext_length + SGX_AESGCM_IV_SIZE + SGX_AESGCM_MAC_SIZE);
     (void)cipher_length;
     (void)plaintext_length;
@@ -31,10 +37,14 @@ void ecall_encrypt(uint8_t *plaintext, uint32_t plaintext_length,
 }
 
 void ecall_project(uint8_t *condition, size_t condition_length,
                    uint8_t *input_rows, size_t input_rows_length,
                    uint8_t **output_rows, size_t *output_rows_length) {
+  // Guard against operating on arbitrary enclave memory
+  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
+  sgx_lfence();
+
   try {
     project(condition, condition_length,
             input_rows, input_rows_length,
             output_rows, output_rows_length);
   } catch (const std::runtime_error &e) {
@@ -43,10 +53,14 @@ void ecall_project(uint8_t *condition, size_t condition_length,
 }
 
 void ecall_filter(uint8_t *condition, size_t condition_length,
                   uint8_t *input_rows, size_t input_rows_length,
                   uint8_t **output_rows, size_t *output_rows_length) {
+  // Guard against operating on arbitrary enclave memory
+  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
+  sgx_lfence();
+
   try {
     filter(condition, condition_length,
            input_rows, input_rows_length,
            output_rows, output_rows_length);
   } catch (const std::runtime_error &e) {
@@ -54,10 +68,14 @@ void ecall_filter(uint8_t *condition, size_t condition_length,
   }
 }
 
 void ecall_sample(uint8_t *input_rows, size_t input_rows_length,
                   uint8_t **output_rows, size_t *output_rows_length) {
+  // Guard against operating on arbitrary enclave memory
+  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
+  sgx_lfence();
+
   try {
     sample(input_rows, input_rows_length,
            output_rows, output_rows_length);
   } catch (const std::runtime_error &e) {
     ocall_throw(e.what());
@@ -66,10 +84,14 @@ void ecall_sample(uint8_t *input_rows, size_t input_rows_length,
 
 void ecall_find_range_bounds(uint8_t *sort_order, size_t sort_order_length,
                              uint32_t num_partitions,
                              uint8_t *input_rows, size_t input_rows_length,
                              uint8_t **output_rows, size_t *output_rows_length) {
+  // Guard against operating on arbitrary enclave memory
+  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
+  sgx_lfence();
+
   try {
     find_range_bounds(sort_order, sort_order_length,
                       num_partitions,
                       input_rows, input_rows_length,
                       output_rows, output_rows_length);
@@ -81,10 +103,15 @@ void ecall_find_range_bounds(uint8_t *sort_order, size_t sort_order_length,
 void ecall_partition_for_sort(uint8_t *sort_order, size_t sort_order_length,
                               uint32_t num_partitions,
                               uint8_t *input_rows, size_t input_rows_length,
                               uint8_t *boundary_rows, size_t boundary_rows_length,
                               uint8_t **output_partitions, size_t *output_partition_lengths) {
+  // Guard against operating on arbitrary enclave memory
+  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
+  assert(sgx_is_outside_enclave(boundary_rows, boundary_rows_length) == 1);
+  sgx_lfence();
+
   try {
     partition_for_sort(sort_order, sort_order_length,
                        num_partitions,
                        input_rows, input_rows_length,
                        boundary_rows, boundary_rows_length,
@@ -95,10 +122,14 @@ void ecall_partition_for_sort(uint8_t *sort_order, size_t sort_order_length,
 }
 
 void ecall_external_sort(uint8_t *sort_order, size_t sort_order_length,
                          uint8_t *input_rows, size_t input_rows_length,
                          uint8_t **output_rows, size_t *output_rows_length) {
+  // Guard against operating on arbitrary enclave memory
+  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
+  sgx_lfence();
+
   try {
     external_sort(sort_order, sort_order_length,
                   input_rows, input_rows_length,
                   output_rows, output_rows_length);
   } catch (const std::runtime_error &e) {
@@ -107,10 +138,14 @@ void ecall_external_sort(uint8_t *sort_order, size_t sort_order_length,
 }
 
 void ecall_scan_collect_last_primary(uint8_t *join_expr, size_t join_expr_length,
                                      uint8_t *input_rows, size_t input_rows_length,
                                      uint8_t **output_rows, size_t *output_rows_length) {
+  // Guard against operating on arbitrary enclave memory
+  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
+  sgx_lfence();
+
   try {
     scan_collect_last_primary(join_expr, join_expr_length,
                               input_rows, input_rows_length,
                               output_rows, output_rows_length);
   } catch (const std::runtime_error &e) {
@@ -120,10 +155,15 @@ void ecall_scan_collect_last_primary(uint8_t *join_expr, size_t join_expr_length
 
 void ecall_non_oblivious_sort_merge_join(uint8_t *join_expr, size_t join_expr_length,
                                          uint8_t *input_rows, size_t input_rows_length,
                                          uint8_t *join_row, size_t join_row_length,
                                          uint8_t **output_rows, size_t *output_rows_length) {
+  // Guard against operating on arbitrary enclave memory
+  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
+  assert(sgx_is_outside_enclave(join_row, join_row_length) == 1);
+  sgx_lfence();
+
   try {
     non_oblivious_sort_merge_join(join_expr, join_expr_length,
                                   input_rows, input_rows_length,
                                   join_row, join_row_length,
                                   output_rows, output_rows_length);
@@ -136,10 +176,14 @@ void ecall_non_oblivious_aggregate_step1(
   uint8_t *agg_op, size_t agg_op_length,
   uint8_t *input_rows, size_t input_rows_length,
   uint8_t **first_row, size_t *first_row_length,
   uint8_t **last_group, size_t *last_group_length,
   uint8_t **last_row, size_t *last_row_length) {
+  // Guard against operating on arbitrary enclave memory
+  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
+  sgx_lfence();
+
   try {
     non_oblivious_aggregate_step1(
       agg_op, agg_op_length,
       input_rows, input_rows_length,
       first_row, first_row_length,
@@ -155,10 +199,17 @@ void ecall_non_oblivious_aggregate_step2(
   uint8_t *input_rows, size_t input_rows_length,
   uint8_t *next_partition_first_row, size_t next_partition_first_row_length,
   uint8_t *prev_partition_last_group, size_t prev_partition_last_group_length,
   uint8_t *prev_partition_last_row, size_t prev_partition_last_row_length,
   uint8_t **output_rows, size_t *output_rows_length) {
+  // Guard against operating on arbitrary enclave memory
+  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
+  assert(sgx_is_outside_enclave(next_partition_first_row, next_partition_first_row_length) == 1);
+  assert(sgx_is_outside_enclave(prev_partition_last_group, prev_partition_last_group_length) == 1);
+  assert(sgx_is_outside_enclave(prev_partition_last_row, prev_partition_last_row_length) == 1);
+  sgx_lfence();
+
   try {
     non_oblivious_aggregate_step2(
       agg_op, agg_op_length,
       input_rows, input_rows_length,
       next_partition_first_row, next_partition_first_row_length,
diff --git a/src/enclave/Enclave/Enclave.edl b/src/enclave/Enclave/Enclave.edl
index abd2d7f1..d8acfc79 100644
--- a/src/enclave/Enclave/Enclave.edl
+++ b/src/enclave/Enclave/Enclave.edl
@@ -86,11 +86,22 @@ enclave {
                                               [in,count=16] uint8_t* gcm_mac);
   };
 
   untrusted {
     void ocall_print_string([in, string] const char *str);
-    void ocall_malloc(size_t size, [out] uint8_t **ret);
+
+    /**
+     * Allocate memory outside of the enclave and return the pointer in `ret`.
+     *
+     * Before dereferencing the resulting pointer, the caller must check whether it is actually
+     * outside the enclave using `sgx_is_outside_enclave()`. Otherwise, an attacker could cause the
+     * enclave to perform unexpected operations on its own memory. The function `ocall_malloc()`
+     * wraps this function with such a bounds check and most callers should use that function
+     * instead.
+     */
+    void unsafe_ocall_malloc(size_t size, [out] uint8_t **ret);
+
     void ocall_free([user_check] uint8_t *buf);
     void ocall_exit(int exit_code);
     void ocall_throw([in, string] const char *message);
   };
 
diff --git a/src/enclave/Enclave/util.cpp b/src/enclave/Enclave/util.cpp
index 3f514e9d..ed863928 100644
--- a/src/enclave/Enclave/util.cpp
+++ b/src/enclave/Enclave/util.cpp
@@ -2,10 +2,11 @@
 
 #include <climits>
 #include <cstdio>
 
 #include "Enclave_t.h"
+#include "sgx_lfence.h"
 
 int printf(const char *fmt, ...) {
   char buf[BUFSIZ] = {'\0'};
   va_list ap;
   va_start(ap, fmt);
@@ -36,10 +37,18 @@ std::string string_format(const std::string &fmt, ...) {
 
 void exit(int exit_code) {
   ocall_exit(exit_code);
 }
 
+void ocall_malloc(size_t size, uint8_t **ret) {
+  unsafe_ocall_malloc(size, ret);
+
+  // Guard against overwriting enclave memory
+  assert(sgx_is_outside_enclave(*ret, size) == 1);
+  sgx_lfence();
+}
+
 void print_bytes(uint8_t *ptr, uint32_t len) {
   for (uint32_t i = 0; i < len; i++) {
     printf("%u", *(ptr + i));
     printf(" - ");
   }
diff --git a/src/enclave/Enclave/util.h b/src/enclave/Enclave/util.h
index 22bf0f43..b4e0b523 100644
--- a/src/enclave/Enclave/util.h
+++ b/src/enclave/Enclave/util.h
@@ -16,10 +16,18 @@ int printf(const char *fmt, ...);
 void exit(int exit_code);
 namespace std {
     using ::exit;
 }
 
+/**
+ * Allocate memory outside of the enclave and return the pointer in `ret`.
+ *
+ * This is a checked wrapper around `unsafe_ocall_malloc`. The resulting pointer is safe to write
+ * to.
+ */
+void ocall_malloc(size_t size, uint8_t **ret);
+
 std::string string_format(const std::string &fmt, ...);
 
 void print_bytes(uint8_t *ptr, uint32_t len);
 
 /** Return 0 if equal, and -1 if not equal. */

Vulnerable code: void ocall_malloc(size_t size, uint8_t **ret) {
    void ocall_malloc(size_t size, [out] uint8_t **ret);
Fixed code: void unsafe_ocall_malloc(size_t size, uint8_t **ret) {
#include "sgx_lfence.h"
  // Guard against encrypting or overwriting enclave memory
  assert(sgx_is_outside_enclave(plaintext, plaintext_length) == 1);
  assert(sgx_is_outside_enclave(ciphertext, cipher_length) == 1);
  sgx_lfence();

  // Guard against operating on arbitrary enclave memory
  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
  sgx_lfence();

  // Guard against operating on arbitrary enclave memory
  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
  sgx_lfence();

  // Guard against operating on arbitrary enclave memory
  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
  sgx_lfence();

  // Guard against operating on arbitrary enclave memory
  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
  sgx_lfence();

  // Guard against operating on arbitrary enclave memory
  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
  assert(sgx_is_outside_enclave(boundary_rows, boundary_rows_length) == 1);
  sgx_lfence();

  // Guard against operating on arbitrary enclave memory
  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
  sgx_lfence();

  // Guard against operating on arbitrary enclave memory
  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
  sgx_lfence();

  // Guard against operating on arbitrary enclave memory
  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
  assert(sgx_is_outside_enclave(join_row, join_row_length) == 1);
  sgx_lfence();

  // Guard against operating on arbitrary enclave memory
  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
  sgx_lfence();

  // Guard against operating on arbitrary enclave memory
  assert(sgx_is_outside_enclave(input_rows, input_rows_length) == 1);
  assert(sgx_is_outside_enclave(next_partition_first_row, next_partition_first_row_length) == 1);
  assert(sgx_is_outside_enclave(prev_partition_last_group, prev_partition_last_group_length) == 1);
  assert(sgx_is_outside_enclave(prev_partition_last_row, prev_partition_last_row_length) == 1);
  sgx_lfence();


    /**
     * Allocate memory outside of the enclave and return the pointer in `ret`.
     *
     * Before dereferencing the resulting pointer, the caller must check whether it is actually
     * outside the enclave using `sgx_is_outside_enclave()`. Otherwise, an attacker could cause the
     * enclave to perform unexpected operations on its own memory. The function `ocall_malloc()`
     * wraps this function with such a bounds check and most callers should use that function
     * instead.
     */
    void unsafe_ocall_malloc(size_t size, [out] uint8_t **ret);

#include "sgx_lfence.h"
void ocall_malloc(size_t size, uint8_t **ret) {
  unsafe_ocall_malloc(size, ret);

  // Guard against overwriting enclave memory
  assert(sgx_is_outside_enclave(*ret, size) == 1);
  sgx_lfence();
}

/**
 * Allocate memory outside of the enclave and return the pointer in `ret`.
 *
 * This is a checked wrapper around `unsafe_ocall_malloc`. The resulting pointer is safe to write
 * to.
 */
void ocall_malloc(size_t size, uint8_t **ret);