Navigation Menu

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 466: update os.urandom #65504

Closed
ncoghlan opened this issue Apr 19, 2014 · 31 comments
Closed

PEP 466: update os.urandom #65504

ncoghlan opened this issue Apr 19, 2014 · 31 comments
Labels
extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

BPO 21305
Nosy @ncoghlan, @vstinner, @giampaolo, @tiran, @benjaminp, @alex, @dstufft, @MojoVampire
Files
  • backport-urandom.diff
  • backport-urandom.diff
  • backport-urandom.diff
  • 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-08-28.16:30:19.163>
    created_at = <Date 2014-04-19.00:51:14.459>
    labels = ['extension-modules', 'interpreter-core', 'type-feature']
    title = 'PEP 466: update os.urandom'
    updated_at = <Date 2014-08-28.16:30:19.161>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2014-08-28.16:30:19.161>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-08-28.16:30:19.163>
    closer = 'python-dev'
    components = ['Extension Modules', 'Interpreter Core']
    creation = <Date 2014-04-19.00:51:14.459>
    creator = 'ncoghlan'
    dependencies = []
    files = ['36437', '36496', '36497']
    hgrepos = []
    issue_num = 21305
    keywords = ['patch', 'needs review']
    message_count = 31.0
    messages = ['216824', '216852', '216858', '216859', '216860', '216861', '216862', '217281', '217292', '217364', '217366', '217369', '217372', '217374', '217425', '217429', '217432', '217470', '217472', '217476', '217478', '217494', '217496', '225172', '225684', '226025', '226026', '226027', '226028', '226029', '226031']
    nosy_count = 12.0
    nosy_names = ['ncoghlan', 'janssen', 'vstinner', 'giampaolo.rodola', 'christian.heimes', 'benjamin.peterson', 'alex', 'neologix', 'tshepang', 'python-dev', 'dstufft', 'josh.r']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue21305'
    versions = ['Python 2.7']

    @ncoghlan
    Copy link
    Contributor Author

    Tracker issue for the os.urandom persistent file descriptor backport to 2.7 described in PEP-466.

    @ncoghlan ncoghlan added the type-feature A feature request or enhancement label Apr 19, 2014
    @pitrou
    Copy link
    Member

    pitrou commented Apr 19, 2014

    Why exactly does it need to be backported? os.urandom under 2.7 works fine.

    @ncoghlan
    Copy link
    Contributor Author

    It was in the list of security fixes Alex and Donald wanted to reduce
    vulnerabilities in 2.x network services, and Guido was OK with backporting
    it.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 19, 2014

    It was in the list of security fixes Alex and Donald wanted to reduce
    vulnerabilities in 2.x network services, and Guido was OK with backporting
    it.

    What security issue is there exactly? os.urandom() does a similar thing
    in 2.7 and 3.x (it reads from /dev/urandom).

    @alex
    Copy link
    Member

    alex commented Apr 19, 2014

    It's not a security issue per-se, but if you're doing many small reads, there's such an enormous performance and scalability difference that if users run into an issue, they're likely to work around it by using a non-CS PRNG, and compromising their application security.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 19, 2014

    It's not a security issue per-se, but if you're doing many small
    reads, there's such an enormous performance and scalability difference
    that if users run into an issue, they're likely to work around it by
    using a non-CS PRNG, and compromising their application security.

    Do you have any examples of this or is it just a gratuitous inference?

    @pitrou
    Copy link
    Member

    pitrou commented Apr 19, 2014

    Note that the 3.4 scheme is not fully debugged yet: bpo-21207. There is a reason we don't backport new features!

    Regardless, I'm not interested in this, so I'll let you take the risk of regressions if you want to.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Apr 27, 2014

    Like Antoine, I'm really skeptical about the backport: honestly, this change doesn't bring much in a normal application. To run into the number of open file descriptors limit (so the "scalability" aspect), one would need to have *many* concurrent threads reading from /dev/urandom. For the "performance" aspect, I have a hard time believing that the overhead of the extra open() + close() syscalls is significant in a realistic workload. If reading from /dev/urandom becomes a bottleneck, this means that you're depleting your entropy pool anyway, so you're in for some potential trouble...

    There is a reason we don't backport new features!

    Couldn't agree more. This whole "let's backport security enhancements" sounds scary to me.

    @ncoghlan
    Copy link
    Contributor Author

    Yep, it's scary indeed, but such a long lived feature release is a novel
    situation that may require some adjustments to our risk management.

    However, we can still decide to defer some of the changes until 2.7.8, even
    though the notion of backporting them has been approved in principle.

    For 2.7.7, we should probably focus on the more essential SSL enhancements.

    @dstufft
    Copy link
    Member

    dstufft commented Apr 28, 2014

    "Depleting" /dev/urandom isn't actually a thing. /dev/urandom on all modern *nix OSs uses a fast PRNG which is secure as long as it has received enough bytes of initial entropy.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Apr 28, 2014

    "Depleting" /dev/urandom isn't actually a thing. /dev/urandom on all modern *nix OSs uses a fast PRNG which is secure as long as it has received enough bytes of initial entropy.

    I didn't say "deplete /dev/urandom", I said that when reading from
    /dev/urandom "you're depleting your entropy pool". So reading from
    /dev/urandom won't block, but it can starve processes that read from
    /dev/random, and that's a problem.

    See https://groups.google.com/forum/#!msg/fa.linux.kernel/Ocl01d8TzT0/KDCon2ZUm1AJ

    I think since 2.6 Linux uses two different entropy pools for
    /dev/random and /dev/urandom, but that might not be true for every OS.

    @dstufft
    Copy link
    Member

    dstufft commented Apr 28, 2014

    I don't think what you're worrying about here is something that has a high chance of happening, if it even occurs in the wild at all. To be clear in order for that to matter at all in the context of this ticket, some software would need to be reading from /dev/random (which almost zero software should be doing) on a system where you have a high number of threads or async handlers all reading from /dev/urandom at the same time and reading enough data off of it to drop the entropy estimation in the primary pool down below whatever amount of data that the other process is attempting to read from /dev/random. In that case no "trouble" will occur and the process reading from /dev/random will just block waiting on additional entropy to be collected so that the entropy estimation is high enough to fulfill the request.

    AFAIK there are zero practical concerns from reading as much as you want off of /dev/urandom as often as you want.

    What this change does is make it "safe" to just simply use os.urandom in order to generate random bytes in a Python application. The current situation is such that any use of os.urandom is potentially a place where your application will crash in highly concurrent environments. This will drive people to either:

    A) Poorly reimplement the persistent FD option, especially troublesome on Windows because the simple approach to doing so will flat out not work on Windows
    B) Use a userspace CSPRNG, this is considered ill advised by most reputable cryptographer's as most of them have had issues at one point in time or another, and they all generally depend on the security of /dev/urandom anyways so if /dev/urandom fall they fall as well.

    Using os.urandom is the *right* thing to do for getting random in an application, but the current implementation effectively punishes people who use it if their application is highly concurrent.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Apr 28, 2014

    Using os.urandom is the *right* thing to do for getting random in an application, but the current implementation effectively punishes people who use it if their application is highly concurrent.

    And I argue that this scenario is almost as likely as the one you
    depict above: we never had a bug report before, and if you have a look
    at the the bug report which led to the change in question, it's not
    clear at all that all threads were indeed reading from /dev/urandom
    when EMFILE was raised. Since reading from /dev/urandom shouldn't
    block, it's not clear at all how a realistic workload would actually
    hit the file descriptor limit because RLIMIT_NOFILE threads are
    reading from /dev/urandom.

    But don't get me wrong, I'm not saying this change is useless, it
    actually makes sense to use a persistent FD. But backporting always
    has a risk, which has to be balanced.

    @dstufft
    Copy link
    Member

    dstufft commented Apr 28, 2014

    But backporting always has a risk, which has to be balanced.

    Sure, which is why a PEP was written, discussed and accepted to find that balance.

    @vstinner
    Copy link
    Member

    Please don't backport this "feature". We had to wait 20 years before someone requested the feature, but only a few months before the first user reported an issue ("regression"?).

    IMO it would be much better to use explicitly a random.SystemRandom instance which would keep the fd open, and only use it if you hit the bug (many threads calling os.urandom in parallel.

    @dstufft
    Copy link
    Member

    dstufft commented Apr 28, 2014

    Well except random.SystemRandom doesn't keep the file open (At least in 2.7) and actually it just calls os.urandom under the covers, also it doesn't make it very nice to get a glob of random bytes.

    @dstufft
    Copy link
    Member

    dstufft commented Apr 28, 2014

    Just verified that 3.x also does not exhibit this behavior with random.SystemRandom (except implicitly through os.urandom doing it).

    @vstinner
    Copy link
    Member

    Le 29 avr. 2014 00:22, "Donald Stufft" <report@bugs.python.org> a écrit :

    Well except random.SystemRandom doesn't keep the file open (At least in
    2.7) and actually it just calls os.urandom under the covers, also it
    doesn't make it very nice to get a glob of random bytes.

    Yes, I'm proposing to enhance this class.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Apr 29, 2014

    Yes, I'm proposing to enhance this class.

    The problem is AFAICT there's currently no way to get a file
    descriptor to the underlying /dev/urandom (and I don't know how it
    works on Windows).

    Also, this would duplicate the work which has already been done.

    @dstufft
    Copy link
    Member

    dstufft commented Apr 29, 2014

    One of the reasons the PEP was done the way it was done was it allowed you to write 2/3 compatible code without version checks. Enhancing that class won't land until 3.5 which is 18+ months away. Further more the os.urandom persistent FD's already exists and is a complete superset of functionality that a random.SystemRandom random would have.

    This conversation would have been a lot more useful when the PEP was being discussed on python-dev.

    @ncoghlan
    Copy link
    Contributor Author

    Note that the discussion of this PEP *did* suffer from the "language summit
    effect" where folks that couldn't make it to the summit are missing some of
    the context. I believe I included all of the key motivating points in the
    PEP itself, but it's still not the same as being there from a "why take the
    risk?" perspective.

    Of the approved feature backports, this one was the most borderline, and as
    noted above, I think it could comfortably wait until 2.7.8. The SSL changes
    are most significant, followed by the constant time comparison function,
    and only then the other updates.

    @vstinner
    Copy link
    Member

    The problem is AFAICT there's currently no way to get a file
    descriptor to the underlying /dev/urandom (and I don't know how it
    works on Windows).

    We can reimplement os.urandom in SystemRandom on UNIX to keep the file (fd)
    open. The code is very simple, basically it's just a call to file.read(n).

    Adding a randbytes() method in Python 3.5 would be nice.

    The io module can handle boring things for you, like calling read in a loop
    until you get enough bytes and handle InterruptError.

    Except if you would prefer to use os.read or FileIO.read to avoid readahead.

    @vstinner
    Copy link
    Member

    (and I don't know how it
    works on Windows).

    On Windows, the OS CryptoAPI is used and a handle is kept open between
    calls to os.urandom. On Windows, I don't think that it's a an issue to keep
    a handle open. Handle are not sequential numbers and users don't manipulate
    them directly.

    @vstinner
    Copy link
    Member

    See also the issue bpo-22181: "os.urandom() should use Linux 3.17 getrandom() syscall".

    @alex
    Copy link
    Member

    alex commented Aug 22, 2014

    Attached patch backports the persistent FD for urandom.

    @alex alex added extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Aug 22, 2014
    @benjaminp
    Copy link
    Contributor

    You should probably backport _PyRandom_Fini and cleanup the FD like a good citizen.

    @alex
    Copy link
    Member

    alex commented Aug 28, 2014

    This patch adds the finalizer to the backport -- not sure how I missed this the first time.

    @vstinner
    Copy link
    Member

    @alex: please disable git format in your hgrc, so the bug tracker can create a "review" link to Rietveld for your patches.

    @alex
    Copy link
    Member

    alex commented Aug 28, 2014

    Victor -- new patch is in hg format.

    @vstinner
    Copy link
    Member

    The third backport-urandom.diff (the one with the review link) looks good to me.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 28, 2014

    New changeset 3e7f88550788 by Benjamin Peterson in branch '2.7':
    PEP-466: backport persistent urandom fd (closes bpo-21305)
    http://hg.python.org/cpython/rev/3e7f88550788

    @python-dev python-dev mannequin closed this as completed Aug 28, 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
    extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants