Improve openssl_random_pseudo_bytes()

SammyK · SammyK · commit c53ea278a0c6 · 2018-10-20T08:56:31.000-04:00
CSPRNG implementations should always fail closed. Now
openssl_random_pseudo_bytes() will fail closed by throwing an
`\Exception` if it is unable to return crypto-strong bytes.

The second `$crypto_strong` parameter doesn't do anything despite the
docs stating otherwise. This unnecessarily confusing parameter is now
deprecated and will be removed in PHP 8.0.

In addition to removing the second parameter in 8.0, the ZPP macros will
be updated to `ZEND_PARSE_PARAMS_THROW` so passing a second argument
will cause a fatal error.
diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c
@@ -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"
@@ -6801,32 +6802,34 @@ PHP_FUNCTION(openssl_random_pseudo_bytes)
 	zend_string *buffer = NULL;
 	zval *zstrong_result_returned = NULL;
 
-	if (zend_parse_parameters(ZEND_NUM_ARGS(), "l|z/", &buffer_length, &zstrong_result_returned) == FAILURE) {
-		return;
-	}
+	ZEND_PARSE_PARAMETERS_START(1, 2)
+		Z_PARAM_LONG(buffer_length)
+		Z_PARAM_OPTIONAL
+		Z_PARAM_ZVAL_DEREF(zstrong_result_returned)
+	ZEND_PARSE_PARAMETERS_END();
 
 	if (zstrong_result_returned) {
 		zval_ptr_dtor(zstrong_result_returned);
-		ZVAL_FALSE(zstrong_result_returned);
+		ZVAL_TRUE(zstrong_result_returned);
+		php_error_docref(NULL, E_DEPRECATED, "Calling openssl_random_pseudo_bytes() with the second argument is deprecated");
 	}
 
 	if (buffer_length <= 0
 #ifndef PHP_WIN32
 		|| ZEND_LONG_INT_OVFL(buffer_length)
 #endif
 			) {
-		RETURN_FALSE;
+		zend_throw_exception(zend_ce_error, "Length must be greater than 0", 0);
+		return;
 	}
 	buffer = zend_string_alloc(buffer_length, 0);
 
 #ifdef PHP_WIN32
 	/* random/urandom equivalent on Windows */
 	if (php_win32_get_random_bytes((unsigned char*)buffer->val, (size_t) buffer_length) == FAILURE){
 		zend_string_release_ex(buffer, 0);
-		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
 
@@ -6835,21 +6838,15 @@ PHP_FUNCTION(openssl_random_pseudo_bytes)
 	/* FIXME loop if requested size > INT_MAX */
 	if (RAND_bytes((unsigned char*)ZSTR_VAL(buffer), (int)buffer_length) <= 0) {
 		zend_string_release_ex(buffer, 0);
-		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();
 	}
 #endif
 
 	ZSTR_VAL(buffer)[buffer_length] = 0;
 	RETVAL_NEW_STR(buffer);
-
-	if (zstrong_result_returned) {
-		ZVAL_BOOL(zstrong_result_returned, 1);
-	}
 }
 /* }}} */
 
diff --git a/ext/openssl/tests/openssl_random_pseudo_bytes_basic.phpt b/ext/openssl/tests/openssl_random_pseudo_bytes_basic.phpt
@@ -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"
diff --git a/ext/openssl/tests/openssl_random_pseudo_bytes_error.phpt b/ext/openssl/tests/openssl_random_pseudo_bytes_error.phpt
@@ -0,0 +1,18 @@
+--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;
+}
+
+openssl_random_pseudo_bytes(1, $strong);
+?>
+--EXPECTF--
+Length must be greater than 0
+
+Deprecated: openssl_random_pseudo_bytes(): Calling openssl_random_pseudo_bytes() with the second argument is deprecated in %s on line %d
