Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cookies should be set as 'secure' over SSL #1756

Closed
rcubetrac opened this issue Sep 11, 2008 · 6 comments

Comments

@rcubetrac
Copy link

commented Sep 11, 2008

Reported by mkj on 11 Sep 2008 15:57 UTC as Trac ticket #1485336

Even if a website is set up using SSL, an active attacker can steal cookies unless the cookies have been set 'secure' - see http://fscked.org/blog/fully-automated-active-https-cookie-hijacking

Roundcube doesn't set cookies as secure. It looks like the only place that needs changing is the second setcookie() in
session.inc

Something like

setcookie(session_name(), $random, $lifetime, $cookie[$cookie['domain']('path'],),  $_SERVER["HTTPS"]);

might be the way to go? (I haven't used PHP much so am guessing from the docs).

Migrated-From: http://trac.roundcube.net/ticket/1485336

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Sep 12, 2008

Milestone changed by @alecpl on 12 Sep 2008 06:03 UTC

later => 0.2-beta

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Sep 12, 2008

Comment by robin on 12 Sep 2008 06:48 UTC

Index: program/include/session.inc
===================================================================
--- program/include/session.inc (revision 1760)
+++ program/include/session.inc (working copy)
@@ -184,7 +184,8 @@
   $lifetime = $cookie[? time() + $cookie['lifetime']('lifetime']) : 0;

   setcookie(session_name(), '', time() - 3600);
-  setcookie(session_name(), $random, $lifetime, $cookie[$cookie['domain']('path'],));
+  setcookie(session_name(), $random, $lifetime, $cookie[$cookie['domain']('path'],),
+                       $_SERVER[&& ($_SERVER['HTTPS']('HTTPS'])!='off'));

   return true;
 }

I have no test-RC running over https so cannot test that. Over http this doesn't have any impact.

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Sep 12, 2008

Comment by mkj on 12 Sep 2008 17:32 UTC

I've tested that patch here (with roundcube 0.1.1 though) and it sets the logged-in sessionid cookie as secure, but doesn't set the initial pre-login session cookie as secure. I'm not sure if that really matters though? Does the pre-login session cookie get used for anything?

session_set_cookie_params() could be used to setup the initial cookie as secure I think.

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Sep 15, 2008

Comment by tensor on 15 Sep 2008 09:19 UTC

I guess the prelogin cookie should also be secured.

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Sep 18, 2008

Comment by robin on 18 Sep 2008 11:54 UTC

Pre-login does not need to be 'secured'. Session cookie security fixed in d0b973c.

@rcubetrac

This comment has been minimized.

Copy link
Author

commented Sep 18, 2008

Status changed by robin on 18 Sep 2008 11:54 UTC

new => closed

@rcubetrac rcubetrac closed this Sep 18, 2008
@rcubetrac rcubetrac added this to the 0.2-beta milestone Mar 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.