Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added sgx feature for rand with no-std #764

Closed
wants to merge 2 commits into from
Closed

Added sgx feature for rand with no-std #764

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6669766
Added rust-sgx feature for rand
elichai Jan 17, 2019
9bd37dd
Remove unstable features
elichai Jan 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,10 @@ libc = { version = "0.2.45" }
[target.'cfg(any(target_os = "redox", all(unix, not(any(target_os = "macos", target_os = "ios")))))'.dependencies]
lazy_static = "1.2"


[target.'cfg(not(target_env = "sgx"))'.dependencies]
sgx_trts = { version = "1.0.4", optional = true }

# Keep this in sync with `[dependencies]` in pregenerate_asm/Cargo.toml.
[build-dependencies]
cc = "1.0.26"
Expand All @@ -315,6 +319,7 @@ internal_benches = []
slow_tests = []
test_logging = []
use_heap = []
sgx = ["sgx_trts"]

# XXX: debug = false because of https://github.com/rust-lang/rust/issues/34122

Expand Down
2 changes: 2 additions & 0 deletions src/aead.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
//! [AEAD]: http://www-cse.ucsd.edu/~mihir/papers/oem.html
//! [`crypto.cipher.AEAD`]: https://golang.org/pkg/crypto/cipher/#AEAD



use self::block::{Block, BLOCK_LEN};
use crate::{
constant_time, cpu, error,
Expand Down
1 change: 0 additions & 1 deletion src/cpu.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
// WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
// OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
// CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

#[inline(always)]
pub fn cache_detected_features() {
#[cfg(not(target_os = "ios"))]
Expand Down
6 changes: 5 additions & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,18 @@
not(feature = "use_heap"),
unix,
not(any(target_os = "macos", target_os = "ios")),
any(not(target_os = "linux"), feature = "dev_urandom_fallback")
any(not(target_os = "linux"), feature = "dev_urandom_fallback", feature = "sgx")
)
),
no_std
)]

#![cfg_attr(feature = "internal_benches", allow(unstable_features))]
#![cfg_attr(feature = "internal_benches", feature(test))]

#![cfg_attr(feature = "sgx", allow(unstable_features))] // This is because rust-sgx cuurently use nightly-2018-10-01-x86_64-unknown-linux-gnu
#![cfg_attr(feature = "sgx", feature(min_const_fn))] // In that version `min_const_fn` is still unstable

elichai marked this conversation as resolved.
Show resolved Hide resolved
#[macro_use]
mod debug;

Expand Down
1 change: 1 addition & 0 deletions src/polyfill.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
//! Polyfills for functionality that will (hopefully) be added to Rust's
//! standard library soon.


use core;

#[macro_use]
Expand Down
28 changes: 25 additions & 3 deletions src/rand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,19 +100,27 @@ impl sealed::Sealed for SystemRandom {}
use self::urandom::fill as fill_impl;

#[cfg(any(
all(target_os = "linux", not(feature = "dev_urandom_fallback")),
all(target_os = "linux", not(any(feature = "dev_urandom_fallback", feature = "sgx"))),
windows
))]
use self::sysrand::fill as fill_impl;

#[cfg(all(target_os = "linux", feature = "dev_urandom_fallback"))]
use self::sysrand_or_urandom::fill as fill_impl;


#[cfg(all(
target_os = "linux",
feature = "sgx",
not(any(feature = "dev_urandom_fallback", feature = "use_heap"))))]
use self::sgxrand::fill as fill_impl;


#[cfg(any(target_os = "macos", target_os = "ios"))]
use self::darwin::fill as fill_impl;
use crate::sealed;

#[cfg(target_os = "linux")]
#[cfg(all(target_os = "linux", not(feature = "sgx")))]
mod sysrand_chunk {
use crate::{c, error};
use libc;
Expand Down Expand Up @@ -168,7 +176,7 @@ mod sysrand_chunk {
}
}

#[cfg(any(target_os = "linux", windows))]
#[cfg(all(any(target_os = "linux", windows), not(feature = "sgx")))]
mod sysrand {
use super::sysrand_chunk::chunk;
use crate::error;
Expand Down Expand Up @@ -248,6 +256,20 @@ mod sysrand_or_urandom {
}
}

#[cfg(all(
target_os = "linux",
feature = "sgx",
not(any(feature = "dev_urandom_fallback", feature = "use_heap"))))]
mod sgxrand {
use sgx_trts::trts;
use crate::error;

pub fn fill(dest: &mut [u8]) -> Result<(), error::Unspecified> {
trts::rsgx_read_rand(dest).map_err(|_| error::Unspecified)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BoringSSL already has the assembly language code that ring can import, so don't worry about writing assembly language code for this.

Copy link

@dingelish dingelish Jan 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

@briansmith briansmith Jan 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dingelish Thanks for the explanation. I agree that we should just more directly use RDRAND and avoid sgx_read_rand.

}

}

#[cfg(any(target_os = "macos", target_os = "ios"))]
mod darwin {
use crate::{c, error};
Expand Down