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.
Comment | File | Size | Author |
---|---|---|---|
#60 | 1739986-60-move-drupal-get-hash-salt.patch | 3.53 KB | dcam |
#58 | 1739986_58.patch | 5.58 KB | andypost |
#55 | 1739986.patch | 5.67 KB | RobLoach |
#47 | 1739986_0.patch | 5.71 KB | RobLoach |
#45 | 1739986.patch | 6.29 KB | RobLoach |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedHere'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.
Comment #2
pwolanin CreditAttribution: pwolanin commentedThis patch also converts the new instance added to core since the other patch got in already.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedSimple refactor.
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedAdd tag
Comment #5.0
moshe weitzman CreditAttribution: moshe weitzman commentedUpdated issue summary.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commented#4: fallback_fix.diff queued for re-testing.
Comment #8
pwolanin CreditAttribution: pwolanin commentedLooks reasonable to me, though I'm not familiar with the APC loader and why it needs the value.
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedThat APC loader related code is not changing here.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedSomeone 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.
Comment #11
mhrabovcin CreditAttribution: mhrabovcin commentedI've reviewed and tested patch on site without salt set in settings.php. Marking as RTBC.
Comment #12
catchLooks good to me. Committed/pushed to 8.x, moving to 7.x for backport.
Comment #13
sunUnfortunately, 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:
Now, let's trace that:
...which rightfully and correctly ends up in this:
So, at minimum, the committed the patch would have to be complemented with this:
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.
Comment #14
sunFor now, I'd strongly recommend to revert this patch from HEAD. The revert applies cleanly, thus:
Comment #15
sunComment #16
moshe weitzman CreditAttribution: moshe weitzman commentedIf 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.Comment #17
sunCan 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.
Comment #18
catchUnless 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.
Comment #19
sunThe problem is a race condition:
1)
_drupal_bootstrap_configuration()
calls_drupal_initialize_db_test_prefix()
2)
_drupal_initialize_db_test_prefix()
calls intodrupal_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 intodrupal_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.Comment #20
sunAdditionally adjusting outdated docs.
Comment #21
moshe weitzman CreditAttribution: moshe weitzman commentedThats fine for now. Thanks Sun.
Please wait for green before commit.
Comment #22
sunTestbot hiccup due to broken HEAD earlier today.
#20 is green: http://qa.drupal.org/pifr/test/390613
Comment #23
sunStill 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.
Comment #24
Dries CreditAttribution: Dries commentedLooks good. Committed to 8.x.
Comment #25
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis reduced the security of Drupal out of the box without any good reason?
The initial committed patch was bogus:
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?
Comment #26
moshe weitzman CreditAttribution: moshe weitzman commentedDamien - 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.
Comment #27
catchThere'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?
Comment #28
BerdirCan't say that I fully understand everything here but a rollback patch is something I can do :)
Comment #30
BerdirWell, apparently I'm not capable of doing that ;)
Let's try again.
Comment #32
Berdir#30: rollback-1739986-30.patch queued for re-testing.
Random upgrade path failure.
Comment #33
catchComment #34
catchOK rolled back for now.
Comment #35
pwolanin CreditAttribution: pwolanin commentedShould we re-roll #2, since that had no impact on the test behavior?
Comment #36
pwolanin CreditAttribution: pwolanin commentedHere'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.
Comment #37
andypostRelated #1555862: DrupalWebTestCase::drupalGetToken() does not add hash salt
Comment #38
RobLoachDid a grep through for drupal_hash_salt, and:
Comment #40
pwolanin CreditAttribution: pwolanin commented#38: 1739986.patch queued for re-testing.
Comment #42
RobLoach#38: 1739986.patch queued for re-testing.
Comment #44
RobLoach#36: 1739986-36.patch queued for re-testing.
[EDIT] Interesting, I'll run a re-roll on this....
Comment #45
RobLoachComment #47
RobLoachComment #48
RobLoach#47: 1739986_0.patch queued for re-testing.
Comment #49
RobLoach#47: 1739986_0.patch queued for re-testing.
Comment #50
pwolanin CreditAttribution: pwolanin commentedLooks fine to me.
Comment #51
RobLoach#47: 1739986_0.patch queued for re-testing.
Comment #52
RobLoachLet's RTBC it then.
Comment #53
xjm#47: 1739986_0.patch queued for re-testing.
Comment #55
RobLoachComment #56
RobLoachBack to RTBC.
Comment #57
alexpottPatch needs a reroll - no longer applies
Comment #58
andypostAPC classloader hunk changed
Comment #59
alexpottCommitted f3966d6 and pushed to 8.x. Thanks!
Comment #60
dcam CreditAttribution: dcam commentedBackported #58 to D7.
Comment #61
andypostGreat!
Comment #62
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/30a95c8
Comment #63
pwolanin CreditAttribution: pwolanin commentedI think I have an outstanding patch to add this sort of functionality to 6.x, so this needs potentially to be backported there too.
Comment #64
jcisio CreditAttribution: jcisio commentedLink to the issue for #63 #590656: Harden one-time login links against vulnerability from disclosure of SQL backups, or SQL 'SELECT' injection.
Comment #64.0
jcisio CreditAttribution: jcisio commentedUpdated issue summary.
Comment #65
sunThe 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).
Comment #66
cilefen CreditAttribution: cilefen commentedI 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.
Comment #71
LuxianDigging old issue here, but run into this problem today and I think I found where the issue is.
Steps to reproduce:
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
insidesettings.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: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.
Comment #72
stephencamilo CreditAttribution: stephencamilo as a volunteer commentedComment #73
hestenetReset issue status.