Skip to content

Commit ded8fb7

Browse files
committed
Fix UAF issues with PCRE after request shutdown
There are two related issues, each tested. First problem: What happens is that on the CLI SAPI we have a per-request pcre cache, and on there the request shutdown for the pcre module happens prior to the remaining live object destruction. So when the SPL object wants to clean up the regular expression object it gets a use-after-free. Second problem: Very similarly, the non-persistent resources are destroyed after request shutdown, so on the CLI SAPI the pcre request cache is already gone, but if a userspace stream references a regex in the pcre cache, this breaks. Two things that come immediately to mind: - We could fix it by no longer treating the CLI SAPI special and just use the same lifecycle as the module. This simplifies the pcre module code a bit too. I wonder why we even have the separation in the first place. The downside here is that we're using more the system allocator than Zend's allocator for cache entries. - We could modify the shutdown code to not remove regular expressions with a refcount>0 and modify php_pcre_pce_decref code such that it becomes php_pcre_pce_decref's job to clean up when the refcount becomes 0 during shutdown. However, this gets nasty quickly. I chose the first solution here as it should be reliable and simple. Closes GH-15064.
1 parent 9698ad2 commit ded8fb7

File tree

7 files changed

+89
-59
lines changed

7 files changed

+89
-59
lines changed

NEWS

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ PHP NEWS
99
- Opcache:
1010
. Fixed bug GH-15657 (Segmentation fault in dasm_x86.h). (nielsdos)
1111

12+
- PCRE:
13+
. Fix UAF issues with PCRE after request shutdown. (nielsdos)
14+
1215
- SOAP:
1316
. Fixed bug #73182 (PHP SOAPClient does not support stream context HTTP
1417
headers in array form). (nielsdos)

UPGRADING.INTERNALS

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,9 @@ PHP 8.4 INTERNALS UPGRADE NOTES
358358
- pcre_get_compiled_regex_cache_ex() now provides an option to collect extra
359359
options (from modifiers used in the expression, for example), and calls
360360
pcre2_set_compile_extra_options() with those options.
361+
- Removed per-request cache, the cache is now always per process or
362+
per thread depending on whether you use NTS or ZTS.
363+
This was removed due to fundamental ordering issues between destructors.
361364

362365
g. ext/standard
363366
- Added the php_base64_encode_ex() API with flag parameters, value can be

ext/opcache/ZendAccelerator.c

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2611,10 +2611,6 @@ static void accel_reset_pcre_cache(void)
26112611
{
26122612
Bucket *p;
26132613

2614-
if (PCRE_G(per_request_cache)) {
2615-
return;
2616-
}
2617-
26182614
ZEND_HASH_MAP_FOREACH_BUCKET(&PCRE_G(pcre_cache), p) {
26192615
/* Remove PCRE cache entries with inconsistent keys */
26202616
if (zend_accel_in_shm(p->key)) {

ext/pcre/php_pcre.c

Lines changed: 39 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ static MUTEX_T pcre_mt = NULL;
9292

9393
ZEND_TLS HashTable char_tables;
9494

95-
static void free_subpats_table(zend_string **subpat_names, uint32_t num_subpats, bool persistent);
95+
static void free_subpats_table(zend_string **subpat_names, uint32_t num_subpats);
9696

9797
static void php_pcre_free_char_table(zval *data)
9898
{/*{{{*/
@@ -168,25 +168,13 @@ static void php_free_pcre_cache(zval *data) /* {{{ */
168168
pcre_cache_entry *pce = (pcre_cache_entry *) Z_PTR_P(data);
169169
if (!pce) return;
170170
if (pce->subpats_table) {
171-
free_subpats_table(pce->subpats_table, pce->capture_count + 1, true);
171+
free_subpats_table(pce->subpats_table, pce->capture_count + 1);
172172
}
173173
pcre2_code_free(pce->re);
174174
free(pce);
175175
}
176176
/* }}} */
177177

178-
static void php_efree_pcre_cache(zval *data) /* {{{ */
179-
{
180-
pcre_cache_entry *pce = (pcre_cache_entry *) Z_PTR_P(data);
181-
if (!pce) return;
182-
if (pce->subpats_table) {
183-
free_subpats_table(pce->subpats_table, pce->capture_count + 1, false);
184-
}
185-
pcre2_code_free(pce->re);
186-
efree(pce);
187-
}
188-
/* }}} */
189-
190178
static void *php_pcre_malloc(PCRE2_SIZE size, void *data)
191179
{
192180
return pemalloc(size, 1);
@@ -303,12 +291,7 @@ static PHP_GINIT_FUNCTION(pcre) /* {{{ */
303291
{
304292
php_pcre_mutex_alloc();
305293

306-
/* If we're on the CLI SAPI, there will only be one request, so we don't need the
307-
* cache to survive after RSHUTDOWN. */
308-
pcre_globals->per_request_cache = strcmp(sapi_module.name, "cli") == 0;
309-
if (!pcre_globals->per_request_cache) {
310-
zend_hash_init(&pcre_globals->pcre_cache, 0, NULL, php_free_pcre_cache, 1);
311-
}
294+
zend_hash_init(&pcre_globals->pcre_cache, 0, NULL, php_free_pcre_cache, 1);
312295

313296
pcre_globals->backtrack_limit = 0;
314297
pcre_globals->recursion_limit = 0;
@@ -326,9 +309,7 @@ static PHP_GINIT_FUNCTION(pcre) /* {{{ */
326309

327310
static PHP_GSHUTDOWN_FUNCTION(pcre) /* {{{ */
328311
{
329-
if (!pcre_globals->per_request_cache) {
330-
zend_hash_destroy(&pcre_globals->pcre_cache);
331-
}
312+
zend_hash_destroy(&pcre_globals->pcre_cache);
332313

333314
php_pcre_shutdown_pcre2();
334315
zend_hash_destroy(&char_tables);
@@ -491,10 +472,6 @@ static PHP_RINIT_FUNCTION(pcre)
491472
return FAILURE;
492473
}
493474

494-
if (PCRE_G(per_request_cache)) {
495-
zend_hash_init(&PCRE_G(pcre_cache), 0, NULL, php_efree_pcre_cache, 0);
496-
}
497-
498475
return SUCCESS;
499476
}
500477
/* }}} */
@@ -504,10 +481,6 @@ static PHP_RSHUTDOWN_FUNCTION(pcre)
504481
pcre2_general_context_free(PCRE_G(gctx_zmm));
505482
PCRE_G(gctx_zmm) = NULL;
506483

507-
if (PCRE_G(per_request_cache)) {
508-
zend_hash_destroy(&PCRE_G(pcre_cache));
509-
}
510-
511484
zval_ptr_dtor(&PCRE_G(unmatched_null_pair));
512485
zval_ptr_dtor(&PCRE_G(unmatched_empty_pair));
513486
ZVAL_UNDEF(&PCRE_G(unmatched_null_pair));
@@ -530,18 +503,18 @@ static int pcre_clean_cache(zval *data, void *arg)
530503
}
531504
/* }}} */
532505

533-
static void free_subpats_table(zend_string **subpat_names, uint32_t num_subpats, bool persistent) {
506+
static void free_subpats_table(zend_string **subpat_names, uint32_t num_subpats) {
534507
uint32_t i;
535508
for (i = 0; i < num_subpats; i++) {
536509
if (subpat_names[i]) {
537-
zend_string_release_ex(subpat_names[i], persistent);
510+
zend_string_release_ex(subpat_names[i], true);
538511
}
539512
}
540-
pefree(subpat_names, persistent);
513+
pefree(subpat_names, true);
541514
}
542515

543516
/* {{{ static make_subpats_table */
544-
static zend_string **make_subpats_table(uint32_t name_cnt, pcre_cache_entry *pce, bool persistent)
517+
static zend_string **make_subpats_table(uint32_t name_cnt, pcre_cache_entry *pce)
545518
{
546519
uint32_t num_subpats = pce->capture_count + 1;
547520
uint32_t name_size, ni = 0;
@@ -556,7 +529,7 @@ static zend_string **make_subpats_table(uint32_t name_cnt, pcre_cache_entry *pce
556529
return NULL;
557530
}
558531

559-
subpat_names = pecalloc(num_subpats, sizeof(zend_string *), persistent);
532+
subpat_names = pecalloc(num_subpats, sizeof(zend_string *), true);
560533
while (ni++ < name_cnt) {
561534
unsigned short name_idx = 0x100 * (unsigned char)name_table[0] + (unsigned char)name_table[1];
562535
const char *name = name_table + 2;
@@ -566,10 +539,8 @@ static zend_string **make_subpats_table(uint32_t name_cnt, pcre_cache_entry *pce
566539
* Although we will be storing them in user-exposed arrays, they cannot cause problems
567540
* because they only live in this thread and the last reference is deleted on shutdown
568541
* instead of by user code. */
569-
subpat_names[name_idx] = zend_string_init(name, strlen(name), persistent);
570-
if (persistent) {
571-
GC_MAKE_PERSISTENT_LOCAL(subpat_names[name_idx]);
572-
}
542+
subpat_names[name_idx] = zend_string_init(name, strlen(name), true);
543+
GC_MAKE_PERSISTENT_LOCAL(subpat_names[name_idx]);
573544
name_table += name_size;
574545
}
575546
return subpat_names;
@@ -871,7 +842,7 @@ PHPAPI pcre_cache_entry* pcre_get_compiled_regex_cache_ex(zend_string *regex, bo
871842

872843
/* Compute and cache the subpattern table to avoid computing it again over and over. */
873844
if (name_count > 0) {
874-
new_entry.subpats_table = make_subpats_table(name_count, &new_entry, !PCRE_G(per_request_cache));
845+
new_entry.subpats_table = make_subpats_table(name_count, &new_entry);
875846
if (!new_entry.subpats_table) {
876847
if (key != regex) {
877848
zend_string_release_ex(key, false);
@@ -892,7 +863,7 @@ PHPAPI pcre_cache_entry* pcre_get_compiled_regex_cache_ex(zend_string *regex, bo
892863
* as hash keys especually for this table.
893864
* See bug #63180
894865
*/
895-
if (!(GC_FLAGS(key) & IS_STR_PERMANENT) && !PCRE_G(per_request_cache)) {
866+
if (!(GC_FLAGS(key) & IS_STR_PERMANENT)) {
896867
zend_string *str = zend_string_init(ZSTR_VAL(key), ZSTR_LEN(key), 1);
897868
GC_MAKE_PERSISTENT_LOCAL(str);
898869

@@ -963,18 +934,18 @@ PHPAPI void php_pcre_free_match_data(pcre2_match_data *match_data)
963934
}
964935
}/*}}}*/
965936

966-
static void init_unmatched_null_pair(void) {
937+
static void init_unmatched_null_pair(zval *pair) {
967938
zval val1, val2;
968939
ZVAL_NULL(&val1);
969940
ZVAL_LONG(&val2, -1);
970-
ZVAL_ARR(&PCRE_G(unmatched_null_pair), zend_new_pair(&val1, &val2));
941+
ZVAL_ARR(pair, zend_new_pair(&val1, &val2));
971942
}
972943

973-
static void init_unmatched_empty_pair(void) {
944+
static void init_unmatched_empty_pair(zval *pair) {
974945
zval val1, val2;
975946
ZVAL_EMPTY_STRING(&val1);
976947
ZVAL_LONG(&val2, -1);
977-
ZVAL_ARR(&PCRE_G(unmatched_empty_pair), zend_new_pair(&val1, &val2));
948+
ZVAL_ARR(pair, zend_new_pair(&val1, &val2));
978949
}
979950

980951
static zend_always_inline void populate_match_value_str(
@@ -1020,15 +991,29 @@ static inline void add_offset_pair(
1020991
/* Add (match, offset) to the return value */
1021992
if (PCRE2_UNSET == start_offset) {
1022993
if (unmatched_as_null) {
1023-
if (Z_ISUNDEF(PCRE_G(unmatched_null_pair))) {
1024-
init_unmatched_null_pair();
1025-
}
1026-
ZVAL_COPY(&match_pair, &PCRE_G(unmatched_null_pair));
994+
do {
995+
if (Z_ISUNDEF(PCRE_G(unmatched_null_pair))) {
996+
if (UNEXPECTED(EG(flags) & EG_FLAGS_IN_SHUTDOWN)) {
997+
init_unmatched_null_pair(&match_pair);
998+
break;
999+
} else {
1000+
init_unmatched_null_pair(&PCRE_G(unmatched_null_pair));
1001+
}
1002+
}
1003+
ZVAL_COPY(&match_pair, &PCRE_G(unmatched_null_pair));
1004+
} while (0);
10271005
} else {
1028-
if (Z_ISUNDEF(PCRE_G(unmatched_empty_pair))) {
1029-
init_unmatched_empty_pair();
1030-
}
1031-
ZVAL_COPY(&match_pair, &PCRE_G(unmatched_empty_pair));
1006+
do {
1007+
if (Z_ISUNDEF(PCRE_G(unmatched_empty_pair))) {
1008+
if (UNEXPECTED(EG(flags) & EG_FLAGS_IN_SHUTDOWN)) {
1009+
init_unmatched_empty_pair(&match_pair);
1010+
break;
1011+
} else {
1012+
init_unmatched_empty_pair(&PCRE_G(unmatched_empty_pair));
1013+
}
1014+
}
1015+
ZVAL_COPY(&match_pair, &PCRE_G(unmatched_empty_pair));
1016+
} while (0);
10321017
}
10331018
} else {
10341019
zval val1, val2;

ext/pcre/php_pcre.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@ ZEND_BEGIN_MODULE_GLOBALS(pcre)
7878
#ifdef HAVE_PCRE_JIT_SUPPORT
7979
bool jit;
8080
#endif
81-
bool per_request_cache;
8281
php_pcre_error_code error_code;
8382
/* Used for unmatched subpatterns in OFFSET_CAPTURE mode */
8483
zval unmatched_null_pair;

ext/pcre/tests/gh15205_1.phpt

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
--TEST--
2+
GH-15205: UAF when destroying RegexIterator after pcre request shutdown
3+
--CREDITS--
4+
YuanchengJiang
5+
--FILE--
6+
<?php
7+
$array = new ArrayIterator(array('a', array('b', 'c')));
8+
$regex = [new RegexIterator($array, '/Array/')];
9+
echo "Done\n";
10+
?>
11+
--EXPECT--
12+
Done

ext/pcre/tests/gh15205_2.phpt

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
--TEST--
2+
GH-15205: UAF when destroying stream after pcre request shutdown
3+
--CREDITS--
4+
nicolaslegland
5+
--FILE--
6+
<?php
7+
// Prime cache
8+
preg_replace('/pattern/', 'replace', 'subject');
9+
10+
class wrapper
11+
{
12+
public function stream_open($path, $mode, $options, &$opened_path)
13+
{
14+
return true;
15+
}
16+
17+
public function stream_close()
18+
{
19+
echo "Close\n";
20+
preg_replace('/pattern/', 'replace', 'subject');
21+
preg_match('/(4)?(2)?\d/', '23456', $matches, PREG_OFFSET_CAPTURE | PREG_UNMATCHED_AS_NULL);
22+
preg_match('/(4)?(2)?\d/', '23456', $matches, PREG_OFFSET_CAPTURE);
23+
}
24+
25+
public $context;
26+
}
27+
28+
stream_wrapper_register('wrapper', 'wrapper');
29+
$handle = fopen('wrapper://', 'rb');
30+
?>
31+
--EXPECT--
32+
Close

0 commit comments

Comments
 (0)