Add support for getrandom(2), add type check on file descriptor #1513

paragonie-scott · 2015-09-08T23:50:39Z

Enhancements for random_bytes(). cc @SammyK @ircmaxell @lt

KalleZ · 2015-09-09T00:05:54Z

Hi

Patch looks good, the only thing (minor) is the C++ comment but thats hardly worth mentioning (since we confront C89)

cc @SammyK @weltling @Tyrael

paragonie-scott · 2015-09-09T00:09:12Z

You mean the // one? I can fix that too.

KalleZ · 2015-09-09T00:16:05Z

ext/standard/random.c

@@ -85,8 +89,29 @@ static int php_random_bytes(void *bytes, size_t size)
 	}
 #elif HAVE_DECL_ARC4RANDOM_BUF
 	arc4random_buf(bytes, size);
+#elif defined SYS_getrandom
+	/* Linux getrandom(2) syscall */
+	size_t read_bytes = 0;


Although we mainly do this on Windows, but declarations at first but I'm not sure there is any recent (and sane) compiler that will complain here, but not a biggie.

KalleZ · 2015-09-09T00:17:03Z

Yeha I mean the double slash, but else its fine. I added an inline comment note as well

paragonie-scott · 2015-09-09T00:19:43Z

Okay, discussing this in room 11 and making a few tweaks. Will send a squashed commit as soon as the test pass locally.

paragonie-scott · 2015-09-09T01:40:38Z

(Closed & reopened because Travis appeared to be sticking.)

paragonie-scott · 2015-09-09T10:14:16Z

Oh hey, the tests passed. Neat. :)

lt · 2015-09-09T10:19:31Z

Had getrandom(2) in originally and removed it for unremembered reasons. Probably due to lack of kernel support back then, but here we are in the future.

paragonie-scott · 2015-09-09T10:23:37Z

The future is a wonderful place, indeed. :)

The original patch was just going to be checking the fstat result for sanity, but while I was here I implemented it too. :)

paragonie-scott · 2015-09-09T17:30:53Z

All: The latest commit also removes /dev/arandom, for reasons outlined here.

SammyK · 2015-09-09T21:11:11Z

Had getrandom(2) in originally and removed it for unremembered reasons.

Found the quote from @ircmaxell while discussing original RFC:

Another thought to avoid the file descriptor issue was in newer kernels just tie in the linux getrandom(2) syscall and BSD getentropy(2) syscall directly. But based on getentropy(2)'s docs it specifies that it's only meant to seed an arc4 generator, not provide a stream of bytes.

SammyK · 2015-09-09T21:13:20Z

ext/standard/random.c

+		     |      |           |
+		    [#######=============] (we're going to write over the = region)
+		             \\\\\\\\\\\\\
+		              amount_to_read


I'm digging the ascii art comments!

ircmaxell · 2015-09-14T22:25:39Z

So apparently the 256 byte limit is about checking return results, and not a fundamental design limit. So +1 from me.

paragonie-scott · 2015-09-15T00:04:25Z

(Travis was stuck.)

nikic · 2015-09-15T09:17:01Z

I've recently become aware of the fact that arc4random_buf on many operating systems (including current OSX) is still an actual RC4 generator (skipping some initial values), with potentially bad seeding at that. I'd suggest we drop this as well and only use getrandom or /dev/urandom.

nikic · 2015-09-15T09:17:52Z

ext/standard/random.c

@@ -30,6 +30,10 @@
 #if PHP_WIN32
 # include "win32/winutil.h"
 #endif
+#ifdef __linux__
+# include <sys/syscall.h>


This header shouldn't be necessary anymore, right?

No, I forgot to remove that.

lt · 2015-09-15T09:31:33Z

I'm sure I read somewhere that OSX actually uses arc4random for urandom. Will have a quick look around to see if I can find evidence of that.

Aside from older BSDs which other OS are you aware of? (so we can research the truth there too)

nikic · 2015-09-15T09:37:20Z

@lt Wikipedia says:

In OpenBSD 5.5, released in May 2014, arc4random was modified to use ChaCha20.[13][14] As of January 2015, implementation of arc4random in NetBSD[15][16] also uses ChaCha20, however, implementation of arc4random in FreeBSD,[17] Linux's libbsd,[18] and Mac OS X[19] are still based on RC4.

And the OSX implementation was just turned up in my Twitter feed: https://opensource.apple.com/source/Libc/Libc-1044.10.1/gen/FreeBSD/arc4random.c

lt · 2015-09-15T09:49:55Z

Discarding (at least) the initial 256 bytes mitigates the known biasing weaknesses in RC4 as far as I am aware. So is the only issue here the dodgy seeding?

nikic · 2015-09-15T12:38:55Z

@lt I'm not up to date on RC4 research, but as far as I remember multi-byte biases (unlike the single-byte ones) recur periodically and affect more than the first few bytes.

paragonie-scott · 2015-09-15T21:48:02Z

The 256-byte issue with RC4 is for encrypting information as a stream cipher. It probably doesn't degrade the security of Fortuna implementations, especially since the kernel will reseed it frequently.

That said, there are still exploitable RC4 biases even after discarding the first N bytes of output. ChaCha20 should be the standard these days.

paragonie-scott · 2015-09-21T07:55:02Z

The 1, 9 came from http://insanecoding.blogspot.com/2014/05/a-good-idea-with-bad-usage-devurandom.html and confirmed by ls -lah /dev | grep urandom.

@defuse

Fix to_read, throw exception if syscall fails Fixes thanks to feedback from sarnold at ##crypto on freenode Correction on error conditions Remove dead code (thanks @defuse) It turns out getrandom can take >256, getentropy refuses. Better semantics Thanks @defuse for catching my silly mistake here Cast to size_t to be explicit Let's simplify the logic a bit Let's be consistent; define everything before we do any logic Continuously check that the file descriptor is still a valid one Add device type check on fd initialization

smalyshev · 2015-09-28T20:23:20Z

ext/standard/random.c

+		}
+#ifdef __linux__
+		// Make sure that /dev/urandom is the proper urandom device on Linux
+		if (st.st_rdev != makedev(1, 9)) {


I don't like hardcoding this. Does it come from some standard that /dev/urandom on Linux always must be the same? Why we need to check this anyway? If someone has permissions to mess with /dev to fool us, the system has way more problems than PHP could fix. Checking if the file has been opened successfully, e.g. to see if we aren't in broken chroot() env or broken permissions - that's fine, but beyond that I don't think we should bother, and especially hardcode device numbers inside the code.

Does it come from some standard that /dev/urandom on Linux always must be the same?

http://insanecoding.blogspot.co.uk/2014/05/a-good-idea-with-bad-usage-devurandom.html

The Linux user manual page for the random devices explicitly informs the reader of these magic numbers, so hopefully they won't change. I have no official sources for the magic numbers on the other OSs.

https://www.kernel.org/doc/Documentation/devices.txt

I'm removing it for now. If this turns out to be a source for security issues down the line, I'd be surprised. But if it does, we can always look back and have a reference point for how to re-implement it.

paragonie-scott · 2015-09-28T20:41:54Z

@jedisct1 made a similar argument against makedev in jedisct1/libsodium#300, and since newer kernels will just provide getrandom there's not much benefit in supporting these magic numbers.

php-pulls · 2015-09-29T07:18:00Z

Comment on behalf of ab at php.net:

Merged into 7.0.

Thanks.

php-pulls · 2015-09-29T08:38:13Z

Comment on behalf of ab at php.net:

and closed :)

SammyK · 2015-09-29T14:54:47Z

🎉

dragoonis · 2015-09-29T16:57:19Z

👍

nikic · 2015-10-05T14:59:09Z

Could you please take a look at https://bugs.php.net/bug.php?id=70641?

jedisct1 · 2015-10-05T15:33:34Z

Including linux/ headers is asking for trouble regarding portability across Linux distros.
You'd better use syscall(SYS_getrandom, ...) which only requires sys/syscall.h.

Also, regarding the file type check: jedisct1/libsodium#302 :(

See: * https://bugs.php.net/bug.php?id=70641 * php#1513 (comment)

See: * https://bugs.php.net/bug.php?id=70641 * #1513 (comment)

KalleZ reviewed Sep 9, 2015
View reviewed changes

paragonie-scott closed this Sep 9, 2015

paragonie-scott reopened this Sep 9, 2015

SammyK reviewed Sep 9, 2015
View reviewed changes

paragonie-scott closed this Sep 15, 2015

paragonie-scott reopened this Sep 15, 2015

nikic reviewed Sep 15, 2015
View reviewed changes

paragonie-scott added 2 commits September 21, 2015 04:03

Different error message if makedev check fails

5415d27

smalyshev reviewed Sep 28, 2015
View reviewed changes

smalyshev added the Feature label Sep 28, 2015

Remove makedev() check.

7ce623b

php-pulls closed this Sep 29, 2015

paragonie-scott mentioned this pull request Oct 5, 2015

Fix bug #70641 #1549

Closed

paragonie-scott added a commit to paragonie-scott/php-src that referenced this pull request Oct 5, 2015

random_* - Better compatibility on Linux distros

ec62c0b

See: * https://bugs.php.net/bug.php?id=70641 * php#1513 (comment)

paragonie-scott mentioned this pull request Oct 5, 2015

random_* - Better compatibility on Linux distros #1550

Closed

php-pulls pushed a commit that referenced this pull request Oct 5, 2015

Fixed bug #70641 (Random_* - Better compatibility on Linux distros)

63617a6

See: * https://bugs.php.net/bug.php?id=70641 * #1513 (comment)

Add support for getrandom(2), add type check on file descriptor #1513

Add support for getrandom(2), add type check on file descriptor #1513

Uh oh!

Conversation

paragonie-scott commented Sep 8, 2015

Uh oh!

KalleZ commented Sep 9, 2015

Uh oh!

paragonie-scott commented Sep 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KalleZ commented Sep 9, 2015

Uh oh!

paragonie-scott commented Sep 9, 2015

Uh oh!

paragonie-scott commented Sep 9, 2015

Uh oh!

paragonie-scott commented Sep 9, 2015

Uh oh!

lt commented Sep 9, 2015

Uh oh!

paragonie-scott commented Sep 9, 2015

Uh oh!

paragonie-scott commented Sep 9, 2015

Uh oh!

SammyK commented Sep 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ircmaxell commented Sep 14, 2015

Uh oh!

paragonie-scott commented Sep 15, 2015

Uh oh!

nikic commented Sep 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lt commented Sep 15, 2015

Uh oh!

nikic commented Sep 15, 2015

Uh oh!

lt commented Sep 15, 2015

Uh oh!

nikic commented Sep 15, 2015

Uh oh!

paragonie-scott commented Sep 15, 2015

Uh oh!

paragonie-scott commented Sep 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paragonie-scott commented Sep 28, 2015

Uh oh!

php-pulls commented Sep 29, 2015

Uh oh!

php-pulls commented Sep 29, 2015

Uh oh!

SammyK commented Sep 29, 2015

Uh oh!

dragoonis commented Sep 29, 2015

Uh oh!

nikic commented Oct 5, 2015

Uh oh!

jedisct1 commented Oct 5, 2015

Uh oh!

Uh oh!