Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2bb7e65
Add support for getrandom(2), add type check on file descriptor
paragonie-scott Sep 9, 2015
5415d27
Different error message if makedev check fails
paragonie-scott Sep 21, 2015
7ce623b
Remove makedev() check.
paragonie-scott Sep 28, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ext/standard/config.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,11 @@ dnl Check for arc4random on BSD systems
dnl
AC_CHECK_DECLS([arc4random_buf])

dnl
dnl Check for getrandom on newer Linux kernels
dnl
AC_CHECK_DECLS([getrandom])

dnl
dnl Setup extension sources
dnl
Expand Down
55 changes: 50 additions & 5 deletions ext/standard/random.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@
#if PHP_WIN32
# include "win32/winutil.h"
#endif
#ifdef __linux__
# include <linux/random.h>
#endif

#ifdef ZTS
int random_globals_id;
Expand Down Expand Up @@ -75,6 +78,7 @@ PHP_MSHUTDOWN_FUNCTION(random)
/* }}} */

/* {{{ */

static int php_random_bytes(void *bytes, size_t size)
{
#if PHP_WIN32
Expand All @@ -85,26 +89,67 @@ static int php_random_bytes(void *bytes, size_t size)
}
#elif HAVE_DECL_ARC4RANDOM_BUF
arc4random_buf(bytes, size);
#elif HAVE_DECL_GETRANDOM
/* Linux getrandom(2) syscall */
size_t read_bytes = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

size_t amount_to_read = 0;
ssize_t n;

/* Keep reading until we get enough entropy */
do {
amount_to_read = size - read_bytes;
/* Below, (bytes + read_bytes) is pointer arithmetic.

bytes read_bytes size
| | |
[#######=============] (we're going to write over the = region)
\\\\\\\\\\\\\
amount_to_read
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm digging the ascii art comments!


*/
n = getrandom(bytes + read_bytes, amount_to_read, 0);

if (n == -1) {
if (errno == EINTR || errno == EAGAIN) {
/* Try again */
continue;
}
/*
If the syscall fails, we are doomed. The loop that calls
php_random_bytes should be terminated by the exception instead
of proceeding to demand more entropy.
*/
zend_throw_exception(zend_ce_exception, "Could not gather sufficient random data", errno);
return FAILURE;
}

read_bytes += (size_t) n;
} while (read_bytes < size);
#else
int fd = RANDOM_G(fd);
struct stat st;
size_t read_bytes = 0;
ssize_t n;

if (fd < 0) {
#if HAVE_DEV_ARANDOM
fd = open("/dev/arandom", O_RDONLY);
#elif HAVE_DEV_URANDOM
#if HAVE_DEV_URANDOM
fd = open("/dev/urandom", O_RDONLY);
#endif
if (fd < 0) {
zend_throw_exception(zend_ce_exception, "Cannot open source device", 0);
return FAILURE;
}

/* Does the file exist and is it a character device? */
if (fstat(fd, &st) != 0 || !S_ISCHR(st.st_mode)) {
close(fd);
zend_throw_exception(zend_ce_exception, "Error reading from source device", 0);
return FAILURE;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to have this check here? Performing a stat for every single random number generated seems very dubious to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TOCTOU.

Doing it every read might not be necessary, but it's conservative. If we can say with 100% certainty that there is no operating system on Earth that would let the file descriptor stick open while the device was unlinked and an non-character device was installed in its place, even with a ring0 exploit, then this is not necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get what you're trying to do here, I just don't think it's worth it. I'm okay with doing a sanity check when acquiring the FD, but I don't want to have a significant slowdown by repeating it every time. Knowing that it is a chardev doesn't tell you much anyway -- after all, /dev/zero is a chardev as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I remember reading a way to sanity check that a while back, but can't find the reference. I'll update the PR as soon as I find it again.

RANDOM_G(fd) = fd;
}

while (read_bytes < size) {
ssize_t n = read(fd, bytes + read_bytes, size - read_bytes);
n = read(fd, bytes + read_bytes, size - read_bytes);
if (n <= 0) {
break;
}
Expand Down
3 changes: 3 additions & 0 deletions ext/standard/tests/random/random_bytes.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ var_dump(strlen(bin2hex(random_bytes(16))));

var_dump(is_string(random_bytes(10)));

var_dump(is_string(random_bytes(257)));

?>
--EXPECT--
int(32)
bool(true)
bool(true)