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.

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

Add RNG for Fortanix SGX #883

Closed
wants to merge 1 commit into from

Conversation

@npmccallum
Copy link

commented Aug 1, 2019

Currently, ring doesn't provide a random number generator for SGX at all.
This is probably by accident, but is, in fact, the correct behavior.

Due to distrust of the OS, SGX enclave applications need a source of
randomness that is not forgable by the OS. This pretty much leaves only
the RDRAND instruction. Fortunately, LLVM provides an intrinsic for this
instruction.

Compiling to the Fortanix SGX target currently requires Rust nightly.
The implementation in this patch takes advantage of this to call the
LLVM intrinsic directly; a Rust feature that will never be stabilized.
At some point in the future, the Fortanix SGX target may be stabilized
and then we will need a way to invoke RDRAND wihout nightly Rust.

@josephlr

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

This PR could be greatly simplified by just using the implementation from getrandom, just without the CPUID detection (as you're on SGX) and without the AMD RDRAND bug detection.

See the implementation: https://github.com/rust-random/getrandom/blob/master/src/rdrand.rs

This implementation also fixes two issues present in this PR:

@npmccallum

This comment has been minimized.

Copy link
Author

commented Aug 1, 2019

@josephlr Given the desire for a small TCB inside an SGX enclave, I'm in favor of reducing the number of crates required. Given that Rust already has a stable RDRAND intrinsic, I'd rather use that directly and fix the loop issue. Does that work for you?

Add RNG for Fortanix SGX
Currently, ring doesn't provide a random number generator for SGX at all.
This is probably by accident, but is, in fact, the correct behavior.

Due to distrust of the OS, SGX enclave applications need a source of
randomness that is not forgable by the OS. This pretty much leaves only
the RDRAND instruction. Fortunately, Rust provides an intrinsic for this
instruction.

@npmccallum npmccallum force-pushed the npmccallum:master branch from 8565bfc to e3cfc2a Aug 1, 2019

@npmccallum

This comment has been minimized.

Copy link
Author

commented Aug 1, 2019

@josephlr The latest push uses the stable RDRAND intrinsic in a tight loop.

@josephlr

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

@josephlr Given the desire for a small TCB inside an SGX enclave, I'm in favor of reducing the number of crates required. Given that Rust already has a stable RDRAND intrinsic, I'd rather use that directly and fix the loop issue. Does that work for you?

Oh ya, I should have clarified what I meant. Just use the getrandom implementation as a guide for this PR. We don't want ring to use getrandom for this (that's a bigger decision for a later time).

@npmccallum

This comment has been minimized.

Copy link
Author

commented Aug 1, 2019

It looks like the only test failure was a spurious one.

Regarding the test coverage decrease, I'm not sure how to test this.

@lkatalin Do you know how to write tests that run inside SGX?

@lkatalin

This comment has been minimized.

Copy link

commented Aug 1, 2019

@npmccallum I could certainly give it a try.

// Section 5.2.1
for _ in 0..10 {
let mut r = 0;
if unsafe { _rdrand64_step(&mut r) } == 1 {

This comment has been minimized.

Copy link
@josephlr

josephlr Aug 1, 2019

Contributor

Explain why this is safe (i.e. that SGX always supports RDRAND).

}

pub fn fill(mut dest: &mut [u8]) -> Result<(), error::Unspecified> {
while dest.len() > 0 {

This comment has been minimized.

Copy link
@josephlr

josephlr Aug 1, 2019

Contributor

This can me made more clear (and more efficient by eliding memcpy calls) by using chunks_exact_mut, like the getrandom implementation does.

See the comparison here: https://rust.godbolt.org/z/gwprxd

use core::arch::x86_64::_rdrand64_step;
use crate::error;

#[inline]

This comment has been minimized.

Copy link
@josephlr

josephlr Aug 1, 2019

Contributor

#[inline] does nothing on a private function, the attribute is only useful for pub functions/methods.

use crate::error;

#[inline]
fn rdrand() -> Result<u64, error::Unspecified> {

This comment has been minimized.

Copy link
@josephlr

josephlr Aug 1, 2019

Contributor

If you end up having multiple call-sites for this function (i.e. if you make the chunks_exact_mut changes), it might be worth having this function return a [u8; 8] instead of a u64.

@josephlr

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Regarding the test coverage decrease, I'm not sure how to test this.

If you just want to check that the RDRAND implementation is good, you could test this on a normal x86_64 machine (without SGX) and just do the CPUID check.

@briansmith

This comment has been minimized.

Copy link
Owner

commented Aug 1, 2019

Just use the getrandom implementation as a guide for this PR.

Do not do this, because the license is not compatible.

I will probably implement this myself so I don't have to worry about the license implications.

Have somebody with spending authority email me: brian@briansmith.org if/when you want to see progress on this.

@npmccallum

This comment has been minimized.

Copy link
Author

commented Aug 1, 2019

@briansmith I did not use getrandom as a guide. I never even looked at it. I only looked at the Intel documentation. Please tell me that ring is not pay-to-merge.

@josephlr

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Just use the getrandom implementation as a guide for this PR.

Do not do this, because the license is not compatible.

This is incorrect, getrandom is dual licensed under Apache 2.0 and MIT, which is compatible with Ring's ISC style license. If significant code is copied (which isn't necessary for this PR), then the copyright notice would need to be included in the file.

@briansmith briansmith closed this Aug 2, 2019

@briansmith

This comment has been minimized.

Copy link
Owner

commented Aug 2, 2019

There's already a PR for this.

@josephlr

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

There's already a PR for this.

#738 I'm assuming

@briansmith

This comment has been minimized.

Copy link
Owner

commented Aug 2, 2019

Please tell me that ring is not pay-to-merge.

I have a small number of days per month I work on stuff that isn't paid for. This has resulted in a backlog, unfortunately. Three are a variety of factors I use to prioritize the backlog. The #1 thing somebody can do to get items on the backlog prioritized higher is sponsor this project financially. That's not the only criteria, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.