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

std.rand: set DefaultCsprng to Gimli, and require a larger seed #6622

Merged
merged 1 commit into from
Oct 16, 2020

Conversation

jedisct1
Copy link
Contributor

@jedisct1 jedisct1 commented Oct 9, 2020

DefaultCsprng is documented as a cryptographically secure RNG.

While ISAAC is a CSPRNG, the variant we have, ISAAC64 is not. A 64 bit seed is a bit small to satisfy that claim.

We also saw it being used with the current date as a seed, that also defeats the point of a CSPRNG.

Set DefaultCsprng to Gimli instead of ISAAC64, rename the parameter from init_s to secret_seed + add a comment to clarify what kind of seed is expected here.

Instead of directly touching the internals of the Gimli implementation (which can change/be architecture-specific), add an init() function to the state.

Our Gimli-based CSPRNG was also not backtracking resistant. Gimli is a permutation; it can be reversed. If the state was ever leaked, future secrets, but also all the previously generated ones could be recovered. Clear the rate after a squeeze in order to prevent this.

Finally, a dumb test was added just to exercise DefaultCsprng since we don't use it anywhere.

@jedisct1
Copy link
Contributor Author

jedisct1 commented Oct 9, 2020

There are still major issues with our current RNG interface.

If RNG states have to be passed around, they will inevitably end up being copied. Identical output streams will be generated. That can be catastrophic.

There is also currently no thread safety. That can be easily and immediately fixed, though.

They are not fork()-safe either. This will also cause identical output streams to be generated. If UUIDs are meant to uniquely identify a product, that can have pretty serious implications.

When secure random numbers are required, maybe we currently shouldn't rely on applications to take care of the points above. That seems very dangerous.

In C, at least on BSD, macOS and WASI (libc), the problem was solved with arc4random(), that automatically seeds a local CSPRNG from the system once and then uses a stream cipher to quickly derive everything else from it, in a thread-safe way. On OpenBSD, absolutely all the base applications requiring secure random numbers use this and nothing else. This is fast, simple and safe.

In Rust, rand::thread_rng() does the same and is what virtually everyone mechanically uses when secure random numbers are required (or OsRng but arbitrarily). This is simple and safe to use from anywhere, and doesn't require passing states around.

Would it be worth having something similar? We currently have crypto.getRandom() that has the above security guarantees, but requires a syscall every time some output is needed. It also doesn't implement the Rand interface.

@jedisct1
Copy link
Contributor Author

jedisct1 commented Oct 9, 2020

On the fact that applications shouldn't have to seed a CSPRNG, see this example code in a recent issue #6384 (comment)

    var seed = @intCast(u64, std.time.timestamp());
    var csprng = std.rand.DefaultCsprng.init(seed);

Timestamps, especially with a second granularity, are very predictable. Doing this in real-world applications is unfortunately still very common.

While that can be totally fine for a regular RNG (to run Monte-Carlo simulations or whatever - and we still want manual seeding here), it is definitely not to generate any kind of unpredictable material.

Zig could provide some safety here by having a default, secure, fast, global, thread-safe, fork-safe CSPRNG just like arc4random().

But as pointed out by @daurnimator , how would that work in a freestanding environment? Could an application define its own global CSPRNG?

lib/std/rand.zig Show resolved Hide resolved
lib/std/rand.zig Show resolved Hide resolved
lib/std/rand.zig Show resolved Hide resolved
@daurnimator
Copy link
Contributor

There are still major issues with our current RNG interface.

These deserve to be in an issue rather than be hidden away in a PR

If RNG states have to be passed around, they will inevitably end up being copied. Identical output streams will be generated. That can be catastrophic.

This is another case of #591 for which we have #3803

They are not fork()-safe either. This will also cause identical output streams to be generated.

Fork safety is really hard and both OS/architecture specific; I'm not sure we can provide a general solution here.
e.g. on linux I'm not sure if there is anything better than https://daurnimator.com/post/120415954844/linux-fork-detection-using-thread-specific ? I also had some discussions with @AGWA at the time on alternatives.

In C, at least on BSD, macOS and WASI (libc), the problem was solved with arc4random(), that automatically seeds a local CSPRNG from the system once and then uses a stream cipher to quickly derive everything else from it, in a thread-safe way. On OpenBSD, absolutely all the base applications requiring secure random numbers use this and nothing else. This is fast, simple and safe.

This is what zig should be implementing.

When linking against libc (which is the stable interface on BSDs) we would straight up use arc4random

For a static linux we should even be able to do it all without any syscalls: use AT_RANDOM from the auxiliary vector to seed the first thread; then when spawning other threads or forks, derive theirs.

It also doesn't implement the Rand interface.

That would be a good addition IMO.

lib/std/rand.zig Outdated Show resolved Hide resolved
`DefaultCsprng` is documented as a cryptographically secure RNG.

While `ISAAC` is a CSPRNG, the variant we have, `ISAAC64` is not.
A 64 bit seed is a bit small to satisfy that claim.

We also saw it being used with the current date as a seed, that
also defeats the point of a CSPRNG.

Set `DefaultCsprng` to `Gimli` instead of `ISAAC64`, rename
the parameter from `init_s` to `secret_seed` + add a comment to
clarify what kind of seed is expected here.

Instead of directly touching the internals of the Gimli implementation
(which can change/be architecture-specific), add an `init()` function
to the state.

Our Gimli-based CSPRNG was also not backtracking resistant. Gimli
is a permutation; it can be reverted. So, if the state was ever leaked,
future secrets, but also all the previously generated ones could be
recovered. Clear the rate after a squeeze in order to prevent this.

Finally, a dumb test was added just to exercise `DefaultCsprng` since
we don't use it anywhere.
@jedisct1
Copy link
Contributor Author

Detecting forks can be complicated, but not so much if forking is done using functions from the Zig standard library. It may be reasonable to not provide much guarantees about forks being made by other means.

@jedisct1
Copy link
Contributor Author

Pinning is indeed a great proposal! It will indeed solve that issue, as well as for anything that shouldn't be cloned.

@andrewrk andrewrk merged commit 51a3d06 into ziglang:master Oct 16, 2020
@andrewrk
Copy link
Member

There are still major issues with our current RNG interface.

If RNG states have to be passed around, they will inevitably end up being copied. Identical output streams will be generated. That can be catastrophic.

pinning should nip this in the bud

There is also currently no thread safety. That can be easily and immediately fixed, though.

I'd like to do this via wrapping the API, not by baking in thread safety into the random interface.

They are not fork()-safe either. This will also cause identical output streams to be generated. If UUIDs are meant to uniquely identify a product, that can have pretty serious implications.

I don't recognize general purpose use of fork() as a valid use case. Redis tries to be clever and uses it as a way to dump a backup of data while still running, but ultimately it's a footgun because the copy-on-write nature ends up nondeterministically requiring up to double the memory of a machine already and intentionally at memory capacity. I've seen this cause an outage in production. It's such a strange and dubious API that I would even say someone who uses fork() without execve() is intentionally trying to copy their state, including random number generators. If they don't want that they shouldn't use fork().

When secure random numbers are required, maybe we currently shouldn't rely on applications to take care of the points above. That seems very dangerous.

I support the policy of making the path of least resistance the robust and secure way of doing things.

In C, at least on BSD, macOS and WASI (libc), the problem was solved with arc4random(), that automatically seeds a local CSPRNG from the system once and then uses a stream cipher to quickly derive everything else from it, in a thread-safe way. On OpenBSD, absolutely all the base applications requiring secure random numbers use this and nothing else. This is fast, simple and safe.

In Rust, rand::thread_rng() does the same and is what virtually everyone mechanically uses when secure random numbers are required (or OsRng but arbitrarily). This is simple and safe to use from anywhere, and doesn't require passing states around.

Would it be worth having something similar? We currently have crypto.getRandom() that has the above security guarantees, but requires a syscall every time some output is needed. It also doesn't implement the Rand interface.

I'm in favor of a per-thread CSPRNG available for general use.

Zig could provide some safety here by having a default, secure, fast, global, thread-safe, fork-safe CSPRNG just like arc4random().

But as pointed out by @daurnimator , how would that work in a freestanding environment? Could an application define its own global CSPRNG?

We could follow the pattern set by std.log, panic, and event loop, where there is a convenient default, but the root source file has the option to override the global CSPRNG. On freestanding targets this would have the effect of making use of it be a compile error, however providing the implementation in the root source file would resolve the errors.

@andrewrk
Copy link
Member

andrewrk commented Oct 16, 2020

https://github.com/ziglang/zig/blob/master/lib/std/fs.zig has 2 callsites of std.crypto.getrandom that would benefit from this abstraction.

@jedisct1 would you be willing to file the proposal for this stuff that we talked about in this thread? never mind; I'm typing it up 👍

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

Successfully merging this pull request may close these issues.

4 participants