Skip to content

Make session cookies httponly by default #13720

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DaWe35
Copy link

@DaWe35 DaWe35 commented Mar 15, 2024

Story

My website I wrote 9 years ago was hacked recently. It was quite tricky to exploit (I mean it took 9 years for someone to discover the bug), but I made a mistake and my website was vulnerable to XSS.
I'm a self-taught programmer and PHP was the first language I learnt. I think 9 years ago I did a pretty good job eliminating XSS and other injections, but I never expected anyone to go this deep in a hobby website and actually compose a working exploit.

Importance of default values

With the injected code, the attacker was able to steal session cookies. I'm trying hard to make session cookies httponly on all my websites, but somehow this one was not set (probably because of the age of the site). I believe this attack wouldn't have any consequences if httponly were 1 by default. I hope my input helps to make the web better, and I'm sorry if my knowledge lacks. I really believe defaults should be safe (I tried to read all the related discussions eg. #7913)

httponly

I believe it is uncommon that a website needs javascript access for a session cookie*, therefore making httponly=1 by default would not break many websites. I also believe it would make a lot of website much safer, since there are a tons of websites with the insecure default. Changing this default value is a net positive in my opinion.

  • it is not really useful to read a session id by JS, because it's just a random hash without the session data. Websites usually pass actual session values (and not the hash itself) to JS for example by echoing them. When sending an Ajax request to the same domain, the cookie is already part of the request header, further reducing the need of the token access by JS

use_strict_mode

According to php.net:

Enabling session.use_strict_mode is mandatory for general session security. All sites are advised to enable this. See session_create_id() example code for more details.

My only question is that why it's not default? I can hardly imagine changing this variable resulting in any damage or broken code. If it is advised to be 1, I think it should be 1 by default.

secure ??

In this PR, I did not include any changes related to secure, but I'd be happy to make more defaults secure.

I think changing secure to 1 would be amazing, but I also understand that this can break things (for example development mostly happens on localhost without SSL, and some websites still run in flexible SSL mode on Cloudflare). I'm not sure what is the best approach to this, I think we should still try to move forward with secure defaults, but displaying a helpful error message is essential so at least dev envs can be fixed in no time, when this config variable is missing in the local dev env.

Why not all cookies, why only sessions?

In this PR, I only changed defaults related to session cookies
I'm not sure if it's a good idea to set all cookies to httponly=1, but as I said, the safer is better, so I can support that. It will cause inconvenience and break things in some situations, therefore I'm not changing any setcookie() or setrawcookie() defaults now.

@devnexen
Copy link
Member

Thanks. You would need to have a look at the failed tests :

=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
Bug #74892 Url Rewriting (trans_sid) not working on urls that start with # [ext/session/tests/bug74892.phpt]
Bug #80774 (session_name() problem with backslash) [ext/session/tests/bug80774.phpt]
GH-9200: setcookie has an obsolete expires date format [ext/session/tests/gh9200.phpt]
Test session_regenerate_id() function : basic functionality [ext/session/tests/session_regenerate_id_cookie.phpt]

@jorgsowa
Copy link
Contributor

Regarding hardening use_strict_mode there was RFC delined a few years ago: https://wiki.php.net/rfc/session-use-strict-mode

The benefit of the change is questionable. How does it strengthen security? In my opinion, it just gives the information to the attackers on which session ID is occupied or not. With this option disabled, attackers don't get this information as every session ID is valid for them.

Regarding httponly the RFC was also declined a few years ago, but I think this topic should be revised again: https://wiki.php.net/rfc/precise_session_management

@DaWe35
Copy link
Author

DaWe35 commented Mar 17, 2024

Hi @jorgsowa,
Thank you for your answer. Can I ask why php.net recommends the usage of use_strict_mode=1 if the benefit is questionable, as you say? I'm getting really confused, now I don't even know what setting to use on my server, but there is an official website what clearly says

Enabling session.use_strict_mode is mandatory for general session security.

Is php.net wrong then?

@jorgsowa
Copy link
Contributor

I can't find the proper discussion about this topic right now. One of them you have here: https://externals.io/message/94484#94583

I don't have a strong opinion and also I don't know the implementation details to be sure about the proper solution. I just wanted to inform you that changing this value has some trade-offs and this should be again discussed on the internals list.

Regardless, documentation and the default setting clearly mismatch, which should be improved either way. Thank you for raising this topic.

@DaWe35
Copy link
Author

DaWe35 commented Mar 17, 2024

Thank you so much, I really appreciate it. It's my first time digging in this repo, so sorry about me not knowing the internal discussion protocols.

@kocoten1992
Copy link

How does it strengthen security?

I think of this scenario: A (attacker) was able inject unescape javascript (xss) into a post which B (user) can see, js can to see all document.cookie and without proper csp header, can send that info to A server, then it is possible that A can impersonate B? Then it does strengthening security a little..

@jorgsowa
Copy link
Contributor

@DaWe35 could you send your thoughts on the internals mailing list? I'm not a core developer and you will get more feedback there. I think the topic you raised is important and should be addressed by more experienced devs.

You can find a guide on how to enroll on the mailing list Internals list here:
https://www.php.net/mailing-lists.php

@jorgsowa
Copy link
Contributor

Hi @DaWe35 , do you need help with writing RFC? It would be good to be ready with this change on the next PHP version.

@DaWe35
Copy link
Author

DaWe35 commented Jun 20, 2024

Hi @DaWe35 , do you need help with writing RFC? It would be good to be ready with this change on the next PHP version.

image

yes, thank you, this stupid email list kills me

@jorgsowa
Copy link
Contributor

I'm glad you are continuing the topic. The proper email list you need to send the message to is internals@lists.php.net

@cmb69
Copy link
Member

cmb69 commented Sep 13, 2024

yes, thank you, this stupid email list kills me

@DaWe35, since quite some time you need to be subscribed to the mailing list to send messages (a spam reduction measure). On https://www.php.net/mailing-lists.php you find a form for online subscription (that may work, or not; I've never tried it), and also instructions for manual subscription (what worked for me a couple of weeks ago). So please, try again!

@DaWe35
Copy link
Author

DaWe35 commented Sep 13, 2024

@cmb69 I was subscribed and all my emails bounced. I don't like when projects make it a hustle to report a bug, I'm trying to help, but I'm not willing to spend more time on this. It seems the PHP team doesn't want me to contact them. This should have been fixed years ago anyway

@cmb69
Copy link
Member

cmb69 commented Sep 13, 2024

I was subscribed and all my emails bounced.

Maybe @derickr or @jimwins have an idea what went wrong.

It seems the PHP team doesn't want me to contact them.

Highly unlikely. :)

@DaWe35
Copy link
Author

DaWe35 commented Sep 13, 2024

Tried again, failed. I'm leaving the text here for you guys.
image
image

Message I failed to send:

I know there are many developers who forgot to set the session cookie to httpOnly, like me. This one setting can greatly reduce the scope of an XSS attack, and I'm shocked httpOnly is not on by default (like in other languages).

I believe it is uncommon that a website needs javascript access for a session cookie*, therefore making httponly=1 by default would not break many websites. I also believe it would make a lot of websites much safer, since there are tons of websites with the insecure default. Changing this default value is a net positive in my opinion.

Entire discussion: https://github.com/php/php-src/pull/13720

I also found a documentation miscommunication about use_strict_mode, [jorgsowa](https://github.com/php/php-src/pull/13720#issuecomment-2002080485) told me it shouldn't always be better, but [php.net](http://php.net/) still recommends it.

*it is not really useful to read a session id by JS, because it's just a random hash without the session data. Websites usually pass actual session values (and not the hash itself) to JS for example by echoing them. When sending an Ajax request to the same domain, the cookie is already part of the request header, further reducing the need of the token access by JS

Best,
DaWe :)

@cmb69
Copy link
Member

cmb69 commented Sep 13, 2024

@DaWe35, the message states that you wouldn't be subscribed to the mailing list. Did you actually use the same email address (From) which you had used to subscribe?

@DaWe35
Copy link
Author

DaWe35 commented Sep 13, 2024

@cmb69 yes I mean I got the denial email about not being able to subscribe

@cmb69
Copy link
Member

cmb69 commented Sep 13, 2024

I mean I got the denial email about not being able to subscribe

Ah, I see. Apparently, the form at mailing-list.php is still broken, since it sends to the wrong recipient (internals-digest+subscribe instead of internals+subscribe-digest). /cc @heiglandreas

But you can also subscribe via email; just send a mail to internals+subscribe-digest@ (or maybe internals+subscribe-nomail@) (use Subject: subscribe), and follow the instructions in the reply.

PS: php/web-master#27 should fix this issue.

@derickr
Copy link
Member

derickr commented Sep 13, 2024

I've merged the ML PR. Will take up to an hour to make it to the published site.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants