Use constant time string comparison in FormKey validator

[p0pr0ck5]

Description

CSRF tokens should be considered sensitive strings. While the risk of a malicious actor attempting gleam the form key via a timing attack is very low, we should still follow best practices in verifying this token.

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)

[magento-cicd2]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ orlangur
❌ p0pr0ck5

[orlangur]

Hi @p0pr0ck5, form key is regenerated each time you enter it incorrectly, isn't it?

[p0pr0ck5]

Hey @orlangur, I'm having trouble following the code path to see exactly what's going on under the hood. Even if the key does rotate I would still say this is a good best-practice to implement, no?

[orlangur]

Even if the key does rotate I would still say this is a good best-practice to implement, no?

@p0pr0ck5 no, I think. The whole point of attack is to examine one hash multiple times, if the hash is new each time, attack is not possible.

Could you please try attack scenario in action:

1. Open some CSRF-protected form in two tabs
2. Copy form key via Dev Tools and modify it in DOM for one of the tabs, assure form key is identical
3. Submit one form with invalid form key
4. Check if form key is regenerated in reopened form
5. Try submitting another form with old form key

[p0pr0ck5]

@orlangur I understand what you're saying. Can you highlight where in the code this rotation occurs, for the benefit of my edification? I'm also still struggling to understand the harm this brings- even if the key rotates, why wouldn't this be considered a secure token that should have best practices applied to it? Is it not saner to incorporate a more secure approach simply because it is a more secure approach that hardens the application, and does not bring any ill consequences?

[orlangur]

@p0pr0ck5,

Can you highlight where in the code this rotation occurs, for the benefit of my edification?

Honestly, I have no idea about it, will try to play with code and then tell you after I process currently assigned PRs and have some time.

Is it not saner to incorporate a more secure approach

There is no such general approach, it is applicable only to a particular vector of attack. It must not be applied blindly if attack vector is not possible in particular place (otherwise someone would desire to replace any string comparison with such call).

[orlangur]

Hi @p0pr0ck5, after some googling and reading https://www.sjoerdlangkemper.nl/2016/04/21/combining-csrf-with-timing-attacks/
and https://medium.com/@barryvdh/csrf-protection-in-laravel-explained-146d89ff1357 I see that it seems more like a theoretical attacking vector, however, constant time string comparison wouldn't hurt.

I'm also taking into consideration that as every other framework like Drupal/Symfony/Laravel is comparing CSRF token like this, you're probably not the last person who will report it for Magento :)

Please sign our Contributor License Agreement so that this PR can be merged.

[p0pr0ck5]

@orlangur thanks for the update. Signed.

[orlangur]

@magento-cicd2 badge still looks yellow to me, I assume it's temporary lag or something.

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-2143 has been created to process this Pull Request

[magento-engcom-team]

@p0pr0ck5 thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[magento-engcom-team]

Hi @p0pr0ck5. Thank you for your contribution.
The changes from your Pull Request should be available with the upcoming 2.2.6 release.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.