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: Make os.urandom() blocking on Linux #71963

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

PEP 524: Make os.urandom() blocking on Linux #71963

vstinner opened this issue Aug 16, 2016 · 14 comments
Labels
type-security A security issue

Comments

@vstinner
Copy link
Member

BPO 27776
Nosy @ncoghlan, @vstinner, @AraHaan
Files
  • urandom_nonblock.patch
  • urandom_nonblock-2.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-09-06.23:43:59.794>
    created_at = <Date 2016-08-16.12:55:06.620>
    labels = ['type-security']
    title = 'PEP 524: Make os.urandom() blocking on Linux'
    updated_at = <Date 2016-09-07.04:30:09.790>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2016-09-07.04:30:09.790>
    actor = 'ncoghlan'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-09-06.23:43:59.794>
    closer = 'vstinner'
    components = []
    creation = <Date 2016-08-16.12:55:06.620>
    creator = 'vstinner'
    dependencies = []
    files = ['44126', '44150']
    hgrepos = []
    issue_num = 27776
    keywords = ['patch']
    message_count = 14.0
    messages = ['272852', '272853', '272855', '272864', '272865', '273025', '273111', '273112', '273232', '274668', '274670', '274680', '274707', '274755']
    nosy_count = 4.0
    nosy_names = ['ncoghlan', 'vstinner', 'python-dev', 'Decorater']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue27776'
    versions = ['Python 3.6']

    @vstinner
    Copy link
    Member Author

    Issue to track the implementation of the PEP-524.

    @vstinner vstinner added the type-security A security issue label Aug 16, 2016
    @AraHaan
    Copy link
    Mannequin

    AraHaan mannequin commented Aug 16, 2016

    Wow. ( linux only pep? inb4 a windows thing gets wedged in)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 16, 2016

    New changeset 980e2c781810 by Victor Stinner in branch 'default':
    Issue bpo-27776: Cleanup random.c
    https://hg.python.org/cpython/rev/980e2c781810

    New changeset 265644bad99e by Victor Stinner in branch 'default':
    Issue bpo-27776: _PyRandom_Init() doesn't call PyErr_CheckSignals() anymore
    https://hg.python.org/cpython/rev/265644bad99e

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 16, 2016

    New changeset 86d0d74bc2e1 by Victor Stinner in branch 'default':
    Issue bpo-27776: Cleanup random.c
    https://hg.python.org/cpython/rev/86d0d74bc2e1

    New changeset ad141164c792 by Victor Stinner in branch 'default':
    Issue bpo-27776: dev_urandom(raise=0) now closes the file descriptor on error
    https://hg.python.org/cpython/rev/ad141164c792

    @vstinner
    Copy link
    Member Author

    Patch to make os.urandom() blocking on Linux 3.17+, but use non-blocking urandom in _random.Random constructor and _random.Random.seed() with no seed is set.

    @ncoghlan
    Copy link
    Contributor

    I have a few requests for clarification and confirmation as review comments, but overall +1 from me.

    (I'd still like a warning when we need to block in order to make life easier for system administrators attempting to debug any apparent system hangs, but as per the security-sig discussion, I can pursue that in a follow-up RFE and a separate patch, while this patch implements the PEP precisely as accepted)

    @vstinner
    Copy link
    Member Author

    Enhanced patch to address Nick's comments and fix mistakes. The new patch now also updates the documentation.

    I restored the code in _random.Random.seed() to fallback on the system clock: _PyOS_URandomNonblock() *can* fail is /dev/urandom is missing or not readable. I enhanced this part to not only read the system clock, but also use the current process identifier and get also the monotonic clock. Moreover, 64 bits are now used instead of 32 bits from the system clock (use a resolution of 1 nanoscond, not only 1 second).

    I didn't test yet the fall back on clocks/pid. It should be tested manually by modifying _PyOS_URandomNonblock() to always fail.

    @vstinner
    Copy link
    Member Author

    _PyOS_URandomNonblock() *can* fail is /dev/urandom is missing or not readable

    Oh. It looks like Python initialization currently fails with a fatal error in this case, see _PyRandom_Init().

    Maybe we should also fall back on clocks/pid in _PyRandom_Init()?

    @ncoghlan
    Copy link
    Contributor

    +1 for a fallback in the SIPHash initialisation as well.

    That's the case where Nathaniel Smith suggested we may want to issue a warning that the process shouldn't be used to handle untrusted inputs (since that particular remote DoS defence won't be working properly), but the monotonic time + the PID should be sufficiently unpredictable seeding for that case (since there are plenty of lower hanging fruit for attackers to go after).

    For testing, is there some way we could integrate an automated test of the deliberately misbehaving _PyOS_UrandomNonBlock into the testembed helper? If we can come up with a sensible way to do that, it could potentially help with testing the os.getrandom() BlockingIOError generation as well.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 6, 2016

    New changeset 45fc0c83ed42 by Victor Stinner in branch 'default':
    os.urandom() now blocks on Linux
    https://hg.python.org/cpython/rev/45fc0c83ed42

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 6, 2016

    Nick: "+1 for a fallback in the SIPHash initialisation as well."

    Sorry but I don't know a simple function to implement this. We might use the LCG RNG, but it's not really designed to be "secure". I don't think that it makes sense to initialize a shiny SIPHash with a crappy LCG RNG :-)

    So I skip my turn on this idea and let others implement them if anyone consider that it's worth it.

    To be clear: Python 3 doesn't start when getrandom() and /dev/urandom are not available or don't work, but it's not something new. Python 3.1 already starts with:

        fd = open("/dev/urandom", O_RDONLY);
        if (fd < 0)
            Py_FatalError("Failed to open /dev/urandom");

    --

    os.urandom() is now blocking, I close the issue.

    @vstinner vstinner closed this as completed Sep 6, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 7, 2016

    New changeset ebbfc053360a by Victor Stinner in branch 'default':
    Issue bpo-27776: include process.h on Windows for getpid()
    https://hg.python.org/cpython/rev/ebbfc053360a

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Sep 7, 2016

    If /dev/urandom isn't available, Python refusing to start is likely to be one of the least of the system's problems, so Py_FatalError sounds reasonable to me - my +1 for a fallback above was a matter of "sounds good if you can find a way to make it work".

    Thanks for all your work on getting this designed and implemented, Victor!

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Sep 7, 2016

    I've reviewed all the open issues that come up when searching for "getrandom" on the issue tracker, and closed all the ones that were either out of date or rejected based on PEP-524 being accepted and implemented.

    For the remainder, I either wasn't clear on whether they could be closed or not (in which case I posted a comment asking Victor to take a look at them), or else they were clearly still valid and I posted a relevant status update.

    Doing a similar search for "urandom", I checked the ones where the titles seemed relevant and commented where a status update and possible closure seemed appropriate.

    @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-security A security issue
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants