-
Notifications
You must be signed in to change notification settings - Fork 428
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
OsRng run-time failure / the rand_os crate / modularity #681
Comments
I do not consider |
The current I am not proposing to adjust |
I disagree with it. I believe that The breakage caused by splitting code into
|
You can't have target specific features with cargo atm, therefore it is impossible to correctly implement failing to compile at the moment. I have indirect dependencies on rand through criterion and parking_lot. You can't realistically disable the rand_os dependency (via rand's std feature as far as I understand) for both of these crates. Therefore my use case will always (or at least until cargo can support target specific features) compile in rand_os without using stdweb or wasm_bindgen. So failing to compile is not a realistic option until then. What is needed is either a split between the -unknown and a potential -web target and / or fixing cargo. |
Noted, but the alternative is to keep telling those who complain about broken CI that they need more complex set-up to add the required feature on WASM... I guess this is possible, but it's a support nightmare. @CryZe you can probably do this — force your CI tests to depend on WASM + stdweb (or -bindgen). However if you don't use Rand directly, you will need to add it (as a dev-dependency or via another crate only used for testing).
Well, I disagree with that, because there is a plan to eventually make the two compatible.
Agreed; however for now we have to support |
It's not my tests that are broken, it's the actual compile (the tests work just fine). For my actual compile, I can't activate either stdweb or wasm-bindgen feature, because I'm not using them (I'm using a different binding generator, so this is kind of the truly "unknown" use case where I'm fine with rand not having a notion of an OS RNG). Also my crate does not exercise any code paths in the wasm compiled code that uses rand_os. However it gets compiled regardless because criterion is a dev-dependency, so cargo not separating features across different dependency kinds or targets is a big problem. |
But you could work around this by adding one of the features in your build? |
I'm confused why this can't be done with combinations of |
The stdweb feature doesn't work as I'm not using cargo web (this wouldn't make sense for my unknown use case):
However I just tried the wasm-bindgen feature too and it does seem like it does not conflict with anything the unknown use case needs. So I'm actually fine with that.
Cargo propagates features of any cfg combination in the Cargo.toml to all dev-dependencies / (normal-)dependencies and all cfg combinations. They are all in a shared feature space, the cfg'ing or dev-dependency-ing doesn't do anything. This is a bug which is not fixed. Therefore within your actual code your cfg will be against the wrong feature flags, that are actually coming from a different target of a different dependency kind. So here it is the dev-dependency criterion compiled for the host that needs OsRng, but the actual wasm32-unknown-unknown target that your code is targeting doesn't need or want OsRng, but criterion's rand/std feature gets propagated to rand regardless and thus OsRng gets compiled in, even though it's only needed in the dev-dependency on the host (x86, not wasm). |
I'd prefer if I've no opinion on I think runtime failures sound perfectly acceptable here too. |
IIRC jitter fallback is only needed for Windows. One option will be to incorporate it into Windows OS RNG implementation inside
In my understanding JitterRng should be cryptographically secure, the main issue with it is its performance. Citing
|
Interesting point about We could put it behind a feature gate which is disabled by default — most people won't notice the difference, and those that do can consider enabling it.
It will — at the very least we will avoid known security problems and only use things we have reason to believe are secure. Whether or not this meets your standards of security is another matter; currently we don't have formal standards and I don't really want to go there. |
Before we get too carried away (#685) we should discuss what we actually want. One possible option, compatible with @vks's "rand is just a shim" goal, is:
Rationale:
Limitation:
|
What is a rationale for having
If we'll get
I disagree. |
But why do we want to roll the fall-back into the same RNG? It's not simply a wrapper around the OS RNG any more. Same goes for embedded device-specific RNGs and As I said already, I'd prefer to keep
This is already possible. The entire reason However, I guess it still works nicely as a separate crate, though with the number of crates we're collecting I'm wondering again about using multiple repos (core / entropy stuff / rngs / sampling). |
Failing at compile time (i.e. solution 1) is ideal IMHO. I think that the important thing to me is that it should always be very clear if a RNG is not a CSPRNG. The other issue and RFC into I don't know what the right balance is here, but I agree with many of the sentiments expressed above:
As much as possible, fail at compile time and leverage the type system to make secure/insecure RNGs as obvious as can be. |
BTW, do check out the |
Ah, I was judging by the current state of things. I don't see a good way of enabling overwritable
I told ya. ;) As for WASM and |
BTW can you please remind the rationale for it? |
I donno. It's defined in |
I think readability dictates that There is a case that |
We still have the question of whether We already mentioned that it may be used to implement |
@dhardy I'm gonna shut up now, lol, and make sure that I'm familiar with the API before commenting further. Apologies, I should have done my homework before weighing in. |
See the last line of impl<R: BlockRngCore + CryptoRng> CryptoRng for BlockRng<R> {} A impl<R: CryptoBlockRngCore> CryptoRng for BlockRng<R> {} ... but it's probably better just to let crates like |
Well, I agree with @burdges that placing On the other hand, we haven't resolved how I would definitely not but Conclusion: none of this really has an impact on whether we have a separate |
Cool. Afaik, there is no real reason for the |
@burdges A Personally I would prefer not to have any "legacy crypto" crates in my project whatsoever. "It's the only way to be sure!" |
I generally disagree with extremist POVs... maybe there's a more tactful way of expressing your intent? What do you mean by "legacy crypto", besides ISAAC? ISAAC will no longer be included in Rand once we remove the deprecations (probably in 0.7). |
I think poor documentation sounds like the biggest threat @tarcieri and micro-crates almost always make documentation worse for interlocking components, ala As a rule, I think CSPRNG algorithms like ISAAC or ChaCha mostly present documentation acceptably when done as individual crates, so not worried there. In more detail, there is a minor documentation improvement in placing the CSPRNG algorithms side-by-side, and a In this case, our distinction runs like:
|
I don't see any critical problems with documentation which can not be solved.
I strongly disagree here. As for AFAIK the main reason for |
I brought this sort of thing up on #648 already, but when you pull in
Okay, so we have one '90s cipher (ISAAC), a member of the venerable eSTREAM portfolio, a descendent of a member of the eSTREAM portfolio which is now widely deployed in IETF protocols, and two non-cryptographic RNGs. The exact vintage of the cryptography, though, is a bit of a red herring, because really I don't want any userspace CSPRNGs in my project. So yes, while this is 3 stream ciphers, two of which are fairly obscure, and two non-cryptographic RNGs, all I want is |
@tarcieri @tarcieri and all: I wonder if an RNG (implementing In other words: perhaps we should build a simple "getrandom" crate, then discuss including that within @newpavlov if we make a "getrandom" function (filling a buffer), then there are three possibilities:
Because of this I think moving |
Hm, it may be worth to do it. I don't have any objections to this idea. Also having an implementation may help with the RFC acceptance.
Until we will get I think that in the current form |
Good. I suggest we start with a new repository under https://github.com/rust-random since this will not be dependent on any part of I was wondering if rather than try to get this functionality included in I really think the jitter stuff belongs there if we want it, rather than as an RNG. Yes, that module is currently 882 lines, but a large portion of that is documentation and error handling. The second point of this is to step back and look at what Rand is — it is about PRNGs, conversions of values to other Rust types, numeric distributions and random algorithms. Beyond Thus we can have (The one issue I see here is that CSPRNGs like ChaCha may be used both by crypto libs and as PRNGs within Rand. Perhaps we can move the core algorithms to a pure-crypto lib, then build |
I personally like
No, I think it certainly shouldn't be a part of Overall I don't quite understand the reluctance toward splitting Jitter RNG into its own crate. It's a self-contained algorithm, and however we will use it in future, a separate crate will not result in any substantial amount of harm, while being easier to add, move or remove.
I believe we will still need
I hope in future we will use a single |
You're now saying you don't think the jitter fallback should be used on Windows by default, but somehow applications which might need it should enable the fallback, and do so by overwriting the My reluctance is that I am not convinced we will want to keep Additionally, I'm not sure whether we should make applications provide a timer to
You point me to an example using only |
I'd be all for a Concretely I'd only want the existing OS random functions ( Speaking of RDRAND, it seems like that would be preferable to falling back to |
Also am I the only one who finds the idea of |
Have you any idea how slow @tarcieri I am not well versed in side-channel attacks, and don't know where to start answering such a question, aside from the obvious: some type of local hardware or software bug needs to be present, and if that can already read your memory then everything is vulnerable. How practical it is to guess the output of a generator based on nano-second jitter I don't know, but you are right that we shouldn't fully trust it, and RDRAND is likely preferable (that may also not be fully trusted, but if you don't trust your CPU then secure entropy sources aren't your only problem). We now have a slight architectural problem though because @nagisa's RDRAND implementation is dependent on |
@dhardy an example of a potential attack vector is attacker-controlled JavaScript running cotenant on the same CPU as a Rust program. As we saw last year, covert channels are exploitable from web browsers. So the question for |
@tarcieri
We can make
In the linked case it will panic, which seems is not a problem for dalek developers. But some of the crypto crate developers (e.g. @briansmith and @ctz IIUC) prefer to expose every potential error even if it's highly unlikely. And I don't think that |
I know next to nothing about the current
Perhaps it should be moved to
I'd prefer it be a guarantee as opposed to a best guess. A more concrete question to ask is: is the current TRNG algorithm designed to be sidechannel resistant? A quick search for papers on this topic pulls up: It seems provable sidechannel resistance for TRNGs is a thing. |
@tarcieri Your first article on GAROs sounds like it is observational rather than a definitive proof? I didn't read the whole article. @newpavlov probably we should have an issue on Summary: But as to what to do, it might still be best not to use a jitter-based-rng in our
This prevents us from making |
I made a separate issue for |
@newpavlov I think this means we should either delete Ideally I'd like another backup source in place before removing it however. |
I don't mind moving |
Agreed, we shouldn't remove it before 0.7, and even then we might want a deprecation warning. Fair point, the crate name doesn't need to change, so there's no reason we can't merge #685 now. |
I think the time has come to close this thread. It has several progeny:
The continuation of the remainder of this discussion can move to #760. |
Roughly, rand could be modularised as follows:
rand_core
rand_os
JitterRng
EntropyRng
ThreadRng
seq
distributions
Rng
Roughly 30% of the current code comes under non-linear distributions, which is its own story (#290). For the rest I don't think there's sufficient utility for more crates.
I regret accepting #643 as-is; it "promises" to support all
std
platforms yet fails on WASM with a compile error (#674, #678) and on any unsupported platforms. The reasons that is an issue are convoluted (probably involving users depending onrand
with default features yet not needing them all), but the conclusion of #678 is that Rand should always build forwasm32-unknown-unknown
, failing if necessary at run-time.We used to use run-time failure within
EntropyRng
but disableOsRng
at compile time where unneeded. Practical solutions are:rand_os
must compile but without theOsRng
struct on unsupported platforms.EntropyRng
is currently available on all platforms but with run-time failure when no source is available, however it requires a complexcfg
pattern in order to disable the missingOsRng
dependency when not available; it would therefore make sense to have this in the same crate.OsRng
on WASM, and use run-time failure on missing implementation.OsRng
on all platforms, and use run-time failure on missing implementation.I think the cleanest solution would be (1), but @tarcieri @naftulikay I understand there would be resistance to moving
JitterRng
intorand_os
. Is this really justified? I guess in part this depends on whether the RFC proposed in #648 gets accepted, though this will be a while coming.Otherwise we can do (2) or (3); the difference is that (2) would cause build failures on any platform where Rand is used with
std
/default features butOsRng
is unavailable. I'm not sure whether this is a good idea?I suppose (2) is most sensible now.
CC @newpavlov @burdges
The text was updated successfully, but these errors were encountered: