From 37eca95d440b37dc7615674ce1c3230eac31354d Mon Sep 17 00:00:00 2001 From: dilanbhalla Date: Tue, 11 Aug 2020 23:53:50 -0700 Subject: [PATCH] restructured library --- ql/src/experimental/CWE-327/BrokenCrypto.ql | 15 ++ .../CWE-327/BrokenCryptoAlgorithm.ql | 15 -- .../BrokenCryptoAlgorithmCustomizations.qll | 61 ------- .../experimental/CWE-327/CryptoLibraries.qll | 157 ++++++------------ ql/test/experimental/CWE-327/BadCrypto.go | 27 +-- .../CWE-327/BrokenCrypto.expected | 4 + .../experimental/CWE-327/BrokenCrypto.qlref | 1 + .../CWE-327/BrokenCryptoAlgorithm.expected | 2 - .../CWE-327/BrokenCryptoAlgorithm.qlref | 1 - 9 files changed, 89 insertions(+), 194 deletions(-) create mode 100644 ql/src/experimental/CWE-327/BrokenCrypto.ql delete mode 100644 ql/src/experimental/CWE-327/BrokenCryptoAlgorithm.ql delete mode 100644 ql/src/experimental/CWE-327/BrokenCryptoAlgorithmCustomizations.qll create mode 100644 ql/test/experimental/CWE-327/BrokenCrypto.expected create mode 100644 ql/test/experimental/CWE-327/BrokenCrypto.qlref delete mode 100644 ql/test/experimental/CWE-327/BrokenCryptoAlgorithm.expected delete mode 100644 ql/test/experimental/CWE-327/BrokenCryptoAlgorithm.qlref diff --git a/ql/src/experimental/CWE-327/BrokenCrypto.ql b/ql/src/experimental/CWE-327/BrokenCrypto.ql new file mode 100644 index 00000000000..5e31c8b0b8c --- /dev/null +++ b/ql/src/experimental/CWE-327/BrokenCrypto.ql @@ -0,0 +1,15 @@ +/** + * @name Use of a broken or risky cryptographic algorithm + * @description Using broken or weak cryptographic algorithms can allow an attacker to compromise security. + * @kind problem + * @problem.severity error + * @id go/broken-crypto + * @tags security + */ + +import go +import CryptoLibraries + +from CryptographicOperation badcrypto +where badcrypto.getAlgorithm().isWeak() +select badcrypto, "Use of weak or broken cryptographic algorithm" diff --git a/ql/src/experimental/CWE-327/BrokenCryptoAlgorithm.ql b/ql/src/experimental/CWE-327/BrokenCryptoAlgorithm.ql deleted file mode 100644 index ed7c61e2f47..00000000000 --- a/ql/src/experimental/CWE-327/BrokenCryptoAlgorithm.ql +++ /dev/null @@ -1,15 +0,0 @@ -/** - * @name Use of a broken or risky cryptographic algorithm - * @description Using broken or weak cryptographic algorithms can allow an attacker to compromise security. - * @kind problem - * @problem.severity error - * @id go/weak-cryptographic-algorithm - * @tags security - */ - -import go -import BrokenCryptoAlgorithmCustomizations::BrokenCryptoAlgorithm - -from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink -where cfg.hasFlowPath(source, sink) -select sink.getNode(), "Sensitive data is used in a broken or weak cryptographic algorithm." diff --git a/ql/src/experimental/CWE-327/BrokenCryptoAlgorithmCustomizations.qll b/ql/src/experimental/CWE-327/BrokenCryptoAlgorithmCustomizations.qll deleted file mode 100644 index 346213179da..00000000000 --- a/ql/src/experimental/CWE-327/BrokenCryptoAlgorithmCustomizations.qll +++ /dev/null @@ -1,61 +0,0 @@ -/** - * Provides default sources, sinks and sanitizers for reasoning about - * sensitive information in broken or weak cryptographic algorithms, - * as well as extension points for adding your own. - */ - -import go -private import semmle.go.security.SensitiveActions -private import CryptoLibraries - -module BrokenCryptoAlgorithm { - /** - * A data flow source for sensitive information in broken or weak cryptographic algorithms. - */ - abstract class Source extends DataFlow::Node { } - - /** - * A data flow sink for sensitive information in broken or weak cryptographic algorithms. - */ - abstract class Sink extends DataFlow::Node { } - - /** - * A sanitizer for sensitive information in broken or weak cryptographic algorithms. - */ - abstract class Sanitizer extends DataFlow::Node { } - - /** - * A sensitive source. - */ - class SensitiveSource extends Source { - SensitiveSource() { this.asExpr() instanceof SensitiveExpr } - } - - /** - * An expression used by a broken or weak cryptographic algorithm. - */ - class WeakCryptographicOperationSink extends Sink { - WeakCryptographicOperationSink() { - exists(CryptographicOperation application | - application.getAlgorithm().isWeak() and - this.asExpr() = application.getInput() - ) - } - } - - /** - * A configuration depicting taint flow from sensitive information to weak cryptographic algorithms. - */ - class Configuration extends TaintTracking::Configuration { - Configuration() { this = "BrokenCryptoAlgorithm" } - - override predicate isSource(DataFlow::Node source) { source instanceof Source } - - override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } - - override predicate isSanitizer(DataFlow::Node node) { - super.isSanitizer(node) or - node instanceof Sanitizer - } - } -} diff --git a/ql/src/experimental/CWE-327/CryptoLibraries.qll b/ql/src/experimental/CWE-327/CryptoLibraries.qll index 6b31623b8f1..89b1d484b83 100644 --- a/ql/src/experimental/CWE-327/CryptoLibraries.qll +++ b/ql/src/experimental/CWE-327/CryptoLibraries.qll @@ -6,79 +6,20 @@ import go /** * Names of cryptographic algorithms, separated into strong and weak variants. - * * The names are normalized: upper-case, no spaces, dashes or underscores. - * * The names are inspired by the names used in real world crypto libraries. - * * The classification into strong and weak are based on Wikipedia, OWASP and google (2017). */ private module AlgorithmNames { - predicate isStrongHashingAlgorithm(string name) { - name = "DSA" or - name = "ED25519" or - name = "ES256" or - name = "ECDSA256" or - name = "ES384" or - name = "ECDSA384" or - name = "ES512" or - name = "ECDSA512" or - name = "SHA2" or - name = "SHA224" or - name = "SHA256" or - name = "SHA384" or - name = "SHA512" or - name = "SHA3" - } - predicate isWeakHashingAlgorithm(string name) { - name = "HAVEL128" or - name = "MD2" or - name = "MD4" or name = "MD5" or - name = "PANAMA" or - name = "RIPEMD" or - name = "RIPEMD128" or - name = "RIPEMD256" or - name = "RIPEMD320" or - name = "SHA0" or name = "SHA1" } - predicate isStrongEncryptionAlgorithm(string name) { - name = "AES" or - name = "AES128" or - name = "AES192" or - name = "AES256" or - name = "AES512" or - name = "RSA" or - name = "RABBIT" or - name = "BLOWFISH" - } - predicate isWeakEncryptionAlgorithm(string name) { name = "DES" or - name = "3DES" or - name = "TRIPLEDES" or - name = "TDEA" or - name = "TRIPLEDEA" or - name = "ARC2" or - name = "RC2" or - name = "ARC4" or - name = "RC4" or - name = "ARCFOUR" or - name = "ARC5" or - name = "RC5" + name = "RC4" } - - predicate isStrongPasswordHashingAlgorithm(string name) { - name = "ARGON2" or - name = "PBKDF2" or - name = "BCRYPT" or - name = "SCRYPT" - } - - predicate isWeakPasswordHashingAlgorithm(string name) { none() } } private import AlgorithmNames @@ -87,20 +28,9 @@ private import AlgorithmNames * A cryptographic algorithm. */ private newtype TCryptographicAlgorithm = - MkHashingAlgorithm(string name, boolean isWeak) { - isStrongHashingAlgorithm(name) and isWeak = false - or - isWeakHashingAlgorithm(name) and isWeak = true - } or + MkHashingAlgorithm(string name, boolean isWeak) { isWeakHashingAlgorithm(name) and isWeak = true } or MkEncryptionAlgorithm(string name, boolean isWeak) { - isStrongEncryptionAlgorithm(name) and isWeak = false - or isWeakEncryptionAlgorithm(name) and isWeak = true - } or - MkPasswordHashingAlgorithm(string name, boolean isWeak) { - isStrongPasswordHashingAlgorithm(name) and isWeak = false - or - isWeakPasswordHashingAlgorithm(name) and isWeak = true } /** @@ -116,13 +46,11 @@ abstract class CryptographicAlgorithm extends TCryptographicAlgorithm { abstract string getName(); /** - * Holds if the name of this algorithm matches `name` modulo case, - * white space, dashes and underscores. + * Holds if the name of this algorithm matches `name` modulo case, white space, dashes and underscores. */ bindingset[name] predicate matchesName(string name) { exists(name.regexpReplaceAll("[-_]", "").regexpFind("(?i)\\Q" + getName() + "\\E", _, _)) - // name.toUpperCase().regexpReplaceAll("[-_ ]", "").regexpMatch(".*" + getName() + ".*") } /** @@ -132,7 +60,7 @@ abstract class CryptographicAlgorithm extends TCryptographicAlgorithm { } /** - * A hashing algorithm such as `MD5` or `SHA512`. + * A hashing algorithm */ class HashingAlgorithm extends MkHashingAlgorithm, CryptographicAlgorithm { string name; @@ -146,7 +74,7 @@ class HashingAlgorithm extends MkHashingAlgorithm, CryptographicAlgorithm { } /** - * An encryption algorithm such as `DES` or `AES512`. + * An encryption algorithm */ class EncryptionAlgorithm extends MkEncryptionAlgorithm, CryptographicAlgorithm { string name; @@ -159,29 +87,10 @@ class EncryptionAlgorithm extends MkEncryptionAlgorithm, CryptographicAlgorithm override predicate isWeak() { isWeak = true } } -/** - * A password hashing algorithm such as `PBKDF2` or `SCRYPT`. - */ -class PasswordHashingAlgorithm extends MkPasswordHashingAlgorithm, CryptographicAlgorithm { - string name; - boolean isWeak; - - PasswordHashingAlgorithm() { this = MkPasswordHashingAlgorithm(name, isWeak) } - - override string getName() { result = name } - - override predicate isWeak() { isWeak = true } -} - /** * An application of a cryptographic algorithm. */ abstract class CryptographicOperation extends Expr { - /** - * Gets the input the algorithm is used on, e.g. the plain text input to be encrypted. - */ - abstract Expr getInput(); - /** * Gets the applied algorithm. */ @@ -189,11 +98,9 @@ abstract class CryptographicOperation extends Expr { } /** - * Below are the cryptographic functions that have been implemented so far for this library. * Class that checks for use of Md5 package. */ class Md5 extends CryptographicOperation { - Expr input; CryptographicAlgorithm algorithm; SelectorExpr sel; CallExpr call; @@ -203,10 +110,33 @@ class Md5 extends CryptographicOperation { algorithm.matchesName(sel.getBase().toString()) and algorithm.matchesName("MD5") and sel.getSelector().toString() = call.getCalleeName().toString() and - call.getArgument(0) = input + ( + call.getCalleeName().toString() = "New" or + call.getCalleeName().toString() = "Sum" + ) } - override Expr getInput() { result = input } + override CryptographicAlgorithm getAlgorithm() { result = algorithm } +} + +/** + * Class that checks for use of SHA1 package. + */ +class Sha1 extends CryptographicOperation { + CryptographicAlgorithm algorithm; + SelectorExpr sel; + CallExpr call; + + Sha1() { + this = call and + algorithm.matchesName(sel.getBase().toString()) and + algorithm.matchesName("SHA1") and + sel.getSelector().toString() = call.getCalleeName().toString() and + ( + call.getCalleeName().toString() = "New" or + call.getCalleeName().toString() = "Sum" + ) + } override CryptographicAlgorithm getAlgorithm() { result = algorithm } } @@ -215,7 +145,6 @@ class Md5 extends CryptographicOperation { * Class that checks for use of Des package. */ class Des extends CryptographicOperation { - Expr input; CryptographicAlgorithm algorithm; SelectorExpr sel; CallExpr call; @@ -225,10 +154,30 @@ class Des extends CryptographicOperation { algorithm.matchesName(sel.getBase().toString()) and algorithm.matchesName("DES") and sel.getSelector().toString() = call.getCalleeName().toString() and - call.getArgument(0) = input + ( + call.getCalleeName().toString() = "NewCipher" or + call.getCalleeName().toString() = "NewTripleDESCipher" + ) + } + + override CryptographicAlgorithm getAlgorithm() { result = algorithm } +} + +/** + * Class that checks for use of RC4 package. + */ +class Rc4 extends CryptographicOperation { + CryptographicAlgorithm algorithm; + SelectorExpr sel; + CallExpr call; + + Rc4() { + this = call and + algorithm.matchesName(sel.getBase().toString()) and + algorithm.matchesName("RC4") and + sel.getSelector().toString() = call.getCalleeName().toString() and + call.getCalleeName().toString() = "NewCipher" } - override Expr getInput() { result = input } - override CryptographicAlgorithm getAlgorithm() { result = algorithm } } diff --git a/ql/test/experimental/CWE-327/BadCrypto.go b/ql/test/experimental/CWE-327/BadCrypto.go index c4437dc93e6..1feaf165257 100644 --- a/ql/test/experimental/CWE-327/BadCrypto.go +++ b/ql/test/experimental/CWE-327/BadCrypto.go @@ -3,24 +3,29 @@ package main import ( "crypto/des" "crypto/md5" - "fmt" + "crypto/rc4" + "crypto/sha1" + "crypto/sha256" ) func main() { password := []byte("password") - - var tripleDESKey []byte - tripleDESKey = append(tripleDESKey, password[:16]...) - tripleDESKey = append(tripleDESKey, password[:8]...) - // BAD, des is a weak crypto algorithm - _, err := des.NewTripleDESCipher(tripleDESKey) - if err != nil { - panic(err) - } + des.NewTripleDESCipher(password) // BAD, md5 is a weak crypto algorithm - fmt.Printf("%x", md5.Sum(password)) + md5.Sum(password) + key := []byte{ 1, 2, 3, 4, 5, 6, 7 } + // BAD, rc4 is a weak crypto algorithm + rc4.NewCipher(key) + + data := []byte("this page intentionally left blank.") + // BAD, sha1 is a weak crypto algorithm + sha1.Sum(data) + + sha256key := []byte("hello world") + // GOOD, sha256 is a strong crypto algorithm + sha256.Sum256([]byte(sha256key)) } diff --git a/ql/test/experimental/CWE-327/BrokenCrypto.expected b/ql/test/experimental/CWE-327/BrokenCrypto.expected new file mode 100644 index 00000000000..931db0c242d --- /dev/null +++ b/ql/test/experimental/CWE-327/BrokenCrypto.expected @@ -0,0 +1,4 @@ +| BadCrypto.go:15:2:15:33 | call to NewTripleDESCipher | Use of weak or broken cryptographic algorithm | +| BadCrypto.go:18:2:18:18 | call to Sum | Use of weak or broken cryptographic algorithm | +| BadCrypto.go:22:2:22:19 | call to NewCipher | Use of weak or broken cryptographic algorithm | +| BadCrypto.go:26:2:26:15 | call to Sum | Use of weak or broken cryptographic algorithm | diff --git a/ql/test/experimental/CWE-327/BrokenCrypto.qlref b/ql/test/experimental/CWE-327/BrokenCrypto.qlref new file mode 100644 index 00000000000..502955695eb --- /dev/null +++ b/ql/test/experimental/CWE-327/BrokenCrypto.qlref @@ -0,0 +1 @@ +experimental/CWE-327/BrokenCrypto.ql diff --git a/ql/test/experimental/CWE-327/BrokenCryptoAlgorithm.expected b/ql/test/experimental/CWE-327/BrokenCryptoAlgorithm.expected deleted file mode 100644 index 1ecef951863..00000000000 --- a/ql/test/experimental/CWE-327/BrokenCryptoAlgorithm.expected +++ /dev/null @@ -1,2 +0,0 @@ -| BadCrypto.go:18:35:18:46 | tripleDESKey | Sensitive data is used in a broken or weak cryptographic algorithm. | -| BadCrypto.go:24:27:24:34 | password | Sensitive data is used in a broken or weak cryptographic algorithm. | diff --git a/ql/test/experimental/CWE-327/BrokenCryptoAlgorithm.qlref b/ql/test/experimental/CWE-327/BrokenCryptoAlgorithm.qlref deleted file mode 100644 index dc7d541336c..00000000000 --- a/ql/test/experimental/CWE-327/BrokenCryptoAlgorithm.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/CWE-327/BrokenCryptoAlgorithm.ql