Should random() be banned?

#1

Post by pabloest » Thu, Feb 11, 2021, 5:03 PM UTC

Re: Should random() be banned?

#2

Post by draw_down » Thu, Feb 11, 2021, 5:29 PM UTC

We did essentially that at my employer. I think the rationale is good, biasing toward a secure random function makes sense because the downside (as I understand it) is performance, but defaulting to insecure random has worse downsides. And if there ends up being a hot path where secure random is too inefficient, you can change it in that case. (This is in a context where a secure random function is readily at hand, when that's not the case, it could be trickier.)

Re: Should random() be banned?

#3

Post by ThePadawan » Thu, Feb 11, 2021, 5:30 PM UTC

I'm not big on bans.

What I am big on is forcing developers to make deliberate choices. That's why I like React's policy of naming functionality "dangerouslySetInnerHTML" or "__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED".

If you add usages for these in a PR I'm reviewing without justification, it's not getting merged.

So why not make cryptographically unsafe random unsafeRandom() or shittyRandom() or iCopyPastedThisFromStackOverflowRandom()?

Re: Should random() be banned?

#4

Post by madsbuch » Thu, Feb 11, 2021, 5:32 PM UTC

Is there something I am overlooking here? Randomness is used for many other things than cryptographic usages.

Eg. for randomised algorithms you need a fast source of randomness.

Re: Should random() be banned?

#5

Post by cpeterso » Thu, Feb 11, 2021, 5:37 PM UTC

ThePadawan wrote: post #3

I'm not big on bans. What I am big on is forcing developers to make deliberate choices. That's why I like React's policy of naming functionality "dangerouslySetInnerHTML" or "__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED". If you add usages for these in a PR I'm reviewing without justification, it's not getting merged. So why not make cryptographically unsafe random unsafeRandom() or shittyRandom() or iCopyPastedThisF…

Firefox has a secret setting used in test automation called “turn_off_all_security_so_that_viruses_can_take_over_this_computer”.

https://searchfox.org/mozilla-central/rev/3ff133d19f87da2ba0...

Re: Should random() be banned?

#6

Post by zrm » Thu, Feb 11, 2021, 5:41 PM UTC

ThePadawan wrote: post #3

I'm not big on bans. What I am big on is forcing developers to make deliberate choices. That's why I like React's policy of naming functionality "dangerouslySetInnerHTML" or "__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED". If you add usages for these in a PR I'm reviewing without justification, it's not getting merged. So why not make cryptographically unsafe random unsafeRandom() or shittyRandom() or iCopyPastedThisF…

It's also a good idea to give safer things shorter names.

So make random() a CSPRNG (and an alias for SecureRandom() for people who want to be explicit) while InsecureFastRandom() is just what it says and has no other name. Then if you really need performance over unpredictability, it's there, but nobody is confused about what they're getting. And lazy people who don't like to type or pay close attention get the safe one.

Re: Should random() be banned?

#7

Post by kasperni » Thu, Feb 11, 2021, 5:41 PM UTC

If any, I would say it should be other way around. There are very few use cases outside of cryptography. So flag it if people uses a cryptographically secure PRNG directly. In almost all cases they would be better off finding a library that does what they need.

Re: Should random() be banned?

#8

Post by Spivak » Thu, Feb 11, 2021, 5:41 PM UTC

ThePadawan wrote: post #3

I'm not big on bans. What I am big on is forcing developers to make deliberate choices. That's why I like React's policy of naming functionality "dangerouslySetInnerHTML" or "__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED". If you add usages for these in a PR I'm reviewing without justification, it's not getting merged. So why not make cryptographically unsafe random unsafeRandom() or shittyRandom() or iCopyPastedThisF…

So you’re not big on bans but if you use dangerouslySetInnerHTML then it’s definitely not getting merged? Is that not a ban? Do you just not like when tooling enforces it?

Re: Should random() be banned?

#9

Post by sp332 » Thu, Feb 11, 2021, 5:43 PM UTC

ThePadawan wrote: post #3

I'm not big on bans. What I am big on is forcing developers to make deliberate choices. That's why I like React's policy of naming functionality "dangerouslySetInnerHTML" or "__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED". If you add usages for these in a PR I'm reviewing without justification, it's not getting merged. So why not make cryptographically unsafe random unsafeRandom() or shittyRandom() or iCopyPastedThisF…

The "ban" can be evaded by telling semgrep to ignore it for one line. https://semgrep.dev/docs/ignoring-findings/ This doesn't really scale though - if someone bans it with a different tool, you'd have to tell each tool to ignore this line.

Re: Should random() be banned?

#10

Post by iratewizard » Thu, Feb 11, 2021, 5:43 PM UTC

ThePadawan wrote: post #3

I'm not big on bans. What I am big on is forcing developers to make deliberate choices. That's why I like React's policy of naming functionality "dangerouslySetInnerHTML" or "__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED". If you add usages for these in a PR I'm reviewing without justification, it's not getting merged. So why not make cryptographically unsafe random unsafeRandom() or shittyRandom() or iCopyPastedThisF…

Beginner friendliness is something to remember, too. There are half a dozen words you could use to describe pseudoRandom(). Random() is easy for a first year or non-professional to remember.