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
-- 
2.34.1

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 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

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