From e78f3a7ea2bdc269580406b91b1f902994462067 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 31 May 2025 12:14:27 +0200 Subject: [PATCH 1/6] Fix reference type confusion and leak in user random engine --- ext/random/engine_user.c | 12 +++++++--- .../tests/02_engine/user_leak_on_error.phpt | 24 +++++++++++++++++++ .../02_engine/user_reference_return.phpt | 20 ++++++++++++++++ 3 files changed, 53 insertions(+), 3 deletions(-) create mode 100644 ext/random/tests/02_engine/user_leak_on_error.phpt create mode 100644 ext/random/tests/02_engine/user_reference_return.phpt diff --git a/ext/random/engine_user.c b/ext/random/engine_user.c index b45924d3bb7da..ce68521c129a1 100644 --- a/ext/random/engine_user.c +++ b/ext/random/engine_user.c @@ -27,6 +27,7 @@ static uint64_t generate(php_random_status *status) uint64_t result = 0; size_t size; zval retval; + zend_string *zstr; zend_call_known_instance_method_with_0_params(s->generate_method, s->object, &retval); @@ -34,8 +35,14 @@ static uint64_t generate(php_random_status *status) return 0; } + if (UNEXPECTED(Z_ISREF(retval))) { + zstr = Z_STR_P(Z_REFVAL(retval)); + } else { + zstr = Z_STR(retval); + } + /* Store generated size in a state */ - size = Z_STRLEN(retval); + size = ZSTR_LEN(zstr); /* Guard for over 64-bit results */ if (size > sizeof(uint64_t)) { @@ -46,11 +53,10 @@ static uint64_t generate(php_random_status *status) if (size > 0) { /* Endianness safe copy */ for (size_t i = 0; i < size; i++) { - result += ((uint64_t) (unsigned char) Z_STRVAL(retval)[i]) << (8 * i); + result += ((uint64_t) (unsigned char) ZSTR_VAL(zstr)[i]) << (8 * i); } } else { zend_throw_error(random_ce_Random_BrokenRandomEngineError, "A random engine must return a non-empty string"); - return 0; } zval_ptr_dtor(&retval); diff --git a/ext/random/tests/02_engine/user_leak_on_error.phpt b/ext/random/tests/02_engine/user_leak_on_error.phpt new file mode 100644 index 0000000000000..339c2269a3a0e --- /dev/null +++ b/ext/random/tests/02_engine/user_leak_on_error.phpt @@ -0,0 +1,24 @@ +--TEST-- +User engine leak on error +--FILE-- +field; + } +} + +$randomizer = new Random\Randomizer(new MyEngine); +try { + $randomizer->getBytes(64); +} catch (Random\BrokenRandomEngineError $e) { + echo $e->getMessage(), "\n"; +} + +?> +--EXPECT-- +A random engine must return a non-empty string diff --git a/ext/random/tests/02_engine/user_reference_return.phpt b/ext/random/tests/02_engine/user_reference_return.phpt new file mode 100644 index 0000000000000..882176a3a87e3 --- /dev/null +++ b/ext/random/tests/02_engine/user_reference_return.phpt @@ -0,0 +1,20 @@ +--TEST-- +User engine reference return value +--FILE-- +field; + } +} + +$randomizer = new Random\Randomizer(new MyEngine); +var_dump($randomizer->getBytes(64)); + +?> +--EXPECT-- +string(64) "abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdefabcd" From f11c9f928d85fe358a591ad0c4f273afffb29ff3 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 31 May 2025 13:51:18 +0200 Subject: [PATCH 2/6] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Tim Düsterhus --- ext/random/tests/02_engine/user_leak_on_error.phpt | 4 ++-- ext/random/tests/02_engine/user_reference_return.phpt | 9 +++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/ext/random/tests/02_engine/user_leak_on_error.phpt b/ext/random/tests/02_engine/user_leak_on_error.phpt index 339c2269a3a0e..190f8f705f82a 100644 --- a/ext/random/tests/02_engine/user_leak_on_error.phpt +++ b/ext/random/tests/02_engine/user_leak_on_error.phpt @@ -1,5 +1,5 @@ --TEST-- -User engine leak on error +Random: Engine: User: Empty strings do not leak --FILE-- getBytes(64); } catch (Random\BrokenRandomEngineError $e) { - echo $e->getMessage(), "\n"; + echo $e->getMessage(), PHP_EOL; } ?> diff --git a/ext/random/tests/02_engine/user_reference_return.phpt b/ext/random/tests/02_engine/user_reference_return.phpt index 882176a3a87e3..5b0a7cffb4e68 100644 --- a/ext/random/tests/02_engine/user_reference_return.phpt +++ b/ext/random/tests/02_engine/user_reference_return.phpt @@ -3,7 +3,11 @@ User engine reference return value --FILE-- getBytes(64)); ?> From 84088264fd86a445d55f02a52ae1583e91cbff3e Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 31 May 2025 13:53:02 +0200 Subject: [PATCH 3/6] More review --- ext/random/tests/02_engine/user_leak_on_error.phpt | 10 +++++++--- ext/random/tests/02_engine/user_reference_return.phpt | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/ext/random/tests/02_engine/user_leak_on_error.phpt b/ext/random/tests/02_engine/user_leak_on_error.phpt index 190f8f705f82a..117ef6c5f472b 100644 --- a/ext/random/tests/02_engine/user_leak_on_error.phpt +++ b/ext/random/tests/02_engine/user_leak_on_error.phpt @@ -3,7 +3,11 @@ Random: Engine: User: Empty strings do not leak --FILE-- getBytes(64); -} catch (Random\BrokenRandomEngineError $e) { +} catch (BrokenRandomEngineError $e) { echo $e->getMessage(), PHP_EOL; } diff --git a/ext/random/tests/02_engine/user_reference_return.phpt b/ext/random/tests/02_engine/user_reference_return.phpt index 5b0a7cffb4e68..f54e65bed7559 100644 --- a/ext/random/tests/02_engine/user_reference_return.phpt +++ b/ext/random/tests/02_engine/user_reference_return.phpt @@ -1,5 +1,5 @@ --TEST-- -User engine reference return value +Random: Engine: User: Returning by reference works --FILE-- Date: Sat, 31 May 2025 13:53:52 +0200 Subject: [PATCH 4/6] spaces --- ext/random/tests/02_engine/user_leak_on_error.phpt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ext/random/tests/02_engine/user_leak_on_error.phpt b/ext/random/tests/02_engine/user_leak_on_error.phpt index 117ef6c5f472b..2656ea7f8d3d6 100644 --- a/ext/random/tests/02_engine/user_leak_on_error.phpt +++ b/ext/random/tests/02_engine/user_leak_on_error.phpt @@ -10,10 +10,10 @@ use Random\BrokenRandomEngineError; class MyEngine implements Engine { private $field = ''; - public function &generate(): string - { - return $this->field; - } + public function &generate(): string + { + return $this->field; + } } $randomizer = new Randomizer(new MyEngine()); From ae07f738b096876f333d597057de9e88f014bdfb Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 31 May 2025 14:37:20 +0200 Subject: [PATCH 5/6] Review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Tim Düsterhus --- ext/random/tests/02_engine/user_leak_on_error.phpt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ext/random/tests/02_engine/user_leak_on_error.phpt b/ext/random/tests/02_engine/user_leak_on_error.phpt index 2656ea7f8d3d6..2b26a4a94c57b 100644 --- a/ext/random/tests/02_engine/user_leak_on_error.phpt +++ b/ext/random/tests/02_engine/user_leak_on_error.phpt @@ -7,12 +7,12 @@ use Random\Engine; use Random\Randomizer; use Random\BrokenRandomEngineError; -class MyEngine implements Engine { - private $field = ''; - - public function &generate(): string +final class MyEngine implements Engine +{ + public function generate(): string { - return $this->field; + // Create a non-interned empty string. + return preg_replace('/./', '', random_bytes(4)); } } From 359b384fb924a6a24d67f5e2540c3af9e5f5c219 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 31 May 2025 14:50:33 +0200 Subject: [PATCH 6/6] review --- .../tests/02_engine/user_leak_on_error.phpt | 28 ------------------- .../engine_unsafe_empty_string.phpt | 3 +- 2 files changed, 2 insertions(+), 29 deletions(-) delete mode 100644 ext/random/tests/02_engine/user_leak_on_error.phpt diff --git a/ext/random/tests/02_engine/user_leak_on_error.phpt b/ext/random/tests/02_engine/user_leak_on_error.phpt deleted file mode 100644 index 2b26a4a94c57b..0000000000000 --- a/ext/random/tests/02_engine/user_leak_on_error.phpt +++ /dev/null @@ -1,28 +0,0 @@ ---TEST-- -Random: Engine: User: Empty strings do not leak ---FILE-- -getBytes(64); -} catch (BrokenRandomEngineError $e) { - echo $e->getMessage(), PHP_EOL; -} - -?> ---EXPECT-- -A random engine must return a non-empty string diff --git a/ext/random/tests/03_randomizer/engine_unsafe_empty_string.phpt b/ext/random/tests/03_randomizer/engine_unsafe_empty_string.phpt index 6aec7180b1ee5..8333df88d6454 100644 --- a/ext/random/tests/03_randomizer/engine_unsafe_empty_string.phpt +++ b/ext/random/tests/03_randomizer/engine_unsafe_empty_string.phpt @@ -10,7 +10,8 @@ final class EmptyStringEngine implements Engine { public function generate(): string { - return ''; + // Create a non-interned empty string. + return preg_replace('/./', '', random_bytes(4)); } }