Python: Port py/weak-cryptographic-algorithm

The other query (py/weak-sensitive-data-hashing) is added in future commit

Author: RasmusWL

python/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.qhelp

@@ -15,22 +15,28 @@
                secure than it appears to be.
           </p>
 
+          <p>
+               This query alerts on any use of a weak cryptographic algorithm, that is
+               not a hashing algorithm. Use of broken or weak cryptographic hash
+               functions are handled by the
+               <code>py/weak-sensitive-data-hashing</code> query.
+          </p>
+
      </overview>
      <recommendation>
 
           <p>
                Ensure that you use a strong, modern cryptographic
-               algorithm. Use at least AES-128 or RSA-2048 for
-               encryption, and SHA-2 or SHA-3 for secure hashing.
+               algorithm, such as AES-128 or RSA-2048.
           </p>
 
      </recommendation>
      <example>
 
           <p>
-               The following code uses the <code>pycrypto</code>
+               The following code uses the <code>pycryptodome</code>
                library to encrypt some secret data. When you create a cipher using
-               <code>pycrypto</code> you must specify the encryption
+               <code>pycryptodome</code> 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.
@@ -39,8 +45,12 @@
           <sample src="examples/broken_crypto.py" />
 
           <p>
-               WARNING: Although the second example above is more robust,
-               pycrypto is no longer actively maintained so we recommend using <code>cryptography</code> instead.
+               NOTICE: the original
+               <a href="https://pypi.org/project/pycrypto/"><code>pycrypto</code></a>
+               PyPI package that provided the <code>Crypto</code> module is not longer
+               actively maintained, so you should use the
+               <a href="https://pypi.org/project/pycryptodome/"><code>pycryptodome</code></a>
+               PyPI package instead (which has a compatible API).
           </p>
 
      </example>

python/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql

@@ -1,7 +1,7 @@
 /**
  * @name Use of a broken or weak cryptographic algorithm
  * @description Using broken or weak cryptographic algorithms can compromise security.
- * @kind path-problem
+ * @kind problem
  * @problem.severity warning
  * @precision high
  * @id py/weak-cryptographic-algorithm
@@ -10,21 +10,14 @@
  */
 
 import python
-import semmle.python.security.Paths
-import semmle.python.security.SensitiveData
-import semmle.python.security.Crypto
+import semmle.python.Concepts
 
-class BrokenCryptoConfiguration extends TaintTracking::Configuration {
-  BrokenCryptoConfiguration() { this = "Broken crypto configuration" }
-
-  override predicate isSource(TaintTracking::Source source) {
-    source instanceof SensitiveDataSource
-  }
-
-  override predicate isSink(TaintTracking::Sink sink) { sink instanceof WeakCryptoSink }
-}
-
-from BrokenCryptoConfiguration config, TaintedPathSource src, TaintedPathSink sink
-where config.hasFlowPath(src, sink)
-select sink.getSink(), src, sink, "$@ is used in a broken or weak cryptographic algorithm.",
-  src.getSource(), "Sensitive data"
+from Cryptography::CryptographicOperation operation, Cryptography::CryptographicAlgorithm algorithm
+where
+  algorithm = operation.getAlgorithm() and
+  algorithm.isWeak() and
+  not algorithm instanceof Cryptography::HashingAlgorithm and // handled by `py/weak-sensitive-data-hashing`
+  not algorithm instanceof Cryptography::PasswordHashingAlgorithm // handled by `py/weak-sensitive-data-hashing`
+select operation,
+  "The cryptographic algorithm " + algorithm.getName() +
+    " is broken or weak, and should not be used."

python/ql/src/experimental/Security-old-dataflow/CWE-327/BrokenCryptoAlgorithm.ql

@@ -0,0 +1,28 @@
+/**
+ * @name OLD QUERY: Use of a broken or weak cryptographic algorithm
+ * @description Using broken or weak cryptographic algorithms can compromise security.
+ * @kind path-problem
+ * @problem.severity warning
+ * @id py/old/weak-cryptographic-algorithm
+ * @deprecated
+ */
+
+import python
+import semmle.python.security.Paths
+import semmle.python.security.SensitiveData
+import semmle.python.security.Crypto
+
+class BrokenCryptoConfiguration extends TaintTracking::Configuration {
+  BrokenCryptoConfiguration() { this = "Broken crypto configuration" }
+
+  override predicate isSource(TaintTracking::Source source) {
+    source instanceof SensitiveDataSource
+  }
+
+  override predicate isSink(TaintTracking::Sink sink) { sink instanceof WeakCryptoSink }
+}
+
+from BrokenCryptoConfiguration config, TaintedPathSource src, TaintedPathSink sink
+where config.hasFlowPath(src, sink)
+select sink.getSink(), src, sink, "$@ is used in a broken or weak cryptographic algorithm.",
+  src.getSource(), "Sensitive data"

python/ql/test/query-tests/Security/CWE-327-BrokenCryptoAlgorithm/BrokenCryptoAlgorithm.expected

@@ -1,8 +1,2 @@
-edges
-| test_cryptography.py:5:17:5:30 | a password | test_cryptography.py:8:29:8:37 | a password |
-| test_cryptography.py:5:17:5:30 | a password | test_cryptography.py:8:29:8:37 | a password |
-| test_pycrypto.py:5:17:5:30 | a password | test_pycrypto.py:7:27:7:35 | a password |
-| test_pycrypto.py:5:17:5:30 | a password | test_pycrypto.py:7:27:7:35 | a password |
-#select
-| test_cryptography.py:8:29:8:37 | dangerous | test_cryptography.py:5:17:5:30 | a password | test_cryptography.py:8:29:8:37 | a password | $@ is used in a broken or weak cryptographic algorithm. | test_cryptography.py:5:17:5:30 | get_password() | Sensitive data |
-| test_pycrypto.py:7:27:7:35 | dangerous | test_pycrypto.py:5:17:5:30 | a password | test_pycrypto.py:7:27:7:35 | a password | $@ is used in a broken or weak cryptographic algorithm. | test_pycrypto.py:5:17:5:30 | get_password() | Sensitive data |
+| test_cryptodome.py:11:13:11:42 | ControlFlowNode for Attribute() | The cryptographic algorithm ARC4 is broken or weak, and should not be used. |
+| test_cryptography.py:13:13:13:44 | ControlFlowNode for Attribute() | The cryptographic algorithm ARC4 is broken or weak, and should not be used. |

python/ql/test/query-tests/Security/CWE-327-BrokenCryptoAlgorithm/README.md

@@ -0,0 +1,3 @@
+Note that the tests in this directory are very shallow, and simply show that the query is able to produce alerts.
+
+More in-depth tests can be found for the individual frameworks that we have modeled `Cryptography::CryptographicOperation` for.

python/ql/test/query-tests/Security/CWE-327-BrokenCryptoAlgorithm/TestNode.expected

@@ -0,0 +1,13 @@
+# snippet from python/ql/test/experimental/library-tests/frameworks/cryptodome/test_rc4.py
+from Cryptodome.Cipher import ARC4
+
+import os
+
+key = os.urandom(256//8)
+
+secret_message = b"secret message"
+
+cipher = ARC4.new(key)
+encrypted = cipher.encrypt(secret_message) # NOT OK
+
+print(secret_message, encrypted)

python/ql/test/query-tests/Security/CWE-327-BrokenCryptoAlgorithm/TestNode.ql

@@ -1,9 +1,16 @@
-from cryptography.hazmat.primitives.ciphers import Cipher, algorithms
-from secrets_store import get_password
+# snippet from python/ql/test/experimental/library-tests/frameworks/cryptography/test_rc4.py
+from cryptography.hazmat.primitives.ciphers import algorithms, Cipher
+import os
 
-def get_badly_encrypted_password():
-    dangerous = get_password()
-    cipher = Cipher(algorithms.ARC4(key), _, _)
-    encryptor = cipher.encryptor()
-    return encryptor.update(dangerous) + encryptor.finalize()
+key = os.urandom(256//8)
 
+algorithm = algorithms.ARC4(key)
+cipher = Cipher(algorithm, mode=None)
+
+secret_message = b"secret message"
+
+encryptor = cipher.encryptor()
+encrypted = encryptor.update(secret_message) # NOT OK
+encrypted += encryptor.finalize()
+
+print(secret_message, encrypted)