Contrib.social

Avoid abuse of config sync import tarballs

Open on Drupal.org →
Created on 27 May 2025, about 2 months ago

Problem/Motivation

This issue has been discussed with the Drupal Security Team, and approved for handling in public as there is no direct vulnerability / exploit.

It's possible to abuse the config sync import functionality in order to upload malicious files.

In order to access the functionality, a user must have one or more restricted permissions.

At present \Drupal\config\Form\ConfigImportForm will extract the content of an uploaded tarball into the config_sync_directory without any filtering or sanitisation of file attributes.

This means it's possible to include e.g. malicious executable files in the tarball and these will be extracted to the sync directory.

A bad actor would then need a way of actually executing those malicious files, but getting them onto the webserver at a predictable location and with the executable bit in tact could be one step in a chained attack.

Note that it should not be possible to use e.g. directory traversal to extract a file outside the sync directory, but this depends on the implementation in the PEAR Archive_Tar library. There has been at least one relevant vulnerability in the library in the past (e.g. https://github.com/advisories/GHSA-p8q8-jfcv-g2h2 ).

Steps to reproduce

Use the tarball import functionality at /admin/config/development/configuration/full/import to upload a tarball with content other than valid .yml configuration exports.

For example, an executable shell script within the tarball will be extracted into the sync directory.

Proposed resolution

The tarball import should only extract what appear to be valid .yml config files.

It should also sanitise the permissions of imported files; for example ensuring that they are not executable.

Remaining tasks

MR / review / merge etc..

User interface changes

I don't think we need to present error messages when there are files in the tarball that will not be extracted, or permissions that will be sanitised?

Introduced terminology

n/a

API changes

n/a

Data model changes

n/a

Release notes snippet

I don't think this needs a release note, as there is no change to the legitimate functionality.

📌 Task
Status

Active

Version

11.0 🔥

Component

configuration system

Created by

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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

Merge Requests

Comments & Activities

  • Issue created by @mcdruid
  • Merge request !12251validate filenames and sanitise permissions → (Open) created by mcdruid
  • Comment about 2 months ago →
    🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
  • Pipeline finished with Failed
    about 2 months ago
    Total: 164s
    #507487
  • Comment about 2 months ago →
    🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
  • Pipeline finished with Failed
    about 2 months ago
    Total: 463s
    #507497
  • Comment about 2 months ago →
    🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

    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.

  • Pipeline finished with Success
    about 2 months ago
    Total: 1986s
    #507528
  • Comment about 2 months ago →
    🇬🇧United Kingdom 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.
  • Comment 28 days ago →
    🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

    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.

  • Comment 28 days ago →
    🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

    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.

  • Pipeline finished with Success
    28 days ago
    Total: 3459s
    #532204
  • Comment 28 days ago →
    🇬🇧United Kingdom 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.

  • Comment 14 days ago →
    🇺🇸United States smustgrave

    I don't think we need to present error messages when there are files in the tarball that will not be extracted, or permissions that will be sanitised?

    I would agree, not sure how many users import this way but imagine they are aware of the other files if they use this approach.

    Looks like it's legitimate to have subdirectories within the tarball, based on \Drupal\Tests\config\Functional\ConfigExportImportUITest::testExportImportCollections.

    Maybe translations?

    Test-only feature has already been ran 

    1) Drupal\Tests\config\Functional\ConfigImportUploadTest::testImportTarballFiltering
Failed asserting that true is false.
/builds/issue/drupal-3526769/core/modules/config/tests/src/Functional/ConfigImportUploadTest.php:89
FAILURES!
Tests: 2, Assertions: 21, Failures: 1.
Exiting with EXIT_CODE=1

    Which seems to fail at a valid assertion for this issue.

    Changes themselves look fine to me (least no objection)

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024