Contrib.social

Always rename dot files like Drupal 7

Open on Drupal.org →
Created on 8 August 2022, almost 3 years ago
Updated 23 March 2025, 3 months ago

Problem/Motivation

https://www.drupal.org/sa-core-2022-014 → improved handling of dot files in the case that htaccess is in the list of allowed extensions. However Drupal 7 will always rename dot files. I think Drupal 9 and 10 should have the same behaviour - for example uploading a .htpasswd file could have interesting consequences.

The problem is that 

    // Dot files are renamed regardless of security settings.
    $filename = trim($filename, '.');

is not actually true because further down the method we do an early exit if we think file_validate_extensions() is going to reject it. But unfortunately the logic in file_validate_extensions() and this method are not 100% the same. At some point we should add a static method that file_validate_extensions() calls so we can be sure that the logic is 100% the same.

However in this case the fix is simpler we should do

    if ($filename !== $event->getFilename()) {
      $event->setFilename($filename)->setSecurityRename();
    }

immediately after the trim to make the comment correct. In fact this should go after the null byte removal as both of these parts are security related.

Steps to reproduce

Allow files with the htpasswd extension and then upload a file called .htpasswd

Proposed resolution

Always rename dot files.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

📌 Task
Status

Needs work

Version

11.0 🔥

Component

file system

Created by

🇬🇧United Kingdom alexpott 🇪🇺🌍

Live updates comments and jobs are added and updated live.
  • Security improvements

    It makes Drupal less vulnerable to abuse or misuse. Note, this is the preferred tag, though the Security tag has a large body of issues tagged to it. Do NOT publicly disclose security vulnerabilities; contact the security team instead. Anyone (whether security team or not) can apply this tag to security improvements that do not directly present a vulnerability e.g. hardening an API to add filtering to reduce a common mistake in contributed modules.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • @prudloff opened merge request.
  • Comment 3 months ago →
    🇫🇷France prudloff Lille

    I did a quick manual test and I think this is still a concern.

    I rebased the patch.
    I'm not sure the failing AssetAggregationAcrossPagesTest is related?

  • Comment 3 months ago →
    🇧🇪Belgium borisson_ Mechelen, 🇧🇪

    I don't see why that test would start failing, and I can't find an issue about it having random failures. Since this a security hardening I think we should move this through.

  • Comment 3 months ago →
    🇳🇿New Zealand quietone

    This should have a title that has meaning that includes people who only know Drupal 8 and above. As is, the title, and thus the commit message, only makes sense for those that know what Drupal 7 did. Therefor, tagging for a title update.

  • Comment 3 months ago →
    🇬🇧United Kingdom alexpott 🇪🇺🌍
  • Comment 3 months ago →
    🇺🇸United States greggles Denver, Colorado, USA

    Seems good to add the "why" - it's about extra security through defense in depth

  • Comment 3 months ago →
    🇧🇪Belgium borisson_ Mechelen, 🇧🇪

    Removing the tag, title looks great now.

  • Comment 9 days ago →
    System Message
    • longwave → committed 56fe980b on 11.x 
      Issue #3302448 by prudloff, alexpott: Always rename dot files regardless...
  • Status changed to Downport 9 days ago
  • Comment 9 days ago →
    🇬🇧United Kingdom longwave UK

    This change makes sense to me and simplifies the code path a bit while making things more secure, so good all round.

    Committed 56fe980 and pushed to 11.x. Thanks!

    As this is a minor behaviour change, but also a security improvement, I'm on the fence somewhat about backporting to 11.2.x. This is definitely eligible for 10.6.x (and perhaps 10.5.x if we also commit to 11.2.x), but the MR doesn't apply, so marking PTBP so we can decide what to do next here.

  • Comment 9 days ago →
    System Message
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024