OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
868 stars 436 forks source link

Support "SameSite" cookie property #414

Closed colinmollenhour closed 3 years ago

colinmollenhour commented 6 years ago

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.

tmotyl commented 6 years ago

sounds good :)

colinmollenhour commented 6 years ago

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 commented 4 years ago

Reference https://github.com/magento/magento2/issues/26377

onlinebizsoft commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@colinmollenhour I will look into this!

seansan commented 4 years ago

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?

So one would need some logic to

https://stackoverflow.com/a/60716636/1173670 https://github.com/magento/magento2/issues/26377 https://github.com/OpenMage/magento-lts/pull/922

rvelhote commented 4 years ago

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 commented 4 years ago
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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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.

kanevbg commented 3 years ago

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

fballiano commented 2 years ago

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