Added sgx feature for rand with no-std
#764
Conversation
|
I tired to understand the |
| use crate::error; | ||
|
|
||
| pub fn fill(dest: &mut [u8]) -> Result<(), error::Unspecified> { | ||
| trts::rsgx_read_rand(dest).map_err(|_| error::Unspecified) |
There was a problem hiding this comment.
Is this really a good idea? I looked at the source code for sgx_raed_rand in the Intel SGX SDK. It does feature detection using CPUID to try to detect whether RDRAND is available, and falls back to rand() if feature detection says RDRAND isn't available. But, feature detection like that isn't secure in SGX because the CPUID has to be executed outside the enclave.
In #738, @akash-fortanix and @jethrogb suggested to just hard-code the use of RDRAND, i.e. require RDRAND be available within SGX, since in practice it seems like all SGX-capable CPUs include RDRAND, despite Intel's warnings. That seems more secure to me. What do you think?
Importantly, I'd rather avoid having two different implementations for this for the two different SGX targets.
There was a problem hiding this comment.
It sounds like you're saying that Intel's secure randomness isn't really secure.
Assembly is out of my yard.
@dingelish What do you think?
There was a problem hiding this comment.
Hi Brian, as far as I know, RDRAND is always enabled in CPU supporting SGX. The feature detection is only enabled in simulation mode and the fallback only works on sim+oldcpu (no rdrand) combinations.
#ifndef SE_SIM
/* We expect the CPU has RDRAND support for HW mode. Otherwise, an exception will be thrown
* do_rdrand() will try to call RDRAND for 10 times
*/
if(0 == do_rdrand(rand_num))
return SGX_ERROR_UNEXPECTED;
#else
I don't know if you are considering to support the simulation mode of SGX. I would like to recommend to just hard code it for simplification. I would like to say that users needs to add additional stuffs to claim that this is a simulation mode (currently it is defined by env SGX_MODE=SW).
There was a problem hiding this comment.
BoringSSL already has the assembly language code that ring can import, so don't worry about writing assembly language code for this.
There was a problem hiding this comment.
It sounds like you're saying that Intel's secure randomness isn't really secure.
Assembly is out of my yard.
@dingelish What do you think?
I've not dug into the randomness of RDRAND much. What I know is that people choose to trust it or not.
Technically the randomness of RDRAND seems good enough.
Update: a proof of RDRAND 'bug' caused by bad impl of urandom.
https://cr.yp.to/talks/2014.01.09/slides-dan+tanja-20140109-4x3.pdf
There was a problem hiding this comment.
@dingelish Whether or not RDRAND is secure, both of these approaches ultimately use RDRAND, right? That is, the sgx_read_rand function uses RDRAND under the hood, right? The main questoin is whether it is better to use the sgx_read_rand wrapper or whether it's better to just directly use RDRAND, right?
I have to say, I don't understand the Intel SDK source code at all. My concern was already raised as a bug at intel/linux-sgx#111. It is confusing to have two different implementations of the function (if I understand correctly): the one you pointed me to and the one at https://github.com/intel/linux-sgx/blob/master/common/src/sgx_read_rand.cpp.
This makes me lean in favor of just using RDRAND directly for all SGX targets. WDYT?
There was a problem hiding this comment.
Yes everything will ultimately use RDRAND. I believe in foreseeable future this is the only randomness source in SGX.
You can forget about sgx_read_rand, a function exported by Intel SGX SDK, because ring does not need to depends on Intel SGX SDK. Simply wrap RDRAND or just use intrinsic functions and that'll be 100% fine for SGX enclave.
Yes. There are two implementations of sgx_read_rand. One is for the untrusted side, and another is for the trusted side (SGX Enclave). The code snipped he referred to is used in the untrusted side.
There was a problem hiding this comment.
@dingelish Thanks for the explanation. I agree that we should just more directly use RDRAND and avoid sgx_read_rand.
|
@briansmith Update me when you decide how you want to do this. |
|
@briansmith Hi, Any updates on this? Thanks! |
|
Another problem is the in (here's what's being called when you use Intel's sdk |
|
Hi, based on the discussion in this issue and elsewhere, I created Issue #775 to track what needs to be done to support SGX, which seems to be a synthesis of what Baidu has done and what Fortanix has done. I think we should discuss how to get the SGX port working in that issue, instead of in the individual Baidu and Fortanix PRs. My understanding is that Baidu already updated their SGX SDK so that the new feature flag being proposed here isn't necessary, right? I'm going to close this specific PR because I think there are some details here that should be changed, but I do appreciate the PR and I think everything that's proposed here will happen, perhaps just slightly differently. |
Hi, I decided to implement sgx support to ring according to #744
This should work after this is merged: #759
(you can run
cargo build --no-default-features --features sgx)I added
min_const_fnbecause rust-sgx right now supports only up tonightly-2018-10-01, and at that version this is still an unstable feature.@briansmith I would love a feedback :)