From 7b359738054d0dc51bacbda74a2235f3cd602e28 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Sat, 22 Aug 2026 05:03:32 +1000 Subject: [PATCH] Refuse the extension a whitespace-only name hid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Filename::extensionComponents()` normalized before it dropped the first dot-separated field, and `normalizeComponents()` drops a field that is nothing but spaces. `" .php"` therefore normalized to `['php']` and the drop took that for the name, leaving `refuseBlockedExtensions()` nothing to match. The file was written, and `mod_mime` splits it on the same dot and runs it. It splits with a bounded `explode()` now, so the first field is dropped whatever it holds — the field `deviceComponent()` takes. Reachable only from a `FileInfoInterface` of your own or a `resolveFilename()` override, which is the case those refusals exist for; the shipped `FileInfo` never produces an empty first field. The deny-list gains nine entries with a documented execution path: `asis` (mod_asis serves the file as a complete HTTP response), `erb`, `rhtml`, `htr`, `idc`, `printer`, `cfm`, `cfml` and `cfc`. `Filename::sanitizeForDisplay()` bounds its result at the new `MAX_DISPLAY_LENGTH`, so a validation of your own cannot flood whatever renders `getErrors()`. 2048 rather than `MAX_LENGTH`, because a message naming a long allow-list is legitimately longer than a filename. The bound is a parameter, so `FileList::describeKey()` names the 255 a field name gets rather than cutting for itself. `forceValidUtf8()` takes an incomplete trailing sequence off before it converts. `symfony/polyfill-mbstring` converts through `iconv()`, which answers `false` for a sequence the end of the string cuts short rather than dropping it and keeping the rest, so a client name ending mid-character came back as `unnamed-file` there — and since the polyfill ships no `mb_strcut()`, a byte cut made one out of a name that arrived whole. Only an incomplete tail matches, so a complete sequence at the end survives; a lookbehind keeps the scan linear, since a run of lead bytes otherwise retries at every offset. The leading-dot refusal in `refuseUnsafeName()` was documented as covering what the deny-list could not see. The deny-list sees `.htaccess` now, so that rationale is restated as the one that still holds: it is a list of extensions and does not cover `.env`. The CHANGELOG's account of what 3.x did is scoped to the path it was ever true on: checked against 3.1.0, the shipped `FileInfo` renamed `.htaccess` to `unnamed-file.htaccess` and reduced `../escape.txt` to `escape.txt`, so only a `FileInfoInterface` of your own reached the upload directory with either. Two layers treat a space as significant and both are ASCII: `trim()` at the ends of a name and of a deny-list component, and the `rtrim($filename, " .")` that takes off what Windows resolves away. Nothing pinned what the other space characters do, so the ASCII cases that were covered would not have noticed a change to either. They are left alone, at every position: a name keeps one leading, interior or trailing; an extension carrying one is discarded whole; `\u{00A0}con` is a file rather than the console; and a deny-list component keeps it, so `php` beside one is not `php` and `evil.php\u{00A0}` is written under a name nothing maps to the interpreter. Refusing that would mean inventing a rule no file system applies, so the test asserts the name it is stored under. `U+200B` is the exception and is covered as one, being a zero-width mark in `Filename::BIDI_CONTROLS`; `U+2000`/`U+200A` bracket the block that constant matches from `U+200B`, and `U+202F`/`U+205F` sit just outside its other two byte ranges, so a change to any of the three fails there. `tests/Upload/UnicodeSpaces.php` holds the set, required from `tests/bootstrap.php` like the other fixtures nothing autoloads, because the answer has to hold at both layers or they disagree about what a filename is. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 6 +- CLAUDE.md | 9 +- README.md | 15 ++- src/Upload/FileList.php | 7 +- src/Upload/Filename.php | 51 ++++++++-- src/Upload/Storage/FileSystem.php | 21 ++-- tests/Upload/FileInfoTest.php | 60 ++++++++++++ tests/Upload/FileTest.php | 18 ++++ tests/Upload/FilenameTest.php | 124 ++++++++++++++++++++++-- tests/Upload/Storage/FileSystemTest.php | 64 +++++++++++- tests/Upload/UnicodeSpaces.php | 45 +++++++++ tests/bootstrap.php | 1 + 12 files changed, 380 insertions(+), 41 deletions(-) create mode 100644 tests/Upload/UnicodeSpaces.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 15db32e..254f80c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ A security release. New protections are on by default and will refuse some uploa ## Defaults That Changed -* **An extension deny-list is on.** `FileSystem` refuses to write anything in `FileSystem::getDefaultBlockedExtensions()`: the 59 executable extensions in `EXECUTABLE_EXTENSIONS`, plus 15 markup ones in `MARKUP_EXTENSIONS` (`html`, `htm`, `xhtml`, `xht`, `xhtm`, `svg`, `svgz`, `xml`, `xsl`, `xslt`, `js`, `mjs`, `swf`, `mht`, `mhtml`). A server doesn't execute markup, but serving it from your own origin is stored XSS. **To accept one of them, drop those entries and keep the rest of the list** — `array_diff(FileSystem::getDefaultBlockedExtensions(), ['svg', 'svgz'])` — and sanitize the contents yourself +* **An extension deny-list is on.** `FileSystem` refuses to write anything in `FileSystem::getDefaultBlockedExtensions()`: the 68 executable extensions in `EXECUTABLE_EXTENSIONS`, plus 15 markup ones in `MARKUP_EXTENSIONS` (`html`, `htm`, `xhtml`, `xht`, `xhtm`, `svg`, `svgz`, `xml`, `xsl`, `xslt`, `js`, `mjs`, `swf`, `mht`, `mhtml`). A server doesn't execute markup, but serving it from your own origin is stored XSS. **To accept one of them, drop those entries and keep the rest of the list** — `array_diff(FileSystem::getDefaultBlockedExtensions(), ['svg', 'svgz'])` — and sanitize the contents yourself * **A blocked extension is refused in any dot-separated component of the name**, not only the one `pathinfo()` returns, since a web server does not necessarily treat the last component as the extension. Trailing dots and spaces are removed first, and the first component is never treated as an extension, so a file called `php` is still stored. The shipped `FileInfo` presents one component either way: `setName()` rewrites interior dots to hyphens as it always has, storing `archive.config.zip` as `archive-config.zip` * **Stored files get mode `0640`** (`FileSystem::DEFAULT_MODE`). 3.x left the mode to the umask * **`File::upload()` throws when no validations have been added** @@ -17,13 +17,13 @@ A security release. New protections are on by default and will refuse some uploa * **`FileInfo::setName()` rewrites the C1 controls (`U+0080`–`U+009F`) and `\x7F` to `-`.** The filter covered C0 only, and C1 survived because those bytes are valid UTF-8. One of them ends a line for anything reading `\R`, so a stored name could span what looked like several lines of a log. `Storage\FileSystem` refuses both, in `resolveFilename()` * **`FileInfo::setName()` deletes bidi and invisible characters** (`Filename::BIDI_CONTROLS`): Unicode's `Bidi_Control` property including `U+061C`, the zero-width marks `U+200B`–`U+200F`, the line and paragraph separators `U+2028` and `U+2029`, `U+206A`–`U+206F`, and the BOM. They let a name display in an admin listing, email or log as something other than what it is. `Storage\FileSystem` refuses a name still carrying one, in `upload()` rather than in the `resolveFilename()` seam. `Filename::hasControlCharacters()` and `hasBidiControls()` are public so an override can apply them itself * **`Filename::RESERVED_WINDOWS_NAMES` gained `COM0`, `LPT0` and the superscript variants** (`COM¹`, `COM²`, `COM³`, `LPT¹`, `LPT²`, `LPT³`), which Microsoft lists alongside `COM1`–`COM9`. A matching name is blanked to `unnamed-file` -* **Storage refuses a destination that is not a plain filename.** The name is reduced to a `basename()`, and `''`, `.`, `..`, a leading dot and names containing a null byte are rejected. `resolveFilename()`, the protected naming seam, derives the name; `upload()` decides whether it may be written, so **an override of the seam cannot drop any of these refusals**. The leading dot mattered most: the deny-list carries `htaccess`, but a dotfile presents no extension for it to match, so `.htaccess` reached the upload directory +* **Storage refuses a destination that is not a plain filename.** The name is reduced to a `basename()`, and `''`, `.`, `..`, a leading dot and names containing a null byte are rejected. `resolveFilename()`, the protected naming seam, derives the name; `upload()` decides whether it may be written, so **an override of the seam cannot drop any of these refusals**. The shipped `FileInfo` never hands storage one of these names, in 3.x any more than now: `../escape.txt` came out as `escape.txt` and `.htaccess` as `unnamed-file.htaccess`, which the deny-list refuses in turn. A custom implementation had nothing behind it in 3.x — one returning `../escape.txt` wrote outside the upload directory, and one returning `.htaccess` wrote it literally. The leading dot is refused here rather than by the deny-list, which is a list of extensions and does not cover `.env` * **`FileSystem::upload()` refuses a destination that is a symbolic link** * **Uploads are staged.** The file is written to a temporary name in the destination directory and moved onto the destination in one operation, which doesn't follow a symlink standing there, so no partial content is readable under the final name * **The overwrite guard no longer has a check-then-create window.** With `overwrite = false` the existence check and the create are one exclusive operation, so two concurrent requests can no longer both claim the same name, and a placeholder left by a failed move is cleaned up. On POSIX the file created is then verified by inode to be the destination rather than a symlink's target; Windows before PHP 7.4 reports no inode, so that verification does not hold there. A process killed mid-transfer leaves a 0-byte file * **A failed `chmod()` aborts the upload** rather than storing the file at the umask's mode while reporting `0640` * **`Validation\FileType::allow()` rejects empty and whitespace-only values.** An empty media type is what `getMimetype()` returns for a file it can't read, and it would have matched one. A lone `.` is rejected as an extension; a leading dot is accepted and removed -* **Every string `getErrors()` returns is sanitized, message as well as filename.** Raw `$_FILES[…]['name']` reached it from the constructor, a name from a custom `FileInfoInterface` from `isValid()`, and a validation failure's message was reported exactly as thrown, so a line break, terminal escape or bidi override could land in a string the README tells you to render. Both halves now go through `Filename::sanitizeForDisplay()`: bidi controls deleted, control characters collapsed to a space, and the result forced to valid UTF-8 where `ext-mbstring` is loaded, so `json_encode(getErrors())` cannot return `false` on one bad byte. No shipped validator puts anything but configuration into a message, so that half is for a validator of your own. The shipped `FileInfo` was unaffected, and sanitizing is still not escaping. Every append goes through one `protected` recorder, `File::recordError()`, so a `File` subclass recording an error of its own is covered too +* **Every string `getErrors()` returns is sanitized, message as well as filename.** Raw `$_FILES[…]['name']` reached it from the constructor, a name from a custom `FileInfoInterface` from `isValid()`, and a validation failure's message was reported exactly as thrown, so a line break, terminal escape or bidi override could land in a string the README tells you to render. Both halves now go through `Filename::sanitizeForDisplay()`: bidi controls deleted, control characters collapsed to a space, and the result forced to valid UTF-8 where `ext-mbstring` is loaded, so `json_encode(getErrors())` cannot return `false` on one bad byte. No shipped validator puts anything but configuration into a message, so that half is for a validator of your own. The shipped `FileInfo` was unaffected, and sanitizing is still not escaping. A string is also cut to `Filename::MAX_DISPLAY_LENGTH` (2048 bytes), which is longer than any message this library composes and short enough that a validator of your own cannot flood whatever renders the list. Every append goes through one `protected` recorder, `File::recordError()`, so a `File` subclass recording an error of its own is covered too * **`getHash()`, `getMimetype()`, `getSize()` and `getDimensions()` handle a missing file**, returning `''`/`false`/`0` where `getSize()` raised a `RuntimeException`, `getHash()` a `TypeError`, and the other two emitted warnings * **A lifecycle callback can't call back into its own `File`.** `isValid()`, `upload()` and `uploadValid()` throw `\LogicException` when one is already running on that object. Each run resets the error list at its start, and `upload()` and `uploadValid()` reset the locator list too, so a nested call could send a failed file to storage and lose the record of what was already written * **A validator throwing something other than `Upload\Exception` no longer aborts the batch.** It records `Validation could not be completed` and nothing else: not the message, which can contain server paths, and not the class name. `\LogicException` is re-thrown, since PHP defines that type as a bug in the program diff --git a/CLAUDE.md b/CLAUDE.md index e08604d..2cb5e3e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,7 +6,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co `gravitypdf/upload` — a standalone PHP library for validating and storing `$_FILES` uploads. It is a maintained fork of the abandoned `codeguy/upload` (declared via `"replace"` in composer.json), renamed to the `GravityPdf\Upload` namespace. -Supports PHP 7.3 through 8.5. Any change must remain syntax- and behaviour-compatible across that whole range — CI runs the test suite on all eight versions. The only required runtime dependency is `ext-fileinfo`. `ext-mbstring` is under `suggest`: without it, filename truncation can split a multibyte character and the valid-UTF-8 guarantee does not hold. All three configurations `composer.json` permits are covered by CI — the extension, `symfony/polyfill-mbstring`, and neither — because nothing else sees the difference: an unguarded `mb_*` call once turned a rejected upload into a fatal error on an install without the extension. `forceValidUtf8()` owns that guard now, so it travels with the function rather than with a caller. The polyfill does **not** ship `mb_strcut()`, which is why `finalize()` gates only its truncation on that and calls `forceValidUtf8()` unconditionally; it is also `iconv()`-backed and warns on the input the repair exists for, so that call is silenced with `@`. +Supports PHP 7.3 through 8.5. Any change must remain syntax- and behaviour-compatible across that whole range — CI runs the test suite on all eight versions. The only required runtime dependency is `ext-fileinfo`. `ext-mbstring` is under `suggest`: without it, filename truncation can split a multibyte character and the valid-UTF-8 guarantee does not hold. All three configurations `composer.json` permits are covered by CI — the extension, `symfony/polyfill-mbstring`, and neither — because nothing else sees the difference: an unguarded `mb_*` call once turned a rejected upload into a fatal error on an install without the extension. `forceValidUtf8()` owns that guard now, so it travels with the function rather than with a caller. The polyfill does **not** ship `mb_strcut()`, which is why `finalize()` gates only its truncation on that and calls `forceValidUtf8()` unconditionally; it is also `iconv()`-backed and warns on the input the repair exists for, so that call is silenced with `@`. `iconv()` answers `false` for a sequence the end of the string cuts short, where the extension drops it and keeps the rest, so `forceValidUtf8()` takes the partial tail off before it converts. Without that, a client name ending mid-character came back as `unnamed-file` on a polyfilled install. ## Commands @@ -141,6 +141,11 @@ found separately. `MAX_LENGTH`, `MAX_EXTENSION_LENGTH`, `CONTROL_CHARACTERS`, `B (`deviceComponent()`, `extensionComponents()`, `normalizeComponents()`) rather than splitting for themselves. Add a rule there, not in a caller. +`extensionComponents()` splits on `.` and drops the first field **before** it normalizes. +Normalizing first drops a component that is nothing but spaces, leaving the extension as the +only field for the drop to take: `" .php"` normalized to `['php']`, the deny-list was handed +`[]`, and the file was written. + `Filename::sanitizeForDisplay()` is the same rules applied to prose rather than to a name — the two character sets, with controls collapsed to a space instead of `-` and none of the filename budget, device-name or `%`/`/` handling. `File` runs a validator's message through it and @@ -178,7 +183,7 @@ With `overwrite = false`, `reserveDestination()` claims the name first with an e `resolveFilename()` reduces the name to a `basename()`, strips trailing dots and spaces (Windows resolves `evil.php.` to `evil.php`), and rewrites `<>:"|?*` to `-` — rewritten, not refused, because POSIX allows all of them, and `:` would otherwise name an NTFS alternate data stream. That is all it does: it decides what the name **is**, and `upload()` decides whether it may be written. -**No refusal is in there.** `resolveFilename()` is protected — the seam for changing how names are chosen — and while a refusal sat inside it, an override that said nothing about the subject dropped it. `upload()` runs all three against whatever the seam returns, and all three are `private`: `refuseUnsafeName()` (`''`, a leading `.`, `Filename::CONTROL_CHARACTERS`, `Filename::BIDI_CONTROLS`), `refuseReservedWindowsName()` and `refuseBlockedExtensions()`. The leading `.` is in the first of those rather than left to the deny-list because the deny-list cannot see one: `extensionComponents()` drops the empty component before the dot along with the first real one, so `.htaccess` presents nothing to match and `.env` is not on the list at all. A refusal rather than a rewrite throughout — inventing a filename is the value object's job, not storage's. **Do not move any of them back into the seam**, and do not answer "a subclass can reproduce it": `return basename($fileInfo->getNameWithExtension())` is the override people actually write. The collision message in `reserveDestination()` treats the seam's output the same way, running the basename through `Filename::sanitizeForDisplay()` before quoting it — the base `resolveFilename()` refuses control and bidi characters, an override need not, and a storage message is written to a log. A name left with nothing by that says `'A file with that name already exists'` rather than quoting an empty string. A blocked extension is refused in **any** dot-separated component, because a web server does not necessarily treat the last component as the extension. Do not narrow that to one component, and do not move the two refusals back. +**No refusal is in there.** `resolveFilename()` is protected — the seam for changing how names are chosen — and while a refusal sat inside it, an override that said nothing about the subject dropped it. `upload()` runs all three against whatever the seam returns, and all three are `private`: `refuseUnsafeName()` (`''`, a leading `.`, `Filename::CONTROL_CHARACTERS`, `Filename::BIDI_CONTROLS`), `refuseReservedWindowsName()` and `refuseBlockedExtensions()`. The leading `.` is in the first of those rather than left to the deny-list, which is a list of extensions and does not cover `.env` at all. A refusal rather than a rewrite throughout — inventing a filename is the value object's job, not storage's. **Do not move any of them back into the seam**, and do not answer "a subclass can reproduce it": `return basename($fileInfo->getNameWithExtension())` is the override people actually write. The collision message in `reserveDestination()` treats the seam's output the same way, running the basename through `Filename::sanitizeForDisplay()` before quoting it — the base `resolveFilename()` refuses control and bidi characters, an override need not, and a storage message is written to a log. A name left with nothing by that says `'A file with that name already exists'` rather than quoting an empty string. A blocked extension is refused in **any** dot-separated component, because a web server does not necessarily treat the last component as the extension. Do not narrow that to one component, and do not move the two refusals back. **`move_uploaded_file()` is the storage half of the provenance decision.** It refuses any source PHP did not receive as an upload, so `Storage\FileSystem` cannot store a `FileList` file — the class validates and then fails at the write — until `acceptFilesNotUploadedByPhp()` says otherwise. That is deliberate, and the pairing is the point: `FileInfo::isUploadedFile()` says where the file came from, this says the caller is willing to store it, and neither is a default. Do not make the fallback automatic on a failed `move_uploaded_file()` — a path an attacker steered would take exactly that branch, which is the whole attack the SAPI check exists to stop. With the opt-in on, `moveFile()` renames, with a copy behind it. The copy is **not** the cross-file-system path, whatever the PSR-7 adapter plan says: `rename(2)` does fail with `EXDEV` between a tmpfs tmp directory and a disk upload directory, but PHP's plain-files wrapper catches that itself and copies, so `rename()` returns `true` — verified against two real mounts, and pinned by a test that skips when it cannot find a second file system. The fallback covers what PHP does not absorb, a stream-wrapper source among it. Both send the bytes at the **staging** path rather than the destination, so the copy's symlink-following is closed off by the unguessable name the same way `move_uploaded_file()`'s own copy fallback is. `moveFile()` is `protected` for the same testability reason `moveUploadedFile()` is; `moveIntoStaging()` picks between them and is `private`. diff --git a/README.md b/README.md index 09da61e..1c1d0d6 100644 --- a/README.md +++ b/README.md @@ -511,8 +511,9 @@ translated at the throw rather than at the render, and `Exception::getMessage()` the English you search your log for. `xgettext` extracts either form. Your message goes through `Filename::sanitizeForDisplay()` first: bidi controls are deleted, -runs of control characters collapse to a single space, surrounding whitespace is trimmed, and -the result is forced to valid UTF-8 where `ext-mbstring` is loaded. A message built from user +runs of control characters collapse to a single space, the line is cut to +`Filename::MAX_DISPLAY_LENGTH`, surrounding whitespace is trimmed, and the result is forced to +valid UTF-8 where `ext-mbstring` is loaded. A message built from user input cannot forge a log line, move a terminal cursor, or make `json_encode($file->getErrors())` return `false`. It is still not escaped, so escape on output. @@ -732,14 +733,17 @@ your own. |---|---| | PHP | `php` `php2` `php3` `php4` `php5` `php6` `php7` `php8` `phps` `phtml` `phtm` `phar` `pht` `inc` | | Server-side includes | `shtml` `shtm` `stm` | -| CGI and scripts | `cgi` `fcgi` `pl` `py` `rb` `sh` `bash` `ps1` | +| Apache mod_asis | `asis` | +| CGI and scripts | `cgi` `fcgi` `pl` `py` `rb` `sh` `bash` `ps1` `erb` `rhtml` | | Java | `jsp` `jspx` `jspf` `jsw` `jsv` `jshtml` `jar` `war` | | ASP / ASP.NET | `asp` `aspx` `asa` `asax` `ascx` `ashx` `asmx` `cer` `cshtml` `vbhtml` | +| Legacy IIS | `htr` `idc` `printer` | +| ColdFusion | `cfm` `cfml` `cfc` | | Windows binaries | `exe` `dll` `com` `bat` `cmd` `msi` `scr` `vbs` `ws` `wsf` `hta` | | Server configuration | `htaccess` `htpasswd` `ini` `conf` `config` | | Markup and script | `html` `htm` `xhtml` `xht` `xhtm` `svg` `svgz` `xml` `xsl` `xslt` `js` `mjs` `swf` `mht` `mhtml` | -The first seven groups are `FileSystem::EXECUTABLE_EXTENSIONS`, which a server runs. The +Every group but the last is `FileSystem::EXECUTABLE_EXTENSIONS`, which a server runs. The last is `FileSystem::MARKUP_EXTENSIONS`, which a browser renders. Serving one from your own origin is stored XSS; SVG is in that group because it carries `