A few days ago, I was working on an internal player API for SiegeGG Pro and encountered this error:
Uncaught ArgumentCountError: exec() expects at least 1 argument, 0 given
Now, here is the thing, I never called that function. The exec
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.
In 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
the $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.
Therefore, the is_callable($value)
is true for Exec’s player name as
a function with the same name exists, it’s—surprise—exec
.
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
function.
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
function like request
or 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
in the 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.