From 98f4c2991305f56fe327a788322f85042ac0a11c Mon Sep 17 00:00:00 2001 From: Arthur Baars Date: Fri, 4 Nov 2022 12:28:19 +0100 Subject: [PATCH] Ruby: weak crypto: do not report weak hash algorithms Weak hash algorithms such as MD5 and SHA1 are often used in non security sensitive contexts and reporting all uses is far too noisy. --- .../2022-11-04-weak-crypto-hash.md | 4 ++++ .../security/cwe-327/BrokenCryptoAlgorithm.ql | 8 ++++++-- .../cwe-327/BrokenCryptoAlgorithm.expected | 10 ---------- .../security/cwe-327/broken_crypto.rb | 18 +++++++++--------- 4 files changed, 19 insertions(+), 21 deletions(-) create mode 100644 ruby/ql/src/change-notes/2022-11-04-weak-crypto-hash.md diff --git a/ruby/ql/src/change-notes/2022-11-04-weak-crypto-hash.md b/ruby/ql/src/change-notes/2022-11-04-weak-crypto-hash.md new file mode 100644 index 000000000000..88d81156770c --- /dev/null +++ b/ruby/ql/src/change-notes/2022-11-04-weak-crypto-hash.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `rb/weak-cryptographic-algorithm` has been updated to no longer report uses of hash functions such as `MD5` and `SHA1` even if they are known to be weak. These hash algorithms are used very often in non-sensitive contexts, making the query too imprecise in practice. diff --git a/ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.ql b/ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.ql index 30d76cf894c0..b4082c669aac 100644 --- a/ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.ql +++ b/ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.ql @@ -15,8 +15,12 @@ import codeql.ruby.Concepts from Cryptography::CryptographicOperation operation, string msgPrefix where - operation.getAlgorithm().isWeak() and - msgPrefix = "The cryptographic algorithm " + operation.getAlgorithm().getName() + exists(Cryptography::CryptographicAlgorithm algorithm | + algorithm = operation.getAlgorithm() and + algorithm.isWeak() and + msgPrefix = "The cryptographic algorithm " + algorithm.getName() and + not algorithm instanceof Cryptography::HashingAlgorithm + ) or operation.getBlockMode().isWeak() and msgPrefix = "The block mode " + operation.getBlockMode() select operation, msgPrefix + " is broken or weak, and should not be used." diff --git a/ruby/ql/test/query-tests/security/cwe-327/BrokenCryptoAlgorithm.expected b/ruby/ql/test/query-tests/security/cwe-327/BrokenCryptoAlgorithm.expected index d4378b8f7194..62f621fd8c4f 100644 --- a/ruby/ql/test/query-tests/security/cwe-327/BrokenCryptoAlgorithm.expected +++ b/ruby/ql/test/query-tests/security/cwe-327/BrokenCryptoAlgorithm.expected @@ -17,13 +17,3 @@ | broken_crypto.rb:75:1:75:24 | call to new | The cryptographic algorithm RC4 is broken or weak, and should not be used. | | broken_crypto.rb:77:1:77:29 | call to new | The cryptographic algorithm RC4 is broken or weak, and should not be used. | | broken_crypto.rb:79:1:79:35 | call to new | The cryptographic algorithm RC4 is broken or weak, and should not be used. | -| broken_crypto.rb:81:1:81:28 | call to hexdigest | The cryptographic algorithm MD5 is broken or weak, and should not be used. | -| broken_crypto.rb:84:1:84:31 | call to base64digest | The cryptographic algorithm MD5 is broken or weak, and should not be used. | -| broken_crypto.rb:87:1:87:20 | call to digest | The cryptographic algorithm MD5 is broken or weak, and should not be used. | -| broken_crypto.rb:89:1:89:21 | call to update | The cryptographic algorithm MD5 is broken or weak, and should not be used. | -| broken_crypto.rb:90:1:90:17 | ... << ... | The cryptographic algorithm MD5 is broken or weak, and should not be used. | -| broken_crypto.rb:95:1:95:34 | call to bubblebabble | The cryptographic algorithm MD5 is broken or weak, and should not be used. | -| broken_crypto.rb:97:11:97:37 | call to file | The cryptographic algorithm MD5 is broken or weak, and should not be used. | -| broken_crypto.rb:103:1:103:21 | call to digest | The cryptographic algorithm SHA1 is broken or weak, and should not be used. | -| broken_crypto.rb:104:1:104:17 | ... << ... | The cryptographic algorithm SHA1 is broken or weak, and should not be used. | -| broken_crypto.rb:106:1:106:37 | call to digest | The cryptographic algorithm SHA1 is broken or weak, and should not be used. | diff --git a/ruby/ql/test/query-tests/security/cwe-327/broken_crypto.rb b/ruby/ql/test/query-tests/security/cwe-327/broken_crypto.rb index 1de64c091305..69dcd6b472bb 100644 --- a/ruby/ql/test/query-tests/security/cwe-327/broken_crypto.rb +++ b/ruby/ql/test/query-tests/security/cwe-327/broken_crypto.rb @@ -78,30 +78,30 @@ # BAD: weak encryption algorithm OpenSSL::Cipher::RC4.new 'hmac-md5' -Digest::MD5.hexdigest('foo') # BAD: weak hash algorithm +Digest::MD5.hexdigest('foo') # OK: don't report hash algorithm even if it is weak Digest::SHA256.hexdigest('foo') # GOOD: strong hash algorithm -Digest::MD5.base64digest('foo') # BAD: weak hash algorithm +Digest::MD5.base64digest('foo') # OK: don't report hash algorithm even if it is weak md5 = Digest::MD5.new -md5.digest 'message' # BAD: weak hash algorithm +md5.digest 'message' # OK: don't report hash algorithm even if it is weak -md5.update 'message1' # BAD: weak hash algorithm +md5.update 'message1' # # OK: don't report hash algorithm even if it is weak md5 << 'message2' # << is an alias for update sha256 = Digest::SHA256.new sha256.digest 'message' # GOOD: strong hash algorithm -Digest::MD5.bubblebabble 'message' # BAD: weak hash algorithm +Digest::MD5.bubblebabble 'message' # OK: don't report hash algorithm even if it is weak -filemd5 = Digest::MD5.file 'testfile' +filemd5 = Digest::MD5.file 'testfile' # OK: don't report hash algorithm even if it is weak filemd5.hexdigest -Digest("MD5").hexdigest('foo') # BAD: weak hash algorithm +Digest("MD5").hexdigest('foo') # OK: don't report hash algorithm even if it is weak sha1 = OpenSSL::Digest.new('SHA1') -sha1.digest 'message' # BAD: weak hash algorithm +sha1.digest 'message' # OK: don't report hash algorithm even if it is weak sha1 << 'message' # << is an alias for update -OpenSSL::Digest.digest('SHA1', "abc") # BAD: weak hash algorithm +OpenSSL::Digest.digest('SHA1', "abc") # OK: don't report hash algorithm even if it is weak OpenSSL::Digest.digest('SHA3-512', "abc") # GOOD: strong hash algorithm \ No newline at end of file