DEV Community

Christ-loisele Atidegla
Christ-loisele Atidegla

Posted on

The Laravel upload check that checks nothing, one word away from the two that work

Here is a file upload check that provides no security at all.

if (in_array($file->getClientMimeType(), ['image/jpeg', 'image/png'])) {
    $file->store('avatars');
}
Enter fullscreen mode Exit fullscreen mode

getClientMimeType() returns the Content-Type header that came with the upload. The browser sends that header, and whoever is uploading decides what the browser sends. Rename a PHP file to .jpg, set the header to image/jpeg, and this passes.

That much is unsurprising once you say it out loud. The reason the mistake keeps happening is the interesting part, and it is not carelessness.

Laravel already gives you the right answer, twice

Both of these are safe:

$request->validate(['avatar' => 'required|file|mimes:jpg,png']);
$request->validate(['avatar' => 'required|file|mimetypes:image/jpeg,image/png']);
Enter fullscreen mode Exit fullscreen mode

I want to be precise about why, because I got this wrong myself and shipped a static analysis rule based on the wrong version. Reading the framework source settles it.

validateMimes calls $value->guessExtension(). validateMimetypes calls $value->getMimeType(). Symfony's UploadedFile does not override getMimeType(), so both end up in File::getMimeType():

public function getMimeType(): ?string
{
    return MimeTypes::getDefault()->guessMimeType($this->getPathname());
}
Enter fullscreen mode Exit fullscreen mode

guessMimeType($path) inspects the file on disk. So both validation rules read the actual contents. Neither reads the client header.

UploadedFile does expose the header, as getClientMimeType(), and it has a sibling called guessClientExtension() that derives an extension from that same untrusted value. Neither is used by any validation rule.

So the framework offers two safe checks and one unsafe accessor, separated by the word Client, sitting next to each other in autocomplete.

The difference between the two safe ones

mimes: takes extensions and compares against guessExtension(), which is the detected MIME type mapped back to an extension. mimetypes: takes MIME types and compares against the detected type directly.

Both read the file. mimes: loses a little information in the round trip through the extension map, so mimetypes: is the more direct expression if you want to be exact about which types you accept. Using both together is stronger and costs nothing.

The bit almost nobody knows

Both rules call a private helper first:

protected function shouldBlockPhpUpload($value, $parameters)
{
    if (in_array('php', $parameters)) {
        return false;
    }

    $phpExtensions = [
        'php', 'php3', 'php4', 'php5', 'php7', 'php8', 'phtml', 'phar',
    ];

    return in_array(trim(strtolower($value->getClientOriginalExtension())), $phpExtensions);
}
Enter fullscreen mode Exit fullscreen mode

If the client's original filename ends in a PHP extension, validation fails regardless of what the content sniffing says, unless you explicitly listed php as an allowed type. It is a belt and braces check against the polyglot file trick, where a file is a valid image and valid PHP at the same time.

This one does read a client supplied value, and that is correct here, because it is used to reject rather than to accept. Hostile input can be trusted to say no.

Why this needs a framework-aware rule

A generic security scanner has nothing to hook onto. getClientMimeType() is an ordinary method call, and whether it is a vulnerability depends on what you do with the result. Logging it is fine. Echoing it back is fine. Branching on it to decide whether to store a file is not.

That is roughly what the rule says:

patterns:
  - pattern-either:
      - pattern: in_array($F->getClientMimeType(), ...)
      - pattern: $F->getClientMimeType() === $X
      - pattern: $X === $F->getClientMimeType()
Enter fullscreen mode Exit fullscreen mode

It matches comparisons, because a comparison is where the value drives a decision. Reading the value into a log line does not, and flagging that would be noise.

It lives in stacksec, a small Semgrep ruleset for Laravel and Next.js. You can run it against a project with nothing installed:

npx --yes semgrep --config https://raw.githubusercontent.com/catidegla/stacksec/main/rules .
Enter fullscreen mode Exit fullscreen mode

Semgrep's own registry already carries Laravel rules, including raw SQL injection and mass assignment, so run p/php alongside rather than instead. These add the cases that live in framework idioms.

The postscript worth reading

The first version of that rule flagged mimetypes: and claimed it trusted the client header. That is backwards. It fired on correct code at high confidence, and it would have talked people out of a safe validation rule.

I caught it while checking a search ranking, when the sources disagreed with my own README. The fix took ten minutes.

The mistake is written up in the repo's REJECTED.md rather than quietly patched out.

Top comments (0)