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

PEP 524: Add os.getrandom() #71965

Closed
vstinner opened this issue Aug 16, 2016 · 17 comments
Closed

PEP 524: Add os.getrandom() #71965

vstinner opened this issue Aug 16, 2016 · 17 comments
Labels
3.7 (EOL) end of life docs Documentation in the Doc dir stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@vstinner
Copy link
Member

BPO 27778
Nosy @ncoghlan, @vstinner, @tiran, @vadmium
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
  • Files
  • getrandom.patch
  • getrandom_errno.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 2016-10-17.16:21:08.288>
    created_at = <Date 2016-08-16.17:25:41.671>
    labels = ['3.7', 'type-feature', 'library', 'docs']
    title = 'PEP 524: Add os.getrandom()'
    updated_at = <Date 2017-03-31.16:36:09.313>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:09.313>
    actor = 'dstufft'
    assignee = 'docs@python'
    closed = True
    closed_date = <Date 2016-10-17.16:21:08.288>
    closer = 'vstinner'
    components = ['Documentation', 'Library (Lib)']
    creation = <Date 2016-08-16.17:25:41.671>
    creator = 'vstinner'
    dependencies = []
    files = ['44127', '44760']
    hgrepos = []
    issue_num = 27778
    keywords = ['patch']
    message_count = 17.0
    messages = ['272867', '273024', '274662', '274698', '274717', '274722', '274724', '274730', '274733', '276323', '276356', '276386', '276389', '277068', '277070', '277335', '278815']
    nosy_count = 6.0
    nosy_names = ['ncoghlan', 'vstinner', 'christian.heimes', 'docs@python', 'python-dev', 'martin.panter']
    pr_nums = ['552']
    priority = 'low'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27778'
    versions = ['Python 3.6', 'Python 3.7']

    @vstinner
    Copy link
    Member Author

    Attached patch adds os.getrandom(): thin wrapper on the Linux getrandom() syscall.

    os.getrandom() can return less bytes than requested.

    The patch is incomplete: it doesn't include documentation.

    I chose to not implement a loop to not loose entropy if a following call fails (ex: fail with EINTR). Rationale:
    https://mail.python.org/pipermail/security-sig/2016-July/000072.html

    We should also add Solaris support later.

    See also bpo-27776: "PEP-524: Make os.urandom() blocking on Linux".

    @vstinner vstinner added type-security A security issue stdlib Python modules in the Lib dir labels Aug 16, 2016
    @ncoghlan
    Copy link
    Contributor

    Given docs (with the Linux-only platform support disclaimer), +1 for this as an initial implementation.

    Providing it on Solaris as well can be a separate patch, but it's less important there (since /dev/urandom and os.urandom() are already blocking APIs)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 6, 2016

    New changeset 27267d2fb091 by Victor Stinner in branch 'default':
    Add os.getrandom()
    https://hg.python.org/cpython/rev/27267d2fb091

    @vstinner vstinner closed this as completed Sep 6, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Sep 7, 2016

    HAVE_GETRANDOM_SYSCALL seems to be a compile-time library check, not a runtime check. I compiled and run on Linux 3.15.5, and os.getrandom() exists but raises ENOSYS:

    ======================================================================
    ERROR: test_getrandom0 (test.test_os.GetRandomTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/media/disk/home/proj/python/cpython/Lib/test/test_os.py", line 1280, in test_getrandom0
        empty = os.getrandom(0)
    OSError: [Errno 38] Function not implemented

    @vadmium vadmium reopened this Sep 7, 2016
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Sep 7, 2016

    Huh, I thought I'd already filed an issue for that, but it looks like it was only a security-sig thread: https://mail.python.org/pipermail/security-sig/2016-June/000060.html

    I've now remedied that omission and filed http://bugs.python.org/issue27990 to cover it explicitly.

    Since that was a pre-existing problem that also happens to affect this API, rather than something new introduced by Victor's patch, closing this again.

    @ncoghlan ncoghlan closed this as completed Sep 7, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 7, 2016

    New changeset 7a243a40b421 by Victor Stinner in branch 'default':
    Fix test_os.GetRandomTests()
    https://hg.python.org/cpython/rev/7a243a40b421

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 7, 2016

    HAVE_GETRANDOM_SYSCALL seems to be a compile-time library check, not a runtime check. I compiled and run on Linux 3.15.5, and os.getrandom() exists but raises ENOSYS:

    Oh, I'm surprised the configure sees getrandom() as available. But well ok, the error can occur if you compile Python on a more recent kernel than the running kernel.

    I fixed the unit test: skip getrandom() tests if getrandom() fails with ENOSYS.

    Do you think that it's worth to document this case?

    @vstinner vstinner reopened this Sep 7, 2016
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Sep 7, 2016

    Ah, I'd missed that Martin was talking about the other way around from bpo-27990.

    Yes, I think it's worth documenting that os.getrandom() may raise OSError if the running kernel doesn't provide the syscall - that's going to be pretty easy to trigger by running a container with Python 3.6 on a container host running an older Linux kernel.

    @vadmium
    Copy link
    Member

    vadmium commented Sep 7, 2016

    I run Arch Linux, but only update packages when I have to. As a result, I am running Linux 3.15 installed and running, but the linux-api-headers has more recently been updated to 4.7 (i.e. matching Linux 4.7).

    @tiran
    Copy link
    Member

    tiran commented Sep 13, 2016

    3.6 is in beta phase. Are you interested to add the feature to 3.7?

    @tiran tiran added the 3.7 (EOL) end of life label Sep 13, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Sep 13, 2016

    I understand it’s already implemented, and Victor just reopened it for more documentation.

    @ncoghlan
    Copy link
    Contributor

    Right, the only missing piece now is documentation of the ENOSYS case, which end users may encounter if a Python 3.6 binary that supports os.getrandom() is run against an older kernel.

    That's pretty easy to trigger via containers, as getrandom() was added in Linux 3.17 and hasn't generally been backported to LTS distribution kernels.

    Debian 8: based on 3.16
    Ubuntu 14.04: 3.13 default, 4.4 (from 16.04) available as of 14.04.5
    RHEL/CentOS 7: based on 3.10
    RHEL/CentOS 6: based on 2.6

    So of those potential LTS container hosts, a recent Ubuntu or Fedora container running Python 3.6 will currently get ENOSYS for everything except a fresh Ubuntu 14.04 install that uses the Ubuntu 16.04 kernel.

    @tiran
    Copy link
    Member

    tiran commented Sep 14, 2016

    Oh sorry, I looked in the wrong location and missed it.

    • if (PyErr_CheckSignals() < 0) {return NULL;} does not free buffer with PyMem_Free(buffer);

    • The function allocates memory once with PyMem_Malloc() and later a second time with PyBytes_FromStringAndSize(buffer, n). You can avoid the first allocation and a memcpy() with PyBytes_FromStringAndSize(NULL, n) and PyBytes_AS_STRING().

    • The syscall can also raise EPERM as reported by a user on QNAP. IIRC a seccomp policy caused EPERM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 20, 2016

    New changeset d31b4de433b7 by Victor Stinner in branch '3.6':
    Fix memleak in os.getrandom()
    https://hg.python.org/cpython/rev/d31b4de433b7

    @vstinner
    Copy link
    Member Author

    I pushed the fix for the issue bpo-27955, os.urandom() now handles getrandom() failing with EPERM.

    @christian: Thanks for your review, I pushed a change fixing the two issues that you reported (memory leak and inefficient temporarily buffer).

    I attached getrandom_errno.patch: a change proposing to document ENOSYS and EPERM. What do you think?

    @tiran
    Copy link
    Member

    tiran commented Sep 24, 2016

    I think the documentation is too specific. We typically don't document all possible error numbers. Something along the lines "fails with OSError when getrandom is not supported" is sufficient.

    @tiran tiran added the docs Documentation in the Doc dir label Sep 24, 2016
    @tiran tiran added type-feature A feature request or enhancement and removed type-security A security issue labels Sep 24, 2016
    @vstinner
    Copy link
    Member Author

    Because of the lack of interest for getrandom_errno.patch, and Christian saying that it's not good to document specific errors, I now close the bug.

    Thank you all for your help on this nice security enhancement in Python 3.6!

    @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
    3.7 (EOL) end of life docs Documentation in the Doc dir stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants