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

Support "SameSite" cookie property #414

Closed
colinmollenhour opened this issue Dec 15, 2017 · 21 comments · Fixed by #1246
Closed

Support "SameSite" cookie property #414

colinmollenhour opened this issue Dec 15, 2017 · 21 comments · Fixed by #1246
Labels

Comments

@colinmollenhour
Copy link
Member

What are everyone's thoughts on modifying Magento to support the SameSite cookie declaration? This is supported currently by Chrome and Firefox and when set prevents cookies from being sent on requests that did not originate from the same domain and thus is an effective prevention against CSRF. Magento already uses the formkey block for CSRF prevention but it seems every patch adds places where this was missed and that does nothing for third-party code that doesn't implement it. I believe official support will eventually come with PHP 7.3, but until then the cookie header can be set directly, such as with this third-party library: https://github.com/delight-im/PHP-Cookie

With SameSite (and users using supported browsers) the user's cookie would not even be sent to the server so CSRF attempts would be stopped dead at the authentication step for all requests (including GET) rather than depending on the forms and controllers to have implemented formkey (which is only supported for POST).

I am not aware of any way that this would break BC because the class that would need to be modified cannot be rewritten using config.xml as it is only an abstract (in practice) class.

  • Mage_Core_Model_Session_Abstract_Varien
@tmotyl
Copy link
Contributor

tmotyl commented Dec 21, 2017

sounds good :)

@colinmollenhour
Copy link
Member Author

Note, PHP 7.3 is now GA and supports SameSite natively (ref(:

- session.cookie_samesite
  . New INI option to allow to set the SameSite directive for cookies. Defaults
    to "" (empty string), so no SameSite directive is set. Can be set to "Lax"
    or "Strict", which sets the respective SameSite directive.

So I think the third-party library route should be skipped in favor of just requiring PHP 7.3 (see #129 and #449 as prerequisites) and the question is, make this a default or make it configurable? I propose to make it configurable separately for Admin and Frontend and use "Strict" by default for Admin and "Lax" by default for Frontend.

From OWASP:

The strict value will prevent the cookie from being sent by the browser to the target site in all cross-site browsing context, even when following a regular link. For example, for a GitHub-like website this would mean that if a logged-in user follows a link to a private GitHub project posted on a corporate discussion forum or email, GitHub will not receive the session cookie and the user will not be able to access the project.

A bank website however most likely doesn't want to allow any transactional pages to be linked from external sites so the strict flag would be most appropriate here.

The default lax value provides a reasonable balance between security and usability for websites that want to maintain user's logged-in session after the user arrives from an external link. In the above GitHub scenario, the session cookie would be allowed when following a regular link from an external website while blocking it in CSRF-prone request methods (e.g. POST).

@onlinebizsoft
Copy link

Reference magento/magento2#26377

@onlinebizsoft
Copy link

onlinebizsoft commented Mar 26, 2020

I am not aware of any way that this would break BC because the class that would need to be modified cannot be rewritten using config.xml as it is only an abstract (in practice) class.

Mage_Core_Model_Session_Abstract_Varien

@colinmollenhour Why don't we rewrite the model core/cookie which might be better way of changing this abstract class?

@othuress
Copy link

For the samesite cookie policy means loss of sales since or payment integration with adyen fails with this new policy. In essence it is a catastrophe that Magento does not support this, need to be fixed urgently.

@colinmollenhour
Copy link
Member Author

@othuress Can you submit a PR for the issue as described above? I think we've agreed on what needs to be done so should be quick to get it merged.

@othuress
Copy link

@colinmollenhour What is an PR?

This?
"Same site cookie policy issue in Magento2 Since the SameSite Cookie Changes redirect payment method flows can break when the issuer is redirecting back to the Magento site with a POST request. When Magento processes the POST request it starts with loading the cart from the session. In order to do so it reaches to the SESSION cookie. Unfortunately the SESSION cookie is not set as SameSite=None; Secure therefore the browser is going to decline using the cookie so the process fails and Magento redirects back to the empty cart page. Since the redirect is not handled by Magento the payment is not finalised and the order stays as new.

To solve this issue the SESSION cookie should be set as SameSite=None; Secure so POST requests from outside of the Magento website domain can also be processed."

@colinmollenhour
Copy link
Member Author

PR - Pull Request - Most of us don't have time dedicated to feature development so if you can contribute a thorough pull request for the issue it will become a reality much faster. :)

@othuress
Copy link

@colinmollenhour I will look into this!

@seansan
Copy link
Contributor

seansan commented Aug 10, 2020

Is this something to look at (soon)?

Just upgraded chrome and getting multiple warnings. We are on php 7.2 ...

To get things straight - the problem and solution can be the following? Please edit if incorrect

So what I understand is something like this?

  1. < php 7.2 the setcookie command has no logic to add the new attributes?
  2. php 7.3+ does this natively and there is no worry? what about JS code adding the cookies? or does it just offer the option in the call and you still have to add it into the call?

Also to be certain of the process: the cookie logic is?

  • if local cookie then use SameSite=Lax
  • if https turned on (which is almost 100%) then use flag secure ... SameSite=Lax;Secure
  • if remote use SameSite=None;Secure

So one would need some logic to

  • detect local or remote cookie? both in php and JS functions .. resulting in Lax or None
  • detect https for local ... resulting in addition of Secure

https://stackoverflow.com/a/60716636/1173670
magento/magento2#26377
#922

@rvelhote
Copy link
Contributor

I was testing this with PHP 7.4. I set session.cookie_samesite to Lax, cleared all cookies and when the cookie was first set it had the samesite attribute however while navigating the samesite attribute disappeared.

I tried forcing the samesite attribute directly in Mage_Core_Model_Cookie in the set method and now the samesite attribute is set correctly in all requests. This means that setting it in php.ini seems to be insufficient.

In Mage_Core_Model_Session_Abstract_Varien Magento does call_user_func_array('session_set_cookie_params', $cookieParams); however if passed an additional samesite key it will generate a warning saying that session_set_cookie_params params expects at most 5 parameters instead of 6. Calling session_set_cookie_params directly works but is not widely compatible.

While testing payment redirect scenarios the Strict setting for session cookies causes those types of payments to fail. I found no issues with Lax however there's still more testing to do.

@adityaputra
Copy link

1. < php 7.2 the setcookie command has no logic to add the new attributes?

2. php 7.3+ does this natively and there is no worry?  what about JS code adding the cookies? or does it just offer the option in the call and you still have to add it into the call?

In our case under Magento 1.9, nginx, PHP 7.1, where we couldn't really do PHP setcookie, also we have tried this solution using header() method but nothing worked.

Our solution was then to update the Magento's core_config_data table and set web/cookie/cookie_path value to:
/; SameSite=None; Secure
I know that this isn't a permanent solution but it might be useful for a temporary workaround.

@seansan
Copy link
Contributor

seansan commented Aug 27, 2020

Thanks for the update. I am trying to understand how "critical" this is (for the PSP issue like Buckaroo linked above I can imagine that case is urgent)?

@adityaputra
Copy link

@seansan if you do Magento 1 and use third party integrations (which most magento websites do), this is the top priority for now. In our case we still support several Magento 1 clients and this week at least we have four clients reporting this issue, and all are related to payment.

Those clients reporting this issue are all using third party credit card payment service where the customer need to be redirected to the bank's payment gateway, then in normal case he will be redirected back to checkout success page upon successful payment. But due to this issue, although order has been successfully placed with confirmed payment, customer was redirected back to empty shopping cart page and logged out automatically (which caused confusion for the customers).

@ioweb-gr
Copy link
Contributor

This is actually an issue with multiple platforms like Prestashop 1.7 and third party payment modules. I can confirm that clients of ours with M1 and Prestashop and customers using Chrome the same thing happens as @adityaputra mentions.

@colinmollenhour
Copy link
Member Author

colinmollenhour commented Aug 27, 2020

As mentioned on #922 I think the SameSite=None for payment gateway redirects should be implemented using a separate cookie, one that only authenticates the user on the return page and not in general. This way the primary cookie can be left alone and you aren't giving up 100% of the benefit of the SameSite feature.

  1. Some time before redirecting, set a new cookie with SameSite=None and a value that can later be used to retrieve the real session id in some manner that is single-use only. E.g. encrypt "real_session_id:nonce" and store "nonce" in the session.
  2. Upon return only for the controller action that the user returns to, and before loading the session, read the cookie value of this SameSite=None cookie, validate the nonce, and inject the real session id so that the user is effectively logged back into the same session id as before.
  3. Delete this single-use cookie or just leave it be to expire.

Edit: maybe this can be implemented in a generalized way so that it can be used for any situation that uses a redirect back to a specific controller action. For example, the payload could contain "real_session_id:nonce:authorized_controller_action".

@SarunasCard
Copy link

SarunasCard commented Sep 17, 2020

@seansan

Thanks for the update. I am trying to understand how "critical" this is (for the PSP issue like Buckaroo linked above I can imagine that case is urgent)?

Multiple clients of ours report no payments processed because shop buyers sessions are lost after 3D secure redirection. This is critical because all payments going through 3D secure redirection fail.

@waynetheisinger
Copy link

Multiple clients of ours report no payments processed because shop buyers sessions are lost after 3D secure redirection. This is critical because all payments going through 3D secure redirection fail.

@SarunasCard are they running PHP7.3 - we had the same issue on a client's website but upgrading from PHP 5.6 to PHP7.3 seemed to fix it.

@SarunasCard
Copy link

@SarunasCard are they running PHP7.3 - we had the same issue on a client's website but upgrading from PHP 5.6 to PHP7.3 seemed to fix it.

Thank you @waynetheisinger. This could help few clients. But not everyone can upgrade PHP version (fast) while this issue cause huge amount of lost sales orders every day. And on top of that lots of leaving customers and additional work to manage failing orders.

Flyingmana pushed a commit that referenced this issue Dec 29, 2020
* Support SameSite cookie property (fixes issue #414)

* Support SameSite cookie property (fixes issue #414)

* Open in new tab

* Default fallback to None just in case someone forgets to clear cache

* * Force secure if SameSite None
* SameSite consistency for renew and delete functions

* Code formatting #OCD :)

* PHP 7.0 compatibility

* Fix pipeline errors

* Removed incorrect copyright docs
@sreichel sreichel added the fixed label Dec 29, 2020
@kanevbg
Copy link
Contributor

kanevbg commented Jan 13, 2021

SameSite=None will not work with all web clients, eg Safari has bug:
https://caniuse.com/same-site-cookie-attribute

@fballiano
Copy link
Contributor

SameSite=None will not work with all web clients, eg Safari has bug: https://caniuse.com/same-site-cookie-attribute

luckily safari supports this now

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

Successfully merging a pull request may close this issue.