diff --git a/go/ql/integration-tests/query-suite/go-code-scanning.qls.expected b/go/ql/integration-tests/query-suite/go-code-scanning.qls.expected index 20fcacbc3896..d8d3e0a13e75 100644 --- a/go/ql/integration-tests/query-suite/go-code-scanning.qls.expected +++ b/go/ql/integration-tests/query-suite/go-code-scanning.qls.expected @@ -18,7 +18,9 @@ ql/go/ql/src/Security/CWE-295/DisabledCertificateCheck.ql ql/go/ql/src/Security/CWE-312/CleartextLogging.ql ql/go/ql/src/Security/CWE-322/InsecureHostKeyCallback.ql ql/go/ql/src/Security/CWE-326/InsufficientKeySize.ql +ql/go/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql ql/go/ql/src/Security/CWE-327/InsecureTLS.ql +ql/go/ql/src/Security/CWE-327/WeakSensitiveDataHashing.ql ql/go/ql/src/Security/CWE-338/InsecureRandomness.ql ql/go/ql/src/Security/CWE-347/MissingJwtSignatureCheck.ql ql/go/ql/src/Security/CWE-352/ConstantOauth2State.ql diff --git a/go/ql/integration-tests/query-suite/go-security-and-quality.qls.expected b/go/ql/integration-tests/query-suite/go-security-and-quality.qls.expected index ee0ec8f42ba7..71bcb2b03300 100644 --- a/go/ql/integration-tests/query-suite/go-security-and-quality.qls.expected +++ b/go/ql/integration-tests/query-suite/go-security-and-quality.qls.expected @@ -41,7 +41,9 @@ ql/go/ql/src/Security/CWE-295/DisabledCertificateCheck.ql ql/go/ql/src/Security/CWE-312/CleartextLogging.ql ql/go/ql/src/Security/CWE-322/InsecureHostKeyCallback.ql ql/go/ql/src/Security/CWE-326/InsufficientKeySize.ql +ql/go/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql ql/go/ql/src/Security/CWE-327/InsecureTLS.ql +ql/go/ql/src/Security/CWE-327/WeakSensitiveDataHashing.ql ql/go/ql/src/Security/CWE-338/InsecureRandomness.ql ql/go/ql/src/Security/CWE-347/MissingJwtSignatureCheck.ql ql/go/ql/src/Security/CWE-352/ConstantOauth2State.ql diff --git a/go/ql/integration-tests/query-suite/go-security-extended.qls.expected b/go/ql/integration-tests/query-suite/go-security-extended.qls.expected index 9116f7b7ebf6..b0447a6c3eb3 100644 --- a/go/ql/integration-tests/query-suite/go-security-extended.qls.expected +++ b/go/ql/integration-tests/query-suite/go-security-extended.qls.expected @@ -19,7 +19,9 @@ ql/go/ql/src/Security/CWE-295/DisabledCertificateCheck.ql ql/go/ql/src/Security/CWE-312/CleartextLogging.ql ql/go/ql/src/Security/CWE-322/InsecureHostKeyCallback.ql ql/go/ql/src/Security/CWE-326/InsufficientKeySize.ql +ql/go/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql ql/go/ql/src/Security/CWE-327/InsecureTLS.ql +ql/go/ql/src/Security/CWE-327/WeakSensitiveDataHashing.ql ql/go/ql/src/Security/CWE-338/InsecureRandomness.ql ql/go/ql/src/Security/CWE-347/MissingJwtSignatureCheck.ql ql/go/ql/src/Security/CWE-352/ConstantOauth2State.ql diff --git a/go/ql/integration-tests/query-suite/not_included_in_qls.expected b/go/ql/integration-tests/query-suite/not_included_in_qls.expected index 3b79e47da44f..7b8b86a7a533 100644 --- a/go/ql/integration-tests/query-suite/not_included_in_qls.expected +++ b/go/ql/integration-tests/query-suite/not_included_in_qls.expected @@ -14,7 +14,6 @@ ql/go/ql/src/experimental/CWE-203/Timing.ql ql/go/ql/src/experimental/CWE-285/PamAuthBypass.ql ql/go/ql/src/experimental/CWE-287/ImproperLdapAuth.ql ql/go/ql/src/experimental/CWE-321-V2/HardCodedKeys.ql -ql/go/ql/src/experimental/CWE-327/WeakCryptoAlgorithm.ql ql/go/ql/src/experimental/CWE-369/DivideByZero.ql ql/go/ql/src/experimental/CWE-400/DatabaseCallInLoop.ql ql/go/ql/src/experimental/CWE-522-DecompressionBombs/DecompressionBombs.ql diff --git a/go/ql/lib/go.qll b/go/ql/lib/go.qll index 16f2f1702faa..688214aae855 100644 --- a/go/ql/lib/go.qll +++ b/go/ql/lib/go.qll @@ -33,6 +33,7 @@ import semmle.go.frameworks.AwsLambda import semmle.go.frameworks.Beego import semmle.go.frameworks.BeegoOrm import semmle.go.frameworks.Bun +import semmle.go.frameworks.CryptoLibraries import semmle.go.frameworks.RsCors import semmle.go.frameworks.Couchbase import semmle.go.frameworks.Echo diff --git a/go/ql/lib/qlpack.yml b/go/ql/lib/qlpack.yml index bf2586d9089e..145bb4747ff6 100644 --- a/go/ql/lib/qlpack.yml +++ b/go/ql/lib/qlpack.yml @@ -6,6 +6,7 @@ extractor: go library: true upgrades: upgrades dependencies: + codeql/concepts: ${workspace} codeql/dataflow: ${workspace} codeql/mad: ${workspace} codeql/threat-models: ${workspace} diff --git a/go/ql/lib/semmle/go/Concepts.qll b/go/ql/lib/semmle/go/Concepts.qll index 1931f16871ab..acb16b62d077 100644 --- a/go/ql/lib/semmle/go/Concepts.qll +++ b/go/ql/lib/semmle/go/Concepts.qll @@ -8,6 +8,10 @@ import go import semmle.go.dataflow.FunctionInputsAndOutputs import semmle.go.concepts.HTTP import semmle.go.concepts.GeneratedFile +private import codeql.concepts.ConceptsShared +private import semmle.go.dataflow.internal.DataFlowImplSpecific + +private module ConceptsShared = ConceptsMake; /** * A data-flow node that executes an operating system command, @@ -505,3 +509,98 @@ module UnmarshalingFunction { abstract string getFormat(); } } + +/** + * Provides models for cryptographic things. + */ +module Cryptography { + private import ConceptsShared::Cryptography as SC + + /** + * A data-flow node that is an application of a cryptographic algorithm. For example, + * encryption, decryption, signature-validation. + * + * Extend this class to refine existing API models. If you want to model new APIs, + * extend `CryptographicOperation::Range` instead. + */ + class CryptographicOperation extends SC::CryptographicOperation { } + + class EncryptionAlgorithm = SC::EncryptionAlgorithm; + + class HashingAlgorithm = SC::HashingAlgorithm; + + class PasswordHashingAlgorithm = SC::PasswordHashingAlgorithm; + + module CryptographicOperation = SC::CryptographicOperation; + + class BlockMode = SC::BlockMode; + + class CryptographicAlgorithm = SC::CryptographicAlgorithm; + + /** A data flow node that initializes a hash algorithm. */ + abstract class HashAlgorithmInit extends DataFlow::Node { + /** Gets the hash algorithm being initialized. */ + abstract HashingAlgorithm getAlgorithm(); + } + + /** A data flow node that is an application of a hash algorithm. */ + abstract class HashOperation extends CryptographicOperation::Range { + override BlockMode getBlockMode() { none() } + } + + /** A data flow node that initializes an encryption algorithm. */ + abstract class EncryptionAlgorithmInit extends DataFlow::Node { + /** Gets the encryption algorithm being initialized. */ + abstract EncryptionAlgorithm getAlgorithm(); + } + + /** + * A data flow node that initializes a block cipher mode of operation, and + * may also propagate taint for encryption algorithms. + */ + abstract class BlockModeInit extends DataFlow::CallNode { + /** Gets the block cipher mode of operation being initialized. */ + abstract BlockMode getMode(); + + /** Gets a step propagating the encryption algorithm through this call. */ + abstract predicate step(DataFlow::Node node1, DataFlow::Node node2); + } + + /** + * A data flow node that is an application of an encryption algorithm, where + * the encryption algorithm and the block cipher mode of operation (if there + * is one) have been initialized separately. + */ + abstract class EncryptionOperation extends CryptographicOperation::Range { + DataFlow::Node encryptionFlowTarget; + DataFlow::Node inputNode; + + override DataFlow::Node getInitialization() { + EncryptionFlow::flow(result, encryptionFlowTarget) + } + + override EncryptionAlgorithm getAlgorithm() { + result = this.getInitialization().(EncryptionAlgorithmInit).getAlgorithm() + } + + override DataFlow::Node getAnInput() { result = inputNode } + + override BlockMode getBlockMode() { + result = this.getInitialization().(BlockModeInit).getMode() + } + } + + /** + * An `EncryptionOperation` which is a method call where the encryption + * algorithm and block cipher mode of operation (if there is one) flow to the + * receiver and the input is an argument. + */ + abstract class EncryptionMethodCall extends EncryptionOperation instanceof DataFlow::CallNode { + int inputArg; + + EncryptionMethodCall() { + encryptionFlowTarget = super.getReceiver() and + inputNode = super.getArgument(inputArg) + } + } +} diff --git a/go/ql/lib/semmle/go/frameworks/CryptoLibraries.qll b/go/ql/lib/semmle/go/frameworks/CryptoLibraries.qll new file mode 100644 index 000000000000..154ac82e7a2a --- /dev/null +++ b/go/ql/lib/semmle/go/frameworks/CryptoLibraries.qll @@ -0,0 +1,484 @@ +/** + * Provides classes for modeling cryptographic libraries. + */ + +import go +import semmle.go.Concepts::Cryptography +private import codeql.concepts.internal.CryptoAlgorithmNames + +/** + * A data flow call node that is an application of a hash operation where the + * hash algorithm is defined in any earlier initialization node, and the input + * is the first argument of the call. + */ +abstract class DirectHashOperation extends HashOperation instanceof DataFlow::CallNode { + override DataFlow::Node getInitialization() { result = this } + + override DataFlow::Node getAnInput() { result = super.getArgument(0) } +} + +private module HashConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof HashAlgorithmInit } + + predicate isSink(DataFlow::Node sink) { any() } +} + +/** Tracks the flow of hash algorithms. */ +module HashFlow = DataFlow::Global; + +/** + * A data flow node that initializes a block mode and propagates the encryption + * algorithm from the first argument to the receiver. + */ +abstract class StdLibNewEncrypter extends BlockModeInit { + override predicate step(DataFlow::Node node1, DataFlow::Node node2) { + node1 = this.getArgument(0) and + node2 = this.getResult(0) + } +} + +private module EncryptionConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + source instanceof EncryptionAlgorithmInit or + source instanceof BlockModeInit + } + + predicate isSink(DataFlow::Node sink) { any() } + + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + any(BlockModeInit nbcm).step(node1, node2) + } +} + +/** + * Tracks algorithms and block cipher modes of operation used for encryption. + */ +module EncryptionFlow = DataFlow::Global; + +private module Crypto { + private module Aes { + private class NewCipher extends EncryptionAlgorithmInit { + NewCipher() { + exists(Function f | this = f.getACall().getResult(0) | + f.hasQualifiedName("crypto/aes", "NewCipher") + ) + } + + override EncryptionAlgorithm getAlgorithm() { result.matchesName("AES") } + } + } + + private module Des { + private class NewCipher extends EncryptionAlgorithmInit { + NewCipher() { + exists(Function f | this = f.getACall().getResult(0) | + f.hasQualifiedName("crypto/des", "NewCipher") + ) + } + + override EncryptionAlgorithm getAlgorithm() { result.matchesName("DES") } + } + + private class NewTripleDESCipher extends EncryptionAlgorithmInit { + NewTripleDESCipher() { + exists(Function f | this = f.getACall().getResult(0) | + f.hasQualifiedName("crypto/des", "NewTripleDESCipher") + ) + } + + override EncryptionAlgorithm getAlgorithm() { result.matchesName("TRIPLEDES") } + } + } + + private module Md5 { + private class Sum extends DirectHashOperation instanceof DataFlow::CallNode { + Sum() { this.getTarget().hasQualifiedName("crypto/md5", "Sum") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("MD5") } + } + + private class New extends HashAlgorithmInit instanceof DataFlow::CallNode { + New() { this.getTarget().hasQualifiedName("crypto/md5", "New") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("MD5") } + } + } + + private module Rc4 { + private class CipherXorKeyStream extends CryptographicOperation::Range instanceof DataFlow::CallNode + { + CipherXorKeyStream() { + this.(DataFlow::MethodCallNode) + .getTarget() + .hasQualifiedName("crypto/rc4", "Cipher", "XORKeyStream") + } + + override DataFlow::Node getInitialization() { result = this } + + override EncryptionAlgorithm getAlgorithm() { result.matchesName("RC4") } + + override DataFlow::Node getAnInput() { result = super.getArgument(1) } + + override BlockMode getBlockMode() { none() } + } + } + + /** + * Cryptographic operations from the `crypto/sha1` package. + */ + private module Sha1 { + private class Sum extends DirectHashOperation instanceof DataFlow::CallNode { + Sum() { this.getTarget().hasQualifiedName("crypto/sha1", "Sum") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA1") } + } + + private class New extends HashAlgorithmInit instanceof DataFlow::CallNode { + New() { this.getTarget().hasQualifiedName("crypto/sha1", "New") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA1") } + } + } + + /** + * Cryptographic operations from the `crypto/sha256` package. + */ + private module Sha256 { + private class Sum256 extends DirectHashOperation instanceof DataFlow::CallNode { + Sum256() { this.getTarget().hasQualifiedName("crypto/sha256", "Sum256") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA256") } + } + + private class Sum224 extends DirectHashOperation instanceof DataFlow::CallNode { + Sum224() { this.getTarget().hasQualifiedName("crypto/sha256", "Sum224") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA224") } + } + + private class New extends HashAlgorithmInit instanceof DataFlow::CallNode { + New() { this.getTarget().hasQualifiedName("crypto/sha256", "New") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA256") } + } + + private class New224 extends HashAlgorithmInit instanceof DataFlow::CallNode { + New224() { this.getTarget().hasQualifiedName("crypto/sha256", "New224") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA224") } + } + } + + private module Sha3 { + private class Sum224 extends DirectHashOperation instanceof DataFlow::CallNode { + Sum224() { this.getTarget().hasQualifiedName("crypto/sha3", "Sum224") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA3224") } + } + + private class Sum256 extends DirectHashOperation instanceof DataFlow::CallNode { + Sum256() { this.getTarget().hasQualifiedName("crypto/sha3", "Sum256") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA3256") } + } + + private class Sum384 extends DirectHashOperation instanceof DataFlow::CallNode { + Sum384() { this.getTarget().hasQualifiedName("crypto/sha3", "Sum384") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA3384") } + } + + private class Sum512 extends DirectHashOperation instanceof DataFlow::CallNode { + Sum512() { this.getTarget().hasQualifiedName("crypto/sha3", "Sum512") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA3512") } + } + + private class SumShake128 extends DirectHashOperation instanceof DataFlow::CallNode { + SumShake128() { this.getTarget().hasQualifiedName("crypto/sha3", "SumSHAKE128") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHAKE128") } + } + + private class SumShake256 extends DirectHashOperation instanceof DataFlow::CallNode { + SumShake256() { this.getTarget().hasQualifiedName("crypto/sha3", "SumSHAKE256") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHAKE256") } + } + + private class New224 extends HashAlgorithmInit instanceof DataFlow::CallNode { + New224() { this.getTarget().hasQualifiedName("crypto/sha3", "New224") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA3224") } + } + + private class New256 extends HashAlgorithmInit instanceof DataFlow::CallNode { + New256() { this.getTarget().hasQualifiedName("crypto/sha3", "New256") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA3256") } + } + + private class New384 extends HashAlgorithmInit instanceof DataFlow::CallNode { + New384() { this.getTarget().hasQualifiedName("crypto/sha3", "New384") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA3384") } + } + + private class New512 extends HashAlgorithmInit instanceof DataFlow::CallNode { + New512() { this.getTarget().hasQualifiedName("crypto/sha3", "New512") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA3512") } + } + + private class NewShake128 extends HashAlgorithmInit instanceof DataFlow::CallNode { + NewShake128() { + this.getTarget().hasQualifiedName("crypto/sha3", ["NewCSHAKE128", "NewSHAKE128"]) + } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHAKE128") } + } + + private class NewShake256 extends HashAlgorithmInit instanceof DataFlow::CallNode { + NewShake256() { + this.getTarget().hasQualifiedName("crypto/sha3", ["NewCSHAKE256", "NewSHAKE256"]) + } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHAKE256") } + } + + private class ShakeWrite extends HashOperation instanceof DataFlow::MethodCallNode { + ShakeWrite() { this.getTarget().hasQualifiedName("crypto/sha3", "SHAKE", "Write") } + + override HashAlgorithmInit getInitialization() { HashFlow::flow(result, super.getReceiver()) } + + override HashingAlgorithm getAlgorithm() { result = this.getInitialization().getAlgorithm() } + + override DataFlow::Node getAnInput() { result = super.getArgument(0) } + } + } + + private module Sha512 { + private class Sum384 extends DirectHashOperation instanceof DataFlow::CallNode { + Sum384() { this.getTarget().hasQualifiedName("crypto/sha512", "Sum384") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA384") } + } + + private class Sum512 extends DirectHashOperation instanceof DataFlow::CallNode { + Sum512() { this.getTarget().hasQualifiedName("crypto/sha512", "Sum512") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA512") } + } + + private class Sum512_224 extends DirectHashOperation instanceof DataFlow::CallNode { + Sum512_224() { this.getTarget().hasQualifiedName("crypto/sha512", "Sum512_224") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA512224") } + } + + private class Sum512_256 extends DirectHashOperation instanceof DataFlow::CallNode { + Sum512_256() { this.getTarget().hasQualifiedName("crypto/sha512", "Sum512_256") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA512256") } + } + + private class New extends HashAlgorithmInit instanceof DataFlow::CallNode { + New() { this.getTarget().hasQualifiedName("crypto/sha512", "New") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA512") } + } + + private class New384 extends HashAlgorithmInit instanceof DataFlow::CallNode { + New384() { this.getTarget().hasQualifiedName("crypto/sha512", "New384") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA384") } + } + + private class New512_224 extends HashAlgorithmInit instanceof DataFlow::CallNode { + New512_224() { this.getTarget().hasQualifiedName("crypto/sha512", "New512_224") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA512224") } + } + + private class New512_256 extends HashAlgorithmInit instanceof DataFlow::CallNode { + New512_256() { this.getTarget().hasQualifiedName("crypto/sha512", "New512_256") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("SHA512256") } + } + } + + private module Cipher { + private class NewCbcEncrypter extends StdLibNewEncrypter { + NewCbcEncrypter() { this.getTarget().hasQualifiedName("crypto/cipher", "NewCBCEncrypter") } + + override BlockMode getMode() { result = "CBC" } + } + + private class NewCfbEncrypter extends StdLibNewEncrypter { + NewCfbEncrypter() { this.getTarget().hasQualifiedName("crypto/cipher", "NewCFBEncrypter") } + + override BlockMode getMode() { result = "CFB" } + } + + private class NewCtr extends StdLibNewEncrypter { + NewCtr() { this.getTarget().hasQualifiedName("crypto/cipher", "NewCTR") } + + override BlockMode getMode() { result = "CTR" } + } + + private class NewGcm extends StdLibNewEncrypter { + NewGcm() { + exists(string name | this.getTarget().hasQualifiedName("crypto/cipher", name) | + name = ["NewGCM", "NewGCMWithNonceSize", "NewGCMWithRandomNonce", "NewGCMWithTagSize"] + ) + } + + override BlockMode getMode() { result = "GCM" } + } + + private class NewOfb extends StdLibNewEncrypter { + NewOfb() { this.getTarget().hasQualifiedName("crypto/cipher", "NewOFB") } + + override BlockMode getMode() { result = "OFB" } + } + + private class AeadSeal extends EncryptionMethodCall { + AeadSeal() { + this.(DataFlow::MethodCallNode) + .getTarget() + .hasQualifiedName("crypto/cipher", "AEAD", "Seal") and + inputArg = 2 + } + } + + private class BlockEncrypt extends EncryptionMethodCall { + BlockEncrypt() { + this.(DataFlow::MethodCallNode) + .getTarget() + .hasQualifiedName("crypto/cipher", "Block", "Encrypt") and + inputArg = 1 + } + } + + private class BlockModeCryptBlocks extends EncryptionMethodCall { + BlockModeCryptBlocks() { + this.(DataFlow::MethodCallNode) + .getTarget() + .hasQualifiedName("crypto/cipher", "BlockMode", "CryptBlocks") and + inputArg = 1 + } + } + + private class StreamXorKeyStream extends EncryptionMethodCall { + StreamXorKeyStream() { + this.(DataFlow::MethodCallNode) + .getTarget() + .hasQualifiedName("crypto/cipher", "Stream", "XORKeyStream") and + inputArg = 1 + } + } + + private class StreamReader extends EncryptionOperation { + StreamReader() { + lookThroughPointerType(this.getType()).hasQualifiedName("crypto/cipher", "StreamReader") and + exists(DataFlow::Write w, DataFlow::Node base, Field f | + f.hasQualifiedName("crypto/cipher", "StreamReader", "S") and + w.writesField(base, f, encryptionFlowTarget) and + DataFlow::localFlow(base, this) + ) and + exists(DataFlow::Write w, DataFlow::Node base, Field f | + f.hasQualifiedName("crypto/cipher", "StreamReader", "R") and + w.writesField(base, f, inputNode) and + DataFlow::localFlow(base, this) + ) + } + } + + /** + * Limitation: StreamWriter wraps a Writer, so we need to look forward to + * where the Writer is written to. Currently this is done using local flow, + * so it only works within one function. + */ + private class StreamWriter extends EncryptionOperation { + StreamWriter() { + lookThroughPointerType(this.getType()).hasQualifiedName("crypto/cipher", "StreamWriter") and + inputNode = this and + exists(DataFlow::Write w, DataFlow::Node base, Field f | + w.writesField(base, f, encryptionFlowTarget) and + f.hasQualifiedName("crypto/cipher", "StreamWriter", "S") + | + base = this or + TaintTracking::localTaint(base, this.(DataFlow::PostUpdateNode).getPreUpdateNode()) + ) + } + } + } +} + +private module Hash { + private class HashSum extends HashOperation instanceof DataFlow::MethodCallNode { + HashSum() { this.getTarget().implements("hash", "Hash", "Sum") } + + override HashAlgorithmInit getInitialization() { HashFlow::flow(result, super.getReceiver()) } + + override HashingAlgorithm getAlgorithm() { result = this.getInitialization().getAlgorithm() } + + override DataFlow::Node getAnInput() { result = super.getArgument(0) } + } +} + +private DataFlow::Node getANonIoWriterPredecessor(DataFlow::Node node) { + node.getType().implements("io", "Writer") and + exists(DataFlow::Node pre | TaintTracking::localTaintStep(pre, node) | + if pre.getType().implements("io", "Writer") + then result = getANonIoWriterPredecessor(pre) + else result = pre + ) +} + +/** + * Taint flowing to an `io.Writer` (such as `hash.Hash` or `*sha3.SHAKE`) via + * its implementation of the `io.Writer` interface. + */ +private class FlowToIoWriter extends HashOperation instanceof DataFlow::Node { + HashAlgorithmInit init; + DataFlow::Node input; + + FlowToIoWriter() { + this.getType().implements("io", "Writer") and + HashFlow::flow(init, this) and + // If we have `h.Write(taint)` or `io.WriteString(h, taint)` then it's + // the post-update node of `h` that gets tainted. + exists(DataFlow::PostUpdateNode pun | pun.getPreUpdateNode() = this | + input = getANonIoWriterPredecessor(pun) + ) + } + + override HashAlgorithmInit getInitialization() { result = init } + + override HashingAlgorithm getAlgorithm() { result = this.getInitialization().getAlgorithm() } + + override DataFlow::Node getAnInput() { result = input } +} + +/** + * Currently only weak algorithms from the `golang.org/x/crypto` module are + * modeled here. + */ +private module GolangOrgXCrypto { + private module Md4 { + private class New extends HashAlgorithmInit instanceof DataFlow::CallNode { + New() { this.getTarget().hasQualifiedName("golang.org/x/crypto/md4", "New") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("MD4") } + } + } + + private module Ripemd160 { + private class New extends HashAlgorithmInit instanceof DataFlow::CallNode { + New() { this.getTarget().hasQualifiedName("golang.org/x/crypto/ripemd160", "New") } + + override HashingAlgorithm getAlgorithm() { result.matchesName("RIPEMD160") } + } + } +} diff --git a/go/ql/lib/semmle/go/security/WeakSensitiveDataHashingCustomizations.qll b/go/ql/lib/semmle/go/security/WeakSensitiveDataHashingCustomizations.qll new file mode 100644 index 000000000000..4893193ef4fe --- /dev/null +++ b/go/ql/lib/semmle/go/security/WeakSensitiveDataHashingCustomizations.qll @@ -0,0 +1,171 @@ +/** + * Provides default sources, sinks and sanitizers for detecting "use of a + * broken or weak cryptographic hashing algorithm on sensitive data" + * vulnerabilities, as well as extension points for adding your own. This is + * divided into two general cases: + * - hashing sensitive data + * - hashing passwords (which requires the hashing algorithm to be + * sufficiently computationally expensive in addition to other requirements) + */ + +import go +private import semmle.go.security.SensitiveActions + +/** + * Provides default sources, sinks and sanitizers for detecting "use of a broken or weak + * cryptographic hashing algorithm on sensitive data" vulnerabilities on sensitive data that does + * NOT require computationally expensive hashing, as well as extension points for adding your own. + * + * Also see the `ComputationallyExpensiveHashFunction` module. + */ +module NormalHashFunction { + /** + * A data flow source for "use of a broken or weak cryptographic hashing algorithm on sensitive + * data" vulnerabilities that does not require computationally expensive hashing. That is, a + * piece of sensitive data that is not a password. + */ + abstract class Source extends DataFlow::Node { + Source() { not this instanceof ComputationallyExpensiveHashFunction::Source } + + /** + * Gets the classification of the sensitive data. + */ + abstract string getClassification(); + } + + /** + * A data flow sink for "use of a broken or weak cryptographic hashing algorithm on sensitive + * data" vulnerabilities that applies to data that does not require computationally expensive + * hashing. That is, a broken or weak hashing algorithm. + */ + abstract class Sink extends DataFlow::Node { + /** + * Gets the name of the weak hashing algorithm. + */ + abstract string getAlgorithmName(); + } + + /** + * A barrier for "use of a broken or weak cryptographic hashing algorithm on sensitive data" + * vulnerabilities that applies to data that does not require computationally expensive hashing. + */ + abstract class Barrier extends DataFlow::Node { } + + /** + * A flow source modeled by the `SensitiveData` library. + */ + class SensitiveDataAsSource extends Source { + SensitiveExpr::Classification classification; + + SensitiveDataAsSource() { + classification = this.asExpr().(SensitiveExpr).getClassification() and + not classification = SensitiveExpr::password() and // (covered in ComputationallyExpensiveHashFunction) + not classification = SensitiveExpr::id() // (not accurate enough) + } + + override SensitiveExpr::Classification getClassification() { result = classification } + } + + /** + * A flow sink modeled by the `Cryptography` module. + */ + class WeakHashingOperationInputAsSink extends Sink { + Cryptography::HashingAlgorithm algorithm; + + WeakHashingOperationInputAsSink() { + exists(Cryptography::CryptographicOperation operation | + algorithm.isWeak() and + algorithm = operation.getAlgorithm() and + this = operation.getAnInput() + ) + } + + override string getAlgorithmName() { result = algorithm.getName() } + } +} + +/** + * Provides default sources, sinks and sanitizers for detecting "use of a broken or weak + * cryptographic hashing algorithm on sensitive data" vulnerabilities on sensitive data that DOES + * require computationally expensive hashing, as well as extension points for adding your own. + * + * Also see the `NormalHashFunction` module. + */ +module ComputationallyExpensiveHashFunction { + /** + * A data flow source for "use of a broken or weak cryptographic hashing algorithm on sensitive + * data" vulnerabilities that does require computationally expensive hashing. That is, a + * password. + */ + abstract class Source extends DataFlow::Node { + /** + * Gets the classification of the sensitive data. + */ + abstract string getClassification(); + } + + /** + * A data flow sink for "use of a broken or weak cryptographic hashing algorithm on sensitive + * data" vulnerabilities that applies to data that does require computationally expensive + * hashing. That is, a broken or weak hashing algorithm or one that is not computationally + * expensive enough for password hashing. + */ + abstract class Sink extends DataFlow::Node { + /** + * Gets the name of the weak hashing algorithm. + */ + abstract string getAlgorithmName(); + + /** + * Holds if this sink is for a computationally expensive hash function (meaning that hash + * function is just weak in some other regard. + */ + abstract predicate isComputationallyExpensive(); + } + + /** + * A barrier for "use of a broken or weak cryptographic hashing algorithm on sensitive data" + * vulnerabilities that applies to data that does require computationally expensive hashing. + */ + abstract class Barrier extends DataFlow::Node { } + + /** + * A flow source modeled by the `SensitiveData` library. + */ + class PasswordAsSource extends Source { + SensitiveExpr::Classification classification; + + PasswordAsSource() { + classification = this.asExpr().(SensitiveExpr).getClassification() and + classification = SensitiveExpr::password() + } + + override SensitiveExpr::Classification getClassification() { result = classification } + } + + /** + * A flow sink modeled by the `Cryptography` module. + */ + class WeakPasswordHashingOperationInputSink extends Sink { + Cryptography::CryptographicAlgorithm algorithm; + + WeakPasswordHashingOperationInputSink() { + exists(Cryptography::CryptographicOperation operation | + ( + algorithm instanceof Cryptography::PasswordHashingAlgorithm and + algorithm.isWeak() + or + algorithm instanceof Cryptography::HashingAlgorithm // Note that HashingAlgorithm and PasswordHashingAlgorithm are disjoint + ) and + algorithm = operation.getAlgorithm() and + this = operation.getAnInput() + ) + } + + override string getAlgorithmName() { result = algorithm.getName() } + + override predicate isComputationallyExpensive() { + algorithm instanceof Cryptography::PasswordHashingAlgorithm + } + } +} diff --git a/go/ql/src/experimental/CWE-327/WeakCryptoAlgorithm.qhelp b/go/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.qhelp similarity index 88% rename from go/ql/src/experimental/CWE-327/WeakCryptoAlgorithm.qhelp rename to go/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.qhelp index 0283e142fbad..a65af78c0aec 100644 --- a/go/ql/src/experimental/CWE-327/WeakCryptoAlgorithm.qhelp +++ b/go/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.qhelp @@ -28,10 +28,10 @@

- The following code uses the different packages to encrypt/hash - some secret data. The first few examples uses DES, MD5, RC4, and SHA1, - which are older algorithms that are now considered weak. The following - examples use AES and SHA256, which are stronger, more modern algorithms. + The following code uses the different packages to encrypt + some secret data. The first example uses DES, + which is an older algorithm that is now considered weak. The following + example uses AES, which is a stronger, more modern algorithm.

diff --git a/go/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql b/go/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql new file mode 100644 index 000000000000..67b2d0d6d2b4 --- /dev/null +++ b/go/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql @@ -0,0 +1,33 @@ +/** + * @name Use of a broken or weak cryptographic algorithm + * @description Using broken or weak cryptographic algorithms can compromise security. + * @kind problem + * @problem.severity warning + * @security-severity 7.5 + * @precision high + * @id go/weak-cryptographic-algorithm + * @tags security + * external/cwe/cwe-327 + * external/cwe/cwe-328 + */ + +import go + +from Cryptography::CryptographicOperation operation, string msgPrefix, DataFlow::Node init +where + init = operation.getInitialization() and + // `init` may be a `BlockModeInit`, a `EncryptionAlgorithmInit`, or `operation` itself. + ( + not init instanceof BlockModeInit and + exists(Cryptography::CryptographicAlgorithm algorithm | + algorithm = operation.getAlgorithm() and + algorithm.isWeak() and + msgPrefix = "The cryptographic algorithm " + algorithm.getName() and + not algorithm instanceof Cryptography::HashingAlgorithm + ) + or + not init instanceof EncryptionAlgorithmInit and + operation.getBlockMode().isWeak() and + msgPrefix = "The block mode " + operation.getBlockMode() + ) +select operation, "$@ is broken or weak, and should not be used.", init, msgPrefix diff --git a/go/ql/src/Security/CWE-327/WeakSensitiveDataHashing.qhelp b/go/ql/src/Security/CWE-327/WeakSensitiveDataHashing.qhelp new file mode 100644 index 000000000000..1fa64e4adaf2 --- /dev/null +++ b/go/ql/src/Security/CWE-327/WeakSensitiveDataHashing.qhelp @@ -0,0 +1,97 @@ + + + +

+ Using 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: +

+
    +
  • + pre-image attacks: if you know a hash value h(x), + you should not be able to easily find the input x. +
    • +
  • + collision attacks: if you know a hash value h(x), + you should not be able to easily find a different input y + with the same hash value h(x) = h(y). +
    • +
+

+ In cases with a limited input space, such as for passwords, the hash + function also needs to be computationally expensive to be resistant to + brute-force attacks. Passwords should also have an unique salt applied + before hashing, but that is not considered by this query. +

+ +

+ 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 rb/weak-cryptographic-algorithm query. +

+ + + + +

+ Ensure that you use a strong, modern cryptographic hash function: +

+ +
    +
  • + such as Argon2, scrypt, bcrypt, or PBKDF2 for passwords and other data with limited input space. +
    • +
  • + such as SHA-2, or SHA-3 in other cases. +
    • +
+ + + + +

+ The following example shows two functions for checking whether the hash + of a secret matches a known value. + + The first function uses SHA-1 that is known to be vulnerable to collision attacks. + + The second function uses SHA-256 that is a strong cryptographic hashing function. +

+ + + + + +

+ The following example shows two functions for hashing passwords. + + The first example 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. + + The second function uses PBKDF2, which is a strong password hashing algorithm. +

+ + + + + + +
  • OWASP: Password Storage Cheat Sheet
    • +     + +     diff --git a/go/ql/src/Security/CWE-327/WeakSensitiveDataHashing.ql b/go/ql/src/Security/CWE-327/WeakSensitiveDataHashing.ql new file mode 100644 index 000000000000..0a38d9729f01 --- /dev/null +++ b/go/ql/src/Security/CWE-327/WeakSensitiveDataHashing.ql @@ -0,0 +1,118 @@ +/** + * @name Use of a broken or weak cryptographic hashing algorithm on sensitive data + * @description Using broken or weak cryptographic hashing algorithms can compromise security. + * @kind path-problem + * @problem.severity warning + * @security-severity 7.5 + * @precision high + * @id go/weak-sensitive-data-hashing + * @tags security + * external/cwe/cwe-327 + * external/cwe/cwe-328 + * external/cwe/cwe-916 + */ + +import go +import semmle.go.security.WeakSensitiveDataHashingCustomizations + +/** + * Provides a taint-tracking configuration for detecting use of a broken or weak + * cryptographic hash function on sensitive data, that does NOT require a + * computationally expensive hash function. + */ +module NormalHashFunctionFlow { + import NormalHashFunction + + private module Config implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof Source } + + predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + predicate isBarrier(DataFlow::Node node) { node instanceof Barrier } + + predicate isBarrierIn(DataFlow::Node node) { + // make sources barriers so that we only report the closest instance + isSource(node) + } + + predicate isBarrierOut(DataFlow::Node node) { + // make sinks barriers so that we only report the closest instance + isSink(node) + } + + predicate observeDiffInformedIncrementalMode() { any() } + } + + import TaintTracking::Global +} + +/** + * Provides a taint-tracking configuration for detecting use of a broken or weak + * cryptographic hashing algorithm on passwords. + * + * Passwords has stricter requirements on the hashing algorithm used (must be + * computationally expensive to prevent brute-force attacks). + */ +module ComputationallyExpensiveHashFunctionFlow { + import ComputationallyExpensiveHashFunction + + private module Config implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof Source } + + predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + predicate isBarrier(DataFlow::Node node) { node instanceof Barrier } + + predicate isBarrierIn(DataFlow::Node node) { + // make sources barriers so that we only report the closest instance + isSource(node) + } + + predicate isBarrierOut(DataFlow::Node node) { + // make sinks barriers so that we only report the closest instance + isSink(node) + } + + predicate observeDiffInformedIncrementalMode() { any() } + } + + import TaintTracking::Global +} + +/** + * Global taint-tracking for detecting both variants of "use of a broken or weak + * cryptographic hashing algorithm on sensitive data" vulnerabilities. The two configurations are + * merged to generate a combined path graph. + */ +module WeakSensitiveDataHashingFlow = + DataFlow::MergePathGraph; + +import WeakSensitiveDataHashingFlow::PathGraph + +from + WeakSensitiveDataHashingFlow::PathNode source, WeakSensitiveDataHashingFlow::PathNode sink, + string ending, string algorithmName, string classification +where + NormalHashFunctionFlow::flowPath(source.asPathNode1(), sink.asPathNode1()) and + algorithmName = sink.getNode().(NormalHashFunction::Sink).getAlgorithmName() and + classification = source.getNode().(NormalHashFunction::Source).getClassification() and + ending = "." + or + ComputationallyExpensiveHashFunctionFlow::flowPath(source.asPathNode2(), sink.asPathNode2()) and + algorithmName = sink.getNode().(ComputationallyExpensiveHashFunction::Sink).getAlgorithmName() and + classification = + source.getNode().(ComputationallyExpensiveHashFunction::Source).getClassification() and + ( + sink.getNode().(ComputationallyExpensiveHashFunction::Sink).isComputationallyExpensive() and + ending = "." + or + not sink.getNode().(ComputationallyExpensiveHashFunction::Sink).isComputationallyExpensive() and + ending = + " for " + classification + + " hashing, since it is not a computationally expensive hash function." + ) +select sink.getNode(), source, sink, + "$@ is used in a hashing algorithm (" + algorithmName + ") that is insecure" + ending, + source.getNode(), "Sensitive data (" + classification + ")" diff --git a/go/ql/src/Security/CWE-327/examples/Crypto.go b/go/ql/src/Security/CWE-327/examples/Crypto.go new file mode 100644 index 000000000000..b3f71f0772bc --- /dev/null +++ b/go/ql/src/Security/CWE-327/examples/Crypto.go @@ -0,0 +1,20 @@ +package main + +import ( + "crypto/aes" + "crypto/des" +) + +func EncryptMessageWeak(key []byte, message []byte) (dst []byte) { + // BAD, DES is a weak crypto algorithm + block, _ := des.NewCipher(key) + block.Encrypt(dst, message) + return +} + +func EncryptMessageStrong(key []byte, message []byte) (dst []byte) { + // GOOD, AES is a weak crypto algorithm + block, _ := aes.NewCipher(key) + block.Encrypt(dst, message) + return +} diff --git a/go/ql/src/experimental/CWE-327/examples/InsecureRandomness.go b/go/ql/src/Security/CWE-327/examples/InsecureRandomness.go similarity index 100% rename from go/ql/src/experimental/CWE-327/examples/InsecureRandomness.go rename to go/ql/src/Security/CWE-327/examples/InsecureRandomness.go diff --git a/go/ql/src/Security/CWE-327/examples/WeakPasswordHashing.go b/go/ql/src/Security/CWE-327/examples/WeakPasswordHashing.go new file mode 100644 index 000000000000..671edede7d36 --- /dev/null +++ b/go/ql/src/Security/CWE-327/examples/WeakPasswordHashing.go @@ -0,0 +1,21 @@ +package main + +import ( + "crypto/pbkdf2" + "crypto/rand" + "crypto/sha256" + "crypto/sha512" +) + +func GetPasswordHashBad(password string) [32]byte { + // BAD, SHA256 is a strong hashing algorithm but it is not computationally expensive + return sha256.Sum256([]byte(password)) +} + +func GetPasswordHashGood(password string) []byte { + // GOOD, PBKDF2 is a strong hashing algorithm and it is computationally expensive + salt := make([]byte, 16) + rand.Read(salt) + key, _ := pbkdf2.Key(sha512.New, password, salt, 4096, 32) + return key +} diff --git a/go/ql/src/Security/CWE-327/examples/WeakSecretHashing.go b/go/ql/src/Security/CWE-327/examples/WeakSecretHashing.go new file mode 100644 index 000000000000..fd65b802548b --- /dev/null +++ b/go/ql/src/Security/CWE-327/examples/WeakSecretHashing.go @@ -0,0 +1,19 @@ +package main + +import ( + "crypto/sha1" + "crypto/sha256" + "slices" +) + +func SecretMatchesKnownHashBad(secret []byte, known_hash []byte) bool { + // BAD, SHA1 is a weak crypto algorithm and secret is sensitive data + h := sha1.New() + return slices.Equal(h.Sum(secret), known_hash) +} + +func SecretMatchesKnownHashGood(secret []byte, known_hash []byte) bool { + // GOOD, SHA256 is a strong hashing algorithm + h := sha256.New() + return slices.Equal(h.Sum(secret), known_hash) +} diff --git a/go/ql/src/change-notes/2025-10-21-go-weak-crypto-algorithm.md b/go/ql/src/change-notes/2025-10-21-go-weak-crypto-algorithm.md new file mode 100644 index 000000000000..d5f9a08fb7f8 --- /dev/null +++ b/go/ql/src/change-notes/2025-10-21-go-weak-crypto-algorithm.md @@ -0,0 +1,5 @@ +--- +category: newQuery +--- +* Added a new query, `go/weak-crypto-algorithm`, to detect the use of a broken or weak cryptographic algorithm. A very simple version of this query was originally contributed as an [experimental query by @dilanbhalla](https://github.com/github/codeql-go/pull/284). +* Added a new query, `go/weak-sensitive-data-hashing`, to detect the use of a broken or weak cryptographic hash algorithm on sensitive data. diff --git a/go/ql/src/experimental/CWE-327/CryptoLibraries.qll b/go/ql/src/experimental/CWE-327/CryptoLibraries.qll deleted file mode 100644 index 5518067b668b..000000000000 --- a/go/ql/src/experimental/CWE-327/CryptoLibraries.qll +++ /dev/null @@ -1,213 +0,0 @@ -/** - * Provides classes for modeling cryptographic libraries. - */ - -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 OWASP and Wikipedia (2020). - * - * Sources (more links in qhelp file): - * https://en.wikipedia.org/wiki/Strong_cryptography#Cryptographically_strong_algorithms - * https://en.wikipedia.org/wiki/Strong_cryptography#Examples - * https://cheatsheetseries.owasp.org/cheatsheets/Cryptographic_Storage_Cheat_Sheet.html - */ -private module AlgorithmNames { - predicate isStrongHashingAlgorithm(string name) { - name = - [ - "DSA", "ED25519", "SHA256", "SHA384", "SHA512", "SHA3", "ES256", "ECDSA256", "ES384", - "ECDSA384", "ES512", "ECDSA512", "SHA2", "SHA224" - ] - } - - predicate isWeakHashingAlgorithm(string name) { - name = - [ - "HAVEL128", "MD2", "SHA1", "MD4", "MD5", "PANAMA", "RIPEMD", "RIPEMD128", "RIPEMD256", - "RIPEMD320", "SHA0" - ] - } - - predicate isStrongEncryptionAlgorithm(string name) { - name = ["AES", "AES128", "AES192", "AES256", "AES512", "RSA", "RABBIT", "BLOWFISH"] - } - - predicate isWeakEncryptionAlgorithm(string name) { - name = - [ - "DES", "3DES", "ARC5", "RC5", "TRIPLEDES", "TDEA", "TRIPLEDEA", "ARC2", "RC2", "ARC4", - "RC4", "ARCFOUR" - ] - } - - predicate isStrongPasswordHashingAlgorithm(string name) { - name = ["ARGON2", "PBKDF2", "BCRYPT", "SCRYPT"] - } - - predicate isWeakPasswordHashingAlgorithm(string name) { none() } -} - -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 - 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 - } - -/** - * A cryptographic algorithm. - */ -abstract class CryptographicAlgorithm extends TCryptographicAlgorithm { - /** Gets a textual representation of this element. */ - string toString() { result = this.getName() } - - /** - * Gets the name of this algorithm. - */ - abstract string getName(); - - /** - * 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" + this.getName() + "\\E", _, _)) - } - - /** - * Holds if this algorithm is weak. - */ - abstract predicate isWeak(); -} - -/** - * A hashing algorithm such as `MD5` or `SHA512`. - */ -class HashingAlgorithm extends MkHashingAlgorithm, CryptographicAlgorithm { - string name; - boolean isWeak; - - HashingAlgorithm() { this = MkHashingAlgorithm(name, isWeak) } - - override string getName() { result = name } - - override predicate isWeak() { isWeak = true } -} - -/** - * An encryption algorithm such as `DES` or `AES512`. - */ -class EncryptionAlgorithm extends MkEncryptionAlgorithm, CryptographicAlgorithm { - string name; - boolean isWeak; - - EncryptionAlgorithm() { this = MkEncryptionAlgorithm(name, isWeak) } - - override string getName() { result = name } - - 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 DataFlow::Node { - /** - * Gets the input the algorithm is used on, e.g. the plain text input to be encrypted. - */ - abstract Expr getInput(); - - /** - * Gets the applied algorithm. - */ - abstract CryptographicAlgorithm getAlgorithm(); -} - -/** - * A cryptographic operation from the `crypto/md5` package. - */ -class Md5 extends CryptographicOperation, DataFlow::CallNode { - Md5() { this.getTarget().hasQualifiedName("crypto/md5", ["New", "Sum"]) } - - override Expr getInput() { result = this.getArgument(0).asExpr() } - - override CryptographicAlgorithm getAlgorithm() { - result.matchesName(this.getTarget().getPackage().getName()) - } -} - -/** - * A cryptographic operation from the `crypto/sha1` package. - */ -class Sha1 extends CryptographicOperation, DataFlow::CallNode { - Sha1() { this.getTarget().hasQualifiedName("crypto/sha1", ["New", "Sum"]) } - - override Expr getInput() { result = this.getArgument(0).asExpr() } - - override CryptographicAlgorithm getAlgorithm() { - result.matchesName(this.getTarget().getPackage().getName()) - } -} - -/** - * A cryptographic operation from the `crypto/des` package. - */ -class Des extends CryptographicOperation, DataFlow::CallNode { - Des() { this.getTarget().hasQualifiedName("crypto/des", ["NewCipher", "NewTripleDESCipher"]) } - - override Expr getInput() { result = this.getArgument(0).asExpr() } - - override CryptographicAlgorithm getAlgorithm() { - result.matchesName(this.getTarget().getPackage().getName()) - } -} - -/** - * A cryptographic operation from the `crypto/rc4` package. - */ -class Rc4 extends CryptographicOperation, DataFlow::CallNode { - Rc4() { this.getTarget().hasQualifiedName("crypto/rc4", "NewCipher") } - - override Expr getInput() { result = this.getArgument(0).asExpr() } - - override CryptographicAlgorithm getAlgorithm() { - result.matchesName(this.getTarget().getPackage().getName()) - } -} diff --git a/go/ql/src/experimental/CWE-327/WeakCryptoAlgorithm.ql b/go/ql/src/experimental/CWE-327/WeakCryptoAlgorithm.ql deleted file mode 100644 index ffaaae8e02a5..000000000000 --- a/go/ql/src/experimental/CWE-327/WeakCryptoAlgorithm.ql +++ /dev/null @@ -1,20 +0,0 @@ -/** - * @name Use of a weak cryptographic algorithm - * @description Using weak cryptographic algorithms can allow an attacker to compromise security. - * @kind path-problem - * @problem.severity error - * @id go/weak-crypto-algorithm - * @tags security - * experimental - * external/cwe/cwe-327 - * external/cwe/cwe-328 - */ - -import go -import WeakCryptoAlgorithmCustomizations -import WeakCryptoAlgorithm::Flow::PathGraph - -from WeakCryptoAlgorithm::Flow::PathNode source, WeakCryptoAlgorithm::Flow::PathNode sink -where WeakCryptoAlgorithm::Flow::flowPath(source, sink) -select sink.getNode(), source, sink, "$@ is used in a weak cryptographic algorithm.", - source.getNode(), "Sensitive data" diff --git a/go/ql/src/experimental/CWE-327/WeakCryptoAlgorithmCustomizations.qll b/go/ql/src/experimental/CWE-327/WeakCryptoAlgorithmCustomizations.qll deleted file mode 100644 index b9104f1fe096..000000000000 --- a/go/ql/src/experimental/CWE-327/WeakCryptoAlgorithmCustomizations.qll +++ /dev/null @@ -1,66 +0,0 @@ -/** - * Provides default sources, sinks and sanitizers for reasoning about - * sensitive information in weak cryptographic algorithms, - * as well as extension points for adding your own. - */ - -import go -private import semmle.go.security.SensitiveActions -private import CryptoLibraries - -/** - * Provides default sources, sinks and sanitizers for reasoning about - * sensitive information in weak cryptographic algorithms, - * as well as extension points for adding your own. - */ -module WeakCryptoAlgorithm { - /** - * A data flow source for sensitive information in weak cryptographic algorithms. - */ - abstract class Source extends DataFlow::Node { } - - /** - * A data flow sink for sensitive information in weak cryptographic algorithms. - */ - abstract class Sink extends DataFlow::Node { } - - /** - * A sanitizer for sensitive information in 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 weak cryptographic algorithm. - */ - class WeakCryptographicOperationSink extends Sink { - WeakCryptographicOperationSink() { - exists(CryptographicOperation application | - application.getAlgorithm().isWeak() and - this.asExpr() = application.getInput() - ) - } - } - - private module Config implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { source instanceof Source } - - predicate isSink(DataFlow::Node sink) { sink instanceof Sink } - - predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer } - - predicate observeDiffInformedIncrementalMode() { any() } - } - - /** - * Tracks taint flow from sensitive information to weak cryptographic - * algorithms. - */ - module Flow = TaintTracking::Global; -} diff --git a/go/ql/src/experimental/CWE-327/examples/Crypto.go b/go/ql/src/experimental/CWE-327/examples/Crypto.go deleted file mode 100644 index bc2b2fdeba44..000000000000 --- a/go/ql/src/experimental/CWE-327/examples/Crypto.go +++ /dev/null @@ -1,53 +0,0 @@ -package main - -import ( - "crypto/aes" - "crypto/des" - "crypto/md5" - "crypto/rc4" - "crypto/sha1" - "crypto/sha256" -) - -func main() { - public := []byte("hello") - - password := []byte("123456") - buf := password // testing dataflow by passing into different variable - - // BAD, des is a weak crypto algorithm and password is sensitive data - des.NewTripleDESCipher(buf) - - // BAD, md5 is a weak crypto algorithm and password is sensitive data - md5.Sum(buf) - - // BAD, rc4 is a weak crypto algorithm and password is sensitive data - rc4.NewCipher(buf) - - // BAD, sha1 is a weak crypto algorithm and password is sensitive data - sha1.Sum(buf) - - // GOOD, password is sensitive data but aes is a strong crypto algorithm - aes.NewCipher(buf) - - // GOOD, password is sensitive data but sha256 is a strong crypto algorithm - sha256.Sum256(buf) - - // GOOD, des is a weak crypto algorithm but public is not sensitive data - des.NewTripleDESCipher(public) - - // GOOD, md5 is a weak crypto algorithm but public is not sensitive data - md5.Sum(public) - - // GOOD, rc4 is a weak crypto algorithm but public is not sensitive data - rc4.NewCipher(public) - - // GOOD, sha1 is a weak crypto algorithm but public is not sensitive data - sha1.Sum(public) - - // GOOD, aes is a strong crypto algorithm and public is not sensitive data - aes.NewCipher(public) - - // GOOD, sha256 is a strong crypto algorithm and public is not sensitive data - sha256.Sum256(public) -} diff --git a/go/ql/test/query-tests/Security/CWE-327/BrokenCryptoAlgorithm.expected b/go/ql/test/query-tests/Security/CWE-327/BrokenCryptoAlgorithm.expected new file mode 100644 index 000000000000..00eb67fea0ba --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-327/BrokenCryptoAlgorithm.expected @@ -0,0 +1,29 @@ +| encryption.go:30:2:30:36 | call to Encrypt | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:34:2:34:42 | call to Seal | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:38:2:38:42 | call to Seal | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:42:2:42:42 | call to Seal | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:46:2:46:42 | call to Seal | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:50:2:50:47 | call to CryptBlocks | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:54:2:54:45 | call to XORKeyStream | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:56:22:56:91 | struct literal | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:59:21:59:68 | &... [postupdate] | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:59:22:59:68 | struct literal | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:59:22:59:68 | struct literal [postupdate] | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:60:10:60:24 | ctrStreamWriter [postupdate] | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:65:2:65:45 | call to XORKeyStream | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:69:2:69:45 | call to XORKeyStream | $@ is broken or weak, and should not be used. | encryption.go:28:2:28:31 | ... := ...[0] | The cryptographic algorithm DES | +| encryption.go:76:2:76:32 | call to Encrypt | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:80:2:80:38 | call to Seal | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:84:2:84:38 | call to Seal | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:88:2:88:42 | call to Seal | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:92:2:92:42 | call to Seal | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:96:2:96:43 | call to CryptBlocks | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:100:2:100:41 | call to XORKeyStream | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:102:22:102:87 | struct literal | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:105:21:105:68 | &... [postupdate] | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:105:22:105:68 | struct literal | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:105:22:105:68 | struct literal [postupdate] | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:106:10:106:24 | ctrStreamWriter [postupdate] | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:111:2:111:45 | call to XORKeyStream | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:115:2:115:45 | call to XORKeyStream | $@ is broken or weak, and should not be used. | encryption.go:74:2:74:40 | ... := ...[0] | The cryptographic algorithm TRIPLEDES | +| encryption.go:166:2:166:33 | call to XORKeyStream | $@ is broken or weak, and should not be used. | encryption.go:166:2:166:33 | call to XORKeyStream | The cryptographic algorithm RC4 | diff --git a/go/ql/test/query-tests/Security/CWE-327/BrokenCryptoAlgorithm.qlref b/go/ql/test/query-tests/Security/CWE-327/BrokenCryptoAlgorithm.qlref new file mode 100644 index 000000000000..a618df1ed20a --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-327/BrokenCryptoAlgorithm.qlref @@ -0,0 +1,4 @@ +query: Security/CWE-327/BrokenCryptoAlgorithm.ql +postprocess: + - utils/test/PrettyPrintModels.ql + - utils/test/InlineExpectationsTestQuery.ql diff --git a/go/ql/test/query-tests/Security/CWE-327/Crypto.go b/go/ql/test/query-tests/Security/CWE-327/Crypto.go deleted file mode 100644 index 75229b020a86..000000000000 --- a/go/ql/test/query-tests/Security/CWE-327/Crypto.go +++ /dev/null @@ -1,53 +0,0 @@ -package main - -import ( - "crypto/aes" - "crypto/des" - "crypto/md5" - "crypto/rc4" - "crypto/sha1" - "crypto/sha256" -) - -func crypto() { - public := []byte("hello") - - password := []byte("123456") - buf := password // testing dataflow by passing into different variable - - // BAD, des is a weak crypto algorithm and password is sensitive data - des.NewTripleDESCipher(buf) - - // BAD, md5 is a weak crypto algorithm and password is sensitive data - md5.Sum(buf) - - // BAD, rc4 is a weak crypto algorithm and password is sensitive data - rc4.NewCipher(buf) - - // BAD, sha1 is a weak crypto algorithm and password is sensitive data - sha1.Sum(buf) - - // GOOD, password is sensitive data but aes is a strong crypto algorithm - aes.NewCipher(buf) - - // GOOD, password is sensitive data but sha256 is a strong crypto algorithm - sha256.Sum256(buf) - - // GOOD, des is a weak crypto algorithm but public is not sensitive data - des.NewTripleDESCipher(public) - - // GOOD, md5 is a weak crypto algorithm but public is not sensitive data - md5.Sum(public) - - // GOOD, rc4 is a weak crypto algorithm but public is not sensitive data - rc4.NewCipher(public) - - // GOOD, sha1 is a weak crypto algorithm but public is not sensitive data - sha1.Sum(public) - - // GOOD, aes is a strong crypto algorithm and public is not sensitive data - aes.NewCipher(public) - - // GOOD, sha256 is a strong crypto algorithm and public is not sensitive data - sha256.Sum256(public) -} diff --git a/go/ql/test/query-tests/Security/CWE-327/CryptoAlgorithm.expected b/go/ql/test/query-tests/Security/CWE-327/CryptoAlgorithm.expected new file mode 100644 index 000000000000..55e9aed2e93c --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-327/CryptoAlgorithm.expected @@ -0,0 +1,2 @@ +testFailures +invalidModelRow diff --git a/go/ql/test/query-tests/Security/CWE-327/CryptoAlgorithm.ql b/go/ql/test/query-tests/Security/CWE-327/CryptoAlgorithm.ql new file mode 100644 index 000000000000..ab096f9df2ac --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-327/CryptoAlgorithm.ql @@ -0,0 +1,47 @@ +import go +import ModelValidation +import utils.test.InlineExpectationsTest + +module Test implements TestSig { + string getARelevantTag() { result = "CryptographicOperation" } + + predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "CryptographicOperation" and + exists( + CryptographicOperation::Range ho, string algorithm, string initialization, string blockMode + | + algorithm = ho.getAlgorithm().toString() + "." and + ( + blockMode = " blockMode: " + ho.getBlockMode().toString() + "." + or + not exists(ho.getBlockMode()) and blockMode = "" + ) and + exists(int c | c = count(ho.getInitialization()) | + c = 0 and initialization = "" + or + c = 1 and + initialization = + " init from line " + + strictconcat(DataFlow::Node init | + init = ho.getInitialization() + | + init.getStartLine().toString(), "," + ) + "." + or + c > 1 and + initialization = + " init from lines " + + strictconcat(DataFlow::Node init | + init = ho.getInitialization() + | + init.getStartLine().toString(), "," + ) + "." + ) and + ho.getLocation() = location and + element = ho.toString() and + value = "\"" + algorithm + blockMode + initialization + "\"" + ) + } +} + +import MakeTest diff --git a/go/ql/test/query-tests/Security/CWE-327/WeakCryptoAlgorithm.expected b/go/ql/test/query-tests/Security/CWE-327/WeakCryptoAlgorithm.expected deleted file mode 100644 index 53cfd40145d3..000000000000 --- a/go/ql/test/query-tests/Security/CWE-327/WeakCryptoAlgorithm.expected +++ /dev/null @@ -1,17 +0,0 @@ -edges -| Crypto.go:16:9:16:16 | password | Crypto.go:19:25:19:27 | buf | provenance | | -| Crypto.go:16:9:16:16 | password | Crypto.go:22:10:22:12 | buf | provenance | | -| Crypto.go:16:9:16:16 | password | Crypto.go:25:16:25:18 | buf | provenance | | -| Crypto.go:16:9:16:16 | password | Crypto.go:28:11:28:13 | buf | provenance | | -nodes -| Crypto.go:16:9:16:16 | password | semmle.label | password | -| Crypto.go:19:25:19:27 | buf | semmle.label | buf | -| Crypto.go:22:10:22:12 | buf | semmle.label | buf | -| Crypto.go:25:16:25:18 | buf | semmle.label | buf | -| Crypto.go:28:11:28:13 | buf | semmle.label | buf | -subpaths -#select -| Crypto.go:19:25:19:27 | buf | Crypto.go:16:9:16:16 | password | Crypto.go:19:25:19:27 | buf | $@ is used in a weak cryptographic algorithm. | Crypto.go:16:9:16:16 | password | Sensitive data | -| Crypto.go:22:10:22:12 | buf | Crypto.go:16:9:16:16 | password | Crypto.go:22:10:22:12 | buf | $@ is used in a weak cryptographic algorithm. | Crypto.go:16:9:16:16 | password | Sensitive data | -| Crypto.go:25:16:25:18 | buf | Crypto.go:16:9:16:16 | password | Crypto.go:25:16:25:18 | buf | $@ is used in a weak cryptographic algorithm. | Crypto.go:16:9:16:16 | password | Sensitive data | -| Crypto.go:28:11:28:13 | buf | Crypto.go:16:9:16:16 | password | Crypto.go:28:11:28:13 | buf | $@ is used in a weak cryptographic algorithm. | Crypto.go:16:9:16:16 | password | Sensitive data | diff --git a/go/ql/test/query-tests/Security/CWE-327/WeakCryptoAlgorithm.qlref b/go/ql/test/query-tests/Security/CWE-327/WeakCryptoAlgorithm.qlref deleted file mode 100644 index 00d68df5a7c5..000000000000 --- a/go/ql/test/query-tests/Security/CWE-327/WeakCryptoAlgorithm.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/CWE-327/WeakCryptoAlgorithm.ql diff --git a/go/ql/test/query-tests/Security/CWE-327/WeakSensitiveDataHashing.expected b/go/ql/test/query-tests/Security/CWE-327/WeakSensitiveDataHashing.expected new file mode 100644 index 000000000000..301658e22cb8 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-327/WeakSensitiveDataHashing.expected @@ -0,0 +1,22 @@ +#select +| hashing.go:20:8:20:22 | secretByteSlice | hashing.go:20:8:20:22 | secretByteSlice | hashing.go:20:8:20:22 | secretByteSlice | $@ is used in a hashing algorithm (MD5) that is insecure. | hashing.go:20:8:20:22 | secretByteSlice | Sensitive data (secret) | +| hashing.go:21:10:21:24 | secretByteSlice | hashing.go:21:10:21:24 | secretByteSlice | hashing.go:21:10:21:24 | secretByteSlice | $@ is used in a hashing algorithm (MD5) that is insecure. | hashing.go:21:10:21:24 | secretByteSlice | Sensitive data (secret) | +| hashing.go:22:20:22:31 | secretString | hashing.go:22:20:22:31 | secretString | hashing.go:22:20:22:31 | secretString | $@ is used in a hashing algorithm (MD5) that is insecure. | hashing.go:22:20:22:31 | secretString | Sensitive data (secret) | +| hashing.go:23:10:23:24 | secretByteSlice | hashing.go:23:10:23:24 | secretByteSlice | hashing.go:23:10:23:24 | secretByteSlice | $@ is used in a hashing algorithm (MD5) that is insecure. | hashing.go:23:10:23:24 | secretByteSlice | Sensitive data (secret) | +| hashing.go:25:17:25:31 | secretByteSlice | hashing.go:25:17:25:31 | secretByteSlice | hashing.go:25:17:25:31 | secretByteSlice | $@ is used in a hashing algorithm (SHA1) that is insecure. | hashing.go:25:17:25:31 | secretByteSlice | Sensitive data (secret) | +| hashing.go:26:11:26:25 | secretByteSlice | hashing.go:26:11:26:25 | secretByteSlice | hashing.go:26:11:26:25 | secretByteSlice | $@ is used in a hashing algorithm (SHA1) that is insecure. | hashing.go:26:11:26:25 | secretByteSlice | Sensitive data (secret) | +| hashing.go:28:16:28:30 | secretByteSlice | hashing.go:28:16:28:30 | secretByteSlice | hashing.go:28:16:28:30 | secretByteSlice | $@ is used in a hashing algorithm (MD4) that is insecure. | hashing.go:28:16:28:30 | secretByteSlice | Sensitive data (secret) | +| hashing.go:29:22:29:36 | secretByteSlice | hashing.go:29:22:29:36 | secretByteSlice | hashing.go:29:22:29:36 | secretByteSlice | $@ is used in a hashing algorithm (RIPEMD160) that is insecure. | hashing.go:29:22:29:36 | secretByteSlice | Sensitive data (secret) | +| hashing.go:80:16:80:23 | password | hashing.go:80:16:80:23 | password | hashing.go:80:16:80:23 | password | $@ is used in a hashing algorithm (SHA256) that is insecure for password hashing, since it is not a computationally expensive hash function. | hashing.go:80:16:80:23 | password | Sensitive data (password) | +edges +nodes +| hashing.go:20:8:20:22 | secretByteSlice | semmle.label | secretByteSlice | +| hashing.go:21:10:21:24 | secretByteSlice | semmle.label | secretByteSlice | +| hashing.go:22:20:22:31 | secretString | semmle.label | secretString | +| hashing.go:23:10:23:24 | secretByteSlice | semmle.label | secretByteSlice | +| hashing.go:25:17:25:31 | secretByteSlice | semmle.label | secretByteSlice | +| hashing.go:26:11:26:25 | secretByteSlice | semmle.label | secretByteSlice | +| hashing.go:28:16:28:30 | secretByteSlice | semmle.label | secretByteSlice | +| hashing.go:29:22:29:36 | secretByteSlice | semmle.label | secretByteSlice | +| hashing.go:80:16:80:23 | password | semmle.label | password | +subpaths diff --git a/go/ql/test/query-tests/Security/CWE-327/WeakSensitiveDataHashing.qlref b/go/ql/test/query-tests/Security/CWE-327/WeakSensitiveDataHashing.qlref new file mode 100644 index 000000000000..ec9ae0993a26 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-327/WeakSensitiveDataHashing.qlref @@ -0,0 +1,4 @@ +query: Security/CWE-327/WeakSensitiveDataHashing.ql +postprocess: + - utils/test/PrettyPrintModels.ql + - utils/test/InlineExpectationsTestQuery.ql diff --git a/go/ql/test/query-tests/Security/CWE-327/encryption.go b/go/ql/test/query-tests/Security/CWE-327/encryption.go new file mode 100644 index 000000000000..2a1bd53440b8 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-327/encryption.go @@ -0,0 +1,167 @@ +package main + +import ( + "bytes" + "crypto/aes" + "crypto/cipher" + "crypto/des" + "crypto/rc4" + "io" + "os" +) + +var dst []byte = make([]byte, 16) +var secretByteSlice []byte = []byte("") + +const secretString string = "" + +var public []byte = []byte("") + +func getUserID() []byte { + return []byte("") +} + +// Note that we do not alert on decryption as we may need to decrypt legacy formats + +func BlockCipherDes() { + // BAD, des is a weak crypto algorithm + block, _ := des.NewCipher(nil) + + block.Encrypt(dst, secretByteSlice) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="DES. init from line 28." + block.Decrypt(dst, secretByteSlice) + + gcm1, _ := cipher.NewGCM(block) + gcm1.Seal(nil, nil, secretByteSlice, nil) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="DES. init from line 28." + gcm1.Open(nil, nil, secretByteSlice, nil) + + gcm2, _ := cipher.NewGCMWithNonceSize(block, 12) + gcm2.Seal(nil, nil, secretByteSlice, nil) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="DES. init from line 28." + gcm2.Open(nil, nil, secretByteSlice, nil) + + gcm3, _ := cipher.NewGCMWithRandomNonce(block) + gcm3.Seal(nil, nil, secretByteSlice, nil) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="DES. init from line 28." + gcm3.Open(nil, nil, secretByteSlice, nil) + + gcm4, _ := cipher.NewGCMWithTagSize(block, 12) + gcm4.Seal(nil, nil, secretByteSlice, nil) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="DES. init from line 28." + gcm4.Open(nil, nil, secretByteSlice, nil) + + cbcEncrypter := cipher.NewCBCEncrypter(block, nil) + cbcEncrypter.CryptBlocks(dst, secretByteSlice) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="DES. blockMode: CBC. init from lines 28,49." + cipher.NewCBCDecrypter(block, nil).CryptBlocks(dst, secretByteSlice) + + ctrStream := cipher.NewCTR(block, nil) + ctrStream.XORKeyStream(dst, secretByteSlice) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="DES. blockMode: CTR. init from lines 28,53." + + ctrStreamReader := &cipher.StreamReader{S: ctrStream, R: bytes.NewReader(secretByteSlice)} // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="DES. blockMode: CTR. init from lines 28,53." + io.Copy(os.Stdout, ctrStreamReader) + + ctrStreamWriter := &cipher.StreamWriter{S: ctrStream, W: os.Stdout} // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="DES. blockMode: CTR. init from lines 28,53." + io.Copy(ctrStreamWriter, bytes.NewReader(secretByteSlice)) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="DES. blockMode: CTR. init from lines 28,53." + + // deprecated + + cfbStream := cipher.NewCFBEncrypter(block, nil) + cfbStream.XORKeyStream(dst, secretByteSlice) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="DES. blockMode: CFB. init from lines 28,64." + cipher.NewCFBDecrypter(block, nil).XORKeyStream(dst, secretByteSlice) + + ofbStream := cipher.NewOFB(block, nil) + ofbStream.XORKeyStream(dst, secretByteSlice) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="DES. blockMode: OFB. init from lines 28,68." +} + +func BlockCipherTripleDes() { + // BAD, triple des is a weak crypto algorithm and secretByteSlice is sensitive data + block, _ := des.NewTripleDESCipher(nil) + + block.Encrypt(dst, getUserID()) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="TRIPLEDES. init from line 74." + block.Decrypt(dst, getUserID()) + + gcm1, _ := cipher.NewGCM(block) + gcm1.Seal(nil, nil, getUserID(), nil) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="TRIPLEDES. init from line 74." + gcm1.Open(nil, nil, getUserID(), nil) + + gcm2, _ := cipher.NewGCMWithNonceSize(block, 12) + gcm2.Seal(nil, nil, getUserID(), nil) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="TRIPLEDES. init from line 74." + gcm2.Open(nil, nil, getUserID(), nil) + + gcm3, _ := cipher.NewGCMWithRandomNonce(block) + gcm3.Seal(nil, nil, secretByteSlice, nil) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="TRIPLEDES. init from line 74." + gcm3.Open(nil, nil, secretByteSlice, nil) + + gcm4, _ := cipher.NewGCMWithTagSize(block, 12) + gcm4.Seal(nil, nil, secretByteSlice, nil) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="TRIPLEDES. init from line 74." + gcm4.Open(nil, nil, secretByteSlice, nil) + + cbcEncrypter := cipher.NewCBCEncrypter(block, nil) + cbcEncrypter.CryptBlocks(dst, getUserID()) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="TRIPLEDES. blockMode: CBC. init from lines 74,95." + cipher.NewCBCDecrypter(block, nil).CryptBlocks(dst, getUserID()) + + ctrStream := cipher.NewCTR(block, nil) + ctrStream.XORKeyStream(dst, getUserID()) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="TRIPLEDES. blockMode: CTR. init from lines 74,99." + + ctrStreamReader := &cipher.StreamReader{S: ctrStream, R: bytes.NewReader(getUserID())} // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="TRIPLEDES. blockMode: CTR. init from lines 74,99." + io.Copy(os.Stdout, ctrStreamReader) + + ctrStreamWriter := &cipher.StreamWriter{S: ctrStream, W: os.Stdout} // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="TRIPLEDES. blockMode: CTR. init from lines 74,99." + io.Copy(ctrStreamWriter, bytes.NewReader(getUserID())) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="TRIPLEDES. blockMode: CTR. init from lines 74,99." + + // deprecated + + cfbStream := cipher.NewCFBEncrypter(block, nil) + cfbStream.XORKeyStream(dst, secretByteSlice) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="TRIPLEDES. blockMode: CFB. init from lines 110,74." + cipher.NewCFBDecrypter(block, nil).XORKeyStream(dst, secretByteSlice) + + ofbStream := cipher.NewOFB(block, nil) + ofbStream.XORKeyStream(dst, secretByteSlice) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="TRIPLEDES. blockMode: OFB. init from lines 114,74." +} + +func BlockCipherAes() { + // GOOD, aes is a strong crypto algorithm + block, _ := aes.NewCipher(nil) + + block.Encrypt(dst, secretByteSlice) // $ CryptographicOperation="AES. init from line 120." + block.Decrypt(dst, secretByteSlice) + + gcm1, _ := cipher.NewGCM(block) + gcm1.Seal(nil, nil, secretByteSlice, nil) // $ CryptographicOperation="AES. init from line 120." + gcm1.Open(nil, nil, secretByteSlice, nil) + + gcm2, _ := cipher.NewGCMWithNonceSize(block, 12) + gcm2.Seal(nil, nil, secretByteSlice, nil) // $ CryptographicOperation="AES. init from line 120." + gcm2.Open(nil, nil, secretByteSlice, nil) + + gcm3, _ := cipher.NewGCMWithRandomNonce(block) + gcm3.Seal(nil, nil, secretByteSlice, nil) // $ CryptographicOperation="AES. init from line 120." + gcm3.Open(nil, nil, secretByteSlice, nil) + + gcm4, _ := cipher.NewGCMWithTagSize(block, 12) + gcm4.Seal(nil, nil, secretByteSlice, nil) // $ CryptographicOperation="AES. init from line 120." + gcm4.Open(nil, nil, secretByteSlice, nil) + + cbcEncrypter := cipher.NewCBCEncrypter(block, nil) + cbcEncrypter.CryptBlocks(dst, secretByteSlice) // $ CryptographicOperation="AES. blockMode: CBC. init from lines 120,141." + cipher.NewCBCDecrypter(block, nil).CryptBlocks(dst, secretByteSlice) + + ctrStream := cipher.NewCTR(block, nil) + ctrStream.XORKeyStream(dst, secretByteSlice) // $ CryptographicOperation="AES. blockMode: CTR. init from lines 120,145." + + ctrStreamReader := &cipher.StreamReader{S: ctrStream, R: bytes.NewReader(secretByteSlice)} // $ CryptographicOperation="AES. blockMode: CTR. init from lines 120,145." + io.Copy(os.Stdout, ctrStreamReader) + + ctrStreamWriter := &cipher.StreamWriter{S: ctrStream, W: os.Stdout} // $ CryptographicOperation="AES. blockMode: CTR. init from lines 120,145." + io.Copy(ctrStreamWriter, bytes.NewReader(secretByteSlice)) // $ CryptographicOperation="AES. blockMode: CTR. init from lines 120,145." + + // deprecated + + cfbStream := cipher.NewCFBEncrypter(block, nil) + cfbStream.XORKeyStream(dst, secretByteSlice) // $ CryptographicOperation="AES. blockMode: CFB. init from lines 120,156." + cipher.NewCFBDecrypter(block, nil).XORKeyStream(dst, secretByteSlice) + + ofbStream := cipher.NewOFB(block, nil) + ofbStream.XORKeyStream(dst, secretByteSlice) // $ CryptographicOperation="AES. blockMode: OFB. init from lines 120,160." +} + +func CipherRc4() { + c, _ := rc4.NewCipher(nil) + c.XORKeyStream(dst, getUserID()) // $ Alert[go/weak-cryptographic-algorithm] CryptographicOperation="RC4. init from line 166." +} diff --git a/go/ql/test/query-tests/Security/CWE-327/go.mod b/go/ql/test/query-tests/Security/CWE-327/go.mod new file mode 100644 index 000000000000..59c69723ef55 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-327/go.mod @@ -0,0 +1,5 @@ +module test + +go 1.24.0 + +require golang.org/x/crypto v0.43.0 diff --git a/go/ql/test/query-tests/Security/CWE-327/hashing.go b/go/ql/test/query-tests/Security/CWE-327/hashing.go new file mode 100644 index 000000000000..4943da353846 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-327/hashing.go @@ -0,0 +1,81 @@ +package main + +//go:generate depstubber -vendor golang.org/x/crypto/md4 "" New +//go:generate depstubber -vendor golang.org/x/crypto/ripemd160 "" New + +import ( + "crypto/md5" + "crypto/sha1" + "crypto/sha256" + "crypto/sha3" + "crypto/sha512" + "io" + + "golang.org/x/crypto/md4" + "golang.org/x/crypto/ripemd160" +) + +func WeakHashes() { + h := md5.New() + h.Sum(secretByteSlice) // $ Alert[go/weak-sensitive-data-hashing] CryptographicOperation="MD5. init from line 19." + h.Write(secretByteSlice) // $ Alert[go/weak-sensitive-data-hashing] CryptographicOperation="MD5. init from line 19." + io.WriteString(h, secretString) // $ Alert[go/weak-sensitive-data-hashing] CryptographicOperation="MD5. init from line 19." + md5.Sum(secretByteSlice) // $ Alert[go/weak-sensitive-data-hashing] CryptographicOperation="MD5. init from line 23." + + sha1.New().Sum(secretByteSlice) // $ Alert[go/weak-sensitive-data-hashing] CryptographicOperation="SHA1. init from line 25." + sha1.Sum(secretByteSlice) // $ Alert[go/weak-sensitive-data-hashing] CryptographicOperation="SHA1. init from line 26." + + md4.New().Sum(secretByteSlice) // $ Alert[go/weak-sensitive-data-hashing] CryptographicOperation="MD4. init from line 28." + ripemd160.New().Sum(secretByteSlice) // $ Alert[go/weak-sensitive-data-hashing] CryptographicOperation="RIPEMD160. init from line 29." + + // Only alert when sensitive data is hashed. + md5.New().Sum(public) // $ CryptographicOperation="MD5. init from line 32." + md5.Sum(public) // $ CryptographicOperation="MD5. init from line 33." + sha1.New().Sum(public) // $ CryptographicOperation="SHA1. init from line 34." + sha1.Sum(public) // $ CryptographicOperation="SHA1. init from line 35." +} + +func StrongHashes() { + sha256.New224().Sum(secretByteSlice) // $ CryptographicOperation="SHA224. init from line 39." + sha256.Sum224(secretByteSlice) // $ CryptographicOperation="SHA224. init from line 40." + + sha256.New().Sum(secretByteSlice) // $ CryptographicOperation="SHA256. init from line 42." + sha256.Sum256(secretByteSlice) // $ CryptographicOperation="SHA256. init from line 43." + + sha512.New().Sum(secretByteSlice) // $ CryptographicOperation="SHA512. init from line 45." + sha512.Sum512(secretByteSlice) // $ CryptographicOperation="SHA512. init from line 46." + + sha512.New384().Sum(secretByteSlice) // $ CryptographicOperation="SHA384. init from line 48." + sha512.Sum384(secretByteSlice) // $ CryptographicOperation="SHA384. init from line 49." + + sha512.New512_224().Sum(secretByteSlice) // $ CryptographicOperation="SHA512224. init from line 51." + sha512.Sum512_224(secretByteSlice) // $ CryptographicOperation="SHA512224. init from line 52." + + sha512.New512_256().Sum(secretByteSlice) // $ CryptographicOperation="SHA512256. init from line 54." + sha512.Sum512_256(secretByteSlice) // $ CryptographicOperation="SHA512256. init from line 55." + + sha3.New224().Sum(secretByteSlice) // $ CryptographicOperation="SHA3224. init from line 57." + sha3.Sum224(secretByteSlice) // $ CryptographicOperation="SHA3224. init from line 58." + + sha3.New256().Sum(secretByteSlice) // $ CryptographicOperation="SHA3256. init from line 60." + sha3.Sum256(secretByteSlice) // $ CryptographicOperation="SHA3256. init from line 61." + + sha3.New384().Sum(secretByteSlice) // $ CryptographicOperation="SHA3384. init from line 63." + sha3.Sum384(secretByteSlice) // $ CryptographicOperation="SHA3384. init from line 64." + + sha3.New512().Sum(secretByteSlice) // $ CryptographicOperation="SHA3512. init from line 66." + sha3.Sum512(secretByteSlice) // $ CryptographicOperation="SHA3512. init from line 67." + + sha3.NewSHAKE128().Write(secretByteSlice) // $ CryptographicOperation="SHAKE128. init from line 69." + sha3.NewCSHAKE128(nil, nil).Write(secretByteSlice) // $ CryptographicOperation="SHAKE128. init from line 70." + sha3.SumSHAKE128(secretByteSlice, 100) // $ CryptographicOperation="SHAKE128. init from line 71." + + sha3.NewSHAKE256().Write(secretByteSlice) // $ CryptographicOperation="SHAKE256. init from line 73." + sha3.NewCSHAKE256(nil, nil).Write(secretByteSlice) // $ CryptographicOperation="SHAKE256. init from line 74." + sha3.SumSHAKE256(secretByteSlice, 100) // $ CryptographicOperation="SHAKE256. init from line 75." +} + +func PasswordHashing() { + password := []byte("") + sha256.Sum256(password) // $ Alert[go/weak-sensitive-data-hashing] CryptographicOperation="SHA256. init from line 80." +} diff --git a/go/ql/test/query-tests/Security/CWE-327/vendor/golang.org/x/crypto/md4/stub.go b/go/ql/test/query-tests/Security/CWE-327/vendor/golang.org/x/crypto/md4/stub.go new file mode 100644 index 000000000000..777cc50c0c1c --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-327/vendor/golang.org/x/crypto/md4/stub.go @@ -0,0 +1,16 @@ +// Code generated by depstubber. DO NOT EDIT. +// This is a simple stub for golang.org/x/crypto/md4, strictly for use in testing. + +// See the LICENSE file for information about the licensing of the original library. +// Source: golang.org/x/crypto/md4 (exports: ; functions: New) + +// Package md4 is a stub of golang.org/x/crypto/md4, generated by depstubber. +package md4 + +import ( + hash "hash" +) + +func New() hash.Hash { + return nil +} diff --git a/go/ql/test/query-tests/Security/CWE-327/vendor/golang.org/x/crypto/ripemd160/stub.go b/go/ql/test/query-tests/Security/CWE-327/vendor/golang.org/x/crypto/ripemd160/stub.go new file mode 100644 index 000000000000..995d3339d8cd --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-327/vendor/golang.org/x/crypto/ripemd160/stub.go @@ -0,0 +1,16 @@ +// Code generated by depstubber. DO NOT EDIT. +// This is a simple stub for golang.org/x/crypto/ripemd160, strictly for use in testing. + +// See the LICENSE file for information about the licensing of the original library. +// Source: golang.org/x/crypto/ripemd160 (exports: ; functions: New) + +// Package ripemd160 is a stub of golang.org/x/crypto/ripemd160, generated by depstubber. +package ripemd160 + +import ( + hash "hash" +) + +func New() hash.Hash { + return nil +} diff --git a/go/ql/test/query-tests/Security/CWE-327/vendor/modules.txt b/go/ql/test/query-tests/Security/CWE-327/vendor/modules.txt new file mode 100644 index 000000000000..1927762821ca --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-327/vendor/modules.txt @@ -0,0 +1,4 @@ +# golang.org/x/crypto v0.43.0 +## explicit +golang.org/x/crypto/md4 +golang.org/x/crypto/ripemd160 diff --git a/ruby/ql/src/queries/security/cwe-327/WeakSensitiveDataHashing.qhelp b/ruby/ql/src/queries/security/cwe-327/WeakSensitiveDataHashing.qhelp index 422cbb835141..33d0c845a66d 100644 --- a/ruby/ql/src/queries/security/cwe-327/WeakSensitiveDataHashing.qhelp +++ b/ruby/ql/src/queries/security/cwe-327/WeakSensitiveDataHashing.qhelp @@ -67,7 +67,7 @@ The 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 first function uses SHA-1 that is known to be vulnerable to collision attacks. The second function uses SHA-256 that is a strong cryptographic hashing function.

    diff --git a/ruby/ql/test/library-tests/security/CryptoAlgorithms.expected b/ruby/ql/test/library-tests/security/CryptoAlgorithms.expected index eedddb2df9ff..3f1ba8e12c67 100644 --- a/ruby/ql/test/library-tests/security/CryptoAlgorithms.expected +++ b/ruby/ql/test/library-tests/security/CryptoAlgorithms.expected @@ -34,6 +34,8 @@ strongHashingAlgorithms | SHA3256 | | SHA3384 | | SHA3512 | +| SHA512224 | +| SHA512256 | | SHAKE128 | | SHAKE256 | | SM3 | diff --git a/shared/concepts/codeql/concepts/internal/CryptoAlgorithmNames.qll b/shared/concepts/codeql/concepts/internal/CryptoAlgorithmNames.qll index efcd870c724a..86392890cafb 100644 --- a/shared/concepts/codeql/concepts/internal/CryptoAlgorithmNames.qll +++ b/shared/concepts/codeql/concepts/internal/CryptoAlgorithmNames.qll @@ -23,7 +23,8 @@ predicate isStrongHashingAlgorithm(string name) { "BLAKE3", // "DSA", "ED25519", "ES256", "ECDSA256", "ES384", "ECDSA384", "ES512", "ECDSA512", "SHA2", - "SHA224", "SHA256", "SHA384", "SHA512", "SHA3", "SHA3224", "SHA3256", "SHA3384", "SHA3512", + "SHA224", "SHA256", "SHA384", "SHA512", "SHA512224", "SHA512256", "SHA3", "SHA3224", + "SHA3256", "SHA3384", "SHA3512", // see https://cryptography.io/en/latest/hazmat/primitives/cryptographic-hashes/#cryptography.hazmat.primitives.hashes.SHAKE128 "SHAKE128", "SHAKE256", // see https://cryptography.io/en/latest/hazmat/primitives/cryptographic-hashes/#sm3