-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
ThreadRng / EntropyRng improvements #579
Conversation
Updated: I added a There are a number of things I don't like about this. The code is untidy. We can't actually unimplement I'm also unsure whether we want |
I would appreciate some feedback on this, in particular the design using a
Alternatives with static linkage are (1)
or (2) an external crate
So perhaps using an external crate is the best option? |
src/rngs/entropy.rs
Outdated
/// Is this source available? | ||
/// | ||
/// The default implementation returns `true`. | ||
fn is_available() -> bool { true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we even provide a default implementation? I feel like this should always be implemented explicitly.
src/rngs/entropy.rs
Outdated
@@ -48,6 +54,7 @@ use rngs; | |||
#[derive(Debug)] | |||
pub struct EntropyRng { | |||
source: Source, | |||
_dummy: Rc<()> // enforce !Send |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably cleaner to use core::marker::PhantomData
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PhantomData<Rc<()>>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, probably. I'm not sure that Rc<()>
is a zero-sized type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to not rely on Rc just for !Send, raw pointers are not send: https://play.rust-lang.org/?gist=e350c6d98704bae32e0feb593687bf62&version=stable&mode=debug&edition=2015
Do we have a use case for The other two commits look fine to me. |
Yes, this is for |
Maybe we should get some input from the embedded community to make sure we are addressing their needs? cc @japaric |
I don't have the bandwidth to look into this right now but I'll tell the other WG members to look into this during our next meeting (which is in 5 minutes) |
Can someone sum up what the changes are vs using |
The main point is that Specifically, this PR is about making There are still some other things to do to make |
@dhardy That's a noble goal but let me rephrase my question: With |
Hm, I just had a closer look at the proposal. The
All information between those two must be exchanged using In my opinion it would be more useful (and idiomatic) if the RNG conceptually worked more like a |
let mut entropy_source = EntropyRng::new();
let r = Hc128Core::from_rng(&mut entropy_source).unwrap_or_else(|err| panic!(...)); There are no changes planned for Yes, Do you mean the main loop and interrupt handlers cannot share code? Or that global memory cannot be used to host the implementation? As shown by the When it comes to |
Well, only via functions called from all sides.
Sharing via memory is only possible using synchronisation primitives (e.g. RefCell and Mutex) or
In a nutshell: It's much easier to share something with an interrupt handler that was obtained from a call during initialisation than it is to share something that should ultimately be handled outside of main.
There're no threads in
In a single_threaded world it's not as bad as it sounds. ;) Coming back to my example above: What we'd really like to have here is separation of concerns; some MCUs have really nice TRNGs which can be set up and trigger an interrupt when they have a new nice true random value for us, so an interrupt handler should be able to seed that into the software RNG. On the other hand you might need random values in one or more other interrupt handlers so those should be able to fetch values when they need them. So the best approach would either be: Take an RNG, stuff it in a |
Using entropy when it becomes available rather than when your program/device starts is a challenge... the same one that has led to questionable behaviour in Linux in the past (producing output from What is In that case, there are really only three directions:
Have you thoughts on this? I don't like the idea of being forced into a "best effort" compromise with potential security issues, but I don't know that the other options are viable in general (any kind of persistent storage would have to be handled outside of this lib). That said, my idea for making
If we use something like the spin crate for mutexes, is there some way these can be dropped when single threaded? Or is the overhead likely to be insignificant anyway? |
The spin crate might not be appropriate in some cases. For example, if you are on an interrupt driven paradigm, you can deadlock with the spin crate, but have better alternative as, for example, https://docs.rs/cortex-m/0.5.6/cortex_m/interrupt/struct.Mutex.html See for example https://docs.rs/shared-bus/0.1.2/shared_bus/ |
Great, so there isn't even a good broadly-applicable synchronisation library available? Rand is supposed to be a general-purpose lib, not something targetting ARM Cortex M. |
shared bus propose an approach to this problem: an interface of a mutex that can be implemented by the user, with an implementation using std's mutex. The embedded dev can then use the spin crate or any custom mutex he want. |
A hardware RNG peripheral.
Depends. In this case you can poll the RNG if it has entropy (well, in some cases they're random numbers and not raw entropy) available and it will be somewhat quick, in the milliseconds range. In other cases you might not be so lucky.
Yeah, best effort is horrible. But OTOH shouldn't there be a way to re-seed entropy at a later point? I also don't think blocking is a good idea but you really want to let the user know: "Hey, I can't give you random values because: not enough entropy, you might want seed some", e.g. by returning a
Sounds great to me, but certainly want the option to re-seed later. That would also allow for re-seeding on demand (not sure the RNG will deplete the entropy pool or just continue chugging along) instead of periodically, e.g. when the RNG hands out an |
Make the user specify the synchronisation implementation to use. ;) |
That's what I mean by best-effort. Output low-entropy (potentially guessable) "random" numbers now, but allow some type of entropy injection in order to improve security. But really if we go this route, we want some API to allow users the choice between "best effort available now" and "wait until it's secure".
Are you talking about Parking Lot's lock_api? This looks more like a building block for mutexes than something another lib can use. Or is there some other crate with user-replaceable mutexes?
Yes, but how? |
PRNGs don't deplete the entropy pool (theoretically they have finite length, but the cycle length is huge). However, if they are initialised with zero entropy or very little entropy then an observer can potentially guess the next output. The trick to making them secure is to use sufficient entropy to start with (> ~100 bits). It's also important that "reseeding" or "entropy injection" only actually uses this once enough is available for whatever security criterion is chosen (e.g. 100 or 256 bits); since if the PRNG is directly adjusted each time a little entropy is available then "entropy exhaustion" attacks are still possible. But it's possible the TRNG implementation will do the necessary accumulation. |
That's not what I meant, though. 😉 I would want the combination:
Check out https://crates.io/crates/shared-bus, I think it explains it nicely. |
let manager = shared_bus::BusManager::<std::sync::Mutex<_>, _>::new(i2c); This is not compatible with What might be best is a new crate, |
I see. For me it would be fine to just ignore additional entropy once enough has been supplied. I would still find it beneficial for initialisation purposes if I could try to use and if the PRNG deems that not enough entropy is available it'll just return an |
This is actually what djb recommends (because additional entropy does not really improve security but might reduce it), so I agree this is probably the best approach. |
@vks what do you think of the idea of making a |
Are the I kinda dislike exposing anything like |
@dhardy sorry I'm absolutely overloaded right now and so I don't have time to work through the design here. Sorry about that :( |
No that's fine Alex. |
Can't we make a mutable static which will hold a pointer to function which will be source of entropy? For example for modern linux it will be simply |
So here is a small prototype which seems to work:
// in the real crate if `std` is enabled use syscalls and panicking function otherwise
// function must fill the whole buffer or return an error
pub fn default_entropy(buf: &mut [u8]) -> Result<(), u8> {
buf.iter_mut().for_each(|v| *v = 1);
Ok(())
}
pub static mut ENTROPY_SOURCE: fn(&mut [u8]) -> Result<(), u8> = default_entropy;
// we probably want to keep it as thin as possible
pub struct DefaultRng;
// in real crate use RngCore+CryptoRng instead
impl DefaultRng {
pub fn new() -> Self { DefaultRng }
pub fn try_fill_bytes(&mut self, buf: &mut [u8]) -> Result<(), Error> {
unsafe { ENTROPY_SOURCE(buf) }
}
} Crate library: extern crate rand;
use rand::DefaultRng;
pub fn foo() -> u32 {
let mut buf = [0u8; 32];
let mut rng = DefaultRng::new();
rng.try_fill_bytes(&mut buf).unwrap();
let mut c = 0u32;
for v in buf.iter() { c += *v as u32; }
c
} App crate: extern crate crate_lib;
extern crate rand;
// this function can use HW source or accumulated entropy pool
pub fn custom_entropy(buf: &mut [u8]) -> Result<(), Error> {
buf.iter_mut().for_each(|v| *v = 2);
Ok(())
}
fn main() {
unsafe {
rand::ENTROPY_SOURCE = custom_entropy;
}
println!("{}", crate_lib::foo());
} App crate prints 64 instead of 32 as expected. Of course Alternatively we may want to split We also could make entropy source signature a bit closer to |
@newpavlov isn't that approach similar to the one implemented in this PR, except that it's slightly lower level (function pointer instead of trait object pointer)? Functionally I don't see much difference, and @burdges points still stand. As @burdges says, it may make sense to hide this behind a feature flag, or only when not using |
Yes, it's similar, but IMHO much simpler, you are always guaranteed to have "default" entropy source (but without I think we could omit And yes, I think this "default RNG" should be defined outside of Contrary to your proposal I don't think that we should provide anything for |
You are aware that even reading
An "unavailable" stub for |
I think If we need Afaik, there is never any reason to call Why distinguish between |
Yes, this is why I've used It's possible to create pub type EntropySource = fn(&mut [u8]) -> Result<usize, Error>;
#[cfg(target_has_atomic = "ptr")]
static ENTROPY_SOURCE: AtomicPtr<EntropySource> =
AtomicPtr::new(default_entropy as *mut EntropySource);
#[cfg(not(target_has_atomic = "ptr"))]
static mut ENTROPY_SOURCE: EntropySource = default_entropy;
#[cfg(target_has_atomic = "ptr")]
pub unsafe fn set_custom_entropy(source: EntropySource) {
// Not sure if `Release` is enough, to be safe we could use `SeqCst` instead
ENTROPY_SOURCE.store(source as *mut EntropySource, Ordering::Release);
}
#[cfg(not(target_has_atomic = "ptr"))]
pub unsafe fn set_custom_entropy(source: EntropySource) {
ENTROPY_SOURCE = source;
} I guess we could remove
Can you provide a link with the problem description? I think at the very least it should be disabled for cryptographic entropy source. And if it will be possible to redefine entropy source with |
I'd assume Also, we can poison Is there a discussion of the receiver type
Is it okay that |
Thanks for the suggestion @burdges; sounds sensible. Yes, the interface must be thread-safe.
Right, this is an abuse of trait objects instead of function pointers (the idea being that the user sets a trait-object, which can then provide all the above functions). For our purposes
See 180 ("fixed" by #181, which was superseded by #196).
|
As long as there's awareness that in |
So it's about I am a bit hesitant about using trait objects, please keep in mind, that entropy source should be usable on embedded platforms without allocations, atomics and mutexes. Simple function is easy to understand and reason about in the presence of interrupts, while IIRC it's currently impossible to create trait objects from raw pointers on stable. What is the motivation behind
In all three cases we don't need those methods. For entropy source users they are not needed as well. I see why we would need |
This was done a long time ago (0.4).
Trait objects are part of the language. Atomics are part of the core lib, essential with multiple CPU (core)s, and should be trivial with a single CPU. The current design doesn't need allocations and wouldn't need real mutexes except that I took the lazy option here (it's only a proof of concept).
|
@dhardy What is the status of this? It seems like some of the changes here rather belong to the |
That's a complicated question. IIRC there are several topics of discussion:
So yes, at least some of this now belongs in the |
Ok, then let's close this and move the discussion to rust-random/getrandom#4. |
No, lets not. There are several points in here not yet tracked elsewhere. I'll re-read the conversation when I can find the time. |
Okay, I read through and added a few notes to getrandom#4. Some discussion points may be worth referencing if anyone wishes to pursue |
ThreadRng
(Future ofthread_rng
#463 and ??) — I think we only didn't do this before because of safety concerns, but (1) use of raw pointers automatically implies the type is neither send nor sync, (2)thread_local
instantiates automatically so there is always a valid object and (3) none of the types involved implementDrop
EntropyRng
a little (working towards Makethread_rng()
available withoutstd
#313)set_custom_entropy
functionNext step: allow custom entropy source. Unfortunately theEntropySource
trait is still not good enough for run-time injection because the size & alignment of instances is unknown (it would be nice if this could be enforced somehow).