Port to SGX

[briansmith]

Various people want ring to work on SGX. Here's what needs to be done to have a decent port, AFAICT:

• Replace the use of CPUID in cpu-intel.c with something SGX-compatible for SGX targets. Baidu patched ring to use sgx_cpuid.
• Change detect_implementation() in aes.rs so that SGX targets will only use the constant-time AES-NI or constant-time VPAES implementations, and never any of the other leaky implementations.
• Change detect_implementation() in gcm.rs so that SGX will only use the constant-time CLMUL implementation or the ghash-ssse3-x86_64.pl implementation (which hasn't been imported from BoringSSL yet), and never any of the other leaky implementations.
• For SGX targets only, change rand.rs for SGX targets to use RDRAND via the BoringSSL rdrand-x86_64.pl. I filed https://bugs.chromium.org/p/boringssl/issues/detail?id=262 to ask the BoringSSL developers about the lack of retry logic in that code. Document clearly in the ring:rand documentation that on SGX only, RDRAND is used.
• Remove use of fprintf from the constant_time_test.c.
• Merge Replace libstd with spin crate in cpu::cache_detected_features. #759: Use spin::Once instead of std::sync::Once in cpu.rs. (Needed by Baidu only, not Fortanix).
• Update Travis CI to build SGX test target. Take this from Add support for x86_64-fortanix-unknown-sgx target #738.
• Create a new document for SGX-specific issues:
  • Document SGX-specific risks with how feature detection is done. In particular, document that because the CPU feature detection is done outside of the enclave, it can be tampered with, and because we rely on the results of this feature detection, it is possible for untrusted code outside of the enclave to trick us into thinking that a CPU instruction is available when it is not, and thus they can force us to halt the enclave with an illegal instruction fault.
  • Warn in this document, in addition to the ring::rand API documentation, that ring::rand is implemented solely in terms of RDRAND for SGX targets.
  • Document that SGX tests aren't run in CI. File a new issue in the issue tracker and link to the issue in this section of this new SGX-specific documentation.
  • Document that the SGX port is tested to build correctly only in Nightly Rust. File a new issue in the issue tracker about adding stable and beta channel support, and link to the issue in this section of this new SGX-specific documentation.

Is there anything else that needs to be done to properly support SGX?

[briansmith]

/cc @jethrogb @akash-fortanix @elichai @dingelish.

[briansmith]

@elichai @dingelish Would there be any way for us to add build ring and its tests for your SGX target, even if we can't run them in CI, similar to what Fortanix proposes to do for their target in #738? I would love for CI to at least verify that the code build successfully for your target.

[briansmith]

Also, I would love it if somebody could explain what the difference is between these two targets besides the fact that the fortanix target implements most of libstd. I heard that the Baidu toolchain has better support for Intel's toolchain and maybe some other aspects of SGX that Intel supports, whereas Fortanix's toolchain replaces some of what Intel does with Fortanix's own toolchain. But, what is the practical effect of the use of Intel vs Fortanix toolchains, as far as somebody who wants to distribute an SGX-based application is concerned?

[dingelish]

Hey Brian, thanks for your support!

I can provide an SGX enclave which runs all your test cases in tests dir. The enclave will return non-zero or trigger a signal on error. The environment setup would be done by a Dockerfile and I'll provide it on docker hub as well. I'm not familiar with CI but I think I can try with it and provide you a CI script :-)

Basically, I would say that the fundamental difference between these two impls are whether or not trust Intel's software stack. Baidu's implementation does depend on Intel's pre-built static library while Fortanix's does not. One example is Google's Asylo. Google does not 100% trust Intel and their Asylo includes a large patch on Intel SGX SDK. Enigma uses Baidu's approach and I believe the authors understand that the software stack is built on top of Intel's SDK. I think people have different understandings regard to their relationship with Intel as well as the ownership of liability. Intel has the liability to fix vulnerabilities in their SDK libraries such as the most recent fix to INTEL-SA-00202. And I believe Fortanix will fix similar bugs (if exists) on their side.

It's good to see that we have different choices :-)

[jethrogb]

Change detect_implementation() in gcm.rs so that SGX will only use the constant-time CLMUL implementation or the ghash-ssse3-x86_64.pl implementation (which hasn't been imported from BoringSSL yet), and never any of the other leaky implementations.

As mentioned in #738, I'd want this to use std::is_x86_feature_detected on x86_64-fortanix-unknown-sgx. I don't think this is compatible with what Baidu wants to do.

document that because the CPU feature detection is done outside of the enclave, it can be tampered with, and because we rely on the results of this feature detection, it is possible for untrusted code outside of the enclave to trick us into thinking that a CPU instruction is available when it is not, and thus they can force us to halt the enclave with an illegal instruction fault

This is only true for Intel's toolchain, I don't intend feature detection to work this way on x86_64-fortanix-unknown-sgx.

[dingelish]

I prefer low-level (or even hard-coded) solution without std.

[elichai]

@jethrogb Now I'm curious, how do you intend to do feature detection without an ocall(so without cpuid instruction)?

[jethrogb]

@elichai Here's one option #738 (comment). Another is to use only compile-time feature flags.

[elichai]

Hmm that's an interesting idea to do it via local attestation.
Although it will be very easy to hide features if a malicious OS want by killing these enclaves (but it won't be able to fake working instructions)

[jethrogb]

Although it will be very easy to hide features if a malicious OS want by killing these enclaves (but it won't be able to fake working instructions)

Require the local attestation at enclave startup. OS has two choices: don't run or proceed with feature detection.

[elichai]

@jethrogb you'll still need to make another attestation after the feature detection. which can be interfered by the OS.

[jethrogb]

Also, I would love it if somebody could explain what the difference is between these two targets

Baidu provides Rust bindings to the Intel SDK. This means you're both buying in to Intel's large C codebase and Intel's model for building enclaves. You have to define unsafe interfaces for ECALLs/OCALLs, which are known to be susceptible to vulnerabilities.

The Fortanix EDP doesn't use any of Intel's code. If my audit is correct, the only C code in a x86_64-fortanix-unknown-sgx binary is libuwind (can be disabled with panic=abort), plus what you put there yourself. (Although you are still relying on the Intel's provisioning & quoting enclaves based on their SDK if you use remote attestation.) Depending on C code partially defeats the purpose of writing your enclave in Rust.

A small ABI has been specifically designed for use with SGX and consists of less than 20 calls. It's designed to avoid common pitfalls in enclave interface design (Iago attacks, struct padding issues, etc.). It's kept small intentionally to aid security audits.

The EDP is integrated with Rust's std and doesn't require you to modify dependencies in order to use them. Also, if you design your enclave application as a microservice, you don't have to write any untrusted code.

NB. The Fortanix EDP website is live now. You can find more detailed documentation at https://edp.fortanix.com

[briansmith]

As mentioned in #738, I'd want this to use std::is_x86_feature_detected on x86_64-fortanix-unknown-sgx. I don't think this is compatible with what Baidu wants to do.

OK, I don't have any objections to that in theory, but I see a practical problem with it: The OpenSSL code assumes that it has access to the entire results of cpuid and xgetbv through the GFp_ia32cap_P global variable. In particular, the assembly language code assumes so. Here is the output of (cd crypto; grep -r GFp_ia32cap_P | grep x86_64 | grep -v "\.extern")

chacha/asm/chacha-x86_64.pl:    mov     GFp_ia32cap_P+4(%rip),%r10
chacha/asm/chacha-x86_64.pl:    shr             \$32,%r10               # GFp_ia32cap_P+8
fipsmodule/aes/asm/aesni-x86_64.pl:     leaq    GFp_ia32cap_P(%rip),%r10
fipsmodule/aes/asm/aesni-x86_64.pl:     leaq    GFp_ia32cap_P(%rip),%r10
fipsmodule/bn/asm/x86_64-mont.pl:       mov     GFp_ia32cap_P+8(%rip),%r11d
fipsmodule/bn/asm/x86_64-mont.pl:       mov     GFp_ia32cap_P+8(%rip),%eax
fipsmodule/bn/asm/x86_64-mont5.pl:      leaq    GFp_ia32cap_P(%rip),%r11
fipsmodule/bn/asm/x86_64-mont5.pl:      leaq    GFp_ia32cap_P(%rip),%r11
fipsmodule/bn/asm/x86_64-mont5.pl:      leaq    GFp_ia32cap_P(%rip),%r11
fipsmodule/ec/asm/p256-x86_64-asm.pl:   leaq    GFp_ia32cap_P(%rip), %rcx
fipsmodule/ec/asm/p256-x86_64-asm.pl:   leaq    GFp_ia32cap_P(%rip), %rcx
fipsmodule/ec/asm/p256-x86_64-asm.pl:   leaq    GFp_ia32cap_P(%rip), %rcx
fipsmodule/ec/asm/p256-x86_64-asm.pl:   leaq    GFp_ia32cap_P(%rip), %rcx
fipsmodule/ec/asm/p256-x86_64-asm.pl:   leaq    GFp_ia32cap_P(%rip), %rax
fipsmodule/ec/asm/p256-x86_64-asm.pl:   leaq    GFp_ia32cap_P(%rip), %rax
fipsmodule/ec/asm/p256-x86_64-asm.pl:   leaq    GFp_ia32cap_P(%rip), %rcx
fipsmodule/ec/asm/p256-x86_64-asm.pl:   leaq    GFp_ia32cap_P(%rip), %rcx
fipsmodule/ec/asm/p256-x86_64-asm.pl:   leaq    GFp_ia32cap_P(%rip), %rcx
fipsmodule/modes/asm/ghash-x86_64.pl:   leaq            GFp_ia32cap_P(%rip),%rax
fipsmodule/sha/asm/sha512-x86_64.pl:    leaq    GFp_ia32cap_P(%rip),%r11
poly1305/asm/poly1305-x86_64.pl:        mov     GFp_ia32cap_P+4(%rip),%r9

So how are you planning to make this work through std::is_x86_feature_detected?

My main concern is that I don't want to add support for SGX targets at all until we have the actual code that avoids using the side-channel-leaky AES and GCM implementations in SGX. My secondary concern is that, whatever we land first, if there are limitations or the implementation is incomplete or suboptimal, that suboptimality needs to be clearly documented.

[briansmith]

@elichai Could you (or somebody from your team) contribute a PR that replaces the use of CPUID with sgx_cpuid for your target via an #if defined(...). I'm not sure what condition to use to detect SGX though?

Also, if your target is a no_std target, how did you get RSA working? Could you contribute a PR for that too?

[jethrogb]

I filed https://bugs.chromium.org/p/boringssl/issues/detail?id=262 to ask the BoringSSL developers about the lack of retry logic in that code. Document clearly in the ring:rand documentation that on SGX only, RDRAND is used.

@briansmith I don't want to create a Chromium bug tracker account, but I've definitely seen RDRAND fail on bare hardware if you use it a lot. I don't remember the exact hw/sw setup, but it was something like continuous blast in a loop on all cores.

[elichai]

@briansmith I'm not sure either, what I did wouldn't be compatible with fortanix.
AFAIK there's no "feature" to know you run inside of SGX, but maybe @jethrogb @dingelish will correct me.
What I did is to check the SGX_SDK env var, and if it's set to pass a -DSGX to the compiler to be used by the ifdef.

about the RSA, I don't use RSA, can you point me to where it's a problem with no-std? Is it the vec! and ToOwned in https://github.com/briansmith/ring/blob/master/src/rsa/bigint.rs?

[briansmith]

@briansmith I'm not sure either, what I did wouldn't be compatible with fortanix.
AFAIK there's no "feature" to know you run inside of SGX, but maybe @jethrogb @dingelish will correct me.
What I did is to check the SGX_SDK env var, and if it's set to pass a -DSGX to the compiler to be used by the ifdef.

I think a better solution is to modify ring's build.rs so that it looks at TARGET_ENV and TARGET_VENDOR and, when they match your target, add a -DRING_BAUDI_SGX=1 argument to the C compilation. Then in cpu-intel.c you can use #if defined(RING_BAUDI_SGX) to choose between sgx_cpuid and the regular CPUID code.

about the RSA, I don't use RSA, can you point me to where it's a problem with no-std? Is it the vec! and ToOwned in https://github.com/briansmith/ring/blob/master/src/rsa/bigint.rs?

Yes, exactly. Does your team using ring with --no-default-features in SGX?

[elichai]

No, we don't use --no-default-features I guess because the imports are in a local scope and we don't use the RSA it never tries to import the std.

about what to do with it, I'm not too familiar with the insides of RSA implementations but I think the best thing is if it possible to accept a big allocated memory from the outside that will be sufficient for the operations.
(about the ownership I'm not sure what's the best way to do it, I would hope it's possible after meddling with lifetimes)

And AFAIK liballoc does work in sgx(unless baidu got some magic in behind I don't know about), and if @jethrogb liballoc works in your enviroment too I think that can be solved through that.

about the TARGET I don't know of any specific target that is only in SGX, @dingelish what do you say? is there some way to know that the build.rs happens for SGX? (without using Xargo)

[dingelish]

@elichai I think something like -DRING_BAUDI_SGX=1 is good, and we can control the flag in a variety of ways.

[briansmith]

As of ab0726d, ring's tests never do file I/O.

There would still be more work to do to get them to work for Baidu's no_std environment since they still use std::vec::Vec and std::string::String. I think it wouldn't be too hard to work around that. @dingelish @elichai How do you run ring's tests now?

[briansmith]

As of 03e6ef8, 32cf372, 1194b80, bd96baa, ring doesn't #include <stdlib.h> or #include <stdio.h> and doesn't reference fprintf or stderr from C code.

[dingelish]

@briansmith
This enclave tests all test cases of ring. See if this kind of enclave is good for CI. And I'd like to maintain it. Thanks!
https://github.com/dingelish/ring-sgx-test

[elichai]

@briansmith any updates on this?

[briansmith]

Please take a look at PR #869, which removes the usage of libstd in RSA in favor of using the alloc crate instead.

[briansmith]

"In particular, our end-to-end exploit can leak the entire private key of a secure enclave running on a separate CPU core after only a single digital signature operation." - https://www.vusec.net/projects/crosstalk/

[jethrogb]

Microcode updates addressing the issue are available.

[dingelish]

Microcode updates addressing the issue are available.

thanks bro! this link is helpful.
i'd like to see a more completed removal of TSX in all Intel CPUs.

[briansmith]

I think this is substantially easier now. See #744 (comment) and the review I did for UEFI support in #1406.

[briansmith]

@DragonDev1906 wrote in #2043:

The x86_64-fortanix-unknown-sgx target has target_os = "unknown" instead of none for some reason. This means that although ring compiles for this target there is no way to use ring with RDRAND. The following feature flag + line works for embedded devices but not in SGX.

all(feature = "less-safe-getrandom-custom-or-rdrand", target_os = "none"),