Ruby: Add `rb/weak-cryptographic-algorithm` query #8421

alexrford · 2022-03-13T21:54:23Z

Ports py/weak-cryptographic-algorithm to Ruby. Currently supports OpenSSL ciphers.

This is generally similar, using the Cryptography::CryptographicOperation concept. There is a slight tweak in moving the isWeak() predicate into CryptographicOperation rather than only having it on CryptographicAlgorithm, which allows us to account for operations with ciphers that use a weak block mode (in practice, this means ECB). The CryptographicAlgorithm only includes the block encryption algorithm itself, not the block mode in the context where the algorithm is used in a stream cipher.

The side-effect of this is that the alert messages can currently be misleading, e.g. suggesting that the block encryption algorithm is weak when it is actually the block mode.

The best solutions to this that I can see are:

Keeping CryptographicAlgorithm and the block mode separate, but introducing a proper BlockMode class with e.g. an isWeak() predicate of its own. CryptographicOperation could then have a BlockMode getBlockMode() predicate rather than its own isWeak() predicate, and the query could check both the encryption algorithm and the block mode.
Include the block mode in the CryptographicAlgorithm, and add 2 predicates - isWeakEncryptionAlgorithm and isWeakBlockMode, to that class. Then CryptographicOperation#isWeak() could be removed.

github-actions · 2022-03-13T21:55:45Z

QHelp previews:

ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.qhelp

Use of a broken or weak cryptographic algorithm

Using broken or weak cryptographic algorithms can leave data vulnerable to being decrypted or forged by an attacker.

Many cryptographic algorithms provided by cryptography libraries are known to be weak, or flawed. Using such an algorithm means that encrypted or hashed data is less secure than it appears to be.

Recommendation

Ensure that you use a strong, modern cryptographic algorithm, such as AES-128 or RSA-2048.

Example

The following code uses the OpenSSL library to encrypt some secret data. When you create a cipher using OpenSSL you must specify the encryption algorithm to use. The first example uses DES, which is an older algorithm that is now considered weak. The second example uses AES, which is a stronger modern algorithm.

require 'openssl'

class Encryptor
  attr_accessor :secret_key

  def encrypt_message_weak(message)
    cipher = OpenSSL::Cipher.new('des') # BAD: weak encryption
    cipher.encrypt
    cipher.key = secret_key
    cipher.update(message)
    cipher.final
  end

  def encrypt_message_strong(message)
    cipher = OpenSSL::Cipher::AES128.new # GOOD: strong encryption
    cipher.encrypt
    cipher.key = secret_key
    cipher.update(message)
    cipher.final
  end
end

References

NIST, FIPS 140 Annex a: Approved Security Functions.
NIST, SP 800-131A: Transitions: Recommendation for Transitioning the Use of Cryptographic Algorithms and Key Lengths.
OWASP: Rule - Use strong approved cryptographic algorithms.
Common Weakness Enumeration: CWE-327.

github-actions · 2022-03-14T08:58:10Z

QHelp previews:

ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.qhelp

Use of a broken or weak cryptographic algorithm

Using broken or weak cryptographic algorithms can leave data vulnerable to being decrypted or forged by an attacker.

Many cryptographic algorithms provided by cryptography libraries are known to be weak, or flawed. Using such an algorithm means that encrypted or hashed data is less secure than it appears to be.

Recommendation

Ensure that you use a strong, modern cryptographic algorithm, such as AES-128 or RSA-2048.

Example

The following code uses the OpenSSL library to encrypt some secret data. When you create a cipher using OpenSSL you must specify the encryption algorithm to use. The first example uses DES, which is an older algorithm that is now considered weak. The second example uses AES, which is a stronger modern algorithm.

require 'openssl'

class Encryptor
  attr_accessor :secret_key

  def encrypt_message_weak(message)
    cipher = OpenSSL::Cipher.new('des') # BAD: weak encryption
    cipher.encrypt
    cipher.key = secret_key
    cipher.update(message)
    cipher.final
  end

  def encrypt_message_strong(message)
    cipher = OpenSSL::Cipher::AES128.new # GOOD: strong encryption
    cipher.encrypt
    cipher.key = secret_key
    cipher.update(message)
    cipher.final
  end
end

References

NIST, FIPS 140 Annex a: Approved Security Functions.
NIST, SP 800-131A: Transitions: Recommendation for Transitioning the Use of Cryptographic Algorithms and Key Lengths.
OWASP: Rule - Use strong approved cryptographic algorithms.
Common Weakness Enumeration: CWE-327.

aibaars

This looks good to me.

aibaars · 2022-03-17T15:09:20Z

ruby/ql/lib/codeql/ruby/Concepts.qll

@@ -762,3 +762,58 @@ module Logging {
    abstract DataFlow::Node getAnInput();
  }
 }
+
+/**
+ * Provides models for cryptographic things.


Perhaps things -> concepts.

aibaars · 2022-03-17T15:31:47Z

ruby/ql/lib/codeql/ruby/security/OpenSSL.qll

+    |
+      cipher.matchesName(cipherName)


I think this can be moved to line 381 .

exists(string cipherName | cipher.matchesName(cipherName) | ... or ... )

aibaars · 2022-03-17T15:33:03Z

ruby/ql/lib/codeql/ruby/security/OpenSSL.qll

+      // CBC is used by default
+      cipherMode.isBlockMode("CBC")
+      or
+      // `OpenSSL::Cipher::AES` instantiations


This is quite a large block of text and I wonder if it would improve readability if split into smaller predicates.

I've split this into 4 separate predicates that each cover a subset of cases, I think it's slightly clearer.

github-actions · 2022-03-20T20:21:13Z

QHelp previews:

ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.qhelp

Use of a broken or weak cryptographic algorithm

Using broken or weak cryptographic algorithms can leave data vulnerable to being decrypted or forged by an attacker.

Many cryptographic algorithms provided by cryptography libraries are known to be weak, or flawed. Using such an algorithm means that encrypted or hashed data is less secure than it appears to be.

Recommendation

Ensure that you use a strong, modern cryptographic algorithm, such as AES-128 or RSA-2048.

Example

The following code uses the OpenSSL library to encrypt some secret data. When you create a cipher using OpenSSL you must specify the encryption algorithm to use. The first example uses DES, which is an older algorithm that is now considered weak. The second example uses AES, which is a stronger modern algorithm.

require 'openssl'

class Encryptor
  attr_accessor :secret_key

  def encrypt_message_weak(message)
    cipher = OpenSSL::Cipher.new('des') # BAD: weak encryption
    cipher.encrypt
    cipher.key = secret_key
    cipher.update(message)
    cipher.final
  end

  def encrypt_message_strong(message)
    cipher = OpenSSL::Cipher::AES128.new # GOOD: strong encryption
    cipher.encrypt
    cipher.key = secret_key
    cipher.update(message)
    cipher.final
  end
end

References

NIST, FIPS 140 Annex a: Approved Security Functions.
NIST, SP 800-131A: Transitions: Recommendation for Transitioning the Use of Cryptographic Algorithms and Key Lengths.
OWASP: Rule - Use strong approved cryptographic algorithms.
Common Weakness Enumeration: CWE-327.

nickrolfe

Sorry, I know you already had a review and were just looking for re-approval, but I have a couple of comments.

ruby/ql/lib/codeql/ruby/Concepts.qll

nickrolfe · 2022-03-29T16:33:50Z

ruby/ql/test/query-tests/security/cwe-327/BrokenCryptoAlgorithm.expected

@@ -0,0 +1,18 @@
+| broken_crypto.rb:4:8:4:34 | call to new | The cryptographic algorithm DES is broken or weak, and should not be used. |
+| broken_crypto.rb:8:1:8:18 | call to update | The cryptographic algorithm DES is broken or weak, and should not be used. |
+| broken_crypto.rb:12:8:12:43 | call to new | The cryptographic algorithm AES is broken or weak, and should not be used. |


It's a bit of a shame that this message doesn't mention the specific cipher mode that makes it weak. Would that be difficult to add?

Yeah, the inaccurate message here is not great. At the level of CryptographicOperation, we have no concept of the block mode, so the weakness/strength of the operation is just a kind of black box. The solutions that I've considered are:

Keeping CryptographicAlgorithm and the block mode separate, but introducing a proper BlockMode class with e.g. an isWeak() predicate of its own. CryptographicOperation could then have a BlockMode getBlockMode() predicate rather than its own isWeak() predicate, and the query could check both the encryption algorithm and the block mode.

Include the block mode in the CryptographicAlgorithm, and add 2 predicates - isWeakEncryptionAlgorithm and isWeakBlockMode, to that class. Then CryptographicOperation#isWeak() could be removed.

I think I prefer 1 overall as it seems like the intention of the CryptographicAlgorithm class (shared with JS and Python) is to represent the algorithm modulo any block mode. Do you have any preferences between these?

1 sounds like a nice solution, if it's not too difficult to implement.

github-actions · 2022-03-31T16:02:54Z

QHelp previews:

ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.qhelp

Use of a broken or weak cryptographic algorithm

Using broken or weak cryptographic algorithms can leave data vulnerable to being decrypted or forged by an attacker.

Many cryptographic algorithms provided by cryptography libraries are known to be weak, or flawed. Using such an algorithm means that encrypted or hashed data is less secure than it appears to be.

Recommendation

Ensure that you use a strong, modern cryptographic algorithm, such as AES-128 or RSA-2048.

Example

The following code uses the OpenSSL library to encrypt some secret data. When you create a cipher using OpenSSL you must specify the encryption algorithm to use. The first example uses DES, which is an older algorithm that is now considered weak. The second example uses AES, which is a stronger modern algorithm.

require 'openssl'

class Encryptor
  attr_accessor :secret_key

  def encrypt_message_weak(message)
    cipher = OpenSSL::Cipher.new('des') # BAD: weak encryption
    cipher.encrypt
    cipher.key = secret_key
    cipher.update(message)
    cipher.final
  end

  def encrypt_message_strong(message)
    cipher = OpenSSL::Cipher::AES128.new # GOOD: strong encryption
    cipher.encrypt
    cipher.key = secret_key
    cipher.update(message)
    cipher.final
  end
end

References

NIST, FIPS 140 Annex a: Approved Security Functions.
NIST, SP 800-131A: Transitions: Recommendation for Transitioning the Use of Cryptographic Algorithms and Key Lengths.
OWASP: Rule - Use strong approved cryptographic algorithms.
Common Weakness Enumeration: CWE-327.

ruby/ql/lib/codeql/ruby/security/OpenSSL.qll

…ic-algorithm

github-actions · 2022-03-31T16:22:22Z

QHelp previews:

ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.qhelp

Use of a broken or weak cryptographic algorithm

Using broken or weak cryptographic algorithms can leave data vulnerable to being decrypted or forged by an attacker.

Many cryptographic algorithms provided by cryptography libraries are known to be weak, or flawed. Using such an algorithm means that encrypted or hashed data is less secure than it appears to be.

Recommendation

Ensure that you use a strong, modern cryptographic algorithm, such as AES-128 or RSA-2048.

Example

The following code uses the OpenSSL library to encrypt some secret data. When you create a cipher using OpenSSL you must specify the encryption algorithm to use. The first example uses DES, which is an older algorithm that is now considered weak. The second example uses AES, which is a stronger modern algorithm.

require 'openssl'

class Encryptor
  attr_accessor :secret_key

  def encrypt_message_weak(message)
    cipher = OpenSSL::Cipher.new('des') # BAD: weak encryption
    cipher.encrypt
    cipher.key = secret_key
    cipher.update(message)
    cipher.final
  end

  def encrypt_message_strong(message)
    cipher = OpenSSL::Cipher::AES128.new # GOOD: strong encryption
    cipher.encrypt
    cipher.key = secret_key
    cipher.update(message)
    cipher.final
  end
end

References

NIST, FIPS 140 Annex a: Approved Security Functions.
NIST, SP 800-131A: Transitions: Recommendation for Transitioning the Use of Cryptographic Algorithms and Key Lengths.
OWASP: Rule - Use strong approved cryptographic algorithms.
Common Weakness Enumeration: CWE-327.

nickrolfe

LGTM. I'll leave it up to you whether you address my comment about the block mode in the alert message.

alexrford · 2022-04-05T13:31:21Z

I'll merge this as is and work on the block mode changes as a follow-up. CryptographicOperation is partly shared with Python - it would be nice to align these so the query implementation can be identical.

alexrford added 6 commits March 13, 2022 21:21

Ruby: add CryptographicOperation concept

489391e

Ruby: model CipherOperations for OpenSSL

4234cfe

Ruby: qhelp for rb/weak-cryptographic-algorithm

446141a

Ruby: tests for rb/weak-cryptographic-algorithm

40b87e6

Ruby: Add rb/weak-cryptographic-algorithm query

94d5f3b

Ruby: changenote for rb/weak-cryptographic-algorithm

fc232ce

alexrford added the Ruby label Mar 13, 2022

github-actions bot added the documentation label Mar 13, 2022

alexrford marked this pull request as ready for review March 14, 2022 08:39

alexrford requested a review from a team as a code owner March 14, 2022 08:39

Ruby: Add qldoc for Cryptography module (from python version)

6eca036

aibaars reviewed Mar 17, 2022

View reviewed changes

alexrford added 3 commits March 18, 2022 14:38

Ruby: Improve Cryptography module qldoc

62bc035

Ruby: Tidy an exists

2bd25da

Ruby: split up CipherInstantiation charpred

b79bb72

alexrford requested a review from aibaars March 20, 2022 20:22

nickrolfe reviewed Mar 29, 2022

View reviewed changes

Ruby: replace a range field with instanceof

2b66dfa

nickrolfe reviewed Mar 31, 2022

View reviewed changes

ruby/ql/lib/codeql/ruby/security/OpenSSL.qll Outdated Show resolved Hide resolved

alexrford added 2 commits March 31, 2022 17:17

Merge remote-tracking branch 'origin/main' into ruby/weak-cryptograph…

882f78c

…ic-algorithm

Ruby: replace use of deprecated getStringOrSymbol()

8b0ebbf

nickrolfe approved these changes Mar 31, 2022

View reviewed changes

alexrford merged commit ccd7bb5 into github:main Apr 5, 2022

alexrford deleted the ruby/weak-cryptographic-algorithm branch April 5, 2022 13:35

Ruby: Add rb/weak-cryptographic-algorithm query #8421

Ruby: Add rb/weak-cryptographic-algorithm query #8421

Uh oh!

Conversation

alexrford commented Mar 13, 2022

Uh oh!

github-actions bot commented Mar 13, 2022

Use of a broken or weak cryptographic algorithm

Recommendation

Example

References

Uh oh!

github-actions bot commented Mar 14, 2022

Use of a broken or weak cryptographic algorithm

Recommendation

Example

References

Uh oh!

aibaars left a comment

Choose a reason for hiding this comment

Uh oh!

aibaars Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

aibaars Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

aibaars Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

alexrford Mar 20, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 20, 2022

Use of a broken or weak cryptographic algorithm

Recommendation

Example

References

Uh oh!

nickrolfe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickrolfe Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

alexrford Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

nickrolfe Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 31, 2022

Use of a broken or weak cryptographic algorithm

Recommendation

Example

References

Uh oh!

Uh oh!

github-actions bot commented Mar 31, 2022

Use of a broken or weak cryptographic algorithm

Recommendation

Example

References

Uh oh!

nickrolfe left a comment

Choose a reason for hiding this comment

Uh oh!

alexrford commented Apr 5, 2022

Uh oh!

Uh oh!

Ruby: Add `rb/weak-cryptographic-algorithm` query #8421

Ruby: Add `rb/weak-cryptographic-algorithm` query #8421