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.

Files: 
CommentFileSizeAuthor
#60 1739986-60-move-drupal-get-hash-salt.patch3.53 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 40,429 pass(es).
[ View ]
#58 1739986_58.patch5.58 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,276 pass(es).
[ View ]
#55 1739986.patch5.67 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 53,590 pass(es).
[ View ]
#47 1739986_0.patch5.71 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1739986_0_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#45 1739986.patch6.29 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#38 1739986.patch6.29 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#36 1739986-36.patch5.19 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 52,140 pass(es).
[ View ]
#30 rollback-1739986-30.patch4.61 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,024 pass(es).
[ View ]
#28 rollback-1739986-28.patch4.17 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#20 drupal8.hash-salt.20.patch3.03 KBsun
PASSED: [[SimpleTest]]: [MySQL] 48,789 pass(es).
[ View ]
#19 drupal8.hash-salt.19.patch1.44 KBsun
Test request sent.
[ View ]
#4 fallback_fix.diff4.12 KBmoshe weitzman
PASSED: [[SimpleTest]]: [MySQL] 46,394 pass(es).
[ View ]
#4 interdiff.txt772 bytesmoshe weitzman
#2 1739986-2.patch4.04 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 40,645 pass(es).
[ View ]
#1 1739986-1.patch3.62 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 40,250 pass(es).
[ View ]

Comments

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
StatusFileSize
new3.62 KB
PASSED: [[SimpleTest]]: [MySQL] 40,250 pass(es).
[ View ]

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.

StatusFileSize
new4.04 KB
PASSED: [[SimpleTest]]: [MySQL] 40,645 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Simple refactor.

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
StatusFileSize
new772 bytes
new4.12 KB
PASSED: [[SimpleTest]]: [MySQL] 46,394 pass(es).
[ View ]

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.

Issue tags:+needs backport to D7

Add tag

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.

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

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

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

That APC loader related code is not changing here.

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.

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.

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.

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.

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

git revert a13f75750040ea720c1597a4d659ff7be743fa2f
git commit
git push

Status:Active» Reviewed & tested by the community

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.44 KB
Test request sent.
[ View ]

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.

StatusFileSize
new3.03 KB
PASSED: [[SimpleTest]]: [MySQL] 48,789 pass(es).
[ View ]

Additionally adjusting outdated docs.

Status:Needs review» Reviewed & tested by the community

Thats fine for now. Thanks Sun.

Please wait for green before commit.

Testbot hiccup due to broken HEAD earlier today.

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

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.

Status:Reviewed & tested by the community» Fixed

Looks good. Committed to 8.x.

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?

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.

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?

Status:Active» Needs review
StatusFileSize
new4.17 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new4.61 KB
PASSED: [[SimpleTest]]: [MySQL] 50,024 pass(es).
[ View ]

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.

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

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

Random upgrade path failure.

Status:Needs review» Reviewed & tested by the community

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

OK rolled back for now.

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

Status:Active» Needs review
StatusFileSize
new5.19 KB
PASSED: [[SimpleTest]]: [MySQL] 52,140 pass(es).
[ View ]

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.

StatusFileSize
new6.29 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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.

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.

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

StatusFileSize
new6.29 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1739986_0_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Issue tags:-needs backport to D7

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

Issue tags:+needs backport to D7

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

Looks fine to me.

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

Status:Needs review» Reviewed & tested by the community

Let's RTBC it then.

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.

Status:Needs work» Needs review
StatusFileSize
new5.67 KB
PASSED: [[SimpleTest]]: [MySQL] 53,590 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

Status:Reviewed & tested by the community» Needs work

Patch needs a reroll - no longer applies

Status:Needs work» Needs review
StatusFileSize
new5.58 KB
PASSED: [[SimpleTest]]: [MySQL] 54,276 pass(es).
[ View ]

APC classloader hunk changed

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

Committed f3966d6 and pushed to 8.x. Thanks!

Assigned:moshe weitzman» Unassigned
Status:Patch (to be ported)» Needs review
StatusFileSize
new3.53 KB
PASSED: [[SimpleTest]]: [MySQL] 40,429 pass(es).
[ View ]

Backported #58 to D7.

Status:Needs review» Reviewed & tested by the community

Great!

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

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.

Issue summary:View changes

Updated issue summary.

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