[D7] Full path disclosure from errors on maintenance pages

Created on 4 October 2024, 26 days ago

Problem/Motivation

This is a D7 backport of πŸ› Maintenance pages leak sensitive environment information. Needs review

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Active

Version

7.0 ⚰️

Component

base system

Created by

πŸ‡ͺπŸ‡ͺEstonia ram4nd Tallinn

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 @ram4nd
  • Adding a tag that the D8+ issue had.

  • Pipeline finished with Failed
    25 days ago
    Total: 341s
    #300958
  • Pipeline finished with Failed
    25 days ago
    #300959
  • πŸ‡ͺπŸ‡ͺEstonia ram4nd Tallinn
  • First commit to issue fork.
  • πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

    Thanks, new branch / MR which adds a helper function to strip DRUPAL_ROOT from errors that will be displayed.

    I'm not sure if replacing the actual path with the text DRUPAL_ROOT is less confusing than just removing it completely, but it could be a bit misleading if e.g. instead of:

    /var/www/html/sites/default/settings.php
    

    ..we output:

    /sites/default/settings.php
    

    I don't think we want to use something like /path/to/drupal which would probably also confuse some people as it looks real but is not.

    Open to suggestions but as it stands this will show an error like:

    No such file or directory in include_once() (line 848 of DRUPAL_ROOT/sites/default/settings.php).
    
  • Pipeline finished with Success
    23 days ago
    #302898
  • πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

    It might not be easy to test the exact scenario from the IS, but I'd think that we should be able to add a basic test that verifies this is working as expected when an error is displayed..

  • Pipeline finished with Success
    23 days ago
    #302942
  • Pipeline finished with Success
    23 days ago
    #302957
  • πŸ‡ΈπŸ‡°Slovakia poker10

    Thank you for reporting and working on this!

    I have tested the MR also manually on a test site - checked the install.php, the update.php and a regular page served via index.php. I can confirm that with the patch from the MR, the DRUPAL_ROOT path is stripped from the error messages on all these three places correctly. I have also tested it on windows, where it seems to work as well.

    Tests-only job seems to fail as expected: https://git.drupalcode.org/project/drupal/-/jobs/2975882

    All other tests seems to pass: https://git.drupalcode.org/project/drupal/-/pipelines/302957

    There is still a "Needs tests" tag, but I am not sure if we need more tests - do you think so? I would say the added tests covers the check.

    I have also updated the issue summary with the current proposed resolution. Do we need a change record for D7? D11 fix did not have it, but since we are stripping this globally, should we consider it?

    In overall, I think this looks good, thanks!

  • πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

    @ram4nd I think you've pushed a couple of unrelated branches to the MR remote? No harm done, but just wanted to make sure you're aware :)

    @poker10 thanks for the review, and yes I think the testing looks okay as it is (I've removed the tag).

    I don't love the fallback:

        else {
          // As a fallback, make sure DRUPAL_ROOT's value is not in the path.
          $error['%file'] = str_replace(DRUPAL_ROOT, 'DRUPAL_ROOT', $error['%file']);
    }
    

    ...but my thinking was that if the value of DRUPAL_ROOT appears somewhere other than at the very beginning of a file path for some reason, it'd be pretty confusing to just make that part disappear. I'm not sure when this would ever happen, but let's say for some reason the docroot path is reflected in a longer path to a network mount or something like that..

    I think it's probably worth leaving it in, but I've made a little tweak to the conditional to avoid the fallback running at all when DRUPAL_ROOT is not present in the string in the first place.

    A wise man once told me to avoid assignments inside if conditions, but I'd argue it's worth doing here to avoid doing unnecessary work for every error.

    If you're happy with this little tweak @poker10 (and assuming tests all pass) I think it's ready to commit.

Production build 0.71.5 2024