Advanced search of issues seems broken, so sorry that this may be a duplicate.

I have previously used session.cookie_secure with drupal sites to ensure secure authenticated access.

This depends on having different session names for the HTTP and HTTPS sites; otherwise, the HTTP site will not see the session cookie and so overwrite it with a new one. Thus, as soon as an authenticated HTTPS user visits the equivalent HTTP url, their original HTTPS session will be gone from the browser cookiejar.

What I need is either a way to "force" the session name, or, if drupal wants to require auto-generated session names, it needs to check if session.cookie_secure is enabled, and if so, generate different session names for the HTTP and HTTPS sites. I think a patch would be pretty simple.

Or if I'm just going about this the wrong way and no patch is needed please enlighten me :)

Comments

mfb’s picture

Status: Active » Needs review
FileSize
1.03 KB

Here's the patch I had in mind...

drumm’s picture

Status: Needs review » Closed (works as designed)

Setting $cookie_domain in settings.php directly sets the session name.

mfb’s picture

Status: Closed (works as designed) » Needs review

This doesn't help the situation. I have two sites -- an HTTP site and an HTTPS site -- with the same cookie domain, but different session names.

I either need a way to "force" the session name, or the patch, which automatically generates different session names if session.cookie_secure is in use.

drumm’s picture

I think this patch might be okay, but would like to see more reviews.

chx’s picture

Why differentitate? Just always use the protocol-less part. Why not?

moshe weitzman’s picture

you can do conditional cookie_domain. not sure if it helps something like

if (strpos('https', SERVER["SERVER_PROTOCOL"]) === FALSE) {
  $cookie_domain = 'foo';
}
else {
    $cookie_domain = 'bar';
}
mfb’s picture

The two sites, http://site.example.com/ and https://site.example.com/ have the same cookie domain, but have always used different session names (at least until I upgraded to drupal 5.2).

They need identical cookie domains because the cookies should apply to the same host name. They need different session names, otherwise the session cookies will overwrite each other in the browser.

drumm’s picture

This is important since in many situations you never want to
- Leak the cookie https-established cookie over http
- Use a cookie http-established cookie on https

Any reviews? I think the patch is okay, but I will not commit this patch without a second opinion or three.

JohnAlbin’s picture

Status: Needs review » Needs work

If the $cookie_domain is specified and ini_get('session.cookie_secure') is set, Mark’s patch won’t work.

Mark’s use-case is one that I hadn’t considered when we were fixing the session bugs in 5.2. I would need to think about this further before I could suggest a solution.

mfb’s picture

I agree there is a larger flaw in this function.. I was trying to find the least invasive way to fix it.

Currently if you set the cookie domain, then you are forcing the session name as well, so I didn't try to handle that case.

I'd like the ability to control cookie domain and session name separately, but would that still qualify as a bug fix or is that something for drupal 6?

JohnAlbin’s picture

I think I need to understand the use-case better.

If the session name was different for HTTP and HTTPS and the user logged in under HTTPS, that user would still be an anonymous user under the HTTP site. I don't see how this is useful. Why do you even have a HTTP version of the site?

The mod_rewrite rules could redirect people visiting the HTTP site to the HTTPS version. (Or would the redirect message include the session cookie?)

mfb’s picture

For an e-commerce site or any other site where security is important (perhaps even a blog for political dissidents in burma ;)
* You want to make the site available via HTTP for general readership.
* However, you want to restrict all authenticated sessions to HTTPS in order to prevent sessions from being hijacked (due to the unencrypted session key being passed over the network).

The answer is to force the session cookie to HTTPS only. Thus it will not be passed back to the HTTP site.

You do not, however, want to regenerate a new session every time the user views an HTTP page -- this would be unnecessary database/CPU overhead.

So you generate a non-secure session on the HTTP site.

In order to allow these secure and non-secure sessions to co-exist on the same cookie domain, you need to set different session names for the HTTP and HTTPS sitez. I used to use php/apache config to hard code the session names for each apache virtual host.

If you redirect all user logins to the HTTPS site, then the practical result is that all sessions on the HTTP site are unauthenticated anonymous sessions. All authenticated sessions are restricted to the HTTPS site.

JohnAlbin’s picture

Most e-commerce sites want to allow users to add items to a shopping cart under HTTP. And only do the payment under HTTPS. Mark, your e-commerce usage seems odd, but I can imagine a more generic application that would require HTTPS only access while still allowing anonymous HTTP access.

I’ve researched ini_get('session.cookie_secure') on the php.net website and Mark’s description of what is going on seems more like a bug in PHP. From the PHP docs:

session.cookie_secure specifies whether [session] cookies should only be sent over secure connections

So if session.cookie_secure is set to true, PHP shouldn’t be sending Drupal’s session cookie over HTTP. But it sounds like PHP is generating a new session ID on each HTTP request. Which isn’t what the PHP docs says it would do.

Can anyone confirm that PHP is acting this way?

mfb’s picture

Yes this it typical, thus you have anonymous shopping carts via HTTP, and require HTTPS during login/registration/checkout. (This varies, I've also done sites where only authenticated users could shop.)

It's not a PHP bug. When a client accepts a secure-only session cookie, it won't be handed back to a HTTP site. If you go back to the HTTP site at this point, Drupal will automatically generate a new session cookie because it didn't receive one. The HTTP site should not be configured to issue secure-only session cookies -- you do want to maintain anonymous sessions, rather than generate a new session on every page request.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
1.33 KB

Okay, now I get it. You have two separate Apache configs. The HTTPS config has ini_get('session.cookie_secure') On and the HTTP config has it Off.

Here's a patch that should allow separate session names for HTTP and HTTPS. The flag to enable this functionality is if ini_get('session.cookie_secure') is true.

Since Apache requires a separate config for HTTP versus HTTPS, I think that trigger is reasonable. Also, one could toggle ini_get('session.cookie_secure') config in the settings.php file based on the protocol.

Is this yet another thing that we need to document in the settings.php file?

JohnAlbin’s picture

Status: Needs review » Needs work

Nevermind. That patch is incompatible with:

Drupal automatically generates a unique session cookie name for each site based on on its full domain name. If you have multiple domains pointing at the same Drupal site, you can either redirect them all to a single domain (see comment in .htaccess), or uncomment the line below and specify their shared base domain. Doing so assures that users remain logged in as they cross between your various domains.

I’ll have to re-do it.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
804 bytes

Try this one.

mfb’s picture

The one edge case for this would be if you have multiple SSL-only drupal sites on an overlapping cookie domain. In this case, I guess you would have to use session.cookie_path to differentiate them.

mfb’s picture

oh never mind, just noticed the "." in ".=" :P
Thanks for the patch, this should do it.

bjaspan’s picture

Subscribe.

Please see http://drupal.org/node/66970 and http://drupal.org/node/65371#comment-123944 for my past analysis and proposed solution to this problem. It's late now, I'll read this issue more carefully in the morning.

mfb’s picture

Simply using two different session names, one for HTTP and one for HTTPS, worked fine for me (in the past, when this was a little easier on users). What I'm trying to do here is just make this work nicely in drupal core itself, without requiring an additional module. Although of course i support any larger scale improvements to usage of SSL in drupal ;)

As I mentioned above, the only problem we have is users going back to the HTTP site and overwriting their HTTPS cookie, due to drupal forcing identical session name on the two cookies.

mfb’s picture

Version: 5.x-dev » 7.x-dev
FileSize
792 bytes

I rerolled this patch so it applies cleanly. Let's try to fix this in drupal 7 (at least)?

keith.smith’s picture

Status: Needs review » Needs work

Very minor, but we usually capitalize acronyms in comments, even HTTP and HTTPS.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
792 bytes

ok.

JohnAlbin’s picture

Title: How do you use session.cookie_secure with drupal if you cannot set session.name? » session.cookie_secure: SSL cookie gets over-written by non-SSL cookie
Leeteq’s picture

Subscribing.

Leeteq’s picture

Subscribing.

mfb’s picture

Thanks for the improved title. Can anyone give this *really* simple patch a quick review?

JohnAlbin’s picture

Actually, Mark, since its my patch, you could review it. :-)

mfb’s picture

Status: Needs review » Reviewed & tested by the community

I hereby dub this patch reviewed and tested.
Applies cleanly, minimal code and works as advertised.

dharamgollapudi’s picture

Subscribing...

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Placement of the code comments is not consistent with the rest of the code. Would be great if the comment explained why you added 'SSL' and not something else, or why a different name needs to be used.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
900 bytes

Updated the comments.

mfb’s picture

This comment is clear to me. But just in case it's not clear to anyone else, here's yet another wording..:
"If the user specifies secure session cookies, in order to prevent a cookie collision between the HTTPS and HTTP sites, we force the browser to use different cookies by setting different session names for the HTTPS and HTTP sites."

mfb’s picture

Status: Needs review » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Needs work

I suggest that we leave out the more complex dot-handling for now. It's probably better to focus on testing, and to introduce the complex dot-handling when we have a real need for it, and when we can actually write tests for it.

Damien Tournoud’s picture

@Dries: I'm not sure what your comment meant. Did you answer to the correct issue?

Dries’s picture

I commented on the wrong issue, sowwy.

Damien Tournoud’s picture

Status: Needs work » Reviewed & tested by the community

So this is still in reviewed & tested mode, than.

I also support that patch.

Dries’s picture

Does that mean you can't change 'http://' to 'https://' without getting logged out?

mfb’s picture

Without this patch, if you use session.cookie_secure then changing from https to http will log you out (even if you go back to https, because the http session cookie overwrote the https session cookie). With this patch, that will no longer happen.

For those not using session.cookie_secure (vast majority of sites, unfortunately..), nothing changes.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

Dries question made me realize that the patch is wrong. We should add "SSL" only when we are in HTTPS mode. Thus, we also need the check that moshe suggested in #6.

mfb’s picture

Status: Needs work » Reviewed & tested by the community

I wouldn't say that's necessary. If you have session.cookie_secure on a non-secure site, you have much bigger problems on your hands (like, no one can have a session, login, etc.)

I would only add this logic if we had a real-world case where a HTTP site would set a cookie that only the HTTPS site could read.

Also, moshe's suggestion doesn't work if you need both the HTTP and HTTPS sites to have the same cookie domain.

Damien Tournoud’s picture

Ok. I was under the (false) impression that session.cookie_secure only sets the secure flag on HTTPS connexions. But you are right.

The patch looks good as it is.

JohnAlbin’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
929 bytes

Damien’s confusion is something that others (including myself) could easily make. So I’ve made a minor tweak to the comments again.

“If the user specifies secure session cookies, the browser will use two different cookies for HTTPS and HTTP.”
  becomes:
“If the user specifies secure session cookies on the SSL version of a site, the browser will use two different cookies for HTTPS and HTTP.”

Only the doc change needs review.

Damien Tournoud’s picture

Looks good to me.

bjaspan’s picture

Status: Needs review » Reviewed & tested by the community

I remain concerned that this is a very-site specific hack that will make a more general solution more difficult because the more general solution will "break this feature."

In #20 I referred to posts on this topic. I suggest that Drupal should create two cookies when a user logs in via SSL: a secure and non-secure cookie. When the site is accessed via SSL, Drupal checks that both cookies are valid; otherwise, it only uses the non-secure cookie (which is the only one it has in this case). It is then up to other code (e.g. securepages) to enforce the site's rules for which pages must be accessed via SSL.

Damien Tournoud’s picture

@bjaspan: This patch does not break the implementation of another solution. If you are in cookie_secure mode, you could not create a non-secure cookie anyway.

mfb’s picture

Preventing hijacked sessions is easy, you simply use session.cookie_secure.

This issue was initially created as a bug report for drupal 5, because drupal 5 made this configuration rather unpleasant by no longer allowing session names to be customized in the apache config (as a result a user could easily have their authenticated secure session cookie overwritten by an unauthenticated non-secure session cookie).

I'd still like to see this patch backported to 5 and 6 (I've already had to patch all sites where I needed to use secure session cookies) and I don't feel like any extra functionality is required.

If we want to build a session.cookie_secure option into drupal that might be a nice to have; on the other hand configuration via apache/php directives is quite simple. In any case this patch would be a prerequisite for that to work -- restoring the ability to separate SSL and non-SSL session cookies on the same domain.

JohnAlbin’s picture

If you are in cookie_secure mode, you could not create a non-secure cookie anyway.

Actually, if you are in session.cookie_secure mode, you cannot create a non-secure session cookie. Non-session cookies shouldn't be affected by that setting.

Barry, I've looked carefully at your proposal to make sure it’s not incompatible with this patch.

However, the session.cookie_secure setting sounds like it IS incompatible with your proposal (it would prevent the non-SSL session cookie from being used by the SSL site.) But that's a PHP setting, not a drupal setting. This patch just makes drupal work with that PHP setting; it doesn't force anyone to use session.cookie_secure.

Mark, we'll definitely get this backported to D6 and D5!

Dries’s picture

I'd like to see a final reply from Barry before I commit this.

mfb’s picture

I take it virtually no one is using secure cookies with drupal or this would make more progress.. If anyone can help resolve this issue I could even offer a reward.. a plush hand-knitted druplicon!! :)

bjaspan’s picture

I apologize for not getting back to this. I'll try to do so this week.

Dries’s picture

Status: Reviewed & tested by the community » Needs review

I'll mark this 'code needs review' awaiting bjaspan's review.

JohnAlbin’s picture

Barry? I think my assessment (in #50) of your patch is valid. Can we move forward on this issue? I'll let Barry or Dries bump this back to RTBC.

mfb’s picture

FileSize
341.86 KB

OK I have other incentives that could be offered to work on this issue, like a genuine, vintage Drupal pokemon card!!

In an age of massive DNS vulnerabilities and blanket wiretapping plus normal dangers of sniffed session cookies, we should make sure there are no inconveniences for sites that want to use secure session cookies.

gpanula’s picture

subscribing.... and I'll add $10 to pool

populist’s picture

I saw a presentation at Defcon (a security conference) last weekend where an attack exploiting this weakness was discussed. There is some good information on the presenter's blog: Fully Automatic Active HTTPS Cookie Hijacking.

Dries’s picture

Status: Needs review » Needs work

The link in #58 is handy. I think we can improve our PHP comments further based on that. I think it is useful to document in the code what this patch prevents from doing. If not, many of us might be scratching our heads 2 years down the road. I'll commit the next version of this patch.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
1.08 KB

I expanded the comment to say why a user might choose to use session.cookie_secure on the SSL version of their website.

Dries’s picture

Version: 7.x-dev » 5.x-dev
Status: Needs review » Needs work

I've committed this to CVS HEAD and DRUPAL-6. I'm updating the version to 5.x and marking it code needs work (needs a quick re-roll). Thanks mfb and John.

christefano’s picture

Status: Needs work » Needs review
FileSize
883 bytes

Rerolled for Drupal 5.

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

Hmm... the patch in #60 applies to DRUPAL-5 for me. “Hunk #1 succeeded at 290 (offset -83 lines).”

But ignoring that, the patch in #62 is functionally identical. So, RTBC! :-)

Robin Monks’s picture

#63: +1

baldwinlouie’s picture

subscribing

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 5.x. (sorry or the delay, on vacation)

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.