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

Files: 
CommentFileSizeAuthor
#78 SecurePagesTestCase.log_.txt31.11 KBgrendzy
#72 1050746-POST-APOCALYPSE.patch5.97 KBEverett Zufelt
PASSED: [[SimpleTest]]: [MySQL] 38,322 pass(es).
[ View ]
#66 1050746-POST-APOCALYPse-D7.patch5.97 KBEverett Zufelt
PASSED: [[SimpleTest]]: [MySQL] 37,890 pass(es).
[ View ]
#28 1050746-POST-APOCALYPSE.patch6.01 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,356 pass(es).
[ View ]
#25 1050746-24.patch5.97 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1050746-24.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#4 1050746.patch5.97 KBgrendzy
PASSED: [[SimpleTest]]: [MySQL] 33,303 pass(es).
[ View ]
sessions.patch5.39 KBgrendzy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sessions_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Needs review» Needs work

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

subscribe

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

Status:Needs work» Needs review
StatusFileSize
new5.97 KB
PASSED: [[SimpleTest]]: [MySQL] 33,303 pass(es).
[ View ]

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?

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.

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

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:

<?php
$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
<?php
$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:

<?php
            $key
['sid'] = $_COOKIE[$insecure_session_name];
?>

should be replaced with
<?php
           
// 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.

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.

Subscribe.

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:

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

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

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.

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.

<?php
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:

<?php
   
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?

amarcus: please open an issue in the securepages queue.

Tagging issues not yet using summary template.

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

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

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

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

Subscribing

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?

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?

StatusFileSize
new5.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1050746-24.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Just a reroll to compensate for fuzz.

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?

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?

StatusFileSize
new6.01 KB
PASSED: [[SimpleTest]]: [MySQL] 33,356 pass(es).
[ View ]

Rerolled for core/.

Status:Needs review» Needs work

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

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

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

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.

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

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

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.

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

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!

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.

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.

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

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.

Added issue summary.

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

Status:Needs review» Reviewed & tested by the community

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.

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.

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

Issue tags:+session

Adding session tag.

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

Issue summary:View changes

issue summary

subscribe

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

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

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?

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

sub

@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!

Status:Needs work» Needs review

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

Status:Needs review» Needs work

Back to Needs Work.

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.

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.

StatusFileSize
new5.97 KB
PASSED: [[SimpleTest]]: [MySQL] 37,890 pass(es).
[ View ]

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

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.

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!

#64 is ok and it is indeed rtbc.

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.

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

StatusFileSize
new5.97 KB
PASSED: [[SimpleTest]]: [MySQL] 38,322 pass(es).
[ View ]

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

Thank you.

Patch applied and working...

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

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

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

sessions.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new31.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.

Issue summary:View changes

link 47

Issue tags:-Needs tests

Seems that this has tests already.

Status:Reviewed & tested by the community» Fixed

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

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.

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.

Issue tags:+7.13 release notes

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

Because I get error

I just checked that comes with those patches - Thanks

Issue tags:+SecurePages

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

Issue summary:View changes

update backport status

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?

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.

Status:Closed (fixed)» Active

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.