Contrib.social

It should not be possible to install Drupal database into a site where it is already installed when pending translation imports exists

Open on Drupal.org →
Created on 20 June 2024, 5 months ago
Updated 11 August 2024, 4 months ago

Problem/Motivation

In some scenarios it is possible to access the /core/install.php even when a site is already installed. This can lead to regenerating the admin user. Original raised with the security team but advised its fine to publicly post.

This is possible because this code here in core/includes/install.core.inc

  if ($install_state['config_verified'] && empty($task)) {
    if (count($kernel->getConfigStorage()->listAll())) {
      $task = NULL;
      throw new AlreadyInstalledException($container->get('string_translation'));
    }
  }

Checks that $task is empty, and if not it assumes its in the new installation process and let's the install continue without throwing the AlreadyInstalledException.
When translations are pending import, the task is set as `install_import_translations` because this code will result in TRUE:

$needs_translations = $locale_module_installed && ((count($install_state['translations']) > 1 && !empty($install_state['parameters']['langcode']) && $install_state['parameters']['langcode'] != 'en') || \Drupal::languageManager()->isMultilingual());

Steps to reproduce

You can see this issue by:

  1. Create a drupal site
  2. Install the site and enable translations
  3. Deploy with any importing translation failure
  4. Go to /core/install.php and reinstall Drupal, setting the admin user and password to whatever you want
  5. Go to the site on completion and login as your new admin user. The existing database is not wiped, but the admin user is overridden

Note that the Contact module must also be enabled on the site otherwise the install process triggers a fatal error stopping the above from happening. So probably many sites don't have this potential issue as a result.

A simpler way to reproduce on any site programmatically without having to reproduce a more complex failure:

  1. Run drush state:set install_task 'install_import_translations' --input-format=string
  2. Go to /core/install.php and reinstall Drupal, setting the admin user and password to whatever you want
  3. Go to the site on completion and login as your new admin user. The existing database is not wiped, but the admin user is overridden

Proposed resolution

Double-check that Drupal is not yet installed.

For those coming across this same issue, if you get any import translation failure, you can manually run the following to re-protect your site:
drush state:set install_task 'done' --input-format=string

Remaining tasks

Provide an additional check

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

Prevent re-installation of Drupal

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Install  →

Last updated 1 day ago

No maintainer
Created by

🇬🇧United Kingdom scott_euser

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

  • 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 @scott_euser
  • Merge request !8463Prevent re-install if site already exists → (Open) created by scott_euser
  • Status changed to Needs review 5 months ago
  • Comment 5 months ago →
    🇬🇧United Kingdom scott_euser

    Could use some suggestions on how to add tests for this (if its possible for the install)

  • Comment 5 months ago →
    🇬🇧United Kingdom scott_euser
  • Comment 5 months ago →
    🇬🇧United Kingdom scott_euser
  • Pipeline finished with Failed
    5 months ago
    Total: 599s
    #203395
  • Status changed to Needs work 5 months ago
  • Comment 5 months ago →
    🇺🇸United States smustgrave

    Appears to have relevant test failures.

    Also MR should be updated for 11.x vs 11.0.x

  • Pipeline finished with Canceled
    5 months ago
    #208023
  • Pipeline finished with Canceled
    5 months ago
    Total: 126s
    #208024
  • Pipeline finished with Failed
    5 months ago
    Total: 564s
    #208025
  • Pipeline finished with Failed
    5 months ago
    Total: 652s
    #208382
  • Status changed to Needs review 5 months ago
  • Comment 5 months ago →
    🇬🇧United Kingdom scott_euser

    Okay I updated the approach to check for the existence of user 1 already.

    I updated the issue summary with an explanation on how to simply reproduce this on any Drupal site via Drush (of course because its programmatically it then does not have the risk, but hopefully its helpful for testing and moving this issue forward).

    The tests will continue to fail though because the problem is that the installation tests actually do as the issue here defines, with different causes.

    1. InstallerTestBase::setUp() calls parent ::setUp()
    2. parent ::setUp() calls ::installDrupal() which is overridden by InstallerTestBase and installs drupal
    3. InstallerTestBase::setUp() then runs checks that Drupal can be installed and the tests fail if not BUT Drupal is already installed because ::installDrupal() is run.

    So really its testing that Drupal can be reinstalled when its already installed which actually seems like it might not be correct?

    I did add a test which shows with a simple state change, the installer can be re-run without the code change (which eventually leads to user 1 getting overridden).

    Given this is going to affect so many tests (36 tests) because of the above, I wonder if the simpler thing to do is just to allow a $setting variable like $settings['disable_installer'] = TRUE; and if that would be easier to get in. Essentially I do not want to put lots of effort into something that will likely never get merged.

    Ultimately I want to be able to prevent existing sites from getting user 1 overridden as to me that's a high risk issue, so being able to disable the installer would similarly achieve that. I can of course manually set the state to done to avoid the issue but a future failure can cause the state to change back as its possible that state is wiped in which case the installer goes through the steps again and if it has e.g. import translation failures, it will not consider that step done.

  • Comment 4 months ago →
    🇳🇿New Zealand quietone
  • Comment 4 months ago →
    🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Clarifying that this was logged privately to the security team who approved for it to be public

  • Status changed to Needs work 4 months ago
  • Comment 4 months ago →
    🇺🇸United States smustgrave

    So definitely seems like something to fix. Posted about it in #needs-review-queue-initiative and as @larowlan mentioned was originally security issue made public.

    Before adding $settings['disable_installer'] = TRUE is there any benefit to having this as FALSE? If there are scenarios then I think the setting makes sense but if something that should always be TRUE maybe we just don't allow it?

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024