classification
Title: PEP 524: Make os.urandom() blocking on Linux
Type: security Stage:
Components: Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Decorater, ncoghlan, python-dev, vstinner
Priority: normal Keywords: patch

Created on 2016-08-16 12:55 by vstinner, last changed 2016-09-07 04:30 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
urandom_nonblock.patch vstinner, 2016-08-16 16:53 review
urandom_nonblock-2.patch vstinner, 2016-08-19 12:28 review
Messages (14)
msg272852 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-16 12:55
Issue to track the implementation of the PEP 524.
msg272853 - (view) Author: Decorater (Decorater) * Date: 2016-08-16 13:08
Wow. ( linux only pep? inb4 a windows thing gets wedged in)
msg272855 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-16 13:25
New changeset 980e2c781810 by Victor Stinner in branch 'default':
Issue #27776: Cleanup random.c
https://hg.python.org/cpython/rev/980e2c781810

New changeset 265644bad99e by Victor Stinner in branch 'default':
Issue #27776: _PyRandom_Init() doesn't call PyErr_CheckSignals() anymore
https://hg.python.org/cpython/rev/265644bad99e
msg272864 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-16 16:47
New changeset 86d0d74bc2e1 by Victor Stinner in branch 'default':
Issue #27776: Cleanup random.c
https://hg.python.org/cpython/rev/86d0d74bc2e1

New changeset ad141164c792 by Victor Stinner in branch 'default':
Issue #27776: dev_urandom(raise=0) now closes the file descriptor on error
https://hg.python.org/cpython/rev/ad141164c792
msg272865 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-16 16:53
Patch to make os.urandom() blocking on Linux 3.17+, but use non-blocking urandom in _random.Random constructor and _random.Random.seed() with no seed is set.
msg273025 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-08-18 13:02
I have a few requests for clarification and confirmation as review comments, but overall +1 from me.

(I'd still like a warning when we need to block in order to make life easier for system administrators attempting to debug any apparent system hangs, but as per the security-sig discussion, I can pursue that in a follow-up RFE and a separate patch, while this patch implements the PEP precisely as accepted)
msg273111 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-19 12:28
Enhanced patch to address Nick's comments and fix mistakes. The new patch now also updates the documentation.

I restored the code in _random.Random.seed() to fallback on the system clock: _PyOS_URandomNonblock() *can* fail is /dev/urandom is missing or not readable. I enhanced this part to not only read the system clock, but also use the current process identifier and get also the monotonic clock. Moreover, 64 bits are now used instead of 32 bits from the system clock (use a resolution of 1 nanoscond, not only 1 second).

I didn't test yet the fall back on clocks/pid. It should be tested manually by modifying _PyOS_URandomNonblock() to always fail.
msg273112 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-19 12:33
> _PyOS_URandomNonblock() *can* fail is /dev/urandom is missing or not readable

Oh. It looks like Python initialization currently fails with a fatal error in this case, see _PyRandom_Init().

Maybe we should also fall back on clocks/pid in _PyRandom_Init()?
msg273232 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-08-20 17:54
+1 for a fallback in the SIPHash initialisation as well.

That's the case where Nathaniel Smith suggested we may want to issue a warning that the process shouldn't be used to handle untrusted inputs (since that particular remote DoS defence won't be working properly), but the monotonic time + the PID should be sufficiently unpredictable seeding for that case (since there are plenty of lower hanging fruit for attackers to go after).

For testing, is there some way we could integrate an automated test of the deliberately misbehaving _PyOS_UrandomNonBlock into the testembed helper? If we can come up with a sensible way to do that, it could potentially help with testing the os.getrandom() BlockingIOError generation as well.
msg274668 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-06 23:37
New changeset 45fc0c83ed42 by Victor Stinner in branch 'default':
os.urandom() now blocks on Linux
https://hg.python.org/cpython/rev/45fc0c83ed42
msg274670 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-09-06 23:43
Nick: "+1 for a fallback in the SIPHash initialisation as well."

Sorry but I don't know a simple function to implement this. We might use the LCG RNG, but it's not really designed to be "secure". I don't think that it makes sense to initialize a shiny SIPHash with a crappy LCG RNG :-)

So I skip my turn on this idea and let others implement them if anyone consider that it's worth it.

To be clear: Python 3 doesn't start when getrandom() and /dev/urandom are not available or don't work, but it's not something new. Python 3.1 already starts with:

    fd = open("/dev/urandom", O_RDONLY);
    if (fd < 0)
        Py_FatalError("Failed to open /dev/urandom");

--

os.urandom() is now blocking, I close the issue.
msg274680 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-07 00:04
New changeset ebbfc053360a by Victor Stinner in branch 'default':
Issue #27776: include process.h on Windows for getpid()
https://hg.python.org/cpython/rev/ebbfc053360a
msg274707 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-07 02:27
If /dev/urandom isn't available, Python refusing to start is likely to be one of the least of the system's problems, so Py_FatalError sounds reasonable to me - my +1 for a fallback above was a matter of "sounds good if you can find a way to make it work".

Thanks for all your work on getting this designed and implemented, Victor!
msg274755 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-07 04:30
I've reviewed all the open issues that come up when searching for "getrandom" on the issue tracker, and closed all the ones that were either out of date or rejected based on PEP 524 being accepted and implemented.

For the remainder, I either wasn't clear on whether they could be closed or not (in which case I posted a comment asking Victor to take a look at them), or else they were clearly still valid and I posted a relevant status update.

Doing a similar search for "urandom", I checked the ones where the titles seemed relevant and commented where a status update and possible closure seemed appropriate.
History
Date User Action Args
2016-09-07 04:30:09ncoghlansetmessages: + msg274755
2016-09-07 02:27:41ncoghlansetmessages: + msg274707
2016-09-07 00:04:00python-devsetmessages: + msg274680
2016-09-06 23:43:59vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg274670
2016-09-06 23:37:44python-devsetmessages: + msg274668
2016-08-20 17:54:45ncoghlansetmessages: + msg273232
2016-08-19 12:33:03vstinnersetmessages: + msg273112
2016-08-19 12:28:58vstinnersetfiles: + urandom_nonblock-2.patch

messages: + msg273111
2016-08-18 13:02:13ncoghlansetnosy: + ncoghlan
messages: + msg273025
2016-08-16 16:53:02vstinnersetfiles: + urandom_nonblock.patch
keywords: + patch
messages: + msg272865
2016-08-16 16:47:57python-devsetmessages: + msg272864
2016-08-16 13:25:02python-devsetnosy: + python-dev
messages: + msg272855
2016-08-16 13:08:31Decoratersetnosy: + Decorater
messages: + msg272853
2016-08-16 12:55:06vstinnercreate