From d169bc2be77d5e30e7948625d3f00942961d7ba7 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Wed, 29 May 2024 19:06:18 +0200 Subject: [PATCH 1/4] Improve randomness of uploaded file names and files created by tempnam() --- .../tests/file/tempnam_variation9.phpt | 20 +++++------ main/php_open_temporary_file.c | 35 ++++++++++++++----- 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/ext/standard/tests/file/tempnam_variation9.phpt b/ext/standard/tests/file/tempnam_variation9.phpt index 22f3ea109bf2e..1dd578d01d92b 100644 --- a/ext/standard/tests/file/tempnam_variation9.phpt +++ b/ext/standard/tests/file/tempnam_variation9.phpt @@ -57,17 +57,17 @@ if (file_exists($file_path)) { --EXPECTF-- *** Testing tempnam() maximum prefix size *** -- Iteration 0 -- -File name is => begin_%rx{7}%r_end%r.{6}%r -File name length is => 23 +File name is => begin_%rx{7}%r_end%r.{38}%r +File name length is => 55 -- Iteration 1 -- -File name is => begin_%rx{53}%r_end%r.{6}%r -File name length is => 69 +File name is => begin_%rx{53}%r_end%r.{38}%r +File name length is => 101 -- Iteration 2 -- -File name is => begin_%rx{54}%r_en%r.{6}%r -File name length is => 69 +File name is => begin_%rx{54}%r_en%r.{38}%r +File name length is => 101 -- Iteration 3 -- -File name is => begin_%rx{55}%r_e%r.{6}%r -File name length is => 69 +File name is => begin_%rx{55}%r_e%r.{38}%r +File name length is => 101 -- Iteration 4 -- -File name is => begin_%rx{57}%r%r.{6}%r -File name length is => 69 +File name is => begin_%rx{57}%r%r.{38}%r +File name length is => 101 diff --git a/main/php_open_temporary_file.c b/main/php_open_temporary_file.c index dcea78358486d..1a52126d06b4d 100644 --- a/main/php_open_temporary_file.c +++ b/main/php_open_temporary_file.c @@ -15,7 +15,9 @@ */ #include "php.h" +#include "zend_long.h" #include "php_open_temporary_file.h" +#include "ext/random/php_random.h" #include #include @@ -89,11 +91,13 @@ static int php_do_open_temporary_file(const char *path, const char *pfx, zend_st #ifdef PHP_WIN32 char *opened_path = NULL; size_t opened_path_len; - wchar_t *cwdw, *pfxw, pathw[MAXPATHLEN]; + wchar_t *cwdw, *random_prefix_w, pathw[MAXPATHLEN]; #else char opened_path[MAXPATHLEN]; char *trailing_slash; #endif + uint64_t random[2]; + char *random_prefix; char cwd[MAXPATHLEN]; cwd_state new_state; int fd = -1; @@ -128,6 +132,14 @@ static int php_do_open_temporary_file(const char *path, const char *pfx, zend_st return -1; } + /* Extend the prefix to increase randomness */ + if (php_random_bytes_silent(&random, sizeof(random)) == FAILURE) { + random[0] = php_random_generate_fallback_seed(); + random[1] = php_random_generate_fallback_seed(); + } + + spprintf(&random_prefix, 0, "%s%016" PRIx64 "%016" PRIx64, pfx, random[0], random[1]); + #ifndef PHP_WIN32 if (IS_SLASH(new_state.cwd[new_state.cwd_length - 1])) { trailing_slash = ""; @@ -135,7 +147,8 @@ static int php_do_open_temporary_file(const char *path, const char *pfx, zend_st trailing_slash = "/"; } - if (snprintf(opened_path, MAXPATHLEN, "%s%s%sXXXXXX", new_state.cwd, trailing_slash, pfx) >= MAXPATHLEN) { + if (snprintf(opened_path, MAXPATHLEN, "%s%s%sXXXXXX", new_state.cwd, trailing_slash, random_prefix) >= MAXPATHLEN) { + efree(random_prefix); efree(new_state.cwd); return -1; } @@ -143,19 +156,21 @@ static int php_do_open_temporary_file(const char *path, const char *pfx, zend_st #ifdef PHP_WIN32 cwdw = php_win32_ioutil_any_to_w(new_state.cwd); - pfxw = php_win32_ioutil_any_to_w(pfx); - if (!cwdw || !pfxw) { + random_prefix_w = php_win32_ioutil_any_to_w(random_prefix); + if (!cwdw || !random_prefix_w) { free(cwdw); - free(pfxw); + free(random_prefix_w); + efree(random_prefix); efree(new_state.cwd); return -1; } - if (GetTempFileNameW(cwdw, pfxw, 0, pathw)) { + if (GetTempFileNameW(cwdw, random_prefix_w, 0, pathw)) { opened_path = php_win32_ioutil_conv_w_to_any(pathw, PHP_WIN32_CP_IGNORE_LEN, &opened_path_len); if (!opened_path || opened_path_len >= MAXPATHLEN) { free(cwdw); - free(pfxw); + free(random_prefix_w); + efree(random_prefix); efree(new_state.cwd); return -1; } @@ -165,7 +180,8 @@ static int php_do_open_temporary_file(const char *path, const char *pfx, zend_st * which means that opening it will fail... */ if (VCWD_CHMOD(opened_path, 0600)) { free(cwdw); - free(pfxw); + free(random_prefix_w); + efree(random_prefix); efree(new_state.cwd); free(opened_path); return -1; @@ -174,7 +190,7 @@ static int php_do_open_temporary_file(const char *path, const char *pfx, zend_st } free(cwdw); - free(pfxw); + free(random_prefix_w); #elif defined(HAVE_MKSTEMP) fd = mkstemp(opened_path); #else @@ -194,6 +210,7 @@ static int php_do_open_temporary_file(const char *path, const char *pfx, zend_st } #endif efree(new_state.cwd); + efree(random_prefix); return fd; } /* }}} */ From 3303a2c5617f54aecbb441d2129a801b6f4f78f8 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Thu, 30 May 2024 14:27:03 +0200 Subject: [PATCH 2/4] Revert to 64bits of randomness for compatibility --- .../tests/file/tempnam_variation9.phpt | 20 +++++++++---------- main/php_open_temporary_file.c | 19 ++++++++++++++---- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/ext/standard/tests/file/tempnam_variation9.phpt b/ext/standard/tests/file/tempnam_variation9.phpt index 1dd578d01d92b..98d3d332a1f1f 100644 --- a/ext/standard/tests/file/tempnam_variation9.phpt +++ b/ext/standard/tests/file/tempnam_variation9.phpt @@ -57,17 +57,17 @@ if (file_exists($file_path)) { --EXPECTF-- *** Testing tempnam() maximum prefix size *** -- Iteration 0 -- -File name is => begin_%rx{7}%r_end%r.{38}%r -File name length is => 55 +File name is => begin_%rx{7}%r_end%r.{16,17}%r +File name length is => %r33|34%r -- Iteration 1 -- -File name is => begin_%rx{53}%r_end%r.{38}%r -File name length is => 101 +File name is => begin_%rx{53}%r_end%r.{16,17}%r +File name length is => %r79|80%r -- Iteration 2 -- -File name is => begin_%rx{54}%r_en%r.{38}%r -File name length is => 101 +File name is => begin_%rx{54}%r_en%r.{16,17}%r +File name length is => %r79|80%r -- Iteration 3 -- -File name is => begin_%rx{55}%r_e%r.{38}%r -File name length is => 101 +File name is => begin_%rx{55}%r_e%r.{16,17}%r +File name length is => %r79|80%r -- Iteration 4 -- -File name is => begin_%rx{57}%r%r.{38}%r -File name length is => 101 +File name is => begin_%rx{57}%r%r.{16,17}%r +File name length is => %r79|80%r diff --git a/main/php_open_temporary_file.c b/main/php_open_temporary_file.c index 1a52126d06b4d..880f0e0da00c9 100644 --- a/main/php_open_temporary_file.c +++ b/main/php_open_temporary_file.c @@ -18,6 +18,7 @@ #include "zend_long.h" #include "php_open_temporary_file.h" #include "ext/random/php_random.h" +#include "zend_operators.h" #include #include @@ -86,6 +87,8 @@ * SUCH DAMAGE. */ +static const char letters[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; + static int php_do_open_temporary_file(const char *path, const char *pfx, zend_string **opened_path_p) { #ifdef PHP_WIN32 @@ -96,8 +99,9 @@ static int php_do_open_temporary_file(const char *path, const char *pfx, zend_st char opened_path[MAXPATHLEN]; char *trailing_slash; #endif - uint64_t random[2]; + uint64_t random; char *random_prefix; + char *p; char cwd[MAXPATHLEN]; cwd_state new_state; int fd = -1; @@ -134,11 +138,18 @@ static int php_do_open_temporary_file(const char *path, const char *pfx, zend_st /* Extend the prefix to increase randomness */ if (php_random_bytes_silent(&random, sizeof(random)) == FAILURE) { - random[0] = php_random_generate_fallback_seed(); - random[1] = php_random_generate_fallback_seed(); + random = php_random_generate_fallback_seed(); } - spprintf(&random_prefix, 0, "%s%016" PRIx64 "%016" PRIx64, pfx, random[0], random[1]); + random_prefix = emalloc(strlen(pfx) + sizeof(random) * 2); + p = zend_mempcpy(random_prefix, pfx, strlen(pfx)); + /* Use a compact encoding to not increase the path len too much */ + while (random) { + *p = letters[random % strlen(letters)]; + p++; + random /= strlen(letters); + } + *p = '\0'; #ifndef PHP_WIN32 if (IS_SLASH(new_state.cwd[new_state.cwd_length - 1])) { From 53fa48343dfe6b642d55eb5a51939a9cd67fac3d Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Thu, 30 May 2024 15:44:08 +0200 Subject: [PATCH 3/4] Case-insensitive encoding --- .../tests/file/tempnam_variation9.phpt | 20 +++++++++---------- main/php_open_temporary_file.c | 11 ++++++---- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/ext/standard/tests/file/tempnam_variation9.phpt b/ext/standard/tests/file/tempnam_variation9.phpt index 98d3d332a1f1f..61178a022ce3a 100644 --- a/ext/standard/tests/file/tempnam_variation9.phpt +++ b/ext/standard/tests/file/tempnam_variation9.phpt @@ -57,17 +57,17 @@ if (file_exists($file_path)) { --EXPECTF-- *** Testing tempnam() maximum prefix size *** -- Iteration 0 -- -File name is => begin_%rx{7}%r_end%r.{16,17}%r -File name length is => %r33|34%r +File name is => begin_%rx{7}%r_end%r.{19}%r +File name length is => 36 -- Iteration 1 -- -File name is => begin_%rx{53}%r_end%r.{16,17}%r -File name length is => %r79|80%r +File name is => begin_%rx{53}%r_end%r.{19}%r +File name length is => 82 -- Iteration 2 -- -File name is => begin_%rx{54}%r_en%r.{16,17}%r -File name length is => %r79|80%r +File name is => begin_%rx{54}%r_en%r.{19}%r +File name length is => 82 -- Iteration 3 -- -File name is => begin_%rx{55}%r_e%r.{16,17}%r -File name length is => %r79|80%r +File name is => begin_%rx{55}%r_e%r.{19}%r +File name length is => 82 -- Iteration 4 -- -File name is => begin_%rx{57}%r%r.{16,17}%r -File name length is => %r79|80%r +File name is => begin_%rx{57}%r%r.{19}%r +File name length is => 82 diff --git a/main/php_open_temporary_file.c b/main/php_open_temporary_file.c index 880f0e0da00c9..53098db22a716 100644 --- a/main/php_open_temporary_file.c +++ b/main/php_open_temporary_file.c @@ -87,7 +87,7 @@ * SUCH DAMAGE. */ -static const char letters[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; +static const char letters[] = "0123456789abcdefghijklmnopqrstuvwxyz"; static int php_do_open_temporary_file(const char *path, const char *pfx, zend_string **opened_path_p) { @@ -102,6 +102,7 @@ static int php_do_open_temporary_file(const char *path, const char *pfx, zend_st uint64_t random; char *random_prefix; char *p; + size_t len; char cwd[MAXPATHLEN]; cwd_state new_state; int fd = -1; @@ -141,10 +142,12 @@ static int php_do_open_temporary_file(const char *path, const char *pfx, zend_st random = php_random_generate_fallback_seed(); } - random_prefix = emalloc(strlen(pfx) + sizeof(random) * 2); + /* Use a compact encoding to not increase the path len too much, but do not + * mix case to avoid losing randomness on case-insensitive file systems */ + len = strlen(pfx) + 13 /* log(2**64)/log(strlen(letters)) */ + 1; + random_prefix = emalloc(len); p = zend_mempcpy(random_prefix, pfx, strlen(pfx)); - /* Use a compact encoding to not increase the path len too much */ - while (random) { + while (p + 1 < random_prefix + len) { *p = letters[random % strlen(letters)]; p++; random /= strlen(letters); From 44d33c04221145b300b1017cad2e3ab8d094c7e0 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Thu, 30 May 2024 17:19:58 +0200 Subject: [PATCH 4/4] Use base32 so mod and div get optimized to mask and shift --- main/php_open_temporary_file.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/main/php_open_temporary_file.c b/main/php_open_temporary_file.c index 53098db22a716..4f6785b0ebbf8 100644 --- a/main/php_open_temporary_file.c +++ b/main/php_open_temporary_file.c @@ -87,7 +87,7 @@ * SUCH DAMAGE. */ -static const char letters[] = "0123456789abcdefghijklmnopqrstuvwxyz"; +static const char base32alphabet[] = "0123456789abcdefghijklmnopqrstuv"; static int php_do_open_temporary_file(const char *path, const char *pfx, zend_string **opened_path_p) { @@ -148,9 +148,9 @@ static int php_do_open_temporary_file(const char *path, const char *pfx, zend_st random_prefix = emalloc(len); p = zend_mempcpy(random_prefix, pfx, strlen(pfx)); while (p + 1 < random_prefix + len) { - *p = letters[random % strlen(letters)]; + *p = base32alphabet[random % strlen(base32alphabet)]; p++; - random /= strlen(letters); + random /= strlen(base32alphabet); } *p = '\0';