Skip to content

Commit

Permalink
Improve error reporting in random extension (#9071)
Browse files Browse the repository at this point in the history 
* Use `php_random_bytes_throw()` in Secure engine's generate()

This exposes the underlying exception, improving debugging:

    Fatal error: Uncaught Exception: Cannot open source device in php-src/test.php:5
    Stack trace:
    #0 php-src/test.php(5): Random\Engine\Secure->generate()
    #1 {main}

    Next RuntimeException: Random number generation failed in php-src/test.php:5
    Stack trace:
    #0 php-src/test.php(5): Random\Engine\Secure->generate()
    #1 {main}
      thrown in php-src/test.php on line 5

* Use `php_random_int_throw()` in Secure engine's range()

This exposes the underlying exception, improving debugging:

    Exception: Cannot open source device in php-src/test.php:17
    Stack trace:
    #0 php-src/test.php(17): Random\Randomizer->getInt(1, 3)
    #1 {main}

    Next RuntimeException: Random number generation failed in php-src/test.php:17
    Stack trace:
    #0 php-src/test.php(17): Random\Randomizer->getInt(1, 3)
    #1 {main}

* Throw exception when a user engine returns an empty string

This improves debugging, because the actual reason for the failure is available
as a previous Exception:

    DomainException: The returned string must not be empty in php-src/test.php:17
    Stack trace:
    #0 php-src/test.php(17): Random\Randomizer->getBytes(123)
    #1 {main}

    Next RuntimeException: Random number generation failed in php-src/test.php:17
    Stack trace:
    #0 php-src/test.php(17): Random\Randomizer->getBytes(123)
    #1 {main}

* Throw exception when the range selector fails to get acceptable numbers in 50 attempts

This improves debugging, because the actual reason for the failure is available
as a previous Exception:

    RuntimeException: Failed to generate an acceptable random number in 50 attempts in php-src/test.php:17
    Stack trace:
    #0 php-src/test.php(17): Random\Randomizer->getInt(1, 3)
    #1 {main}

    Next RuntimeException: Random number generation failed in php-src/test.php:17
    Stack trace:
    #0 php-src/test.php(17): Random\Randomizer->getInt(1, 3)
    #1 {main}

* Improve user_unsafe test

Select parameters for ->getInt() that will actually lead to unsafe behavior.

* Fix user_unsafe test

If an engine fails once it will be permanently poisoned by setting
`->last_unsafe`. This is undesirable for the test, because it skews the
results.

Fix this by creating a fresh engine for each "assertion".

* Remove duplication in user_unsafe.phpt

* Catch `Throwable` in user_unsafe.phpt

As we print the full stringified exception we implicitly assert the type of the
exception. No need to be overly specific in the catch block.

* Throw an error if an engine returns an empty string

* Throw an Error if range fails to find an acceptable number in 50 attempts
  • Loading branch information
@TimWolla
TimWolla committed Jul 25, 2022
1 parent 34b352d commit 60f149f
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 68 deletions.
6 changes: 3 additions & 3 deletions ext/random/engine_secure.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ static uint64_t generate(php_random_status *status)
{
zend_ulong r = 0;

if (php_random_bytes_silent(&r, sizeof(zend_ulong)) == FAILURE) {
if (php_random_bytes_throw(&r, sizeof(zend_ulong)) == FAILURE) {
status->last_unsafe = true;
}

Expand All @@ -38,9 +38,9 @@ static uint64_t generate(php_random_status *status)

static zend_long range(php_random_status *status, zend_long min, zend_long max)
{
zend_long result;
zend_long result = 0;

if (php_random_int_silent(min, max, &result) == FAILURE) {
if (php_random_int_throw(min, max, &result) == FAILURE) {
status->last_unsafe = true;
}

Expand Down
1 change: 1 addition & 0 deletions ext/random/engine_user.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ static uint64_t generate(php_random_status *status)
result += ((uint64_t) (unsigned char) Z_STRVAL(retval)[i]) << (8 * i);
}
} else {
zend_throw_error(NULL, "A random engine must return a non-empty string");
status->last_unsafe = true;
return 0;
}
Expand Down
8 changes: 6 additions & 2 deletions ext/random/random.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ static zend_object_handlers random_engine_xoshiro256starstar_object_handlers;
static zend_object_handlers random_engine_secure_object_handlers;
static zend_object_handlers random_randomizer_object_handlers;

#define RANDOM_RANGE_ATTEMPTS (50)

static inline uint32_t rand_range32(const php_random_algo *algo, php_random_status *status, uint32_t umax)
{
uint32_t result, limit, r;
Expand Down Expand Up @@ -118,7 +120,8 @@ static inline uint32_t rand_range32(const php_random_algo *algo, php_random_stat
/* Discard numbers over the limit to avoid modulo bias */
while (UNEXPECTED(result > limit)) {
/* If the requirements cannot be met in a cycles, return fail */
if (++count > 50) {
if (++count > RANDOM_RANGE_ATTEMPTS) {
zend_throw_error(NULL, "Failed to generate an acceptable random number in %d attempts", RANDOM_RANGE_ATTEMPTS);
status->last_unsafe = true;
return 0;
}
Expand Down Expand Up @@ -174,7 +177,8 @@ static inline uint64_t rand_range64(const php_random_algo *algo, php_random_stat
/* Discard numbers over the limit to avoid modulo bias */
while (UNEXPECTED(result > limit)) {
/* If the requirements cannot be met in a cycles, return fail */
if (++count > 50) {
if (++count > RANDOM_RANGE_ATTEMPTS) {
zend_throw_error(NULL, "Failed to generate an acceptable random number in %d attempts", RANDOM_RANGE_ATTEMPTS);
status->last_unsafe = true;
return 0;
}
Expand Down
200 changes: 137 additions & 63 deletions ext/random/tests/03_randomizer/user_unsafe.phpt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,81 +3,155 @@ Random: Randomizer: User: Engine unsafe
--FILE--
<?php

// Empty generator
$randomizer = (new \Random\Randomizer(
new class () implements \Random\Engine {
public function generate(): string
{
return '';
}
}
));
use Random\Randomizer;

try {
var_dump($randomizer->getInt(\PHP_INT_MIN, \PHP_INT_MAX));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
final class EmptyStringEngine implements \Random\Engine {
public function generate(): string
{
return '';
}
}

try {
var_dump(bin2hex($randomizer->getBytes(1)));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
final class HeavilyBiasedEngine implements \Random\Engine {
public function generate(): string
{
return "\xff\xff\xff\xff\xff\xff\xff\xff";
}
}

try {
var_dump($randomizer->shuffleArray(\range(1, 10)));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
}
echo "=====================", PHP_EOL;

try {
var_dump($randomizer->shuffleBytes('foobar'));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
}
foreach ([
EmptyStringEngine::class,
HeavilyBiasedEngine::class,
] as $engine) {
echo $engine, PHP_EOL, "=====================", PHP_EOL, PHP_EOL;

// Infinite loop
$randomizer = (new \Random\Randomizer(
new class () implements \Random\Engine {
public function generate(): string
{
return "\xff\xff\xff\xff\xff\xff\xff\xff";
}
try {
var_dump((new Randomizer(new $engine()))->getInt(0, 123));
} catch (Throwable $e) {
echo $e, PHP_EOL;
}

echo PHP_EOL, "-------", PHP_EOL, PHP_EOL;

try {
var_dump(bin2hex((new Randomizer(new $engine()))->getBytes(1)));
} catch (Throwable $e) {
echo $e, PHP_EOL;
}

echo PHP_EOL, "-------", PHP_EOL, PHP_EOL;

try {
var_dump((new Randomizer(new $engine()))->shuffleArray(\range(1, 10)));
} catch (Throwable $e) {
echo $e, PHP_EOL;
}

echo PHP_EOL, "-------", PHP_EOL, PHP_EOL;

try {
var_dump((new Randomizer(new $engine()))->shuffleBytes('foobar'));
} catch (Throwable $e) {
echo $e, PHP_EOL;
}
));

try {
var_dump($randomizer->getInt(\PHP_INT_MIN, \PHP_INT_MAX));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
echo PHP_EOL, "=====================", PHP_EOL;
}

try {
var_dump(bin2hex($randomizer->getBytes(1)));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
}
?>
--EXPECTF--
=====================
EmptyStringEngine
=====================

try {
var_dump($randomizer->shuffleArray(\range(1, 10)));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
}
Error: A random engine must return a non-empty string in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->getInt(0, 123)
#1 {main}

try {
var_dump($randomizer->shuffleBytes('foobar'));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
}
Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->getInt(0, 123)
#1 {main}

-------

Error: A random engine must return a non-empty string in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->getBytes(1)
#1 {main}

Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->getBytes(1)
#1 {main}

-------

Error: A random engine must return a non-empty string in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
#1 {main}

Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
#1 {main}

-------

Error: A random engine must return a non-empty string in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleBytes('foobar')
#1 {main}

Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleBytes('foobar')
#1 {main}

=====================
HeavilyBiasedEngine
=====================

Error: Failed to generate an acceptable random number in 50 attempts in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->getInt(0, 123)
#1 {main}

Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->getInt(0, 123)
#1 {main}

-------

?>
--EXPECTF--
Random number generation failed
Random number generation failed
Random number generation failed
Random number generation failed
int(%d)
string(2) "ff"
Random number generation failed
Random number generation failed

-------

Error: Failed to generate an acceptable random number in 50 attempts in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
#1 {main}

Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
#1 {main}

-------

Error: Failed to generate an acceptable random number in 50 attempts in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleBytes('foobar')
#1 {main}

Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleBytes('foobar')
#1 {main}

=====================

0 comments on commit 60f149f

Please sign in to comment.