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 :)
Comment | File | Size | Author |
---|---|---|---|
#62 | cookie-secure-5.x-170310.patch | 883 bytes | christefano |
#60 | cookie-secure.patch | 1.08 KB | JohnAlbin |
#56 | pokemon-drupal.png | 341.86 KB | mfb |
#45 | cookie-secure.patch | 929 bytes | JohnAlbin |
#33 | cookie-secure.patch | 900 bytes | JohnAlbin |
Comments
Comment #1
mfbHere's the patch I had in mind...
Comment #2
drummSetting $cookie_domain in settings.php directly sets the session name.
Comment #3
mfbThis 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.
Comment #4
drummI think this patch might be okay, but would like to see more reviews.
Comment #5
chx CreditAttribution: chx commentedWhy differentitate? Just always use the protocol-less part. Why not?
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedyou can do conditional cookie_domain. not sure if it helps something like
Comment #7
mfbThe 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.
Comment #8
drummThis 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.
Comment #9
JohnAlbinIf 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.
Comment #10
mfbI 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?
Comment #11
JohnAlbinI 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?)
Comment #12
mfbFor 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.
Comment #13
JohnAlbinMost 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:
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?
Comment #14
mfbYes 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.
Comment #15
JohnAlbinOkay, 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?
Comment #16
JohnAlbinNevermind. That patch is incompatible with:
I’ll have to re-do it.
Comment #17
JohnAlbinTry this one.
Comment #18
mfbThe 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.
Comment #19
mfboh never mind, just noticed the "." in ".=" :P
Thanks for the patch, this should do it.
Comment #20
bjaspan CreditAttribution: bjaspan commentedSubscribe.
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.
Comment #21
mfbSimply 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.
Comment #22
mfbI rerolled this patch so it applies cleanly. Let's try to fix this in drupal 7 (at least)?
Comment #23
keith.smith CreditAttribution: keith.smith commentedVery minor, but we usually capitalize acronyms in comments, even HTTP and HTTPS.
Comment #24
JohnAlbinok.
Comment #25
JohnAlbinComment #26
Leeteq CreditAttribution: Leeteq commentedSubscribing.
Comment #27
Leeteq CreditAttribution: Leeteq commentedSubscribing.
Comment #28
mfbThanks for the improved title. Can anyone give this *really* simple patch a quick review?
Comment #29
JohnAlbinActually, Mark, since its my patch, you could review it. :-)
Comment #30
mfbI hereby dub this patch reviewed and tested.
Applies cleanly, minimal code and works as advertised.
Comment #31
dharamgollapudi CreditAttribution: dharamgollapudi commentedSubscribing...
Comment #32
Dries CreditAttribution: Dries commentedPlacement 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.
Comment #33
JohnAlbinUpdated the comments.
Comment #34
mfbThis 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."
Comment #35
mfbComment #36
Dries CreditAttribution: Dries commentedI 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.
Comment #37
Damien Tournoud CreditAttribution: Damien Tournoud commented@Dries: I'm not sure what your comment meant. Did you answer to the correct issue?
Comment #38
Dries CreditAttribution: Dries commentedI commented on the wrong issue, sowwy.
Comment #39
Damien Tournoud CreditAttribution: Damien Tournoud commentedSo this is still in reviewed & tested mode, than.
I also support that patch.
Comment #40
Dries CreditAttribution: Dries commentedDoes that mean you can't change 'http://' to 'https://' without getting logged out?
Comment #41
mfbWithout 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.
Comment #42
Damien Tournoud CreditAttribution: Damien Tournoud commentedDries 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.
Comment #43
mfbI 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.
Comment #44
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk. 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.
Comment #45
JohnAlbinDamien’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.
Comment #46
Damien Tournoud CreditAttribution: Damien Tournoud commentedLooks good to me.
Comment #47
bjaspan CreditAttribution: bjaspan commentedI 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.
Comment #48
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.
Comment #49
mfbPreventing 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.
Comment #50
JohnAlbinActually, 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!
Comment #51
Dries CreditAttribution: Dries commentedI'd like to see a final reply from Barry before I commit this.
Comment #52
mfbI 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!! :)
Comment #53
bjaspan CreditAttribution: bjaspan commentedI apologize for not getting back to this. I'll try to do so this week.
Comment #54
Dries CreditAttribution: Dries commentedI'll mark this 'code needs review' awaiting bjaspan's review.
Comment #55
JohnAlbinBarry? 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.
Comment #56
mfbOK 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.
Comment #57
gpanula CreditAttribution: gpanula commentedsubscribing.... and I'll add $10 to pool
Comment #58
populist CreditAttribution: populist commentedI 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.
Comment #59
Dries CreditAttribution: Dries commentedThe 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.
Comment #60
JohnAlbinI expanded the comment to say why a user might choose to use session.cookie_secure on the SSL version of their website.
Comment #61
Dries CreditAttribution: Dries commentedI'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.
Comment #62
christefano CreditAttribution: christefano commentedRerolled for Drupal 5.
Comment #63
JohnAlbinHmm... 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! :-)
Comment #64
Robin Monks CreditAttribution: Robin Monks commented#63: +1
Comment #65
baldwinlouie CreditAttribution: baldwinlouie commentedsubscribing
Comment #66
drummCommitted to 5.x. (sorry or the delay, on vacation)
Comment #67
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.