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) * |
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 (vstinner) * |
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) * |
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) * |
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) * |
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) * |
Date: 2014-04-18 19:08 |
Here is a proposed patch (with tests).
|
msg216795 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
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) * |
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) * |
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) * |
Date: 2014-04-26 12:40 |
Ok, I've committed the patch. Hopefully this will also fix any similar issues.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:01 | admin | set | github: 65406 |
2015-01-05 21:27:53 | vstinner | link | issue23172 superseder |
2014-04-26 12:40:01 | pitrou | set | status: open -> closed resolution: fixed messages:
+ msg217191
stage: patch review -> resolved |
2014-04-26 12:36:08 | python-dev | set | nosy:
+ python-dev messages:
+ msg217190
|
2014-04-23 07:30:38 | neologix | set | messages:
+ msg217056 |
2014-04-22 23:05:45 | grooverdan | set | messages:
+ msg217043 |
2014-04-22 22:57:40 | pitrou | set | messages:
+ msg217041 |
2014-04-22 22:54:18 | grooverdan | set | messages:
+ msg217040 |
2014-04-22 22:17:55 | pitrou | set | files:
+ urandom_fd_reopen2.patch
messages:
+ msg217037 |
2014-04-19 14:43:31 | alex | set | nosy:
+ alex
|
2014-04-18 19:23:44 | pitrou | set | messages:
+ msg216795 |
2014-04-18 19:08:12 | pitrou | set | files:
+ urandom_fd_reopen.patch keywords:
+ patch messages:
+ msg216794
stage: patch review |
2014-04-17 07:06:51 | neologix | set | messages:
+ msg216664 |
2014-04-16 23:55:07 | grooverdan | set | nosy:
+ grooverdan
|
2014-04-16 20:05:44 | rhettinger | set | nosy:
+ christian.heimes, rhettinger messages:
+ msg216575
|
2014-04-16 17:57:48 | pitrou | set | versions:
+ Python 3.5 nosy:
+ neologix
messages:
+ msg216526
type: crash -> behavior |
2014-04-16 17:51:16 | kwirk | set | messages:
+ msg216520 |
2014-04-16 07:30:51 | vstinner | set | nosy:
+ vstinner messages:
+ msg216446
|
2014-04-14 22:08:24 | kwirk | set | messages:
+ msg216232 |
2014-04-12 21:09:50 | kwirk | set | messages:
+ msg215982 |
2014-04-12 20:47:02 | pitrou | set | messages:
+ msg215981 |
2014-04-12 16:09:55 | vstinner | set | nosy:
+ pitrou
|
2014-04-12 15:33:39 | kwirk | create | |