BUGFIX: Sanitize uploaded svg files from suspicious content

[mficzel]

Adding an internal methods isSanitizingRequired and sanitizeImportedFileContent to the resourceManager. The import is adjusted to first determine the mediaType of an imported resource to decide wether sanitizing is needed which for now happens only for SVG files. If no sanitizing is needed the code will perform as before by passing streams or filenames around.

If suspicious content was removed from a warning is logged that mentions the remove data and line. The sanitizing is done using "enshrined/svg-sanitize" that is used by other cms aswell.

The initial implementation will only sanitize SVG files as those can contain malicious scripts. In future this should be expanded to a feature that allows registering of custom sanitizing functions.

The sanitizing logic itself ist basically the same as what is done by typo3 here: https://github.com/TYPO3/typo3/blob/357b07064cf2c7f1735cfb8f73ac4a7248ab040e/typo3/sysext/core/Classes/Resource/Security/SvgSanitizer.php

This addresses the issue described here: https://nvd.nist.gov/vuln/detail/CVE-2023-37611

Review Instructions

The change adds quite a bit of complexity to the importResource method to avoid loading the file content into ram whenever possible. As this method accepts filenames and resources this leads to quite some nested checking. I consider this kindoff necessary as one does not want to read a full video file into php ram to check wether it may be an svg.

Better suggestions are welcome.

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• The PR is created against the lowest maintained branch
• Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
• Reviewer - The first section explains the change briefly for change-logs
• Reviewer - Breaking Changes are marked wit !!! and have upgrade-instructions

[mficzel]

I would rather see this in the resource manager but since the codePath uses $resourceManager->importResource i am not sure it would be wise to be placed there as this would check every resource even those created internally like image crops.

I assume it somehow more correct to validate the uploaded files then the importedResources.

Also i would like to show a nicer error message. Did not manage that yet as Errors or exceptions do not end up in the Media module.

[mficzel]

It should be discussed wether this is actually expected behavior as there are possible valid use cases for scripts inside of svgs as animations like https://www.svgator.com/ i am aware of a former customer that actually used this.

[kdambekalns]

LGTM… 👍 by 👀

[kdambekalns]

We should also point out that people can use CSP to block such attacks. Back then I noted this down:

As far as protecting against this goes, my first thought was "we can't do anything, those files are opened directly." Which might be not wrong, but we can probably do something. At least point people to the need to configure CSP headers to be sent with SVG? That's one thing mentioned in https://digi.ninja/blog/svg_xss.php that could help.

[dlubitz]

I wonder, how does this impact cloud storages like S3?

[mficzel]

I wonder, how does this impact cloud storages like S3?

I do not think that cdns are affected in a special way especially since this is in the resourceManager and not the storage. The side effect of loading the whole file is probably the major one to consider.

The review was older than the refactoring

[mficzel]

@dlubitz removed your approval and re-requested review as the approval was from before the refactoring and you still commented valid points. Was nut sure it was intended that the review was still in place and wanted to prevent a merge wrong assumptions.

[mficzel]

Adjusted the mentioned points

[kdambekalns]

It should be discussed wether this is actually expected behavior as there are possible valid use cases for scripts inside of svgs as animations like https://www.svgator.com/ i am aware of a former customer that actually used this.

That could indeed have side effects. If this had a switch, people could disable sanitation when they apply some other means of blocking such attacks (e.g. a CSP…)

[kdambekalns]

Looks good by careful 👀, good to go. See my last comment on making this optional, though.

[mficzel]

@kdambekalns in my tests the animations survived to my surprise. Consider this safe for now. Also i want to avoid injecting features in a bugfix release. We can still add an opt in flag once we are notified of negative effects.

[kitsunet]

Thanks for taking care, aaaaand I am sorry that I come in so late, I won't block this, but I find sanitizer the wrong approach. IMHO either we straight up block SVG uploads with scripts (which would still mean looking into them :/) or we disable SVGs altogther by default and document at the point where you enable them that you must take care of sending appropriate CSP to prevent inline scripts or include the SVGs in a safe way according to the article on how to make this safe.
I feel sanitizing is just a kind of arms race against more cleverly disguised scripts.

[mficzel]

@kitsunet i first tried to block the upload by throwing an exception. The same library can also tell you wether an svg smells fishy. Problem was that the media utility did not show an understandable error message when an upload failed.

In the end sanitizing the svg was almost the same code and did not require any user interaction or changes in other parts than resource management.

Blocking SVG alltogether seems no viable to me at all. This is a pretty common image format nowadays.

[mficzel]

AFK for the next days. Feed free to adjust / merge this as you like.

@kitsunet if you find a way to show a meaningful error message in the media module by throwing an Exception in https://github.com/neos/flow-development-collection/pull/3172/files#diff-189d35abcc40be6c39fd0d0e15c088658da7c96ee3985b4e2afb6061dd6f3aa9R662 i would appreciate that and gladly abandon the idea of silent sanitizing. Maybe I just did not find the correct exception type. Media module always complained about http errors or something else not suitable for editors

[kitsunet]

I don't think there is an easy way without messing with the whole module and JS, so not blocking, lets do this, better this than nothing.

[kdambekalns]

I'll merge this. For the "reject instead of sanitize" I created an issue, but this is better than the current state, arguably.

[kdambekalns]

What, wait? How can an unrelated Redis test suddenly fail, and only on PHP8?

[kdambekalns]

What, wait? How can an unrelated Redis test suddenly fail, and only on PHP8?

#3194

[mficzel]

Rebase or just merge this after the redis issue was solved?

[kdambekalns]

Rebase or just merge this after the redis issue was solved?

LGTM 😎

[kdambekalns]

Dang, that would have been nice for the releases I just tagged… 🤪

[mficzel]

Just to be sure ... this was the redis issue not this pr?