CRM/Core/Session.php breaks Drupal sessions

[ken]

The initialize() function in CRM/Core/Session.php breaks Drupal sessions in a subtle but nasty way. Session.php currently ignores the fact that Drupal creates 'lazy sessions' for anonymous users.

A 'lazy session' only creates persistent data if there is any content in $_SESSION. Drupal checks this AFTER processing the page request, and if $_SESSION is not empty it adds a row to the Drupal sessions table and sets a session cookie in the page response.

Session.php on the other hand checks if $_SESSION is set BEFORE the page request is handled, and calls drupal_session_start() if it is unset. It fails to check if a 'lazy session' is in progress and fails to destroy the old session.

Impact

After the page request is finished there will be two rows in the Drupal sessions table that share the same ssid (secure session id). Drupal assumes there is only one, which leads to all sorts of subtle, odd behaviours.

Solution

Drupal needs the lazy session to continue, and CiviCRM needs $_SESSION to not be NULL and to be an array. Here is a patch ...

Code: [Select]

--- CRM/Core/Session.php 2014-02-28 17:58:18.128813697 +1100
+++ ../cbf/patches/CRM/Core/Session.php 2014-02-28 19:55:26.014398608 +1100
@@ -119,7 +119,10 @@
         }
         $config =& CRM_Core_Config::singleton();
         if ($config->userSystem->is_drupal && function_exists('drupal_session_start')) {
-          drupal_session_start();
+          if ($GLOBALS['lazy_session'] != true) {
+            drupal_session_start();
+          }
+          $_SESSION = array();
         }
         else {
           session_start();

To reproduce

We need an anonymous user, who has no session data on the server, accessing a vulnerable page via HTTPS.

• Run the securepages module and set it so civicrm pages are forced to go via HTTPS
• Using Chrome, ensure there are no Drupal session cookies in your browser
• Using a data admin tool, empty the Drupal sessions table
• Visit /civicrm/event/info
• There will be a SSESS* session cookie in the browser, but no SESS* cookie. There will be 2 rows in the Drupal sessions table whose ssid column matches the SSESS* cookie data. The expected behaviour is that there should be only one row in sessions where the sid column matches the SESS* cookie and the ssid column matches the SSESS* cookie.
• (If you don't see this result, try clearing cookies, the sessions table and refreshing.)
• Click on Register Now to go to /civicrm/event/register
• Enter your registration details and press Continue >>
• The expected outcome is that we go to the Confirm page, but the observed behaviour is that return to the Registration page but with the registration fields cleared

[ken]

What happens at bootstrap

• When Drupal bootstraps, it calls drupal_session_initialize() [includes/bootstrap.inc, line 2247]
• drupal_session_initialize() will either call drupal_session_start() or set $GLOBALS['lazy_session'] = TRUE [includes/session.inc, lines 251 & 261]
• drupal_session_start() MAY set $_SESSION

So CiviCRM needs to check $GLOBALS['lazy_session'] and set $_SESSION to an empty array.

What happens at the end of the page request

drupal_session_commit() checks if $_SESSION is empty and only writes the session if there is data to store. [includes/session.inc, line 310]

So setting $_SESSION to an empty array in CRM/Core/Session.php won't cause the unnecessary setting of cookies.

[Coleman Watts]

Thanks Ken for investigating and supplying a patch. Do you have any recommendations for testing it?

[mathieu]

I was also running into very weird and hard to reproduce session issues, and the patch proposed by ken (for which I am very grateful!) seems to fix the issue for me as well.

How to reproduce:

* i use CiviContribute with PCP enabled, created a PCP page, etc.
* flush browser cookies
* visit the pcp/info page (anonymously)
* go to the contribute page linked to that PCP page
* submit the form to donate

About 1/3 of the time, I would be redirected back to the entryURL because the qfkey would not validate. It would generate a few qfPrivateKey.

[mathieu]

So far so good, since applying the patch, I have not observed any incidents (I have a script that analyses web server logs for the behaviour, i.e. redirection back to ?reset=1 after a POST, because the qfkey could not validate). c.f. https://github.com/mlutfy/nodisys-hosting-scripts/blob/master/nodisys-civicrm-contribute-analyis

I created a JIRA issue to track this bug: http://issues.civicrm.org/jira/browse/CRM-14356

@Ken: if you don't object, by request of Lobo, I will submit a pull-request with your patch so that we can integrate this in CiviCRM 4.4.5.

[mathieu]

Hmm, re-reading, I wonder:

Should the $_SESSION be initialized only if we called drupal_session_start()? i.e.

Code: [Select]

          if ($GLOBALS['lazy_session'] != TRUE) {
            drupal_session_start();
            $_SESSION = array();
          }

Otherwise, isn't there a risk of losing data set by Drupal or other?

Also, should we instead test using isset, i.e.:

Code: [Select]

          if (! isset($GLOBALS['lazy_session'])) {
            drupal_session_start();
            $_SESSION = array();
          }

This is what Drupal tests for in includes/session.inc, in drupal_session_regenerate().

I really do not have a good understanding of how this works, so I would like to really put emphasis on the fact that I am just wondering, and not suggesting. :-)

[ken]

@colemanw,

I included steps to reproduce in my original post. If that's not working for you, I'm sure how else to trigger this.

@mathieu

"Should the $_SESSION be initialized only if we called drupal_session_start()? Otherwise, isn't there a risk of losing data set by Drupal or other?"

The thing is that ...

• This function has already tested that $_SESSION is not set, and I assume this is being done because the rest of CiviCRM requires that $_SESSION is set. Indeed, if not set, the line straight after this "$this->_session =& $_SESSION;" will bork.
• If you look at drupal_session_start(), it doesn't always set $_SESSION. So $_SESSION has to be set in this case, to be sure, to be sure.
• On the other hand, if a lazy session is in progress, we've already tested that $_SESSION is not set. We have to set it in this case.
• If the session is lazy, and $_SESSION remains an empty array until the end of the response to this request, Drupal will not persist the session

The setting of $_SESSION should remain as per my patch.

"Also, should we instead test using isset"

That's a good idea! $GLOBALS['lazy_session'] is either unset or TRUE, and your suggestion is better practice.

"pull-request"

Yes, please!

[totten]

I've put the patch in a PR and made a small tweak at https://github.com/civicrm/civicrm-core/pull/2883

Follow-up question: In the case where lazy-sessions aren't used and where we call drupal_session_start(), is it still necessary to do $_SESSION=array()? It seems like that's already a side-effect of session_start()/drupal_session_start(), in which case we could avoid replacing the variable:

Code: [Select]

<?php
          if (isset($GLOBALS['lazy_session']) && $GLOBALS['lazy_session'] == true) {
            // Drupal lazy-session handling is active, but $_SESSION appears to be NULL (per previous test).
            // Put in a stub that works with Civi but doesn't prematurely start the lazy-session.
            $_SESSION = array();
          } else {
            // Drupal lazy-session handling isn't active, so it's OK to start the session.
            drupal_session_start(); // Post-condition: $_SESSION is set
          }


[totten]

Joanne and Eileen think this has caused a regression -- http://forum.civicrm.org/index.php/topic,32409.0.html