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.

classification
Title: Re-seed OpenSSL's PRNG after fork
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.2
process
Status: closed Resolution: fixed
Dependencies: 19227 Superseder:
Assigned To: Nosy List: Arfrever, BreamoreBoy, Gabriel Corona, Jeffrey.Walton, barry, benjamin.peterson, christian.heimes, georg.brandl, larry, neologix, pitrou, python-dev, sbt, vajrasky, vstinner
Priority: release blocker Keywords: patch

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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) Date: 2013-08-22 13:15
Thanks, that's embarrassing. :( I'm going to install a spell checker ...
msg195887 - (view) Author: STINNER Victor (vstinner) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) Date: 2013-09-03 18:32
blocker for 2.6.9
msg197816 - (view) Author: Barry A. Warsaw (barry) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) (Python triager) 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) (Python triager) 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) (Python triager) 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) (Python triager) 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) * (Python committer) 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) * (Python committer) 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:49adminsetgithub: 62947
2019-01-05 00:58:17christian.heimessetmessages: + msg333031
2019-01-04 22:53:52vstinnersetmessages: + msg333016
2019-01-04 20:39:37Gabriel Coronasetnosy: + Gabriel Corona
messages: + msg332998
2014-09-30 19:22:31berker.peksagsetstage: commit review -> resolved
2014-09-30 12:49:38georg.brandlsetstatus: open -> closed
resolution: fixed
2014-09-30 12:47:30python-devsetmessages: + msg227893
2014-09-30 12:11:34georg.brandlsetversions: - Python 3.1
2014-07-03 20:33:05BreamoreBoysetnosy: + BreamoreBoy
messages: + msg222212
2014-03-16 20:46:36Jeffrey.Waltonsetnosy: + Jeffrey.Walton
messages: + msg213757
2013-11-17 08:19:34python-devsetmessages: + msg203129
2013-10-29 20:24:34python-devsetmessages: + msg201664
2013-10-29 20:18:47python-devsetmessages: + msg201663
2013-10-27 06:42:35Arfreversetnosy: + Arfrever
2013-10-27 05:59:44georg.brandlsetdependencies: + test_multiprocessing_xxx hangs under Gentoo buildbots
messages: + msg201416
2013-10-19 13:20:16neologixsetmessages: + msg200416
2013-10-19 12:56:08christian.heimessetmessages: + msg200413
versions: - Python 2.7, Python 3.3, Python 3.4
2013-10-19 01:19:32larrysetmessages: + msg200344
2013-09-15 19:21:18barrysetmessages: + msg197816
versions: - Python 2.6
2013-09-03 18:32:28barrysetpriority: normal -> release blocker
nosy: + larry
messages: + msg196856

2013-08-25 12:20:15python-devsetmessages: + msg196123
2013-08-23 22:37:14vstinnersetmessages: + msg196051
2013-08-22 20:38:28barrysetmessages: + msg195917
2013-08-22 15:21:27neologixsetfiles: + test.py

messages: + msg195905
2013-08-22 15:20:15pitrousetmessages: + msg195903
2013-08-22 14:08:30christian.heimessetmessages: + msg195893
2013-08-22 13:57:56christian.heimessetmessages: + msg195892
2013-08-22 13:29:10vstinnersetmessages: + msg195889
2013-08-22 13:27:53neologixsetmessages: + msg195888
2013-08-22 13:20:20vstinnersetmessages: + msg195887
2013-08-22 13:15:47christian.heimessetmessages: + msg195885
2013-08-22 13:10:17vajraskysetnosy: + vajrasky
messages: + msg195884
2013-08-22 11:24:13python-devsetmessages: + msg195877
2013-08-21 16:45:23neologixsetmessages: + msg195804
2013-08-21 16:22:22christian.heimessetmessages: + msg195802
2013-08-21 15:27:45pitrousetmessages: + msg195798
2013-08-21 15:27:33sbtsetmessages: + msg195797
2013-08-21 14:46:08neologixsetmessages: + msg195796
2013-08-21 13:23:43pitrousetmessages: + msg195788
2013-08-21 13:08:55christian.heimessetmessages: + msg195787
2013-08-21 12:45:14neologixsetmessages: + msg195786
2013-08-21 12:22:55christian.heimessetmessages: + msg195784
2013-08-21 12:08:35vstinnersetmessages: + msg195782
2013-08-21 12:08:26neologixsetmessages: + msg195781
2013-08-21 11:49:05christian.heimessetversions: + 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:29python-devsetnosy: + python-dev
messages: + msg195774
2013-08-20 23:46:41vstinnersetmessages: + msg195730
2013-08-20 22:55:33christian.heimessetfiles: + openssl_prng_atfork5.patch
2013-08-20 22:55:08christian.heimessetfiles: - openssl_prng_atfork5.patch
2013-08-20 22:43:33christian.heimessetfiles: + openssl_prng_atfork5.patch

messages: + msg195727
2013-08-18 21:20:37vstinnersetmessages: + msg195578
2013-08-18 13:22:02christian.heimessetfiles: + openssl_prng_atfork4.patch

messages: + msg195560
2013-08-17 21:54:28vstinnersetmessages: + msg195521
2013-08-17 18:30:20neologixsetmessages: + msg195501
2013-08-17 18:21:58pitrousetmessages: + msg195500
2013-08-17 16:07:19christian.heimessetfiles: + openssl_prng_atfork3.patch

messages: + msg195488
2013-08-15 17:45:45vstinnersetmessages: + msg195265
2013-08-15 17:44:04vstinnersetmessages: + msg195264
2013-08-15 13:44:51pitrousetmessages: + msg195256
2013-08-15 13:35:18christian.heimessetmessages: + msg195255
2013-08-15 13:14:06pitrousetmessages: + msg195254
2013-08-15 12:58:30christian.heimessetmessages: + msg195252
2013-08-15 12:50:23pitrousetmessages: + msg195250
2013-08-15 12:46:30christian.heimessetmessages: + msg195249
2013-08-15 12:43:44pitrousetnosy: + neologix
2013-08-15 12:38:03pitrousetversions: - 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:01christian.heimescreate