Problem/Motivation

Drupal 7 (via #1577: Mapping privilege separation on non-SSL/SSL and some follow-ups) introduced a mixed HTTP/HTTPS session mode to core. Unfortunately core never uses this mode, and it's non-functional. The changes to session.inc also make it impossible for a contrib module to implement mixed sessions.

This issue is blocking a release of the Secure Pages module.

Steps to reproduce

Because core does not ever use mixed-mode, the bugs can't be observed with core alone.

The quick method to observe the session failures is to run the tests for Secure Pages. (For now, tests should be run from HTTP, e.g. http://example.com/admin/config/development/testing, with the "Enable Secure Pages" set to "Disabled").
The Secure Pages module contains a thorough test of mixed session handling and they will all pass after applying the needed core patches.

Isolating bugs

The notes below describe how to reproduce each specific bug.

Bug #1
  • Add comment/reply/* to the "Pages" list on admin/config/system/securepages, and also check "Switch back to http pages when there are no matches".
  • Grant anonymous "post comments" and "skip comment approval" permission.
  • Create an article node.
  • Visit the article node as anonymous.
  • Post a comment

Expected behavior is to see the "Your comment has been posted" status message. Instead, no message is shown. Additionally the sessions table is corrupt (sid and ssid should always differ):

+-----+------------+------------+------------+-----------------------------------------------------------------------------+
| uid | sid        | ssid       | timestamp  | session                                                                     |
+-----+------------+------------+------------+-----------------------------------------------------------------------------+
|   0 | VFG95Ro... | VFG95Ro... | 1319159270 | messages|a:1:{s:6:"status";a:1:{i:0;s:29:"Your comment has been posted.";}} |
+-----+------------+------------+------------+-----------------------------------------------------------------------------+
Bug #2

A related problem occurs when posting from https to http:

  • Change the "Pages" setting on admin/config/system/securepages to "node/*" and "user/*".
  • Visit your article node
  • Post an anonymous comment

The status message does not appear as expected. The browser cookie store is empty, and the sessions table is corrupted as before.

Bug #3
  • Change the "Pages" setting on admin/config/system/securepages to "user/login" only
  • Log in
  • Log out

Only the SID cookie is deleted - and not the SSID cookie.
Set-Cookie:SESSab4098d...=deleted; expires=Thu, 21-Oct-2010 15:36:06 GMT; path=/; domain=.jasper; httponly

While it may seems strange to delete an HTTPS cookie during an HTTP request, it's necessary to make mixed sessions work. In practice browsers accept such cookie headers and RFC6265 indicates this "weak integrity" is expected.

Bug #4

$user->timestamp is not correctly initialized in session.inc. When testing mixed sessions this notice occurs:

Undefined property: stdClass::$timestamp Notice session.inc 175 _drupal_session_write()

Proposed resolution

Session.inc should be patched to correct logic flaws.

Remaining tasks

The fix has been committed to D8. A backported patch is currently RTBC.

FWIW, the 7.x-dev release of Secure Pages has nearly 2200 users. I think we can assume a significant fraction of those users are successfully using these patches, since without them mixed sessions do not work at all.

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

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

ogi’s picture

subscribe

mfb’s picture

subscribing to pore over this patch when I have a chance..

grendzy’s picture

Status: Needs work » Needs review
FileSize
5.97 KB
sun’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

oy, is this the reason for why I see a completely new user session for auth users for every single request on SSL in D7?

Akaoni’s picture

Good work, grendzy. ;)

Can confirm this patch fixes synchronicity issues between HTTP and HTTPS session data created when the user is not authenticated in Drupal.
Eg. Without logging into Drupal, when you change session data in HTTP then switch to HTTPS the data is still available. Also, vice-versa for HTTPS to HTTP.
This is useful for session data set by other applications and modules.

Didn't test any Secure Pages stuff, sorry.

What isn't awesome is that it creates an HTTPS cookie just to be able to read the HTTP session. Also, any session data set in HTTPS can still be sidejacked until drupal_session_regenerate() is called (either by Drupal login or a module specifically). Would be nice to set the HTTPS cookie and regenerate the session only when the session is changed via HTTPS.

OldAccount’s picture

I'm using the Secure Pages module with Ubercart and confirm this patch fixed my mixed-mode session issue. Thank you!

amarcus’s picture

Version: 8.x-dev » 7.x-dev

I am using secure pages on an inline credit card form within Drupal Commerce (as opposed to logging in users), and have found two additional issues that this patch does not appear to fix.

In session.inc on line 93 in _drupal_session_read:

$user = db_query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.sid = :sid AND s.uid = 0", array(
        ':sid' => $_COOKIE[$insecure_session_name]))
        ->fetchObject();

should be replaced with

$user = db_query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.sid = :sid", array(
        ':sid' => $_COOKIE[$insecure_session_name]))
        ->fetchObject();

I can't figure out why the check is made for only anonymous unsecured sessions; this has the effect that if a user already is logged in on the http site, they are logged out as soon as the site switches to https. It looks like everything else about the original file and the patch is supposed to handle this case, so I don't see why this wasn't corrected also.

Second, on line 195 in _drupal_session_write:

            $key['sid'] = $_COOKIE[$insecure_session_name];

should be replaced with

            // Use the insecure session as the key and store the ssid, but don't create a separate record
            $key['sid'] = $_COOKIE[$insecure_session_name];
            $fields['ssid'] = $sid;
            unset($key['ssid']);

Otherwise, having both sid and ssid in the db_merge() causes a new database record to be created if there is already an insecure session but the secure session is new, rather than updating the existing insecure record to include the secure session key. This causes problems downstream since the database now stores two separate sets of session data for the same insecure session key - one with a secure session key and one without.

mfb’s picture

As far as I understand, it's intentional that only anonymous sessions can be upgraded from HTTP to HTTPS. There's an assumption that logins are done securely and the HTTP to HTTPS upgrade happens during login.

Otherwise a site admin could login over HTTPS, then have her session hijacked by an eavesdropper while she browses the HTTP site (with $conf['https'] TRUE, she would be logged in on both the HTTPS and HTTP sites), and then the eavesdropper could present that insecure session cookie to the HTTPS site and the request would be recognized as a valid secure session, authenticated as the site admin.

If a site is configured such that some functionality is only permitted via a secure session over HTTPS (like, deleting nodes and users or enabling the PHP filter) then there'd be nothing to stop the eavesdropper who now has a "secure" session on the HTTPS site.

jsenich’s picture

Subscribe.

amarcus’s picture

Ah, thank you for the explanation, mfb. So essentially, if I want users to use HTTPS at any point once their logged in, I have to redirect the login form through HTTPS to create both sessions at the same time (and, of course, protected their credentials).

Since I already have a form_alter hook for the login block form, I just added the $form['#action'] in there:

function my_module_form_user_login_block_alter(&$form, &$form_state, $form_id) {
  // ...
  if (variable_get('https', FALSE) && $path = variable_get('securepages_basepath_ssl', FALSE)) {
    $query = drupal_get_query_parameters($_GET, array('q')) + drupal_get_destination();
    $form['#action'] = $path . url($_GET['q']) . '?' . drupal_http_build_query($query); 
  }
}

But is there a module that already does this? The securepages module seems pretty useless without this critical piece.

mfb’s picture

If you have $conf['https'] == TRUE then you should just be able to set $form['#https'] = TRUE in your form alter function. Looking at the securepages source code, it has some logic to set $form['#https'] = TRUE so I'd assume it should take care of it

grendzy’s picture

Yes, the securepages module does this. mfb is also right in #9, we don't want to upgrade authenticated sessions - you must log in via HTTPS in mixed mode.

amarcus’s picture

Thank you mfb and grendzy.

Yes, I found where the securepages module does it. However, it only seems to work for the user_login_block login form. On our site we built our own custom login form whose form_id is redirected to user_login_block via hook_forms(). In this case, I can get our form to submit to HTTPS the first time by setting $form['#https'] = TRUE. However, if the form is invalid, the securepages module will reset it back to HTTP on the second submission.

function securepages_form_alter(&$form, &$form_state, $form_id) {
    // ..
    // When already in secure mode and submitting a login form to an arbitrary page, this will take it out of secure mode
    elseif ($page_match === 0 && securepages_is_secure() && variable_get('securepages_switch', FALSE)) {
      $url['https'] = FALSE;
      $url['absolute'] = TRUE;
      $form['#action'] = url($url['path'], $url);
    }
  }

  // If the user/login block matches, also secure the login block.
  if (securepages_match('user/login') && $form_id == 'user_login_block' && !securepages_is_secure()) {
    $form['#https'] = TRUE;
  }
}

It seems that an easy way to fix this would be to simply check for $form['#https'] and not take the form out of secure mode in this case. So, line 85 becomes:

    elseif (empty($form['#https']) && $page_match === 0 && securepages_is_secure() && variable_get('securepages_switch', FALSE)) {

On the other hand, submitting an unsecure form on a secure page throws a browser warning. Wouldn't it be better to always submit forms on secure pages in secure mode, and then redirect to insecure mode, if necessary, when the form is redirected after submission?

grendzy’s picture

amarcus: please open an issue in the securepages queue.

amarcus’s picture

xjm’s picture

Tagging issues not yet using summary template.

grendzy’s picture

Version: 7.x-dev » 8.x-dev
sethfisher’s picture

#4: 1050746.patch queued for re-testing.

Damn definitely didn't mean to do that, no way to undo.

grendzy’s picture

that's OK, it was due for a re-test anyway.

BenK’s picture

Subscribing

rickmanelius’s picture

Status: Needs review » Needs work

This is marked 'needs review' but it's unclear from comments after #4 on whether or not we need some modifications to said patch. Needs work?

greggles’s picture

grendzy’s picture

Status: Needs work » Needs review

afaik #4 is a correct solution. The feedback in #8 was addressed by mfb: only anonymous sessions can be upgraded and this is indeed a feature. The feedback from amarcus pertains to the securepages module and that discussion was moved to a new issue.

If anyone is attending the PNW Drupal summit this weekend, look me up. I'm hoping to spend some time in the hacker lounge improving the issue summary, and maybe recruiting reviewers?

xjm’s picture

FileSize
5.97 KB

Just a reroll to compensate for fuzz.

rickmanelius’s picture

I attempted to use this for an ubercart installation in Drupal 7. There are still some big issues with both cookies being served and it's not able to hand off to the https connection when adding items into the shopping cart.

I have many sites working in Drupal 6. Bummed that I can't seem to get this working even with the patch in #25. Is my problem a separate issue altogether? Did anyone else get the patch in #25 working?

mjpa’s picture

Got this patch (coupled with #961508: Dual http/https session cookies interfere with drupal_valid_token()) working on a Commerce site. Have you applied the other patch?

xjm’s picture

Rerolled for core/.

Status: Needs review » Needs work

The last submitted patch, 1050746-POST-APOCALYPSE.patch, failed testing.

rickmanelius’s picture

Hi @mjpa. I assume #27 this was directed at me. Basically you're stating the combining the patch in #25 with #961508: Dual http/https session cookies interfere with drupal_valid_token() solves the issue for drupal commerce? Did you this patch as well #471970: DrupalWebTestCase->getAbsoluteUrl should use internal browser's base URL?

Testing now...

mjpa’s picture

@rickmanelius yes it was. I used all 3 patches but the 3rd one is only for the tests so isn't really necessary. I didn't really try Commerce without the patches but the 3 patches made it so I only needed to login once (I think the login form *needs* to be SSL) and could switch between HTTP & HTTPS without issue.

rickmanelius’s picture

Hi. I know this patch is for Drupal 8. I applied the first 2 patches in D7 and tested just logging in and logging out with secure pages. In many cases, I'm logged in via HTTP but switching over to HTTPS gets access denied.

So at least for Drupal 7, this still needs work. I'll leave it to others to provide feedback for D8.

grendzy’s picture

rickmanelius: you have to log in over HTTPS when using mixed-mode.

rickmanelius’s picture

@grendzy. That's kindof a bummer. If they are trying to access membership content (hence the store for memberships), that means they can't have a login form right there and have to be redirected to /user. I know on D6 this wasn't a problem. For D7, will there ever be a way to not require the login to also be https for that use case?

if not, I'll suck it up and deal with it :) But I didn't know if it's simply an impossibility now with the D7 setup or if it's something that could be adjusted later on...

mjpa’s picture

As long as where the user login form submits to is under SSL it should work fine - you could form_alter() the user login form to have an action of /user/login so the page displaying the form doesn't need to be SSL.

xjm’s picture

Testbot fail was just a testbot thing, and it passed now. :)

xjm’s picture

rickmanelius’s picture

Good point @mjpa. I wonder if that functionality should be a checkbox for secure pages simply because I imagine a lot of people just needing to toggle on secure pages for commerce would expect that type of behavior.

For now and my system, I'll just direct all logins to /user. Thanks!

grendzy’s picture

rickmanelius: the securepages module already alters the login block's action URL to HTTPS. If this isn't working for you please open a support request in the secucepages queue.

rickmanelius’s picture

I have a clean D7 install with secure pages and the applied the two patches listed in the issue summary. Yesterday I was having some success, but I don't think I tested all the scenarios.

I've cleared cache numerous times. If I login at https://site.com/user/login, it's still not registering being logged in when I go back to http and vice versa.

It is handing over items in the shopping cart but only when there isn't a session issue between the http/https connections. When there is, it's accessed denied or redirected back to HTTP.

I don't mind helping out with more testing, but others are claiming they are all set and I'm just not getting the expected behavior at the moment.

xjm’s picture

#41: I suggest filing your issue in the securepages issue queue.

rickmanelius’s picture

Understood @xjm. I was just trying to help with what testing I could. If the patches themselves check out otherwise, awesome. I'll post this in the Drupal 7 port thread of securepages.

grendzy’s picture

Added issue summary.

rickmanelius’s picture

In an effort to move these along by providing feedback, here is a quick overview.

rickmanelius’s picture

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

Status: Reviewed & tested by the community » Needs work

Thanks for fixing the shoddy work I did with the HTTP/HTTPS patch. Unsetting ssid from the merge key in the case where we are a) on HTTP b) the user might have both a sid and an ssid is, I think, not secure. If the user already has an ssid and an sid and has such a record in the session table now you are merging on the sid alone which might result in changing for example the uid (or other data) belonging to the ssid.

corE’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs backport to D7, -D7 stable release blocker, -Contributed project blocker

#28: 1050746-POST-APOCALYPSE.patch queued for re-testing.

corE’s picture

#25: 1050746-24.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs backport to D7, +D7 stable release blocker, +Contributed project blocker

The last submitted patch, 1050746-POST-APOCALYPSE.patch, failed testing.

Akaoni’s picture

Issue tags: +session

Adding session tag.

RaulMuroc’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -session, -Needs backport to D7, -D7 stable release blocker, -Contributed project blocker

#28: 1050746-POST-APOCALYPSE.patch queued for re-testing.

IcanDivideBy0’s picture

IcanDivideBy0’s picture

Issue summary: View changes

issue summary

ardnet’s picture

subscribe

hanoii’s picture

For what it's worth, #4 (for D7) and #28 (for D8) are almost identical and #28 applies fine on D7 as well.

klausi’s picture

For those that are only interested in bug #4 from the issue summary there is a patch suitable for your drush make file in #1410130: Notice: Undefined property: stdClass::$timestamp in _drupal_session_write().

chx’s picture

Status: Needs review » Needs work

Why was this patch set to CNR? I have tried to find reasoning but there's none. What's going on?

xjm’s picture

#57 I believe it got set NR because someone requeued it for testing using the link on the patch, which also sets the status to NR.

jantimon’s picture

sub

xjm’s picture

@Merco: You don't need to post a comment to "subscribe" any more; there is a "Follow" button in the upper-right corner of the issue page. Thanks!

fabioloool’s picture

Status: Needs work » Needs review

#28: 1050746-POST-APOCALYPSE.patch queued for re-testing.

sammyd56’s picture

Status: Needs review » Needs work

Back to Needs Work.

grendzy’s picture

Status: Needs work » Needs review

Hi chx, I've finally had time to review your feedback in #47. Sorry it took so long.

I believe merging on the sid alone is necessary in that condition: we're on HTTP, mixed-mode is on (so the user has an ssid), and we can't know what the ssid is because the secure cookie is unavailable for this request.

The session would look something like this:

+-----+--------+--------+-----------+------------+-------+---------------------------------+
| uid | sid    | ssid   | hostname  | timestamp  | cache | session                         |
+-----+--------+--------+-----------+------------+-------+---------------------------------+
|   1 | zpmfdR | A1u1By | 127.0.0.1 | 1328835176 |     0 | a:1:{s:5:"stuff";s:6:"things";} | 
+-----+--------+--------+-----------+------------+-------+---------------------------------+

We want to load that session, even when only the HTTP cookie is available. We are trusting the site builder to redirect sensitive URLs to HTTPS, where they cannot be hijacked. That's the fundamental compromise of mixed-mode.

Everett Zufelt’s picture

Status: Needs review » Reviewed & tested by the community

Applied and tested, this appears to be working as it should. I agree w/ @grendzy in #64. It is up to the developer to ensure that users are directed to secure URLs for sensative tasks.

Patch in #28 still also applies to 7.x.

Everett Zufelt’s picture

Adding re-roll of #28 against 7.x for drush make

grendzy’s picture

Regarding tests, I would prefer to commit this without additional tests for now. Our testing infrastructure does not support HTTPS, and I don't think the mock that's in simpletest/tests/https.php is capable of doing the kind of testing we need (for example form submissions to https.php are redirected back to /index.php).

On the plus side, the securepages module does have extensive tests so we can at least validate the patch that way.

rickmanelius’s picture

Any chance that we can get enough momentum to push all 3 patches through at some point? So many sites are already using this and it would make so much sense for Drupal 7 to actually have a stable release for this module!

chx’s picture

#64 is ok and it is indeed rtbc.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review

Thanks! Committed/pushed to 8.x. Moving to 7.x for backport.

xjm’s picture

Hum, should we open a followup issue for the additional tests? webchick suggested emulating https in #961508: Dual http/https session cookies interfere with drupal_valid_token().

Everett Zufelt’s picture

This is the patch committed to D8, rolled against D7.

chaskype’s picture

Thank you.

Patch applied and working...

MrPeanut’s picture

I wish I could help, but I don't know enough to test or edit patches. But, I am wondering, could someone tell me approximately what's left to have this be secure? Or, could someone be kind enough to walk me through applying the necessary patches? (I know it's elementary, but I've never applied patches.)

mgifford’s picture

#72: 1050746-POST-APOCALYPSE.patch queued for re-testing.

mgifford’s picture

@MrPeanut - Check out http://drupal.org/node/707484

It needs to get reviewed against Drupal 7 and it needs to meet the criterion to be marked RTBC before it can get into the next maintenance release.

BAM5’s picture

sessions.patch queued for re-testing.

grendzy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
31.11 KB

I've verified via interdiff the latest patch matches #4, and is also identical to what was committed to D8. Additionally I verified SecurePagesTestCase is passing with this patch (combined with the related #961508 and #471970). I'm not a neutral reviewer but I believe we have an RTBC consensus.

grendzy’s picture

Issue summary: View changes

link 47

tim.plunkett’s picture

Issue tags: -Needs tests

Seems that this has tests already.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks! :D Really nice to see this fixed!!

rickmanelius’s picture

I for one say YAY!!!!!

A big thanks to everyone who put in a lot of time and effort to squash this bug. Here's hoping the remaining ones can be completed for an official 1.0 release of the securepages module :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

teunis’s picture

I'm encountering http://drupal.org/node/1545970 - ssid is blank when switching from http to https and back. Old (blank ssid) gets used for http and overwrites https session (session used in http, then updated in https (third party login), then browse site in http)
there are two entries on 'sessions' table - one with ssid, one without.
Running this fixes it temporarily:
delete from sessions where ssid='';

don't change the status of the bug, this is a borderline case when a session is created in http first - answer I got back is "create the sessions in https" (as login does). If this is considered sufficiently important it should be put into a new report.

webchick’s picture

Issue tags: +7.13 release notes
chaskype’s picture

Do I need this patch the D7.14? (http://drupal.org/files/1050746-POST-APOCALYPSE_0.patch)

Because I get error

chaskype’s picture

I just checked that comes with those patches - Thanks

jazzdrive3’s picture

Issue tags: +SecurePages
esbon’s picture

Hi chaskype, drupal 7.17 is out, but I still don't understand if I need to apply patch 1050746-POST-APOCALYPSE_0.patch or not. Could you please provide some help please?

Miguel

esbon’s picture

Issue summary: View changes

update backport status

webservant316’s picture

I am still experience session problems when visiting HTTPS pages with the securepages module.

I am using Drupal 7.26 with no patches and securepages 7.x-beta2. When "$conf[https] = True;" in settings.php the session is always completely wiped out after visiting an HTTPS page and I have to login again. When it is not set and I also REMOVE the login form ids from the Securepage settings page "List of form ids which will have the https flag set to TRUE", then at least I can visit HTTPS pages. However, when on the page my session is NOT recognized, but when returning to HTTP pages it is again recognized.

I cannot figure out the recipe of settings to get this essential module to work properly. Any help?

rakun’s picture

I am also on Drupal 7.26 and securepages 7.x-beta2. I have problem with various forms. One of problematic forms are menu form (created by Menu Editor module 7.x-1.0-beta3). If I open some menu in two browser tabs I get "The form has become outdated. Copy any unsaved work in the form below and then reload this page.". Problem is that even if I close one of tabs and reload page again error will not disappear.

rakun’s picture

Status: Closed (fixed) » Active
grendzy’s picture

Status: Active » Closed (fixed)

Hi rakun, this issue isn't the right place for support requests. Feel free to open a new request in the securepages queue. Please try to describe the steps to recreate the issue starting with a fresh install of Drupal. Because of the complexities of different server setups, and many modules interacting, the community will need a lot of detail from you in order to help with session / SSL issues.

Status: Closed (fixed) » Needs work

The last submitted patch, 72: 1050746-POST-APOCALYPSE.patch, failed testing.

mgifford’s picture

Status: Needs work » Closed (fixed)

This was already marked closed fixed, so it would make sense that one couldn't apply the patch.