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:

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

<?php
...
// 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;
}
?>
Files: 
CommentFileSizeAuthor
#60 280934-60.patch1.16 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#58 280934-58.patch682 bytesdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#50 280934_session_hardening_d6.patch878 bytesandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 280934_session_hardening_d6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 sess_hardening_280934-27.patch819 bytespwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sess_hardening_280934-27.patch.
[ View ]
#24 sess_regen_hardening_280934-23.patch6.59 KBpwolanin
Passed: 7522 passes, 0 fails, 0 exceptions
[ View ]
#22 sess_regen_hardening_280934-22.patch6.58 KBpwolanin
Passed: 7522 passes, 0 fails, 0 exceptions
[ View ]
#20 sess_regen_hardening_280934-20a.patch6.57 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sess_regen_hardening_280934-20a.patch.
[ View ]
#18 sess_regen_hardening_280934-18.patch6.57 KBpwolanin
Passed: 7522 passes, 0 fails, 0 exceptions
[ View ]
#16 sess_regen_hardening_280934-16.patch4.96 KBpwolanin
Failed: 7326 passes, 2 fails, 0 exceptions
[ View ]
#15 sess_regen_tests_only_280934-15a.patch4.96 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sess_regen_tests_only_280934-15a.patch.
[ View ]
#12 sess_regen_hardening_280934-12.patch1.65 KBpwolanin
Unable to apply patch sess_regen_hardening_280934-12.patch
[ View ]
#9 sess_regen_hardening_280934-9.patch1.04 KBswentel
Failed: Failed to install HEAD.
[ View ]
#5 sess_regen_hardening_280934-5.patch981 bytespwolanin
Failed: Failed to apply patch.
[ View ]
#1 sess_regen_hardening_280934-1.patch771 bytespwolanin
Failed: Failed to apply patch.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new771 bytes
Failed: Failed to apply patch.
[ View ]

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

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new981 bytes
Failed: Failed to apply patch.
[ View ]

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.

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

Good to see this addressed.

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

Status:Needs review» Needs work

The last submitted patch failed testing.

StatusFileSize
new1.04 KB
Failed: Failed to install HEAD.
[ View ]

Reroll patch (now uses drupal_session_regenerate)

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

Assigned:Unassigned» pwolanin
Status:Needs work» Needs review
StatusFileSize
new1.65 KB
Unable to apply patch sess_regen_hardening_280934-12.patch
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

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

StatusFileSize
new4.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sess_regen_tests_only_280934-15a.patch.
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new4.96 KB
Failed: 7326 passes, 2 fails, 0 exceptions
[ View ]

the full patch

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.57 KB
Passed: 7522 passes, 0 fails, 0 exceptions
[ View ]

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

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

StatusFileSize
new6.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sess_regen_hardening_280934-20a.patch.
[ View ]

w/ typo fix

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new6.58 KB
Passed: 7522 passes, 0 fails, 0 exceptions
[ View ]

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

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.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new6.59 KB
Passed: 7522 passes, 0 fails, 0 exceptions
[ View ]

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

Status:Reviewed & tested by the community» Fixed

Thanks pwolanin. Great work as usual! :)

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.

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

Status:Patch (to be ported)» Needs review
StatusFileSize
new819 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sess_hardening_280934-27.patch.
[ View ]

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

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.

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

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.

Status:Reviewed & tested by the community» Fixed

Committed to 5.x.

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

Status:Reviewed & tested by the community» Needs work

Rolled back for 6.x.

sub

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

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

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

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?

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

Drupal 6.8 and 5.14 is out with this change.

What are the plans to go on with this issue?

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

so basically you dont need this if using php5 ?

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:

<?php
 
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);
  }
?>

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

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

Title: Hardening - session regenerationUse 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.

sub

Status:Needs work» Needs review
StatusFileSize
new878 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 280934_session_hardening_d6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch from #45

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?

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

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

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

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.

Issue tags:+security

bumping this - we should still get it into 6

Status:Needs work» Needs review
StatusFileSize
new682 bytes
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Rerolled a git patch.

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

StatusFileSize
new1.16 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Yeah, let's do that.

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

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

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

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.