SGX is_enclave_range/is_user_range overflow checking

[okready]

Currently, the is_enclave_range and is_user_range functions in sgx::os::fortanix_sgx::mem do not perform overflow checks for input memory ranges. While debug builds will panic if an overflow occurs, release builds perform no such checking, which can lead to false positive results from either function for overflowing memory ranges.

One of the typical uses for these functions is to validate memory ranges passed into the enclave from untrusted code. Compromised untrusted code can intentionally pass overflowing ranges to enclave code, and without proper overflow checking, these false positives can potentially result in security vulnerabilities such as leakage of secret data or overwriting of enclave data (including code) with data from the untrusted application layer.

While applications can be left responsible for overflow checking, it is another step that application authors have to consider that is easy to overlook. Additionally, the corresponding sgx_is_within_enclave and sgx_is_outside_enclave functions from the Intel SGX SDK handle overflow checks, so developers migrating from the Intel SGX SDK to x86_64-fortanix-unknown-sgx or referring to existing Intel SGX SDK-based code may not expect overflow handling to be omitted from x86_64-fortanix-unknown-sgx.

Adding overflow checks to is_enclave_range and is_user_range should be relatively low-impact. Breakage of existing code is not expected; ranges that do not trigger overflow with the current implementations should have the same results, while overflowing ranges are more than likely already yielding unexpected behavior and exposing possible security vulnerabilities as described and would benefit from the fix (unless the application is already checking for overflow manually, in which case such checks will simply become redundant). Bringing feature parity with the Intel SGX SDK versions of these functions will also avoid unintentional mistakes from switching over.

[jethrogb]

While applications can be left responsible for overflow checking, it is another step that application authors have to consider that is easy to overlook. Additionally, the corresponding sgx_is_within_enclave and sgx_is_outside_enclave functions from the Intel SGX SDK handle overflow checks, so developers migrating from the Intel SGX SDK to x86_64-fortanix-unknown-sgx or referring to existing Intel SGX SDK-based code may not expect overflow handling to be omitted from x86_64-fortanix-unknown-sgx.

I agree that having application authors having to think about integer overflow is not desirable. But that's just a small part of the issue. The EDL interface description that the Intel SDK uses is simply too low level for general use. In the EDP architecture you should define your interface in terms of byte streams, see https://edp.fortanix.com/docs/concepts/architecture/ . Byte streams are a ubiquitous interface for dealing with untrusted input (e.g. network services) and parsing byte streams in Rust is safer than ever before. See also my talk on the subject at FOSDEM earlier this year https://archive.fosdem.org/2020/schedule/event/rustsgx/ .

In order to write an EDP application, you shouldn't be using any Rust feature gates. When you don't, these kind of pitfalls are eliminated. As such, is_enclave_range and is_user_range are not intended to be used by application code. In fact, applications are not expected to have to deal with user memory pointers at all. Even if you're somehow dealing with user memory pointers, you should use the types from https://edp.fortanix.com/docs/api/std/os/fortanix_sgx/usercalls/alloc/index.html. Those types do check for p + len overflow and is_user_range is used internally. But again, you shouldn't be dealing with pointers anyway.

My proposed courses of action are:

1. Do nothing.
2. Improve the documentation of is_enclave_range and is_user_range and/or move them behind a different feature gate.
3. Remove is_enclave_range and is_user_range from the public API.

[okready]

I agree, sticking exclusively with streams for passing data between the enclave and user space is safest, but there still may be a point where one would want to pass a buffer by reference to the enclave instead of copying it over the stream as an optimization (keeping in mind the possible pitfalls when doing so), at which point platform-specific features would be needed. Wrapping all user-space ranges with UserRef seems like it would be sufficient, and panicking is probably fine for most users (having the option for non-panicking checks would give more flexibility, but I don't have any strictly compelling examples myself of where it would be particularly crucial).

That said, what's the downside with putting overflow checking in is_enclave_range and is_user_range? The only difference in the compiled code is a single instruction to branch on overflow after computing start + len (the other alterations in #76345 to account for whether p + len is exactly at the end of the address space not included), so the cost difference is negligible.