@japerry (and/or any other maintainers) could you please - at your ultimate discretion - add credit for the following users that contributed (in most cases in the private security tracker):
boyan.borisov, chris-burge, shiju john, mayankguptadotcom
greggles and cilefen also worked on the security issue.
Thank you!
As far as I can see D11 does not have direct equivalent of file_download_access().
However it does work the same way in that \Drupal\system\FileDownloadController::download counts any headers returned by invoking file_download, and denies access if there are none.
I'd defer to the file system maintainers as to whether they'd like to add a wrapper around that count like D7's file_download_access(). That'd be @kimpepper and @mohit_aghera ( https://git.drupalcode.org/project/drupal/-/blob/11.x/core/MAINTAINERS.t... ).
I'll look at the tests next.
benjifisher → credited mcdruid → .
The security team has discussed a plan that for a few weeks we publish a CVSSv4 score alongside the usual risk score in all SAs.
We can label this as experimental, and perhaps link back to this issue.
The idea of doing this for a relatively short period is to minimise the extra work of doing two risk scores in parallel, but give everyone a chance to try using CVSSv4 before we actually switch over.
Anecdotally, a lot of organisations are still using CVSSv3.1 because 4 introduces some good additional features but along with that comes increased complexity.
https://www.first.org/cvss/calculator/4-0
I'd suggest that we only complete the first section of the v4 calculator initially. That should give us a score that looks like:
CVSS:4.0/AV:N/AC:L/AT:N/PR:L/UI:N/VC:H/VI:L/VA:N/SC:N/SI:N/SA:N
CVSS v4.0 Score: 7.1 / High
We could also provide a link to the calculator with the values pre-filled e.g.
https://www.first.org/cvss/calculator/4-0#CVSS:4.0/AV:N/AC:L/AT:N/PR:L/U...
Once we've done a couple of these in SAs we can put a template here for the markup.
Partly a note-to-self: the current risk calc system lives in the drupalorg project e.g.
https://git.drupalcode.org/project/drupalorg/-/blob/ab295c10770375254eef...
There would likely need to be changes in other places if we were to switch to a new system.
LGTM, thanks.
Thanks Benji, this LGTM.
benjifisher → credited mcdruid → .
Thinking about how we handle existing user accounts if we change the hashing behaviour...
If we start to enforce a limit on the password length, we'd need to decide whether we try to do something helpful / graceful in the UI e.g. tell users that supply a longer input that they should only provide the first 72 bytes of their password, or try to do that for them automatically (and tell the user this is what's happening?).
That may be easier said than done if e.g. the password includes multi-byte characters and the 72 byte threshold falls in the middle of a character. The maxlength attribute "is measured in UTF-16 code units, which is often but not always equal to the number of characters", but I'm not certain what that would mean in all cases where existing passwords exceed the limit in bytes.
I'd think we'd certainly want to avoid e.g. enforcing a limit in the login form which effectively makes it impossible for users with passwords that exceed the limit to authenticate (perhaps they'd be forced to do a password reset - but we can't know that's always available on all Drupal sites?)
If we start pre-hashing passwords (would we do that universally or only when the input is longer than a threshold?), we'd perhaps need to re-generate the stored password hash for existing user accounts (this has been done before in Drupal when the hashing algorithm changed).
Presumably that means:
- A user with a >72 byte password would be successfully authenticated based on the first 72 bytes of the password they supply matching the existing hash.
- We then generate a new hash based on the full password they've supplied and store that for future use.
- The password they've given may or may not match what they originally gave - we can only ever know that the first 72 bytes matched.
- From then onwards, the password they give would need to fully match the one used to generate the new hash.
No way around that AFAICS, but it's pretty ugly; doesn't change the fact we're only considering the first 72 bytes at the moment anyway.
Need to verify whether this (still) actually applies, but here's a strong argument not to pre-hash (or at least to approach doing so with extreme caution):
https://blog.ircmaxell.com/2015/03/security-issue-combining-bcrypt-with....
There's an argument that it's best not to use just a plain hash in the pre- step before bcrypt - e.g. instead of:
bcrypt(sha512($password))
..you'd do:
bcrypt(sha512($salt + $password))
..or use a "pepper" instead of salt, or use a HMAC.
Some references:
For context, discussion around the same thing in Laravel:
https://securinglaravel.com/security-tip-limiting-bcrypt-passwords-to-72...
In Laravel 12 they added a configurable flag to have the Hash class throw an exception when given input longer than the hashing algorithm properly supports.
There's some follow-up discussion about what approach to take in future e.g. adopting a different hash algo. by default.
benjifisher → credited mcdruid → .
Verified that the test-only job fails as expected:
https://git.drupalcode.org/issue/drupal-3526769/-/jobs/5685661
..but tests pass with the changes.
Here's the contents of the test tarball:
core/modules/config/tests/fixtures$ tar ztvf not_just_config.tar.gz
-rw-rw-r-- mcdruid/mcdruid 48 2025-06-26 10:50 config.one.yml
-rw-rw-r-- mcdruid/mcdruid 48 2025-06-26 10:50 config.two.yml
-rwxrwxr-x mcdruid/mcdruid 48 2025-06-26 11:03 executable.yml
-rwxrwxr-x mcdruid/mcdruid 60 2025-06-26 10:52 script.sh
I was wondering about putting that in a comment in the test to save anyone having to examine it themselves.. but perhaps that just risks getting out of sync with the fixture file, and might not be necessary / helpful anyway.
Added a basic test that verifies a shell script cannot be smuggled into a config import tarball, and checks that files are not executable after imports (even when they are inside the tarball).
I think this needs to be a functional test, as the changes being made are in the processing of the import form on submission.
Updates to IS now that we've agreed a proposed solution.
Thank you to the Usability Team who reviewed and discussed this issue in their meeting 📌 Drupal Usability Meeting 2025-05-30 Active . 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.
benjifisher → credited mcdruid → .
benjifisher → credited mcdruid → .
Existing tests pass with the changes in the MR.
We should add one or more tests that show:
- Only .yml files are extracted from the tarball.
- A file with executable permissions in the tarball is extracted with default (664) permissions into the sync directory.
Looks like it's legitimate to have subdirectories within the tarball, based on \Drupal\Tests\config\Functional\ConfigExportImportUITest::testExportImportCollections.
I'd initially assumed there was only ever a flat list of yml files.
Perhaps the module could also do a stable release and opt in to security coverage :)
Thanks for the very quick response @steven jones
+1 to concerns raised in #26 / #27.
Allowing the path to an executable to be set in the UI is a really valuable way for an attacker to escalate XSS to RCE.
One recent related issue was adding Symfony Mailer transports to core 📌 [PP-1] Add symfony mailer transports to DIC Postponed where I believe it's necessary to set anything non-standard via settings.php.
Validation is really important (noting the value is being checked on submit with is_executable() and there are tests showing that command -args type values will fail).
I agree that this makes ✨ Require password for certain admin actions Active a high priority security hardening.
benjifisher → credited mcdruid → .
benjifisher → credited mcdruid → .
In the meantime I've submitted a PR to guzzle with a fairly crude way of minimising the impact of the Gadget Chain:
https://github.com/guzzle/guzzle/pull/3286
We'll see how that goes, but I don't think it's a reason to stop looking at dropping the class from Drupal.
benjifisher → credited mcdruid → .
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.
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.)
Adding REMOTE_ADDR as a trusted proxy may work in some cases, but it can also cause problems such as making it possible to spoof the client IP via X-Forwarded-For (per #5 and the existing cautionary note in the comments).
benjifisher → credited mcdruid → .
Great, thanks for the nudge in the right direction.
Tests passing.
One issue is that comments in the test refer to the gadget chain as "Drupal/FI1" but there's currently no such thing in https://github.com/ambionics/phpggc (a new PR may be opened for it soon though).
I think this should be back at NR.
There is a linting error / warning in the MR but I don't understand what it's saying sufficiently to know whether it's related to this change - it doesn't appear to be at a glance.
Very definitely +1 to the idea of requiring an additional authentication step to install a module via Project Browser to defend against XSS/CSRF attacks.
Benji makes good points about SSO and TFA, and whether requiring the password to be supplied is overly prescriptive; having an API for (re-)authentication would be great.
I can think of other use cases for this too (I'll link to issues I plan to file).
As for being able to toggle the extra protection on/off (e.g. development mode), would it make sense to have this as setting that cannot be altered via the UI?
greggles → credited mcdruid → .
sorry, didn't mean to change status
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.
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:
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..
@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?
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?!)
greggles → credited mcdruid → .
Ok, thanks for confirming.
I think 1.0.x is still marked as the default at https://git.drupalcode.org/project/cms/ FWIW.
benjifisher → credited mcdruid → .
Is this a duplicate of 📌 Add debug mode (and fix virus name for executable) Needs work (which has an MR)?
I've verified that if we inject something like an XSS payload into the scanner output, it gets logged verbatim.
For example (this is a low effort quick hacky test):
+++ b/src/Scanner/Executable.php
@@ -65,6 +65,9 @@ class Executable implements ScannerInterface {
// values greater than 2. Any value of 2 or greater means that the scan
// failed, and the file has not been checked.
exec($cmd, $output, $return_code);
+
+ $output[] = '<script>alert("oh noes!")</script>';
+
$this->_output = implode("\n", $output);
An example log entry in the db looks like:
> SELECT * FROM watchdog \G
*************************** 78. row ***************************
wid: 78
uid: 1
type: Clam AV
message: Virus %virusname detected in uploaded file %filename. Debug output: %output
variables: a:3:{s:9:"%filename";s:14:"/tmp/phpALzyP1";s:10:"%virusname";s:15:"Eicar-Signature";s:7:"%output";s:361:"/tmp/phpALzyP1: Eicar-Signature FOUND
----------- SCAN SUMMARY -----------
Known viruses: 8704988
Engine version: 1.0.7
Scanned directories: 0
Scanned files: 1
Infected files: 1
Data scanned: 0.00 MB
Data read: 0.00 MB (ratio 0.00:1)
Time: 10.241 sec (0 m 10 s)
Start Date: 2025:03:14 14:01:59
End Date: 2025:03:14 14:02:09
<script>alert("oh noes!")</script>";}
severity: 3
The traditional approach in Drupal is to filter on output ( https://www.drupal.org/docs/develop/security/why-does-drupal-filter-on-o... → ). The dblog admin UI escapes this appropriately.
We could pass the debug output through a filter before logging it.. but I'm not sure that's necessary, or best practice.
We don't want to fill syslog log entries with lots of html-encoded characters unnecessarily, for example (I also don't think the typical output of the clam scanner will contain anything bad.. especially as - per my earlier comment - the user-chosen filename is not currently included in these logs).
So I think this MR's ready for review.
Same deal for connection to the daemon by socket - I've added the same scanner output as for the tcp/ip mode.
This is a bit duplicative.. but for now I've added the same message about the failed connection to the daemon to the $_output property so that it gets logged as the scanner output when debug is enabled.
I suppose we could omit the other log entry when debug is enabled, but I'm not certain that's a good idea; it might already be used for monitoring etc..
Daemon mode example log entries (again, verbose + debug enabled):
Uploaded file /tmp/phpuLzC2k checked and found clean. Debug output: stream: OK
Virus Eicar-Signature detected in uploaded file /tmp/phpnGu0G4. Debug output: stream: Eicar-Signature FOUND
...and if I tried to deliberately break the daemon set up..
Uploaded file /tmp/phpkCRZnu could not be checked, and was deleted. Debug output:
Hmm that's not so useful. In that case the daemon wasn't actually listening for connections at all. Would probably be useful to try to log something vaguely informative in that situation.
If I try to break the config of the daemon it just doesn't start up.. so I think not being able to connect is one of the more realistic problems.
The module does actually log messages when it can't connect:
https://git.drupalcode.org/project/clamav/-/blob/2.1.x/src/Scanner/Daemo...
Unable to connect to ClamAV TCP/IP daemon on clamav:3310
Manually tested with executable mode; seems to work as expected.
Example log message when the scanner found a problem (with both verbose and debug enabled):
Virus Eicar-Signature detected in uploaded file /tmp/phpqQahST. Debug output: /tmp/phpqQahST: Eicar-Signature FOUND ----------- SCAN SUMMARY ----------- Known viruses: 8704988 Engine version: 1.0.7 Scanned directories: 0 Scanned files: 1 Infected files: 1 Data scanned: 0.00 MB Data read: 0.00 MB (ratio 0.00:1) Time: 10.072 sec (0 m 10 s) Start Date: 2025:03:14 11:57:30 End Date: 2025:03:14 11:57:40
It'd ideally be better if the original filename of the upload was logged instead of the tmp name, but that's a different issue.
Another example where I deliberately broke the clamscan executable - this is the sort of thing debug mode may be more useful for in reality:
Uploaded file /tmp/phpXYXjA4 could not be checked, and was deleted. Debug output: LibClamAV Error: cli_load(): Can't open file /var/lib/clamav/main.cvd LibClamAV Error: cli_loaddbdir: error loading database /var/lib/clamav/main.cvd LibClamAV Error: cli_loaddbdir: No supported database files found in /var/lib/clamav ERROR: Can't open file or directory ----------- SCAN SUMMARY ----------- Known viruses: 0 Engine version: 1.0.7 Scanned directories: 0 Scanned files: 0 Infected files: 0 Data scanned: 0.00 MB Data read: 0.00 MB (ratio 0.00:1) Time: 0.004 sec (0 m 0 s) Start Date: 2025:03:14 12:02:03 End Date: 2025:03:14 12:02:03
I'll try to do some similar tests with the daemon mode too, and socket if I have time (https://github.com/fwust/ddev-clamav looks like it'll make testing daemon mode pretty easy.. socket mode may involve a bit more manual set up).
There are quite a few linting errors between cspell / phpcs / phpstan.
A couple of them are introduced by the changes in the MR, but that's as a result of following the patterns / conventions that are already there e.g.
32 | WARNING | Property name "$_output" should not be prefixed with an underscore to indicate visibility
| | (PSR2.Classes.PropertyDeclaration.Underscore)
32 | ERROR | Class property $_output should use lowerCamel naming without underscores
| | (Drupal.NamingConventions.ValidVariableName.LowerCamelName)
...where there are other properties like $_virus_name that hit the same problems.
It would probably make sense to address all of this in a dedicated issue, and just follow the existing patterns in this MR.
Rerolled the old patch against 2.1.x and put it into an MR.
There's a todo list in #3
I don't think the _file property is an issue any more - I've removed it as it wasn't being used for anything much.
I've not tested this manually (with any of the different configurations) yet though, and the output from the scanner should be sanitised.
Moving to NR for the testing, but we shouldn't commit this without the sanitisation.
mcdruid → changed the visibility of the branch 3178741-add-debug-mode to hidden.
Switching branches to create a new MR.
Oh, this was already fixed in https://git.drupalcode.org/project/ai/-/commit/ede64f611afb54e8d6919b3ff...
mcdruid → created an issue.
greggles → credited mcdruid → .
Thanks - you're right.. but I already applied the same changes :)
Thank you for the offer - very happy to accept it!
Adding credit from the private security issue.
Tests are passing.
Noting that this test is very similar to the one in 📌 Add tests for SA-CORE-2024-006 Active so anything we want to change here will likely also apply over there.
mcdruid → changed the visibility of the branch 3508240-add-tests-for to hidden.
This was only for 10.x
Adding credit from the private security issue.