Problem/Motivation
Drupal prefers openssl_random_pseudo_bytes() if available in Crypt::randomBytes() in 8.0.x or drupal_random_bytes() in 7.x and 6.x.
PHP used the wrong method in the openssl library now fixed in 5.6.12, 5.5.28, 5.4.44 see: https://bugs.php.net/bug.php?id=70014 but it is NOT classified as a security hole so backports are to older (ie distro) versions are less likely. Just because PHP didn't declare this a security hole doesn't mean it is not. But also read on for other scenarios where this class creates a security hole.
The returned pseudo-random bytes were NOT necessarily cryptographically secure.
Proposed resolution
Add random_compat v2.0.2 to core + use it for Crypt::randomBytes()
Adding v2 should be ok here because in drupal 7.x this includes a fallback.
(so not a concern like in #2763787: Upgrade random_compat to latest version which reverted the upgrade to v2.0.2 in drupal 8.2.x and 8.3.x)
Remaining tasks
review
User interface changes
n/a
API changes
n/a
Data model changes
n/a
RC phase evaluation
Issue category | Bug because the Crypt class doesn't do what the docblock says, and what the docblock says reflects how the class is actually used in Drupal. |
---|---|
Issue priority |
Critical because:
|
Disruption | None. Everything should work exactly the same, except with actual cryptographically secure pseudorandom bytes |
Comments
Comment #2
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedComment #3
nullkernel CreditAttribution: nullkernel as a volunteer commentedI wrote a patch which should ensure that stronger random bytes are generated.
This patch doesn't regenerate the secret key. I am unsure if doing so would cause problems with existing sites.
Comment #4
nullkernel CreditAttribution: nullkernel as a volunteer commentedComment #5
joshtaylor CreditAttribution: joshtaylor at Josh Taylor Development for Bonsai Bookings commentedIs it worth checking if the system is running 5.4 if Drupal 8 minimum version is 5.5?
Comment #6
nullkernel CreditAttribution: nullkernel as a volunteer commentedIt occurred to me that we may want to check to see if random_bytes() is available. If it is, then we don't need to use openssl_random_pseudo_bytes(). It is a PHP 7 feature, though I assume is a better and more efficient source of random bytes.
http://php.net/manual/en/function.random-bytes.php
Comment #7
nullkernel CreditAttribution: nullkernel as a volunteer commentedI have made the following changes:
I am attaching another patch and an interdiff.
Disclaimer: I do not have PHP 7 installed on the computer I am using. The QA test bot is running PHP 5.5 and won't be able to test the PHP 7 code. I believe the code is correct and welcome anyone running PHP 7 to confirm that this is working as expected.
Comment #8
dawehnerNot sure whether we need the PHP 7 part in the issue. #2540694: Deprecate Crypt::randomBytes() in favor of PHP 7 polyfll is an issue about some form of replacement for it.
Comment #9
nullkernel CreditAttribution: nullkernel as a volunteer commentedIn my previous comment I accidentally used a .txt file extention for the patch. Reuploading for the QA bot to test.
Edit: I inadvertently got the filename wrong again (comment number) due to the comment from dawehner at the same time. dawehner, I believe my patch implements one of the proposed solutions of that issue. If the random_bytes() function is available, Crypt::randomBytes() will essentially be a wrapper for it.
Comment #10
dawehnerWell, its still not needed to fix the issue, keep things as reviewable as possible. This additional call doesn't help with that.
Someone argued that the entire fix is kinda pointless, because in case attackers care about that level of security, you have to be on the latest release (or at least the patched one from your distribution) as otherwise you have much bigger issues.
Comment #11
nullkernel CreditAttribution: nullkernel as a volunteer commentedOverall, I like the proposed solution in the issue you linked. I looked over some of the library code but not all of it. At this time I cannot vouch for its security. It seems to do a lot to ensure good random bytes. However I think it is arguably not more efficient than the "additional call" and more difficult to review.
If it is decided to add that library as a dependency, my patch will utilise it. Whether the library is accepted as a core dependency or not, the random_bytes() function should be used if PHP, contrib modules, or website developers make the function available to Drupal.
If you or anyone else want to submit a patch (or possibly overhaul the code), I'd be interested to take a look. I think that the code I proposed is a step in the right direction.
Comment #12
dawehnerYeah, well its security hardening, even if we optimize for a case, which is not that likely, see previous comment.
Comment #13
BerdirDoesn't matter if it is ready, but AFAIK, security hardening issues aren't critical and there's a separate tag for them?
Comment #14
dawehnerGood point.
Also isn't adressed yet.
Comment #15
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedLookslike a reasonable start
should use
isset()
instead of!is_null()
Comment #16
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedPlease leave at Critical until committers have triaged. I'm unsure the real impact of this, but it is a potential security hole if the key is guessable.
Comment #17
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedQuick patch to fix method naming and some other things.
Comment #18
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented+patch
Still needs update function.
Comment #19
catchThis means that we won't use openssl_random_pseudo_bytes() on distribution versions of PHP that are patched but without a version number increment. I think Debian now follows the minor PHP version numbering normally, Ubuntu LTS still patches (hence current 5.5.9 requirement), but not sure if this would affect anything else, or if it's a problem if so.
Comment #20
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@catch - since there was no CVE associated with the PHP bug, it's unclear to me if Ubuntu/RHEL will backport the bugfix.
Given the recent history of openssl problems, not using it might be the better option, so perhaps we should totally remove it again, since it wasn't what we originally used here.
Comment #21
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@catch - From this, it does not look like Debian is not updating the version numbers. Did you see some other announcement? https://packages.debian.org/jessie/php5
Comment #22
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedGiven that we had guidance from Solar Designer @openwall I'm not surprised that the fallback routine looks similar to the NIST for a has-based CSPRNG:
http://csrc.nist.gov/publications/nistpubs/800-90A/SP800-90A.pdf
We don't re-seed, but effectively every page request is a reseed, and I didn't dig in detail to see if there are ways we meaningfully deviate.
Also note that the polyfill for php 7 prefers /dev/urandom if available, so perhaps we should just rip out openssl?
https://github.com/paragonie/random_compat/blob/master/lib/random.php
Comment #23
catchhttps://bugs.debian.org/cgi-bin/bugreport.cgi?bug=757342 and http://www.debian.org/security/2014/dsa-3064 is what I was thinking of.
Comment #24
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@catch - interesting. I see that it's implied, but unclear if it's happening?
Comment #25
nullkernel CreditAttribution: nullkernel as a volunteer commentedI wrote an update function. I ran "drush state-get system.private_key" before and after the update, and the key had changed. I'm not sure what consequences (if any) there are to doing this. I attached a patch and interdiff.
Edit: removed irrelevant remark about backport version numbering.
Comment #27
nullkernel CreditAttribution: nullkernel as a volunteer commentedI'm not sure why those two failures happened, or if my code even caused them. Any ideas?
As far as eliminating OpenSSL, I'm on the fence. With Windows, the code would wind up using the final fallback if random_bytes() isn't available. Ripping it out entirely may be throwing out the baby with the bathwater. I do agree that we should prefer /dev/urandom over openssl. However Windows doesn't have /dev/urandom available to provide entropy.
I think that openssl_pseudo_random_bytes() can provide Windows (and any server using the fallback) a decent source of entropy. The bytes can be thrown into the hashing function along with mt_rand(), $_SERVER, and microtime(). If a Linux distro backports the fix to an earlier version number, openssl bytes would be hashed instead of directly used.
Edit: Removed reference to getmypid().
Comment #28
nullkernel CreditAttribution: nullkernel as a volunteer commentedI made changes based on my previous comment. I think this may get us close to resolving this issue.
In summary:
I haven't changed the code to regenerate the private key. Perhaps it will pass the QA test cases this time around?
Comment #29
nullkernel CreditAttribution: nullkernel as a volunteer commentedComment #31
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedWe should look at the php7 shiv code which sets the buffered read of /dev/urandom to 8 bytes incetead of reading a large block.
Comment #32
chx CreditAttribution: chx commented> If a Linux distro backports the fix to an earlier version number, openssl bytes would be hashed instead of directly used.
that's very unlikely. As I linked above, Debian (which did such porting previously) is now more up to date with the releases instead of backporting, they are now up to 5.4.41 and I presume as security dictates they will update further. This OpenSSL bug was not marked as a security bug and if there's any other distro left that backports (which?) they most probably won't do this one -- in the past such backporting was done for security only.
Comment #33
Heine CreditAttribution: Heine at LimoenGroen for Historisch Centrum Overijssel commentedResponse to #28:
> /dev/urandom (if available) is preferred over OpenSSL.
This makes drupal_random_bytes insecure on Windows, when another process writes a /dev/urandom file with known content.
> If the bytes from openssl_random_pseudo_bytes() aren't considered cryptographically strong, they are recycled in the fallback case's hashing function.
Is the fallback cryptographically strong? If not, is it better, equal or worse than the result from openssl_random_pseudo_bytes? If equal or worse, do we need to change anything here?
As to #31: We should look at the php7 shiv code which sets the buffered read of /dev/urandom to 8 bytes [instead] of reading a large block.
What is the rationale for changing this and the number of bytes requested? Crypt::randomBytesBase64 defaults to 32 bytes.
Comment #34
cilefen CreditAttribution: cilefen commented@pwolanin Can we use random_bytes() on PHP7? And if so, can we make Drupal forwards-compatible?
Comment #35
catchGiven there was no CVE for the PHP release, I don't think this is critical. Looks like straight hardening at most.
Comment #36
cweagansWe should just pull in https://packagist.org/packages/paragonie/random_compat, and always call random_bytes().
Comment #37
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedIt looks like that code has the same problem that Heine is concerned about - if a system that doesn't actually support /dev/urandom someone writes a file there you get non-random content from the file.
Is there a check we can add? Maybe limit use of /dev/urandom to cases when
posix_uname()['sysname']
is Linux or Darwin?Or more simply the posix functions are normally not available on Windows - so we can do
if (function_exists('posix_getpwuid'))
or the like?http://php.net/manual/en/intro.posix.php
The only downside is a false disabling if PHP is built without posix support, but that seems uncommon?
Comment #38
cweagansOpened https://github.com/paragonie/random_compat/issues/49 to discuss. The maintainers of random_compat are very responsive. I've landed a couple PRs in the last day or two.
Comment #39
cweagansThis may be a stupid question, but if you try to
file_put_contents('/dev/urandom', 'something');
on Windows, does it actually work? Seems to me that if PHP doesn't complain loudly about that, it's a PHP bug.Comment #40
cweagansOkay, looks like random_compat had already mitigated that problem, but there's now an explicit condition for Windows hosts. See https://github.com/paragonie/random_compat/issues/49#issuecomment-147579107
Comment #41
cweagansLet's give this a try. This is a straight conversion to random_compat, now that Heine's concern is resolved upstream (the library won't even try to read from /dev/urandom if it's running on Windows).
Comment #42
cweagansComment #43
jibranWe have to exclude tests folder after #2585165: Don't include vendor test code (especially mink) in the Drupal webroot.
Comment #44
cweagansAh, right. Okay, here's a new one.
Comment #46
cweagansReroll for changes in head.
Comment #47
cweagans@chx was asking about why I had the \ in front of random_bytes(). I seem to remember it being something about differences in behavior between php 5.5 and php 7.0 (given that random_bytes() is a built in in 7.0 and a user defined function in 5.5), but I'm not 100% sure, so uploading a patch without the \ to see what happens on 5.5 and 7.0.
Comment #48
cweagansApparently, I was imagining the difference :)
Reviews for #47, plz!
Comment #49
cweagansChanging title, bumping to critical, adding information about why that's the case to the IS.
Comment #50
xjm@cweagans pinged me and asked me to take a look at this. Based on the information in the summary, my feeling is that the issue is major, but I'd defer to the security team on the priority.
Comment #51
chx CreditAttribution: chx commentedComment #52
chx CreditAttribution: chx commentedComment #53
cweagansComment #54
klausiquoting Damien Tournoud: "Note: RAND_pseudo_bytes and RAND_bytes are *exactly the same thing* on *all systems except the most arcane ones*. Which is why the "fix" in PHP was not marked as a security issue, because except on some weird systems that do not have either /dev/random nor /dev/urandom, it is not, actually, a security issue."
Comment #55
alexpottHere's a reroll and the tests directory properly excluded by the script.
Comment #56
cweagans@klausi, the openssl docs disagree https://www.openssl.org/docs/manmaster/crypto/RAND_bytes.html (see second paragraph under "Description"), and so does the code (https://github.com/openssl/openssl/blob/master/crypto/rand/rand_lib.c#L158 vs https://github.com/openssl/openssl/blob/master/crypto/rand/rand_lib.c#L167 + https://github.com/openssl/openssl/blob/master/crypto/rand/md_rand.c#L610). I may be misinterpreting that, though, because my C is very rusty.
For the sake of argument, however, let's assume that rand_pseudo_bytes() and rand_bytes() do the same thing except for a small percentage of systems. Are we really okay with putting sites running on those systems in harms way? It'd be one thing if this were some weird unicode thing or something, but for this? These things that use random data from this class with the expectation that the output is going to be unpredictable:
* Form tokens
* CSRF tokens
* Config directories
* Password salts
If we cannot guarantee that this class will output cryptographically secure random data in all cases, we shouldn't make that claim, and if we can't make that claim, then we can't rely on it for the things that we're using it for.
Even if the openssl issue turns out to be mitigated in part or in full, there are still two other broken paths in Crypt::randomBytes() for PHP installations without ext_openssl: /dev/urandom without any checking that it's actually the right thing, and the userspace "random" data generator (which literally uses nothing suitably random).
Comment #57
chx CreditAttribution: chx commentedAttacking a site via creating a file called c:\dev\urandom is just devious and exceptionally elegant... even if the feasibility of it is questionable.
Comment #58
klausiPatch does not apply anymore.
I agree that the patch is a good improvement, I just don't agree on the critical priority.
Comment #59
catchNo, which is why this is a major bug that we'll fix as soon as it's ready. However it's security hardening for a theoretical vulnerability, not an actual exploitable vulnerability - which is why it's a major bug and public, rather than critical and in the private tracker.
So downgrading back to major, but tagging RC target, could also be fixed in 8.0.1 if it doesn't get into 8.0.0.
Comment #60
alexpottRerolled
Comment #61
cweagans@klausi, Can you please elaborate on why you don't agree with it being critical?
@catch, the ext_openssl issue is not the only thing here - are you saying that all of the reasons given in the "Issue priority" section in the RC phase evaluation in the issue summary are not valid reasons for making this Critical?
@alexpott, thanks for rerolling this :)
Comment #62
cweagans@catch - also, for the record, I did open a security issue about this, but since the public issue predated the security issue, I was told to work on it here. This vulnerability *is* exploitable on Windows if an attacker can also write a file to C:\dev\urandom.
Comment #63
klausi@cweagans: this issue is not critical because there is no direct security vulnerability, no data loss, no unusable system.
I reviewed the patch and everything makes sense to me. The additional random.php file used for auto loading on every single page request only does a couple of file inclusions and function_exists() calls, so this should only have a tiny performance impact.
Comment #64
klausihiding obsolete patch files.
Comment #65
klausiHa, spoken too soon. Executing
ab -n 500 -c 4 http://drupal-8.localhost/node/3
(with page_cache module enabled) multiple times, consistently getting the same results:Core 8.0.x (without patch): 19ms time per request (median)
Patch: 21ms time per request (median)
That would indicate a 10% performance regression for cached pages. Can anyone reproduce those results?
EDIT: PHP version 5.6.4-4ubuntu6.4
Comment #70
dawehnerIs there any reason why this external library cannot use classes?
Comment #71
klausiWith Xdebug disabled, again
ab -n 500 -c 4 http://drupal-8.localhost/node/3
, page_cache enabled:Drupal core: 16 ms time per request (median)
Patch: 17 ms time per request (median)
The random_compat library uses /dev/urandom on my system while core uses openssl_random_pseudo_bytes(), which seems to be faster. Still a difference of 6% in performance for this average page request to /node/3.
Testing autoload performance: reverting the hunks in Crypt.php so that only the new autoload code of the patch runs I get the same 16 ms time per request (median) as without the patch.
So it seems the performance regression in the patch is the usage of /dev/urandom now. If I use core and modify Crypt::randomBytes() so that openssl_random_pseudo_bytes() is not used, then I see the same 17 ms time per request (median) because it falls back to /dev/urandom.
Are we comfortable with sacrificing 1 ms on every Drupal page request just because openssl_random_pseudo_bytes() can somtimes be insecure on old PHP versions?
Comment #72
catchI think we could potentially not do the Crypt::randomBytes() call in Kernel::boot(), and instead re-seed it on demand (and document you shouldn't use mt_rand() directly) to skip the 1ms regression. Need to find the issue that was added.
Comment #73
klausiThe mt_srand() seeding was introduced in #2140433: Port SA-CORE-2013-003 to Drupal 8.
Comment #74
klausiThe mt_srand() call does not make sense to me, filed #2617208: Remove mt_srand() seeding because of performance.
Comment #75
cweagans@dawehner: I don't know that it's actually been discussed, but I'm not sure how much sense it makes for random_compat because it's just a polyfill for two functions.
As I said in https://www.drupal.org/node/2617208#comment-10577338, I think we can just get rid of mt_srand() here since this patch is also removing cryptographic use of mt_rand().
Comment #76
klausiOK, here is a patch that removes the mt_srand() call from DrupalKernel.
The page cache performance tests are now equal to 8.0.x, so the removal from the Kernel bootstrapping definitely helped.
I disabled page_cache and dynamic_page_cache to measure the performance impact on an uncached page, same test with
ab -n 500 -c 4 http://drupal-8.localhost/node/3
:Executed 5 times, median time per request:
core: 160,160,159,160,160 ms => median of medians: 160ms
patch: 163,165,161,162,162 ms => median of medians: 162ms
Which means we have a performance regression of about 2ms = 1% on an uncached page.
Comment #78
klausiRandom test fail, passes now.
Comment #79
klausiI did some more benchmarking on a more powerful computer with the same
ab -n 500 -c 4 http://drupal-8.localhost/node/3
test (page_cache and dynamic page cache disabled):PHP 5.6.11-1ubuntu3.1, medians of 5 test runs:
core: 51,51,51,51,51 ms
patch: 51,52,51,51,51 ms
PHP 7.x master as of 2015-11-20, medians of 5 test runs:
core: 32,32,32,32,32 ms
patch: 31,32,32,32,32 ms
As we can see there is no significant performance difference on this typical Drupal request to an uncached node page.
Taking a closer look at Crypt::randomBytes() in isolation with this benchmark script which calls the static method 1 million times:
Testing with
time php benchmark.php
:PHP 5.6.11-1ubuntu3.1, 5 test runs:
core: 2287,2272,2192,2291,2368 ms
patch: 4624,4600,4730,4612,4603 ms
PHP 7.x master as of 2015-11-20, 5 test runs:
core: 1590,1573,1549,1707,1545 ms
patch: 4016,3980,3971,4020,4004 ms
Quite disappointing that random_bytes() in PHP 7 performs so much worse than openssl_random_pseudo_bytes(), 2.5 times slower.
Of course this is a very artificial benchmark since the typical Drupal use case will never invoke Crypt::randomBytes() one million times. As the first ab benchmark shows a performance regression of 1% is only noticeable on older hardware (and shared hosting probably).
There is FUD flying around that openssl_random_pseudo_bytes() has a weakness https://twitter.com/ircmaxell/status/666667962959532032 , but I have not found hard evidence yet. I'm torn between the superior performance of openssl_random_pseudo_bytes() over random_bytes() and the advantage of getting rid of the code so that we don't have to maintain it anymore.
Comment #80
klausiHm, this does not have to be an all or nothing issue. We can pick the random_compat library, remove the scary parts we all agree that should be removed and keep openssl_random_pseudo_bytes() for performance reasons. We can see this issue as a first step in the right direction until we know more and rather have small improvements instead of all at once.
We have here:
1) adding random_compat library, no measurable performance impact of adding it to the autoloader.
2) keeping openssl_random_pseudo_bytes() for performance reasons
3) using random_bytes() as a secure fallback, thereby removing our own custom PRNG fallback.
Comment #81
cweagansThere's no performance hit, we get rid of code that we have to maintain, and the code we replace it with uses other/better sources of randomness before openssl. Why would we want to use openssl_random_pseudo_bytes() before that?
As you say, that's a very artificial benchmark, and is not indicative of real-world use.
IMO, we should not do anything except call random_bytes() here.
Comment #82
klausiThere is a performance hit for slower hardware (as demonstrated) and as soon as any contrib module gets random bytes somewhere in the critical performance path. We want to use openssl_random_pseudo_bytes() because it is faster.
So we don't agree whether using openssl_random_pseudo_bytes() is worth it or not.
In the meantime I'm offering a patch that removes the other crap where we absolutely agree that it should go. Can we do the latest patch as a compromise until we know more?
Comment #83
cweagansThat's what I'm saying though: There isn't a real-world performance hit, and contrib modules that are getting random bytes in a critical path are doing it wrong.
The performance test that you did was a contrived example. Nobody is ever going to be getting 55 random bytes 1,000,000 times in a row. It's extremely likely that they'll be getting < 128 bytes a small handful of times, and for that, there's no observable difference, and you said that yourself:
This method needs to return cryptographically secure pseudorandom bytes in all circumstances - no matter what OS, version of PHP, or nonstandard configuration is in use. If doing that is too slow for whatever reason, then we need to evaluate whether or not we can actually provide this functionality, and since this functionality backs a lot of our other security components, that's not a good situation to be in.
We should not sacrifice security for performance. In this case, there's only a negligible performance gain by adding back openssl_random_pseudo_bytes(), and it's just not worth it in my opinion.
If it's absolutely necessary, I'd be okay with adding openssl_random_pseudo_bytes() back in a way that a user has to specifically opt-in to using it before random_bytes() (i.e. it should be off by default). However, I think we should use random_bytes() and then open a followup to determine if openssl_random_pseudo_bytes() is really worth the performance gain in all scenarios (and there are a *lot* of them - Windows wasn't considered here at all, nor were systems that are using ext_libsodium). I suspect that it's not, but I'd want to verify that in a test that reflects real-world use (and not just the raw speed of execution of each method 1,000,000 times).
Comment #84
klausiWe don't know the use cases of contrib modules and using random bytes in the critical performance path might be something they want to do - so this is not necessarily wrong.
I'm still missing evidence here that openssl_random_pseudo_bytes() is insecure (even in the older PHP versions). I agree with Damien Tournoud that we just have FUD about it right now. If we cannot come up with an attack scenario that describes on what system and what configuration openssl_random_pseudo_bytes() is exploitable we should stick with it.
Comment #85
klausiTalked to alexpott about this and he is in favor of getting as much custom crypto code removed here as we can, off-loading this to random_compat and PHP 7 core. The performance loss for that is small enough.
We have eliminated the worst performance offender in DrupalKernel in this patch, so I guess I can live with random_bytes() only for the sake of making progress here.
Patch attached that uses random_bytes() only and is the same as in #76.
Comment #86
cweagans@klausi, Sorry I missed you in IRC over the weekend. There are details about the problem here: https://github.com/paragonie/random_compat/issues/6#issuecomment-119564973
+1 to #85
Comment #87
klausiOK, me + cweagans + alexpott agree, no objections, enough for RTBC.
Comment #88
neclimdulI suspected that the extra requires random_compat does was causing the slowdown on the "slow" test. Klausi confirmed there was no opcache on that instance which is good enough for me to write off my concerns.
+1 RTBC.
Comment #89
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThere's a newer version of the library available (1.1.1) - should the patch use that instead?
Comment #90
klausiyes, will do a reroll.
Comment #91
klausiRerolled with
composer update paragonie/random_compat
.Comment #92
neclimdulComment #93
cweagans+1 to RTBC on this. Thanks for moving this forward Klausi. I've been stupidly busy lately. I went ahead and opened the followup I mentioned earlier: https://www.drupal.org/node/2629926
While we're waiting on this to land in 8.0.x and/or 8.1.x, we should figure out how we might backport this to D7 and/or D6 (per the tags on this issue). Pulling in a Composer lib in D8 is reasonably straightforward, but I don't know that we have a reasonable way to do that in D7 or D6. Any ideas here?
Comment #94
cweagansklausi, looks like someone else saw the performance issue you were seeing (I could not reproduce). https://github.com/paragonie/random_compat/issues/78 The fix for that is included in 1.1.1. Does that change resolve the issue satisfactorily?
Comment #95
alexpott@cweagans so we should update the patch - no?
Comment #96
cweagans@alexpott, klausi already rerolled the patch with 1.1.1 in #91. Just wanted to check to see if the new release resolved the issue he was seeing. Still RTBC from my perspective.
Comment #99
neclimdulphantomjs collision. Conflicts in composer.lock and installed.json.
Rebased, reset files to HEAD version and rebuilt with
composer.phar update --lock
Comment #100
cweagansLGTM. Back to RTBC.
Comment #101
alexpottCommitted 5106176 and pushed to 8.0.x and 8.1.x. Thanks!
Comment #104
alexpottI committed this to 8.0.x because afacis this patch does not add any new functionality to Drupal 8 and therefore is only a bug fix and patch eligible. I think in most cases adding a new library would be a minor version activity.
Comment #105
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commentedCommits in #102 & #103 doesn't looks correct.
Function random_bytes() available only in PHP 7.
But according to this page https://www.drupal.org/requirements PHP 5.5.9 is enough for Drupal 8.
I use PHP 5.5.9 and I have Fatal error :(
Fatal error: Call to undefined function Drupal\Component\Utility\random_bytes() in /../test8/core/lib/Drupal/Component/Utility/Crypt.php on line 36
Comment #106
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commentedComment #107
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commentedComment #108
alexpott@eugene.ilyin any chance you can provide a full backtrace? The patch adds a polyfil library for older versions of PHP and the patches are tested on PHP5.5
Comment #109
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commented@alexpott, I've added xdebug backtrace in the attached archive. Is it enough?
Also for information, I've installed composer manager and I've updated my libraries via "composer drupal-update"
Maybe something went wrong...
Comment #110
alexpott@eugene.ilyin yeah you seem to be missing
vendor/paragonie/random_compat
- if you runcomposer install --dry-run
in the drupal root what happens? And please ensure that composer is up-to-date.Comment #111
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commented@alexpott and @bojanz helped me in IRC. Trouble was in my personal state of directory "vendor".
How I've fixed it:
1. I've executed "composer install --dry-run" in root directory
2. After that I've tried to execute "composer drupal-update" but I had an error
3. I've turned back the directory "vendor" to clean state (as in drupal core)
4. I've executed "composer drupal-update" again and my trouble has gone
Comment #112
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commentedComment #114
alexpottAfter discussing with @catch we've decided to revert this patch because adding dependencies is proving to be tricky. For example if you following the instructions to update only the
/core
directory and then run composer install in the drupal root directory nothing happens - it does not get the newparagonie/random_compat
. Therefore, it seems sensible to give ourselves some more time to work out how to give the best advice on updating core dependencies.Comment #115
catchJust to clarify this is reverted against 8.0.x, the 8.1.x patch is still in as is.
Comment #116
cweagans@alexpott @catch So should this work be cleaned up for 8.0.x? Or should we just have users wait to update to 8.1.0 to get this improvement? I'm not really sure why this was rolled back though. If you're using Composer + a subtree split of /core, you'll have to wait for the subtree repo to be updated + wait for Composer to generate new package metadata that includes random_compat as a dependency (which would happen when 8.0.2 is tagged). I'm fairly certain that running on head + composer_manager won't pick up new dependencies until that happens.
Comment #117
alexpott@cweagans here is why... https://github.com/wikimedia/composer-merge-plugin/issues/96 - basically we're breaking the promise of update ./core and you're good to go. And there is no composer command to help either - apart from composer update and that is going to do way more than just install the dependency.
Comment #118
catch@cweagans for me I'd just have this wait until 8.1.x - especially given the security improvement is as far as we know theoretical.
#116 is interesting though, if tagging the release made it all work again, but not sure it's worth waiting to find out. We'd probably need an 8.1.0-beta1 to test that successuflly without impacting real sites.
Comment #119
cweagansEh. Let's just wait for now. People that really want the change can apply the patch to 8.0.x, and they'll likely know how to deal with the composer issues too.
So how should we go about getting this into D7? Just download random_compat, shove it into includes, and require() it from bootstrap.inc? The other changes are pretty straightforward - just want to know what's going to be acceptable in terms of getting the lib loaded.
Comment #120
catchUp to David in the end, but #119 is about what I'd expect.
Comment #121
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI guess, though it does seem a bit wasteful to load all that code when it might not be used.
Another way (which I think may have made sense for D8 too) is to load it from within drupal_random_bytes() the first time that function is called?
A couple other questions I had just now skimming through the D8 patch (I guess they apply to both D7 and D8):
Do we know that this condition won't ever be reached in Drupal? Because we aren't doing anything to handle it...
Currently in drupal_random_bytes() we always return something and never throw an exception.
Comment #122
Crell CreditAttribution: Crell at Palantir.net commentedThe argument made on PHP Internals was that if you ask for cryptographically secure random bytes, and those cannot be provided, then the only secure thing to do is bail out. Proceeding with an insecure string (or worse, false aka empty string) is a security hole. That is, if you hit that exception your site's environment is not safe to use in the first place, and a fatal uncaught exception is appropriate. (Or, alternatively, we could see about translating it to a nicer-looking fatal error message.)
For Drupal 7, we could wrap the include statement in a version_compare() check as it's only needed < PHP 7.0.0. But really, that's what the if (function_exists()) stuff is doing already so I wouldn't worry about it too much in D7 or D8.
Edited: Corrected the direction of the carrot
Comment #123
cweagansIt's not a lot of code all things considered - likely not much more than what drupal_random_bytes() already contains.
I *think* this code is going to get loaded on most page requests anyway, but I have no objection to loading it on the first drupal_random_bytes() call.
It's very rare that a system would be so thin on sources of randomness. IIRC, when I did some digging on phpinfo for a bunch of shared hosts a couple years ago for something else, openssl was enabled 100% of the time, even on the most obscure of hosts, so that would be the most likely worst case scenario.
Comment #124
neclimdulI think this is a missunderstanding. @eugene.ilyin could you provide the steps you did to upgrade? My guess from Alex's upstream report is that composer.lock was maybe not updated? http://cgit.drupalcode.org/drupal/tree/core/UPGRADE.txt#n67 requires all files be updated. If you don't follow that process things will probably go wrong.
Additionally from the discussion people might be looking for
composer.phar update --lock
which will pull in things from composer.json that are missing from composer.lock. This is a pretty important command for people using composer_manager or similar contrib solutions for managing /vendor.Comment #125
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedYeah, I think that part is probably fine as is.
I guess what I want to avoid is throwing a runtime exception on live sites... The library does appear to throw exceptions in a couple different places under a couple different scenarios.
So if we're adding some new requirements to Drupal here that didn't exist before, it should at least be in hook_requirements() somewhere. And we should think a bit how it should be handled on upgrade.
One possible solution would be to keep our old code around, but only use it as a fallback in the case where the library throws an exception. And when a site is in that situation, throw a REQUIREMENT_ERROR in hook_requirements().
This is based on the idea that our current drupal_random_bytes() is not a complete security disaster, even if it's not perfect either (i.e. that a site can still operate with a reasonable level of security in that case, even though the security could be improved).
Comment #126
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI had intended to leave a related issue with the above comment, but it got eaten.
Comment #127
cweagansI strongly object to this. The polyfill only throws an exception when random bytes cannot be generated for whatever reason (usually, it's a lack of a trustworthy source of randomness). If we're saying that drupal_random_bytes() is going to return cryptographically secure random bytes, then it should either do that or throw an exception when it can't. An exception is (in this case) the least undesirable thing, and the most undesirable thing is that we return more predictable random bytes silently.
All of that said, it's very rare that an environment won't have at least one of the polyfill methods available. I think every shared hosting company that I've looked at has at least two options (usually mcrypt and openssl, but some windows hosts have the COM extension enabled too). I don't think this is something to worry about.
A hook_requirements() entry, however, might be a good idea. Just ensure that the host that Drupal is running on has at least ONE of the handfull of polyfill methods available.
Comment #128
stefan.r CreditAttribution: stefan.r commentedJust bumping this as this would be good to get into 7.x
Comment #131
klausiHere is a first D7 patch. I did not include a hook_requirements() check because drupal_random_bytes() is used so early. I agree with @cweagans that a site should fail fast if there is no source of secure random bytes.
Comment #132
stefan.r CreditAttribution: stefan.r commentedThanks!
Comment #133
klausiRemoved some build scripts in random_compat that we really should not ship with Drupal.
Comment #135
daffie CreditAttribution: daffie commentedWhy are we going to use version 2 of paragonie's random_compat library for D7 and version 1 for D8. Should we upgrade the library for D8?
Comment #136
klausiWe should use version 2 in both, the issue to upgrade it in D8 is #2763787: Upgrade random_compat to latest version.
Comment #137
pwolanin CreditAttribution: pwolanin as a volunteer and commentedThere is some suggestion this is affecting production sites, so we should consider this critical
Comment #138
pwolanin CreditAttribution: pwolanin as a volunteer and commentedShould we try/catch in drupal_random_bytes and retain the existing fallback generator? Given that it came from Solar Designer, it's better than an uncaught exception or upgrade failure.
Comment #139
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedRight, like I said above, I really don't like the idea of whitescreening someone's production site just because our random number generator doesn't reach a level of security that we never claimed it had in the first place...
I think allowing the fallback to be used, but putting something about it in hook_requirements() (perhaps a REQUIREMENT_ERROR on runtime and a REQUIREMENT_WARNING on install?) would really be good enough.
Comment #140
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedOops, cross-post (had the issue open in a browser tab for a while). Not entirely sure what "affecting production sites" means in this context though...?
Comment #141
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented- CNW for the try {} catch
Just got a report that on some site this generated the same Session ID twice in a day ...
Comment #142
sarciszewski CreditAttribution: sarciszewski as a volunteer commentedBy naming it Crypt::randomBytes, you're implicitly claiming it will be cryptographically secure. Using random_compat v1 will guarantee that you have a CSPRNG in most installations, but its fallback to OpenSSL will cause problems like https://github.com/ramsey/uuid/issues/80.
Using v2 guarantees a level of security comparable to what PHP 7 offers (and is, therefore, a true polyfill).
My recommendation is to set the version requirement to ^1|^2 so people can choose whichever one works best for their plugins/extensions.
(In case it wasn't clear: I work for Paragon Initiative Enterprises, whose namespace random_compat is published under.)
Comment #143
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe name of the function in Drupal 7 is drupal_random_bytes(), not Crypt::randomBytes(), and the documentation at https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drup... only claims that it is "highly randomized" and "uses the best available pseudo-random source", nothing more than that.
But yes, it is confusing that the issue title here dates from the Drupal 8 version of the patch :)
So we'd probably be better off never using OpenSSL at all, to be safe?
In any case, there is definitely no need to keep the OpenSSL code from the current version of drupal_random_bytes() - I assume the only thing we'd need to keep from that is the final fallback, i.e. the part from here onward:
Since our goal is to hand this off to random_bytes() whenever possible, but just keep a single guaranteed backup method in place to prevent whitescreening the site if all else fails.
Comment #144
klausiAh, we forgot to remove the performance critical mt_srand() seeding during Drupal boostrap as we did for Drupal 8.
Still no exception handling or hook_requirements(), feel free to pick that up.
Comment #145
klausiForgot interdiff.
Comment #146
pwolanin CreditAttribution: pwolanin as a volunteer and commentedWhy do we want to remove this - just to avoid forcing a call to drupal_random_bytes()?
Comment #147
klausiYes, getting random bytes is slow on php5, see analysis in #71.
Comment #148
pwolanin CreditAttribution: pwolanin at SciShield commentedHere's with fallback for Exception
Comment #149
klausiThis will fail on PHP 5.2 because it does not understand the "\" namespace operator.
line length now over 80 characters.
Comment #151
pwolanin CreditAttribution: pwolanin at SciShield commentedoh, I'm an idiot - I passed the --stat flag to git when making the prior patch.
This should address klausi's concerns.
Comment #152
klausiLooks good! Now we also need that try/catch block in system_requirements(). It should show an error on runtime and install time when random_bytes() throws an exception, right?
BTW I'm still in favor of not catching the exception, but I guess any progress is better than nothing?
We are running patch #144 on our production sites successfully now.
Comment #153
sarciszewski CreditAttribution: sarciszewski as a volunteer commentedhttps://github.com/joomla/joomla-cms/blob/91749f845dcf20e097801ecd75da8e...
Comment #154
klausi@sarciszewski: we are talking about the old stable version of Drupal here, which is Drupal 7 and was released in 2011. It's minimum requirement is PHP 5.2 and we are not breaking that compatibility just for fun.
Drupal 8 of course requires PHP 5.5 :)
Comment #155
pwolanin CreditAttribution: pwolanin at SciShield commentedOk, so needs work still for hook requirements?
Comment #156
pwolanin CreditAttribution: pwolanin at SciShield commentedAdded a check to system_requirements()
Comment #159
joseph.olstad@pwolanin
The reason why your patch #156 failed:
+ require_once DRUPAL_ROOT . '/includes/random_compat/lib/random.php';
random_compat and this library was not included in the patch, therefore travis ci will fail during testing.. Try making a new patch that adds this random_compat/lib/random.php
To do this you will git add this and commit it in your local with the other changes , then to generate the patch you will do a git diff of the resulting commit hash tag vs the previous hashtag.
19:43:52 error: patch failed: scripts/run-tests.sh:142
19:43:52 error: scripts/run-tests.sh: patch does not apply
This is pretty easy to generate a new patch, what version of this random.php library are you wanting to include?
Comment #160
klausi@joseph.olstad: the latest patch already contains the library in version 2.0.2, so there is no need to roll a new patch.
Queuing the patch for testing again.
Comment #161
pwolanin CreditAttribution: pwolanin at SciShield commentedI'm not sure what the test fails were - seem to not be there on re-test
Comment #162
joseph.olstadTestbot says its good after a kick. So yes the patch was fine after all. Blame it on the testbot.
Comment #163
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis could use some kind of help text (or, probably better, a link) explaining how to fix the problem.
Wording could be a bit less technical also, maybe like:
Title: "Random number generation"
Summary: "Less secure"
Description: "The server is unable to generate highly randomized numbers, which means certain security features like password reset URLs are not as secure as they should be. This site is using a slow, less-secure fallback generator instead..." ... something like that.
Do we really want this to be a REQUIREMENT_ERROR even on install/update? Won't that prevent Drupal from being installed at all?
Comment #164
klausiI think REQUIREMENT_ERROR is fine. You should really fix your server setup first when there is no source of secure random numbers at all. We only want to permit usage of our weak random number generator for existing sites so that they don't white screen on an update.
Which means we should not run this check in the "update" phase to always allow sites being updated.
Comment #165
YesCT CreditAttribution: YesCT commentedI'll work on the wording changes asked for in #163
Comment #166
YesCT CreditAttribution: YesCT commentedwasn't sure if the "better" to use a link meant making a handbook page, or linking to an existing page. maybe this is text change for now, and the link can be a separate issue?
there is also some whitespace at the end of lines in the patch that we can clean up. not sure if we want that in the 7.x patch, or if it should keep it to match 8.x.
I looked to see if these text changes would mean a change also in a test, and didn't see any.
Comment #167
pwolanin CreditAttribution: pwolanin at SciShield commentedShould skip the check in update phase per klausi?
Comment #168
pwolanin CreditAttribution: pwolanin at SciShield commentedHere's a change to skip the check on update
Comment #169
YesCT CreditAttribution: YesCT commented1.
I asked @pwolanin about only giving the message if not during an update. The requirement will still show up on the status page, it just wont completely die when trying to run update. Seems ok to me.
--
2.
I also asked about no test fail with the change of the exception wording. Seems we could write a test that redefines random_bytes() to force an exception.
See, in bootstrap.inc
But, since random_bytes is in php 7, it might cause a fatal in 7, or we would need a condition in the test to only do the test logic if php is less than 7.
So adding a test might not be worth it.
Not sure if the CI problem with d7 php7 tests is still an issue. related: #2656548: Fully support PHP 7.0 in Drupal 7
Comment #170
YesCT CreditAttribution: YesCT commentedI read the entire (non library part) of the patch.
1.
the white space warning when I did the git apply is from the library we are including, so nothing to change there. see my #166.
2.
There looks like there might be some unrelated nit fixing. (changing comment that is over 80 chars, but not changing the meaning). This hunk should be reverted. https://www.drupal.org/core/scope#creep
I think this happened because of #149 which was looking at an interdiff, which made it looked like those lines were part of a larger change, but when looking at the patch, it is more clear that that section is not being changed.
3.
ha
nit. wrap to 80.
===
I think the only core gate blocking thing might be 2. the unrelated change. @pwolanin is gonna address both though really soon. so not fixing the nit myself.
Comment #171
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedWe can only write a test for PHP < 7.0 so it's not clear to me that's really useful and we don't have exiting test coverage of the byte quality or any such.
I reverted the text change since it's not strictly related to the fix as YesCT suggested.
The changes in #166 look like they match what David suggested.
Comment #172
YesCT CreditAttribution: YesCT commentedthanks! that looks great. expecting the bot to return green. setting to rtbc.
Comment #173
YesCT CreditAttribution: YesCT commentedComment #174
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedLinking to #2763787: Upgrade random_compat to latest version
Comment #175
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis still needs help text or a link which explains how to fix the problem, especially if we're going to go ahead and make this a hard requirement for new installs. Right now the exception message (from the library) just says "There is no suitable CSPRNG installed on your system", which is not useful. We need to give people some information about what they need to install on their server to be able to use this. It could be a link to some outside documentation (if that exists) or a page on drupal.org, etc.
Isn't this still needed for the fallback behavior of drupal_random_bytes()? (Perhaps it doesn't need to be in the bootstrap anymore, but seems like it's still necessary somewhere in the case where the fallback is being used.)
Maybe it should be "Secure" rather than "Successful"?
Also (minor),
catch (Exception $e) {
should be on its own line.Comment #176
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedAlso the help text doesn't look quite right for the case where an installation is actually being blocked... it says "this site is using a slow, less-secure fallback generator" but in fact the site isn't using anything because Drupal won't let you install it.
Comment #177
Heine CreditAttribution: Heine at LimoenGroen commentedI've removed the exception from the status message and added a link to https://www.drupal.org/docs/7/system-requirements/php#csprng which we can add.
In addition, I discovered that many of our sites will default to the fallback, because they use openbase_dir without /dev/urandom and do not have any other random source such as mcrypt or libsodium. This change probably needs a big note in the release notes.
Comment #178
Heine CreditAttribution: Heine at LimoenGroen commentedComment #179
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedCreated a stub at https://www.drupal.org/docs/7/system-requirements/php#csprng
Seems like we need some notes there and need to mention how to enable /dev/urandom under open basedir?
Possibly in the status message we need to check if open_basedir is set and omits that path as suggest that fix?
Comment #180
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedWe can apply a check for the open_basedir setting like the library does in terms of setting an error message
https://github.com/paragonie/random_compat/blob/master/lib/random.php#L8...
Comment #181
Heine CreditAttribution: Heine at LimoenGroen commentedI've added the suggestions in #179, #180. Not certain about the wording.
Comment #182
YesCT CreditAttribution: YesCT commentedI'm looking into adding the mt_rand seed back. Doing some research.
Comment #183
Heine CreditAttribution: Heine at LimoenGroen commentedI did not run a full install on Windows, just random_compat on Windows 10 with php-5.6.26-nts-Win32-VC11-x86.zip . random_compat was able to get random bytes.
The 5.6 binaries from windows.php.net all have mcrypt statically linked; this is also what random_compat uses. When I disabled the mcrypt option in random_compat, it threw an exception.
Comment #184
sarciszewski CreditAttribution: sarciszewski as a volunteer commentedPlease don't fall back to mt_rand(), ever.
http://www.openwall.com/php_mt_seed/
Comment #185
klausi@sarciszewski: we don't fall back to mt_rand() alone - in fact it plays a very small role in the PRNG algorithm we have. So the tool you linked above is worthless when you try to attack a Drupal site using the fallback algorithm.
That being said I agree that we should not have this fallback code since Drupal should not be in business of inventing a PRNG. What could possibly got wrong? :-P
It is a sad sacrifice to not break existing sites on old systems.
Comment #186
cweagans> It is a sad sacrifice to not break existing sites on old systems.
These sites are already broken and the worst part is, they probably don't know it.
Comment #187
heddnWhat if we watchdog log or throw warnings as loudly as we can? Drupal set message warning would be another option. Similar to what we did for deleted but not uninstalled modules in 7.50
Comment #188
cweagansIMO, we should just wrap all of the mt_rand() stuff in an off-by-default flag in settings.php and use the correct PRNG behaviors from random_compat. If a site is really incapable of using any of those methods, then they can opt in to the insecure mt_rand() implementation. If they opt in, we should continue to complain loudly at them and encourage them to move their site to a more capable host.
Comment #189
Heine CreditAttribution: Heine at LimoenGroen commentedAs to #186, whether sites are currently broken depends their PHP version and on the forking model of the server.
PHP bug 70014 is fairly uninteresting (and fixed in supported PHP releases) compared to PHP bug 71915.
How about doing a staged release?
- 7.5x - remove use of openssl_random_pseudobytes, prevent installation for new sites, complain on status / admin via requirements on existing sites.
- 7.6x - remove the fallback.
This would require a change to the message in #181 to tell the user that the fallback will be removed in 7.6x.
Both releases would need very explicit release notes explaining the changes and steps on how to solve the warning / error.
Comment #190
stefan.r CreditAttribution: stefan.r commentedAssigning to David_Rothstein for thoughts on #189.
Comment #191
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commented@sarciszewski - the fallback generator in the patch was originally contributed to Drupal by Solr Designer of openwall - so while it's less than ideal, its worst-case behavior isn't as terrible as some other options. Note also that we are including this fallback only to avoid breaking existing sites on upgrade.
Comment #192
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedadding back assigned to David.
I fell like we are over-thinking this - the fallback code has been in D7 a long time and by blocking new installs I think we are taking a reasonable step to push people to sort this out without causing a lot of pain.
I would also like to see someone describe an attack vector that makes it worth a lot of debate. About the only way I see to directly access some output of the random stream is by causing a new session ID to be generated. From that I don't think you can gain any real insight into the session ID the next user would get.
We can also encourage people to upgrade to php 7 which should never be broken. I suppose we could re-throw the exception on php 7+.
Comment #193
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedAligning messages and code with #2763787: Upgrade random_compat to latest version
Comment #194
YesCT CreditAttribution: YesCT commentedRead the whole patch.
Noticed the message changed from "The server" to "Drupal". (In addition to the other changes in this patch, this one is nice. Cause it makes the message more accurate, and ... while we try not to use the word Drupal in drupal messages, here it is appropriate, and also common in other requirement messages.)
Comment #195
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedTrivial code comment change to match #2763787: Upgrade random_compat to latest version in #104
Comment #197
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedOdd - seems to have been a sporadic failure in upgrade tests. Passed on re-test.
Comment #198
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThis is a David issue and a 7.60 target, so only a quick fly-by review:
--
I might be mistaken, but would it not be easier to just take a hash_base_64 (which uses the sites' private key) of the microtime(NULL) as input to mt_srand().
Would that not ensure a secure and non-guessable mt_srand() start?
Comment #201
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe patch itself might be ready, but the Status Report messages are linking to https://www.drupal.org/docs/7/system-requirements/php#csprng which doesn't have any actual documentation at the moment (it literally just says "See discussion on Drupal 7 issue"). Especially if the patch is going to block new installs, we need actual documentation that explains to people how they can fix the problem on their site.
And I have to say I'm still uneasy about blocking new installs - reading https://www.drupal.org/node/2763787#comment-11650589 and subsequent comments (plus Heine's comments above in the current issue) it seems like there are definitely real-world cases where this happens.
If we're talking about splitting this up into stages (like in #189) I would think the simplest way to do that would be (1) add the library and use it whenever possible, thereby getting more secure random bytes, (2) anything and everything related to adding warnings/errors in the user interface. Because (1) is certainly a completely uncontroversial and reasonably simple change, and it should give the actual security benefit to most sites.
Comment #202
joseph.olstadBumping to 7.61. This didn't make it into 7.60.
Comment #203
joseph.olstadComment #204
Ayesh CreditAttribution: Ayesh commentedCan we please make it that we don't make any performance disadvantage for PHP 7+ users? With the current patch, we include the random.php file for every request, even if the user is running PHP 7. The attached patch short-wires the
function_exists()
calls the random.php file does anyway, but before including the file.Comment #206
joseph.olstadComment #207
MustangGB CreditAttribution: MustangGB commentedComment #208
MustangGB CreditAttribution: MustangGB commentedComment #209
apadernoI take that it's too late to back-port it to Drupal 6, since it's not supported anymore.
Comment #210
joseph.olstadComment #211
izmeez CreditAttribution: izmeez commented@kiamlaluno Any interest in backport for Drupal 6 can be raised in the D6LTS issues, https://www.drupal.org/project/issues/d6lts
Comment #212
apaderno@izmeez Yes, but in this queque, Needs backport to D6 for a Drupal 7 issue is useless, as core maintainers don't do back-ports to Drupal 6 anymore.
Comment #213
izmeez CreditAttribution: izmeez commentedyes, understood.
Comment #214
izmeez CreditAttribution: izmeez commentedDid this get committed to Drupal 7.83 or is it still pending?
Comment #215
mcdruidNope, this wasn't in 7.83
We'll need to add it to the meta priorities issue for the next release (which I have not spun up yet, but will do soon unless somebody beats me to it).
Comment #216
izmeez CreditAttribution: izmeez commentedThanks, it will need a re-roll or more to make it work with Drupal 7.83 version.
Comment #217
mfbAnd the random_compat library is now on version 2.0.20 so presumably should be updated? (At this point, years later, the use of random_bytes() seems like the more useful part of this patch :)
Comment #218
Nikolay ShapovalovReroll for #144
Comment #219
sindurig CreditAttribution: sindurig at jobiqo - job board technology commentedReroll for #144 for Version 7.93
Comment #220
sindurig CreditAttribution: sindurig at jobiqo - job board technology commentedReroll for #144 for Version 7.93
(Missed Copyright in #219)
Comment #221
apadernoComment #222
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedLast three patches seems to be wrong and causing a noise in this issue. @zniki.ru and @sindurig - when doing rerolls, please check carefully which patches are you rerolling and which are the most recent.
I see patches #181, #195 and #204 as more refined comparing to the patch #144 (which was rerolled in #218). I do not see a reason to reroll such an old patch, especially if the reasons are not mentioned in the respective comment. Thanks!
Also the latest patch (#220) fails PHPLint, therefore switching this back to Needs Work.
--------------
Bottom line: PHP 5.6 was EOL four years ago. I think we would need to reconsider if it is still worth adding a new library to actively support the ancient PHP versions in this D7 phase (as there should not be any problems with this starting from PHP 7), or if we should close this.
Comment #223
klausiSorry for the confusion poker10, Sinduri was just rerolling this patch that we have been using for years when we were not on PHP 7 yet.
The reason that this critical bug is still open is that this is or was a security issue. The PHP ticket mentioned in the issue summary now has a CVE, but I have no idea if Linux distros patched this now in old PHP versions or not.
What we know for sure is that PHP 7+ guarantees us cryptographically secure values with the random_bytes() function. The old code around openssl_random_pseudo_bytes() and mt_rand() in drupal_random_bytes() could be a security liability for us in the future, even on PHP 7+.
Now that PHP 5 is EOL for many years we could do a much simpler approach: if the function random_bytes() exists use that and return early. If not then keep using the old code around openssl_random_pseudo_bytes() unchanged as it is now. That way we leave PHP 5 installations open for potential security issues, but at least they can upgrade D7 core more smoothly and we don't intentionally break them. The result is a very simple patch here.
What do you think?
Comment #224
andypostIt could even be improved to support new in PHP 8.2 Random extension https://www.php.net/releases/8.2/en.php#random_extension
Comment #225
mfbHere's a pretty simple patch to use random_bytes() if available. I didn't provide an interdiff since the preferred version hasn't yet been rerolled.
In case there are still any fans of PHP 5 and random_compat out there, it looks like there has only been one new version of random_compat released since I last commented in this issue one year ago (#217), so incorporating and chasing that library seems feasible - but I'd agree with @klausi in #223 that it just doesn't seem necessary at this point in time.
Comment #226
klausiPerfect, this is exactly what I had in mind!
I queued a test run on PHP 5.3 to ensure it passes there as well. If that is green we should be good to go!
Comment #227
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedThanks for the explanation @klausi and for the patch @mfb. This approach looks good!
Comment #229
mcdruidThis is a great enhancement for D7 on modern PHP. Thanks everyone!
Comment #231
joseph.olstadFantastic work! This approach is all of simple, elegant and productive all at once. Assists those of us that need it, without disrupting those that do not. Happiness for everyone! This is what I call a win for all. The best wins go unnoticed. Only those of us that are watching closely can see it.