-
Notifications
You must be signed in to change notification settings - Fork 162
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
Add check for insecure hash algorithms #79
Conversation
@spraints In https://github.com/github/github/pull/165161#discussion_r540253611 @vcsjones had a good idea to use an allow list instead. How about also doing that for the generic Rubocop rules here as well? |
# https://ruby-doc.org/stdlib-2.7.0/libdoc/openssl/rdoc/OpenSSL/Digest.html | ||
DEFAULT_ALLOWED = %w[ | ||
RIPEMD160 | ||
RMD160 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These shouldn't be allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed them (and SHA-224). The default allow list now is:
rubocop-github/lib/rubocop/cop/github/insecure_hash_algorithm.rb
Lines 61 to 65 in 4d952bb
DEFAULT_ALLOWED = %w[ | |
SHA256 | |
SHA384 | |
SHA512 | |
].freeze |
👍 It required a little bit of fiddling to get it to ignore rubocop-github/lib/rubocop/cop/github/insecure_hash_algorithm.rb Lines 61 to 68 in c5a7ece
This leaves these as not allowed:
|
RIPEMD160 is an older function, about the same age as MD5, so we don't want to start using it in new code. SHA-224 isn't one that we use much. It's more or less a truncated form of SHA-256, and SHA-256 itself works in places where SHA-224 would, so we opt for the longer output of SHA-256.
https://api.rubyonrails.org/classes/Digest/UUID.html uuid_v3 uses MD5, so it's only allowed when MD5 is in the "Allowed" list. uuid_v5 uses SHA1, so it's only allowed when SHA1 is in the "Allowed" list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to me 👍
MD5 and SHA1 are both subject to well-known attacks and are not recommended for use. This branch adds a cop that checks for these hash functions and recommends SHA256 in their place.
cc @dbussink @jhawthorn