Problem/Motivation

Vanilla Drupal 7 typically goes through its bootstrap phases in this order:

https://git.drupalcode.org/project/drupal/blob/7.67/includes/bootstrap.i...

 *   - DRUPAL_BOOTSTRAP_CONFIGURATION: Initializes configuration.
 *   - DRUPAL_BOOTSTRAP_PAGE_CACHE: Tries to serve a cached page.
 *   - DRUPAL_BOOTSTRAP_DATABASE: Initializes the database layer.
 *   - DRUPAL_BOOTSTRAP_VARIABLES: Initializes the variable system.
 *   - DRUPAL_BOOTSTRAP_SESSION: Initializes session handling.
 *   - DRUPAL_BOOTSTRAP_PAGE_HEADER: Sets up the page header.
 *   - DRUPAL_BOOTSTRAP_LANGUAGE: Finds out the language of the page.
 *   - DRUPAL_BOOTSTRAP_FULL: Fully loads Drupal. Validates and fixes input data.

To handle SACORE-2018-004 new "request sanitizer" code was added to the end of the DRUPAL_BOOTSTRAP_CONFIGURATION phase:

https://git.drupalcode.org/project/drupal/blob/7.67/includes/bootstrap.i...

  // Initialize the configuration, including variables from settings.php.
  drupal_settings_initialize();

  // Sanitize unsafe keys from the request.
  require_once DRUPAL_ROOT . '/includes/request-sanitizer.inc';
  DrupalRequestSanitizer::sanitize();
}

However, the domain module in D7 works by calling its own bootstrap phases from the end of settings.php, which is processed just before the new request sanitizer code.

The domain bootstrap code needs access to the database and it therefore invokes later phases of core's bootstrap - initially DRUPAL_BOOTSTRAP_DATABASE:

https://git.drupalcode.org/project/domain/blob/7.x-3.x/domain.bootstrap....

That results in other core bootstrap phases being invoked before drupal_settings_initialize() returns and execution reaches the new request sanitizer code.

One of these phases invoked "out of order" is DRUPAL_BOOTSTRAP_VARIABLES:

https://git.drupalcode.org/project/drupal/blob/7.67/includes/bootstrap.i...

That checks if there's a destination query parameter, and if so sanitizes it using the new class:

    // Use the DrupalRequestSanitizer to ensure that the destination's query
    // parameters are not dangerous.
    if (isset($_GET['destination'])) {
      DrupalRequestSanitizer::cleanDestination();
    }

However, if the last couple of lines of the DRUPAL_BOOTSTRAP_CONFIGURATION phase have not yet been reached, the new code has not yet been included.

This can result in a fatal error as the new class has not yet been defined. This is illustrated by the following stack trace:

PHP Fatal error:  Class 'DrupalRequestSanitizer' not found in /path/to/d7/includes/bootstrap.inc on line 2784
PHP Stack trace:
PHP   1. {main}() /path/to/d7/index.php:0
PHP   2. drupal_bootstrap() /path/to/d7/index.php:20
PHP   3. _drupal_bootstrap_configuration() /path/to/d7/includes/bootstrap.inc:2508
PHP   4. drupal_settings_initialize() /path/to/d7/includes/bootstrap.inc:2634
PHP   5. include_once() /path/to/d7/includes/bootstrap.inc:747
PHP   6. include() /path/to/d7/sites/default/settings.php:706
PHP   7. domain_bootstrap() /path/to/d7/sites/all/modules/contrib/domain/settings.inc:26
PHP   8. _domain_bootstrap() /path/to/d7/sites/all/modules/contrib/domain/domain.bootstrap.inc:70
PHP   9. drupal_bootstrap() /path/to/d7/sites/all/modules/contrib/domain/domain.bootstrap.inc:112
PHP  10. _drupal_bootstrap_page_cache() /path/to/d7/includes/bootstrap.inc:2512
PHP  11. drupal_bootstrap() /path/to/d7/includes/bootstrap.inc:2658
PHP  12. _drupal_bootstrap_variables() /path/to/d7/includes/bootstrap.inc:2520

Note this error is avoided if there's already an entry for the new class in the registry as it's then autoloaded.

Proposed resolution

Moving this line:

  require_once DRUPAL_ROOT . '/includes/request-sanitizer.inc';

...slightly earlier in drupal_bootstrap() avoids the situation where the new class is used before it's been defined if something like the domain module calls the bootstrap phases in a different order.

Remaining tasks

  • Review the patch.
  • Commit the patch.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None?

Original report by @dvandijk

With the Domain module active the bootstrap phase doesn't include the includes/request-sanitizer.inc file when a ?destination= URL parameter is present. Therefore resulting in a 500 error on all pages with the ?destination parameter.

The fix is adding an extra require_once on line 2784 in bootstrap.inc (see patch).

CommentFileSizeAuthor
#9 interdiff-2966335-1-9.txt954 bytesmcdruid
#9 2966335-9.patch715 bytesmcdruid
SA-CORE-2018-004-domain.patch581 bytesdvandijk

Comments

dvandijk created an issue. See original summary.

mo6’s picture

mo6
he/him
Primary language Dutch
commented

I confirmed the problem with D7 and domain and verified that this patch works.

steven buteneers’s picture

steven buteneers commented

Please provide steps to reproduce, on our platform running domain access on the current stable version we do not encounter this problem.

mcdruid’s picture

mcdruid commented
Status: Active » Needs work

I've not been able to reproduce this with some quick basic tests, but I can see how it might happen.

The domain include which starts its own bootstrap is typically placed at the end of settings.php

...and settings.php is processed as part of DRUPAL_BOOTSTRAP_CONFIGURATION which calls:

https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/_dru...

...which includes the new request-sanitizer.inc:

  // Initialize the configuration, including variables from settings.php.
  drupal_settings_initialize();

  // Sanitize unsafe keys from the request.
  require_once DRUPAL_ROOT . '/includes/request-sanitizer.inc';
  DrupalRequestSanitizer::sanitize();
}

...but only after settings have been processed. I suppose if the domain module's bootstrap calls DRUPAL_BOOTSTRAP_VARIABLES before drupal_settings_initialize() returns, then the DrupalRequestSanitizer class would not be available when it's called from _drupal_bootstrap_variables().

Rather than hacking patching core for this, I'd recommend placing the include before the one for domain's settings.inc at the end of settings.php e.g.:

require_once DRUPAL_ROOT . '/includes/request-sanitizer.inc';                      
include DRUPAL_ROOT . '/sites/all/modules/contrib/domain/settings.inc';

...or something along those lines.

In terms of being able to reproduce the error, which of domain's sub-modules do you have enabled? I've tried with just domain itself and domain_conf and have yet to see an issue with requests including a destination param.

johan den hollander’s picture

johan den hollander commented

We experienced a 500 error when deleting a file from the admin/content/file page.
After including the require_once DRUPAL_ROOT . '/includes/request-sanitizer.inc'; part in settings.php as suggested by @mcdruid this error is gone...

mcdruid’s picture

mcdruid commented

I debugged this a little on a site with domain enabled which was hitting a Fatal error for requests which include a destination param, and was eventually able to reproduce the problem on a test site; here's a stack trace:

PHP Fatal error:  Class 'DrupalRequestSanitizer' not found in /data/drupal-7.x/includes/bootstrap.inc on line 2784
PHP Stack trace:
PHP   1. {main}() /data/drupal-7.x/index.php:0
PHP   2. drupal_bootstrap() /data/drupal-7.x/index.php:20
PHP   3. _drupal_bootstrap_configuration() /data/drupal-7.x/includes/bootstrap.inc:2508
PHP   4. drupal_settings_initialize() /data/drupal-7.x/includes/bootstrap.inc:2634
PHP   5. include_once() /data/drupal-7.x/includes/bootstrap.inc:747
PHP   6. include() /data/drupal-7.x/sites/default/settings.php:706
PHP   7. domain_bootstrap() /data/drupal-7.x/sites/all/modules/contrib/domain/settings.inc:26
PHP   8. _domain_bootstrap() /data/drupal-7.x/sites/all/modules/contrib/domain/domain.bootstrap.inc:70
PHP   9. drupal_bootstrap() /data/drupal-7.x/sites/all/modules/contrib/domain/domain.bootstrap.inc:112
PHP  10. _drupal_bootstrap_page_cache() /data/drupal-7.x/includes/bootstrap.inc:2512
PHP  11. drupal_bootstrap() /data/drupal-7.x/includes/bootstrap.inc:2658
PHP  12. _drupal_bootstrap_variables() /data/drupal-7.x/includes/bootstrap.inc:2520

It looks like the problem happens when there's no entry in the registry for the DrupalRequestSanitizer class.

If the class is in the registry:

mysql> SELECT * FROM registry WHERE type='class' AND name='DrupalRequestSanitizer';
+------------------------+-------+--------------------------------+--------+--------+
| name                   | type  | filename                       | module | weight |
+------------------------+-------+--------------------------------+--------+--------+
| DrupalRequestSanitizer | class | includes/request-sanitizer.inc |        |      0 |
+------------------------+-------+--------------------------------+--------+--------+

...it gets autoloaded when it's called in _drupal_bootstrap_variables() despite the fact that the request-sanitizer.inc has not yet been explicitly included/required.

If there's no entry for the class in the registry, we hit a Fatal error.

I was able to reproduce this by removing the entry in the registry (and clearing the lookup_cache from cache_bootstrap). Having done that, a request to my test site which includes a destination param causes the 500 Fatal error as the class fails to autoload.

I'm looking into whether a simple cache clear (or a registry rebuild) is enough to fix this.

mcdruid’s picture

mcdruid commented

I verified that rebuilding the registry (e.g. perhaps with registry_rebuild) so that an entry exists for the DrupalRequestSanitizer class resolves this problem.

It can be a bit of a messy process though (see the project page - make sure you take db backups etc..)

The workaround of explicitly doing a require_once includes/request-sanitizer.inc before the domain module's include in settings.php seems fairly harmless, and may be less painful than trying to ensure the registry contains an entry to allow the class to autoload.

I'd argue this issue should probably move to the domain module's queue, as I don't really think it's core's fault that domain "hotwires" the bootstrap process.

David_Rothstein’s picture

David_Rothstein commented
Title: Patch for SA-CORE-2018-004 issues with the Domain module » SA-CORE-2018-004 combined with the Domain Access module can cause 500 errors on pages that have a destination query parameter
Status: Needs work » Needs review

I added a link to this to the 7.59 release notes in the Known Issues section now.

I think a core patch might be reasonable for this. (Triggering a higher bootstrap phase within the bootstrap process is something that should be expected to work.)

The patch itself looks pretty good to me. Another option might just be to move the existing require_once a little earlier, so that the file is already included before the settings.php file is ever loaded. I don't have a strong preference between the two.

mcdruid’s picture

mcdruid commented
StatusFileSize
new 2966335-9.patch715 bytes
new interdiff-2966335-1-9.txt954 bytes

Ok, so here's a patch which moves the require_once to before the call to _drupal_bootstrap_configuration().

As @David_Rothstein says, there's not much difference between this and the original patch, but this means one require instead of two, and there are already a couple of other files required before the function calls in the switch / case within drupal_bootstrap(), so this is perhaps more consistent.

mcdruid’s picture

mcdruid commented

Just noting that while this issue is worked on, I believe the settings.php workaround remains valid, and it shouldn't break things if a change is committed and released in D7 (obviously it could be removed once it was no longer necessary).

4kant’s picture

4kant commented

In my case ( I got thes errors after updating core to 7.59 ) it was enough to update ctools module (or/and entity module).

mcdruid’s picture

mcdruid commented

@4kant I can't tell for sure, but I suspect if that fixed your site it would have been because your registry was rebuilt, after which the "missing" class would be autoloaded.

That's quite likely to be stopping a lot of sites being affected by this AFAICS.

othermachines’s picture

othermachines commented

We just got a report about this problem. The client was uploading a PDF via the "Files" interface provided by file_entity. As a temporary fix we've modified settings.php as suggested in #4 and the error vanished. Thanks!

4kant’s picture

4kant commented

A supplement to #11: in another project with domain access module it was already clearing the cache that solved it.

sano’s picture

sano commented

I was experiencing this error when attempting to log into a site (as user with uid = 1) with Domain module enabled. The #4 advice solved the problem. Thank you.

drupalganesh’s picture

drupalganesh commented

After this update I am getting this error on every page:
PDOException: SQLSTATE[HY000] [2002] No such file or directory in lock_may_be_available() (line 167 of ../includes/lock.inc).

othermachines’s picture

othermachines commented

Hi, @drupalganech - By "this update" are you referring to the patch in this issue? Otherwise I don't think that it is related. Try: (Community Docs) PDOException: SQLSTATE[HY000] [2002] Can't connect to local MySQL server

Chris Charlton’s picture

Chris Charlton
Primary language English
Location Los Angeles
commented

Anything holding #9 from being RTBC?

mcdruid’s picture

mcdruid commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Drupal 7.64 target

I don't think so :)

pmaruszczyk’s picture

pmaruszczyk commented
Issue tags: -Drupal 7.64 target +Drupal 7.65 target

This path is not included in 7.64 version.

pmaruszczyk’s picture

pmaruszczyk commented
Issue tags: -Drupal 7.65 target +Drupal 7.68 target
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Related issues: +#3088836: Rejuvenate D7, a need for more time spent committing and RTBCing Drupal 7 issues
othermachines’s picture

othermachines commented

Patch still applies cleanly.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Related issues: -#3088836: Rejuvenate D7, a need for more time spent committing and RTBCing Drupal 7 issues
mcdruid’s picture

mcdruid commented
Issue tags: +Pending Drupal 7 commit
fabianx’s picture

fabianx commented

We talked about this:

- IS needs some better explanation
- I‘d like to understand the use case more if we need to do request sanitization before loading settings.php

Explicitly not setting to CNW as the code generally is fine

mcdruid’s picture

mcdruid commented
Issue summary: View changes

Updated IS.

mcdruid’s picture

mcdruid commented
Issue summary: View changes
mcdruid’s picture

mcdruid commented
Issue summary: View changes
mcdruid’s picture

mcdruid commented
Issue summary: View changes
mcdruid’s picture

mcdruid commented
Issue summary: View changes
fabianx’s picture

fabianx commented
Assigned: Unassigned » mcdruid

RTBM - now all is clear! Thank you!

Over to mcdruid for commit.

  • mcdruid committed ac4277f on 7.x 
    Issue #2966335 by mcdruid, dvandijk, David_Rothstein: Avoid...
mcdruid’s picture

mcdruid commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Thank you contributors!

Status: Fixed » Closed (fixed)

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