Fix \Magento\Checkout\Controller\Index\Index::isSecureRequest method to take care of current request being secure and also from referer, as stated in phpdoc block

[adrian-martinez-interactiv4]

Description

Fix \Magento\Checkout\Controller\Index\Index::isSecureRequest method to take care of current request being secure and also from referer, as stated in phpdoc block.

After last try to implement a solution for session loss in checkout, this private method did behaviour as expected. Updated as agreed with @sidolov .

Fixed Issues (if relevant)

1. Hit fast twice F5 on checout page, customer loggs out automatically #4301: Hit fast twice F5 on checout page, customer loggs out automatically
2. Concurrent (quick reload) requests on checkout cause cart to empty - related to session_regenerate_id #12362: Concurrent (quick reload) requests on checkout cause cart to empty - related to session_regenerate_id
3. [2.1.11] Add to cart, try to checkout, cart is empty but mini-cart has items. #13427: Add to cart, try to checkout, cart is empty but mini-cart has items.

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[sidolov]

Hi @adrian-martinez-interactiv4 , regenerate_session_id call was added in order to prevent session fixation attacks. Atack possible when browsing between secure and insecure pages. We can’t remove the line in question as it may theoretically compromise security.

We have the story for implementation secure cookies and session transfer mechanism.

For now, I propose to leave a comment under the line with regenerate_session_id call with an explanation why it was added.

[adrian-martinez-interactiv4]

Hi @sidolov , I've read https://www.owasp.org/index.php/Session_fixation article and I think I have understood what it says about session hijacking, but I have some questions about this all.

• Assuming browsing between secure and insecure pages is possible due to configuration, wouldn't this be a problem also for /customer/account section? I mean, why isn't renewed session id when accessing customer account, if session id could have been hijacked ?

• Secure cookies have been already implemented for secure requests when configuration does not allow mixed navigation. Looking at \Magento\Framework\Session\Config::__construct:

  $secureURL = $this->_scopeConfig->getValue('web/secure/base_url', $this->_scopeType);
  $unsecureURL = $this->_scopeConfig->getValue('web/unsecure/base_url', $this->_scopeType);
  $isFullySecuredURL = $secureURL == $unsecureURL;
  $this->setCookieSecure($isFullySecuredURL && $this->_httpRequest->isSecure());
  

  If request is secure and base secure and unsecure urls match, secure cookies are set, and travel encrypted. I wonder if regenerating session id in controller index could be limited to unsecure cookies, denoting mixed/unsecured navigation. This at least would skip the issue for fully secured pages.

  This would be some pseudo-code (objectManager used only for example purpose):

  if(!$this->_objectManager->get(\Magento\Framework\Session\Config::class)->getCookieSecure()) {
      $this->_customerSession->regenerateId();
  }
  

• Since the problem is the time between session has been regenerated and the browser acknowledging the change and updating its cookie, I'm also wondering if session regeneration could be moved just before the response is sent (maybe tied to controller_front_send_response_before event or similar), to reduce this issue as possible.

I'll be looking forward for your comments, thank you in advance.

[sidolov]

Hi @adrian-martinez-interactiv4 , thanks for your investigation!

Good point for the case with fully secured pages, I agree that would be better check and skip regenerateId method call.

Regarding customer/account section - session id regenerated in several scenarios with account manipulation, for example:

• Account/CreatePost
• Account/LoginPost

I think moving session regeneration call to controller_front_send_response_before event may not completely change the situation, we need a complex solution for all cases.

[adrian-martinez-interactiv4]

Updated PR as agreed with @sidolov after last changes.

[magento-engcom-team]

Hi @adrian-martinez-interactiv4. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.2.5 release.

[southerncomputer]

Where should we file CVE for relying on referer from client browser for issecure patch?

Pretty blatant security issue , yes?