Repository navigation
urandom persistent fd - not re-openned after fd close #65406
Description
| BPO | 21207 |
|---|---|
| Nosy | @rhettinger, @pitrou, @vstinner, @tiran, @alex |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2014-04-26.12:40:01.154>
created_at = <Date 2014-04-12.15:33:39.286>
labels = ['type-bug']
title = 'urandom persistent fd - not re-openned after fd close'
updated_at = <Date 2014-04-26.12:40:01.153>
user = 'https://bugs.python.org/kwirk'bugs.python.org fields:
activity = <Date 2014-04-26.12:40:01.153>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2014-04-26.12:40:01.154>
closer = 'pitrou'
components = []
creation = <Date 2014-04-12.15:33:39.286>
creator = 'kwirk'
dependencies = []
files = ['34965', '35006']
hgrepos = []
issue_num = 21207
keywords = ['patch']
message_count = 18.0
messages = ['215973', '215981', '215982', '216232', '216446', '216520', '216526', '216575', '216664', '216794', '216795', '217037', '217040', '217041', '217043', '217056', '217190', '217191']
nosy_count = 9.0
nosy_names = ['rhettinger', 'pitrou', 'vstinner', 'christian.heimes', 'alex', 'grooverdan', 'neologix', 'python-dev', 'kwirk']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue21207'
versions = ['Python 3.4', 'Python 3.5']
Activity
kwirk commentedon Apr 12, 2014
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
pitrou commentedon Apr 12, 2014
Well, if a third-party library decides to close fds it doesn't own, that library should have a bug reported to it.
kwirk commentedon Apr 12, 2014
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").
kwirk commentedon Apr 14, 2014
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'
vstinner commentedon Apr 16, 2014
Which project or Python module does that? Can you show me the code?
kwirk commentedon Apr 16, 2014
Issue where I hit this is in Fail2Ban: fail2ban/fail2ban#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)
pitrou commentedon Apr 16, 2014
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:
rhettinger commentedon Apr 16, 2014
Christian, do you see a security risk with the proposed change?
neologix commentedon Apr 17, 2014
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...
pitrou commentedon Apr 18, 2014
Here is a proposed patch (with tests).
pitrou commentedon Apr 18, 2014
Hmm, the patch doesn't release the GIL around the fstat() calls, I wonder if that's necessary.
pitrou commentedon Apr 22, 2014
Updated patch using an anonymous struct.
grooverdan commentedon Apr 22, 2014
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.
pitrou commentedon Apr 22, 2014
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()...)
grooverdan commentedon Apr 22, 2014
fine by me. was just a thought
neologix commentedon Apr 23, 2014
LGTM!
python-dev commentedon Apr 26, 2014
New changeset a66524ce9551 by Antoine Pitrou in branch '3.4':
Issue bpo-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 bpo-21207: Detect when the os.urandom cached fd has been closed or replaced, and open it anew.
http://hg.python.org/cpython/rev/d3e8db93dc18
pitrou commentedon Apr 26, 2014
Ok, I've committed the patch. Hopefully this will also fix any similar issues.