| Message ID | 20230719142500.13623-1-jo.vanbulck@cs.kuleuven.be (mailing list archive) |
|---|---|
| Headers | show |
| Series | selftests/sgx: Harden test enclave | expand |
On Wed Jul 19, 2023 at 5:24 PM EEST, 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]. Thus, proper and > complete example code is vital for security-sensitive functionality, like the > selftest example enclave. If anyone copied the source code for their own enclave, they would have to publish their source code, given the GPLv2 license. There's a lot of source code in kselftest, which probably has at least some security issues. I'm not sure, at least based on this motivation, why would we care? BR, Jarkko
On 20.07.23 19:25, Jarkko Sakkinen wrote: > There's a lot of source code in kselftest, which probably has at least > some security issues. > > I'm not sure, at least based on this motivation, why would we care? I'd argue that, in general, code examples are often used as templates and may thus inherit any vulnerabilities therein. This may be especially relevant here as your selftest enclave is in my knowledge the only available truly minimal SGX enclave that can be built and extended while only relying on standard tools and no heavy frameworks like the Intel SGX SDK. Thus, as noted before on this mailing list, it may be an attractive start for people who want to build things from scratch. IMHO the example enclave should do a best effort to reasonably follow SGX coding best practices and not have _known_ security vulnerabilities in it. Note that these are not advanced microarchitectural attacks with ugly LFENCE defenses, but plain, architectural memory-safety exploit preventions with minimal sanitization checks, not unlike the existing protections against buffer overflow where best practices are followed for op->type. Apart from that, the added checks only enforce correct behavior in the test framework, only validating that things are sane and as expected. Thus, to some extent, the added checks may even increase resilience of the test framework. Best, Jo
On 7/20/23 12:12, Jo Van Bulck wrote: > On 20.07.23 19:25, Jarkko Sakkinen wrote: >> There's a lot of source code in kselftest, which probably has at least >> some security issues. >> >> I'm not sure, at least based on this motivation, why would we care? > > I'd argue that, in general, code examples are often used as templates > and may thus inherit any vulnerabilities therein. This may be especially > relevant here as your selftest enclave is in my knowledge the only > available truly minimal SGX enclave that can be built and extended while > only relying on standard tools and no heavy frameworks like the Intel > SGX SDK. Thus, as noted before on this mailing list, it may be an > attractive start for people who want to build things from scratch. > > IMHO the example enclave should do a best effort to reasonably follow > SGX coding best practices and not have _known_ security vulnerabilities > in it. On the other hand, if we don't leave glaring, known "security" vulnerabilities in it, even more people will be fooled into trying to use our example code for something that needs actual security. I personally don't know the first thing about writing a secure enclave. I just know it's _really_ hard and I honestly don't expect someone to do it without the help of the SDK.
On 20.07.23 21:56, Dave Hansen wrote: > On the other hand, if we don't leave glaring, known "security" > vulnerabilities in it, even more people will be fooled into trying to > use our example code for something that needs actual security. I see the reasoning, but I'm afraid it's generally hard to stop people from copying good examples as templates for their own projects.. I do believe in the value of clean, minimal open-source example enclave code. In this respect, I personally (and others on the past mailing list as well, it seems) really like the minimal self-contained Linux selftest enclave! I think, with the fixes in this patch series, the Linux selftest enclave can continue to bring value to the community and help in further diversifying the open-source SGX ecosystem. FWIW, I'd not call these "glaring" security holes, but rather subtle oversights that I think most people who would copy the code today may well not be aware of and inherit unknowingly (e.g., reference [2] from my original message did a wide-scale study of the open-source SGX ecosystem as of 2019 and showed exactly these kinds of ABI/API vulnerabilities were widespread and re-occurring in several SGX production projects). > I personally don't know the first thing about writing a secure enclave. > I just know it's _really_ hard and I honestly don't expect someone to do > it without the help of the SDK. Agreed, it _is_ hard indeed. And it has been a moving target over the years, especially with software/compiler defenses for different waves of microarchitectural vulnerabilities (Spectre, LVI, etc.). That being said, I do think that we learned a lot as a community and we have a much better grasp on how to write (reasonably) secure enclave software these days. Sanitizing the ABI and API remains a core enclave software responsibility (whereas microarchitectural attacks can arguably be mostly mitigated through hardware silicon/ucode patches and/or automatic compiler mitigations). I do agree that sane end users should use a shielding runtime to abstract away most of these concerns, where the Intel SGX SDK is just one of many in a growing open-source SGX ecosystem (see for instance earlier references [2,3] for an overview). But there may be good reasons to want to do things from scratch when building your own SGX shielding runtime, e.g., support for custom languages (Rust, Go) or library OSs. I think the Linux selftest enclave helps in further diversifying the open-source SGX ecosystem and can provide an excellent starting point (with the fixes in this patch series). Best, Jo
On Thu Jul 20, 2023 at 7:12 PM UTC, Jo Van Bulck wrote: > On 20.07.23 19:25, Jarkko Sakkinen wrote: > > There's a lot of source code in kselftest, which probably has at least > > some security issues. > > > > I'm not sure, at least based on this motivation, why would we care? > > I'd argue that, in general, code examples are often used as templates > and may thus inherit any vulnerabilities therein. This may be especially > relevant here as your selftest enclave is in my knowledge the only > available truly minimal SGX enclave that can be built and extended while > only relying on standard tools and no heavy frameworks like the Intel > SGX SDK. Thus, as noted before on this mailing list, it may be an > attractive start for people who want to build things from scratch. If you use this code as a template, you have a legal risk in your hands because of GPLv2 licensing. > IMHO the example enclave should do a best effort to reasonably follow > SGX coding best practices and not have _known_ security vulnerabilities > in it. Note that these are not advanced microarchitectural attacks with > ugly LFENCE defenses, but plain, architectural memory-safety exploit > preventions with minimal sanitization checks, not unlike the existing > protections against buffer overflow where best practices are followed > for op->type. I'm not sure what are the "best practices" behavior in the context of a kselftest instance. > Apart from that, the added checks only enforce correct behavior in the > test framework, only validating that things are sane and as expected. > Thus, to some extent, the added checks may even increase resilience of > the test framework. I'm not sure what is "correct" behavior in the context of a kselftest instance. > Best, > Jo This code is not meant for production. I implemented it specifically for kselftest, and that is exactly its scope. BR, Jarkko
On 22.07.23 20:10, Jarkko Sakkinen wrote: > This code is not meant for production. I implemented it specifically for > kselftest, and that is exactly its scope. I see, makes sense. As per Dave's suggestion, I'll see if I can submit a proposed minimal patch to remove any existing sanitization code that is not necessary for kselftest (eg register cleansing) and avoid any misguided impressions of the test enclave being representative. > I'm not sure what is "correct" behavior in the context of a kselftest > instance. True. But at least when defining "correct" as passing the selftests, then I think it makes sense to merge the compiler optimization fixes. As the existing code clearly emits wrong assembly that breaks the selftests when switching optimization levels (which may always also be incorporated by default in future gcc versions or other compilers like clang). Thus, I'll separate this out and submit another patch to ensure correctness with compiler optimizations only. Best, Jo
On Mon Jul 24, 2023 at 10:46 AM UTC, Jo Van Bulck wrote: > On 22.07.23 20:10, Jarkko Sakkinen wrote: > > This code is not meant for production. I implemented it specifically for > > kselftest, and that is exactly its scope. > > I see, makes sense. As per Dave's suggestion, I'll see if I can submit a > proposed minimal patch to remove any existing sanitization code that is > not necessary for kselftest (eg register cleansing) and avoid any > misguided impressions of the test enclave being representative. > > > I'm not sure what is "correct" behavior in the context of a kselftest > > instance. > > True. But at least when defining "correct" as passing the selftests, > then I think it makes sense to merge the compiler optimization fixes. As > the existing code clearly emits wrong assembly that breaks the selftests > when switching optimization levels (which may always also be > incorporated by default in future gcc versions or other compilers like > clang). > > Thus, I'll separate this out and submit another patch to ensure > correctness with compiler optimizations only. > > Best, > Jo It should be relatively easy to relicense the code as most of the commits have Intel copyright. Personally I would not mind because that would give opportunity for code that I wrote to have a wider audience but it needs to be forked with some other license first. BR, Jarkko
On 28.07.23 20:54, Jarkko Sakkinen wrote > It should be relatively easy to relicense the code as most of the > commits have Intel copyright. > > Personally I would not mind because that would give opportunity for > code that I wrote to have a wider audience but it needs to be forked > with some other license first. > I support also the idea of refining the selftest as a run-time, which > could perhaps consist of the following steps: > > 1. Create a repository of the self-compiling selftest with GPLv2. You > could add also AUTHORS file for the initial content by crawling this > data from the git log. > 2. Create a commit with sob's from the required stakeholders, which > changes the license to something more appropriate, and get the > sob's with some process. Thank you Jarkko, appreciated! I plan to start working on the fork from next month onwards. However, I think GPL would be the best license for this project and I'd prefer to stick to it for the time being. Best, Jo
On Mon Aug 7, 2023 at 9:06 AM EEST, Jo Van Bulck wrote: > On 28.07.23 20:54, Jarkko Sakkinen wrote > > It should be relatively easy to relicense the code as most of the > > commits have Intel copyright. > > > > Personally I would not mind because that would give opportunity for > > code that I wrote to have a wider audience but it needs to be forked > > with some other license first. > > > I support also the idea of refining the selftest as a run-time, which > > could perhaps consist of the following steps: > > > > 1. Create a repository of the self-compiling selftest with GPLv2. You > > could add also AUTHORS file for the initial content by crawling this > > data from the git log. > > 2. Create a commit with sob's from the required stakeholders, which > > changes the license to something more appropriate, and get the > > sob's with some process. > > Thank you Jarkko, appreciated! I plan to start working on the fork from > next month onwards. However, I think GPL would be the best license for > this project and I'd prefer to stick to it for the time being. > > Best, > Jo Ask from me permission when you have things moving forward, and I'll very likely give permission to all my post-Intel contributions, which are not that many. PS. You can quite easily get full authors list with some git magic so pretty easy to keep things legal wise in shape. BR, Jarkko
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]. 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 [2,3]. 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 [4]. 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" [5]. 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://jovanbulck.github.io/files/ccs19-tale.pdf [3] https://jovanbulck.github.io/files/systex22-abi.pdf [4] https://jovanbulck.github.io/files/acsac20-fpu.pdf [5] https://patchwork.kernel.org/comment/23216515/ 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 | 55 ++++++ tools/testing/selftests/sgx/test_encl.c | 165 +++++++++++++----- tools/testing/selftests/sgx/test_encl.lds | 1 + .../selftests/sgx/test_encl_bootstrap.S | 29 +++ 6 files changed, 214 insertions(+), 41 deletions(-) base-commit: 1a2945f27157825a561be7840023e3664111ab2f