Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:103369 Return-Path: Delivered-To: mailing list internals@lists.php.net Received: (qmail 88825 invoked from network); 22 Oct 2018 17:32:24 -0000 Received: from unknown (HELO mail-lj1-f169.google.com) (209.85.208.169) by pb1.pair.com with SMTP; 22 Oct 2018 17:32:24 -0000 Received: by mail-lj1-f169.google.com with SMTP id k11-v6so5052319lja.5 for ; Mon, 22 Oct 2018 06:47:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sammyk-me.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=yIUL5TNB62kgJ9JqJzJFc1Pcr1bowc7S7Om8Klf8sI0=; b=px/vhdfw+Ps04rKR5qlmilK9dm1J39MX57Vf35fAwHr0H7oZGF+IDoSPz1tqiUW1wR zAzLediOJRuD4E00XHXB/vRl1cy8WEYTFe6nxWQdEyhMZggBjHiqrqJWS28XkOk6pIz6 DS0R04upQcyJ/FgC52LWnJ2cJO47+e5Yna9DlCMqaW5X35UvnhhfO3Ll64bemjDUmdVq 6G3xOTMEk/6vh7WfIdjS/aSnO0LxzUw/108oE8L18WJBEtz+ijXlJMnsFLf39KHWqli+ 7Pvhihz9A+WnR45FKzZOUTwaW8Nt7b3k/DcG/HhRvR6EMAm3IXLX2eYA7wZlATbDdpa/ pfJw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=yIUL5TNB62kgJ9JqJzJFc1Pcr1bowc7S7Om8Klf8sI0=; b=eHRzUAb/rpLpKVbHSJlUPI0BTHPzDGr1lTVApFIl6met6eqBeZo5MI2N+OF5lkB6Ll c0f7op0IxWZuAKkPamn7xr0qEUldWHraZrtZ+ps3jkoVWhfCZCl2Z5djn/mdKlD23ryQ kNPETn3QWtWwYPHtIXFHgtY3VVMDpkG/LNfD7Q1ZujGDlMApQTFE30Y3jpAtlATcV+nN KTDCb8gGy69VxBpnk3mTAxxJ2qVgRfjasSPZmBcs/Ea1S3+i51ZzghkIwTAWyxC3lyjf 7NDgQ8r/fEll1HbU/r4QF38nwYrEykAxYZ7QnqvX6gCuzoxk7SRdgWsZ931X0VZlxgmH SQsg== X-Gm-Message-State: ABuFfogz58gXAGblfF2/XIeChQ3i0Nir9I7P9O7pvJnwiydTIEbtdNT7 W8DQv51GUBhmn8V+zlV12OFOw6arqzfKAMn78Y8diuyVlZg= X-Google-Smtp-Source: ACcGV63yCSgha7NbuIxu42lSiLIx2+mQIPXrClRBICGnpCSp62Ya8bnRBNlSShbgYVMULjR2c1nsovAJC4Gyd2LQ4gw= X-Received: by 2002:a2e:9955:: with SMTP id r21-v6mr12644185ljj.5.1540216022295; Mon, 22 Oct 2018 06:47:02 -0700 (PDT) MIME-Version: 1.0 References: <1D2D9809-6A2D-49E5-9F86-FBB52AF538B5@thefsb.org> In-Reply-To: <1D2D9809-6A2D-49E5-9F86-FBB52AF538B5@thefsb.org> Date: Mon, 22 Oct 2018 09:46:50 -0400 Message-ID: To: Tom Worster Cc: PHP Internals Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [RFC] Improve openssl_random_pseudo_bytes() From: me@sammyk.me (Sammy Kaye Powers) Hey Tom! Thanks for the feedback! :) On Sun, Oct 21, 2018 at 8:09 PM Tom Worster wrote: > > At first glance I believed you were proposing that `openssl_random_pseudo= _bytes()` should fail with an exception and that this would be an improveme= nt. I would agree with that. Yep! If the function can't gather crypto-strong bytes, this patch will throw an exception: https://github.com/php/php-src/compare/master...SammyK:rfc-improve-openssl-= random-pseudo-bytes > With a little more concentration I see you're proposing something less am= bitious that I'm less enthusiastic about. > The function has been obsolete since 7.0 What makes the function obsolete? The addition of the `random_bytes()` CSPRNG (which uses the kernel's CSPRNG) doesn't invalidate OpenSSL's CSPRNG. > and A Bad Choice=E2=84=A2 in all versions of PHP except when OS=3D=3DWin= dows AND 5.4.0 <=3D PHP < 7.0. What makes it a Bad Choice=E2=84=A2? :) Historically, it certainly has had it's fair share of implementation disasters: 1. It implemented `RAND_pseudo_bytes()` (PRNG) instead of `RAND_bytes()` (CSPRNG) all the way up until PHP 5.6.11: https://github.com/php/php-src/blob/PHP-5.6.11/ext/openssl/openssl.c#L5408 but that got fixed: https://bugs.php.net/bug.php?id=3D70014 2. `RAND_bytes()` is not fork safe by default causing major issues for packages like ramsey/uuid: https://benramsey.com/blog/2016/04/ramsey-uuid/#when-uuids-collide but that got fixed in PHP 5.6.24: https://bugs.php.net/bug.php?id=3D71915 3. It fails open (this patch will fix that) 4. The API is confusing with the second `$crypto_strong` parameter which doesn't do anything (the other thing that this patch will fix) With this patch, `openssl_random_pseudo_bytes()` should finally be implemented properly so it won't be a Bad Choice=E2=84=A2 anymore. :) > The only reason to keep this function is BC I'm not sure of the reasons for why this would be the case. :) > but removing the second param breaks BC for ALL conscientious and safe us= es, Yes, but it also confuses how to use the function in userland. The OpenSSL extension doesn't seem to be going anywhere anytime soon, so for all future uses of this function, the API should be clear and also fail closed without forcing all the fail checks into userland - especially unnecessary ones. :) > i.e. seeking unpredictable (i.e. crypto strong) randoms from 5.4.0 <=3D P= HP < 7.0 on Windows. There's no valid reason to ask for predictable randoms= from OpenSSL and, afaik, its not unpredictable (i.e. it's unsafe) on other= OSs. `RAND_bytes()` is a proper CSPRNG and isn't set up to be used as a deterministic PRNG. > I'd love to see an RFC along the lines of: "Improve PHP's OpenSSL API by = depreciating and eventually removing openssl_random_pseudo_bytes()". Idk th= e right schedule for removing it but how could deprecating it in 7.4 do mor= e harm than good? My original ping to internals was to alias `openssl_random_pseudo_bytes()` to `random_bytes()`, but as others have pointed out, having more than one CSPRNG isn't necessarily a bad thing to have. :) Thanks again for the feedback! Maybe the vote should be split up into two parts: 1) Fail closed & 2) deprecate the second parameter. :) Thanks, Sammy Kaye Powers sammyk.me