random.c: Prefer getrandom() over getentropy() to support glibc 2.24 on Linux

[vstinner]

BPO	29157
Nosy	@ncoghlan, @vstinner, @larryhastings, @tiran, @serhiy-storchaka, @zhangyangyu
Files	getentropy.patch random-2.patch random-py35.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-02-01.17:14:40.554>
created_at = <Date 2017-01-04.17:33:01.185>
labels = ['type-security', '3.7']
title = 'random.c: Prefer getrandom() over getentropy() to support glibc 2.24 on Linux'
updated_at = <Date 2017-02-21.16:28:02.541>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2017-02-21.16:28:02.541>
actor = 'Vladim\xc3\xadr \xc4\x8cun\xc3\xa1t'
assignee = 'none'
closed = True
closed_date = <Date 2017-02-01.17:14:40.554>
closer = 'vstinner'
components = []
creation = <Date 2017-01-04.17:33:01.185>
creator = 'vstinner'
dependencies = []
files = ['46144', '46157', '46181']
hgrepos = []
issue_num = 29157
keywords = ['patch']
message_count = 22.0
messages = ['284659', '284660', '284687', '284723', '284727', '284730', '284733', '284805', '284808', '284809', '284870', '284871', '284872', '285032', '285035', '286660', '287762', '287971', '287972', '288291', '288295', '288310']
nosy_count = 8.0
nosy_names = ['ncoghlan', 'vstinner', 'larry', 'christian.heimes', 'python-dev', 'serhiy.storchaka', 'xiang.zhang', 'Vladim\xc3\xadr \xc4\x8cun\xc3\xa1t']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue29157'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']

[vstinner]

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 bpo-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.

[vstinner]

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().

[ncoghlan]

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.

[vstinner]

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 (bpo-16136): see the PEP-11.

I suggest to use the same code on all maintained Python versions to ease maintenance.

[ncoghlan]

New patch looks good to me, and +1 on applying the refactoring to all supported branches.

[vstinner]

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.)

[tiran]

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

[zhangyangyu]

LGTM.

[python-dev]

New changeset 140f0459fe80 by Victor Stinner in branch 'default':
Issue bpo-29157: dev_urandom() now calls py_getentropy()
https://hg.python.org/cpython/rev/140f0459fe80

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

New changeset 46ca697c6f26 by Victor Stinner in branch 'default':
Issue bpo-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 bpo-29157: enhance py_getrandom() documentation
https://hg.python.org/cpython/rev/4edd6cbf9abf

[vstinner]

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.

[python-dev]

New changeset f8e24a0a1124 by Victor Stinner in branch '3.6':
Issue bpo-29157: Prefer getrandom() over getentropy()
https://hg.python.org/cpython/rev/f8e24a0a1124

[vstinner]

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 :)

[vstinner]

random-py35.patch: Patch for the 3.5 branch. My prepared commit message:
---
Issue bpo-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.

[python-dev]

New changeset 8125d9a8152b by Victor Stinner in branch '3.5':
Issue bpo-29157: Prefer getrandom() over getentropy()
https://hg.python.org/cpython/rev/8125d9a8152b

[vstinner]

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.

[vstinner]

I fixed the bug in Python 2.7, 3.5, 3.6 and 3.7 (default). Thanks for the reviews Nick & Christian!

[vstinner]

Python 2.7, see issue bpo-29188: "Backport random.c from Python 3.5 to Python 2.7". Mercurial commit 13a39142c047, Git commit 01bdbad.

01bdbad

[Vladimrunt]

Nitpick: here you always state that it's about glibc-2.24 but I'm very confident it only started with 2.25.

[tiran]

Some vendors backport security features to older glibc.

[Vladimrunt]

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"

[tiran]

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

[Vladimrunt]

Thanks, I see now. From my point of view it is related to security, but I/we will deal with it somehow.