Disallow dangerous filenames e.g. command injection characters

I'd recommend that we enforce the removal (/ substitution) of the most dangerous characters.

For removal of spaces, that'd be ok as a default (would people really want to turn it off?!)

@kim.pepper although the existing options would remove all of the characters I highlighted as a concern, they do more than that.

I wouldn't advocate stripping filenames down to only ASCII / alpha-numeric characters (by default or as a mandatory setting) because of how restrictive that is for different languages.

I'm proposing a fairly limited number of forbidden characters - those which have significance for the shell - which would be a subset of what would be stripped by any of the existing options IIUC?

Having said that, I went to look for a definitive list of "dangerous characters" and ended up with quite a lot of "it depends".

This is not definitive, but e.g. https://stackoverflow.com/questions/15783701/which-characters-need-to-be...

There are some quite good lists using e.g. the shell escaping option that printf (and apparently ls) offer - e.g.

No, character % does not need to be escaped
No, character + does not need to be escaped
No, character - does not need to be escaped
No, character . does not need to be escaped
No, character / does not need to be escaped
No, character : does not need to be escaped
No, character = does not need to be escaped
No, character @ does not need to be escaped
No, character _ does not need to be escaped
Yes, character   needs to be escaped
Yes, character ! needs to be escaped
Yes, character " needs to be escaped
Yes, character # needs to be escaped
Yes, character $ needs to be escaped
Yes, character & needs to be escaped
Yes, character ' needs to be escaped
Yes, character ( needs to be escaped
Yes, character ) needs to be escaped
Yes, character * needs to be escaped
Yes, character , needs to be escaped
Yes, character ; needs to be escaped
Yes, character < needs to be escaped
Yes, character > needs to be escaped
Yes, character ? needs to be escaped
Yes, character [ needs to be escaped
Yes, character \ needs to be escaped
Yes, character ] needs to be escaped
Yes, character ^ needs to be escaped
Yes, character ` needs to be escaped
Yes, character { needs to be escaped
Yes, character | needs to be escaped
Yes, character } needs to be escaped

I feel like most of those make sense; and there are a fair number of characters that would be allowed.

Whether this would be more or less confusing / user friendly than just stripping all punctuation though is perhaps a usability question.

Looks like the code mostly uses regex; I wonder whether the punct character class would be any use. It'd remove the "safe" characters from the list above which we may not want to do.

To be clear what I'm looking for here is something that would remove all the dangerous shell characters but not limit users to strict ASCII alphanum without e.g. accented letters etc..

FWIW by default Wordpress removes a load of "special" characters from filenames:

https://github.com/WordPress/wordpress-develop/blob/6.7.2/src/wp-include...

function sanitize_file_name( $filename ) {

...

	$special_chars = array( '?', '[', ']', '/', '\\', '=', '<', '>', ':', ';', ',', "'", '"', '&', '$', '#', '*', '(', ')', '|', '~', '`', '!', '{', '}', '%', '+', '’', '«', '»', '”', '“', chr( 0 ) );

...

	$filename = str_replace( $special_chars, '', $filename );

Joomla seems to have decided to let the underlying OS decide what it will and will not allow in filenames :shrug:

https://github.com/joomla/joomla-cms/issues/33214

Thanks, that's a good start. Looks like you've borrowed the list of chars to strip from wordpress. Perhaps we could remove a few of the "safe" ones based on the list in #7; we're only talking about a few e.g. % + = :.

FWIW here's how typo3 does this:

https://github.com/TYPO3-CMS/core/blob/v13.4.9/Classes/Resource/Driver/L...

The comments suggest that it's pretty strict:

... any character not matching [.a-zA-Z0-9_-] is substituted by '_'

...but in actual fact it's a bit more nuanced as there are some settings around allowing / remapping utf8 characters.

I'll try to do some work on this but am short on time just now.

sorry, didn't mean to change status

Thanks - I'm going to tag this for a usability review (per https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett...) as I agree that it's likely stripping spaces from filenames on uploads may not be universally welcomed.

It would, however, have benefits in terms of Security Hardening (it's very hard to achieve Command Injection without any spaces). (Acknowledging the comment in #20 that if a command injection vulnerability of similar exists, this is not the only fix that's needed.)

There are also other benefits like making it easier to iterate over batches of files in the shell etc.. but that's probably out of scope here.

I personally loathe spaces in filenames but I am probably not representative of a broad cross section of users.

So far I think there's some consensus that stripping "special characters" from filenames likely provides enough benefit (in terms of security) to outweigh annoyance some users may feel at having e.g. punctuation marks removed from their filenames, but stripping spaces may be seen as a step too far.

It could be a default that site owners could disable in the file handling options. I'd argue to make the special character stripping mandatory, but that's also open to debate.

Previous comments include some light research on how other CMSs/Frameworks do this. Wordpress and Typo3 both strip (or substitute) most punctuation by default, and it looks like they both swap out spaces for either an underscore or dash AFAICS.

Drupal already has an option to replace whitespace in filenames with _ or - but it's not enabled by default. I'd like to turn that on by default as part of this issue.

What does the UX-Team think about having mandatory stripping of special chars from filenames, plus enabling swapping spaces out for - or _ by default?

(I've not yet updated the IS to eliminate any proposed options as I don't think we have eliminated any yet.. agree we should do that when we get to that point though.)

I've posted some more details about my motivation for pursuing this change in a blog post about a recent Security Advisory:

https://www.mcdruid.co.uk/article/hacking-ai-module-drupal-cms

Hopefully the extra context this provides outweighs any whiff of self-promotion :)

To summarise the proposal briefly, the change would be:

• Filenames always have dangerous characters stripped (or replaced with an inert character like -). In practice this means most but not all punctuation (e.g. see list in #8).
• Filenames have spaces replaced by -. This is existing optional functionality which would be enabled by default.
• The replacement character used in both cases could be configurable (again, that functionality already exists).

This would be pretty similar to how filenames are sanitised by both WordPress and Typo3.

Thank you to the Usability Team who reviewed and discussed this issue in their meeting #3526141: Drupal Usability Meeting 2025-05-30. I believe it's still on the todo list to comment here, but the meeting recording and transcript are linked to from that issue so we can glean details of their review in the meantime.

To attempt to summarise - the question was:

What does the UX-Team think about having mandatory stripping of special chars from filenames, plus enabling swapping spaces out for - or _ by default?

...and the answer:

benji: okay, there's a rather long list of special characters that are hard coded.

benji: ... You can't remove anything from that list. It's it's hard coded. The only thing you can change in configuration is whether to replace spaces.

benji: ... the main usability question is whether to allow people to override it.

benji: ... I think I agree that we should

benji: use the more secure default. So by default, replace spaces.

benji: Let let people override. And Ralph agrees.

benji: So we're unanimous on that.

Ralf Koller: [T]hat looks like a reasonable change, definitely.

I'll ask @benjifisher to correct me if that's not quite accurate.

Another important issue that was brought up was whether we're proposing to retrospectively tackle existing files.

I think that's a "no"; the likelihood of unintended consequences is really high if we tried to rename existing files, and we should limit the scope to future uploads only. (This is what Benji and the team correctly assumed).

So let's proceed on the basis that we want to:

• Introduce mandatory replacement / removal of "dangerous characters", and..
• Enable replacement of spaces in filenames by default, but allow this to be overridden.

Benji mentioned in the UX meeting that any overrides may have to happen only in settings.php as otherwise an attacker can potentially make those changes themselves (e.g. via XSS in the UI).

That's definitely a good point, but I think in the case of allowing spaces in filenames I'd be comfortable allowing that change to made in the UI if the dangerous characters are always removed with no way of overriding that behaviour.

Updates to IS now that we've agreed a proposed solution.

Thanks for picking this back up.

I'm not yet sure exactly why this is happening, but looks like the problem's in \Drupal\file\EventSubscriber\FileEventSubscriber::sanitizeFilename

...specifically:

    if ($fileSettings->get('filename_sanitization.replace_whitespace')) {
      $filename = preg_replace('/\s/u', $replacement, trim($filename));
    }

When the filename containing the invalid unicode goes through that regex, it seems to end up set to null.

We can reproduce this in isolation in a PHP (8.3.6) shell:

php > $filename = "x\xc0xx.gif";
php > $replacement = '-';

php > var_dump($filename);
string(8) "x�xx.gif"

php > $filename = preg_replace('/\s/u', $replacement, trim($filename));

php > var_dump($filename);
NULL

So the sanitization ends up effectively destroying the filename (the part before extension) completely and we're left with a filename of just gif without the leading dot (this is back in \Drupal\file\Upload\FileUploadHandler::handleFileUpload after the FileUploadSanitizeNameEvent has been dispatched):

That filename then fails the allowed extension validation because... well, everything's gone pear shaped.

Not sure why the whitespace stripping regex is doing this, but that seems to be why we're seeing the weird test failure.

php > $filename = "x\xc0xx.gif";
php > $replacement = '-';
php > $filename = preg_replace('/\s/u', $replacement, trim($filename));

php > var_dump(preg_last_error());
int(4)

php > var_dump(PREG_BAD_UTF8_ERROR);
int(4)

AFAICS we're hitting:

https://www.php.net/manual/en/pcre.constants.php#constant.preg-bad-utf8-...

...the last error was caused by malformed UTF-8 data (only when running a regex in UTF-8 mode).

So I'm not sure if that's a problem with the test data or what.. but if it's possible to feed that input to the sanitisation as a filename, we probably ought to handle the error more gracefully.

It doesn't look like preg_replace() throws anything catchable here, so we might have to check for the null value and decide if we want to check preg_last_error() to provide detailed feedback / logging about what's gone wrong, or simply bail out without trying to do further validation etc..

Yeah I think a separate issue to work on the unicode handling problem would make sense.

Removing prefix as we're no longer blocked by the invalid UTF-8 handling issue.

Declaration: I used AI (specifically Claude) to help come up with the changes I've just pushed to the MR. I have manually reviewed it all, and it's the outcome of several rounds of "discussion" about the approach and implementation.

I think I suggested earlier in this issue that stripping spaces would help protect against command injection. I've since learned that's not true. I'll close the MR where we worked on just doing that.

The changes I've pushed to the surviving MR:

• Define a constant in PCRE syntax that matches a set of characters / bytes which we should never allow in filenames regardless of settings. These include control and formatting characters, shell metacharacters, and angle brackets and quotes (which are dangerous in both the shell and XSS context).
• Remove those dangerous characters from filenames.
• Do some extra sanitisation to ensure we don't leave a leading . or - both of which can be dangerous.
• Ensure we don't end up with a completely empty filename or just an extension after sanitisation.
• Live in SecurityFileUploadEventSubscriber, not FileEventSubscriber - because it already runs last, already strips null bytes unconditionally, and already flags setSecurityRename().
• Include some tests which hopefully cover most eventualities.

I think this achieves what this issue set out to do - namely disallowing dangerous characters in filenames (mostly in the context of command injection and XSS).

There are no update hooks, and none are needed. These protections aren't configurable, so there's no config to migrate - every site gets them as soon as this lands. That's one advantage over changing a filename_sanitization default, which would only have applied to new installs (and/or required update hooks etc..).

Existing filenames aren't touched; sanitisation only runs on upload, so this applies to newly uploaded files. Retrospectively renaming existing files is out of scope (could be done in contrib?).

We all have our preferences about filenames; personally I detest spaces in them but they're not a significant security risk by themselves so I'd leave that to configuration.

Shell metacharacters and the components of html tags / attributes are dangerous in filenames. We can say "contrib and custom code should handle this properly" but I don't think it's a defensible position for core to provide a footgun which ends up causing vulnerabilities in numerous contrib modules (and no doubt custom code).

We could throw the net wider, but a quick comparison: WordPress, Joomla and Typo3 all sanitise filenames and do something broadly similar to what's been proposed here (details below).

I'd argue we leave preferences to configurable transliteration settings, but strip outright dangerous characters from filenames unconditionally.. which is what my proposed changes in the MR implements.

Detailed round up of how other CMSes handle filenames:

• WordPress removes a fixed list of 32 punctuation characters, transliterates accents to plain ascii, and replaces spaces with dashes: sanitize_file_name()
• Joomla transliterates to ascii then deletes anything that isn't A-Za-z0-9._- or a space: File::makeSafe()
• TYPO3 replaces unsafe chars (and spaces) with _, accents and emoji survive: LocalDriver::sanitizeFileName()

None allow these safety features to be disabled, but there is some configurability via e.g. WP filters.

This MR leaves accents, spaces and emoji alone, so it's the least destructive of the four on legitimate filenames.

Happy to swap to replacing chars rather than removing; one reason I'd gone with the latter is that we already do that for null bytes but that's a bit different to sanitising a visible character.

We could use the replacement_character from file.settings (and I'd personally prefer a dash to an underscore) but we're in the system module, and that configuration lives in the file module; there's no dependency declared which guarantees file.settings is available here.

On the other hand the system module already uses an underscore as a replacement character elsewhere (later on in the same method in fact), so I've gone with that.

As for reducing the characters we're eliminating; I'd argue against that. Command injection can be achieved with a subshell (even within escaped double quotes) e.g.:

php > $filename = 'a$(date -u)b.txt';
php > shell_exec("stat \"$filename\"");
stat: cannot statx 'aWed 16 Sep 11:10:09 UTC 2026b.txt': No such file or directory

...and ampersands allow chaining commands:

php > $filename = 'a&date -u&b.txt';
php > echo shell_exec("stat $filename"); 
sh: 1: b.txt: not found
stat: cannot statx 'a': No such file or directory
Wed 16 Sep 11:14:53 UTC 2026

Yes, in both cases the code's not handling the unsafe input correctly, but that's the point of us trying to remove the footgun.

Same with XSS; done properly with twig there shouldn't be a problem.. but the Security Team has seen examples where it's not done properly.

Finally, core already replaces : * ? " < > | with _ in FileSystem::createFilename() but only on Windows, because they're illegal there; characters like < > " can't appear in a portable filename.

None of the other CMSes we looked at go with a small subset of potentially bad characters. I'd stand by the ones we've got in the MR currently.

Thanks Mohit!

I think it probably does warrant a CR as it's a fairly noticeable change in terms of UI/UX.

I'll draft one.

d'oh

Draft CR: https://www.drupal.org/node/3624076