Issue18747
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2013-08-15 12:35 by christian.heimes, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
openssl_prng_atfork.patch | christian.heimes, 2013-08-15 12:34 | review | ||
openssl_prng_atfork3.patch | christian.heimes, 2013-08-17 16:07 | review | ||
openssl_prng_atfork4.patch | christian.heimes, 2013-08-18 13:22 | review | ||
openssl_prng_atfork5.patch | christian.heimes, 2013-08-20 22:55 | review | ||
test.py | neologix, 2013-08-22 15:21 |
Messages (59) | |||
---|---|---|---|
msg195247 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-15 12:34 | |
A couple of reports and check-in messages like Postgres / pgcrypto CVE-2013-1900 http://bugs.ruby-lang.org/issues/4579 http://www.exim.org/lurker/message/20130402.171710.92f14a60.fi.html suggests that OpenSSL's PRNG should be reset or re-seeded after fork(). Otherwise child processes can generate the same or similar pseudo random values. Python doesn't have an API to run code before and after fork yet. The patch uses pthread_atfork() for the task. It's available on all pthread platforms -- which are all official supported platforms that have fork(), too. The patch doesn't use RAND_cleanup() like Postgres because child process would hav to initial the PRNG again by opening and reading from /dev/urandom. The atfork prepare hook pulls from random bytes from the PRNG and stores them in a static buffer. The child handler seeds the PRNG from that buffer + pid + current time. PID and current time are mixed into the state to extenuate race conditions. |
|||
msg195248 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2013-08-15 12:38 | |
Are there any uses of the OpenSSL PRNG from Python? Is the PRNG used for SSL session keys, or another mechanism? |
|||
msg195249 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-15 12:46 | |
The ssl module exposes OpenSSL's PRNG and advertises the API as secure CPRNG: http://docs.python.org/3/library/ssl.html#random-generation OpenSSL uses its own PRNG to create (amongst others) session keys for SSL connections. |
|||
msg195250 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2013-08-15 12:50 | |
> The ssl module exposes OpenSSL's PRNG and advertises the API as secure > CPRNG: http://docs.python.org/3/library/ssl.html#random-generation AFAICT, Python's PRNG isn't reset after fork, so I don't think OpenSSL's should be reset. OTOH, multiprocessing does reseed the random module after fork, so it should also do so for the ssl module if already loaded. We may add a note in the ssl docs stating that it's better to reseed after fork(). |
|||
msg195252 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-15 12:58 | |
Python doesn't have a builtin PRNG. We use the OS's CPRNG such as /dev/urandom or CryptGenRandom(). Both use a system wide state and are not affected by process state. OpenSSL's PRNG is different because it uses an internal state. AFAIK it only polls the system's entropy poll when the PRNG is used for the first time. It's not only multiprocessing. What about forking webservers etc. that use HTTPS? |
|||
msg195254 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2013-08-15 13:14 | |
> Python doesn't have a builtin PRNG. Of course it does. It's in the random module, and you can seed() it: >>> random.seed(5) >>> random.random() 0.6229016948897019 >>> random.random() 0.7417869892607294 >>> random.seed(5) >>> random.random() 0.6229016948897019 See e.g. issue12856. And the multiprocessing module: http://hg.python.org/cpython/file/92039fb68483/Lib/multiprocessing/forkserver.py#l195 > We use the OS's CPRNG such as /dev/urandom or CryptGenRandom(). "We"? > It's not only multiprocessing. What about forking webservers etc. > that use HTTPS? Well, are they affected? I haven't understood your previous answer. ("OpenSSL uses its own PRNG to create (amongst others) session keys for SSL connections") Note that it seems debatable whether it's an OpenSSL bug: http://www.openwall.com/lists/oss-security/2013/04/12/3 |
|||
msg195255 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-15 13:35 | |
Am 15.08.2013 15:14, schrieb Antoine Pitrou: >> Python doesn't have a builtin PRNG. > > Of course it does. It's in the random module, and you can seed() it: Now you are nit-picking. Although random is a PRNG, it is not a CPRNG. I'm clearly talking about the integrity of a CPRNG here. >> We use the OS's CPRNG such as /dev/urandom or CryptGenRandom(). > > "We"? Python's os.urandom() and _PyOS_URandom(). > Well, are they affected? I haven't understood your previous answer. > ("OpenSSL uses its own PRNG to create (amongst others) session keys for SSL connections") Yes, they are are affected. A forking HTTPS server does: 1) listen() on a TCP port 2) accept() new TCP connection 3) fork() a child process to handle request 4) do SSL handshake in the child process 5) read/write data in child process When the OpenSSL's CPRNG is already initialized before 3) than all child processes created by 3) will have almost the same PRNG state. According to http://bugs.ruby-lang.org/issues/4579 the PRNG will return the same value when PID numbers are recycled. > Note that it seems debatable whether it's an OpenSSL bug: > http://www.openwall.com/lists/oss-security/2013/04/12/3 It probably is an OpenSSL bug but the declaration doesn't help us. It's not the first time Python has to work around OpenSSL, e.g. #18709. |
|||
msg195256 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2013-08-15 13:44 | |
> When the OpenSSL's CPRNG is already initialized before 3) than all child > processes created by 3) will have almost the same PRNG state. According > to http://bugs.ruby-lang.org/issues/4579 the PRNG will return the same > value when PID numbers are recycled. Thanks. Here is some discussion of the reseeding strategy: http://marc.info/?l=openssl-dev&m=130432419329454&w=2 More precisely, instead of reseeding in the child, you can simply perturb the PRNG with a constant in the parent, to make sure the same PRNG state doesn't get re-used. |
|||
msg195264 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-08-15 17:44 | |
This issue was already discussed in the atfork issue: http://bugs.python.org/issue16500#msg179838 See also: http://www.openwall.com/lists/oss-security/2013/04/12/3 "I believe it is wrong to fix this in PostgreSQL. Rather, this is a bug in the OpenSSL fork protection code. It should either install a fork hook, or reseed the PRNG from /dev/urandom if a PID change is detected." |
|||
msg195265 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-08-15 17:45 | |
Another link: http://article.gmane.org/gmane.comp.encryption.openssl.user/48480/match=does+child+process+still+need+reseeded+after+fork |
|||
msg195488 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-17 16:07 | |
Here is a patch that is based on Apache's mod_ssl code. mod_ssl perturbs the PRNG state more often but I think that's overkill for Python. The new patch only affects the PRNG state of the child process. In my opinion it is the better way to solve this issue. The RAND API does some internal locking. Bad things might happen if one thread of a process calls fork() while another is in the middle of a locked RAND operation. |
|||
msg195500 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2013-08-17 18:21 | |
The patch looks fine on the principle, but you should add a test. |
|||
msg195501 - (view) | Author: Charles-François Natali (neologix) * | Date: 2013-08-17 18:30 | |
2013/8/17 Christian Heimes <report@bugs.python.org>: > Here is a patch that is based on Apache's mod_ssl code. mod_ssl perturbs the PRNG state more often but I think that's overkill for Python. > > The new patch only affects the PRNG state of the child process. In my opinion it is the better way to solve this issue. The RAND API does some internal locking. Bad things might happen if one thread of a process calls fork() while another is in the middle of a locked RAND operation. Ouch, this would mean making the interpreter prone to deadlock/crash (since the atfork hook is not async-signal safe) :-\ |
|||
msg195521 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-08-17 21:54 | |
openssl_prng_atfork3.patch: Why not using seconds (only micro or nanoseconds) in the seed? Add a few more bits should not reduce the entropy. OpenSSL does hash all these bytes anyway. +#if 1 + fprintf(stderr, "PySSL_RAND_atfork_child() seeds %i bytes in %i\n", + (int)sizeof(seed), seed.pid); +#endif This should be removed from the final patch ;-) The patch is specific to pthread. Do we need something similar on Windows. Windows has no fork, but I don't know if OpenSSL CPRNG state can be inherited somehow? Does Python support other platforms (other than pthread or Windows)? Instead of using pthread_atfork(), we can add an hook in the Python binding of OpenSSL checking the pid. I don't know which functions should be modified. ssl.RAND_bytes() is probably not enough :-) |
|||
msg195560 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-18 13:22 | |
The lastest patch mixes seconds into the time field of seed. Python 3.3+ support only NT and POSIX threads. 2.7 and 3.2 have GNU pth and other threading API but I neither have a system to test other threading libs nor the motivation to port my patch to an ancient threading library. It's not just RAND_bytes(). OpenSSL's PRNG is also used by OpenSSL internally, e.g. as entropy source for SSL/TLS handshakes. How many function do you want to patch? |
|||
msg195578 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-08-18 21:20 | |
PySSL_RAND_atfork_child(void) can be simplified by calling _PyTime_gettimeofday(). This function uses the most(*) accurate function and already implements fallbacks on error. (*) _PyTime_gettimeofday() does not use clock_gettime() because of a linker issue: Python is not linked to librt, which is required to get clock_gettime() on Linux with glibc < 2.17. So you get a few less bits of entropy, but does it really matter? If it matters, you have probably to use a better random of entropy than the system time... Anyway, it would be easier to move the _PyTime_timeval and similar structures into the seed structure instead of shifting bits. |
|||
msg195727 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-20 22:43 | |
Thanks Victor, it sounds like a good suggestion. I have modified my patch to use _PyTime_gettimeofday(). Python 2.7 doesn't have _PyTime_gettimeofday() so I'm going to use time(NULL). |
|||
msg195730 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-08-20 23:46 | |
openssl_prng_atfork5.patch: - you should use RAND_pseudo_bytes() instead of RAND_bytes() in the unit test, to not skip the test depending on the entropy - the change in Misc/NEWS is strange - please remove dead code ("#if 0") - PySSL_RAND_atfork_child() comment is wrong, it is no more able to get time with a nanosecond resolution (write "(microseconds or seconds)") - can you explain the choice of "char stack[72]" (72 bytes): why no more, why not less? |
|||
msg195774 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-08-21 11:43 | |
New changeset 8e1194c39bed by Christian Heimes in branch '3.3': Issue #18747: Re-seed OpenSSL's pseudo-random number generator after fork. http://hg.python.org/cpython/rev/8e1194c39bed New changeset 49e23a3adf26 by Christian Heimes in branch 'default': Issue #18747: Re-seed OpenSSL's pseudo-random number generator after fork. http://hg.python.org/cpython/rev/49e23a3adf26 New changeset 2e6aa6c29be2 by Christian Heimes in branch '2.7': Issue #18747: Re-seed OpenSSL's pseudo-random number generator after fork. http://hg.python.org/cpython/rev/2e6aa6c29be2 |
|||
msg195777 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-21 11:49 | |
I have taken care of Antoine's and Victor's reviews. The fix has landed in Python 2.7, 3.3 and 3.4. What about 2.6, 3.1 and 3.2? After all it's a security fix (although I don't consider its severity as high). |
|||
msg195781 - (view) | Author: Charles-François Natali (neologix) * | Date: 2013-08-21 12:08 | |
> Christian Heimes added the comment: > > I have taken care of Antoine's and Victor's reviews. The fix has landed in Python 2.7, 3.3 and 3.4. What about 2.6, 3.1 and 3.2? After all it's a security fix (although I don't consider its severity as high). There's still the #if 0 in the patch you committed. And basically, because PySSL_RAND_atfork_child() is not async-signal safe, the interpreter is now subject to random deadlocks/crash in multi-threaded processes. I personally don't consider this a security fix... |
|||
msg195782 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-08-21 12:08 | |
3.52 +#if 0 3.53 + fprintf(stderr, "PySSL_RAND_atfork_child() seeds %i bytes in pid %i\n", 3.54 + (int)sizeof(seed), seed.pid); 3.55 +#endif Don't you want to remove this debug code? 1.8 + def test_random_fork(self): 1.9 + status = ssl.RAND_status() 1.10 + if not status: 1.11 + self.fail("OpenSSL's PRNG has insufficient randomness") The test uses ssl.RAND_pseudo_bytes(), the check on ssl.RAND_status() must be removed. |
|||
msg195784 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-21 12:22 | |
Am 21.08.2013 14:08, schrieb Charles-François Natali: > And basically, because PySSL_RAND_atfork_child() is not async-signal > safe, the interpreter is now subject to random deadlocks/crash in > multi-threaded processes. I personally don't consider this a security > fix... Which part of the function is not async-signal safe? It doesn't interact with any file descriptors nor does it use any syscalls except for getpid() and time(). Python's _ssl module doesn't re-initialize its locks on fork. That's an outstanding issue that is not related to the change. |
|||
msg195786 - (view) | Author: Charles-François Natali (neologix) * | Date: 2013-08-21 12:45 | |
> Which part of the function is not async-signal safe? It doesn't interact > with any file descriptors nor does it use any syscalls except for > getpid() and time(). gettimeofday() and RAND_add() The functions which are guaranteed to be async-signal safe are listed here: https://www.securecoding.cert.org/confluence/display/seccode/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers And here's what David Butenhof says about pthread_atfork(): """ The real answer is that pthread_atfork() is a completely useless and stupid mechanism that was a well intentioned but ultimately pointless attempt to carve a "back door" solution out of an inherently insoluable design conflict. """ More here: https://groups.google.com/forum/#!msg/comp.programming.threads/ThHE32-vRsg/3j-YICgSQzoJ IMO it's really an openssl issue, and it should be fixed there (mainly because they are the only one in position of setuping a safe atfork-hook, like the glibc does for malloc & co). |
|||
msg195787 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-21 13:08 | |
Do you have a proposal for a better way to fix the issue? I don't think that we can hope for a fix from OpenSSL. |
|||
msg195788 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2013-08-21 13:23 | |
> Do you have a proposal for a better way to fix the issue? I don't > think that we can hope for a fix from OpenSSL. Instead of reseeding in the child, you can perturb the state in the parent after fork. As far as I understand, only the "child" callback in pthread_atfork() needs to be async-signal-safe: "It is suggested that programs that use fork() call an exec function very soon afterwards in the child process, thus resetting all states. In the meantime, only a short list of async-signal-safe library routines are promised to be available." http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_atfork.html |
|||
msg195796 - (view) | Author: Charles-François Natali (neologix) * | Date: 2013-08-21 14:46 | |
2013/8/21 Antoine Pitrou <report@bugs.python.org>: > Instead of reseeding in the child, you can perturb the state in the parent > after fork. > As far as I understand, only the "child" callback in pthread_atfork() needs to be async-signal-safe: That's not completely true: fork() is supposed to be async-signal safe: it can be called from a signal handler (I wouldn't do this, but that's allowed), but most notably, it can be called after another fork(). For example, let's say you have a multi-threaded process, and it fork()s. If you reseed from "parent" (right before or after fork), then it's safe. But once you're in the child process, the process state is "unsafe" until exec(), i.e. you can only call async-signal safe functions. So if you fork() again before exec() - which is common, e.g. if you use the double-fork() idiom for daemons - then you'll call the reseed in an unsafe context. In other words, the "parent" callback is called in the context of a "child" callback in the child process. So it's not really safe either, although the window is narrower. The only 100% safe way I can think of - apart from ignoring the problem - would be to check if the PID has changed upon entering ssl function (caching it to avoid calling getpid() everytime), keeping in mind that there will still be the risk of deadlock/crash anyway if the user enters the ssl library before exec()... Another, probably cleaner way would be to finally add the atfork() module (issue #16500), and register this reseed hook (which could then be implemented in ssl.py). forking() in a multi-threaded context is an unsolvable problem... |
|||
msg195797 - (view) | Author: Richard Oudkerk (sbt) * | Date: 2013-08-21 15:27 | |
On 21/08/2013 3:46pm, Charles-François Natali wrote: > Another, probably cleaner way would be to finally add the atfork() > module (issue #16500), and register this reseed hook (which could then > be implemented in ssl.py). Wouldn't that still suffer from the double-fork() issue? |
|||
msg195798 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2013-08-21 15:27 | |
> [snip insightful explanation] > So it's not really safe either, although the window is narrower. Gosh. Well, the narrower window would be fine with me :-) |
|||
msg195802 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-21 16:22 | |
Oh heck, signal, threads and fork really don't mix. :( Under which condition can a non-async safe function cause trouble? Is it just fork() inside a signal handler or can an incoming signal during fork() also cause havoc? The OpenSSL PRNG is only buggy when used in a forking application where the master process initializes the PRNG but never uses it. All child processes inherit the same state. OpenSSL tries to work around the problem by feeding the PID into the PRNG state. But as soon as PIDs get recycled, subsequent child processes get the same random numbers. Antoine's proposal works, too, because it perturbs the master's PRNG state regularly. |
|||
msg195804 - (view) | Author: Charles-François Natali (neologix) * | Date: 2013-08-21 16:45 | |
>> Another, probably cleaner way would be to finally add the atfork() >> module (issue #16500), and register this reseed hook (which could then >> be implemented in ssl.py). > > Wouldn't that still suffer from the double-fork() issue? Not for the "officially" supported way of creating child processes, subprocess, since it calls fork() + exec() from C. The problem with pthread_atfork() is that it would make the whole interpreter non-safe even for correct code paths that call fork() + exec(), which is not the case now. Not reseeding with pthread_atfork() or atfork module, but only when the ssl library is entered, just means that if a deadlock must occur, it will only occur if the process re-enters the ssl library after fork(). > Under which condition can a non-async safe function cause trouble? Is it just fork() inside a signal handler or can an incoming signal during fork() also cause havoc? Actually, it doesn't need a signal to cause havoc: imagine that thread A is inside an ssl function, holding a mutex protecting process-wide internal ssl data. Then, thread B forks: in the child process, the ssl mutex is already held (since the adress space is COW), so upon next ssl library call in the child, the process will deadlock (trying to acquire a mutex already held). That's an example with lock, but the same holds if the library uses a global/static variable, etc. As an approximation, sync-signal safe means "reentrant" (async-signal safe comes from the fact that such restrictions apply to signal handler, for the exact same reasons). So you can pretty much only use reentrant functions between fork() and exec() in a multi-threaded process (i.e. no malloc(), no strtok()...). Which basically means that you can't run Python code at all between fork() and exec() in a multi-threaded code. |
|||
msg195877 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-08-22 11:24 | |
New changeset 39c9dbed3aa1 by Christian Heimes in branch '3.3': Issue #18747: Use a parent atfork handler instead of a child atfork handler. http://hg.python.org/cpython/rev/39c9dbed3aa1 New changeset 25e05147d662 by Christian Heimes in branch 'default': Issue #18747: Use a parent atfork handler instead of a child atfork handler. http://hg.python.org/cpython/rev/25e05147d662 New changeset d542f905081a by Christian Heimes in branch '2.7': Issue #18747: Use a parent atfork handler instead of a child atfork handler. http://hg.python.org/cpython/rev/d542f905081a New changeset 6ec43643c54f by Christian Heimes in branch '3.3': Issue #18747: Update Misc/NEWS to reflect the latest changeset. http://hg.python.org/cpython/rev/6ec43643c54f New changeset 956261a143eb by Christian Heimes in branch 'default': Issue #18747: Update Misc/NEWS to reflect the latest changeset. http://hg.python.org/cpython/rev/956261a143eb New changeset 14490ced507e by Christian Heimes in branch '2.7': Issue #18747: Update Misc/NEWS to reflect the latest changeset. http://hg.python.org/cpython/rev/14490ced507e |
|||
msg195884 - (view) | Author: Vajrasky Kok (vajrasky) * | Date: 2013-08-22 13:10 | |
Some typos on last commit.... "current time (miliseconds or seconds) and an uninitialized arry." should be "current time (miliseconds or seconds) and an uninitialized array." "instead of a child handler because fork() is suppose to be async-signal" should be "instead of a child handler because fork() is supposed to be async-signal" "A pthread_atfork() parent handler is used to seeded the PRNG with pid, time" should be "A pthread_atfork() parent handler is used to seed the PRNG with pid, time" |
|||
msg195885 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-22 13:15 | |
Thanks, that's embarrassing. :( I'm going to install a spell checker ... |
|||
msg195887 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-08-22 13:20 | |
PySSL_RAND_atfork_parent() still uses getpid(). This number is not very random in the *parent* process :-) |
|||
msg195888 - (view) | Author: Charles-François Natali (neologix) * | Date: 2013-08-22 13:27 | |
> STINNER Victor added the comment: > > PySSL_RAND_atfork_parent() still uses getpid(). This number is not > very random in the *parent* process :-) :-) IMO this patch has been rushed in and should be reverted for now. It's still not async-signal safe, had typos, plus this problem noted by Victor. |
|||
msg195889 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-08-22 13:29 | |
> IMO this patch has been rushed in and should be reverted for now. Agreed. |
|||
msg195892 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-22 13:57 | |
Am 22.08.2013 15:20, schrieb STINNER Victor: > > STINNER Victor added the comment: > > PySSL_RAND_atfork_parent() still uses getpid(). This number is not > very random in the *parent* process :-) That's fine and doesn't diminish the properties of the PRNG. In fact the patch could use a hard coded value to perturb the PRNG. It's only important to modify the PRNG state of the *parent* process so that recycled PIDs of *child* processes don't lead to repeated pseudo-random values. PID, time and stack are just hard-to-guess properties of the process. That's all. |
|||
msg195893 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-08-22 14:08 | |
Yet another good blog post about the issue: OpenSSL PRNG Is Not (Really) Fork-safe http://emboss.github.io/blog/2013/08/21/openssl-prng-is-not-really-fork-safe/ |
|||
msg195903 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2013-08-22 15:20 | |
> IMO this patch has been rushed in and should be reverted for now. > It's still not async-signal safe, had typos, plus this problem noted > by Victor. That's not really a problem. You merely have to *perturb* the random state in the parent, so that the next child gets a different initial state. As pointed out in a mailing-list message, mixing in a constant could be enough to perturb the state. As for not being async-signal safe, it's only in the double fork() case, which is much less of an issue IMO. |
|||
msg195905 - (view) | Author: Charles-François Natali (neologix) * | Date: 2013-08-22 15:21 | |
>> PySSL_RAND_atfork_parent() still uses getpid(). This number is not >> very random in the *parent* process :-) > > That's fine and doesn't diminish the properties of the PRNG. In fact the > patch could use a hard coded value to perturb the PRNG. It's only > important to modify the PRNG state of the *parent* process so that > recycled PIDs of *child* processes don't lead to repeated pseudo-random > values. Yeah, it doesn't weaken the PRNG, but since we're using current time and stack content to reseed it, using the parent PID which doesn't change doesn't bring much (since we chose to add entropy and not just a constant, which would be sufficient as you noted). Anyway, for those interested, here's a reproducer. |
|||
msg195917 - (view) | Author: Barry A. Warsaw (barry) * | Date: 2013-08-22 20:38 | |
On Aug 21, 2013, at 11:49 AM, Christian Heimes wrote: >I have taken care of Antoine's and Victor's reviews. The fix has landed in >Python 2.7, 3.3 and 3.4. What about 2.6, 3.1 and 3.2? After all it's a >security fix (although I don't consider its severity as high). I'd like to get this into 2.6. I'll apply the patch after I fix my environment so it can actually build 2.6 with ssl enabled. I'll follow up here with status. |
|||
msg196051 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-08-23 22:37 | |
Oh i forgot that i wrote a patch for openssl: https://bitbucket.org/haypo/hasard/src/tip/patches/openssl_rand_fork.patch |
|||
msg196123 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-08-25 12:20 | |
New changeset 8b962e831ff0 by Christian Heimes in branch '3.3': Issue #18747: Fix spelling errors in my commit message and comments, http://hg.python.org/cpython/rev/8b962e831ff0 New changeset 7b1da249ab6d by Christian Heimes in branch 'default': Issue #18747: Fix spelling errors in my commit message and comments, http://hg.python.org/cpython/rev/7b1da249ab6d New changeset 3f30c281eb1c by Christian Heimes in branch '2.7': Issue #18747: Fix spelling errors in my commit message and comments, http://hg.python.org/cpython/rev/3f30c281eb1c |
|||
msg196856 - (view) | Author: Barry A. Warsaw (barry) * | Date: 2013-09-03 18:32 | |
blocker for 2.6.9 |
|||
msg197816 - (view) | Author: Barry A. Warsaw (barry) * | Date: 2013-09-15 19:21 | |
After further contemplation and without objection from __ap__ and Crys on IRC, I am de-targeting this for 2.6. I won't apply it for 2.6.9. |
|||
msg200344 - (view) | Author: Larry Hastings (larry) * | Date: 2013-10-19 01:19 | |
I can't follow all this. Is this considered fixed in 3.4 or not? |
|||
msg200413 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2013-10-19 12:56 | |
It's considered fixed but we have seen issues. The issues are tracked in #19227. AFAIK the fixed hasn't landed in 3.2 yet. I'm removing 2,7, 3.3 and 3.4 from the versions list. |
|||
msg200416 - (view) | Author: Charles-François Natali (neologix) * | Date: 2013-10-19 13:20 | |
Well, I don't mean to be a pain, but I still think this patch shouldn't have been applied: it makes fork() not async-safe, which can trigger hard to debug, random bugs (#19227 might or might not be a direct consequence). at the very least, I still it shouldn't have been backported. |
|||
msg201416 - (view) | Author: Georg Brandl (georg.brandl) * | Date: 2013-10-27 05:59 | |
#19227 must be solved before anything is backported to the security branches. |
|||
msg201663 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-10-29 20:18 | |
New changeset 5942eea8cf41 by Christian Heimes in branch '3.3': Issue #19227 / Issue #18747: Remove pthread_atfork() handler to remove OpenSSL re-seeding http://hg.python.org/cpython/rev/5942eea8cf41 New changeset cd4007fb9c7e by Christian Heimes in branch '3.3': Issue #18747: document issue with OpenSSL's CPRNG state and fork http://hg.python.org/cpython/rev/cd4007fb9c7e New changeset 705f2addd0f0 by Christian Heimes in branch 'default': Issue #19227 / Issue #18747: Remove pthread_atfork() handler to remove OpenSSL re-seeding http://hg.python.org/cpython/rev/705f2addd0f0 New changeset 4d761ce0ac74 by Christian Heimes in branch '2.7': Issue #19227 / Issue #18747: Remove pthread_atfork() handler to remove OpenSSL re-seeding http://hg.python.org/cpython/rev/4d761ce0ac74 New changeset 22e166d5c4c7 by Christian Heimes in branch '2.7': Issue #18747: document issue with OpenSSL's CPRNG state and fork http://hg.python.org/cpython/rev/22e166d5c4c7 |
|||
msg201664 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-10-29 20:24 | |
New changeset ad779da9e351 by Christian Heimes in branch '2.7': Issue #19227 / Issue #18747: Remove pthread_atfork() handler to remove OpenSSL re-seeding http://hg.python.org/cpython/rev/ad779da9e351 |
|||
msg203129 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-11-17 08:19 | |
New changeset 4e6aa98bb11c by Christian Heimes in branch '3.3': Issue #19227 / Issue #18747: Remove pthread_atfork() handler to remove OpenSSL re-seeding http://hg.python.org/cpython/rev/4e6aa98bb11c |
|||
msg213757 - (view) | Author: Jeffrey Walton (Jeffrey.Walton) * | Date: 2014-03-16 20:46 | |
> It probably is an OpenSSL bug but the declaration doesn't help us. > It's not the first time Python has to work around OpenSSL, e.g. #18709. Sorry to dig up an old issue. But here's some reading on it if interested. Ben Laurire pushed a patch to mix in PID and time. The PID was already being used, so the patch adds the time. See "Mixing time into the pool", http://www.mail-archive.com/openssl-dev@openssl.org/msg33012.html. And the commit: https://github.com/openssl/openssl/commit/3cd8547a2018ada88a4303067a2aa15eadc17f39. OpenSSL added a wiki page for reading on the subject: http://wiki.openssl.org/index.php/Random_fork-safety. |
|||
msg222212 - (view) | Author: Mark Lawrence (BreamoreBoy) * | Date: 2014-07-03 20:33 | |
Is this really an enhancement request or should it be a security issue? Which versions if any actually need work doing on them? |
|||
msg227893 - (view) | Author: Roundup Robot (python-dev) | Date: 2014-09-30 12:47 | |
New changeset bdf73458df5f by Christian Heimes in branch '3.2': Issue #18747: document issue with OpenSSL's CPRNG state and fork https://hg.python.org/cpython/rev/bdf73458df5f |
|||
msg332998 - (view) | Author: Gabriel Corona (Gabriel Corona) | Date: 2019-01-04 20:39 | |
Now that the default PRNG of the 'random' package is automatically reseeded at fork, wouldn't it make sense to reseed the OpenSSL seed as well? (At the same time the OpenSSL wiki states [1] that "The situation has changed greatly, starting with OpenSSL 1.1.1 which completely rewrote RNG. The concerns [of fork unsafety] do not really apply any more".) [1] https://wiki.openssl.org/index.php/Random_fork-safety |
|||
msg333016 - (view) | Author: STINNER Victor (vstinner) * | Date: 2019-01-04 22:53 | |
The issue is closed. If you want to change anything, please open a new issue. IMHO this issue is too long, it's better to start a fresh issue with up to date info, just mention this old issue: bpo-18747. |
|||
msg333031 - (view) | Author: Christian Heimes (christian.heimes) * | Date: 2019-01-05 00:58 | |
I have no plans to work on the issue any more. OpenSSL 1.1.1 has fixed the RNG issue with a new DRBG implementation. Eventually all platforms will move to 1.1.1 because it also provides TLS 1.3. In the mean time, application can work around the limitation by seeding OpenSSL by calling ssl.RAND_add(). |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:57:49 | admin | set | github: 62947 |
2019-01-05 00:58:17 | christian.heimes | set | messages: + msg333031 |
2019-01-04 22:53:52 | vstinner | set | messages: + msg333016 |
2019-01-04 20:39:37 | Gabriel Corona | set | nosy:
+ Gabriel Corona messages: + msg332998 |
2014-09-30 19:22:31 | berker.peksag | set | stage: commit review -> resolved |
2014-09-30 12:49:38 | georg.brandl | set | status: open -> closed resolution: fixed |
2014-09-30 12:47:30 | python-dev | set | messages: + msg227893 |
2014-09-30 12:11:34 | georg.brandl | set | versions: - Python 3.1 |
2014-07-03 20:33:05 | BreamoreBoy | set | nosy:
+ BreamoreBoy messages: + msg222212 |
2014-03-16 20:46:36 | Jeffrey.Walton | set | nosy:
+ Jeffrey.Walton messages: + msg213757 |
2013-11-17 08:19:34 | python-dev | set | messages: + msg203129 |
2013-10-29 20:24:34 | python-dev | set | messages: + msg201664 |
2013-10-29 20:18:47 | python-dev | set | messages: + msg201663 |
2013-10-27 06:42:35 | Arfrever | set | nosy:
+ Arfrever |
2013-10-27 05:59:44 | georg.brandl | set | dependencies:
+ test_multiprocessing_xxx hangs under Gentoo buildbots messages: + msg201416 |
2013-10-19 13:20:16 | neologix | set | messages: + msg200416 |
2013-10-19 12:56:08 | christian.heimes | set | messages:
+ msg200413 versions: - Python 2.7, Python 3.3, Python 3.4 |
2013-10-19 01:19:32 | larry | set | messages: + msg200344 |
2013-09-15 19:21:18 | barry | set | messages:
+ msg197816 versions: - Python 2.6 |
2013-09-03 18:32:28 | barry | set | priority: normal -> release blocker nosy: + larry messages: + msg196856 |
2013-08-25 12:20:15 | python-dev | set | messages: + msg196123 |
2013-08-23 22:37:14 | vstinner | set | messages: + msg196051 |
2013-08-22 20:38:28 | barry | set | messages: + msg195917 |
2013-08-22 15:21:27 | neologix | set | files:
+ test.py messages: + msg195905 |
2013-08-22 15:20:15 | pitrou | set | messages: + msg195903 |
2013-08-22 14:08:30 | christian.heimes | set | messages: + msg195893 |
2013-08-22 13:57:56 | christian.heimes | set | messages: + msg195892 |
2013-08-22 13:29:10 | vstinner | set | messages: + msg195889 |
2013-08-22 13:27:53 | neologix | set | messages: + msg195888 |
2013-08-22 13:20:20 | vstinner | set | messages: + msg195887 |
2013-08-22 13:15:47 | christian.heimes | set | messages: + msg195885 |
2013-08-22 13:10:17 | vajrasky | set | nosy:
+ vajrasky messages: + msg195884 |
2013-08-22 11:24:13 | python-dev | set | messages: + msg195877 |
2013-08-21 16:45:23 | neologix | set | messages: + msg195804 |
2013-08-21 16:22:22 | christian.heimes | set | messages: + msg195802 |
2013-08-21 15:27:45 | pitrou | set | messages: + msg195798 |
2013-08-21 15:27:33 | sbt | set | messages: + msg195797 |
2013-08-21 14:46:08 | neologix | set | messages: + msg195796 |
2013-08-21 13:23:43 | pitrou | set | messages: + msg195788 |
2013-08-21 13:08:55 | christian.heimes | set | messages: + msg195787 |
2013-08-21 12:45:14 | neologix | set | messages: + msg195786 |
2013-08-21 12:22:55 | christian.heimes | set | messages: + msg195784 |
2013-08-21 12:08:35 | vstinner | set | messages: + msg195782 |
2013-08-21 12:08:26 | neologix | set | messages: + msg195781 |
2013-08-21 11:49:05 | christian.heimes | set | versions:
+ Python 2.6, Python 3.1, Python 2.7, Python 3.2, Python 3.3 nosy: + barry, georg.brandl, benjamin.peterson messages: + msg195777 stage: patch review -> commit review |
2013-08-21 11:43:29 | python-dev | set | nosy:
+ python-dev messages: + msg195774 |
2013-08-20 23:46:41 | vstinner | set | messages: + msg195730 |
2013-08-20 22:55:33 | christian.heimes | set | files: + openssl_prng_atfork5.patch |
2013-08-20 22:55:08 | christian.heimes | set | files: - openssl_prng_atfork5.patch |
2013-08-20 22:43:33 | christian.heimes | set | files:
+ openssl_prng_atfork5.patch messages: + msg195727 |
2013-08-18 21:20:37 | vstinner | set | messages: + msg195578 |
2013-08-18 13:22:02 | christian.heimes | set | files:
+ openssl_prng_atfork4.patch messages: + msg195560 |
2013-08-17 21:54:28 | vstinner | set | messages: + msg195521 |
2013-08-17 18:30:20 | neologix | set | messages: + msg195501 |
2013-08-17 18:21:58 | pitrou | set | messages: + msg195500 |
2013-08-17 16:07:19 | christian.heimes | set | files:
+ openssl_prng_atfork3.patch messages: + msg195488 |
2013-08-15 17:45:45 | vstinner | set | messages: + msg195265 |
2013-08-15 17:44:04 | vstinner | set | messages: + msg195264 |
2013-08-15 13:44:51 | pitrou | set | messages: + msg195256 |
2013-08-15 13:35:18 | christian.heimes | set | messages: + msg195255 |
2013-08-15 13:14:06 | pitrou | set | messages: + msg195254 |
2013-08-15 12:58:30 | christian.heimes | set | messages: + msg195252 |
2013-08-15 12:50:23 | pitrou | set | messages: + msg195250 |
2013-08-15 12:46:30 | christian.heimes | set | messages: + msg195249 |
2013-08-15 12:43:44 | pitrou | set | nosy:
+ neologix |
2013-08-15 12:38:03 | pitrou | set | versions:
- Python 2.6, Python 3.1, Python 2.7, Python 3.2, Python 3.3 nosy: + pitrou, sbt messages: + msg195248 type: security -> enhancement |
2013-08-15 12:35:01 | christian.heimes | create |