Use the timing-safe compare() helper for the CSRF formkey check - #2650
Merged
Conversation
FORM.accepts() compared the anti-CSRF _formkey with `formkey in formkeys`, which uses CPython's short-circuiting `==` and is not constant-time. gluon.utils.compare (hmac.compare_digest-backed, already used for the signature check at gluon/html.py:581) is applied instead, so the token comparison is timing-safe and consistent with the rest of the module. compare is already imported; behavior is unchanged. Same hardening class as web2py#1321.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2650 +/- ##
==========================================
+ Coverage 50.70% 50.71% +0.01%
==========================================
Files 42 42
Lines 15500 15500
Branches 3409 3409
==========================================
+ Hits 7859 7861 +2
+ Misses 6599 6598 -1
+ Partials 1042 1041 -1 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FORM.accepts()validates the anti-CSRF form key with a plain list-membership test:formkey in formkeyscompares the attacker-supplied_formkeyagainst each stored key using CPython's==, which short-circuits on the first differing character and is not constant-time.web2py already ships a timing-safe comparison helper for exactly this,
gluon.utils.compare(documented "Compares two strings and not vulnerable to timing attacks", backed byhmac.compare_digest), and already uses it for the analogous signature check in the same file:This applies that same helper to the formkey check, so the anti-CSRF token comparison is timing-safe and consistent with the rest of the module:
compareis already imported at the top of the file, so no new import is needed. Behavior is unchanged: a valid formkey is still accepted and an invalid one is still rejected; only the per-key comparison is now constant-time.Testing
gluon/tests/test_html.pyandgluon/tests/test_form.pypass unchanged (86 passed). I also verified directly that a valid formkey is accepted and a wrong formkey is rejected after the change.This is the same hardening class as #1321 ("fixed timing attack in gluon.utils.compare"), applied to the formkey comparison site.