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

Switch from using PyCrypto's Random to using os.urandom. #297

Merged
merged 3 commits into from Apr 14, 2014

Conversation

Projects
None yet
4 participants
@alex
Contributor

alex commented Mar 30, 2014

There's several reasons for this change:

  1. It's faster for reads up to 1024 bytes (nearly 10x faster for 16 byte reads) -- it's also likely to get much much faster for large reads in the near future
  2. It receives considerably more security review since it's in the kernel.
  3. It's yet another step towards running on PyPy.
  4. Using userspace CSPRNGs is considered something of an anti-pattern. See:
    http://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/
    http://webcache.googleusercontent.com/search?q=cache:2nTvpCgKZXIJ:www.2uo.de/myths-about-urandom/+&cd=3&hl=en&ct=clnk&gl=us
Switch from using PyCrypto's Random to using os.urandom.
There's several reasons for this change:

1) It's faster for reads up to 1024 bytes (nearly 10x faster for 16 byte reads)
2) It receives considerably more security review since it's in the kernel.
3) It's yet another step towards running on PyPy.
4) Using userspace CSPRNGs is considered something of an anti-pattern. See:
   http://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/
   http://webcache.googleusercontent.com/search?q=cache:2nTvpCgKZXIJ:www.2uo.de/myths-about-urandom/+&cd=3&hl=en&ct=clnk&gl=us
@bitprophet

This comment has been minimized.

Member

bitprophet commented Mar 30, 2014

Already spoke with Alex about this but I'm generally +1 on this provided I can get additional smart people confirming there's no major downsides to the switch. (Sadly I myself do not qualify as "smart people" when it comes to crypto or math :()

@alex

This comment has been minimized.

Contributor

alex commented Mar 30, 2014

The first citation I provide in point (4) is by Thomas Ptacek, who's a fairly well known security researcher / practitioner.

@dstufft

This comment has been minimized.

dstufft commented Mar 30, 2014

Always use os.urandom IMO, most user space RNG is reliant on os.urandom for it's seed, so if os.urandom is compromised it's likely your user space RNG is compromised too.

@offbyone

This comment has been minimized.

Contributor

offbyone commented Mar 31, 2014

Is it worth leaving support for the userspace prng in case this raises NotImplementedError?

@dstufft

This comment has been minimized.

dstufft commented Mar 31, 2014

I'm generally of the opinion that things should fail hard when security can't be guaranteed instead of fail soft.

Additionally taking a look at PyCrypto.Random it appears to be using Fortuna seeded by "os rng", time.time(), and time.clock(). Taking a look at what constitutes an "os rng" for PyCrypto I can see:

  1. If os.name is "posix" then it uses Crypto.Random.OSRNG.posix.
  2. If os.name is "nt" then it uses Crypto.Random.OSRNG.nt.
  3. If os has urandom it uses that
  4. Otherwise it fails with NotImplemented.

Taking a deeper look:

  • Crypto.Random.OSRNG.posix basically just reads from /dev/urandom the same as os.urandom does.
  • Crypto.Random.OSRNG.nt uses CryptGenRandom the same as os.urandom does, however it applies a fix for the fact that CryptGenRandom doesn't provide forward secrecy (see http://eprint.iacr.org/2007/419). I am unaware if os.urandom has this same work around or not.
  • Obviously os.urandom is os.urandom.

I don't believe there is going to be any reasonable situation where Crypto.Random succeeds where os.urandom would fail. The windows work around would require more investigation into if CPython applies the same protections, whether those protections are even able to actually protect, and if this is even still a problem on whatever versions of Windows paramiko supports (assuming it supports any versions of Windows).

@alex

This comment has been minimized.

Contributor

alex commented Mar 31, 2014

I agree with @dstufft. I don't think there are any real world cases where os.urandom fails that can be safely worked around.

@offbyone

This comment has been minimized.

Contributor

offbyone commented Mar 31, 2014

I have learned much today. Thanks, @alex and @dstufft. I'd +1 this but pretty clearly I'm getting schooled in here :)

@alex

This comment has been minimized.

Contributor

alex commented Mar 31, 2014

As a follow up, the issues in CryptGenRandom which PyCrypto tries to mitigate are no longer applicable on modern Windows. Vista and newer never had this problem, and on XP it's fixed with SP3.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Mar 31, 2014

That seems like a reasonable fallback to me, @offbyone - @alex?

EDIT: hrrrrgh thanks a lot github :( last thing I saw was @offbyone's first comment. reading...

EDIT 2: OK, that all sounds good, thanks a bundle @alex / @dstufft!

I'll read the actual code changes over sometime soon & merge this in - you guys know what you're doing and I've tooted a few times w/o anybody coming in to pushback. Works For Me.

@alex

This comment has been minimized.

Contributor

alex commented Mar 31, 2014

I updated the ECDSA code to not pass urandom, since that's already the default. The 2.7 builder broke though, I think it must be spurious, because the test that failed looks totally unrelated and all the other builders passed.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Mar 31, 2014

Yea I see one or two random errors pop up occasionally, suspect they're due to the recent Python 3 merge, but haven't seen them frequently enough to nail down yet. Almost definitely unrelated to your changes.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Apr 8, 2014

Deleted some comments. The perils of being at the nearly indistinguishable bottom of multiple Github ticket pages :(

Merge branch 'master' into use-urandom
Conflicts:
	paramiko/dsskey.py
	paramiko/ecdsakey.py
	paramiko/hostkeys.py
	paramiko/kex_gex.py
	paramiko/kex_group1.py
	paramiko/pkey.py
	paramiko/primes.py
	paramiko/rsakey.py
	tests/test_pkey.py

@bitprophet bitprophet merged commit 191fd46 into paramiko:master Apr 14, 2014

bitprophet added a commit that referenced this pull request Apr 14, 2014

@alex alex deleted the alex:use-urandom branch Apr 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment