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 - 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.

Issue fork drupal-3526769

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

mcdruid created an issue. See original summary.

mcdruid opened merge request !12251

mcdruid’s picture

mcdruid commented
Status: Active » Needs review
Issue tags: +Needs tests
mcdruid’s picture

mcdruid commented
Issue summary: View changes
mcdruid’s picture

mcdruid commented

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.

mcdruid’s picture

mcdruid commented
Status: Needs review » Needs work

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.
mcdruid’s picture

mcdruid commented
Status: Needs work » Needs review
Issue tags: -Needs tests

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 import (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.

mcdruid’s picture

mcdruid commented

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.

mcdruid’s picture

mcdruid commented

Verified that the test-only job fails as expected:

https://git.drupalcode.org/issue/drupal-3526769/-/jobs/5685661

..but tests pass with the changes.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

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)

catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.5.x-dev
Status: Reviewed & tested by the community » Fixed

This looks good to me.

I briefly thought we should warn, but it's far more likely someone would upload their own malicious tarball than persuade someone else to do so, do we want to tell the attacker what we did? And also if someone accidentally bundles up a .orig file or something else weird, do they really need to know we ignored it?

Committed/pushed to 11.x and cherry-picked to 11.2.x, 10.6.x and 10.5.x, thanks!

  • catch committed b3c4e4af on 10.5.x 
    Issue #3526769 by mcdruid, smustgrave: Avoid abuse of config sync import...

  • catch committed dd72d0bf on 10.6.x 
    Issue #3526769 by mcdruid, smustgrave: Avoid abuse of config sync import...

  • catch committed 0fff4d79 on 11.2.x 
    Issue #3526769 by mcdruid, smustgrave: Avoid abuse of config sync import...

catch closed merge request !12251

  • catch committed 0bb87cce on 11.x 
    Issue #3526769 by mcdruid, smustgrave: Avoid abuse of config sync import...
catch’s picture

catch
he/him
Primary language English
commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.