Encourage best practice in the HTTP Token authentication example code #24504
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @arthurnn (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| @@ -347,7 +347,7 @@ def opaque(secret_key) | |||
| # private | |||
| # def authenticate | |||
| # authenticate_or_request_with_http_token do |token, options| | |||
| # token == TOKEN | |||
| # ActiveSupport::SecurityUtils.variable_size_secure_compare(token, TOKEN) | |||
There was a problem hiding this comment.
Can you change to secure_compare, variable_size_secure_compare is an internal method.
|
It's ready for another look, thanks! |
| @@ -347,7 +347,12 @@ def opaque(secret_key) | |||
| # private | |||
| # def authenticate | |||
| # authenticate_or_request_with_http_token do |token, options| | |||
| # token == TOKEN | |||
| # # Compare the tokens in a time-constant manner, to mitigate | |||
| # # timing attacks | |||
There was a problem hiding this comment.
missing '.' => timing attacks.
|
@nickmalcolm Can you squash the commits, then its ready to go in. |
…enticate` method, to use the `secure_compare` method with two constant-length strings. This defends against timing attacks, and is best practice. Using `==` for sensitive actions is not recommended, and this was the source of a CVE fixed in October 2015: rails@17e6f15
|
@vipulnsward squashed :) |
|
Thanks @nickmalcolm ! |
Summary
This modifies the HTTP Token authentication example code. It currently uses
==during an authentication method. This is vulnerable to timing attacks. Using something likeActiveSupport::SecurityUtils.secure_compareis more secure. That method was actually introduced to mitigate a CVE in the same file, fixed in October 2015: 17e6f15It may seem trivial, but we should always encourage secure best practice, even if it does make the example code a bit more obtuse.