-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 rb/weak-sensitive-data-hashing
query port
#16781
Add rb/weak-sensitive-data-hashing
query port
#16781
Conversation
QHelp previews: ruby/ql/src/queries/security/cwe-327/WeakSensitiveDataHashing.qhelpUse of a broken or weak cryptographic hashing algorithm on sensitive dataUsing a broken or weak cryptographic hash function can leave data vulnerable, and should not be used in security related code. A strong cryptographic hash function should be resistant to:
As an example, both MD5 and SHA-1 are known to be vulnerable to collision attacks. Since it's OK to use a weak cryptographic hash function in a non-security context, this query only alerts when these are used to hash sensitive data (such as passwords, certificates, usernames). Use of broken or weak cryptographic algorithms that are not hashing algorithms, is handled by the RecommendationEnsure that you use a strong, modern cryptographic hash function:
ExampleThe following example shows two functions for checking whether the hash of a certificate matches a known value -- to prevent tampering. The first function uses MD5 that is known to be vulnerable to collision attacks. The second function uses SHA-256 that is a strong cryptographic hashing function. require 'openssl'
def certificate_matches_known_hash_bad(certificate, known_hash)
hash = OpenSSL::Digest.new('SHA1').digest certificate
hash == known_hash
end
def certificate_matches_known_hash_good(certificate, known_hash)
hash = OpenSSL::Digest.new('SHA256').digest certificate
hash == known_hash
end ExampleThe following example shows two functions for hashing passwords. The first function uses SHA-256 to hash passwords. Although SHA-256 is a strong cryptographic hash function, it is not suitable for password hashing since it is not computationally expensive. require 'openssl'
def get_password_hash(password, salt)
OpenSSL::Digest.new('SHA256').digest(password + salt) # BAD
end The second function uses Argon2 (through the require 'argon2'
def get_initial_hash(password)
Argon2::Password.create(password)
end
def check_password(password, known_hash)
Argon2::Password.verify_password(password, known_hash)
end References
|
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.
Looks good
I've noticed that some sensitive data queries exclude the id
classification, but this appears not to. It seems like it's python counterpart doesn't either.
Does it matter very much?
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.
LGTM 👍
I don't quite understand the need to move SensitiveDataSources away from the SensitiveActions.qll
file in f217de9? -- even when rb/weak-sensitive-data-hashing
is the only Ruby query to use it, would there be a problem from it existing in the file? (as I understand it, it wouldn't affect existing uses of SensitiveNode)
I've noticed that some sensitive data queries exclude the id classification, but this appears not to. It seems like it's python counterpart doesn't either.
Does it matter very much?
we've done that since the results were simply not interesting for most people. Personally I feel someone doing MD5 of usernames sounds like a potential problem, so for now I think we should keep it 👍
In the performance evaluation I see an increase in 'String cache size' for a single project 🤔 I didn't notice any kind of string manipulation being introduced in this PR... do you have an explanation for this? (maybe I overlooked something) -- If this project had 1000 new alerts I might understand it from the additional alert messages, but it doesn't seem like it even has 1 new alert. |
This was just to avoid committing to a new public interface at this point - it shouldn't change anything in terms of functionality of this or any other query. The new sources could potentially be useful for other future queries, at which point it would make sense to undo this change (and maybe tidy up the interface before making it public if necessary).
Honestly, I don't have an explanation for this. The alert message concatenation should be the only new string manipulation in this PR. |
This is a replacement of #12782, porting
py/weak-sensitive-data-hashing
to Ruby.As with the previous PR, the python version is a little more sophisticated in terms of its source modeling. Compared to before, we have moved towards a concept of
SensitiveDataSource
s as sources rather than just anySensitiveNode
. This accounts for cases like:where using
SensitiveNode
s as sources would result in the read ofpasswd
as an argument tosome_unremarkable_method
also being marked as a source.Ideally we would move this out to a shared file to match Python's
SensitiveDataSources.qll
, but I've kept this as a private module inSensitiveDataHashingCustomization.qll
to restrict the impact of this change for now.