ElkArte Community

Elk Development => Feature Discussion => Topic started by: ahrasis on April 11, 2019, 03:25:20 am

Title: Option to limit PHPSESSID cookie to https?
Post by: ahrasis on April 11, 2019, 03:25:20 am
I was checking on why my test forum doesn't have a full green padlock when I accidentally looked into its cookie details and found out that its cookie is sent for secure connections only but PHPSESSID is sent for any kind of connection (not secure) even when logged with an admin account.

I am not sure whether this ok (or bad) but I was thinking of why not check if the forum is using https and limit all PHPSESSID cookies to https only. Or if that is too much, may be only limit the administrator's PHPSESSID cookie to https. But may be best if both options are made available.

This may be nothing to worry about in the current way and I also have no idea on how this would benefit the forum users or admins and how to secure PHPSESSID cookie yet.
Title: Re: Option to limit PHPSESSID cookie to https?
Post by: ahrasis on April 11, 2019, 04:12:40 am
After looking deeper into Session.php, I think it should work if we add something like this:
Code: [Select]
	@ini_set('arg_separator.output', '&');

// Secure PHPSESSIONID
if (parse_url($boardurl, PHP_URL_SCHEME)==='https')
@ini_set('session.cookie_secure', true);

But there is also code "to stop people from using bad junky PHPSESSIDs" in there, so I am not sure whether adding this is necessary or otherwise redundant, though my guess is securing it via https is better, if https is already used in the url.

What do you all think?
Title: Re: Option to limit PHPSESSID cookie to https?
Post by: ahrasis on April 11, 2019, 09:00:44 am
I think I PRed the above solution so it can be immediately added to 1.1.6 if agreeable: https://github.com/elkarte/Elkarte/pull/3303.
Title: Re: Option to limit PHPSESSID cookie to https?
Post by: Spuds on April 11, 2019, 02:55:10 pm
It seems a reasonable thing to enable (or try to) when httpS  The php manual has
Quote
Allow access to the session ID cookie only when the protocol is HTTPS. If a website is only accessible via HTTPS, it should enable this setting.
but I'm far from an expert on all things session related!

Title: Re: Option to limit PHPSESSID cookie to https?
Post by: badmonkey on April 15, 2019, 04:46:44 pm
Thanks for this @ahrasis‍  Security is always a top concern.

Using on 3 sites, all working perfectly.  8)
Title: Re: Option to limit PHPSESSID cookie to https?
Post by: badmonkey on April 16, 2019, 05:54:41 am
Woops. Seems I spoke too soon. Registration is, in fact, an issue. It leads to this error:

Code: [Select]
Wrong value type sent to the database. Integer expected. (id_member)
Title: Re: Option to limit PHPSESSID cookie to https?
Post by: ahrasis on April 16, 2019, 07:01:29 am
I guess we have to test it some more though my test on a newly installed forum reveals no error.

Edited: I searched and found that your mentioned error is actually an already reported bug (https://www.elkarte.net/community/index.php?topic=5510.0) for 1.1.5 which may have nothing to do with this PHPSESSID security fix. The said bug is already tracked (https://github.com/elkarte/Elkarte/issues/3278) and should be fixed by 1.1.6 patch.

Those who tested this PHPSESSID security fix and have any problems or error with their login or logout session or registration, please do report its outcome.
Title: Re: Option to limit PHPSESSID cookie to https?
Post by: badmonkey on April 16, 2019, 01:02:16 pm
Thanks @ahrasis‍  I'll give that a try. Odd the issue didn't arise until this code was changed. Or perhaps it was just bad luck? I dunno. Will report back soon!


Edit: ah...got it. The error was only thrown if there is a registration error. https cookies work perfectly and the fix in the referenced thread works as well. Cool!  8)
Title: Re: Option to limit PHPSESSID cookie to https?
Post by: ahrasis on April 16, 2019, 03:25:43 pm
Thank you for updating @badmonkey. Do report if there is any other problem / error found while using secure PHPSESSID cookie.

If none is further reported, hopefully this can get into 1.1.6 patches as well.