note, this was part of SA-2008-044 for D6

The original bug report to the security team:

As you know, all anonymous users are assigned a session id. When that anonymous user authenticates, the session is supposed to regenerate to provide a new session_id. We are finding that this is not happening on our site.

After running the authentication process through a debugger, I discovered that the reason the session is not being regenerated is due to a Workflow_NG action that gets executed as part hook_user(). We currently have a redirect action tied to user login, so that instead of a user being redirected to ?q=user/xxxx, the user is brought to a welcome back page (node/148).

In user.module:

function user_login_submit($form_id, $form_values) {
global $user;
if ($user->uid) {
   // To handle the edge case where this function is called during a
   // bootstrap, check for the existence of t().
   if (function_exists('t')) {
     $message = t('Session opened for %name.', array('%name' => $user->name));
   }
   else {
      $message = "Session opened for ". check_plain($user->name);
   }
       //die('current user = '.$user->uid);
   watchdog('user', $message);

   // Update the user table timestamp noting user has logged in.
   db_query("UPDATE {users} SET login = %d WHERE uid = %d", time(), $user->uid);

   user_module_invoke('login', $form_values, $user);

   sess_regenerate();
   return 'user/'. $user->uid;
}
}

We see the db_query() call execute, and the user_module_invoke() executes. The redirect action in Workflow_NG gets executed as part of this. This uses drupal_goto(), which has an exit() call at the end. Because of this, sess_regenerate() never gets called.

I believe that this vulnerability exists in core because of the order in which the functions are called. I propose that sess_regenerate() be called prior to the user_module_invoke() call and have confirmed that changing the order of the function calls in user.module fixes the issue.

...
// Update the user table timestamp noting user has logged in.
db_query("UPDATE {users} SET login = %d WHERE uid = %d", time(), $user->uid);

sess_regenerate(); 
user_module_invoke('login', $form_values, $user);

return 'user/'. $user->uid;
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Needs review
FileSize
771 bytes

patch for 7.x (note, simple re-roll of the 6.x patch originally by Heine as suggested by Erich C. Beyrent in the e-mail above).

Dries’s picture

Status: Needs review » Needs work

1. I'd explicitly mention in the code comments that we want to regenerate the session ID before calling the hook, and why.

2. I think we should write a test for this. For obvious reasons, this is really important to get right and keep right.

pwolanin’s picture

Any suggestions on how to write the test? I think that would require a dummy module (and/or a dummy module hook?)

Damien Tournoud’s picture

Great, another use case for http://drupal.org/node/268063!

We need to write a small test module with a hook that breaks the contract, for example by redirecting the user to another page, and check if the session has been regenerated.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
981 bytes

Added more of a code comment - the test really requires a dummy module. Postponing the test until we have a mechanism (e.g. issue linked above) for dummy modules in the testing framework.

Damien Tournoud’s picture

@pwolanin: you don't need to wait for #268063: Move includes/tests to simpletest/tests & provide hidden .info propery... The only thing this issue does is add the ability to hide a module from the modules administration page.

The following conventions where discussed (especially with chx, webchick and boombatower on IRC), and need to be documented properly:

  • Test modules go to the /tests subdirectory of the module they are testing, or in /modules/simpletest/tests for include tests,
  • You add hidden = TRUE to the test module's .info file,
  • The test module should be named "_test" (if it makes sense)
gram.steve’s picture

Good to see this addressed.

Just deleted a longer comment but can't figure out how to delete this one altogether.

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

swentel’s picture

Reroll patch (now uses drupal_session_regenerate)

pwolanin’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Assigned: Unassigned » pwolanin
Status: Needs work » Needs review
FileSize
1.65 KB

failure was due to HEAD not installing properly. Re-rolled patch with the addition of the PHP 5.2-only feature of setting "httpsonly" on the session cookie as an additional hardening.

ran all tests myself: 7299 passes, 0 fails, and 0 exceptions

scor’s picture

Status: Needs review » Reviewed & tested by the community

patch working on my machine as well: 7299 passes, 0 fails, and 0 exceptions

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Up above, Dries recommended we write a test for this so it stays fixed. Now we have the ability to create hidden modules that can be used for testing purposes, so let's do that. :)

pwolanin’s picture

@webchick - argh - these tests killed my afternoon... but working now.

Here's a patch with only the tests. Without the patch from #12, there are 2 fails.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.96 KB

the full patch

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.57 KB

whoops - posted the test-only patch twice. And testbot has demonstrated that without the rest of the patch there are 2 fails...

keith.smith’s picture

when (if) you reroll, there's an "incorrectoly" in there.

pwolanin’s picture

w/ typo fix

scor’s picture

Status: Needs review » Needs work

there is a trailing space on the line session_set_cookie_params($lifetime, $path, $domain, $secure, TRUE);

pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.58 KB

ok - fixed (though I think webchick has also been removing trailing spaces before commit).

catch’s picture

This all looks RTBC to me, except it needs to be re-rolled for a couple of missing t() calls in the assertions. Told pwolanin this in irc so consider the following patch pre-approved.

pwolanin’s picture

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

ok, wrapped assertion text in t() as required.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Thanks pwolanin. Great work as usual! :)

pwolanin’s picture

Status: Fixed » Patch (to be ported)

is it worth backporting the httponly cookie part? I think adding that param will just be a no-op with older version of PHP.

pwolanin’s picture

Version: 7.x-dev » 6.x-dev
pwolanin’s picture

Status: Patch (to be ported) » Needs review
FileSize
819 bytes

Tested this locally and works with PHP 5.2 and Drupal 6.x

Gábor Hojtsy’s picture

Extract is always crazy, since it could overwrite whatever is in the current scope. However, we only have $old_session_id in this scope, so it might not be a problem there I guess. Still for extra safety and best practice, I'd use extract(..., EXTR_PREFIX_ALL, 'session') or something, so we'd get $session_lifetime, $session_path, etc. Otherwise looks good.

pwolanin’s picture

@Gabor - given that this is such a short function with almost no variables in scope, the extract should be harmless (i.e. this version is in 7 already). If you see some reason to worry, we could just use EXTR_SKIP?

Gábor Hojtsy’s picture

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

EXTR_SKIP and EXTR_OVERWRITE are just not good enough generally because you never know then which value you got at the end, and that could be quite confusing for working with the variables. Since this was already accepted in Drupal 7, and that it has a very low likeliness that there will be some new variables added later to this function which might conflict with the extracted ones (even those, which are not visible in variables now used from the extract), I did commit this to Drupal 6. Thanks.

Moving down to Drupal 5.x for consideration.

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 5.x.

drumm’s picture

Version: 5.x-dev » 6.x-dev
Status: Fixed » Reviewed & tested by the community
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Rolled back for 6.x.

hass’s picture

sub

Damien Tournoud’s picture

Incredible, so there *are* people using Drupal on PHP4 :)

dzhu’s picture

From patch code:

+ // This has no effect for PHP < 5.2.0.

This HAS effect at PHP 4.3.9, the error:

Wrong parameter count for session_set_cookie_params() in file /path/to/drupal/includes/session.inc in line 103.

Where the line 103 is:
session_set_cookie_params($lifetime, $path, $domain, $secure, TRUE);

Heine’s picture

@ dzhu, please read the issues before commenting (see #33, #34). Thank you.

dzhu’s picture

According to system requirements for Drupal 5 and 6, it's PHP version 4.3.5 compatible...

2 Heine, thanks, but what's with Drupal 6.7 current code? Will it be changed?

Heine’s picture

@dzhu Releases cannot be changed. The fix will go out as 6.8 and 5.14 later today.

Gábor Hojtsy’s picture

Drupal 6.8 and 5.14 is out with this change.

michtoen’s picture

What are the plans to go on with this issue?

andypost’s picture

Investigation of HTTPOnly flag

For anyone looking for which browsers support the HTTPOnly flag, per my research:

IE 6 SP 1 and higher.
Firefox 3 and higher.
Opera 9.50 and higher.

As of Safari 3.1.1 (April 2008), Safari did not yet support this flag.

This cookie flag was developed by Microsoft and is slowly making its way into other browsers: http://msdn2.microsoft.com/en-us/library/ms533046.aspx

Firefox 2.0 also supports them, but only since version 2.0.0.5.

http://bugzilla.mozilla.org/show_bug.cgi?id=178993

This feature is partially supported by browsers

expatme’s picture

so basically you dont need this if using php5 ?

pwolanin’s picture

Do we want to provide this for people using PHP 5.2+? I guess the solution is to just wrap the code in an if block and check the PHP version.

I.e. similar to the code in system.install:


  if (version_compare(phpversion(), '5.2.0') >= 0) {
    extract(session_get_cookie_params(), EXTR_PREFIX_ALL, 'session_')
    // Set "httponly" to TRUE to reduce the risk of session stealing via XSS.
    // This has no effect for PHP < 5.2.0.
    session_set_cookie_params($session_lifetime, $session_path, $session_domain, $session_secure, TRUE);
  }
andypost’s picture

Suppose version check is required but what's about people using lower php versions?

pwolanin’s picture

People using lower versions of PHP will (continue to) lack this feature. We suggest you get on PHP 5.2+ in general.

Gábor Hojtsy’s picture

Title: Hardening - session regeneration » Use httponly cookie support when available
Category: bug » task
Priority: Critical » Normal

@pwolanin: let's roll a patch with this change then :) The original issue was marked critical, but this hardening is not critical anymore. Also retitled for what we are looking at.

vitis’s picture

sub

andypost’s picture

Status: Needs work » Needs review
FileSize
878 bytes

Patch from #45

pwolanin’s picture

This looks reasonable - thought maybe easier to do an ini_set?

http://us.php.net/manual/en/session.configuration.php#ini.session.cookie...

In fact - is this a work-around for older php versions? Seems like PHP ingnores ini_set() for a setting it doesn't know about?

pwolanin’s picture

From the php docs, sounds like it would be safer to do this before we call session_start()

Testing the above patch - I'm not seeing it have effect. What am I missing?

I think we could replace the changes with:

ini_set('session.cookie_httponly', TRUE);
andypost’s picture

@pwolanin I think ini_set could not work on some hostings so this require more reviews

pwolanin’s picture

@andypost - it's INI_ALL, so it shoudl work equally well.

pwolanin’s picture

Note that Drupal 7 is now actually doing this initially via init_set:

includes/bootstrap.inc:537:  // Use httponly session cookies.
includes/bootstrap.inc:538:  ini_set('session.cookie_httponly', '1');

Status: Needs review » Needs work

The last submitted patch, sess_hardening_280934-27.patch, failed testing.

pwolanin’s picture

Issue tags: +Security

bumping this - we should still get it into 6

damiankloip’s picture

Status: Needs work » Needs review
FileSize
682 bytes

Rerolled a git patch.

andypost’s picture

I think we still need to implement ini_set() as D7 does

damiankloip’s picture

FileSize
1.16 KB

Yeah, let's do that.

andypost’s picture

+++ b/sites/default/default.settings.phpundefined
@@ -165,6 +165,7 @@ ini_set('session.use_cookies',      1);
+ini_set('session.cookie_httponly', '1');

Suppose proper place for that in conf_init() because D7 does it in drupal_environment_initialize()

mgifford’s picture

Is this fixed in D7 & D8? Will it get rolled into D6 at this point in the release cycle?

mgifford’s picture

60: 280934-60.patch queued for re-testing.

mgifford’s picture

Issue summary: View changes
Status: Needs review » Closed (won't fix)

Seems to be in D7 & D8 so marking this closed.

includes/bootstrap.inc: ini_set('session.cookie_httponly', '1');
core/includes/bootstrap.inc: ini_set('session.cookie_httponly', '1');

We're not looking to add this to the D6 version I assume.

RytoEX’s picture

Is there a specific reason that this is not going into D6? Cookies that lack HTTPOnly get flagged in OWASP security compliance tests.