Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Process] Strong args escaping on Windows + deprecate compat settings #21347

Closed
wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jan 19, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #19993, #10486, #12488
License MIT
Doc PR -

Related to #3381.
Follow up of #21254.

  • deprecated !VAR! expansion inside escaped arguments
  • deprecated not-inheriting environment variables
  • deprecated configuring proc_open() options
  • deprecated configuring enhanced Windows compatibility
  • deprecated configuring enhanced sigchild compatibility

This PR uses a radically different approach to value injection into command lines on Windows.
Instead of try to play against cmd.exe, we play with cmd.exe: strings are injected through environment variables, and are resolved without any parsing of their values thanks to delayed expansion. Works around all edge cases I know about.

Copy link
Contributor

@romainneutron romainneutron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for this particularly hard work on Windows platform

👍

$uid = uniqid('', true);
$varCount = 0;
$varCache = array('""' => '""');
$cmd = preg_replace_callback(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet


* On Windows, `!VAR!` expansion inside escaped arguments is deprecated.

* Not-inheriting environment variables is deprecated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the - should be a space

* Not-inheriting environment variables is deprecated.

* Configuring `proc_open()` options, Windows compatibility and sigchild
compatibility is deprecated - they will be always enabled in 4.0.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be split in 2 entries, as they will be always enabled in 4.0 does not apply to options

-----

* deprecated `!VAR!` expansion inside escaped arguments
* deprecated not-inheriting environment variables
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be a space

$this->options = array_replace(array('suppress_errors' => true, 'binary_pipes' => true), $options);
if (null !== $options) {
@trigger_error(sprintf('The $options parameter of the %s constructor is deprecated since version 3.3 and will be removed in 4.0.', __CLASS__), E_USER_DEPRECATED);
$this->options = array_replace(array('suppress_errors' => true), $options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will not configure suppress_errors by default. Is it expected ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean binary_pipes? that's an option that used to exist on the never released PHP 6...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understood. fixed

{
$process = new Process($commandline, $cwd, $env, $input, $timeout, $options);
$process = new Process($commandline, $cwd, $env, $input, $timeout);
$process->inheritEnvironmentVariables(true, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this call ? Isn't inheriting the default already ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately no - so yes, this call is required.
Other comments addressed also.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this means that people are then required to call this deprecated method to have the non-deprecated behavior, and then they have to pass the internal second argument to avoid the deprecation warning (and their code won't still be ready for 4.0, if the method is meant to be removed in 4.0, breaking the upgrade path)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, then no :)

here is the upgrade path:

for people that set $env but do not enable inheriting, they'll see this:

Not inheriting environment variables is deprecated since Symfony 3.3 and will always happen in 4.0. Call "Process::inheritEnvironmentVariables()" instead before running the command.

so they'll do it.

then people set $env and do enable inheriting, will see this:

The Process::inheritEnvironmentVariables method is deprecated since version 3.3 and will be removed in 4.0. Environment variables will always be inherited. To silence this notice, set the second argument of "Process::inheritEnvironmentVariables()" to "false", then wrap the call in a "method_exists()" check.

then wrap the call in a "method_exists()" check.

see eg this line for the Dotenv component.

I think that's enough an upgrade path for that matter. Better for you with these explanations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we never asked them to use our internal silencing arguments before, because using them makes it far too easy to call the method without warning without checking for it to be safe, and because these were meant to be internal.
Btw, your second argument is not even part of the method signature, meaning that their IDEs will complain about passing unknown arguments.
I don't think this is the best upgrade path.

On a side note, if we really want to keep this upgrade path, we should give them the right update path from the start IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the best upgrade path.
we should give them the right update path from the start IMO.

That's true - a smoother upgrade path would be to not deprecate the method in 3.x, but do it in 4.1. We already did so in the past.
Yet, I think this is not worth it for the feature we're talking about. So a more brutal - yet continuous - upgrade path in enough here. At least that what I'm proposing here.

Taking this into account, what path to you think we should take?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with the smoother upgrade path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smoother upgrade path done

@johnstevenson
Copy link
Contributor

Hey, this looks really nice and cunning! But it is going to break ProcessUtils::escapeArgument as a standalone function isn't it?

@nicolas-grekas
Copy link
Member Author

As far as I tested correctly, using the function standalone is going to still work nice. With edge cases of course, but that's still reasonable, and safe.

@johnstevenson
Copy link
Contributor

It's Windows, so there will always be edge cases! I hadn't realized that cmd doesn't handle line-endings so a "first\nsecond" argument will end up as first!LF!second, which may or may not be better than first and no more arguments!

More importantly though, using consecutive double-quotes "" will break command-line parsing at some point, because you have no control of the quoted-string state (so the wrong value could bleed into the next argument).

@johnstevenson
Copy link
Contributor

As a follow up to the above, ie using the standalone function, with the example from #19993

$params = [
    'path=%PATH%',
    'quote="',
    'colors=red & blue',
];

the C/C++ runtime will correctly parse this as

path=%PATH%
quote="
colors=red & blue

whereas CommandLineToArgvW (and older runtimes) will find

path=%PATH%
quote=" colors=red
&
blue

@nicolas-grekas
Copy link
Member Author

using consecutive double-quotes "" will break command-line parsing

from the cmd.exe, double quotes are safe, so this can only lead to bad arguments, and not to shell injection. Far less an issue to me.

whereas CommandLineToArgvW (and older runtimes) will find [...]

can you give examples of such situations? I tried with php 5.3.3, no issue.
at some point, if using the function directly breaks, people should use the Process class. The fact that the function and the class are in the same component makes it a more likely situation. Since doing so is free from any issues, I see little need for trying harder.

@johnstevenson
Copy link
Contributor

I have given you an example of calling CommandLineToArgvW, above.

@nicolas-grekas
Copy link
Member Author

@johnstevenson would you like to send a PR on my fork/branch patching both prepareWindowsCommandLine & escapeArgument in a way that'd be better to you? I'd be happy to add it here!

@taylorotwell
Copy link
Contributor

Regarding inheriting environment variables... so in 4.0 there will be no way at all to not pass along the existing environment variables to a new process?

@nicolas-grekas
Copy link
Member Author

That's what I'm proposing yes, for discussion of course.
Do you see any use case where that would be an issue ? If you have unwanted env vars, you'll still be able to remove them explicitly for the command.
From my experience, not having eg PATH or HTTP_PROXY, etc. is more problematic. I'd prefer people to care about setting/unsetting the environment vars they know about, rather than allowing them to start with no ENV and discover later that they missed setting some var.
Makes sense to you ?

@taylorotwell
Copy link
Contributor

Dumb question maybe, but in 4.0 how would one unset the environment variables they don't want to pass along?

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jan 20, 2017

By setting them to false: ->setEnv(['foo' => false])

@nicolas-grekas nicolas-grekas force-pushed the shell-args branch 2 times, most recently from 3db4904 to 1aeeb12 Compare January 20, 2017 22:31
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jan 22, 2017

Inspecting cmd.exe behavior when delayed expansion is enabled, the escaping is even more contextual: if the command line contains a !, anywhere in the line, then carets become delayed-time escaping chars. If the line contains no !, carets are only first level escaping chars. That's contextual escaping on top of contextual escaping on top of contextual escaping.

I confirm that the current escapeArgument is good enough: it works perfectly when used together with Process. And works well enough when used without - and if that "well enough" is not good enough for someone, one should just use it together with Process.

@nicolas-grekas nicolas-grekas force-pushed the shell-args branch 4 times, most recently from d5197c0 to 38e65b7 Compare January 23, 2017 07:22
@johnstevenson
Copy link
Contributor

That's contextual escaping on top of contextual escaping on top of contextual escaping.

I find it easier to understand in terms of cmd,exe doing 2 passes on the command line: the first on the entire line (where an unquoted ^ caret will escape the next character and be removed) and the second on the inidividual commands. Whatever, it is a complicated business.

Your injection solution is very neat, but it does require that escapeArgument is used in conjunction with the Process class. Here's something you can try at home!

Filename: args.ps1
------------------

for ($i=0;$i -lt $args.length;$i++)
{
    Write-Host $args[$i]
}
$params = [
    'powershell',
    '-File',
    'args.ps1',
    'path=%PATH%',
    'quote="',
    'colors=red & blue',
];

$escaper = ['Symfony\Component\Process\ProcessUtils', 'escapeArgument'];
$command = implode(' ', array_map($escaper, $params));
passthru($command);

The above results in the following output:

path=%PATH%
quote=" colors=red
&
blue

So this doesn't actually fix #19993

@johnstevenson
Copy link
Contributor

would you like to send a PR on my fork/branch patching both prepareWindowsCommandLine & escapeArgument in a way that'd be better to you?

Sure, but I would use a modified version of my approach rather than injection, which would give us a different set of capabilities/limitations, as outlined below:

                                    With Injection  |   Without Injection
                                    (standalone)    |   (bypassing shell)
----------------------------------------------------|--------------------
Keeps bypass-shell                  No              |   Yes
Keeps delayed expansion             No              |   No
Handles internal quotes             Yes     (No)    |   Yes
Handles \n in arguments             Yes     (No)    |   No      (Yes)
Handles space and %..%              Yes     (No)    |   No      (Yes)
    in first argument
    
* Note 'standalone' refers to using escapeArgument on its own

The last two items, I'd suggest, are pretty unlikely.

  • The only other way in PHP to use \n in arguments is to use proc_open with the bypass shell option
  • You would need a module name like C:\Program Files\%my%prog.exe

@nicolas-grekas
Copy link
Member Author

@johnstevenson thanks to you insistence, I found another approach, see #21474 :)

@nicolas-grekas nicolas-grekas deleted the shell-args branch January 31, 2017 15:07
fabpot added a commit that referenced this pull request Feb 8, 2017
…ars, fixing signaling and escaping (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Process] Accept command line arrays and per-run env vars, fixing signaling and escaping

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #12488, #11972, #10025, #11335, #5759, #5030, #19993, #10486
| License       | MIT
| Doc PR        | -

I think I found a way to fix this network of issues once for all.
Of all the linked ones, only the last two are still open: the remaining were closed in dead ends.

Instead of trying to make `ProcessUtil::escapeArgument` work correctly on Windows - which is impossible as discussed in #21347 - this PR deprecates it in favor of a more powerful approach.

Depending on the use case:

- when a simple command should be run, `Process` now accepts an array of arguments (the "binary" being the first arg). Making this the responsibility of `Process` (instead of `ProcessBuilder`) gives two benefits:
  - escape becomes an internal detail that doesn't leak - thus can't be misused ([see here](#21347 (comment)))
  - since we know we're running a single command, we can prefix it automatically by "exec" - thus fixing a long standing issue with signaling

```php
        $p = new Process(array('php', '-r', 'echo 123;'));
        echo $p->getCommandLine();
        // displays on Linux:
        // exec 'php' '-r' 'echo 123;'
```

- when a shell expression is required, passing a string is still allowed. To make it easy and look-like sql prepared statements, env vars can be used when running the command. Since the shell is OS-specific (think Windows vs Linux) - this PR assumes no portability, so one should just use each shell's specific syntax.

From the fixtures:
```php
        $env = array('FOO' => 'Foo', 'BAR' => 'Bar');
        $cmd = '\\' === DIRECTORY_SEPARATOR ? 'echo !FOO! !BAR! !BAZ!' : 'echo $FOO $BAR $BAZ';
        $p = new Process($cmd, null, $env);
        $p->run(null, array('BAR' => 'baR', 'BAZ' => 'baZ'));

        $this->assertSame('Foo baR baZ', rtrim($p->getOutput()));
        $this->assertSame($env, $p->getEnv());
```

Commits
-------

330b61f [Process] Accept command line arrays and per-run env vars, fixing signaling and escaping
symfony-splitter pushed a commit to symfony/process that referenced this pull request Feb 8, 2017
…ars, fixing signaling and escaping (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Process] Accept command line arrays and per-run env vars, fixing signaling and escaping

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #12488, #11972, #10025, #11335, #5759, #5030, #19993, #10486
| License       | MIT
| Doc PR        | -

I think I found a way to fix this network of issues once for all.
Of all the linked ones, only the last two are still open: the remaining were closed in dead ends.

Instead of trying to make `ProcessUtil::escapeArgument` work correctly on Windows - which is impossible as discussed in #21347 - this PR deprecates it in favor of a more powerful approach.

Depending on the use case:

- when a simple command should be run, `Process` now accepts an array of arguments (the "binary" being the first arg). Making this the responsibility of `Process` (instead of `ProcessBuilder`) gives two benefits:
  - escape becomes an internal detail that doesn't leak - thus can't be misused ([see here](symfony/symfony#21347 (comment)))
  - since we know we're running a single command, we can prefix it automatically by "exec" - thus fixing a long standing issue with signaling

```php
        $p = new Process(array('php', '-r', 'echo 123;'));
        echo $p->getCommandLine();
        // displays on Linux:
        // exec 'php' '-r' 'echo 123;'
```

- when a shell expression is required, passing a string is still allowed. To make it easy and look-like sql prepared statements, env vars can be used when running the command. Since the shell is OS-specific (think Windows vs Linux) - this PR assumes no portability, so one should just use each shell's specific syntax.

From the fixtures:
```php
        $env = array('FOO' => 'Foo', 'BAR' => 'Bar');
        $cmd = '\\' === DIRECTORY_SEPARATOR ? 'echo !FOO! !BAR! !BAZ!' : 'echo $FOO $BAR $BAZ';
        $p = new Process($cmd, null, $env);
        $p->run(null, array('BAR' => 'baR', 'BAZ' => 'baZ'));

        $this->assertSame('Foo baR baZ', rtrim($p->getOutput()));
        $this->assertSame($env, $p->getEnv());
```

Commits
-------

330b61fecb [Process] Accept command line arrays and per-run env vars, fixing signaling and escaping
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants