| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | user.module |
| Category: | task |
| Priority: | normal |
| Assigned: | pwolanin |
| Status: | needs work |
| Issue tags: | security |
Issue Summary
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;
}
?>
Comments
#1
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).
#2
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.
#3
Any suggestions on how to write the test? I think that would require a dummy module (and/or a dummy module hook?)
#4
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.
#5
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.
#6
@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:
hidden = TRUEto the test module's .info file,#7
Good to see this addressed.
Just deleted a longer comment but can't figure out how to delete this one altogether.
#8
The last submitted patch failed testing.
#9
Reroll patch (now uses drupal_session_regenerate)
#10
#11
The last submitted patch failed testing.
#12
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
#13
patch working on my machine as well: 7299 passes, 0 fails, and 0 exceptions
#14
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. :)
#15
@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.
#16
the full patch
#17
The last submitted patch failed testing.
#18
whoops - posted the test-only patch twice. And testbot has demonstrated that without the rest of the patch there are 2 fails...
#19
when (if) you reroll, there's an "incorrectoly" in there.
#20
w/ typo fix
#21
there is a trailing space on the line
session_set_cookie_params($lifetime, $path, $domain, $secure, TRUE);#22
ok - fixed (though I think webchick has also been removing trailing spaces before commit).
#23
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.
#24
ok, wrapped assertion text in t() as required.
#25
Thanks pwolanin. Great work as usual! :)
#26
is it worth backporting the httponly cookie part? I think adding that param will just be a no-op with older version of PHP.
#27
#28
Tested this locally and works with PHP 5.2 and Drupal 6.x
#29
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.
#30
@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?
#31
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.
#32
Committed to 5.x.
#33
Rolled back for 5.x. #345495: Wrong parameter count for session_set_cookie_params() error after drupal 5.13 update and 6.7 update
#34
Rolled back for 6.x.
#35
sub
#36
Incredible, so there *are* people using Drupal on PHP4 :)
#37
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);#38
@ dzhu, please read the issues before commenting (see #33, #34). Thank you.
#39
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?
#40
@dzhu Releases cannot be changed. The fix will go out as 6.8 and 5.14 later today.
#41
Drupal 6.8 and 5.14 is out with this change.
#42
What are the plans to go on with this issue?
#43
Investigation of HTTPOnly flag
This feature is partially supported by browsers
#44
so basically you dont need this if using php5 ?
#45
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:
<?phpif (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);
}
?>
#46
Suppose version check is required but what's about people using lower php versions?
#47
People using lower versions of PHP will (continue to) lack this feature. We suggest you get on PHP 5.2+ in general.
#48
@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.
#49
sub
#50
Patch from #45
#51
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?
#52
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);#53
@pwolanin I think ini_set could not work on some hostings so this require more reviews
#54
@andypost - it's INI_ALL, so it shoudl work equally well.
#55
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');
#56
The last submitted patch, sess_hardening_280934-27.patch, failed testing.
#57
bumping this - we should still get it into 6