ext/pcntl: pcntl_exec add open_basedir restriction check.

[devnexen]

if the admin wants to disallow relative path or other unauthorized accesses.

[vrza]

Might be worth considering the semantics of access restriction error handling specific to PHP open_basedir, compared to regular file system access error handling.

For example:

<?php

if (pcntl_exec('/root/bin/date') === false) {
    var_dump(pcntl_get_last_error() === PCNTL_EACCES);
}

if (pcntl_exec('/nonexistent') === false) {
    var_dump(pcntl_get_last_error() === PCNTL_ENOENT);
}

outputs

PHP Warning:  pcntl_exec(): Error has occurred: (errno 13) Permission denied in /home/random/code/php/exec/exec.php on line 3
bool(true)
PHP Warning:  pcntl_exec(): Error has occurred: (errno 2) No such file or directory in /home/random/code/php/exec/exec.php on line 7
bool(true)

Notice how granular the error handling in the Unix API is Although it could tell the user "you passed in a bad value", it goes the extra mile to introduce specific error numbers that give the user more insight into what happened ("file does not exist", or "you don't have acccess"), and empowers the user to easily introduce different code paths for different types of errors.

In that vein, we should have more specific errors associated with open_basedir restriction, as it's a very specific kind of error. that we completely control, rather than a generic ValueError.

[devnexen]

The aim is not for cases as your examples but more case where the path is usually allowed. Let's say the admin does not want the php user access to /bin/ls for example.

[vrza]

Let's say the admin does not want the php user access to /bin/ls for example.

Okay, lets examine this example. /bln/lsis a perfectly valid value for the path, and the file probably exists, but the open_basedir setting, that might have nothing to do with user code, but specific to the deployment, might be restricting it. So I would see that situation as an e.g. AccessError,, rather than a ValueError.

We would be wrong, or in the least unclear, to claim that /bln/ls is an incorrect path value.

[devnexen]

Yes your AccessError proposal makes sense ; sure.

[vrza]

Yeah, that's the point, let's be very specific about what kind of error occurred, to empower the user to easily handle different situations.

With open_basedir checks being in many places, it would make sense to me if these would have an even more specific error associated, e.g. OpenBasedirAccessError.

[vrza]

Although not likely, $TEST_PHP_EXECUTABLE might be under $HOME/bin.

Perhaps ensure that test will always be correct regardless of environment it runs in.

[Girgias]

I think this needs a discussion on the internals mailing list.

Normally we issue warnings for this case, so promoting it might make sense for new functions but I don't know what other people think about this.

[devnexen]

Ok will do

[vrza]

On one hand, it does break the API contract in a subtle way, and thus could break existing code paths for some users.

On the other hand, in some of these cases, their existing code + config combination might also pose a security issue.

[devnexen]

it won't go any further, the idea got enough downvotes :)

[vrza]

@devnexen I do think you are onto a legit security issue, the question is how to address it most elegantly. Perhaps we should open a GitHub issue ticket for discussion.

[devnexen]

Yes I meant it s over in the manner I envisioned but sure we should find another way.

[vrza]

We are on the same page. What you envision could be the way it's handled in the next API-breaking version of PHP (e.g. PHP 9). But 8.x users should be offered some opt-in fix instead.

[vrza]

Perhaps if we introduce an ephemeral (PHP 8.x only) php.ini feature flag, like pcntl_exec_respect_open_basedir (default false). Or even strict_open_basedir if there are multiple places where this is not honored.

In PHP 9.x we could remove the flag and make the secure behavior default.

[vrza]

And at the end of the day, open_basedir is known to be unreliable, and I'm of the opinion that this feature, at least as originally designed, is not necessary in the language at all (I don't see a case where I would use it myself), so not sure if it's worth spending much time on improving it, unless the design approach is changed fundamentally. Users are better off relying on external sandboxing (e.g. FreeBSD jails).

[Girgias]

Note that already 5y ago there was talk about removing and deprecating the setting: https://externals.io/message/105606

Maybe something to try and push for again.