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

Fix comments about csrf_state #245

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Fix comments about csrf_state #245

merged 1 commit into from
Jan 23, 2024

Conversation

ikehz
Copy link
Contributor

@ikehz ikehz commented Jan 15, 2024

Clarify that the state parameter should be compared to the csrf_token.secret().

Fixes #208.

@ramosbugs
Copy link
Owner

Hey, thanks for fixing the variable name! I would prefer not to add .secret() as it suggests users should directly compare the secret as a string using ==, which would create a timing side-channel vulnerability.

To avoid introducing the vulnerability, the comparison must be done in constant time, either using a constant-time crate like constant_time_eq (which could break if a future compiler version decides to be overly smart about its optimizations), or by first computing a cryptographically-secure hash (e.g., SHA-256) of both values and then comparing the hashes using == (see #232).

The timing-resistant-secret-traits feature flag adds a safe (but comparatively expensive) PartialEq implementation to the secret types. Timing side-channels are why PartialEq is not auto-derived for this crate's secret types, and the lack of PartialEq is intended to prompt users to think more carefully about these comparisons.

I would suggest either only fixing the variable name, or adding a separate section about securely comparing secrets such as the CSRF state, and then referencing it in each example.

@ikehz
Copy link
Contributor Author

ikehz commented Jan 22, 2024

Thanks so much for the thorough feedback! Fixed instructions to compare just against csrf_token. Sent #246 for the additional docs.

@ramosbugs ramosbugs merged commit bc66c72 into ramosbugs:main Jan 23, 2024
4 checks passed
@ramosbugs
Copy link
Owner

Thanks!

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

Successfully merging this pull request may close these issues.

Comments in examples mention csrf_state which does not exist in the code
2 participants