Problem/Motivation
During review of #2122761: SessionHttpsTest only runs half the tests it was discovered that the $form
variable was populated but never used in several places of SessionHttpsTest
. Close inspection of the matter revealed that this was the case when enforcing a login through one of the http.php
and https.php
mock front-controllers. Nevertheless, the current code should be improved in order to improve readability.
Proposed resolution
Let's take a second look on SessionHttpsTest
and clean it up:
- Remove usage of the globals
$base_secure_url
and$base_insecure_url
frombootstrap.inc
- Remove the globals
$is_http_mock
and$is_https_mock
. - Turn
$secure_session_name
and$insecure_session_name
into instance variables onSessionHttpsTest
and populate them duringsetUp()
. - Extract the new helper methods
SessionHttpsTest::loginHttp()
andSessionHttpsTest::loginHttps()
in order to make the test-code more self-documenting - Make
testHttpsSession()
work when the test runs in an HTTPS environment by consequently usinghttpUrl()
andhttpsUrl()
if necessary. - Fix redirect to non-existing URL after POST requests (see #9)
Remaining tasks
Review.
User interface changes
None.
API changes
None.
History of the SessionHttpsTest class
The SessionHttpsTest
was introduced by #1577: Mapping privilege separation on non-SSL/SSL (opened in 2003 and committed in 2009) along with most of the SSL mixed mode functionality. The patch brought in the globals $is_https
, $base_secure_url
, $base_insecure_url
and the variable $conf['https']
. Also the https.php mock was added and in order to enforce redirects to go through HTTP from within a mocked HTTPS request, hook_goto_alter was introduced as well. In order to be able to store both, secure and insecure session ids, the session table got an additional column (ssid).
Subsequently it was discovered that sites operating in mixed mode SSL were vulnerable to an impersonation attack #575280: Impersonation when an https session exists.. In addition to fixing the vulnerability test coverage was extended. Special attention went into making the tests work when the test-runner was a HTTPS environment. In order to make this possible, the http.php mock front-controller was added.
Also noteworthy is #813492: HTTPS sessions use an invalid merge query because it intermittedly roled back changes introduced in #575280: Impersonation when an https session exists. and also left a bogus assortion which is still found in current code: The headers argument is third in drupalGet()
, not second. The assertion fails if the parameters are correct (opened #2330279: When operating in mixed mode SSL: data from anonymous non-HTTP session is still available after login).
// Test that session data saved before login is not available using the
// pre-login anonymous cookie.
$this->cookies = array();
$this->drupalGet('session-test/get', array('Cookie: ' . $anonymous_cookie));
$this->assertNoText($session_data, 'Initial anonymous session is inactive after login.');
The redirect bug (see #9) was introduced in #1668866: Replace drupal_goto() with RedirectResponse. The problem is that RedirectResponse::getTargetUrl()
returns an absolute URL. However due to the fact that SessionHttpsTest did not examine response HTTP status, and probably due to #2122761: SessionHttpsTest only runs half the tests this slipped through the cracks.
Beta phase evaluation
Issue category | Bug |
---|---|
Issue priority | Major because it affects security related end-to-end tests |
Unfrozen changes | Unfrozen because it only changes tests. (SessionHttpsTest) |
Prioritized changes | The main goal of this issue is security. |
Comment | File | Size | Author |
---|---|---|---|
#66 | 2265099-66.diff | 14.32 KB | znerol |
#66 | interdiff.txt | 831 bytes | znerol |
#60 | 2265099-60.patch | 15.13 KB | rpayanm |
Comments
Comment #1
znerol CreditAttribution: znerol commented::http[s]Url()
is used wherever its necessary::testMixedModeSslSession()
work on HTTPS test environment::setUp()
.Comment #2
znerol CreditAttribution: znerol commented$base_insecure_url
and$base_secure_url
::http[s]Url()
.Comment #3
znerol CreditAttribution: znerol commentedExtract inlined login code via mock http/https to dedicated methods. This reveals two different uses of
$form[0]['action']
:drupalPost(NULL, ...);
will submit to one of the mock front controllers (user login form)That essentially addresses the concern raised by @dawehner in #2122761-9: SessionHttpsTest only runs half the tests:
Comment #5
znerol CreditAttribution: znerol commentedFix missing
$path
variable inSessionTestSubscriber
.Comment #7
znerol CreditAttribution: znerol commentedComment #8
znerol CreditAttribution: znerol commentedNo need to enforce authentication from within session test request-subscriber.
Comment #9
znerol CreditAttribution: znerol commentedDo not tamper with redirect responses inside the system under test. Instead assign the responsibility of modifying the protocol of the location header appropriately to the test case itself.
This also fixes a problem with the current https test where the browser was redirected to an in-existing URL after form submission. However this went unnoticed because the HTTP status of the final response is not checked (see screenshot):
Comment #10
znerol CreditAttribution: znerol commentedComment #11
znerol CreditAttribution: znerol commentedReroll.
Comment #12
znerol CreditAttribution: znerol commentedComment #13
znerol CreditAttribution: znerol commentedReroll. I'm bumping this to critical because this fixes a serious defect in an important test (security-wise).
Comment #15
cosmicdreams CreditAttribution: cosmicdreams commentedThis line is suspect as we're concatenating 'index.php' to $base_url but we're not storing that result. Effectively this does nothing.
Does the fact that it does nothing break assumptions of what this test is doing? Looks like the following functions may all be wrong as a result;
loginHttp
loginHttps
testCsrfTokenWithMixedModeSsl
Not a full answer but curious.
Even though cookies is a sensical name for this. There isn't any container named cookies in this class or the base class WebTestBase.
however this is a cookieFile. Should that be used?
Comment #16
dawehner@znerol
Do you think you can fix the failures? Would be great to not have the security problem for too long.
Comment #17
znerol CreditAttribution: znerol commentedYes, in the meantime I discovered some additional problems with the tests. My working copy is still a big mess though, I'll try to put together a patch soon.
Comment #18
znerol CreditAttribution: znerol commentedFixes the bad reroll from #19.
Note that the patch still contains bugs, e.g.:
Should be
httpsUrl()
.Also issues pointed out in #15 not yet addressed.
Comment #19
znerol CreditAttribution: znerol commentedComment #20
znerol CreditAttribution: znerol commentedThis fixes the obvious bugs pointed out in #15 and #19. Regrettably I still see failures when the test-runner is a HTTPS environment. Will have to set-up wireshark in order to understand what is going on there.
Comment #22
znerol CreditAttribution: znerol commentedComment #23
cosmicdreams CreditAttribution: cosmicdreams commentedYay! fewer globals.
Does nulling out the curlCookies make more sense inside the curlClose(); function?
Is this something we want to optionally do?
why are we removing priority?
Comment #24
znerol CreditAttribution: znerol commentedAdded some historical information i digged up from old issues (
git log --patch
ftw).Comment #25
znerol CreditAttribution: znerol commentedComment #26
cosmicdreams CreditAttribution: cosmicdreams commentedOK, in reviewing the interdiffs it appears that the priority ensured that the tests get access to specialized logic that was altering the request. It appears that this was a special case that the original developers introduced because they thought it was necessary. Apparently it isn't anymore so great. This patch seems RTBC to me.
Comment #27
znerol CreditAttribution: znerol commented@cosmicdreams: Give me some time, there is still commented out code in the patch:
Also I like to check whether we can get rid of the
curlClose()
calls now that cookies are managed manually (viacurlCookies
property).Comment #28
znerol CreditAttribution: znerol commentedOpened #2330279: When operating in mixed mode SSL: data from anonymous non-HTTP session is still available after login.
Comment #29
znerol CreditAttribution: znerol commentedRegrettably this does not have the desired effect. In fact it is not possible to disable the internal cookie handling mechanism of curl after either
CURLOPT_COOKIEJAR
orCURLOPT_COOKIEFILE
has been set to any value. Therefore the only way to get rid of cookies which were picked up by curl during previous requests is closing the curl handle.Comment #30
znerol CreditAttribution: znerol commentedSwitching back to curl built-in cookie handling. Cleaned up the patch such that it is hopefully review-worthy now.
Comment #31
xjmThanks @znerol!
Downgrading to major based on discussion with @znerol; this is important but not release-blocking, and there isn't an actual vulnerability here, just a need for hardening and better test coverage for safe refactoring later.
Comment #32
znerol CreditAttribution: znerol commentedReroll after #2330279: When operating in mixed mode SSL: data from anonymous non-HTTP session is still available after login landed in core. This version now successfully runs all tests regardless of whether the test-runner is HTTP or HTTPS.
Comment #33
znerol CreditAttribution: znerol commentedReroll.
Comment #37
znerol CreditAttribution: znerol commentedComment #38
znerol CreditAttribution: znerol commentedComment #39
znerol CreditAttribution: znerol commentedStackMiddleware\Session
obviously does not belong into this issue.Comment #40
mgiffordThanks @znerol for all your work on this!
What needs to be tested to mark this RTBC.
I do agree that
* An absolute URL.
isn't very useful. But I do wonder if this could be shortened to a single line:Say "Absolute URL prepared for the https.php mock front controller"?
Comment #41
znerol CreditAttribution: znerol commentedManual testing of the patch should be performed like this:
SessionHttpsTest
from the simpletest UI (once through http, once through https)SessionHttpsTest
from the CLI:Comment #42
znerol CreditAttribution: znerol commentedIn fact it is not an absolute URL at all but a relative one. The function simply prepends the
$url
parameter with the path to the mock frontcontroller:return 'core/modules/system/tests/http.php/' . $url;
.Comment #43
mgiffordI think this is longer and less clear than it needs to be "The URL prepared in a way such that the https.php mock front controller is used."
How about something like:
"URL prepared for the https.php mock front controller."
Comment #44
znerol CreditAttribution: znerol commentedFixed.
Comment #45
dawehnerAre we sure this is not a critical issue as it seems to block #2283637: Provide test coverage to prove that an AuthenticationProvider can initiate a session ?
Oh, that is great for itself!
For sure that is part of another issue, but is there a reason we can't create a request object suited for the HTTP and one for the HTTPs usecase?
Can we do this changes because we drop mix mode?
It would be great to maybe have a short documentation line telling what is going on: We disable the automatic redirect, in order to do what?...
Can't you also use $this->container->get('url_generator')->generateFromRoute('', [], ['absolute' => TRUE]);?
Comment #46
znerol CreditAttribution: znerol commentedSee #31, but it should be fixed nevertheless rather sooner than later.
$this->insecureSessionName
and$this->$secureSessionName
.loginHttp
andloginHttps
methods. Note that even if mixed mode sessions are removed, we still need to test the characteristics of the resulting cookies when logging in via HTTP vs HTTPS.Comment #48
znerol CreditAttribution: znerol commentedRe #45.5, empty route name results in route not found, need explicitly supply
<front>
.Comment #49
znerol CreditAttribution: znerol commentedReroll after #2342593: Remove mixed SSL support from core.
Comment #54
mgiffordWhat was I doing trying to test a diff???? Anyways, got the patch in #48.
Comment #56
neclimdulJust a re-roll
Comment #59
almaudoh CreditAttribution: almaudoh commentedComment #60
rpayanmComment #61
Fabianx CreditAttribution: Fabianx commentedGonna be bold. It looks great, RTBC!
Needs a beta evaluation (unfrozen: tests-only) and a CR addition to remove $base_url and $base_secure_url. (sniff, I liked those ...)
Comment #62
znerol CreditAttribution: znerol commentedJust to be clear, it does not remove
$base_url
, the issue summary is still accurate. I do not think that the removed globals are API because they were only used for tests in core. Which CR needs an update?Comment #63
znerol CreditAttribution: znerol commentedComment #64
alexpottThese have been around since d7 - contrib code might be relying on them - we should update a CR or provide a new one. It would be great if we can get rid of DrupalKernel::initializeRequestGlobals() - is there an issue to do that? We could just remove the removals from this issue and do the cleanup there.
Comment #65
znerol CreditAttribution: znerol commentedWhat about
$is_http_mock
and$is_https_mock
? Do we need a CR update for them?Comment #66
znerol CreditAttribution: znerol commentedComment #67
alexpott@znerol for
$is_http_mock
and$is_https_mock
- i don't think so - that is truly just test stuff for the tests you are changing.Comment #68
znerol CreditAttribution: znerol commentedComment #69
Fabianx CreditAttribution: Fabianx commentedContrib code like securepages / secureforms / ... indeed tries to rely on those super globals, however it should be fine to remove them with a proper CR, etc.
Thanks for restoring for now.
Back to RTBC
Comment #70
alexpottThis is a test only change and therefore permitted in beta. Committed d29700f and pushed to 8.0.x. Thanks!