You are here

Fix fallback in drupal_get_hash_salt(), move it to bootstrap.inc, use instead of $GLOBALS['drupal_hash_salt']

There are now several instances in boostrap.inc that use $GLOBALS['drupal_hash_salt'] - apparently we don't want to call drupal_get_hash_salt() because that function is in common.inc. However, if an admin accidentally left the variable empty, this could lead to the site being vulnerabile to some attacks this value is meant to protect against. Thus, we should move the function to bootstrap.inc and use it everywhere instead of the global.

In addition, we use Database::getConnectionInfo('default') instead of $databases in drupal_get_hash_salt() since $databases reflects the parent site, not the system-under-test when in a Simpletest run. See DrupalWebTestCase::changeDatabasePrefix.

I built upon this hardening patch to fix a bug in drupal_get_hash_salt(). That function falls back to $GLOBALS['databases'] when no $drupal_has_salt is defined in settings.php. Problem is that $databases points to the parent site not the System-Under-test during a simpletest run. So we switch to using Database::getConnectionInfo('default') which does reflect the new database prefix that simpletest has injected during setUp(). The new code does not use additional DB connections when making a salt but thats not needed at all.

Since my bug fix really wants to build upon the refactor here, I've rolled them into one patch and provided an interdiff.

Someone is bound to ask about tests. We already have a test for this and the test already fails. The "problem" is that we don't currently ask testbot to run the suite without a salt. I don't think we need to, so IMO no new tests are required.

I would totally have supplied that as a follow-up patch, but it does not resolve the brokenness of tests.

What effectively seems to happen is that web tests are running against the parent site, because drupal_valid_test_ua() does not evaluate to TRUE for requests issued by the internal browser of WebTestBase.

I was struggling with this for a couple of days now and wrongly assumed it was a problem with my existing D8 dev site until I re-installed today and still got that fatal error.

If the bot is happy, there is no reason to rush a rollback. Also rollback does not sound like a proper fix for What effectively seems to happen is that web tests are running against the parent site, because drupal_valid_test_ua() does not evaluate to TRUE for requests issued by the internal browser of WebTestBase.. Lets fix the core problem.

To clarify once more: This prevents me from running any web tests on HEAD currently. Neither through Simpletest (UI) nor run-tests.sh. Running a (web) test is only possible by reverting the changes of the git commit right now.

4) But the call to drupal_valid_test_ua() calls into drupal_get_hash_salt(), which tests against the database default connection info in settings.php.

5) Consequently, the drupal_valid_test_ua() condition in _drupal_initialize_db_test_prefix() can never evaluate to TRUE, since it is _drupal_initialize_db_test_prefix() that is supposed to change the database connection info.

I actually don't see why we have that fallback value in the first place — if it happens to be empty, then the only impact is that the generated final hashes of the callers are minimally less secure, but that's really all about it.

drupal_valid_test_ua() itself adds additional entropy by adding filesystem information for /core/includes/bootstrap.inc to the final hash, so that's sufficiently secure either way.

It also corrects the order in which functions are being called in _drupal_bootstrap_configuration() - even though the current problem is eliminated, the classloader really needs to be registered first.

This reduced the security of Drupal out of the box without any good reason?

The initial committed patch was bogus:

I built upon this hardening patch to fix a bug in drupal_get_hash_salt(). That function falls back to $GLOBALS['databases'] when no $drupal_has_salt is defined in settings.php. Problem is that $databases points to the parent site not the System-Under-test during a simpletest run.

This was by design. The salt is actually used in the simpletest UA signature, so it needs to be the same in the site-under-test and in the parent site.

Damien - rollback would not fix the bug where CSRF token related tests fail when settings.php has not supplied any salt. Drupal is designed to to run under this condition but it doesn't pass its own tests. Sorry I was not clear in #4.

The simpletest database prefix is not applied for HTTP requests from the child-site to the child-site, whose page callback/route handler is performing a HTTP request from the child-site to the child-site.

In other words:

1) Simpletest → 2) POST /node/add → 3) POST /mymodule/handler

3) runs against the unprefixed database of the parent site.

Based on a cursory look, the revised approach that has been committed forgot to account for #19.

I had to bisect the 7.x history in order to pinpoint the problem to exactly this commit:
ba644cdc passes
30a95c80 fails

Noteworthy: Possibly caused by revised handling of variables/references in PHP 5.4. PIFR/qa.d.o does not yield the test failures (yet).