classification
Title: random.c: Prefer getrandom() over getentropy() to support glibc 2.24 on Linux
Type: security Stage:
Components: Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Vladimír Čunát, christian.heimes, larry, ncoghlan, python-dev, serhiy.storchaka, vstinner, xiang.zhang
Priority: normal Keywords: patch

Created on 2017-01-04 17:33 by vstinner, last changed 2017-02-21 16:28 by Vladimír Čunát. This issue is now closed.

Files
File name Uploaded Description Edit
getentropy.patch vstinner, 2017-01-04 17:33 review
random-2.patch vstinner, 2017-01-05 10:51 review
random-py35.patch vstinner, 2017-01-06 23:39 review
Messages (22)
msg284659 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-04 17:33
A new getentropy() function was recently added to the glibc:
https://sourceware.org/bugzilla/show_bug.cgi?id=17252

When the Python/random.c file was written (by me), the getentropy() function was only supported on OpenBSD. Later, random.c was modified to *not* use getentropy() on Solaris (Issue #25003).

The problem is that py_getentropy() doesn't handle ENOSYS, and so Python fails at startup with a fatal error (Python 3.6):
   Fatal Python error: failed to get random numbers to initialize Python
or (Python 3.5):
   Fatal Python error: getentropy() failed

The bug was first reported in Fedora 26 (rawhide):
https://bugzilla.redhat.com/show_bug.cgi?id=1410175

Attached patch (written for the default branch) should fix these issues:

* Prefer getrandom() syscall over getentropy() function: getrandom() supports blocking and non-blocking mode on Linux, whereas getentropy() doesn't
* Enhance py_getentropy() to handle ENOSYS: fallback on reading from /dev/urandom and remember that the function doesn't work

I'm not sure that handling ENOSYS is required, since it's no more used on Linux, but it shouldn't hurt. I don't know if py_getentropy() should also handle EPERM?

py_getrandom() catchs errors: EAGAIN, EINTR, EPERM and ENOSYS.

With the patch, py_getentropy() catchs ENOSYS error.
msg284660 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-04 17:34
Python 2.7, 3.5, 3.6 and 3.7 are impacted: they should fail on Linux if compiled with a recent glibc which has getentropy().
msg284687 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-01-05 01:44
Aside from a couple of outdated comments and the EPERM question, the attached patch looks good to me.

Regarding EPERM, I think it would make sense to make py_getrandom and py_getentropy handle that consistently, otherwise I can see future maintainers readily getting confused by the discrepancy.
msg284723 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-05 10:51
New patch (version 2), much larger: it refactors the code, not only fix this specific issue (prefer getrandom() over getentropy()). Changes since getentropy.patch:

* Add a lot of comments to explain in depth how each function is implemented, which errors are handled, etc. It should help to audit the code: this code is very critical for security and so should be, IMHO, well documented.

* handle also EPERM and EINTR errors in getentropy(): retry on EINTR, fallback on /dev/urandom on EPERM -- sadly, I don't have access to a system with getentropy() to test this part of the code.

* call py_getrandom() and py_getentropy() in pyurandom() to make dev_urandom() simpler and so easy to review: dev_urandom() looses its blocking parameter

* Document the cached file descriptor, and cached st_dev+st_ino in dev_urandom().

* Document explicitly that functions are retried on EINTR error. Document that only getrandom() supports non-blocking mode. Document why we prefer an entropy source over others.

I'm not sure that getentropy() can fail with EPERM or EINTR in practice, but it shouldn't harm to handle correctly these errors :-) At least, getentropy() can fail with these errors on Linux since the glibc implements the getentropy() function using the getrandom() syscall (and it's known that getrandom() can fail with these errors). But on Linux, the code now prefers getrandom() over getentropy().

Should we use the new shiny code on all Python versions? Or only fix the reported issue on all Python issues, and use the refactored code in Python default?

Note: Python 2.7 still supports VMS. VMS is unsupported in Python 3.3 and the VMS code was removed in Python 3.4 (issue 16136): see the PEP 11.

I suggest to use the same code on all maintained Python versions to ease maintenance.
msg284727 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-01-05 11:09
New patch looks good to me, and +1 on applying the refactoring to all supported branches.
msg284730 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-05 11:13
Nick: Thanks for the review!

Since random.c is critical for security, I would prefer to have at least a review from another core developer. I added Serhiy and Xiang in the nosy list. I'm also looking at you, Christian! ;-) (Christian reused random.c code in the pycrytography project.)
msg284733 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-01-05 11:19
I'm doing a review now.

By the way I did not copy random.c for cryptography. I took bits and pieces out of it. https://github.com/pyca/cryptography/blob/master/src/_cffi_src/openssl/src/osrandom_engine.c and https://github.com/pyca/cryptography/blob/master/src/_cffi_src/openssl/src/osrandom_engine.h
msg284805 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-01-06 09:53
LGTM.
msg284808 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-01-06 10:40
New changeset 140f0459fe80 by Victor Stinner in branch 'default':
Issue #29157: dev_urandom() now calls py_getentropy()
https://hg.python.org/cpython/rev/140f0459fe80

New changeset 69b23952d122 by Victor Stinner in branch 'default':
Issue #29157: Simplify dev_urandom()
https://hg.python.org/cpython/rev/69b23952d122

New changeset 46ca697c6f26 by Victor Stinner in branch 'default':
Issue #29157: getrandom() is now preferred over getentropy()
https://hg.python.org/cpython/rev/46ca697c6f26

New changeset 4c11a01fa881 by Victor Stinner in branch 'default':
py_getentropy() now supports ENOSYS, EPERM & EINTR
https://hg.python.org/cpython/rev/4c11a01fa881

New changeset 4edd6cbf9abf by Victor Stinner in branch 'default':
Issue #29157: enhance py_getrandom() documentation
https://hg.python.org/cpython/rev/4edd6cbf9abf
msg284809 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-06 10:43
Thanks Nick and Xiang for the review. I splitted again the giant change into small atomic changes, easier to review and understand.

Right now, I only applied the change to default (Python 3.7). I will now wait for buildbots before considering backporting the change.
msg284870 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-01-06 23:08
New changeset f8e24a0a1124 by Victor Stinner in branch '3.6':
Issue #29157: Prefer getrandom() over getentropy()
https://hg.python.org/cpython/rev/f8e24a0a1124
msg284871 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-06 23:14
Christian Heimes: "I'm doing a review now."

Follow-up on #python-dev (IRC):

<Crys> haypo: yes, I looked at the patch and did not see any obvious problem with it. Didn't I tell you?
<Crys> haypo: maybe I forgot :)
msg284872 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-06 23:39
random-py35.patch: Patch for the 3.5 branch. My prepared commit message:
---
Issue #29157: Prefer getrandom() over getentropy()

Copy and then adapt Python/random.c from default branch. Difference between 3.5
and default branches:

* Python 3.5 only uses getrandom() in non-blocking mode: flags=GRND_NONBLOCK
* If getrandom() fails with EAGAIN: py_getrandom() immediately fails and
  remembers that getrandom() doesn't work.
* Python 3.5 has no _PyOS_URandomNonblock() function: _PyOS_URandom()
  works in non-blocking mode on Python 3.5
---

It seems like Python 3.5 is close to a release, I prefer to wait after the release to fix this issue. I don't think that many Linux distributions are affected, since the issue only occurs with glibc 2.24 which is very recent.

@Larry: Do you want this change in Python 3.5.3? The change is quite large.
msg285032 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-01-09 10:22
New changeset 8125d9a8152b by Victor Stinner in branch '3.5':
Issue #29157: Prefer getrandom() over getentropy()
https://hg.python.org/cpython/rev/8125d9a8152b
msg285035 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-09 10:33
> It seems like Python 3.5 is close to a release, I prefer to wait after the release to fix this issue. (...) @Larry: Do you want this change in Python 3.5.3? The change is quite large.

Ah, I was wrong: 3.5 was already open for the next 3.5.4 release, so I pushed my change. Again, I don't think that supporting the glibc 2.24 in Python 3.5 is a bug important enough to post-pone a release.
msg286660 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-02-01 17:14
I fixed the bug in Python 2.7, 3.5, 3.6 and 3.7 (default). Thanks for the reviews Nick & Christian!
msg287762 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-02-14 10:27
Python 2.7, see issue #29188: "Backport random.c from Python 3.5 to Python 2.7". Mercurial commit 13a39142c047, Git commit 01bdbad3e951014c58581635b94b22868537901c.

https://github.com/python/cpython/commit/01bdbad3e951014c58581635b94b22868537901c
msg287971 - (view) Author: Vladimír Čunát (Vladimír Čunát) Date: 2017-02-16 21:45
Nitpick: here you always state that it's about glibc-2.24 but I'm very confident it only started with 2.25.
msg287972 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-02-16 22:03
Some vendors backport security features to older glibc.
msg288291 - (view) Author: Vladimír Čunát (Vladimír Čunát) Date: 2017-02-21 12:49
Why was there no backporting e.g. to 3.4 branch?  I thought you implied that 3.3 and 3.4 are unaffected, but our build farm says otherwise, getting a segfault in bin/python3.4m (3.4.6) and printing "Fatal Python error: getentropy() failed"
msg288295 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-02-21 13:10
Python 3.3 and 3.4 are in security-fix-only mode. Technically this fix does not count as security fix. It's a compatibility fix.

https://docs.python.org/devguide/#status-of-python-branches
msg288310 - (view) Author: Vladimír Čunát (Vladimír Čunát) Date: 2017-02-21 16:28
Thanks, I see now.  From my point of view it is related to security, but I/we will deal with it somehow.
History
Date User Action Args
2017-02-21 16:28:02Vladimír Čunátsetmessages: + msg288310
2017-02-21 13:10:31christian.heimessetmessages: + msg288295
2017-02-21 12:49:56Vladimír Čunátsetmessages: + msg288291
2017-02-16 22:03:49christian.heimessetmessages: + msg287972
2017-02-16 21:45:01Vladimír Čunátsetnosy: + Vladimír Čunát
messages: + msg287971
2017-02-14 10:27:05vstinnersetmessages: + msg287762
2017-02-01 17:14:40vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg286660
2017-01-09 10:33:32vstinnersetmessages: + msg285035
2017-01-09 10:22:23python-devsetmessages: + msg285032
2017-01-06 23:54:18vstinnersettitle: random.c: Prefer getrandom() over getentropy(), handle ENOSYS in py_getentropy() -> random.c: Prefer getrandom() over getentropy() to support glibc 2.24 on Linux
2017-01-06 23:39:02vstinnersetfiles: + random-py35.patch
nosy: + larry
messages: + msg284872

2017-01-06 23:14:32vstinnersetmessages: + msg284871
2017-01-06 23:08:50python-devsetmessages: + msg284870
2017-01-06 10:43:23vstinnersetmessages: + msg284809
2017-01-06 10:40:39python-devsetnosy: + python-dev
messages: + msg284808
2017-01-06 09:53:55xiang.zhangsetmessages: + msg284805
2017-01-05 11:19:44christian.heimessetmessages: + msg284733
2017-01-05 11:13:29vstinnersetnosy: + serhiy.storchaka, xiang.zhang
messages: + msg284730
2017-01-05 11:09:54ncoghlansetmessages: + msg284727
2017-01-05 10:52:00vstinnersetfiles: + random-2.patch

messages: + msg284723
2017-01-05 01:44:27ncoghlansetmessages: + msg284687
2017-01-05 01:36:21ncoghlansetnosy: + ncoghlan
2017-01-04 17:34:05vstinnersetmessages: + msg284660
2017-01-04 17:33:01vstinnercreate