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; }
Comment | File | Size | Author |
---|---|---|---|
#60 | 280934-60.patch | 1.16 KB | damiankloip |
#58 | 280934-58.patch | 682 bytes | damiankloip |
#50 | 280934_session_hardening_d6.patch | 878 bytes | andypost |
#28 | sess_hardening_280934-27.patch | 819 bytes | pwolanin |
#24 | sess_regen_hardening_280934-23.patch | 6.59 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedpatch 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).
Comment #2
Dries CreditAttribution: Dries commented1. 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.
Comment #3
pwolanin CreditAttribution: pwolanin commentedAny suggestions on how to write the test? I think that would require a dummy module (and/or a dummy module hook?)
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedGreat, 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.
Comment #5
pwolanin CreditAttribution: pwolanin commentedAdded 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.
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commented@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 = TRUE
to the test module's .info file,Comment #7
gram.steve CreditAttribution: gram.steve commentedGood to see this addressed.
Just deleted a longer comment but can't figure out how to delete this one altogether.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #9
swentel CreditAttribution: swentel commentedReroll patch (now uses drupal_session_regenerate)
Comment #10
pwolanin CreditAttribution: pwolanin commentedComment #12
pwolanin CreditAttribution: pwolanin commentedfailure 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
Comment #13
scor CreditAttribution: scor commentedpatch working on my machine as well: 7299 passes, 0 fails, and 0 exceptions
Comment #14
webchickUp 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. :)
Comment #15
pwolanin CreditAttribution: pwolanin commented@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.
Comment #16
pwolanin CreditAttribution: pwolanin commentedthe full patch
Comment #18
pwolanin CreditAttribution: pwolanin commentedwhoops - posted the test-only patch twice. And testbot has demonstrated that without the rest of the patch there are 2 fails...
Comment #19
keith.smith CreditAttribution: keith.smith commentedwhen (if) you reroll, there's an "incorrectoly" in there.
Comment #20
pwolanin CreditAttribution: pwolanin commentedw/ typo fix
Comment #21
scor CreditAttribution: scor commentedthere is a trailing space on the line
session_set_cookie_params($lifetime, $path, $domain, $secure, TRUE);
Comment #22
pwolanin CreditAttribution: pwolanin commentedok - fixed (though I think webchick has also been removing trailing spaces before commit).
Comment #23
catchThis 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.
Comment #24
pwolanin CreditAttribution: pwolanin commentedok, wrapped assertion text in t() as required.
Comment #25
Dries CreditAttribution: Dries commentedThanks pwolanin. Great work as usual! :)
Comment #26
pwolanin CreditAttribution: pwolanin commentedis it worth backporting the httponly cookie part? I think adding that param will just be a no-op with older version of PHP.
Comment #27
pwolanin CreditAttribution: pwolanin commentedComment #28
pwolanin CreditAttribution: pwolanin commentedTested this locally and works with PHP 5.2 and Drupal 6.x
Comment #29
Gábor HojtsyExtract 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.
Comment #30
pwolanin CreditAttribution: pwolanin commented@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?
Comment #31
Gábor HojtsyEXTR_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.
Comment #32
drummCommitted to 5.x.
Comment #33
drummRolled back for 5.x. #345495: Wrong parameter count for session_set_cookie_params() error after drupal 5.13 update and 6.7 update
Comment #34
Gábor HojtsyRolled back for 6.x.
Comment #35
hass CreditAttribution: hass commentedsub
Comment #36
Damien Tournoud CreditAttribution: Damien Tournoud commentedIncredible, so there *are* people using Drupal on PHP4 :)
Comment #37
dzhu CreditAttribution: dzhu commentedFrom 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);
Comment #38
Heine CreditAttribution: Heine commented@ dzhu, please read the issues before commenting (see #33, #34). Thank you.
Comment #39
dzhu CreditAttribution: dzhu commentedAccording 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?
Comment #40
Heine CreditAttribution: Heine commented@dzhu Releases cannot be changed. The fix will go out as 6.8 and 5.14 later today.
Comment #41
Gábor HojtsyDrupal 6.8 and 5.14 is out with this change.
Comment #42
michtoen CreditAttribution: michtoen commentedWhat are the plans to go on with this issue?
Comment #43
andypostInvestigation of HTTPOnly flag
This feature is partially supported by browsers
Comment #44
expatme CreditAttribution: expatme commentedso basically you dont need this if using php5 ?
Comment #45
pwolanin CreditAttribution: pwolanin commentedDo 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:
Comment #46
andypostSuppose version check is required but what's about people using lower php versions?
Comment #47
pwolanin CreditAttribution: pwolanin commentedPeople using lower versions of PHP will (continue to) lack this feature. We suggest you get on PHP 5.2+ in general.
Comment #48
Gábor Hojtsy@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.
Comment #49
vitis CreditAttribution: vitis commentedsub
Comment #50
andypostPatch from #45
Comment #51
pwolanin CreditAttribution: pwolanin commentedThis 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?
Comment #52
pwolanin CreditAttribution: pwolanin commentedFrom 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:
Comment #53
andypost@pwolanin I think ini_set could not work on some hostings so this require more reviews
Comment #54
pwolanin CreditAttribution: pwolanin commented@andypost - it's INI_ALL, so it shoudl work equally well.
Comment #55
pwolanin CreditAttribution: pwolanin commentedNote that Drupal 7 is now actually doing this initially via init_set:
Comment #57
pwolanin CreditAttribution: pwolanin commentedbumping this - we should still get it into 6
Comment #58
damiankloip CreditAttribution: damiankloip commentedRerolled a git patch.
Comment #59
andypostI think we still need to implement
ini_set()
as D7 doesComment #60
damiankloip CreditAttribution: damiankloip commentedYeah, let's do that.
Comment #61
andypostSuppose proper place for that in
conf_init()
because D7 does it indrupal_environment_initialize()
Comment #62
mgiffordIs this fixed in D7 & D8? Will it get rolled into D6 at this point in the release cycle?
Comment #63
mgifford60: 280934-60.patch queued for re-testing.
Comment #64
mgiffordSeems 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.
Comment #65
RytoEX CreditAttribution: RytoEX commentedIs there a specific reason that this is not going into D6? Cookies that lack HTTPOnly get flagged in OWASP security compliance tests.