From 60f149f7ada7bbead8bf73dba0d317e77c14e0e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Mon, 25 Jul 2022 09:00:49 +0200 Subject: [PATCH] Improve error reporting in random extension (#9071) * 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 --- ext/random/engine_secure.c | 6 +- ext/random/engine_user.c | 1 + ext/random/random.c | 8 +- .../tests/03_randomizer/user_unsafe.phpt | 200 ++++++++++++------ 4 files changed, 147 insertions(+), 68 deletions(-) diff --git a/ext/random/engine_secure.c b/ext/random/engine_secure.c index 7dda4e9dec1d6..e2da4f4b9bf87 100644 --- a/ext/random/engine_secure.c +++ b/ext/random/engine_secure.c @@ -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; } @@ -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; } diff --git a/ext/random/engine_user.c b/ext/random/engine_user.c index 9e944452e8a58..4f3e0890e3be5 100644 --- a/ext/random/engine_user.c +++ b/ext/random/engine_user.c @@ -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; } diff --git a/ext/random/random.c b/ext/random/random.c index abc2dd2fbbdc5..025b4431ecf10 100644 --- a/ext/random/random.c +++ b/ext/random/random.c @@ -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; @@ -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; } @@ -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; } diff --git a/ext/random/tests/03_randomizer/user_unsafe.phpt b/ext/random/tests/03_randomizer/user_unsafe.phpt index 659159671f29a..4497088ba9da1 100644 --- a/ext/random/tests/03_randomizer/user_unsafe.phpt +++ b/ext/random/tests/03_randomizer/user_unsafe.phpt @@ -3,81 +3,155 @@ Random: Randomizer: User: Engine unsafe --FILE-- 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} + +=====================