Unify Random and SecureRandom #3434

oprypin · 2016-10-17T20:12:39Z

Followup to #3402

Random now has a random_bytes method.
SecureRandom now extends the Random module, so it can use all the normal RNG operations based on the cryptographic source.
In order to have decent performance for this (to avoid calling urandom like 10 times for each generated number), now reimplementing random_bytes can be an alternative to implementing next_u.

RX14 · 2016-10-17T21:23:32Z

src/random.cr

@@ -56,6 +56,14 @@ module Random
  # The integers must be uniformly distributed between 0 and the maximal value for the chosen type.
  abstract def next_u : UInt

+  # Generates a slice filled with *n* random bytes.
+  def random_bytes(n : Int) : Bytes


It would be ideal if this worked the same was as IO#read.

I don't understand

IO#read takes a slice of a fixed size and fills it. It's designed so that you can reuse the slice efficiently.

This takes a size and returns a slice of that size, which means that the allocation isn't controlled by the caller. I know why you do this for performance when filling it with integers, but I dislike the inconsistency.

@RX14 oh, I've considered doing this (I think avoiding allocations could give even better performance actually), but in the end I think simplicity is more important. The consistency is with SecureRandom.random_bytes.

Anyway, I don't mind changing this, but I'm not sure if it's best. Leaving as is for now.

Some buffering would be interesting here too. random_bytes could always get at least.. say.. 32 random bytes from the source. And then successive calls would use this buffer until it is depleted and it needs to be repopulated again. Anyway, this is an improvement for the future. +1 for this pull request.

Buffering would solve many problems that were kind of worked around. But... that just seems not... secure. So I avoided it on purpose.

RX14 · 2016-10-17T21:26:57Z

src/secure_random.cr

+# SecureRandom.uuid            # => "c7ee4add-207f-411a-97b7-0d22788566d6"
+#
+# SecureRandom.rand(10000)        # => 4264
+# [1, 2, 3].shuffle(SecureRandom) # => [1, 3, 2]
 # ```
 #
 # The implementation follows the
 # [libsodium sysrandom](https://github.com/jedisct1/libsodium/blob/6fad3644b53021fb377ca1207fa6e1ac96d0b131/src/libsodium/randombytes/sysrandom/randombytes_sysrandom.c)
 # implementation and uses `getrandom` on Linux (when provided by the kernel),
 # then tries to read from `/dev/urandom`.
 module SecureRandom


I think SecureRandom should become Random::Secure to be more consistent, now that it's a Random implementation.

RX14 · 2016-10-17T21:29:23Z

src/secure_random.cr

 # ```
 #
 # The implementation follows the
 # [libsodium sysrandom](https://github.com/jedisct1/libsodium/blob/6fad3644b53021fb377ca1207fa6e1ac96d0b131/src/libsodium/randombytes/sysrandom/randombytes_sysrandom.c)
 # implementation and uses `getrandom` on Linux (when provided by the kernel),
 # then tries to read from `/dev/urandom`.
 module SecureRandom
+  extend Random


I think it's a bit weird that SecureRandom doesn't include Random like Random::MT19937. I guess it's just because it doesn't need a seed?

It's a module which can't be instantiated. I was really surprised that because extend is used, the SecureRandom module itself can be used exactly like an instance of a class including Random

ysbaddaden · 2016-10-17T22:02:45Z

I think there is a dichotomy.

SecureRandom is meant for cryptography, to generate a bunch of random bytes. For example to generate keys, tokens or unique identifiers that can't be forged.

Random is meant for generating a random number (not many bytes), having numbers informally distributed, and do it fast. For example to pick a random entry in an Array, or flush a deck of card.

I really don't see any use case for SecureRandom.rand(max) for example. Its pointless at best, maybe dangerous at worst (it's no longer secure).

I'm not against unifying the implementations, or to rename the module as Random::Secure, but I'm not sure considering urandom as Random implementation is a good idea.

oprypin · 2016-10-17T22:14:17Z

@ysbaddaden

What if someone wants to shuffle a deck of cards really well for an important game? What if someone wants a lot of random bytes for noise but the use case is not related to cryptography? Should they reimplement this functionality from scratch just because you say it's pointless?

it's no longer secure

?

ysbaddaden · 2016-10-17T22:41:01Z

First of all, thanks for improving Random. Really. I don't mean to be rude or anything. I'm concerned with security or the impression of security given by SecureRandom. Everything it provides must be cryptographically secure (hence why I reworked it to always use urandom, and will rewprk it to use arc4random on OpenBSD).

What if someone wants to shuffle a deck of cards really well for an important game?

I would seed a PRNG from a secure source, and choose a PRNG that returns uniformally distributed numbers. Scratch that, I would use a sort method that uniformally distributes cards in a deck.

?

Taking random bytes and applying a transformation to it makes it less secure, because it's now more predictable than it was before (you reduce the possibilities). Let's say you had a 0..65535 range, you reduced it to 0..10000, hence possibilities were divided by 6.5!

RX14 · 2016-10-17T22:47:31Z

@ysbaddaden The "secure" from secure random comes from it being a sufficiently good source of entropy to perform cryptography with. I don't think that secure random implies that it's results are only to be used for cryptography or other security-like use cases where the amount of entropy matters.

In short I think that Random::Secure should ensure that the quality of entropy is secure, not attempt to enforce that the amount of entropy you draw out is "enough".

oprypin · 2016-10-17T22:47:52Z

I would seed a PRNG from a secure source, and choose a PRNG that returns uniformally distributed numbers.

Sure, that's what's done usually. But the PRNGs have small state (so results' randomness is limited) and produce only, say, 32-bit numbers (that's why this whole Random module is needed in the first place).

Let's say you had a 0..65535 range, you reduced it to 0..10000, hence possibilities were divided by 6.5!

No, if I have a 0...65536 range, I accept 0...60000 when the 0...10000 range is requested (well that was in the other merged PR)

ysbaddaden · 2016-10-18T07:28:14Z

I don't think that secure random implies that it's results are only to be used for cryptography

No, but cryptography quality trumps other usages.

rayhamel · 2016-10-23T07:59:05Z

Perhaps SecureRandom should be renamed to something else? HighEntropyRandom, RobustRandom, UnpredictableRandom or what have you. Cryptography is an area where you either know exactly what you're doing, or you really don't; a name like "SecureRandom" gives the latter group a false sense of "security" while they go about using secure functions insecurely.

oprypin · 2016-10-23T08:09:59Z

https://docs.python.org/3/library/random.html#random.SystemRandom

oprypin · 2016-11-08T22:08:02Z

@asterite Status?

asterite · 2016-11-09T14:52:14Z

@BlaXpirit At oredev, so I'll take a look at this next week :-)

zatherz · 2017-02-11T23:06:44Z

"I'll take a look at this next week"

Sija · 2017-05-04T18:56:13Z

@asterite ping?

RX14 · 2017-05-04T19:39:04Z

It seems asterite has all but left the project recently :(

ysbaddaden · 2017-05-04T19:53:54Z

I'm as strongly opposed to this change as I used to be.

Please provide a Random::System alternative PRNG that will read bytes from the best system source, add Random#random_bytes(Slice). These are great improvements, which I'll merge right away. But pretty please don't touch SecureRandom, thanks!

spalladino · 2017-05-09T21:31:40Z

I back @ysbaddaden on this one, better to keep SecureRandom as is.

oprypin · 2017-05-09T22:32:13Z

Let me just say that I'm in total disbelief on the core team's response.

I do not intend to rework this into something that makes no sense, namely, providing access to system's random source through 2 different modules with an arbitrary split and with some overlapping functionality.

The revelation I had when writing extend Random was kinda worth it, but in the end missing such an opportunity just makes me sad.

ysbaddaden · 2017-05-10T11:33:03Z

I apologize for hard feelings, but usages are different.

System random doesn't mean secure (e.g. arc4random is secure on OpenBSD but isn't on FreeBSD).

Secure means suitable for cryptography usages. We musn't change the bytes fetched from the secure random source in any way. I prefer to be safe than sorry because algorithms were supposed to be good enough, but actually someone found later that it's not.

Hence my strong opinion on the topic.

RX14 · 2017-05-10T12:01:54Z

If we do keep SecureRandom separate from Random, we should at least build SecureRandom on an instance of Random. Users must be able to access the exact same secure random source used for SecureRandom as a Random implementation. Sure we can document the class saying that just because you use the class doesn't mean your random code is secure, but it should be available to the people who do know what they're doing.

ysbaddaden · 2017-05-10T13:00:25Z

Yes, Random::System could preferably use a secure source.

oprypin · 2017-06-03T21:42:13Z

Superseded by #4450

oprypin added 2 commits October 17, 2016 22:56

Add random_bytes; use it for random number generation if overridden

Unverified

This user has not yet uploaded their public signing key.

GPG key ID: CEF2BBFE18BD0E67

Learn about vigilant mode

99d39c4

SecureRandom now extends Random

6678a9c

RX14 reviewed Oct 17, 2016

View reviewed changes

RX14 mentioned this pull request Feb 18, 2017

Add non-allocating method to SecureRandom #4049

Closed

konovod mentioned this pull request Mar 24, 2017

Adds non-allocating random_bytes method to SecureRandom. Fixes #4049 #4191

Merged

oprypin mentioned this pull request May 23, 2017

Crystal::System::Random namespace #4450

Merged

oprypin closed this Jun 3, 2017

ysbaddaden mentioned this pull request Aug 28, 2017

Merge SecureRandom into Random and Random::Secure #4894

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify Random and SecureRandom #3434

Unify Random and SecureRandom #3434

oprypin commented Oct 17, 2016 •

edited

Loading

RX14 Oct 17, 2016

oprypin Oct 17, 2016

RX14 Oct 17, 2016

oprypin Oct 17, 2016 •

edited

Loading

lbguilherme May 9, 2017

oprypin May 9, 2017

RX14 Oct 17, 2016

RX14 Oct 17, 2016

oprypin Oct 17, 2016

ysbaddaden commented Oct 17, 2016

oprypin commented Oct 17, 2016

ysbaddaden commented Oct 17, 2016

RX14 commented Oct 17, 2016

oprypin commented Oct 17, 2016 •

edited

Loading

ysbaddaden commented Oct 18, 2016

rayhamel commented Oct 23, 2016

oprypin commented Oct 23, 2016

oprypin commented Nov 8, 2016

asterite commented Nov 9, 2016

zatherz commented Feb 11, 2017

Sija commented May 4, 2017

RX14 commented May 4, 2017

ysbaddaden commented May 4, 2017

spalladino commented May 9, 2017

oprypin commented May 9, 2017

ysbaddaden commented May 10, 2017

RX14 commented May 10, 2017

ysbaddaden commented May 10, 2017

oprypin commented Jun 3, 2017

Unify Random and SecureRandom #3434

Unify Random and SecureRandom #3434

Conversation

oprypin commented Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oprypin Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysbaddaden commented Oct 17, 2016

oprypin commented Oct 17, 2016

ysbaddaden commented Oct 17, 2016

RX14 commented Oct 17, 2016

oprypin commented Oct 17, 2016 • edited Loading

ysbaddaden commented Oct 18, 2016

rayhamel commented Oct 23, 2016

oprypin commented Oct 23, 2016

oprypin commented Nov 8, 2016

asterite commented Nov 9, 2016

zatherz commented Feb 11, 2017

Sija commented May 4, 2017

RX14 commented May 4, 2017

ysbaddaden commented May 4, 2017

spalladino commented May 9, 2017

oprypin commented May 9, 2017

ysbaddaden commented May 10, 2017

RX14 commented May 10, 2017

ysbaddaden commented May 10, 2017

oprypin commented Jun 3, 2017

oprypin commented Oct 17, 2016 •

edited

Loading

oprypin Oct 17, 2016 •

edited

Loading

oprypin commented Oct 17, 2016 •

edited

Loading