Impersonation when an https session exists.

Additional information from Mark (send to the security team, but can & should be fixed publicly):

I found that running Drupal 7 on a HTTPS URL and logging in results in a session being saved to the sessions table with an empty string session identifier. Once this happens, if the site is also available on a HTTP URL, then anyone can pass an empty string session cookie to the HTTP URL e.g. curl --cookie SESS96efe38290907de812869e22ea7c7c62= http://... and make any request with the privileges of that initial HTTPS login.

I suggest we replace isset($_COOKIE[$insecure_session_name]) by !empty($_COOKIE[$insecure_session_name]) in _drupal_session_read(). The true bug is _drupal_session_read() now accept empty session ids ;)

Given that this is security related, some extra code comments would be nice.

We don't want the session to be accessible by HTTP when generated from HTTPS, so I'm not convinced the fix makes sense. The real bug is accepting an empty session id.

We should ignore the $_COOKIE[$insecure_session_name] on an HTTPS site by default. The reason is that the insecure cookie is, as it says "insecure". And could thus be set (fixated) by a man-in-the-middle on the HTTP site. The contents of this cookie would then be saved into the session table when a user logged in via HTTPS, and then could be used by the man-in-the-middle to gain authenticated access. This is a type of scenario that only allowing logins to the HTTPS site should be able to protect against.

Not sure what you mean. We don't do that. When the user logs in on the HTTPS site, his session will get regenerated. We only use the "insecure" cookie to maintain session data from anonymous (browsing the HTTP site) to authenticated (on both the HTTP and HTTPS site).

Hm, wait. If you are running solely on HTTPS why would the new session code kick in?

When a user logs in on the secure site, the session is created with an empty string sid. <= again wait a second. I think I fought a bit that this can not happen, you never get an empty sid?

What's not clear from this issue and took a long chat session to clarify: the bug appears if you have a mixed HTTP-HTTPS site and the API is off.
db_merge('sessions')->key($key)->fields($fields)->execute(); needs to multiple lines like the original. $GLOBALS['is_https_mock'] GLOBALS is unnecessary as this code is already in the global scope. Wow, array_fill_keys that's new, first time I see this function, handy.

// Alter the redirect when using a mock HTTPS request. this needs to be // Alter the redirect when using a mock HTTPS request and not when on real HTTPS because...

If you need to skip a test because of a PHP bug when the testing environment is on HTTPS, please do it. If there is no bugs.php.net report then please create one and link to it.

And thanks a ton for the great work on this.

I just saw this issue, and I'm happy to report that it doesn't conflict at all with #607008: Fix bugs in https support and force using https for authorize.php if available -- neither in code nor in functionality. Yay. ;) Tested authorize.php in the mixed http/https case with this applied, all is still well. I read over the patch -- great work on the tests! All look sane and reasonable. Patch fixes an important bug, code is clean and happy. RTBC!

+++ includes/session.inc	28 Oct 2009 19:50:25 -0000
@@ -151,12 +151,18 @@ function _drupal_session_write($sid, $va
+    // If the "secure pages" setting is enabled, use the insecure session
+    // identifier as the sid.

Wait. What? That makes no sense. :) If secure pages is /enabled/, don't I want to use the /secure/ session?

Could you please clarify this comment?

+++ modules/simpletest/tests/session.test	28 Oct 2009 19:50:25 -0000
@@ -272,18 +272,61 @@ class SessionHttpsTestCase extends Drupa
+    $this->drupalGet('user');
+    $form = $this->xpath('//form[@id="user-login"]');
+    $form[0]['action'] = $this->httpsUrl('user');
...
+    $this->drupalGet('user');
+    $form = $this->xpath('//form[@id="user-login"]');
+    $form[0]['action'] = $this->httpsUrl('user');

This code looks really odd. Could we just drupalGet('user/login') instead?

Then just as a general note, there is a ton of really useful discussion here that we should make sure is reflected in either the code comments or the tests. I'm not sure the importance of this fix is properly communicated, yet.

I'm on crack. Are you, too?

if you have a https only site you need no api, it Just Works. Or it worked ;) anyways, it's kinda obvious that this API is for mixed HTTP-HTTPS because a HTTPS site is slow. Very very very slow. Unless you have a hardware SSL accelerator which costs mucho money.

Back to the green queue :)

Cool, thank you!

Committed to HEAD.

After this issue was committed, my PIFR 2 client threw 3 exceptions on SessionHttpsTestCase.

http://testing.drupal.org/pifr-2.x/pifr/test/16137

I'll leave the client failure up for a couple hours. Screenshot of specifics attached as well.

I'll mark this fixed again. I think I have an idea about what might be going on.

The tests fail if a non-https service is running on port 443, like a tor bridge. I guess that's probably not a worthy reason for re-opening this issue.

None of the automated test clients are setup for testing https. Should they be?

@deekayen: That was a spammer re-posting mfb's comment from #5. Blocked now.

I wasn't responding to the spammer. I had a non-https service running on port 443, which the test tried to use and failed, so that suggests to me that there's something that the tests could check on 443 if it was working properly. Is it better that the test bots have SSL available for tests, or better that the tests can verify that SSL doesn't exist, or a combo of both (which would lead to inconsistent failures)?

This discussion is probably better moved to the infrastructure queue or PIFT or wherever people talk about testing bot set-ups. :)

The edits to system_update_7057() should be in a new update handler, we need to support sites that have already installed beta1 now.

Do we need a comment at the assertion in the test pointing to this issue nid saying "don't change me"? :/

Glad you tracked this down again, mfb... thanks!

The patch is good but had a chance of a notice in the new http.php. Added a !empty to it.

$conf['https'] should be documented in default.settings.php, yes?

[EDIT] - this was a cross-post, I hadn't intended to change the status. Still, I'm a bit confused by $conf['https'] - that seems like a weird choice considering what it does. It seems more like $conf['enable_https_mixed_sessions'] or something. As it stands we have this:

http://drupal.org/https-information : For better security, leave $conf['https'] at the default value (FALSE) and only use HTTPS for authenticated sessions.

So we're supposed to turn 'https' off to turn it on?

cool, thanks - I'm working on the port of securepages.module so I'll do some more research later on $conf['https'].

Setting the status back per #68.

doh.

Extremely scary security bug! So glad to see we've noticed this regression now and not after 7.0 release :P

This is the identical patch from #69 with two whitespace issue removed.

+// Negated copy of the condition in _drupal_bootstrap(). If the user agent is
+// not from simpletest then disallow access.
+if (!(isset($_SERVER['HTTP_USER_AGENT']) && (strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE))) {
+  exit;
+}

This (from both http.php and https.php) rely on the fact that it is difficult from the client side to forge the User Agent header. Are we sure that's always the case? For example what about XMLHttpRequest and HTTP requests from Flash content?

I think http://api.drupal.org/api/function/drupal_valid_test_ua/7 is the proper way to validate the request, unfortunately it requires DRUPAL_ROOT and REQUEST_TIME to be pre-defined. Maybe we can have http.php and https.php have private versions of drupal_valid_test_ua() that use variables instead of constants to generate the HMAC.

It does not rely on that. If you forge your user agent to start with "simpletest" then the check in bootstrap will kick in. That's what we ensure here. The actual UA check is in bootstrap.

By the way, "forging" the User-Agent string is trivial. Anyone can set that to include the word "simpletest". Checking for that word alone (and not comparing the HMAC) in the User-Agent is not really enforcing anything.

Cross-posted the status, but it looks like there is a problem with the User-Agent check.

In http.php we are checking if the User-Agent contains the string "simpletest", but in bootstrap.inc we check if the User-Agent starts with "simpletest" and then has digits and a semicolon, and only validates then. If you use "something simpletest" as the User-Agent to http(s).php, it passes the initial check, and the bootstrap.inc check is ignored. That would allow http(s).php to be used to make non-test-db requests and fool the server into thinking it is in one mode or the other.

Working on it.

Now using the exact same preg_match() as bootstrap.inc, this should prevent any HTTP/S spoofing attacks.

Here we go. This will make sure it can't be broken again.

#84 is OK for me. There are several places in core that can be cleaned up by this new function, we can leave that to a follow-up though. install.core.inc uses a similar strpos() on the User-Agent, but in that instance it is pointless since we call drupal_boostrap() directly after.

#84: 575280-empty-session-id.patch queued for re-testing.

good

Summary of IRC discussion:

+ * This function does not test for forged headers, it only check whether the
+ * user agent looks like simpletest. drupal_valid_test_ua() needs to be fired
+ * to verify the validity of the UA. This function is called from various
+ * simpletest helpers.

I don't understand the point of this, then. Having two functions that are kinda-sorta validating the HTTP user agent strikes me as practically begging for further security problems. Let's move that logic into drupal_valid_test_ua() and have drupal_valid_test_ua() return the SimpleTest prefix.

Sure thing, I can do that. I have found the weirdest bug ever: drupal_get_bootstrap_phase messes up bootstrap if called before there was a DRUPAL_BOOTSTRAP_FULL phase which we have not found 'cos the only way to get there is by including bootstrap.inc from somethign that's not index.php. But this patch enhances the ability of Drupal to work with other systems.

I looked at the patch for a while comparing the changes vs. #76 and it seems correct. At the same time it improves the worth of drupal_valid_test_ua(), too. Very nice.

#90: 575280-empty-session-id.patch queued for re-testing.

@mfb actually you're right. We removed the 403 exit in #90, so we still have the same security issue with HTTP/S spoofing. I still think #83 is an easy way to fix the problem, just use consistent preg_match() in http.php, the hmac will get checked in bootstrap.inc and we'd be denied access. #90 would just treat it as a normal request, which would allow the spoof.

That's not a spoofing issue however. The invalid UA is disregarded. The fact that it loads proves this -- if it would use the simpletest prefix it would not find the tables.

In HEAD we check for a possible spoof in _drupal_bootstrap_database(), for our purposes here that's good enough.

#90 has the same flaw as #76 it seems, just using a different spoof header ("simpletest123;" instead of "simpletest blah"). I'm re-uploading #83, and here's the difference between #76 and this:

interdiff 575280-empty-session-id_8.patch 575280-empty-session-id_9.patch 
diff -u modules/simpletest/tests/http.php modules/simpletest/tests/http.php
--- modules/simpletest/tests/http.php	29 Oct 2010 22:10:36 -0000
+++ modules/simpletest/tests/http.php	31 Oct 2010 19:09:10 -0000
@@ -8,7 +8,7 @@
 
 // Negated copy of the condition in _drupal_bootstrap(). If the user agent is
 // not from simpletest then disallow access.
-if (!(isset($_SERVER['HTTP_USER_AGENT']) && (strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE))) {
+if (!(isset($_SERVER['HTTP_USER_AGENT']) && preg_match("/^(simpletest\d+);/", $_SERVER['HTTP_USER_AGENT']))) {
   exit;
 }

The change means you can't spoof using "foo simpletest". If you try "simpletest123;" and get through this check, the hmac is checked in bootstrap.inc and the request dies with access denied. I think that is sufficient to address this side issue and get the larger picture (critical sid vulnerability) fixed.

Yeah, no worry for cached responses; checking in the bootstrap database phase is fine. If the response is cached then a spoof would be unable to accomplish anything, in theory.

Well the solution then is to call drupal_valid_test_ua() twice once immediately after configuration is loaded second when the prefix is needed. Can you guys beat that? I have added static caching to the function so it's very fast.

Edit: it is possible that this check completely obsoletes the one in http.php and https.php which would be quite great.

I don't know, I still think all we need is to change 2 lines to fix the problem (#98). Anything more than that is catering too much to this edge-case (alternative entry point which modifies server variables, a rare occurrence).

#102 looks like voodoo, calling drupal_valid_test_ua() at the end of bootstrap configuration, the function returns a simpletest prefix now (which is not documented) and that is not used until later... where it is pulled from the static... and $http_deny = TRUE by default, that is not obvious either... plus the static is broken for normal page loads: static is only set if the preg_match() passes. I am generally unhappy with this patch. It overly complicated and caters to edge-case.

My poor patch throws a notice but it's SO early that noone evah finds it. Also the check is now completely unneeded in http.php, https.php and install.core.inc because it's all neatly wrapped in the function. That actually simplifies and hardens Drupal.

So to recap, the second security hole we discovered and fixed in the recent patches:

1. Set $conf['page_cache_without_database'] to TRUE with additional config / modules to make it work.
2. Enable page caching
3. Change your site to serve different pages on SSL than without (say, without certain pages are not served at all)
4. Restrict access to anonymous SSL by firewall or client certificates
5. the attacker sets user agent to simpletest1 and visits the https.php to request the pages set up and secured in the last two steps. They will be served from cache.

The first sechole, just to make this post a complete summary (copied from mfb):

1. Install Drupal and make available on HTTP and HTTPS URLs, e.g. http://example.com/ and https://example.com/.
2. Login to the HTTP site as uid 1 (this results in a session with ssid = '') and the HTTPS site as uid 1 (this results in a session with sid = '').
3. Bring up the Firecookie tab in Firebug.
4. Hover over both the SESS and SSESS cookies, right click and select Clear Value.
5. Now visit the HTTP and HTTPS sites with your empty string session cookies. You will still be uid 1 on both sites. So will anyone else visiting the sites with empty string session cookies!
6. Profit.

+++ includes/bootstrap.inc	2010-11-01 02:28:15 +0000
@@ -2217,26 +2211,53 @@ function _drupal_bootstrap_page_header()
+ * @param $deny
+ *   Whether to issue HTTP deny headers and exit if the user agent is a forged
+ *   simpletest string.
...
-function drupal_valid_test_ua($user_agent) {
+function drupal_valid_test_ua($user_agent = '', $http_deny = TRUE) {

Docs say $deny, but function sig says $http_deny.

+++ includes/session.inc	2010-10-31 18:42:05 +0000
@@ -180,21 +183,23 @@ function _drupal_session_write($sid, $va
+      // Use the session ID as 'sid' and an empty string as 'ssid', which
+      // _drupal_session_read() will prevent from being selected as a valid
+      // secure session ID.

I don't understand what this is saying. Maybe it can be written more clearly.

Other than those two things, this patch looks OK. It moves the spoof check earlier into the boot configuration phase, which prevents "attackers" from peeking at cached pages that they might not normally be able to see through HTTP. This is a pre-existing, totally unrelated issue to the sid flaw and beyond the scope of this issue IMO, but since we have a working patch we might as well go with it. Fix the docs and I think we are done here.

Fixed the docs as asked. Given that the session fix is good since #69 now carlos8f said we are good to go, I set to RTBC.

#107: 575280-empty-session-id.patch queued for re-testing.

+++ includes/install.core.inc	2010-11-01 02:28:49 +0000
@@ -228,14 +228,6 @@ function install_begin_request(&$install
 
-  // 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 no
-  // installation be permitted using such a prefix.
-  if (isset($_SERVER['HTTP_USER_AGENT']) && strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE) {
-    header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
-    exit;
-  }
-
   drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION);

Latest patch fails because chx tried to sneak this in. We had been talking about it on IRC and decided it was a useless check. This prevents simpletest from ever using install.php. However, it's next to impossible to forge a legitimate simpletest header, and as long as we check the HMAC (which booting configuration now does) I think we can remove the lines in simpletest.test that checks for a 403.

Powered by Dreditor.

@mfb, I think that works (overridding CURLOPT_USERAGENT with CURLOPT_HTTPHEADER), but the header should be one string, like User-Agent: simpletest123;.

I would think that you want to use additionalCurlOptions. I did. The patch has failures yet I post it 'cos I am out of time to work on this now.

+      $this->additionalCurlOptions = array(CURLOPT_USERAGENT => $this->databasePrefix);
$this->assertResponse(403, 'Cannot access install.php with a "simpletest" user-agent header.');

This assertion is not what mfb suggested. We want to make sure install.php returns a 403 if a spoofed simpletest header is present, i.e. "simpletest123;". $this->databasePrefix would produce a valid header and a corresponding 200 OK.

$this->databasePrefix definitely would not produce a legit header because it lacks timestamp and HMAC. And passing in User-Agent: leads to a conflicting situation (a preset cURL option vs this HTTP header), the variable I used is designed to override any CURL options set.

OK, in any case we need to make the assertion more clear. It says "Cannot access install.php with a "simpletest" user-agent header." Let's change that to "invalid simpletest user-agent header", and perhaps use simpletest123; instead of $this->databasePrefix. I tested out passing the User-Agent string directly in the drupalGet(), it should work to override CURLOPT_USERAGENT temporarily.

Additionally, I think we should add assertions to test for a 403 when trying to use a spoofed simpletest header on index.php, http.php, and https.php. Then we could group all these 403 assertions into a new test method, and use $this->additionalCurlOptions.

I'm working on cleaning this up a bit.

As far as I'm concerned, once the overlay bug and Dashboard module bug are closed along with this, that then should mean that Drupal seven will be ready for an RC release.
If I can help by testing patches as long as your patches and such have to deal with using a screen reading program or keyboared alone in Drupal7-I can test them out and let you folks know.
Overlay patch loks fine as far as I'm concerned.
I can't wait until all the issues reach zero and an RC build is released.
I think judging on the number of various bugs I've seen is that the more time spent on these the longer Drupal Seven will not be released. I'd like to see this happen ASAP.
Although I don't have the resources at the moment to fix any outstanding issues myself, I can apply various patches and such.
But like I said-if the patch isn't a screen reader specific type of patch as that would have to deal with using the keyboard and such, then I can't really test for anything.
Let's get an RC out soon, hopefully!
I believe that modules and bugs can always be easily addresed during/after an RC, and even after the oficial release.
Please see comment 182 (I think) on the overlay module screen reader chriticle issue and you'll see my opinion on what we all need to do together as a community to get Drupal into an RC release, and finally the next stable. :)
Let's make Drupal the best we can make it be for all users!

It turns out we can't remove the user agent check in http(s).php, like #104 states. That's because if you have a user agent that looks nothing like simpletest, it is ignored as a simpletest spoof and you can happily make requests to http(s).php using your browser (I was able to verify that).

This patch has a slightly different implementation than @chx's, but I tried to keep the goal the same. It locks down http(s).php so you can never make fake requests to it. To do that, I re-arranged some things in http(s).php and have developed a sneaky way to extract the hash salt from settings.php when we're running the tests.

As a side, a 403 is no longer returned for requests to index.php when the user agent looks like 'simpletest123', it's simply treated as a normal request to index.php.

Let's see how the tests do.

I learned why if (!$this->inCURL()) is necessary in simpletest.test.

Fixed a problem with the static in drupal_valid_test_ua().

Even better, instead of "forging" the header with "simpletest123;", let's use an actual header and slightly modify the HMAC. That ensures the HMAC is actually getting validated.

Improved the test again, now we check for a 200 with our generated $test_ua, then change the HMAC and test for 403.

Yes, that sounds like a better idea. I will include @chx's bootstrap bugfix in there too, although I'm not sure if it affects my patch.

Much better.

On second thought, the bootstrap bugfix doesn't affect the patch, so I left it out and let's open a separate issue if necessary.

Doh. I think that it works because we override $_SERVER['HTTPS'] etc. afterwards anyway, but still, we need to simulate that PHP started out with these server variables to begin with.

@mfb, sorry, for some reason I totally missed that you posted a patch in #127. Looks like #132 is basically the same though, other than the capitalization of 'HTTPS' in comments.

No, no, no! We are not exposing the salt like this. Working on it. Nevermind: i am stupid.

Sorry for the noise. mfb's patch was good http://drupal.org/node/575280#comment-3663234 here linking the patch http://drupal.org/files/issues/575280-empty-session-id_21.patch itself and carlos8f said that he missed the patch and basically implemented the safe and interdiff concurs. So while the latest two green patches are largely the same the comments in mfb's is better. Let's get this sucker in!

Reviewed and studdied, and committed to CVS HEAD. Thanks.

session.patch queued for re-testing.

This issue is already fixed.

I've never used Postgres.... why does the primary key need to be dropped and re-added?

Also... is it possible that after this update runs, there are still rows in {sessions} where sid = '', ssid = '...' that still pose a security problem if someone uses a blank sid in their cookie? We do if ($sid) { in $is_https mode, but not otherwise. Seems like we should always set $user to drupal_anonymous_user() if we have a blank $sid.

Another minor thing, in _drupal_session_read() it's possible if $is_https and !$sid, $user is undefined and might trigger a PHP notice.

The last commit seems to create duplicate entries in the session table when $conf['https'] = TRUE.

Steps to reproduce:
- install HEAD, set $conf['https'] = TRUE.
- Log in via HTTPS
- post a comment via HTTPS.
- post another comment via HTTP.

The session table now looks like this:

+-----+---------------------------------------------+---------------------------------------------+
| uid | sid                                         | ssid                                        |
+-----+---------------------------------------------+---------------------------------------------+
|   1 | 1VnxAJgfdR1e49PrjP1WFXmbJldcyFVm18ZXzgOvbNc |                                             | 
|   1 | 1VnxAJgfdR1e49PrjP1WFXmbJldcyFVm18ZXzgOvbNc | 80wsjXv_o5OMFZBxFQQtbDZmNbE9SpsRhBhCWDYycgs | 
+-----+---------------------------------------------+---------------------------------------------+

As a result, you get really strange behavior, such as drupal_get_messages() failing to retrieve messages that are stuck in the second row.

.

Cross-post.

Yes, thanks! The latest patch fixes the issue I was seeing with duplicate sessions. I've noticed 2 more issues with https, though it's unclear if they are related to this patch:

If an anonymous session is being written because of drupal_set_message(), and it's an https request, $_COOKIE is empty and this test fails:
if (isset($_COOKIE[$insecure_session_name])) {
As a result the insecure cookie never gets sent, and we end up with the following session:

+-----+---------------------------------------------+---------------------------------------------+
| uid | sid                                         | ssid                                        |
+-----+---------------------------------------------+---------------------------------------------+
|   0 | A1ueD72U6wncR4EQMU3g8aR3ptRCcUtjcHNiK7FR4lI | A1ueD72U6wncR4EQMU3g8aR3ptRCcUtjcHNiK7FR4lI | 
+-----+---------------------------------------------+---------------------------------------------+

Second issue, $user->timestamp appears not to be defined for the anonymous user; leading to PHP notices in some of my tests.
if ($is_changed || REQUEST_TIME - $user->timestamp > variable_get('session_write_interval', 180))

If a clearer demonstration of what I'm seeing would be helpful, just checkout the 7.x branch of securepages and run the tests (you'll also need the patch in #961508: Dual http/https session cookies interfere with drupal_valid_token()).

We still haven't fixed the real problem: as Damien points out we are accepting an empty session ID. We've fixed this for $is_https, but not otherwise.

This is a reroll of #144 which returns an anonymous user if empty($sid). @mfb's changes to the update function kill sessions with blank IDs, but sites may have already run that update function and I think we should fix it on the session handler level as well.

Well, this is good.

We need tests for this.

okay.

Come on IRC! :D

The bot should fail the first patch, since it does not include the fix. I moved the fix to drupal_session_initialize(), and simply check !empty($_COOKIE[$session_name]) instead of isset(), so now we don't even session_start() if a blank session ID is passed.

This does not include a test for #142, so it still needs work.

OK, here is a straight isset() to !empty() change, let's see how the bot likes this one.

While I was on the fence with the previous patch (because it changed session writes) I really like this one. I have deleted one assert from the test however -- there is no need to assert the success of a database update. We have update tests for that.

Committed to HEAD! Thanks!