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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Title: drupal_get_hash_salt shoul dbe moved to bootstrap.inc and used instead of $GLOBALS['drupal_hash_salt'] » drupal_get_hash_salt should be moved to bootstrap.inc and used instead of $GLOBALS['drupal_hash_salt']
Status: Active » Needs review
FileSize
3.62 KB

Here's a fix for existing core.

I the added use from #1675260: Implement PHP reading/writing secured against "leaky" script gets in before this, this should be re-rolled.

pwolanin’s picture

FileSize
4.04 KB

This patch also converts the new instance added to core since the other patch got in already.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Simple refactor.

moshe weitzman’s picture

Title: drupal_get_hash_salt should be moved to bootstrap.inc and used instead of $GLOBALS['drupal_hash_salt'] » Fix fallback in drupal_get_hash_salt(), move it to bootstrap.inc, use instead of $GLOBALS['drupal_hash_salt']
Assigned: Unassigned » moshe weitzman
Status: Reviewed & tested by the community » Needs review
FileSize
772 bytes
4.12 KB

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.

moshe weitzman’s picture

Issue tags: +Needs backport to D7

Add tag

moshe weitzman’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, fallback_fix.diff, failed testing.

moshe weitzman’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7

#4: fallback_fix.diff queued for re-testing.

pwolanin’s picture

Looks reasonable to me, though I'm not familiar with the APC loader and why it needs the value.

moshe weitzman’s picture

That APC loader related code is not changing here.

moshe weitzman’s picture

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.

mhrabovcin’s picture

Status: Needs review » Reviewed & tested by the community

I've reviewed and tested patch on site without salt set in settings.php. Marking as RTBC.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Looks good to me. Committed/pushed to 8.x, moving to 7.x for backport.

sun’s picture

Version: 7.x-dev » 8.x-dev
Priority: Normal » Critical
Status: Patch (to be ported) » Active

Unfortunately, this patch completely broke tests for me.

I actually have no idea how this patch was able to pass tests in the first place, due to this:

+function drupal_get_hash_salt() {
...
+  return empty($drupal_hash_salt) ? hash('sha256', serialize(Database::getConnectionInfo('default'))) : $drupal_hash_salt;

Now, let's trace that:

drupal_bootstrap()
_drupal_bootstrap_configuration()
_drupal_initialize_db_test_prefix()
drupal_valid_test_ua()
drupal_get_hash_salt()

...which rightfully and correctly ends up in this:

Fatal error: Class 'Drupal\Core\Database\Database' not found in core\includes\bootstrap.inc on line 2007

So, at minimum, the committed the patch would have to be complemented with this:

diff --git a/core/includes/bootstrap.inc b/core/includes/bootstrap.inc
index ef53eaf..e261ab3 100644
--- a/core/includes/bootstrap.inc
+++ b/core/includes/bootstrap.inc
@@ -2256,12 +2256,12 @@ function _drupal_bootstrap_configuration() {
   // Initialize the configuration, including variables from settings.php.
   drupal_settings_initialize();
 
-  // Make sure we are using the test database prefix in child Drupal sites.
-  _drupal_initialize_db_test_prefix();
-
   // Activate the class loader.
   drupal_classloader();
 
+  // Make sure we are using the test database prefix in child Drupal sites.
+  _drupal_initialize_db_test_prefix();
+
   // Load the procedural configuration system helper functions.
   require_once DRUPAL_ROOT . '/core/includes/config.inc';
 }

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.

sun’s picture

For now, I'd strongly recommend to revert this patch from HEAD. The revert applies cleanly, thus:

git revert a13f75750040ea720c1597a4d659ff7be743fa2f
git commit
git push
sun’s picture

Status: Active » Reviewed & tested by the community
moshe weitzman’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

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.

sun’s picture

Priority: Normal » Critical

Can you provide a patch?

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.

catch’s picture

Unless there's a patch here in the next day or so I'd want to roll this back (or even earlier), however there's now conflicts on the revert so I haven't done so for now.

sun’s picture

Status: Needs work » Needs review
FileSize
1.44 KB

The problem is a race condition:

1) _drupal_bootstrap_configuration() calls _drupal_initialize_db_test_prefix()

2) _drupal_initialize_db_test_prefix() calls into drupal_valid_test_ua()

3) _drupal_initialize_db_test_prefix()'s purpose is to change the database default connection info.

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.

Thus, attached patch removes that database default connection info fallback entirely.

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.

sun’s picture

FileSize
3.03 KB

Additionally adjusting outdated docs.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thats fine for now. Thanks Sun.

Please wait for green before commit.

sun’s picture

Testbot hiccup due to broken HEAD earlier today.

#20 is green: http://qa.drupal.org/pifr/test/390613

sun’s picture

Still need to apply this patch for every other issue I'm working on (and remember to revert it)...

I administratively sent #20 for re-test to hopefully get a proper test result, but as said, it was green before already.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks good. Committed to 8.x.

Damien Tournoud’s picture

Status: Fixed » Active

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.

Why not just reverting the initial patch?

moshe weitzman’s picture

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.

catch’s picture

There's 27 open critical bugs, so I'd like to roll back the committed patches and start again here. Anyone up for posting a rollback patch?

Berdir’s picture

Status: Active » Needs review
FileSize
4.17 KB

Can't say that I fully understand everything here but a rollback patch is something I can do :)

Status: Needs review » Needs work

The last submitted patch, rollback-1739986-28.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.61 KB

Well, apparently I'm not capable of doing that ;)

Let's try again.

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, rollback-1739986-30.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7

#30: rollback-1739986-30.patch queued for re-testing.

Random upgrade path failure.

catch’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Active

OK rolled back for now.

pwolanin’s picture

Should we re-roll #2, since that had no impact on the test behavior?

pwolanin’s picture

Status: Active » Needs review
FileSize
5.19 KB

Here's a re-roll of #2. The bit about the PHP Stoage seems to have moved out of booststrap.inc, but I think the function will still be available.

andypost’s picture

RobLoach’s picture

FileSize
6.29 KB

Did a grep through for drupal_hash_salt, and:

  1. Removed its reference in drupal_settings_initialize() since it wasn't actually being used
  2. Updated some of the documentation around it in drupal_get_token()

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, 1739986.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review

#38: 1739986.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1739986.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review

#38: 1739986.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1739986.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7

#36: 1739986-36.patch queued for re-testing.

[EDIT] Interesting, I'll run a re-roll on this....

RobLoach’s picture

FileSize
6.29 KB

Status: Needs review » Needs work

The last submitted patch, 1739986.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
5.71 KB
RobLoach’s picture

Issue tags: -Needs backport to D7

#47: 1739986_0.patch queued for re-testing.

RobLoach’s picture

Issue tags: +Needs backport to D7

#47: 1739986_0.patch queued for re-testing.

pwolanin’s picture

Looks fine to me.

RobLoach’s picture

#47: 1739986_0.patch queued for re-testing.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Let's RTBC it then.

xjm’s picture

Issue tags: -Needs backport to D7

#47: 1739986_0.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, 1739986_0.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
5.67 KB
RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Patch needs a reroll - no longer applies

andypost’s picture

Status: Needs work » Needs review
FileSize
5.58 KB

APC classloader hunk changed

alexpott’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Patch (to be ported)

Committed f3966d6 and pushed to 8.x. Thanks!

dcam’s picture

Assigned: moshe weitzman » Unassigned
Status: Patch (to be ported) » Needs review
FileSize
3.53 KB

Backported #58 to D7.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Great!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.23 release notes
pwolanin’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

I think I have an outstanding patch to add this sort of functionality to 6.x, so this needs potentially to be backported there too.

jcisio’s picture

jcisio’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Version: 6.x-dev » 7.x-dev
Priority: Normal » Critical
Issue summary: View changes
Status: Patch (to be ported) » Needs work

The committed change broke Simpletest:

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

cilefen’s picture

Component: base system » simpletest.module
Status: Needs work » Postponed (maintainer needs more info)
Issue tags: -Needs backport to D7

I am moving this to the simpletest component to get the maintainer's word on whether #65 is still an issue.

@boombatower Is this still an issue? I can't find much corresponding code looking at #19 and I don't know how to verify it.

  • catch committed a13f757 on 8.3.x
    Issue #1739986 by pwolanin, moshe weitzman: Fixed fallback in...
  • Dries committed 7061f3b on 8.3.x
    - Issue #1739986 by pwolanin, sun, moshe weitzman: Fixed fallback in...
  • catch committed 8d029e8 on 8.3.x
    Issue #1739986 by Berdir: Rolling back this issue to start again.
    
  • alexpott committed f3966d6 on 8.3.x
    Issue #1739986 by RobLoach, pwolanin, sun, moshe weitzman, andypost: Fix...

  • catch committed a13f757 on 8.3.x
    Issue #1739986 by pwolanin, moshe weitzman: Fixed fallback in...
  • Dries committed 7061f3b on 8.3.x
    - Issue #1739986 by pwolanin, sun, moshe weitzman: Fixed fallback in...
  • catch committed 8d029e8 on 8.3.x
    Issue #1739986 by Berdir: Rolling back this issue to start again.
    
  • alexpott committed f3966d6 on 8.3.x
    Issue #1739986 by RobLoach, pwolanin, sun, moshe weitzman, andypost: Fix...

  • catch committed a13f757 on 8.4.x
    Issue #1739986 by pwolanin, moshe weitzman: Fixed fallback in...
  • Dries committed 7061f3b on 8.4.x
    - Issue #1739986 by pwolanin, sun, moshe weitzman: Fixed fallback in...
  • catch committed 8d029e8 on 8.4.x
    Issue #1739986 by Berdir: Rolling back this issue to start again.
    
  • alexpott committed f3966d6 on 8.4.x
    Issue #1739986 by RobLoach, pwolanin, sun, moshe weitzman, andypost: Fix...

  • catch committed a13f757 on 8.4.x
    Issue #1739986 by pwolanin, moshe weitzman: Fixed fallback in...
  • Dries committed 7061f3b on 8.4.x
    - Issue #1739986 by pwolanin, sun, moshe weitzman: Fixed fallback in...
  • catch committed 8d029e8 on 8.4.x
    Issue #1739986 by Berdir: Rolling back this issue to start again.
    
  • alexpott committed f3966d6 on 8.4.x
    Issue #1739986 by RobLoach, pwolanin, sun, moshe weitzman, andypost: Fix...
Luxian’s picture

Digging old issue here, but run into this problem today and I think I found where the issue is.

Steps to reproduce:

  1. Simpletest does a POST request to a page A (1st request)
  2. page A is calling page B using drupal_http_request() (2nd request)

Both request will have a Simpletest specific User-Agent (UA), but the second one will be invalid making 2nd request run using default database connection.

UA for Simpletest relies on hash salt. For 1st request drupal_get_hash_salt() will use variable $databases that contains only the original connection, but for the second request it will contain the test database connection so the hash will be different. During bootstrap for page B database test UA will not be validated because hash is compared before $databases is updated with test connection.

As a workaround, you can define $drupal_hash_salt inside settings.php and then tests will always use the same hash salt as the main website and not rely on $databases.

To fix this problem drupal_get_hash_salt() should behave consistent between test and main website. To achieve this I see two options:
- Use static cache for hash salt, and then make sure the static cache is always generated before $databases is updated with test database credentials
- Overwrite $drupal_hash_salt for test instances inside _drupal_bootstrap_database() before changing $databases and save it in the global variable like this:

<?php 

  // The user agent header is used to pass a database prefix in the request when
  // running tests. However, for security reasons, it is imperative that we
  // validate we ourselves made the request.
  if ($test_prefix = drupal_valid_test_ua()) {
    $GLOBALS['drupal_hash_salt'] = drupal_get_hash_salt(); // THIS WILL FIX IT, BUT FEELS UGLY

    // Set the test run id for use in other parts of Drupal.
    $test_info = &$GLOBALS['drupal_test_info'];
    $test_info['test_run_id'] = $test_prefix;
    $test_info['in_child_site'] = TRUE;

I might be able to provide a patch, but before working on it I want to get some feedback to make sure I'm not missing something.

stephencamilo’s picture

Status: Postponed (maintainer needs more info) » Closed (won't fix)
hestenet’s picture

Status: Closed (won't fix) » Postponed (maintainer needs more info)

Reset issue status.