| Message ID | 20230720221623.9530-1-jo.vanbulck@cs.kuleuven.be (mailing list archive) |
|---|---|
| Headers | show |
| Series | selftests/sgx: Harden test enclave | expand |
On 7/20/23 15:16, Jo Van Bulck wrote: > While I understand that the bare-metal Intel SGX selftest enclave is > certainly not intended as a full-featured independent production runtime, > it has been noted on this mailing list before that "people are likely to > copy this code for their own enclaves" and that it provides a "great > starting point if you want to do things from scratch" [1]. I wholeheartedly agree with the desire to spin up enclaves without the overhead or complexity of the SDK. I think I'm the one that asked for this test enclave in the first place. There *IS* a gap here. Those who care about SGX would be wise to close this gap in _some_ way. But I don't think the kernel should be the place this is done. The kernel should not be hosting a real-world (userspace) SGX reference implementation. I'd fully support if you'd like to take the selftest code, fork it, and maintain it. The SGX ecosystem would be better off if such a project existed. If I can help here in some way like (trying to) release the SGX selftest under a different license, please let me know. The only patches I want for the kernel are to make the test enclave more *obviously* insecure. So, it's a NAK from me for this series. I won't support merging this into the kernel. But at the same time, I'm very sympathetic to your cause, and I do appreciate your effort here.
On 21.07.23 02:24, Dave Hansen wrote: > I wholeheartedly agree with the desire to spin up enclaves without the > overhead or complexity of the SDK. I think I'm the one that asked for > this test enclave in the first place. There *IS* a gap here. Those who > care about SGX would be wise to close this gap in _some_ way. > > But I don't think the kernel should be the place this is done. The > kernel should not be hosting a real-world (userspace) SGX reference > implementation. Okay, makes sense. > I'd fully support if you'd like to take the selftest code, fork it, and > maintain it. The SGX ecosystem would be better off if such a project > existed. If I can help here in some way like (trying to) release the > SGX selftest under a different license, please let me know. Thank you! I agree this would benefit the SGX ecosystem and I'll go ahead with further developing such a standalone fork when I find time probably in the next month or so. For future reference, in case people end up reading this discussion thread, I created a placeholder (atm emtpy) repo here: https://github.com/jovanbulck/bare-sgx Re licensing: no need to re-license, I think GPL would be the best license for such a project anyway. > The only patches I want for the kernel are to make the test enclave more > *obviously* insecure. Makes sense. I'll see if I can create a new proposed minimal patch in this spirit (e.g., removing existing register cleansing and adding an explicit comment) to take away any misguided impression that the test enclave would be a representative example of secure code and make its real purpose clearer. > So, it's a NAK from me for this series. I won't support merging this > into the kernel. But at the same time, I'm very sympathetic to your > cause, and I do appreciate your effort here. Thank you, appreciated!
Hi, This patch series fixes several issues in the SGX example test enclave: 1. Adhere to enclave programming best practices by sanitizing untrusted user inputs (ABI registers and API pointer arguments). 2. Ensure correct behavior with compiler optimizations (gcc -O{1,2,3,s}). Motivation ========== While I understand that the bare-metal Intel SGX selftest enclave is certainly not intended as a full-featured independent production runtime, it has been noted on this mailing list before that "people are likely to copy this code for their own enclaves" and that it provides a "great starting point if you want to do things from scratch" [1]. Indeed, at least one real-world SGX project (Alibaba Inclavare Containers) appears to build directly on the Linux selftest enclave as a skeleton [2]. Thus, proper and complete example code is vital for security-sensitive functionality, like the selftest example enclave. The purpose of this patch series is, hence, to make the test enclave adhere to required enclave programming defensive measures by, to the extent possible and practical, sanitizing attacker-controlled inputs through minimal checks. Note that this is in line with the existing check in the test enclave to guard against buffer overflow of the encl_op_array through the op.type input. Proposed changes ================ This patch series adds the minimally required sanitization checks, as well as makes the test enclave compatible with gcc compiler optimizations. The added functionality is separated in this patch series as follows: 1. Minimal changes in the enclave entry assembly stub as per the x86-64 ABI expected by the C compiler. Particularly, sanitize the DF and AC bits in RFLAGS, which have been dangerously abused in prior SGX attack research [3,4]. Also add a test case to validate the sanitization. Note that, compiling the existing, unmodified test enclave on my machine (gcc v11.3.0) with -Os optimizations yields assembly code that uses the x86 REP string instructions for memcpy/memset. Hence, such a compiled test enclave would be directly vulnerable to severe memory-corruption attacks by trivially inverting RFLAGS.DF before enclave entry (similar to CVE-2019-14565 -- Intel SA-00293 [3]). Finally note that the proposed patch does currently _not_ sanitize the extended CPU state using XSAVE/XRSTOR, as has been shown in prior attack research to be necessary for SGX enclaves using floating-point instructions [5]. I found that prior versions of the selftest enclave _did_ partially cleanse extended CPU state, but that his functionality has been removed, as it was argued that "the test enclave doesn't touch state managed by XSAVE, let alone put secrets into said state" [6]. However, I found that compiling the unmodified test enclave with gcc -O{2,3} optimization levels may still use the XMM registers to store some intermediate results (i.e., clobber them as allowed per the x86-64 ABI). Hence, for now, add the -mno-sse compilation option to prevent this behavior and add a note to explicitly document the assumption that extended state should remain untouched in the selftest enclave. This may also be an argument to consider re-adding the XRSTOR functionality? 2. Make the selftest enclave aware of its protected ELRANGE: add a linker symbol __enclave_base at the start of the enclave binary and reserve space for __enclave_size to be filled in by the untrusted loader when determining the size of the final enclave image (depending on allocated heap etc.). The final value for __enclave_size is filled in before actual enclave loading and will be measured as part of MRENCLAVE, allowing to trust the size within the enclave validation logic. This approach is similar to how this is done in real-world enclave runtimes (e.g., Intel SGX-SDK). 3. Add minimal validation logic in the enclave C code to ensure that incoming pointer struct arguments properly point outside the enclave before dereference, preventing confused-deputy attacks. Use a C macro to copy struct arguments fully inside the enclave to avoid time-of-check to time-of-use issues for nested pointers. Note that the test enclave deliberately allows arbitrary reads/writes in enclave memory through the get_from_addr/put_to_addr operations for explicit testing purposes. Hence, add an explicit note for this case and only allow remaining unchecked pointer dereferences in these functions. 4. Ensure correct behavior under gcc compiler optimizations. Declare encl_op_array static to ensure RIP-relative addressing is used to access the function-pointer table and rebase the loaded function-pointer entries at runtime before jumping. Declare the secinfo structure as volatile to ensure the compiler passes an aligned address to ENCLU. To ensure future compatibility, it may also be worthwhile to rewrite the test framework to exhaustively execute all tests for test_encl.elf compiled with all possible gcc optimizations -O{0,1,2,3,s}? Best, Jo [1] https://patchwork.kernel.org/comment/23202425/ [2] https://github.com/inclavare-containers/inclavare-containers/tree/master/rune/libenclave/internal/runtime/pal/skeleton [3] https://jovanbulck.github.io/files/ccs19-tale.pdf [4] https://jovanbulck.github.io/files/systex22-abi.pdf [5] https://jovanbulck.github.io/files/acsac20-fpu.pdf [6] https://patchwork.kernel.org/comment/23216515/ Changelog --------- v2 - Add explanation for added test cases (Jarkko) - Rename encl_size_pt and reverse xmas tree ordering (Jarkko) - Use static inline functions instead of C macros (Jarkko) Jo Van Bulck (4): selftests/sgx: Harden test enclave ABI selftests/sgx: Store base address and size in test enclave selftests/sgx: Harden test enclave API selftests/sgx: Fix compiler optimizations in test enclave tools/testing/selftests/sgx/Makefile | 2 +- tools/testing/selftests/sgx/load.c | 3 +- tools/testing/selftests/sgx/main.c | 62 ++++++ tools/testing/selftests/sgx/test_encl.c | 198 +++++++++++++----- tools/testing/selftests/sgx/test_encl.lds | 1 + .../selftests/sgx/test_encl_bootstrap.S | 29 +++ 6 files changed, 246 insertions(+), 49 deletions(-)