Skip to content

Commit

Permalink
Fix GHSA-wpj3-hf5j-x4v4: __Host-/__Secure- cookie bypass due to partial
Browse files Browse the repository at this point in the history
CVE-2022-31629 fix

The check happened too early as later code paths may perform more
mangling rules. Move the check downwards right before adding the actual
variable.
  • Loading branch information
nielsdos authored and ericmann committed Apr 9, 2024
1 parent 0d89b54 commit f77e579
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 14 deletions.
63 changes: 63 additions & 0 deletions ext/standard/tests/ghsa-wpj3-hf5j-x4v4.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
--TEST--
ghsa-wpj3-hf5j-x4v4 (__Host-/__Secure- cookie bypass due to partial CVE-2022-31629 fix)
--COOKIE--
..Host-test=ignore_1;
._Host-test=ignore_2;
.[Host-test=ignore_3;
_.Host-test=ignore_4;
__Host-test=ignore_5;
_[Host-test=ignore_6;
[.Host-test=ignore_7;
[_Host-test=ignore_8;
[[Host-test=ignore_9;
..Host-test[]=ignore_10;
._Host-test[]=ignore_11;
.[Host-test[]=ignore_12;
_.Host-test[]=ignore_13;
__Host-test[]=legitimate_14;
_[Host-test[]=legitimate_15;
[.Host-test[]=ignore_16;
[_Host-test[]=ignore_17;
[[Host-test[]=ignore_18;
..Secure-test=ignore_1;
._Secure-test=ignore_2;
.[Secure-test=ignore_3;
_.Secure-test=ignore_4;
__Secure-test=ignore_5;
_[Secure-test=ignore_6;
[.Secure-test=ignore_7;
[_Secure-test=ignore_8;
[[Secure-test=ignore_9;
..Secure-test[]=ignore_10;
._Secure-test[]=ignore_11;
.[Secure-test[]=ignore_12;
_.Secure-test[]=ignore_13;
__Secure-test[]=legitimate_14;
_[Secure-test[]=legitimate_15;
[.Secure-test[]=ignore_16;
[_Secure-test[]=ignore_17;
[[Secure-test[]=ignore_18;
--FILE--
<?php
var_dump($_COOKIE);
?>
--EXPECT--
array(3) {
["__Host-test"]=>
array(1) {
[0]=>
string(13) "legitimate_14"
}
["_"]=>
array(2) {
["Host-test["]=>
string(13) "legitimate_15"
["Secure-test["]=>
string(13) "legitimate_15"
}
["__Secure-test"]=>
array(1) {
[0]=>
string(13) "legitimate_14"
}
}
41 changes: 27 additions & 14 deletions main/php_variables.c
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,21 @@ PHPAPI void php_register_known_variable(const char *var_name, size_t var_name_le
php_register_variable_quick(var_name, var_name_len, value, symbol_table);
}

/* Discard variable if mangling made it start with __Host-, where pre-mangling it did not start with __Host-
* Discard variable if mangling made it start with __Secure-, where pre-mangling it did not start with __Secure- */
static bool php_is_forbidden_variable_name(const char *mangled_name, size_t mangled_name_len, const char *pre_mangled_name)
{
if (mangled_name_len >= sizeof("__Host-")-1 && strncmp(mangled_name, "__Host-", sizeof("__Host-")-1) == 0 && strncmp(pre_mangled_name, "__Host-", sizeof("__Host-")-1) != 0) {
return true;
}

if (mangled_name_len >= sizeof("__Secure-")-1 && strncmp(mangled_name, "__Secure-", sizeof("__Secure-")-1) == 0 && strncmp(pre_mangled_name, "__Secure-", sizeof("__Secure-")-1) != 0) {
return true;
}

return false;
}

PHPAPI void php_register_variable_ex(const char *var_name, zval *val, zval *track_vars_array)
{
char *p = NULL;
Expand Down Expand Up @@ -139,20 +154,6 @@ PHPAPI void php_register_variable_ex(const char *var_name, zval *val, zval *trac
}
var_len = p - var;

/* Discard variable if mangling made it start with __Host-, where pre-mangling it did not start with __Host- */
if (strncmp(var, "__Host-", sizeof("__Host-")-1) == 0 && strncmp(var_name, "__Host-", sizeof("__Host-")-1) != 0) {
zval_ptr_dtor_nogc(val);
free_alloca(var_orig, use_heap);
return;
}

/* Discard variable if mangling made it start with __Secure-, where pre-mangling it did not start with __Secure- */
if (strncmp(var, "__Secure-", sizeof("__Secure-")-1) == 0 && strncmp(var_name, "__Secure-", sizeof("__Secure-")-1) != 0) {
zval_ptr_dtor_nogc(val);
free_alloca(var_orig, use_heap);
return;
}

if (var_len==0) { /* empty variable name, or variable name with a space in it */
zval_ptr_dtor_nogc(val);
free_alloca(var_orig, use_heap);
Expand Down Expand Up @@ -256,6 +257,12 @@ PHPAPI void php_register_variable_ex(const char *var_name, zval *val, zval *trac
return;
}
} else {
if (php_is_forbidden_variable_name(index, index_len, var_name)) {
zval_ptr_dtor_nogc(val);
free_alloca(var_orig, use_heap);
return;
}

gpc_element_p = zend_symtable_str_find(symtable1, index, index_len);
if (!gpc_element_p) {
zval tmp;
Expand Down Expand Up @@ -293,6 +300,12 @@ PHPAPI void php_register_variable_ex(const char *var_name, zval *val, zval *trac
zval_ptr_dtor_nogc(val);
}
} else {
if (php_is_forbidden_variable_name(index, index_len, var_name)) {
zval_ptr_dtor_nogc(val);
free_alloca(var_orig, use_heap);
return;
}

zend_ulong idx;

/*
Expand Down

0 comments on commit f77e579

Please sign in to comment.