A few days ago, I was working on an internal player API for SiegeGG Pro and encoutered this error:
Uncaught ArgumentCountError: exec() expects at least 1 argument, 0 given
Now, here is the thing, I never called that function. The
function in PHP executes an external program. At SiegeGG, we
only use the function for one feature: starting our data importer.
However, I was working with player data. That data is already imported
and there is no reason why the importer would be triggered.
So, if it’s not because of the importer, how did the
exec function end
up getting called? Let’s dig into it.
The error was thrown in the following line of
codingpaws/expose, our dependency we use to simplify the
creation of API entities:
$value = is_callable($value) ? $value() : $value;
This line of code gets executed to generate the value of an API field. For example, the kill-death ratio of a player will result in a division by zero if there are no deaths. In that case, we could conditionally expose the API field and dynamically generate the value.
codingpaws/expose syntax, that instruction would look like this:
$this->expose( 'kd', fn () => $player->kills / $player->deaths, if: $player->deaths > 0 );
The second argument becomes the
$value in the previous code example. If
$value is a callback, and therefore callable, we want to call it to
get the value it returns if the player has died more than zero times.
But, how did this end up calling the
exec function? Remember, the
importer is detached from the API and not active.
To find an answer, I narrowed down the error to the Rainbow Six Siege esports player Exec, who last played 6 years ago. And you might have a suspicion why this caused the error. It turns out, in PHP, a string is callable if a global function exists that is named like the string value.
is_callable($value) is true for Exec’s player name as
a function with the same name exists, it’s—surprise—
Expose goes ahead and calls
exec for us. This is
technically correct but very, very bad here.
Exec is, semantically,
a player name and not a function. But in PHP,
exec is also the
This unintuitive behavior of PHP can even be a security vulnerability as,
right here, Expose deals with arbitrary user data. While an
external user can’t control the arguments, in some cases, they could
execute any global function. As the resulting
$value will be sent via
the API to the requesting user, getting the result of a global Laravel
app could potentially expose secrets and
lead to a compromise of the system or user data.
Luckily, the fix for this is simple. We just need to check that the
$value is not a string and callable to call it. I fixed it in
3b05e9f2 (and previously hotfixed it in SiegeGG production when I
discovered the bug).
-$value = is_callable($value) ? $value() : $value; +$value = !is_string($value) && is_callable($value) ? $value() : $value;
The design decision allowing strings to be callable might’ve been a
useful change when it was made years ago. But it introduces the risk
that arbitrary data becomes callable, potentially opening an application
up for data leaks or other damage. Unfortunately, this is not explained
is_callable docs, although it is somewhat implied.
Thanks to the introduction of the First-class Callable
Syntax, we technically no longer need
is_callable to be
true for names of global functions. But it’s still in PHP for backwards
compatibility and will probably stick with us until at least PHP 9.0, the
next major PHP version.