Use httponly cookie support when available

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.

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

Reroll patch (now uses drupal_session_regenerate)

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

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

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

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

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.

Thanks pwolanin. Great work as usual! :)

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.

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.

Committed to 5.x.

Rolled back for 5.x. #345495: Wrong parameter count for session_set_cookie_params() error after drupal 5.13 update and 6.7 update

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 ?

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

@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

Patch from #45

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

Rerolled a git patch.

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

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.

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.

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