It should be possible to use a custom implementation when RDRAND is available

[briansmith]

See

getrandom/src/lib.rs

Lines 208 to 217 in 30308ae

	} else if #[cfg(all(target_arch = "x86_64", target_env = "sgx"))] {
	#[path = "rdrand.rs"] mod imp;
	} else if #[cfg(all(feature = "rdrand",
	any(target_arch = "x86_64", target_arch = "x86")))] {
	#[path = "rdrand.rs"] mod imp;
	} else if #[cfg(all(feature = "js",
	target_arch = "wasm32", target_os = "unknown"))] {
	#[path = "js.rs"] mod imp;
	} else if #[cfg(feature = "custom")] {
	use custom as imp;

:

Presently, the RDRAND implementation supersedes the custom implementation, and there's no way to prevent that for SGX or when the rdrand feature is explicitly set. Further, there's no way to use RDRAND through this crate from a custom implementation.

More generally, in any target we should be able to avoid using RDRAND output directly. For example, in an SGX and/or UEFI or operating system kernel, one may want to implement their own RNG, perhaps one that conforms to external security standards (FIPS, NIST, etc.), perhaps using RDRAND as one entropy source. Or, the implementation may want to mix the RDRAND output into a DRBG (using NIST terminology) to protect against unknown issues with RDRAND.

And, I think we getrandom even avoid defaulting to RDRAND on any target.

In UEFI and SGX, I would like to force the use of my custom implementation, and I'd like to implement mine partially in terms of RDRAND.

Concretely, I suggest:

1. Stop defaulting to the RDRAND implementation on SGX targets.
2. When the rdrand feature is requested, expose a new API to get the (raw, unbiased) results of RDRAND, independent of the current getrandom() API.
3. When the rdrand and custom features are requested, provide an API for registering the RDRAND implementation as the custom implementation, and document that environments such as UEFI and SGX must do this registration before using getrandom, just like any custom implementation.

[briansmith]

The thing that's special about SGX isn't the target_vendor, but all(target_arch = "x86_64", any(target_os = "none", target_os="unknown"), target_feature = "rdrand"). So maybe we can leave the vendor and target vendor out of the consideration when considering whether to use RDRAND.

Also, the only thing special about SGX is that we have to know that we cannot invoke CPUID. It doesn't matter though because it already has target_feature = "rdrand" so we never need to invoke CPUID on it anyway. Thus we really don't need any SGX-specific configuration.

IMO, most any(target_os = "none", target_os="unknown") environments really need to make their own real CSPRNG that mixes several sources of entropy, instead of just relying on RDRAND. In this sense, it isn't much different than the situation with ESPIDF, where nobody should be using the implementation getrandom currently provides, but we provide it to give them something that "works" so they can port things.

With this in mind, I don't think we need to have RDRAND (or the ESPIDF equivalent) by default for ANY target. I think instead we could require all these targets to register a custom implementation. Somebody could make a getrandom-rdrand crate that registers a custom implementation that uses RDRAND, which we could discourage people from using but also tell them how to use it. Then they can, in their Cargo.toml, choose the targets for which they want that. And rand or other crates can decide on their own what they want to do. rand has its own ChaCha CSPRNG already, so presumably it would use that.

In other words, getrandom should stop defaulting to--or even providing--bad implementations when it doesn't have an OS implementation to use.

[josephlr]

Copying from #438

I would propose fixing #230 by simply having the custom feature take precedence over the rdrand feature. This would allow users who are fine with the CSPRNG of rdrand to simply specify the rdrand feature, while users who want more complexity can use the custom feature to provide their own implementation. If necessary, we can also have an x86-only fn rdrand_fill(&mut [u8]) -> Result<(), Error> or fn rdrand() -> Result<[u8; 8], Error> helper function to make such implementations easier.

For SGX targets we currently unconditionally use the rdrand implementation. This is probably not ideal, given that target_env = "sgx" may not mean the same thing across different targets. I would make the following changes:

• Only use the rdrand implemenation by default on x86_64-fortanix-unknown-sgx. Other unsupported targets will need to explicitly set rdrand.
• Even on x86_64-fortanix-unknown-sgx, have custom take precedence over rdrand, ensuring that our rdrand implementation always interacts with custom in a consistent way.
• Effectively, this just makes the rdrand feature on by default for x86_64-fortanix-unknown-sgx which seems ideal.

[briansmith]

I would propose fixing #230 by simply having the custom feature take precedence over the rdrand feature.

Currently, --features=custom,rdrand means "use rdrand on x86-64 when there's no system getrandom, use my custom implementation on other targets." So in theory your proposal could be a breaking change, if they didn't define custom implementation when target_arch="x86_64" (e.g. they use cfg_if! much like we do).

Perhaps we should create a new custom feature, "custom2", with a better API, and with "overrides features=custom,rdrand" semantics. Then we can issue a deprecated warning when #[cfg_attr(all(feature = "custom", feature = "custom2"), deprecated(note = "The original 'custom' feature is deprecated and ignored when the 'custom2' feature is enabled, and will be deprecated unconditionally in the future.)).

Similarly, when rdrand and custom2 are both enabled at the same time, we should warn that the rdrand feature is deprecated and that the custom2 implementation will be used.

fn rdrand_fill(&mut [u8]) -> Result<(), Error>

Worst-case scenerio, they can use the RDRAND crate or do it themselves using intrinsics or inline assembly. I do think it is a good idea to support an API for RDRAND, but we should design it separately.

Eventually, maybe a year or two from now, when either the original custom or rdrand features are used, we should warn that they are deprecated.