msg284659 - (view) |
Author: STINNER Victor (vstinner) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2017-01-06 09:53 |
LGTM.
|
msg284808 - (view) |
Author: Roundup Robot (python-dev) |
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) * |
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) |
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) * |
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) * |
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) |
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) * |
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) * |
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) * |
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) * |
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) * |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:41 | admin | set | github: 73343 |
2017-02-21 16:28:02 | Vladimír Čunát | set | messages:
+ msg288310 |
2017-02-21 13:10:31 | christian.heimes | set | messages:
+ msg288295 |
2017-02-21 12:49:56 | Vladimír Čunát | set | messages:
+ msg288291 |
2017-02-16 22:03:49 | christian.heimes | set | messages:
+ msg287972 |
2017-02-16 21:45:01 | Vladimír Čunát | set | nosy:
+ Vladimír Čunát messages:
+ msg287971
|
2017-02-14 10:27:05 | vstinner | set | messages:
+ msg287762 |
2017-02-01 17:14:40 | vstinner | set | status: open -> closed resolution: fixed messages:
+ msg286660
|
2017-01-09 10:33:32 | vstinner | set | messages:
+ msg285035 |
2017-01-09 10:22:23 | python-dev | set | messages:
+ msg285032 |
2017-01-06 23:54:18 | vstinner | set | title: 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:02 | vstinner | set | files:
+ random-py35.patch nosy:
+ larry messages:
+ msg284872
|
2017-01-06 23:14:32 | vstinner | set | messages:
+ msg284871 |
2017-01-06 23:08:50 | python-dev | set | messages:
+ msg284870 |
2017-01-06 10:43:23 | vstinner | set | messages:
+ msg284809 |
2017-01-06 10:40:39 | python-dev | set | nosy:
+ python-dev messages:
+ msg284808
|
2017-01-06 09:53:55 | xiang.zhang | set | messages:
+ msg284805 |
2017-01-05 11:19:44 | christian.heimes | set | messages:
+ msg284733 |
2017-01-05 11:13:29 | vstinner | set | nosy:
+ serhiy.storchaka, xiang.zhang messages:
+ msg284730
|
2017-01-05 11:09:54 | ncoghlan | set | messages:
+ msg284727 |
2017-01-05 10:52:00 | vstinner | set | files:
+ random-2.patch
messages:
+ msg284723 |
2017-01-05 01:44:27 | ncoghlan | set | messages:
+ msg284687 |
2017-01-05 01:36:21 | ncoghlan | set | nosy:
+ ncoghlan
|
2017-01-04 17:34:05 | vstinner | set | messages:
+ msg284660 |
2017-01-04 17:33:01 | vstinner | create | |