classification
Title: urandom persistent fd - not re-openned after fd close
Type: behavior Stage: resolved
Components: Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: alex, christian.heimes, grooverdan, haypo, kwirk, neologix, pitrou, python-dev, rhettinger
Priority: normal Keywords: patch

Created on 2014-04-12 15:33 by kwirk, last changed 2014-04-26 12:40 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
urandom_fd_reopen.patch pitrou, 2014-04-18 19:08 review
urandom_fd_reopen2.patch pitrou, 2014-04-22 22:17 review
Messages (18)
msg215973 - (view) Author: Steven Hiscocks (kwirk) Date: 2014-04-12 15:33
I've seen an issue with using urandom on Python 3.4. I've traced down to fd being closed (not by core CPython, but by third party library code). After this, access to urandom fails.

I assume this is related to persistent fd for urandom in http://bugs.python.org/issue18756

$ python -c "import os;os.urandom(1);os.closerange(3,256);os.urandom(1)"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
OSError: [Errno 9] Bad file descriptor
msg215981 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-04-12 20:47
Well, if a third-party library decides to close fds it doesn't own, that library should have a bug reported to it.
msg215982 - (view) Author: Steven Hiscocks (kwirk) Date: 2014-04-12 21:09
I agree in part, but it's quite common to close fd's in some cases like in a child process after using "os.fork()". There is no way, as far as I'm aware, to identify which fd is associated with /dev/urandom to keep it open; or anyway to reopen it such that other libraries which depend on it can use it (for example "tempfile.TemporaryFile").
msg216232 - (view) Author: Steven Hiscocks (kwirk) Date: 2014-04-14 22:08
Just to add for those interested: a possible work around solution is using "os.path.sameopenfile" to check fds against another known fd for urandom.

And for those wish to have a bit of fun (and maybe a security consideration):
python -c "import os;os.urandom(1);os.closerange(3,256);fd = open('/dev/zero');print(os.urandom(10))"
b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
msg216446 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-04-16 07:30
> I agree in part, but it's quite common to close fd's in some cases like in a child process after using "os.fork()"

Which project or Python module does that? Can you show me the code?
msg216520 - (view) Author: Steven Hiscocks (kwirk) Date: 2014-04-16 17:51
Issue where I hit this is in Fail2Ban: https://github.com/fail2ban/fail2ban/issues/687

Lines of code where this occurs: https://github.com/fail2ban/fail2ban/blob/1c65b946171c3bbc626ddcd9320ea2515018677b/fail2ban/server/server.py#L518-530

There are other examples of closing file descriptors in other packages which create daemon processes, as well as code snippets about, as it is typical behaviour to close files. (http://en.wikipedia.org/wiki/Daemon_%28computing%29#Creation)
msg216526 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-04-16 17:57
Well, on the one hand this does sound like a valid use case. On the other hand, once the urandom file descriptor is closed by third-party code, it can very well be re-opened to point to another file, and then os.urandom() will start behaving in a very bad way.

Here is a possible solution in Python:
- when opening the urandom fd for the first time, record its st_ino and st_dev
- when calling urandom() a second time, call fstat() on the fd and check the st_ino and st_dev with the known values
- if the values have changed (or if fstat() fails with EBADF), open a new fd to /dev/urandom, again
msg216575 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-04-16 20:05
Christian, do you see a security risk with the proposed change?
msg216664 - (view) Author: Charles-Fran├žois Natali (neologix) * (Python committer) Date: 2014-04-17 07:06
I was expecting to see such a report :-)

I'm al for the st_ino+st_dev check, it can't hurt.

But everybody must keep in mind that if another thread messes with the
FD between the check and the read, there's nothing we can do...
msg216794 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-04-18 19:08
Here is a proposed patch (with tests).
msg216795 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-04-18 19:23
Hmm, the patch doesn't release the GIL around the fstat() calls, I wonder if that's necessary.
msg217037 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-04-22 22:17
Updated patch using an anonymous struct.
msg217040 - (view) Author: Daniel Black (grooverdan) * Date: 2014-04-22 22:54
maybe you've thought and dismissed this already but os.close could call dev_urandom_close for the urandom_fd. Then there's no fstat calls in every random access. As a sweeping close all isn't going to occur that often and extra open probably isn't that much overhead.
msg217041 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-04-22 22:57
> maybe you've thought and dismissed this already but os.close could
> call dev_urandom_close for the urandom_fd. Then there's no fstat calls
> in every random access.

That's fine if os.close() is indeed used to close fd, but not if some
third-party library uses the C close() function directly. I don't know
how likely that is, but I think it's better to squash the bug
completely, rather than 80% of it :-)

(also some other stdlib code might (?) also call C close()...)
msg217043 - (view) Author: Daniel Black (grooverdan) * Date: 2014-04-22 23:05
fine by me. was just a thought
msg217056 - (view) Author: Charles-Fran├žois Natali (neologix) * (Python committer) Date: 2014-04-23 07:30
> Updated patch using an anonymous struct.

LGTM!
msg217190 - (view) Author: Roundup Robot (python-dev) Date: 2014-04-26 12:36
New changeset a66524ce9551 by Antoine Pitrou in branch '3.4':
Issue #21207: Detect when the os.urandom cached fd has been closed or replaced, and open it anew.
http://hg.python.org/cpython/rev/a66524ce9551

New changeset d3e8db93dc18 by Antoine Pitrou in branch 'default':
Issue #21207: Detect when the os.urandom cached fd has been closed or replaced, and open it anew.
http://hg.python.org/cpython/rev/d3e8db93dc18
msg217191 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-04-26 12:40
Ok, I've committed the patch. Hopefully this will also fix any similar issues.
History
Date User Action Args
2015-01-05 21:27:53haypolinkissue23172 superseder
2014-04-26 12:40:01pitrousetstatus: open -> closed
resolution: fixed
messages: + msg217191

stage: patch review -> resolved
2014-04-26 12:36:08python-devsetnosy: + python-dev
messages: + msg217190
2014-04-23 07:30:38neologixsetmessages: + msg217056
2014-04-22 23:05:45grooverdansetmessages: + msg217043
2014-04-22 22:57:40pitrousetmessages: + msg217041
2014-04-22 22:54:18grooverdansetmessages: + msg217040
2014-04-22 22:17:55pitrousetfiles: + urandom_fd_reopen2.patch

messages: + msg217037
2014-04-19 14:43:31alexsetnosy: + alex
2014-04-18 19:23:44pitrousetmessages: + msg216795
2014-04-18 19:08:12pitrousetfiles: + urandom_fd_reopen.patch
keywords: + patch
messages: + msg216794

stage: patch review
2014-04-17 07:06:51neologixsetmessages: + msg216664
2014-04-16 23:55:07grooverdansetnosy: + grooverdan
2014-04-16 20:05:44rhettingersetnosy: + christian.heimes, rhettinger
messages: + msg216575
2014-04-16 17:57:48pitrousetversions: + Python 3.5
nosy: + neologix

messages: + msg216526

type: crash -> behavior
2014-04-16 17:51:16kwirksetmessages: + msg216520
2014-04-16 07:30:51hayposetnosy: + haypo
messages: + msg216446
2014-04-14 22:08:24kwirksetmessages: + msg216232
2014-04-12 21:09:50kwirksetmessages: + msg215982
2014-04-12 20:47:02pitrousetmessages: + msg215981
2014-04-12 16:09:55hayposetnosy: + pitrou
2014-04-12 15:33:39kwirkcreate