DEV Community

Cover image for My Upload Check Trusted the Attacker's Word: 8 Bytes Fixed It
HideyukiMORI
HideyukiMORI

Posted on

My Upload Check Trusted the Attacker's Word: 8 Bytes Fixed It

Summer Bug Smash: Smash Stories 🐛🛹

A QA pass sent my document uploader a Windows executable that declared itself application/pdf. It was accepted, stored, and listed like any other document.

The MIME allowlist that was supposed to stop it had run, matched, and passed. It wasn't buggy. It was reading a value the uploader had written.

This is the companion to my Bug Smash post about environment variables poisoning a test run — that one was about my machine lying to me. This one is about my input lying to me.

What the check actually covered

The uploader had what looked like several layers of protection. Laid out by who controls each value, the picture changes:

Layer What it checked Who controls it Stops a spoof?
accept=".pdf,.jpg,.jpeg,.png" file picker filter the browser UI ❌ cosmetic only
Declared MIME allowlist a string in the multipart body the uploader
File extension the filename the uploader
First bytes of the payload the actual content the file itself
nosniff + attachment on download how the browser treats the response the server ⚠️ mitigation, not a fix

Three of those layers were reading values the uploading client had chosen. The allowlist contents were correct — PDF, JPEG, PNG, exactly what the compliance rule says. It just never saw the file.

I wasn't validating the file. I was validating a string the uploader chose.

That bottom row deserves a note, because it's the one I could have hidden behind. The download path already sent X-Content-Type-Options: nosniff and Content-Disposition: attachment before any of this happened. That mitigation is real, and it predates the bug — which is exactly why it isn't a fix. It changes what a browser does with a file that I already agreed to store. Rejecting at intake is a different question, and I hadn't answered it.

How it was found

Not by a scanner. By a person following a QA script, escalating one step at a time: send a .exe (rejected, good), send an .svg (rejected, good), then send the .exe bytes with a spoofed application/pdf content type.

That third step is the one automated checks tend not to reach, because it requires deciding to lie about your own request.

The fix: read eight bytes

Keep the declared-MIME allowlist as a cheap first gate, then put content sniffing behind it. No new dependencies — no finfo, no library, composer.json untouched:

private function sniffMimeType(string $tmpPath): ?string
{
    $handle = @fopen($tmpPath, 'rb');
    if ($handle === false) {
        return null;
    }

    $header = fread($handle, 8);
    fclose($handle);

    if ($header === false || $header === '') {
        return null;
    }

    if (str_starts_with($header, '%PDF-')) {
        return 'application/pdf';
    }
    if (str_starts_with($header, "\xFF\xD8\xFF")) {
        return 'image/jpeg';
    }
    if (str_starts_with($header, "\x89PNG\r\n\x1A\n")) {
        return 'image/png';
    }

    return null;
}
Enter fullscreen mode Exit fullscreen mode

Eight bytes because the PNG signature is eight bytes long; the other two are shorter prefixes of the same read. The call site is four lines:

$sniffed = $this->sniffMimeType($input->tmpPath);
if ($sniffed === null || !in_array($sniffed, self::ALLOWED_MIME_TYPES, true)) {
    throw new MimeTypeNotAllowedException($input->mimeType, $sniffed ?? 'unknown');
}
Enter fullscreen mode Exit fullscreen mode

Two decisions in there matter more than the signatures.

Undecidable means rejected. An unreadable file, an empty file, or anything whose first bytes don't match a known signature all return null, and null fails. A sniffer that returns "unknown" and then shrugs is not a gate.

The two rejections say different things. A disallowed declaration gets File type 'x' is not allowed. Only PDF, JPEG, and PNG are accepted. A spoof gets Declared file type 'application/pdf' does not match the file content ('unknown'). Only genuine PDF, JPEG, and PNG files are accepted. Collapsing those into one message would have been less code and would have thrown away the only signal that distinguishes a confused user from someone probing you.

One thing I did not do: SVG is rejected, not sanitized. An SVG renamed and re-declared as PNG fails the signature check and never reaches storage. If you need to accept SVG, this post doesn't help you — that's a different problem with a much larger surface.

The test that couldn't be written

Here's the part that changed how I read my own test suites.

Before the fix, the test helper passed tmpPath: '/tmp/fake-upload'. That's a string. There was no file at that path. There had never been a file at that path.

So a content-based test wasn't merely missing from the suite. It was impossible to write. The validation logic stopped at the declared value because the fixtures had no content for it to go on to.

The check was as deep as my fixtures were.

The line that unlocked everything was tempnam() — create a real temporary file, write real bytes into it, delete it in tearDown(). Once fixtures were files instead of strings, the new cases wrote themselves: a spoofed .exe asserting the mismatch message, an SVG declared as PNG, and genuine JPEG and PNG regressions to prove the gate still lets real documents through.

What's still open

The vulnerability is closed at intake, and I'd rather end on the part I haven't finished than on the part I have.

The end-to-end test that originally caught this still carries its discovery-time assertion:

expect(listed > 0 || modalText.length > 0, 'upload resolved (accepted or errored)').toBeTruthy();
Enter fullscreen mode Exit fullscreen mode

Read that carefully. It passes when the spoofed file is accepted and listed, and it passes when the upload errors out. It was written to observe what happened, back when nobody knew. It would go green whether or not my fix works.

I closed the vulnerability and left the test that found it unable to prove it. A test written to demonstrate a bug does not become a regression test until you flip its expectation.

What I'd tell past me

  1. If the value you're validating arrived in the request body, you're validating the attacker's input, not the file.
  2. Undecidable must mean rejected. "Unknown, so I'll allow it" is not a gate.
  3. Fixtures set the ceiling on what your tests can check. A fixture that isn't a file can never catch a content bug.

What's still in your upload path that the uploader gets to declare?

── Hideyuki Mori (Ayane International) 🔗 hideyuki-mori.com

Top comments (0)