Opened 5 years ago

Closed 4 years ago

#4468 enhancement closed fixed (fixed)

twisted.python.randbytes

Reported by: zooko Owned by:
Priority: normal Milestone:
Component: core Keywords: security
Cc: zooko, zooko@…, thijs Branch: branches/randbytes-without-pycrypto-4468
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description (last modified by exarkun)

twisted/python/randbytes.py doesn't seem to offer a lot of functionality over os.urandom, except for two misfeatures:

  1. The option of setting fallback=True so that it will automatically fallback to insecure random numbers if it can't generate secure ones. This is a terrible idea that nobody should ever do. Behavior like that is one of the causes of the Debian OpenSSL Fiasco, for example. If anyone has a legitimate use case for this behavior I would be fascinated to hear it.
  2. The option of using PyCrypto's Crypto.Util.randpool if it is present. randpool is deprecated (according to the announcement at the top of http://www.dlitz.net/software/pycrypto/ on this date, which references this mailing list thread). I've looked at randpool a bit, and it doesn't offer any advantage over os.urandom that I can see other than the option of falling back to insecure random number generation if it can't generate secure random numbers. randpool also has a lot of other code to do some useless things about estimating entropy, sampling the current clock, and so on. I can't be sure that I understood its source code because that other stuff made it hard to understand the part I was interested in. The latest git version of PyCrypto comes with this warning: "Deprecated. Use Random.new() instead. See http://www.pycrypto.org/randpool-broken". However that link gives me a 404 Not Found.

I suspect both of these modules predate the os.urandom module that was introduced in Python 2.4. Now that Twisted no longer supports Python 2.3, perhaps the time has come to deprecate twisted.python.randbytes in favor of os.urandom.

Change History (11)

comment:1 follow-up: Changed 5 years ago by exarkun

  • Description modified (diff)
  • Summary changed from twisted.python.randpool to twisted.python.randbytes

How about other platforms, where there is no /dev/urandom? Does Python automatically make os.urandom use whatever platform-specific random source they provide? How about future platforms which won't provide this? Or a future Linux replacement for /dev/urandom that's completely superior?

Getting rid of the non-cryptographic fallback may be a good idea, but I don't think completely eliminating this module makes sense. It's a good API for centralizing our decisions about which random number generator to use.

comment:2 Changed 5 years ago by exarkun

  • Resolution set to invalid
  • Status changed from new to closed

Feel free to re-open (or open another more specific ticket) if you have further comments.

comment:3 in reply to: ↑ 1 Changed 5 years ago by thijs

  • Cc thijs added
  • Resolution invalid deleted
  • Status changed from closed to reopened

Replying to exarkun:

How about other platforms, where there is no /dev/urandom?

From the docs: "If a randomness source is not found, NotImplementedError will be raised."

Does Python automatically make os.urandom use whatever platform-specific random source they provide?

"On a UNIX-like system this will query /dev/urandom, and on Windows it will use CryptGenRandom."

How about future platforms which won't provide this? Or a future Linux replacement for /dev/urandom that's completely superior?

Getting rid of the non-cryptographic fallback may be a good idea, but I don't think completely eliminating this module makes sense. It's a good API for centralizing our decisions about which random number generator to use.

I think we should get rid of the fallback and switch to os.urandom, and if that throws a NotImplementedError fallback to Random.new()?

comment:4 Changed 5 years ago by exarkun

I think we should get rid of the fallback and switch to os.urandom, and if that throws a NotImplementedError fallback to Random.new()?

Half of zooko's complaint seems to be that Crypto.Util.randpool shouldn't be used at all, so that fallback probably doesn't make sense.

So, specifically, it sounds like we should drop the use of PyCrypto entirely and then this ticket will be resolved.

We could also remove the /dev/urandom support code, but why should we bother?

comment:5 Changed 5 years ago by glyph

  • Owner changed from glyph to zooko
  • Status changed from reopened to new

For what it's worth, you have to specifically pass fallback to say "I don't care, for this application". Perhaps calling the method secureRandom is misleading, but you can use it for a networked dice-roller or something, so you can run it on your NetBSD set-top box, it's obviously not intended for crypto math. None of the crypto functions in Twisted use that argument.

I officially do not care about the outcome of this ticket though, because for almost all users on almost all platforms, it already uses os.urandom. I agree with exarkun that it is a good place to centralize decisions about random number generation, but since os.urandom itself seems to be doing an okay job of doing that, I won't shed a tear if it goes away either.

comment:6 Changed 4 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/randbytes-without-pycrypto-4468

(In [29376]) Branching to 'randbytes-without-pycrypto-4468'

comment:7 Changed 4 years ago by exarkun

  • Keywords review added
  • Owner zooko deleted

comment:9 Changed 4 years ago by cyli

  • Keywords review removed
  • Owner set to exarkun

Pyflakes reports sys as an unused import in test_randbytes, but other than that it looks good to merge.

comment:10 Changed 4 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [29474]) Merge randbytes-without-pycrypto-4468

Author: exarkun
Reviewer: cyli
Fixes: #4468

Remove the bogus PyCrypto backend to twisted.python.randbytes.

comment:11 Changed 4 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.