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

Feature for using CPU-based RNG #84

Closed
josephlr opened this issue Aug 13, 2019 · 11 comments
Closed

Feature for using CPU-based RNG #84

josephlr opened this issue Aug 13, 2019 · 11 comments
Milestone

Comments

@josephlr
Copy link
Member

For some custom x86 targets I am working on (https:/intel/rust-hypervisor-firmware), it would be nice to have a feature that says "just use the on-CPU RNG".

Having a feature for this seems better than adding more targets to the list of targets that use RDRAND. It would also be useful more conventional targets (i.e. Linux) that don't want to use a syscall to get random data (i.e. for performance).

@newpavlov
Copy link
Member

newpavlov commented Aug 13, 2019

I am not sure about this. I think it falls under using a custom getrandom implementation, so right now such projects should use [patch] or [replace], and hopefully eventually we will get a lang item for it.

Do you suggest using a user-space CSPRNG? What do you plan to use for the initial seed? Either way, getrandom conception is a minimal crate for retrieving system entropy, and I don't think that user-space RNGs fit here.

don't want to use a syscall to get random data (i.e. for performance)

I think if you want performance you should use ThreadRng, not overwrite getrandom.

@josephlr
Copy link
Member Author

I am not sure about this. I think it falls under using a custom getrandom implementation, so right now such projects should use [patch] or [replace], and hopefully eventually we will get a lang item for it.

So this is totally possible, but it requires a patch to the cfg_if statement in lib.rs. Essentially, if my target is x86_64-unknown-none, I have to patch getrandom to use the implementation in rdrand.rs. A lang-item also wouldn't work, because I wouldn't want to substitute my own custom implementation. I would just want getrandom to use a different, already existing implementation.

That's why I think a feature might make more sense.

Do you suggest using a user-space CSPRNG? What do you plan to use for the initial seed? Either way, getrandom conception is a minimal crate for retrieving system entropy, and I don't think that user-space RNGs fit here.

Oh I agree, getrandom should only be about getting the entropy from some target-specific source (either the OS or the CPU).

I think if you want performance you should use ThreadRng, not overwrite getrandom.

Correct, ThreadRng is way better and (IIRC) more performant. My point about Linux was wrong.

@newpavlov
Copy link
Member

A lang-item also wouldn't work, because I wouldn't want to substitute my own custom implementation.

As I see it we would have a getrandom-rdrand crate which will contain only RDRAND implementation, so you will not have to write custom implementation yourself.

I guess we can add this feature until we get this hypothetical lang item. Can you prepare a PR?

@dhardy
Copy link
Member

dhardy commented Aug 13, 2019

As I see it we would have a getrandom-rdrand crate which will contain only RDRAND implementation, so you will not have to write custom implementation yourself.

I guess this makes sense. Then users can use something like following:

getrandom = { version = "0.1", package = "getrandom-rdrand" }

I don't see how this has anything to do with lang items, and I'm less keen on using feature flags (because it makes it too easy to replace a trusted entropy source with another, potentially less trusted one — the AMD CPU bugs have shown that trusting RDRAND is not always a good idea).

@newpavlov
Copy link
Member

newpavlov commented Aug 29, 2019

I'm less keen on using feature flags because it makes it too easy to replace a trusted entropy source with another, potentially less trusted one

I think if we are to introduce such feature it should be at the very end of our cfg_if, meaning it will be impossible to overwrite RNG for example for x86_64-unknown-linux-gnu. But users compiling programs for SGX, UEFI, etc., will be able to simply turn this feature on and use RDRAND instead of the "standard" source which is not really present (SGX) or shouldn't/can't be used (Hermit, UEFI). In other words we will add an optional support for x86-64 targets which are not supported by default, it will be somewhat similar to what we do with wasm32-unknown-unknown target right now.

@josephlr
Will such approach work in your case?

@josephlr
Copy link
Member Author

@newpavlov all of that sounds very reasonable. It also might be a good idea to use a similar mechanism for both custom randomness and CPU randomness. I can put something together and see how it looks.

@dhardy
Copy link
Member

dhardy commented Feb 17, 2020

Review of deployment methods

I think it falls under using a custom getrandom implementation, so right now such projects should use [patch] or [replace], and hopefully eventually we will get a lang item for it.

Ignoring the "lang item", getrandom now supports custom implementations (#109). This is a great option where target-specific drivers are required, since the code (which has little re-use value) can live in another crate. It also avoids weakening security by only acting as a backup option.

Use via [patch] or [replace] allows users to directly override OS sources, potentially weakening security — but we can never stop users doing that. In general I don't think we should recommend this approach (since users may end up weakening security unnecessarily or even breaking things via misconfiguration), but it will always be an option available to users.

getrandom = { version = "0.1", package = "getrandom-rdrand" }

This is essentially [patch] but limited to affecting the current crate, so not really a solution (e.g. for direct users of rand).

it would be nice to have a feature that says "just use the on-CPU RNG"

Feature-flags have the same usage implications as custom implementations via #109, but keep code and dependencies within the same crate. If we need RDRAND code within the getrandom crate itself anyway, then this may be the better option?

Actually, there is one additional possibility: providing another level of fallback for sources (OS if available, otherwise RDRAND if enabled and functional, otherwise custom backend).


Summary: #109 is not necessarily better than a feature flag for this use-case.

@josephlr
Copy link
Member Author

Use via [patch] or [replace] allows users to directly override OS sources, potentially weakening security — but we can never stop users doing that. In general I don't think we should recommend this approach (since users may end up weakening security unnecessarily or even breaking things via misconfiguration), but it will always be an option available to users.

Agreed, this is essentially the worst option as it puts much more burden on users who have getrandom as a transitive dependency (i.e. most users).

Feature-flags have the same usage implications as custom implementations via #109, but keep code and dependencies within the same crate. If we need RDRAND code within the getrandom crate itself anyway, then this may be the better option?

Actually, there is one additional possibility: providing another level of fallback for sources (OS if available, otherwise RDRAND if enabled and functional, otherwise custom backend).

After trying multiple options locally, I think an extension of the fallback mechanism is the best bet. My idea would be for getrandom to do the following:

  • If the target is supported, always use the "official" implementation.
  • If the (off by default) custom feature flag is on, use a custom implementation defined in a different crate.
  • If the (on by default) cpu feature flag is on, use the CPU-based randomness (i.e. RDRAND) defined in the main getrandom crate.
  • Fail to compile

This fallback ordering means that a custom backend can always be used, but that we can also provide a sane fallback for x86 targets without having to deal with additional custom backends. I think using custom instead of cpu when both are present it the best bet. This way, users can override an unsupported x86 targets to use something more specific to a given OS.

Other benefits:

@dhardy @newpavlov What do you think? I'll revise #133 to incorporate this plan.

@dhardy
Copy link
Member

dhardy commented Feb 18, 2020

Sounds good to me.

@newpavlov
Copy link
Member

Sounds good to me as well!

@josephlr
Copy link
Member Author

Closed via #133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants