Chmod 0777 race condition during staging operations

Created on 16 March 2024, about 1 year ago
Updated 24 April 2024, about 1 year ago

Problem/Motivation

Found by Cure53 in security review, determined low risk and could be made public
While analyzing the drawback outlined in ticket DRU-01-006, Cure53 found that the deletion of the staging directory precedes granting user file permission access to the stage directory. This effectively enables attackers to drop symbolic links into the directory between the moment of file access permission and prior to deletion. As this operation is performed recursively on every sub-element of the directory, this also allows attackers to modify and execute production system files before deletion via a race condition. Similarly to DRU-01- 006, this could be abused to execute OS commands in the name of the OS user executing the applicationโ€™s PHP code. A similar flaw was identified within the SiteConfigurationExcluder, which renders the sites/default directory world-writable for all after creating the stage directory.

Steps to reproduce

Proposed resolution

Affected file:
automatic_updates/package_manager/src/Plugin/QueueWorker/Cleaner.php
Affected code:

public function processItem($dir) {
         assert(is_string($dir));
         if (file_exists($dir)) {
             $this->fileSystem->deleteRecursive($dir, function (string $path):
       void {
$this->fileSystem->chmod($path, 0777); });

Additionally affected:
package_manager/src/PathExcluder/ SiteConfigurationExcluder.php::makeDefaultSiteDirectoryWritable
To mitigate this issue, Cure53 advises enforcing permissions to 0600 and 0700 for files and directories respectively, rather than using 0777 recurringly. By doing so, only the owner of the files would be allowed to delete the files before removal. This could be achieved by simply omitting the callback passed to the deleteRecursive function, as the Drupal core function already performs this internally.

Remaining tasks

User interface changes

API changes

Data model changes

๐Ÿ› Bug report
Status

Fixed

Version

3.0

Component

Code

Created by

๐Ÿ‡บ๐Ÿ‡ธUnited States tedbow Ithaca, NY, USA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @tedbow
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States tedbow Ithaca, NY, USA
  • First commit to issue fork.
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    This could be achieved by simply omitting the callback passed to the deleteRecursive function, as the Drupal core function already performs this internally.

    Are you sure?

    I see this in \Drupal\Core\File\FileSystem::unlink() and \Drupal\Core\File\FileSystem::rmdir():

        if (!$this->streamWrapperManager->isValidUri($uri) && str_starts_with(PHP_OS, 'WIN')) {
          chmod($uri, 0700);
        }
    

    Soooo...it's only chmodding if we're on Windows?

  • Pipeline finished with Failed
    about 1 year ago
    Total: 1297s
    #143011
  • Pipeline finished with Failed
    about 1 year ago
    #143091
  • Pipeline finished with Failed
    about 1 year ago
    Total: 1083s
    #143122
  • Pipeline finished with Success
    about 1 year ago
    Total: 1324s
    #143146
  • Assigned to tedbow
  • Status changed to Needs review about 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts
  • Assigned to phenaproxima
  • Status changed to Needs work about 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States tedbow Ithaca, NY, USA
  • Assigned to tedbow
  • Status changed to Needs review about 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    It...it doesn't. It only does that if the operating system is Windows.

    I pointed that out in #4.

  • Status changed to RTBC about 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States tedbow Ithaca, NY, USA

    Looks good #4 is correct the explanation of why this is only done on Windows in the comment for \Drupal\Core\File\FileSystemInterface::rmdir

  • Pipeline finished with Skipped
    about 1 year ago
    #143198
  • Issue was unassigned.
  • Status changed to Fixed about 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts
  • Automatically closed - issue fixed for 2 weeks with no activity.

  • Pipeline finished with Failed
    12 days ago
    Total: 702s
    #502705
  • Pipeline finished with Failed
    12 days ago
    Total: 844s
    #502737
  • Pipeline finished with Failed
    11 days ago
    Total: 86327s
    #502852
  • Pipeline finished with Failed
    4 days ago
    Total: 735s
    #509295
  • Pipeline finished with Success
    4 days ago
    Total: 5370s
    #509370
  • Pipeline finished with Success
    3 days ago
    Total: 1488s
    #510341
  • Pipeline finished with Success
    2 days ago
    Total: 874s
    #510843
  • Pipeline finished with Success
    about 6 hours ago
    Total: 778s
    #512001
  • Pipeline finished with Success
    about 4 hours ago
    Total: 1013s
    #512085
  • Pipeline finished with Failed
    about 4 hours ago
    Total: 714s
    #512123
  • Pipeline finished with Success
    about 4 hours ago
    Total: 871s
    #512149
Production build 0.71.5 2024