silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 65 forks source link

Can't upload images with two dots (`..`) in the name #599

Closed GuySartorelli closed 1 month ago

GuySartorelli commented 2 months ago

Originally mentioned in this slack message

Upgrading from Framework 5.1 to Framework 5.2 has some of my projects broken if there are assets with ... in their name. Originally, I thought it was AKQA's responsive images module, because if I remove that, it doesn't break. Turns out that Jono's focuspoint module "unbreaks" things. Which, isn't ideal.

I mean, having dots in the filename shouldn't, in my opinion, break an entire website. It should if it's actual directory traversal, of course. But my-image-test...-it-breaks.png should not break, as it's a valid filename

After some poking around, it turns out this happens if you upload an image (yes, specifically an image) with .. in the name.

The error you get is:

ERROR [Emergency]: Uncaught InvalidArgumentException: Can not collapse relative folders
IN POST /admin/assets/api/createFile
Line 38 in /var/www/html/vendor/silverstripe/framework/src/Core/Path.php
Click to see stack trace ```text Source ====== 29: $parts = $parts[0]; 30: } 31: 32: // Cleanup and join all parts 33: $parts = array_filter(array_map('trim', array_filter($parts ?? []))); 34: $fullPath = static::normalise(implode(DIRECTORY_SEPARATOR, $parts)); 35: 36: // Protect against directory traversal vulnerability (OTG-AUTHZ-001) 37: if (strpos($fullPath ?? '', '..') !== false) { * 38: throw new InvalidArgumentException('Can not collapse relative folders'); 39: } 40: 41: // Protect against directory traversal vulnerability (OTG-AUTHZ-001) 42: if ($fullPath === '..' || str_ends_with($fullPath, '/..') || str_contains($fullPath, '../')) { 43: throw new InvalidArgumentException('Can not collapse relative folders'); 44: } Trace ===== SilverStripe\Core\Path::join(, cms-logo-with-v2...words) AbstractFileIDHelper.php:100 SilverStripe\Assets\FilenameParsing\AbstractFileIDHelper->swapExtension(cms-logo-with-v2...words.png, FitMaxWzYwLDYwXQ, 1) AbstractFileIDHelper.php:44 SilverStripe\Assets\FilenameParsing\AbstractFileIDHelper->buildFileID(cms-logo-with-v2...words.png, 7e67f6a97a6ad07c5cbd8a23311ea35e990d58c0, FitMaxWzYwLDYwXQ) FileIDHelperResolutionStrategy.php:397 SilverStripe\Assets\FilenameParsing\FileIDHelperResolutionStrategy->buildFileID(SilverStripe\Assets\FilenameParsing\ParsedFileID) FlysystemAssetStore.php:275 SilverStripe\Assets\Flysystem\FlysystemAssetStore->applyToFileOnFilesystem(Closure, SilverStripe\Assets\FilenameParsing\ParsedFileID) FlysystemAssetStore.php:1133 SilverStripe\Assets\Flysystem\FlysystemAssetStore->exists(cms-logo-with-v2...words.png, 7e67f6a97a6ad07c5cbd8a23311ea35e990d58c0, FitMaxWzYwLDYwXQ) ImageManipulation.php:956 SilverStripe\Assets\File->manipulate(FitMaxWzYwLDYwXQ, Closure) ImageManipulation.php:917 SilverStripe\Assets\File->manipulateImage(FitMaxWzYwLDYwXQ, Closure) ImageManipulation.php:414 SilverStripe\Assets\File->FitMax(60, 60) ThumbnailGenerator.php:112 SilverStripe\AssetAdmin\Model\ThumbnailGenerator->generateThumbnail(SilverStripe\Assets\Image, 60, 60) AssetAdmin.php:1181 SilverStripe\AssetAdmin\Controller\AssetAdmin->generateThumbnails(SilverStripe\Assets\Image, ) AssetAdmin.php:1137 SilverStripe\AssetAdmin\Controller\AssetAdmin->getMinimalistObjectFromData(SilverStripe\Assets\Image, ) AssetAdmin.php:1072 SilverStripe\AssetAdmin\Controller\AssetAdmin->getObjectFromData(SilverStripe\Assets\Image, ) AssetAdmin.php:338 SilverStripe\AssetAdmin\Controller\AssetAdmin->apiCreateFile(SilverStripe\Control\HTTPRequest) RequestHandler.php:321 SilverStripe\Control\RequestHandler->handleAction(SilverStripe\Control\HTTPRequest, apiCreateFile) Controller.php:274 SilverStripe\Control\Controller->handleAction(SilverStripe\Control\HTTPRequest, apiCreateFile) RequestHandler.php:200 SilverStripe\Control\RequestHandler->handleRequest(SilverStripe\Control\HTTPRequest) Controller.php:200 SilverStripe\Control\Controller->handleRequest(SilverStripe\Control\HTTPRequest) LeftAndMain.php:839 SilverStripe\Admin\LeftAndMain->handleRequest(SilverStripe\Control\HTTPRequest) AdminRootController.php:124 SilverStripe\Admin\AdminRootController->handleRequest(SilverStripe\Control\HTTPRequest) Director.php:348 ```

To my eye that strpos check in Path::join() is overly aggressive.

PRs

GuySartorelli commented 1 month ago

A git bisect shows a40ab5085a7bf2d46f33effa950814e58a014523 as the first commit that surfaces this bug - and that makes sense, since the swapExtension() method is introduced in that commit and calls Path::join().

That's just surfacing the bug though. I'm pretty confident the root cause is an overly aggressive check in Path::join() rather than anything in the assets module itself.

GuySartorelli commented 1 month ago

Closed as a duplicate of https://github.com/silverstripe/silverstripe-framework/issues/11216