Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

urandom persistent fd - not re-openned after fd close #65406

Closed
kwirk mannequin opened this issue Apr 12, 2014 · 18 comments
Closed

urandom persistent fd - not re-openned after fd close #65406

kwirk mannequin opened this issue Apr 12, 2014 · 18 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@kwirk
Copy link
Mannequin

kwirk mannequin commented Apr 12, 2014

BPO 21207
Nosy @rhettinger, @pitrou, @vstinner, @tiran, @alex
Files
  • urandom_fd_reopen.patch
  • urandom_fd_reopen2.patch
  • 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']

    @kwirk
    Copy link
    Mannequin Author

    kwirk mannequin commented 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

    $ 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

    @kwirk kwirk mannequin added the type-crash A hard crash of the interpreter, possibly with a core dump label Apr 12, 2014
    @pitrou
    Copy link
    Member

    pitrou commented 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
    Copy link
    Mannequin Author

    kwirk mannequin commented 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
    Copy link
    Mannequin Author

    kwirk mannequin commented 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
    Copy link
    Member

    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?

    @kwirk
    Copy link
    Mannequin Author

    kwirk mannequin commented 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
    Copy link
    Member

    pitrou commented 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:

    • 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

    @pitrou pitrou added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Apr 16, 2014
    @rhettinger
    Copy link
    Contributor

    Christian, do you see a security risk with the proposed change?

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented 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
    Copy link
    Member

    pitrou commented Apr 18, 2014

    Here is a proposed patch (with tests).

    @pitrou
    Copy link
    Member

    pitrou commented Apr 18, 2014

    Hmm, the patch doesn't release the GIL around the fstat() calls, I wonder if that's necessary.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 22, 2014

    Updated patch using an anonymous struct.

    @grooverdan
    Copy link
    Mannequin

    grooverdan mannequin commented 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
    Copy link
    Member

    pitrou commented 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.

    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
    Copy link
    Mannequin

    grooverdan mannequin commented Apr 22, 2014

    fine by me. was just a thought

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Apr 23, 2014

    Updated patch using an anonymous struct.

    LGTM!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented 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
    Copy link
    Member

    pitrou commented Apr 26, 2014

    Ok, I've committed the patch. Hopefully this will also fix any similar issues.

    @pitrou pitrou closed this as completed Apr 26, 2014
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants