main: Change the register_argc_argv INI default to Off

[TimWolla]

This partly implements the deprecation of the register_argc_argv INI setting by updating the default value to ensure safe behavior when no INI file is loaded. The actual deprecation warning will follow separately.

RFC: https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_the_register_argc_argv_ini_directive

[TimWolla]

@cmb69 Any idea for the Windows-specific failure?

[cmb69]

The --INI-- section is ignored on Windows, due to 9382673. Simplest solution would be to modify the test so the query string contains an equals sign:

 tests/basic/011.phpt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/basic/011.phpt b/tests/basic/011.phpt
index 03fccaa9b70..fd359fe4032 100644
--- a/tests/basic/011.phpt
+++ b/tests/basic/011.phpt
@@ -3,7 +3,7 @@
 --INI--
 register_argc_argv=1
 --GET--
-ab+cd+ef+123+test
+foo=ab+cd+ef+123+test
 --FILE--
 <?php
 $argc = $_SERVER['argc'];
@@ -15,7 +15,7 @@
 
 ?>
 --EXPECT--
-0: ab
+0: foo=ab
 1: cd
 2: ef
 3: 123

[TimWolla]

@cmb69 Thanks. What a terrible behavioral difference.

So I just realized register_argc_argv not only affects the “bare variables”, but also the contents of $_SERVER. Having the latter in CGI mode doesn't sound too bad actually 🤔 But of course with the current defaults in the suggested INIs it's unlikely that anyone actually relies on $_SERVER['argv'] in CGI mode.

/cc @nicolas-grekas

[nicolas-grekas]

CGI is what register_argc_argv has been made for I suppose, but it's a terrible idea indeed.
I agree the most likely is that this is disabled on CGI most of the time.
About the deprecation, maybe we could make it SAPI-independent by throwing the deprecation only when the setting is set to 1? The only thing we'd need to figure out then is a way for the CLI-alike SAPIs to be able to turn this on without triggering the deprecation.
Then in 9.0 we could just ignore this setting.

[TimWolla]

Sorry for dropping the ball on this. I've thought about this a little more now and I think the solution proposed in the RFC is not quite ideal.

To spell it out once more: register_argc_argv controls both $argc and $argv, as well as $_SERVER['argc'] and $_SERVER['argv']

Now my understanding is that the security concerns are with regard to $argc and $argv, since it's similar to register_globals, so $_SERVER['argc'] and $_SERVER['argv'] are fine.

So it would make sense to me to:

• Decouple the registration of $_SERVER['argc'] and $_SERVER['argv'] from the INI setting.
• Deprecate the INI setting (and the $argc and $argv variables) entirely, instead of just deprecating it for non-CLI SAPIs.

This would make the implementation much simpler and more predictable, since we would not need to define what a CLI SAPI is.

Changing the built-in default to make PHP safer when running without an INI (i.e. what this PR does) definitely makes sense. But I would perhaps defer emitting a deprecation notice to PHP 8.6 and then do another vote based on these new findings?

[nicolas-grekas]

Not sure what made you think so but $_SERVER is actually the most common vector.
See eg symfony/symfony@a77b308
And you'll find similar patches for laravel and craftcms for example,

[TimWolla]

Not sure what made you think so

Quoting from the RFC text:

This INI directive tells PHP whether to declare the argv & argc variables.

---

but $_SERVER is actually the most common vector.

Okay. Can you explain the Symfony fix for me?

Why is it checking for !empty($_GET) including checks for the INI setting and not just bailing out completely if PHP_SAPI !== 'cli'? If the application is designed to be used on the CLI, then to me checking that the SAPI is the CLI SAPI is the obvious solution to fix the issue.

[nicolas-grekas]

Because hardcoding the list of SAPI keeps the issue open for unlisted SAPIs. The world is more vast than just the current public SAPIs.
I can explain the patch, but this security issue has already happened and others will hit it in the future if the hole is not filled. Which is the point of the RFC. Also because there's no use case for this behavior.

About the patch: the issue happens with specially crafted query strings. When $_GET is empty, we know we're not under attack.

[nicolas-grekas]

Quoting from the RFC text:

This INI directive tells PHP whether to declare the argv & argc variables.

Well, that's an ellipsis in the RFC, I should have mentioned any variant of those variables (aka what is in _SERVER also).

[TimWolla]

So, basically what the RFC actually intended to propose (XY problem) is the following:

Deprecate deriving $argc, $argv, $_SERVER['argc'], and $_SERVER['argv'] from $_SERVER['QUERY_STRING'].

In that case, the proper fix is not deprecating the INI setting, but rather emitting a deprecation notice when this code path is taken:

php-src/main/php_variables.c

Lines 678 to 679 in e844e68

	} else if (s && *s) {
	while (1) {

diff --git i/main/php_variables.c w/main/php_variables.c
index 91eb0a7f5ce..72ec60e814f 100644
--- i/main/php_variables.c
+++ w/main/php_variables.c
@@ -676,6 +676,7 @@ PHPAPI void php_build_argv(const char *s, zval *track_vars_array)
                        }
                }
        } else  if (s && *s) {
+               zend_error(E_DEPRECATED, "Deriving the argc from query_string is deprecated");
                while (1) {
                        const char *space = strchr(s, '+');
                        /* auto-type */

results in:

$ sapi/cli/php test5.php --debug=1
int(2)
array(2) {
  [0]=>
  string(9) "test5.php"
  [1]=>
  string(9) "--debug=1"
}
int(2)
array(2) {
  [0]=>
  string(9) "test5.php"
  [1]=>
  string(9) "--debug=1"
}

and when running with the integrated web server:

Deprecated: Deriving the argc from query_string is deprecated in Unknown on line 0

Deprecated: Deriving the argc from query_string is deprecated in php-src/test5.php on line 3

Warning: Undefined variable $argc in php-src/test5.php on line 3

Warning: Undefined variable $argv in php-src/test5.php on line 3
NULL NULL int(1) array(1) { [0]=> string(9) "--debug=1" } 

is printed.

---

The latter output also makes me realize that $argc and $argv are already registered for CLI only, even if register_argc_argv=1.

[TimWolla]

Because hardcoding the list of SAPI keeps the issue open for unlisted SAPIs. The world is more vast than just the current public SAPIs.

No, because it would be an allow-list: Bail out when the SAPI is not CLI, i.e. as the very first line of the application you would use:

if (\PHP_SAPI !== 'cli') {
    \http_response_code(400);
    echo "Must be run on CLI";
    exit();
}

I can explain the patch, but this security issue has already happened and others will hit it in the future if the hole is not filled. Which is the point of the RFC. Also because there's no use case for this behavior.

Yes, I understand. But I'm trying to fully understand the situation before proposing a change that does not actually do the right thing.

[TimWolla]

In that case, the proper fix is not deprecating the INI setting, but rather emitting a deprecation notice when this code path is taken:

Or to clarify: The deprecation notice would mention the INI setting as a fix to suppress the deprecation message, but the issue is not with the INI setting itself, which to me is an important distinction in the mental model.

[nicolas-grekas]

Not sure there's any difference in practical terms.
Removing an ini setting (in PHP9) reduces the complexity of configuring PHP.

[derickr]

So, basically what the RFC actually intended to propose (XY problem) is the following:

Deprecate deriving $argc, $argv, $_SERVER['argc'], and $_SERVER['argv'] from $_SERVER['QUERY_STRING'].

If it had done that, I would have voted against it. But it didn't, so we can't change the behaviour to remove these from $_SERVER[].

[cmb69]

It may make sense to discuss this issue on internals.

[TimWolla]

If it had done that, I would have voted against it. But it didn't, so we can't change the behaviour to remove these from $_SERVER[].

I'm not sure where the quoted sentence says that.

[TimWolla]

It may make sense to discuss this issue on internals.

@cmb69 After the discussion with Nicolas I've properly understood both the current behavior and the problem. A draft for the actual deprecation notice that matches the RFC intent is in #19606.

This PR should be good to go, since it just changes the built-in default to match the recommended default of the example INIs.

[Girgias]

This looks reasonable and matches the RFC proposal AFAIU

[edorian]

RM approval