Problem/Motivation

This was originally reported as a private security issue but cleared by the security team for a public issue

Drupal core's file upload security mechanism relies on an insecure extension blocklist to prevent the upload of executable server-side scripts. When a file with a dangerous extension is uploaded, Drupal renames it by appending _.txt to neutralize execution.
In Drupal 10.5.6, the insecure extensions list (defined in FileSystemInterface::INSECURE_EXTENSIONS and referenced by FileUploadSanitizeNameEvent) blocks the following extensions:
phar | php | pl | py | cgi | asp | js | phtml
However, this list is incomplete. The following PHP-executable extensions are not blocked and upload without being renamed:

  • .php3
  • .php4
  • .php5
  • .php7
  • .php8
  • .pht

Steps to reproduce

Proposed resolution

Add these to the list

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3593394

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

larowlan created an issue. See original summary.

tarawij made their first commit to this issue’s fork.

tarawij opened merge request !15970

tarawij’s picture

tarawij commented
Status: Active » Needs review

I've opened an MR !15970 for this issue.

The MR adds php3, php4, php5, php7, php8 and pht to FileSystemInterface::INSECURE_EXTENSIONS and updates the matching insecure extension regex.

It also includes test coverage for filename sanitisation and JSON:API upload rejection for these extensions.

I'd appreciate any feedback or review. Thanks for your time.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Based on the feedback from @larowlan

If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

tarawij’s picture

tarawij commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Believe feedback from @larowlan has been addressed

matt-whelan’s picture

matt-whelan commented

Tested on a fresh standard install of 12.x-dev (PHP 8.5), applying the MR over a clean checkout. The before/after is clear.

On plain main the real FileUploadSanitizeNameEvent path lets these through unneutralised:

shell.php5   -> shell.php5    passes through
shell.php8   -> shell.php8    passes through
shell.pht    -> shell.pht     passes through

With the MR applied they are all neutralised:

shell.php5   -> shell.php5_.txt
shell.php8   -> shell.php8_.txt
shell.pht    -> shell.pht_.txt

Coverage looks good. The MR adds the new extensions to the SecurityFileUploadEventSubscriberTest unit data set and to the jsonapi FileUploadTest, and both passed for me. No concerns on the change itself.

One scope question, not a blocker. After the fix a few more extensions still pass through, in the same severity class as the ones the MR adds. None of these execute under a stock php-fpm config. The shipped handler is .+\.ph(ar|p|tml)$, so by default only phar, php and phtml run. Drupal's own files-directory .htaccess also neutralises everything via SetHandler and engine off. So this is really defence-in-depth for setups where that .htaccess isn't honoured, like nginx or AllowOverride off, which is exactly why blocking php3 to php8 is worth doing. On the same logic:

  • phtm executes under an older or wildcard handler, the same case as pht. Worth adding.
  • .user.ini is a different mechanism, php-fpm reading auto_prepend_file to chain execution, but a comparable outcome. The stock config does not deny it, so it is worth adding for the nginx / no-htaccess case.
  • shtml is the SSI equivalent (mod_include), off by default, same misconfig class. Worth a mention at least.

For what it's worth I would leave phps off, since it renders source rather than executing and the stock config denies it outright. phpt is marginal. php2 and php6 are not real vectors, as no handler ever shipped, so leaving them off is correct.

None of this blocks the MR, which is a clear improvement on what's there now. It is just whether to round the list out here or in a follow-up.