diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ebb7d51e5..813eb1c675 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/enclave/core/sgx/calls.c b/enclave/core/sgx/calls.c index 9f8c6e04db..83e17824a0 100644 --- a/enclave/core/sgx/calls.c +++ b/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) diff --git a/enclave/core/sgx/report.c b/enclave/core/sgx/report.c index e3ae420bc7..806c5ecaa1 100644 --- a/enclave/core/sgx/report.c +++ b/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; } diff --git a/enclave/core/sgx/tracee.c b/enclave/core/sgx/tracee.c index ea70ce747a..1079794567 100644 --- a/enclave/core/sgx/tracee.c +++ b/enclave/core/sgx/tracee.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -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; } diff --git a/include/openenclave/edger8r/common.h b/include/openenclave/edger8r/common.h index f1f24a86db..3fa63a778e 100644 --- a/include/openenclave/edger8r/common.h +++ b/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 diff --git a/tests/oeedger8r/edl/string.edl b/tests/oeedger8r/edl/string.edl index d04d8f00ea..b6689da863 100644 --- a/tests/oeedger8r/edl/string.edl +++ b/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); + }; }; diff --git a/tests/oeedger8r/enc/teststring.cpp b/tests/oeedger8r/enc/teststring.cpp index 5f44220cb8..18e5947e74 100644 --- a/tests/oeedger8r/enc/teststring.cpp +++ b/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"); } diff --git a/tests/oeedger8r/host/teststring.cpp b/tests/oeedger8r/host/teststring.cpp index 6ae4e5f4c6..01ed9e6bb8 100644 --- a/tests/oeedger8r/host/teststring.cpp +++ b/tests/oeedger8r/host/teststring.cpp @@ -3,11 +3,224 @@ #include "../edltestutils.h" +#include #include #include #include #include "all_u.h" +oe_result_t ecall_string_no_null_terminator_modified( + oe_enclave_t* enclave, + char* s1, + char* s2, + size_t s1_len, + size_t s2_len) +{ + oe_result_t _result = OE_FAILURE; + + /* Marshalling struct */ + ecall_string_no_null_terminator_args_t _args, *_pargs_in = NULL, + *_pargs_out = NULL; + + /* Marshalling buffer and sizes */ + size_t _input_buffer_size = 0; + size_t _output_buffer_size = 0; + size_t _total_buffer_size = 0; + uint8_t* _buffer = NULL; + uint8_t* _input_buffer = NULL; + uint8_t* _output_buffer = NULL; + size_t _input_buffer_offset = 0; + size_t _output_buffer_offset = 0; + size_t _output_bytes_written = 0; + + /* Fill marshalling struct */ + memset(&_args, 0, sizeof(_args)); + _args.s1 = (char*)s1; + _args.s1_len = (s1) ? s1_len : 0; + _args.s2 = (char*)s2; + _args.s2_len = (s2) ? s2_len : 0; + + /* Compute input buffer size. Include in and in-out parameters. */ + OE_ADD_SIZE( + _input_buffer_size, sizeof(ecall_string_no_null_terminator_args_t)); + if (s1) + OE_ADD_SIZE(_input_buffer_size, _args.s1_len * sizeof(char)); + if (s2) + OE_ADD_SIZE(_input_buffer_size, _args.s2_len * sizeof(char)); + + /* Compute output buffer size. Include out and in-out parameters. */ + OE_ADD_SIZE( + _output_buffer_size, sizeof(ecall_string_no_null_terminator_args_t)); + if (s2) + OE_ADD_SIZE(_output_buffer_size, _args.s2_len * sizeof(char)); + + /* Allocate marshalling buffer */ + _total_buffer_size = _input_buffer_size; + OE_ADD_SIZE(_total_buffer_size, _output_buffer_size); + + _buffer = (uint8_t*)malloc(_total_buffer_size); + _input_buffer = _buffer; + _output_buffer = _buffer + _input_buffer_size; + if (_buffer == NULL) + { + _result = OE_OUT_OF_MEMORY; + goto done; + } + + /* Serialize buffer inputs (in and in-out parameters) */ + *(uint8_t**)&_pargs_in = _input_buffer; + OE_ADD_SIZE(_input_buffer_offset, sizeof(*_pargs_in)); + + OE_WRITE_IN_PARAM(s1, _args.s1_len * sizeof(char)); + OE_WRITE_IN_OUT_PARAM(s2, _args.s2_len * sizeof(char)); + + /* Copy args structure (now filled) to input buffer */ + memcpy(_pargs_in, &_args, sizeof(*_pargs_in)); + + /* Call enclave function */ + if ((_result = oe_call_enclave_function( + enclave, + fcn_id_ecall_string_no_null_terminator, + _input_buffer, + _input_buffer_size, + _output_buffer, + _output_buffer_size, + &_output_bytes_written)) != OE_OK) + goto done; + + /* Set up output arg struct pointer */ + *(uint8_t**)&_pargs_out = _output_buffer; + OE_ADD_SIZE(_output_buffer_offset, sizeof(*_pargs_out)); + + /* Check if the call succeeded */ + if ((_result = _pargs_out->_result) != OE_OK) + goto done; + + /* Currently exactly _output_buffer_size bytes must be written */ + if (_output_bytes_written != _output_buffer_size) + { + _result = OE_FAILURE; + goto done; + } + + /* Unmarshal return value and out, in-out parameters */ + OE_CHECK_NULL_TERMINATOR( + _output_buffer + _output_buffer_offset, _args.s2_len); + OE_READ_IN_OUT_PARAM(s2, (size_t)(_args.s2_len * sizeof(char))); + + _result = OE_OK; +done: + if (_buffer) + free(_buffer); + return _result; +} + +oe_result_t ecall_wstring_no_null_terminator_modified( + oe_enclave_t* enclave, + wchar_t* s1, + wchar_t* s2, + size_t s1_len, + size_t s2_len) +{ + oe_result_t _result = OE_FAILURE; + + /* Marshalling struct */ + ecall_wstring_no_null_terminator_args_t _args, *_pargs_in = NULL, + *_pargs_out = NULL; + + /* Marshalling buffer and sizes */ + size_t _input_buffer_size = 0; + size_t _output_buffer_size = 0; + size_t _total_buffer_size = 0; + uint8_t* _buffer = NULL; + uint8_t* _input_buffer = NULL; + uint8_t* _output_buffer = NULL; + size_t _input_buffer_offset = 0; + size_t _output_buffer_offset = 0; + size_t _output_bytes_written = 0; + + /* Fill marshalling struct */ + memset(&_args, 0, sizeof(_args)); + _args.s1 = (wchar_t*)s1; + _args.s1_len = (s1) ? s1_len : 0; + _args.s2 = (wchar_t*)s2; + _args.s2_len = (s2) ? s2_len : 0; + + /* Compute input buffer size. Include in and in-out parameters. */ + OE_ADD_SIZE( + _input_buffer_size, sizeof(ecall_wstring_no_null_terminator_args_t)); + if (s1) + OE_ADD_SIZE(_input_buffer_size, _args.s1_len * sizeof(wchar_t)); + if (s2) + OE_ADD_SIZE(_input_buffer_size, _args.s2_len * sizeof(wchar_t)); + + /* Compute output buffer size. Include out and in-out parameters. */ + OE_ADD_SIZE( + _output_buffer_size, sizeof(ecall_wstring_no_null_terminator_args_t)); + if (s2) + OE_ADD_SIZE(_output_buffer_size, _args.s2_len * sizeof(wchar_t)); + + /* Allocate marshalling buffer */ + _total_buffer_size = _input_buffer_size; + OE_ADD_SIZE(_total_buffer_size, _output_buffer_size); + + _buffer = (uint8_t*)malloc(_total_buffer_size); + _input_buffer = _buffer; + _output_buffer = _buffer + _input_buffer_size; + if (_buffer == NULL) + { + _result = OE_OUT_OF_MEMORY; + goto done; + } + + /* Serialize buffer inputs (in and in-out parameters) */ + *(uint8_t**)&_pargs_in = _input_buffer; + OE_ADD_SIZE(_input_buffer_offset, sizeof(*_pargs_in)); + + OE_WRITE_IN_PARAM(s1, _args.s1_len * sizeof(wchar_t)); + OE_WRITE_IN_OUT_PARAM(s2, _args.s2_len * sizeof(wchar_t)); + + /* Copy args structure (now filled) to input buffer */ + memcpy(_pargs_in, &_args, sizeof(*_pargs_in)); + + /* Call enclave function */ + if ((_result = oe_call_enclave_function( + enclave, + fcn_id_ecall_wstring_no_null_terminator, + _input_buffer, + _input_buffer_size, + _output_buffer, + _output_buffer_size, + &_output_bytes_written)) != OE_OK) + goto done; + + /* Set up output arg struct pointer */ + *(uint8_t**)&_pargs_out = _output_buffer; + OE_ADD_SIZE(_output_buffer_offset, sizeof(*_pargs_out)); + + /* Check if the call succeeded */ + if ((_result = _pargs_out->_result) != OE_OK) + goto done; + + /* Currently exactly _output_buffer_size bytes must be written */ + if (_output_bytes_written != _output_buffer_size) + { + _result = OE_FAILURE; + goto done; + } + + /* Unmarshal return value and out, in-out parameters */ + OE_CHECK_NULL_TERMINATOR_WIDE( + _output_buffer + _output_buffer_offset, _args.s2_len); + OE_READ_IN_OUT_PARAM(s2, (size_t)(_args.s2_len * sizeof(wchar_t))); + + _result = OE_OK; +done: + if (_buffer) + free(_buffer); + return _result; +} + void test_string_edl_ecalls(oe_enclave_t* enclave) { const char* str_value = "Hello, World\n"; @@ -41,6 +254,52 @@ void test_string_edl_ecalls(oe_enclave_t* enclave) // Multiple string params. One null. OE_TEST(ecall_string_fun7(enclave, str, NULL) == OE_OK); + // Test strings without null terminators. + { + char s1[] = "Hello"; + char s2[] = "Hello"; + + // Call function with proper strings. + OE_TEST( + ecall_string_no_null_terminator_modified(enclave, s1, s2, 6, 6) == + OE_OK); + + // Pass s1 without null terminator + OE_TEST( + ecall_string_no_null_terminator_modified(enclave, s1, s2, 5, 6) == + OE_INVALID_PARAMETER); + + // Pass s2 without null terminator + OE_TEST( + ecall_string_no_null_terminator_modified(enclave, s1, s2, 6, 5) == + OE_INVALID_PARAMETER); + } + // Test wstrings without null terminators. + { + OE_UNUSED(ecall_wstring_no_null_terminator_modified); + // wchar_t is not a portable type. Hence the test is performed + // only on Linux. +#ifdef __linux__ + wchar_t s1[] = L"Hello"; + wchar_t s2[] = L"Hello"; + + // Call function with proper strings. + OE_TEST( + ecall_wstring_no_null_terminator_modified(enclave, s1, s2, 6, 6) == + OE_OK); + + // Pass s1 without null terminator + OE_TEST( + ecall_wstring_no_null_terminator_modified(enclave, s1, s2, 5, 6) == + OE_INVALID_PARAMETER); + + // Pass s2 without null terminator + OE_TEST( + ecall_wstring_no_null_terminator_modified(enclave, s1, s2, 6, 5) == + OE_INVALID_PARAMETER); +#endif + } + printf("=== test_string_edl_ecalls passed\n"); } @@ -226,3 +485,17 @@ void ocall_wstring_fun7(wchar_t* s1, wchar_t* s2) OE_TEST(s1 != NULL); OE_TEST(s2 == NULL); } + +void ocall_string_no_null_terminator(bool erasenull, char* s) +{ + size_t size = strlen(s); + if (erasenull) + s[size] = '?'; +} + +void ocall_wstring_no_null_terminator(bool erasenull, wchar_t* s) +{ + size_t size = wcslen(s); + if (erasenull) + s[size] = '?'; +} diff --git a/tools/oeedger8r/Emitter.ml b/tools/oeedger8r/Emitter.ml index afca58dcee..a9f7e4fd0c 100644 --- a/tools/oeedger8r/Emitter.ml +++ b/tools/oeedger8r/Emitter.ml @@ -135,7 +135,7 @@ let oe_mk_ms_struct_name (fname: string) = fname ^ "_args_t" (** [oe_mk_struct_decl] constructs the string of a [struct] definition. *) let oe_mk_struct_decl (fs: string) (name: string) = - sprintf "typedef struct _%s {\n%s oe_result_t _result;\n } %s;\n" name fs name + sprintf "typedef struct _%s {\n oe_result_t _result;\n%s } %s;\n" name fs name (** [oe_gen_marshal_struct_impl] generates a marshalling [struct] definition. *) @@ -412,17 +412,26 @@ let oe_process_output_buffer (os:out_channel) (fd:Ast.func_decl) = match ptype with | Ast.PTPtr (atype, ptr_attr) -> if ptr_attr.Ast.pa_chkptr then - let size = oe_get_param_size (ptype, decl, "_args.") in - match ptr_attr.Ast.pa_direction with - | Ast.PtrOut -> fprintf os " OE_READ_OUT_PARAM(%s, (size_t)(%s));\n" decl.Ast.identifier size - | Ast.PtrInOut -> fprintf os " OE_READ_IN_OUT_PARAM(%s, (size_t)(%s));\n" decl.Ast.identifier size - | _ -> () - else () + let size = oe_get_param_size (ptype, decl, "_args.") in + match ptr_attr.Ast.pa_direction with + | Ast.PtrOut -> + (* strings cannot be out parameters *) + fprintf os " OE_READ_OUT_PARAM(%s, (size_t)(%s));\n" decl.Ast.identifier size + | Ast.PtrInOut -> + (* Check that strings are null terminated. + Note output buffer has already been copied into the enclave.*) + if ptr_attr.Ast.pa_isstr then + fprintf os " OE_CHECK_NULL_TERMINATOR(_output_buffer + _output_buffer_offset, _args.%s_len);\n" decl.Ast.identifier + else if ptr_attr.Ast.pa_iswstr then + fprintf os " OE_CHECK_NULL_TERMINATOR_WIDE(_output_buffer + _output_buffer_offset, _args.%s_len);\n" decl.Ast.identifier + else (); + fprintf os " OE_READ_IN_OUT_PARAM(%s, (size_t)(%s));\n" decl.Ast.identifier size + | _ -> () + else () | _ -> () ) fd.Ast.plist; fprintf os "\n" - (** Generate a cast expression to a specific pointer type. For example, [int*] needs to be cast to {[ @@ -584,6 +593,23 @@ let oe_gen_ecall_function (os:out_channel) (fd: Ast.func_decl) = ) fd.Ast.plist; fprintf os "\n"; + (* Check for null terminators in string parameters *) + fprintf os " /* Check that in/in-out strings are null terminated */\n"; + List.iter (fun (ptype, decl) -> + match ptype with + | Ast.PTPtr (atype, ptr_attr) -> + if ptr_attr.Ast.pa_isstr || ptr_attr.Ast.pa_iswstr then + let check_string = "OE_CHECK_NULL_TERMINATOR" ^ + (if ptr_attr.Ast.pa_iswstr then "_WIDE" else "") in + match ptr_attr.Ast.pa_direction with + | Ast.PtrIn + | Ast.PtrInOut -> fprintf os " %s(pargs_in->%s, pargs_in->%s_len);\n" check_string decl.Ast.identifier decl.Ast.identifier + | _ -> () + else () + | _ -> () + ) fd.Ast.plist; + fprintf os "\n"; + (* Call the enclave function *) fprintf os " /* lfence after checks */\n"; fprintf os " oe_lfence();\n\n";