Fix CVE-2019-0876:

- _handle_sgx_get_report will now write to the supplied argument only
  if it lies in host memory. This fixes data corruption scenario where
  the untrusted host could supply address of an enclave memory location
  to this function and corrupt the contents of that location.
- Check for missing null terminator in oeedger8r generated code.
  oeedger8r now emits null terminator checks for string and wstring in and
  in-out parameters. Attempts by an untrusted host to manually construct a
  marshalling struct with non null-terminated strings will be detected and
  the edge function will report an error.

Author: anakrish

CHANGELOG.md

@@ -41,6 +41,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   `OE_ENCLAVE_TYPE_AUTO` to have the enclave appropriate to your built environment
   be chosen automatically. For instance, building Intel binaries will select SGX
   automatically, where on ARM it will pick TrustZone.
+- Fix CVE-2019-0876
+   - `_handle_sgx_get_report` will now write to the supplied argument if it les in host memory.
+   - Added check for missing null terminator in oeedger8r generated code.
 
 ### Changed
 

enclave/core/sgx/calls.c

@@ -220,12 +220,16 @@ static oe_result_t _handle_call_enclave_function(uint64_t arg_in)
     args = *args_ptr;
 
     // Ensure that input buffer is valid.
-    if (args.input_buffer == NULL || args.input_buffer_size == 0 ||
+    // Input buffer must be able to hold atleast an oe_result_t.
+    if (args.input_buffer == NULL ||
+        args.input_buffer_size < sizeof(oe_result_t) ||
         !oe_is_outside_enclave(args.input_buffer, args.input_buffer_size))
         OE_RAISE(OE_INVALID_PARAMETER);
 
     // Ensure that output buffer is valid.
-    if (args.output_buffer == NULL || args.output_buffer_size == 0 ||
+    // Output buffer must be able to hold atleast an oe_result_t.
+    if (args.output_buffer == NULL ||
+        args.output_buffer_size < sizeof(oe_result_t) ||
         !oe_is_outside_enclave(args.output_buffer, args.output_buffer_size))
         OE_RAISE(OE_INVALID_PARAMETER);
 
@@ -271,13 +275,20 @@ static oe_result_t _handle_call_enclave_function(uint64_t arg_in)
         args.output_buffer_size,
         &output_bytes_written);
 
-    // Copy outputs to host memory.
-    memcpy(args.output_buffer, output_buffer, output_bytes_written);
+    // The output_buffer is expected to point to a marshaling struct,
+    // whose first field is an oe_result_t. The function is expected
+    // to fill this field with the status of the ecall.
+    result = *(oe_result_t*)output_buffer;
 
-    // The ecall succeeded.
-    args_ptr->output_bytes_written = output_bytes_written;
-    args_ptr->result = OE_OK;
-    result = OE_OK;
+    if (result == OE_OK)
+    {
+        // Copy outputs to host memory.
+        memcpy(args.output_buffer, output_buffer, output_bytes_written);
+
+        // The ecall succeeded.
+        args_ptr->output_bytes_written = output_bytes_written;
+        args_ptr->result = OE_OK;
+    }
 
 done:
     if (buffer)

enclave/core/sgx/report.c

@@ -400,10 +400,11 @@ oe_result_t _handle_get_sgx_report(uint64_t arg_in)
     oe_get_sgx_report_args_t enc_arg;
     size_t report_buffer_size = sizeof(sgx_report_t);
 
-    if (host_arg == NULL)
+    if (!oe_is_outside_enclave(host_arg, sizeof(*host_arg)))
         OE_RAISE(OE_INVALID_PARAMETER);
 
     // Validate and copy args to prevent TOCTOU issues.
+    // oe_get_sgx_report_args_t is a flat structure with no nested pointers.
     enc_arg = *host_arg;
 
     // Host is not allowed to pass report data. Otherwise, the host can use the
@@ -420,9 +421,8 @@ oe_result_t _handle_get_sgx_report(uint64_t arg_in)
 
     *host_arg = enc_arg;
     result = OE_OK;
-
+    host_arg->result = result;
 done:
-    if (host_arg)
-        host_arg->result = result;
+
     return result;
 }

enclave/core/sgx/tracee.c

@@ -6,6 +6,7 @@
 #include <openenclave/bits/types.h>
 #include <openenclave/corelibc/stdarg.h>
 #include <openenclave/corelibc/stdio.h>
+#include <openenclave/corelibc/stdlib.h>
 #include <openenclave/corelibc/string.h>
 #include <openenclave/enclave.h>
 #include <openenclave/internal/calls.h>
@@ -73,6 +74,7 @@ bool is_enclave_debug_allowed()
 oe_result_t _handle_oelog_init(uint64_t arg)
 {
     oe_result_t result = OE_FAILURE;
+    char* path = NULL;
     const char* filename = NULL;
     oe_log_filter_t* filter = (oe_log_filter_t*)arg;
     oe_log_filter_t local;
@@ -98,6 +100,17 @@ oe_result_t _handle_oelog_init(uint64_t arg)
         goto done;
     }
 
+    /* Copy path to enclave memory and add a null-terminator */
+    path = (char*)oe_calloc(1, local.path_len + 1);
+    if (path == NULL)
+    {
+        result = OE_OUT_OF_MEMORY;
+        goto done;
+    }
+    oe_secure_memcpy(path, local.path, local.path_len);
+    path[local.path_len] = '\0';
+    local.path = path;
+
     _active_log_level = local.level;
     filename = get_filename_from_path(local.path, local.path_len);
     if (filename)
@@ -112,6 +125,9 @@ oe_result_t _handle_oelog_init(uint64_t arg)
     _debug_allowed_enclave = is_enclave_debug_allowed();
     result = OE_OK;
 done:
+    if (path)
+        oe_free(path);
+
     return result;
 }
 

include/openenclave/edger8r/common.h

@@ -158,6 +158,34 @@ OE_INLINE oe_result_t oe_add_size(size_t* total, size_t size)
 
 #define OE_READ_IN_OUT_PARAM OE_READ_OUT_PARAM
 
+/**
+ * Check that a string is null terminated.
+ */
+#define OE_CHECK_NULL_TERMINATOR(str, size)                  \
+    {                                                        \
+        const char* _str = (const char*)(str);               \
+        size_t _size = (size_t)(size);                       \
+        if (_str && (_size == 0 || _str[_size - 1] != '\0')) \
+        {                                                    \
+            _result = OE_INVALID_PARAMETER;                  \
+            goto done;                                       \
+        }                                                    \
+    }
+
+/**
+ * Check that a wstring is null terminated.
+ */
+#define OE_CHECK_NULL_TERMINATOR_WIDE(str, size)              \
+    {                                                         \
+        const wchar_t* _str = (const wchar_t*)(str);          \
+        size_t _size = (size_t)(size);                        \
+        if (_str && (_size == 0 || _str[_size - 1] != L'\0')) \
+        {                                                     \
+            _result = OE_INVALID_PARAMETER;                   \
+            goto done;                                        \
+        }                                                     \
+    }
+
 OE_EXTERNC_END
 
 #endif // _OE_EDGER8R_COMMON_H

tests/oeedger8r/edl/string.edl

@@ -38,6 +38,13 @@ enclave {
         public void ecall_wstring_fun7([wstring, in] wchar_t* s1, [wstring, in] wchar_t* s2);
 
         public void test_wstring_edl_ocalls();
+
+        // Negative tests for non null-terminated strings
+	    public void ecall_string_no_null_terminator(
+	                [string, in] char* s1, [string, in, out] char* s2);
+        public void ecall_wstring_no_null_terminator(
+	                [wstring, in] wchar_t* s1, [wstring, in, out] wchar_t* s2);
+
     };
 
     untrusted {
@@ -74,6 +81,10 @@ enclave {
 
         // Multiple string parameters
         void ocall_wstring_fun7([wstring, in] wchar_t* s1, [wstring, in] wchar_t* s2);        
-    };
+
+        // Scenario where host does not null terminate in-out strings.
+	void ocall_string_no_null_terminator(bool erasenull, [string, in, out] char* s);
+	void ocall_wstring_no_null_terminator(bool erasenull, [wstring, in, out] wchar_t* s);
+   };
 };
 

tests/oeedger8r/enc/teststring.cpp

@@ -40,6 +40,17 @@ void test_string_edl_ocalls()
     // Multiple string params. One null.
     OE_TEST(ocall_string_fun7(str, NULL) == OE_OK);
 
+    // Test scenario where host does not null-terminate an
+    // in-out string. The first call preserves the null-terminator.
+    // The second call does not preserve the null terminator.
+    {
+        char str1[] = "Hello";
+        OE_TEST(ocall_string_no_null_terminator(false, str1) == OE_OK);
+        OE_TEST(
+            ocall_string_no_null_terminator(true, str1) ==
+            OE_INVALID_PARAMETER);
+    }
+
     printf("=== test_string_edl_ocalls passed\n");
 }
 
@@ -129,6 +140,18 @@ void ecall_string_fun7(char* s1, char* s2)
     OE_TEST(s2 == NULL);
 }
 
+void ecall_string_no_null_terminator(char* s1, char* s2)
+{
+    OE_UNUSED(s1);
+    OE_UNUSED(s2);
+}
+
+void ecall_wstring_no_null_terminator(wchar_t* s1, wchar_t* s2)
+{
+    OE_UNUSED(s1);
+    OE_UNUSED(s2);
+}
+
 void test_wstring_edl_ocalls()
 {
     const wchar_t* str_value = L"Hello, World\n";
@@ -163,6 +186,17 @@ void test_wstring_edl_ocalls()
     // Multiple string params. One null.
     OE_TEST(ocall_wstring_fun7(str, NULL) == OE_OK);
 
+    // Test scenario where host does not null-terminate an
+    // in-out string. The first call preserves the null-terminator.
+    // The second call does not preserve the null terminator.
+    {
+        wchar_t str1[] = L"Hello";
+        OE_TEST(ocall_wstring_no_null_terminator(false, str1) == OE_OK);
+        OE_TEST(
+            ocall_wstring_no_null_terminator(true, str1) ==
+            OE_INVALID_PARAMETER);
+    }
+
     printf("=== test_wstring_edl_ocalls passed\n");
 }