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.
- Install the 7.x version of http://drupal.org/project/securepages
- Set
$conf['https'] = TRUE
in settings.php - Apply two related patches:
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
Comment | File | Size | Author |
---|---|---|---|
#78 | SecurePagesTestCase.log_.txt | 31.11 KB | grendzy |
#72 | 1050746-POST-APOCALYPSE.patch | 5.97 KB | Everett Zufelt |
#66 | 1050746-POST-APOCALYPse-D7.patch | 5.97 KB | Everett Zufelt |
#28 | 1050746-POST-APOCALYPSE.patch | 6.01 KB | xjm |
#25 | 1050746-24.patch | 5.97 KB | xjm |
Comments
Comment #2
ogi CreditAttribution: ogi commentedsubscribe
Comment #3
mfbsubscribing to pore over this patch when I have a chance..
Comment #4
grendzy CreditAttribution: grendzy commentedComment #5
sunoy, is this the reason for why I see a completely new user session for auth users for every single request on SSL in D7?
Comment #6
Akaoni CreditAttribution: Akaoni commentedGood 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.
Comment #7
OldAccount CreditAttribution: OldAccount commentedI'm using the Secure Pages module with Ubercart and confirm this patch fixed my mixed-mode session issue. Thank you!
Comment #8
amarcus CreditAttribution: amarcus commentedI 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:
should be replaced with
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:
should be replaced with
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.
Comment #9
mfbAs 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.
Comment #10
jsenich CreditAttribution: jsenich commentedSubscribe.
Comment #11
amarcus CreditAttribution: amarcus commentedAh, 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:
But is there a module that already does this? The securepages module seems pretty useless without this critical piece.
Comment #12
mfbIf 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
Comment #13
grendzy CreditAttribution: grendzy commentedYes, 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.
Comment #14
amarcus CreditAttribution: amarcus commentedThank 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.
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:
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?
Comment #15
grendzy CreditAttribution: grendzy commentedamarcus: please open an issue in the securepages queue.
Comment #16
amarcus CreditAttribution: amarcus commentedCross posted to http://drupal.org/node/1226702
Comment #17
xjmTagging issues not yet using summary template.
Comment #18
grendzy CreditAttribution: grendzy commentedComment #19
sethfisher CreditAttribution: sethfisher commented#4: 1050746.patch queued for re-testing.
Damn definitely didn't mean to do that, no way to undo.
Comment #20
grendzy CreditAttribution: grendzy commentedthat's OK, it was due for a re-test anyway.
Comment #21
BenK CreditAttribution: BenK commentedSubscribing
Comment #22
rickmanelius CreditAttribution: rickmanelius commentedThis 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?
Comment #23
gregglesComment #24
grendzy CreditAttribution: grendzy commentedafaik #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?
Comment #25
xjmJust a reroll to compensate for fuzz.
Comment #26
rickmanelius CreditAttribution: rickmanelius commentedI 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?
Comment #27
mjpa CreditAttribution: mjpa commentedGot 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?
Comment #28
xjmRerolled for
core/
.Comment #30
rickmanelius CreditAttribution: rickmanelius commentedHi @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...
Comment #31
mjpa CreditAttribution: mjpa commented@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.
Comment #32
rickmanelius CreditAttribution: rickmanelius commentedHi. 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.
Comment #33
grendzy CreditAttribution: grendzy commentedrickmanelius: you have to log in over HTTPS when using mixed-mode.
Comment #34
rickmanelius CreditAttribution: rickmanelius commented@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...
Comment #35
mjpa CreditAttribution: mjpa commentedAs 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.
Comment #37
xjmTestbot fail was just a testbot thing, and it passed now. :)
Comment #38
xjmWhoops. Cat...
Comment #39
rickmanelius CreditAttribution: rickmanelius commentedGood 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!
Comment #40
grendzy CreditAttribution: grendzy commentedrickmanelius: 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.
Comment #41
rickmanelius CreditAttribution: rickmanelius commentedI 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.
Comment #42
xjm#41: I suggest filing your issue in the securepages issue queue.
Comment #43
rickmanelius CreditAttribution: rickmanelius commentedUnderstood @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.
Comment #44
grendzy CreditAttribution: grendzy commentedAdded issue summary.
Comment #45
rickmanelius CreditAttribution: rickmanelius commentedIn an effort to move these along by providing feedback, here is a quick overview.
Comment #46
rickmanelius CreditAttribution: rickmanelius commentedComment #47
chx CreditAttribution: chx commentedThanks 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.
Comment #48
corE CreditAttribution: corE commented#28: 1050746-POST-APOCALYPSE.patch queued for re-testing.
Comment #49
corE CreditAttribution: corE commented#25: 1050746-24.patch queued for re-testing.
Comment #51
Akaoni CreditAttribution: Akaoni commentedAdding session tag.
Comment #52
RaulMuroc CreditAttribution: RaulMuroc commented#28: 1050746-POST-APOCALYPSE.patch queued for re-testing.
Comment #53
IcanDivideBy0 CreditAttribution: IcanDivideBy0 commented#28: 1050746-POST-APOCALYPSE.patch queued for re-testing.
Comment #53.0
IcanDivideBy0 CreditAttribution: IcanDivideBy0 commentedissue summary
Comment #54
ardnet CreditAttribution: ardnet commentedsubscribe
Comment #55
hanoiiFor what it's worth, #4 (for D7) and #28 (for D8) are almost identical and #28 applies fine on D7 as well.
Comment #56
klausiFor 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().
Comment #57
chx CreditAttribution: chx commentedWhy was this patch set to CNR? I have tried to find reasoning but there's none. What's going on?
Comment #58
xjm#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.
Comment #59
jantimon CreditAttribution: jantimon commentedsub
Comment #60
xjm@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!
Comment #61
fabioloool CreditAttribution: fabioloool commented#28: 1050746-POST-APOCALYPSE.patch queued for re-testing.
Comment #62
sammyd56 CreditAttribution: sammyd56 commentedBack to Needs Work.
Comment #64
grendzy CreditAttribution: grendzy commentedHi 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:
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.
Comment #65
Everett Zufelt CreditAttribution: Everett Zufelt commentedApplied 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.
Comment #66
Everett Zufelt CreditAttribution: Everett Zufelt commentedAdding re-roll of #28 against 7.x for drush make
Comment #67
grendzy CreditAttribution: grendzy commentedRegarding 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.
Comment #68
rickmanelius CreditAttribution: rickmanelius commentedAny 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!
Comment #69
chx CreditAttribution: chx commented#64 is ok and it is indeed rtbc.
Comment #70
catchThanks! Committed/pushed to 8.x. Moving to 7.x for backport.
Comment #71
xjmHum, 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().
Comment #72
Everett Zufelt CreditAttribution: Everett Zufelt commentedThis is the patch committed to D8, rolled against D7.
Comment #73
chaskype CreditAttribution: chaskype commentedThank you.
Patch applied and working...
Comment #74
MrPeanut CreditAttribution: MrPeanut commentedI 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.)
Comment #75
mgifford#72: 1050746-POST-APOCALYPSE.patch queued for re-testing.
Comment #76
mgifford@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.
Comment #77
BAM5 CreditAttribution: BAM5 commentedsessions.patch queued for re-testing.
Comment #78
grendzy CreditAttribution: grendzy commentedI'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.
Comment #78.0
grendzy CreditAttribution: grendzy commentedlink 47
Comment #79
tim.plunkettSeems that this has tests already.
Comment #80
webchickCommitted and pushed to 7.x. Thanks! :D Really nice to see this fixed!!
Comment #81
rickmanelius CreditAttribution: rickmanelius commentedI 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 :)
Comment #83
teunis CreditAttribution: teunis commentedI'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.
Comment #84
webchickComment #85
chaskype CreditAttribution: chaskype commentedDo I need this patch the D7.14? (http://drupal.org/files/1050746-POST-APOCALYPSE_0.patch)
Because I get error
Comment #86
chaskype CreditAttribution: chaskype commentedI just checked that comes with those patches - Thanks
Comment #87
jazzdrive3 CreditAttribution: jazzdrive3 commentedComment #88
esbon CreditAttribution: esbon commentedHi 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
Comment #88.0
esbon CreditAttribution: esbon commentedupdate backport status
Comment #89
webservant316 CreditAttribution: webservant316 commentedI 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?
Comment #90
rakun CreditAttribution: rakun commentedI 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.
Comment #91
rakun CreditAttribution: rakun commentedComment #92
grendzy CreditAttribution: grendzy commentedHi 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.
Comment #95
mgiffordThis was already marked closed fixed, so it would make sense that one couldn't apply the patch.