Skip to content

[RFC] Improve openssl_random_pseudo_bytes() #3649

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
SammyK wants to merge 1 commit into from
+23 −7
Closed

[RFC] Improve openssl_random_pseudo_bytes() #3649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 7 additions & 3 deletions ext/openssl/openssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "php.h"
#include "php_ini.h"
#include "php_openssl.h"
#include "zend_exceptions.h"

/* PHP Includes */
#include "ext/standard/file.h"
Expand Down Expand Up @@ -6861,7 +6862,8 @@ PHP_FUNCTION(openssl_random_pseudo_bytes)
|| ZEND_LONG_INT_OVFL(buffer_length)
#endif
) {
RETURN_FALSE;
zend_throw_exception(zend_ce_error, "Length must be greater than 0", 0);
Copy link
Member

@bukka bukka Nov 4, 2018

Choose a reason for hiding this comment

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

I think this can potentially break stuff. I'm fine with throwing on rand error which happens only if there is something really wrong with the environment but this case can happen more likely. Maybe we should reconsider this case and just return empty string or keep it as false.

Copy link
Contributor Author

@SammyK SammyK Nov 11, 2018

Choose a reason for hiding this comment

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

Thanks for the review @bukka! This is to mimic the behavior of random_bytes() like the RFC suggests. This will ensure that openssl_random_pseudo_bytes() can't be used as an attack vector in the event an attacker has access to change $length.

// Let's hope this doesn't exist in the wild! :)
$bytes = openssl_random_pseudo_bytes($_GET['length']);

Copy link
Member

@nikic nikic Nov 20, 2018

Choose a reason for hiding this comment

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

I'm also somewhat concerned about this one. Requesting zero bytes from an RNG is not wrong per-se -- it's an operation with a well-defined return value, namely the empty string. The current return value (false) happens to coerce to that.

I feel like there's not much value in throwing an exception for this one specific case (throwing for negative length or overflowing length is fine), and I strongly suspect that this will lead to BC breakage somewhere, particularly in testing code. E.g. I remember that when we merged in ext/sodium and switched from using a sodium-specific random bytes implementation (which allows 0 length) to PHP's that's one of the issues that came up in testing code that was testing random strings of random length (including zero).

That's why I would prefer to continue allowing this particular case. As the RFC does not spell this out either way, I think we could still do so, if we wanted :)

return;
}
buffer = zend_string_alloc(buffer_length, 0);

Expand All @@ -6872,7 +6874,8 @@ PHP_FUNCTION(openssl_random_pseudo_bytes)
if (zstrong_result_returned) {
ZVAL_FALSE(zstrong_result_returned);
}
RETURN_FALSE;
zend_throw_exception(zend_ce_exception, "Error reading from source device", 0);
return;
}
#else

Expand All @@ -6884,7 +6887,8 @@ PHP_FUNCTION(openssl_random_pseudo_bytes)
if (zstrong_result_returned) {
ZVAL_FALSE(zstrong_result_returned);
}
RETURN_FALSE;
zend_throw_exception(zend_ce_exception, "Error reading from source device", 0);
return;
} else {
php_openssl_store_errors();
}
Expand Down
6 changes: 2 additions & 4 deletions ext/openssl/tests/openssl_random_pseudo_bytes_basic.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,11 @@ openssl_random_pseudo_bytes() tests
<?php if (!extension_loaded("openssl")) print "skip"; ?>
--FILE--
<?php
for ($i = 0; $i < 10; $i++) {
var_dump(bin2hex(openssl_random_pseudo_bytes($i, $strong)));
for ($i = 1; $i < 10; $i++) {
var_dump(bin2hex(openssl_random_pseudo_bytes($i)));
}

?>
--EXPECTF--
string(0) ""
string(2) "%s"
string(4) "%s"
string(6) "%s"
Expand Down
14 changes: 14 additions & 0 deletions ext/openssl/tests/openssl_random_pseudo_bytes_error.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
--TEST--
Test error operation of openssl_random_pseudo_bytes()
--SKIPIF--
<?php if (!extension_loaded("openssl")) print "skip"; ?>
--FILE--
<?php
try {
openssl_random_pseudo_bytes(0);
} catch (Error $e) {
echo $e->getMessage().PHP_EOL;
}
?>
--EXPECTF--
Length must be greater than 0