#28633 closed enhancement (fixed)
Generate better random numbers
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Security | Keywords: | needs-testing has-patch early |
Focuses: | Cc: |
Description
Never used Trac/SVN at all, so I apologize if I'm doing this wrong. I want to submit what a github user would call a pull request. Although this is classified as security, it's not exactly a critical 0day vulnerability I found or anything.
Attachments (15)
Change History (89)
#2
@
11 years ago
Okay, SVN is giving me a headache. Here's the pull request on github. I give up on trying to figure out how to work SVN/Trac.
https://github.com/WordPress/WordPress/pull/84
Basically, this adds a wp_secure_rand() function that takes 1 parameter (the number of bytes desired) and returns a raw binary stream. This should be use in place of rand(), mt_rand(), uniqid(), etc in places where the unpredictability of the output matters to security. (e.g. password resets)
I've also patched wp_rand() to call this function internally, so that all random numbers generated by wp_rand() benefit from this greater security. Where possible, I tried to make changes seamless to developers.
#3
@
11 years ago
Here's links to a git-produced patch and diff for the pull request:
https://github.com/WordPress/WordPress/pull/84.patch
https://github.com/WordPress/WordPress/pull/84.diff
#4
@
11 years ago
I apologize for the ms.php and schema.php patch being literally every line in the file. I think there's a whitespace issue somewhere. The only lines that should be changed are the ones with the string wp_secure_rand in my version.
#5
follow-up:
↓ 11
@
11 years ago
- Keywords needs-patch needs-testing added
You can get GitHub to hide whitespace changes by tacking the w=1
parameter onto the URL: https://github.com/sarciszewski/WordPress/commit/22f587ea4b22ed0e4473a52832b4fbd5ecaa58c1?w=1
Regardless, the patch will need re-doing without the whitespace changes.
#7
@
10 years ago
- Severity changed from normal to major
The patch I provided resolves CVE-2014-6412 -- I would appreciate it if this was given a second look, esp. by the security team.
#8
@
10 years ago
I'm just seeing this. If you've requested a CVE identifier, I'm thinking maybe this should have been reported privately.
http://make.wordpress.org/core/handbook/reporting-security-vulnerabilities/
#9
@
10 years ago
I requested a CVE identifier this week, the patch was submitted in June. I've reported the avenue to exploitation to your security team.
The actual exploitation (which I will not discuss publicly) is not trivial, and I don't expect most script kiddies are capable of doing so.
EDIT: The reason is, I didn't realize until this past Friday how it could be leveraged to abuse a WordPress installation. While I understand that security holes are sensitive matters, the fact remains you had the patch for months and nobody did anything with it.
#10
@
10 years ago
As a follow-up, I feel that I should mention that I don't intend on dropping exploit code on Full Disclosure or anything reckless like that. If the patch I provided isn't sufficient for you, feel free to take your time to fix the problem and make sure the fix is correct. Just, please, keep me updated on where everyone is.
#11
in reply to:
↑ 5
;
follow-up:
↓ 12
@
10 years ago
Replying to johnbillion:
Regardless, the patch will need re-doing without the whitespace changes.
I think line ending format difference is the issue.
#12
in reply to:
↑ 11
;
follow-up:
↓ 13
@
10 years ago
Replying to SergeyBiryukov:
Replying to johnbillion:
Regardless, the patch will need re-doing without the whitespace changes.
I think line ending format difference is the issue.
If the github repo were authoritative, patching this would be a lot smoother on my end. But I don't have the time or patience to learn SVN right now.
#13
in reply to:
↑ 12
@
10 years ago
Replying to sarciszewski:
If the github repo were authoritative, patching this would be a lot smoother on my end. But I don't have the time or patience to learn SVN right now.
This might help:
http://scribu.net/wordpress/svn-patches-from-git.html
http://scribu.net/wordpress/contributing-to-wordpress-using-github.html
#14
@
10 years ago
This is now being handled by the security team and with sarciszewski privately. No need for further conversations here, thanks.
#17
@
10 years ago
Current patch does the following:
- Adds functions:
wp_random_bytes(int $number)
wp_random_positive_int()
wp_secure_rand(int $min, int $max)
- Does not patch
wp_rand()
like the previous patch did. - Updates
wp_generate_password()
to usewp_secure_rand()
instead. - Adds a unit test in
tests/phpunit/tests/functions.php
I'd like to request this gets bumped up for evaluation in 4.1 or 4.1.1
This ticket was mentioned in Slack in #core by sarciszewski. View the logs.
10 years ago
#19
@
10 years ago
The latest patch should now work on incredibly ancient versions of Windows. (a.k.a. 5.2)
x
Supports unsupported PHP versions
x
Has unit tests
This should now be fully compatible with your requirements.
This ticket was mentioned in Slack in #core by sarciszewski. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by sarciszewski. View the logs.
10 years ago
#23
@
10 years ago
From CodeIgniter 3.0-dev:
is_php('5.4') && stream_set_chunk_size($fp, $length);
I think this can resolve the issue Stefan Esser pointed to.
@
10 years ago
Edited the patch file manually because git://develop.git.wordpress.org is not working atm
This ticket was mentioned in Slack in #core by sarciszewski. View the logs.
10 years ago
@
10 years ago
Alternative, based on https://github.com/resonantcore/lib/blob/develop/src/Secure.php which I have tested extensively.
#25
@
10 years ago
This coming in as a public bug report immediately triggered alarm bells. When this happens, and it does sometimes, the security team mobilizes to see whether the report is indeed a vulnerability. It was pretty quickly determined that while there may be something that could be improved here, there was no smoking gun here. At some point, @sarciszewski started emailing me directly, rather than the security team or by posting here. I never saw these due to aggressive email filters. I get a lot of email and don't have time to read lower priority stuff in a timely manner; there's a reason why it's a security team and not a nacin team.
@sarciszewski then posted this to FD: http://seclists.org/fulldisclosure/2015/Feb/42. I'm paraphrasing an old movie here: it's not what was done, it's how it was done. I deal with a lot of tiring stuff every day, and the tone wasn't all that necessary to get the results he wanted. If he really does think WordPress has a critical issue, then the facts would ideally speak for themselves. I sent a tweet in reply to @sarciszewski that vaguely tried to convey my feelings about the tone (not content). That then resulted in a lot of random people replying at me in shall we say less than complimentary ways, because they found the tweet through Reddit, so I deleted it. It's archived on Reddit if you wish to read it.
Like many complicated security-related issues in WordPress, the original code here was written far before I got involved on the project. Someone else mentioned on Reddit some previous circumstances where I ended up somehow being the public face for WordPress.com delivering their cookies in the clear (1), and also WordPress not having user cookies backed by a server-side session (2). For the first part, yeah, it was a bug, and they addressed all aspects of it in under a week. (Also note, I don't work at Automattic or at WordPress.com, and never have.) That second part is a known issue going back to the b2/cafelog project WordPress was forked from 12 years ago. It wasn't something that could be changed overnight. Everyone of course knew it was an issue. Once it became clear we did have a viable and performant solution for it, we shipped it.
There was a reason WordPress cookies only last for 14 days even when "Remember me" is checked, and why of course we enforce cookie expiration times as part of our auth cookie hash. Now that we have sessions, it's perhaps time to extend that beyond 14 days, but with most sites not having SSL, that's still not exactly ideal either. Security and usability always have a tradeoff. And, hey, look, I sure as hell didn't write it that way more than a decade ago.
In the course of my research on this CSPRNG bug report, I found that the original code here was written in 2008 in response to a report by Stefan Esser, a well-known security researcher who is intimately familiar with PHP, and has reported a few issues to us over the years. I couldn't locate any conversations on our end, but I did some research and was comfortable with the solution we had as a result of that work, as hacky as it was. Some of Stefan's comments on Twitter have supported that sentiment, though I've asked more specifically whether he also thinks that's true.
For the record: I think it would be nice to make an adjustment here. This is precisely the kind of security debate that a maintainer loves to see happen out in the open, in a healthy and positive manner, because it only means the software will get better.
#26
@
10 years ago
In schema.php
, $hostname = substr( md5( time() ), 0, 6 ) . '.' . $domain; // Very random hostname!
is used as a random enough hostname only to check that wildcard DNS is supported for subdomain multisite installs. We can leave it out of any improvements.
#27
follow-up:
↓ 28
@
10 years ago
We can leave it out of any improvements.
I disagree strongly for one simple reason: Some developers can and will refer to the WordPress core as a source of authority and inspiration. If we expose an insecure method as "very random", it may lead to bad habits propagating into the next generation.
I learned how to use MySQL from PHP by reading the source code in nulled copies of Invision Power Board circa 2002.
#28
in reply to:
↑ 27
@
10 years ago
Replying to sarciszewski:
If we expose an insecure method as "very random", it may lead to bad habits propagating into the next generation.
I learned how to use MySQL from PHP by reading the source code in nulled copies of Invision Power Board circa 2002.
Absolutely. Improved documentation to clarify code is always good. The "very random" in this case is—or I read it as— tongue in cheek in the context of the surrounding code, though it could be updated.
The md5()
approach itself doesn't really need to be made more complex as security is not a factor here.
#29
follow-up:
↓ 30
@
10 years ago
The
md5()
approach itself doesn't really need to be made more complex as security is not a factor here.
Are you saying to revert it, or "since you made it secure random anyway, leave it, but it wasn't necessary"?
Taking 3 random bytes and converting it to hex is actually simpler than taking a cryptographic hash of the current time, then grabbing the first 6 hexits from it, since the secure random is abstracted away.
#30
in reply to:
↑ 29
@
10 years ago
Replying to sarciszewski:
Are you saying to revert it, or "since you made it secure random anyway, leave it, but it wasn't necessary"?
Let's leave it out of any patch on this ticket. We can address any simplification or documentation of that segment elsewhere.
This ticket was mentioned in Slack in #core by sarciszewski. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by sarciszewski. View the logs.
10 years ago
@
10 years ago
Patch 28633.6 with fixed syntax errors (few missed closing parentheses and try-catch syntax)
#33
@
10 years ago
I think you might need a php version check on the if (function_exists('mcrypt_create_iv'))
line as well. On a Windows machine running 5.2.x, where the mcrypt extension is also enabled, a call made using MCRYPT_DEV_URANDOM
will cause a "Cannot open source device" warning. This warning will prevent logins if display_errors is enabled (which it sadly is by default on most hosts).
Alternatively, error suppression by using @mcrypt_create_iv()
would probably work just as well.
@
10 years ago
@Otto42 - You're absolutely correct. Also, there's a bug in mcrypt on Windows for PHP < 5.3.7 where it always produces insufficient entropy.
#35
@
10 years ago
I've spent some time looking into this, and as a hardening-only patch, this seems like it's going in a good direction. I'm approaching this explicitly as a hardening-only patch as I believe wp_rand()
we have in WordPress is already safe from prediction (and related) attacks, but that it was also written when the latest version of PHP available was 5.2.8, so did not have most of these available to it.
Here's a rundown of the currently available sources that we can use:
- Mcrypt
- If the extension is available
- PHP 5.3.7+ only for Windows (uses native Crypto Library)
- /dev/urandom
- If *nix and readable
- may eat entropy in PHP < 5.3.3 (as it'll use read-ahead buffering)
- not available on Windows
- OpenSSL
- If the extension available
- PHP 5.3.4+ only for Windows, as it's interactive-display random seed uses lots of processing time (30-60s+) on a non-interactive screen
- Uses Windows native Crypto API in PHP 5.4+ (might be a better minimum for windows)
- May use a non-strong crypto source on some "rare" systems (see
$crypto_strong
param) - Also worth noting OpenSSL has a myriad of random generators, I believe at least one of these leverages
/dev/random
(or urandom/srandom) on supported platforms, but I'm unable to determine what platforms/OpenSSL versions at a glance
- Windows specific: CAPICOM
- Deprecated; For Windows 2008 and lower (XP, Visa, 7, 2003, 2008)
- Available when the optional package is installed
- Haven't tested this, seems like it can have permissions issues
- Windows specific: DOTNET API
- Supported in Windows 2008 R2 and greater with .NET 1.0 or greater bindings enabled
- Haven't tested this, seems like it can have permissions issues
- Reports of it not working reliably at all in PHP due to incompatible types
It seems that randomness support on Windows servers is not-great in older versions of PHP, and we should potentially ditch windows-specific external API's and leverage OpenSSL/Mcrypt instead as they work reliably with the native Windows crypto API's in PHP 5.3.7/5.4+.
The simple approach we can take here, assuming that our existing wp_rand()
is not insecure, is to simply beef up wp_rand()
to leverage these alternative random sources when available.
This also leads me to questioning if we should support /dev/urandom
which although is available on many hosting environments, is also less and less useful as OpenSSL or Mcrypt are available on more installs. Although i'm not sure of the penetration of Mcrypt, OpenSSL is very commonly supported. The code to use urandom
is lightweight and straight-forward however, so I don't see a significant reason not to include it.
So, I've attached two patches
- 28633.8.diff This is my WIP patch against the ones suggested here and was based off of 28633_with_md5.patch
- PHP syntax issues corrected
- Windows version checks added
- uses our
mbstring_binary_safe_encoding()
function to avoid the need forwp_binsafe_strlen()
- rewrites the urandom handling in
wp_random_bytes()
to be more explicit and cleaner - fixes
wp_secure_rand()
to return inclusive values between$min
and$max
while making the code easier to read (It can also now handlewp_secure_rand(1, 2)
which it couldn't previously) - a bunch of other formatting and code cleanliness issues
- 28633.9.diff however is an initial rewrite into what I think we should aim for
- it renames the functions (although I'm really not sold on the names I've chosen at all, just wanted to remove the secure keyword)
- moves towards ditching
wp_secure_rand()
and simply havingwp_rand()
- Drops the Windows-specific branches but leaves urandom in play.
- In this scenario these functions would live elsewhere, i'm just including them in this file for ease of patch comparison
Thoughts? Suggestions?
#36
@
10 years ago
I am rather fond of 28633.9.diff personally. Avoiding wp_binsafe_strlen()
is a good idea too :)
This ticket was mentioned in Slack in #core by sarciszewski. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by sarciszewski. View the logs.
10 years ago
#39
@
10 years ago
It is probable that random_bytes
and random_int
will be introduced in PHP 7.0. The vote will be finished at March 28th.
#40
@
10 years ago
- Keywords early added
- Milestone changed from Awaiting Review to Future Release
- Severity changed from major to normal
I'd prefer to land these changes at the start of a cycle, to allow for full testing by everyone and to surface any issues from reliance upon any of the functions.
Based on the PHP7 RFC, which looks like it'll succeed, we should change the direction of the patch to simply provide a compat layer for the PHP7 function signatures instead, which will benefit us in the long term.
The changes:
wp_external_random_bytes()
->random_bytes()
not sure if the byte length changes, or if it's a required param..wp_external_rand()
->random_int()
which also needs to support negative numbers, ie.random_int( -1000, -10 )
should work.wp_external_random_positive_int()
becomes an internal private function, only defined when the wrapper for random_int is, however it might not even be needed based on the negative number support forrandom_int()
#42
@
10 years ago
wp_external_random_positive_int()
is there to generate a random number along a range. If you use bitshifting to get 0..1023
then add the minimum (-1000
), that should be suitable for (-1000, -10)
.
(The result from wp_external_random_positive_int()
will be discarded if it exceeds 990
in that case, and it will start over.)
#43
@
10 years ago
Hey, it's been a while.
I see that this still hasn't gotten merged. This might be a good thing.
I've put together a polyfill library for PHP7's random_int()
and random_bytes()
functions for PHP 5.x projects and (with WordPress in mind) I kept PHP 5.2 compatibility.
https://github.com/paragonie/random_compat
@dd32 If you would prefer to use an external library and have wp_external_random_bytes()
just invoke it rather than try to manage this internally, that might make upgrading/patching any bugs that crop up in the future more sane. "Upgrade random_compat to 1.0.1" is probably an easier change to audit than a comprehensive rewrite of core functions.
This ticket was mentioned in Slack in #core by miss_jwo. View the logs.
9 years ago
#46
@
9 years ago
- Milestone changed from Future Release to 4.4
- Owner set to dd32
- Status changed from new to reviewing
I'm marking for 4.4 for review purposes. I'll go through the libraries code and see if there's going to be any issues - at first glance It all looks pretty straight forward and compatible, so shouldn't be an issue.
#47
@
9 years ago
Sounds great! v1.0.0 has been tagged and released. Additionally, it should work all the way back on 5.2.4 on Windows without ext/mcrypt. :)
https://github.com/paragonie/random_compat/releases/tag/v1.0.0
This ticket was mentioned in Slack in #core by sarciszewski. View the logs.
9 years ago
#49
@
9 years ago
Fun facts: wp_rand()
only returns positive numbers (no matter the input), and it also doesn't care about the order of $min
and $max
.
@sarciszewski Since PHP's implementation of random_int()
has changed in the last few days since PHP7-RC2 was released (They've switched from PHP warnings to Exceptions, which your compat library deals with), I'd like to test this against PHP7-RC3 which is due to be released at the end of the week. I'm having a hard time comparing the compat library and PHP7's behaviours to ensure we don't have any issues at release time.
#51
@
9 years ago
Just a note that RC3 is out now if somebody would like to test, so we don't lose momentum here. :)
#52
@
9 years ago
28633.diff is tested against PHP 5.6 & PHP7-RC3 (This patch doesn't include the random_compat library itself though, for easier review)
A few things to note
wp_rand()
always returns positive numbers, even if a negative range is offeredwp_rand()
accepts the parameters in either order- streamlined the
try {} catch {} catch {}
to avoid needing to use version comparisons
The only issue I noted in the compat library, is that PHP7's random_int()
claims to accept Integers, but accepts numeric types (floats/numeric strings) happily, and it appears that wp_rand()
probably does too. As such, https://github.com/paragonie/random_compat/compare/master...dd32:compat-types?expand=1 is my work-in-progress at allowing it.
I was delayed in testing this thanks to conferences & the PHP7 packages being delayed (I'm testing using Webtatic PHP7 packages for Centos/RHEL 6 (In case anyone wants to also verify my experience)
#53
@
9 years ago
Should it return abs( $val );
or absint( $val );
?
Your patch for random_compat makes sense. I can't believe I overlooked that. As soon as you're ready, I'll merge it and tag version 1.0.2, which is what I'd propose to be included in WP 4.4.
Thanks a ton, Dion.
#54
@
9 years ago
Should it return abs( $val ); or absint( $val );?
Since random_int()
should be returning an int already, they're mostly the same, however for consistency you're right, we should use absval()
.
Your patch for random_compat makes sense. I can't believe I overlooked that. As soon as you're ready, I'll merge it and tag version 1.0.2, which is what I'd propose to be included in WP 4.4.
Thanks for the super-fast merge! I couldn't find any other edge-cases in my testing, so a pretty good job :) The PHP7-style exception handlers seem solid too.
#55
@
9 years ago
One minor thing at a glance is that there is no reference in src/wp-includes/compat.php to the version of PHP that random_int is introduced. I think it makes sense to include a reference so that years in the future, when this compat library is no longer needed, it can be removed.
#56
@
9 years ago
That's a good point. Who knows when 5.6 will stop being supported by WP? I for one might be long gone when hosting companies finally get their act together and stay up-to-date.
#57
@
9 years ago
After asking for more reviews of the code here, the only issue that's been raised, is the following use-case:
$val = wp_rand(); // (int) 0
This is because $min
and $max
are 0
by default, so.. it makes sense, but wp_rand()
decides that this should instead be interpreted as a maximum of 4294967295
. I think we can/should just use 4294967295
when $max = 0
#59
@
9 years ago
Lets see if anything breaks :)
A few thoughts:
- I've used
$max_random_number
which is a 32bit maximum, 64bit systems can achieve much greater sizes. We should update the function to handle 64bit values on 64bit systems. - I've used random_compat
master
here, just for clarity over what's included - that is https://github.com/paragonie/random_compat/commit/456d84c8823207a18fb146c29f8b9e3aa0a9a31c - I've included no tests here, based on that
random_compat
has it's own set of tests
#61
@
9 years ago
Unfortunately we've run into a PHP 5.2 compatibility issue:
https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/84210246
Fatal error: Class Error contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Throwable::getPrevious) in src/wp-includes/random_compat/error_polyfill.php on line 48
It looks like this was caused by https://github.com/paragonie/random_compat/commit/90da7f3c0f4e92a1c44871de038e801d764ccc93
I've reverted it for now as I can't debug it properly on a PHP 5.2 system right this second.
#63
@
9 years ago
@sarciszewski
Here's PHP parsing the committed code: https://3v4l.org/ei523
And here's PHP parsing the code with those method declarations removed: https://3v4l.org/Qsse1
No output is good, so it looks like removing those declarations fixes it, and is compatible with all PHP5 releases.
I'll recommit shortly with that change.
#67
@
9 years ago
- Resolution set to fixed
- Status changed from reviewing to closed
Calling it fixed. Reopen if the sky is falling.
#73
@
9 years ago
I'm marking this as fixed. Please open new tickets for any further issues and reference them here.
Thanks for your interest in WordPress, sarciszewski!
If your issue is a security enhancement rather than a vulnerability, then go ahead and post it here.
If you think it might be a vulnerability, or if you're not sure, then you should email your report to
security@wordpress.org
rather than posting it here.Thank you