xperseguers / t3ext-image_autoresize

TYPO3 Extension image_autoresize. Simplify the way your editors may upload their images.
https://extensions.typo3.org/extension/image_autoresize
GNU General Public License v3.0
16 stars 22 forks source link

File /phpaU5H8j.jpg does not exist since TYPO3 security update #99

Closed sfroemkenjw closed 7 months ago

sfroemkenjw commented 8 months ago

Hello,

since last TYPO3 Update 13.02.2024 we can not upload files with a filesize > 2MB anymore. It will result in File /phpaU5H8j.jpg does not exist. You are creating a virtual ResourceStorage, Driver and Storage Environment to allow processing of files which still resides in /tmp or /temp. Since the Security Update the method call $this->isAllowedAbsolutePath($absoluteBasePath) in calculateBasePath fails, because these temp directories are not allowed anymore. Sure, as a quick fix we as community can temporary set ['BE']['lockRootPath'] = '/', but this will give root access again. So, it's up to you to find a better solution for that.

Nice greetings

Stefan

Patta commented 8 months ago

I can not confirm that in TYPO3 11.5.35 with image_autoresize 2.3.0. Resizing still works.

josefglatz commented 8 months ago

We can also confirm this bug at least in TYPO3 12.

@sfroemkenjw The thrown exception contains the absolute path. Here's an example of Hetzner Managed webspace and TYPO3 12:

The mentioned error message:

Failed initializing storage [0] "Temporary Upload Storage", error: Base path "/usr/home/webspace/.tmp/" is not within the allowed project root path or allowed lockRootPath.

According to the security advisory https://typo3.org/security/advisory/typo3-core-sa-2024-001 the configuration option is now an array and can be therefore configured like

$GLOBALS['TYPO3_CONF_VARS']['BE']['lockRootPath'] = [
  ‘/usr/home/webspace/.tmp/’,
];

I think it's not necessary therefore to say, that you can temporary set the path to the root directory. Just add your allowed paths like the reported one.

SvenJuergens commented 8 months ago

strange, I have also tested it in the backend of an 11.5.36 installation and everything seems to be fine. The process runs successfully and the images are uploaded. However, I can see the entries mentioned in the error logs.

seirerman commented 8 months ago

Can confirm on TYPO3 11.5.36 with image_autoresize 2.3.0, but it's not "filesize > 2MB". It's what i set as threshold value in the image_autoresize settings. Uploading files without image_autoresize installed works for all files.

If I set the threshold to 100MB I can upload almost any image file, but that would negate the purpose of this extension. It's a workaround until this is eventually fixed, though.

xperseguers commented 8 months ago

I just tried to reproduce in TYPO3 v12.4.11 and master branch of image_autoresize but don't know what to do...

I used a threshold of 2MB for a "Presse" directory and could successfully upload and get resized an image that weighted 2.6MB and around 250 KB afterwards.

xperseguers commented 7 months ago

@sfroemkenjw Would help if you could provide class references, FQDN, line numbers, ... when you do such debugging as you seem to have debugged quite deeply. Would prevent me from having to figure everything out again...

xperseguers commented 7 months ago

since last TYPO3 Update 13.02.2024 we can not upload files with a filesize > 2MB anymore. It will result in File /phpaU5H8j.jpg does not exist. You are creating a virtual ResourceStorage, Driver and Storage Environment to allow processing of files which still resides in /tmp or /temp. Since the Security Update the method call $this->isAllowedAbsolutePath($absoluteBasePath) in calculateBasePath fails, because these temp directories are not allowed

=> the call to \TYPO3\CMS\Core\Resource\Driver\LocalDriver::isAllowedAbsolutePath() in \TYPO3\CMS\Core\Resource\Driver\LocalDriver::calculateBasePath()

xperseguers commented 7 months ago

somehow I can now reproduce in TYPO3 v11

xperseguers commented 7 months ago

@ all Please test PR #103

webian commented 7 months ago

Hi @xperseguers, I tested PR https://github.com/xperseguers/t3ext-image_autoresize/pull/103 with TYPO3 11.5.37-dev and it works, thanks!

I would just like to raise a doubt: could the change of adding '/tmp/' to lockRootPath lead to security issues that TYPO3-CORE-SA-2024-001 is trying to address?

It's just a doubt I have and I honestly have a hard time imagining what problem there could be.

xperseguers commented 7 months ago

Hi @webian since it’s not added continuously but only during the context of resizing the image, we’re in a quite “safe” context

webian commented 7 months ago

You're right @xperseguers. Could it be a useful improvement to the safe context to unset the '/tmp/' from lockRootPath after the resize?

froemken commented 7 months ago

Hello @xperseguers

Sorry, it was my last day and last few minutes before holiday last week.

This is the line where the problem starts. So this is a good place to start debugging: https://github.com/xperseguers/t3ext-image_autoresize/blob/2.3.0/Classes/Service/ImageResizer.php#L417

You have wrapped it around a try-catch block. Perfect. But if path is not allowed, nothing really happens. The upload is not stopped, the process still tries to resize the images, and no log entry is made. So in the next method calls the destination file [pathToTmp]/[phpTmpFile] was shortened to just /[phpTmpFile].

Changing the temporary backLockPath should not be the solution.

I don't think accessing files in PhpTmpDir using the FAL API is a good idea in general. I prefer to use GU::upload_to_tempfile() first and then try to get the metadata. In this case there is no need to set backLockPath temporarily and maybe all your virtual storages are not needed anymore. Welcome back to the TYPO3 API and the FAL API. That would be great.

Have a nice weekend.

Stefan

xperseguers commented 7 months ago

Using GU::upload_to_tempfile() sounds odd... this happens during the normal upload workflow.

Right now we use the file to try to extract metadata prior to a destructive resizing process. So what you suggest is to copy that file to some other location within the TYPO3 project, to make FAL happy without having to "touch anything", extract stuff and then take care to delete the file again? And just keep the file in temporary folder which exists for real to be taken into account to further let the upload workflow take place? That sounds pretty inefficient :/

xperseguers commented 7 months ago

You're right @xperseguers. Could it be a useful improvement to the safe context to unset the '/tmp/' from lockRootPath after the resize?

That's possibly an option I could consider, we should just make sure the check only happens when instantiating the virtual folder object and not anymore at a later stage, then it would be easy to do so.