diff --git a/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll b/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll index e5425b2fb88..db527c03f95 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll @@ -51,6 +51,7 @@ private module AsmCrypto { private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode { DataFlow::Node input; CryptographicAlgorithm algorithm; // non-functional + DataFlow::PropRead algorithmSelection; private string algorithmName; private string methodName; @@ -68,11 +69,14 @@ private module AsmCrypto { exists(DataFlow::SourceNode asmCrypto | asmCrypto = DataFlow::globalVarRef("asmCrypto") and algorithm.matchesName(algorithmName) and - this = asmCrypto.getAPropertyRead(algorithmName).getAMemberCall(methodName) and + algorithmSelection = asmCrypto.getAPropertyRead(algorithmName) and + this = algorithmSelection.getAMemberCall(methodName) and input = this.getArgument(0) ) } + override DataFlow::Node getInitialization() { result = algorithmSelection } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } @@ -103,6 +107,7 @@ private module BrowserIdCrypto { private class Apply extends CryptographicOperation::Range instanceof DataFlow::MethodCallNode { CryptographicAlgorithm algorithm; // non-functional + DataFlow::CallNode keygen; Apply() { /* @@ -122,8 +127,7 @@ private module BrowserIdCrypto { */ exists( - DataFlow::SourceNode mod, DataFlow::Node algorithmNameNode, DataFlow::CallNode keygen, - DataFlow::FunctionNode callback + DataFlow::SourceNode mod, DataFlow::Node algorithmNameNode, DataFlow::FunctionNode callback | mod = DataFlow::moduleImport("browserid-crypto") and keygen = mod.getAMemberCall("generateKeypair") and @@ -134,6 +138,8 @@ private module BrowserIdCrypto { ) } + override DataFlow::Node getInitialization() { result = keygen } + override DataFlow::Node getAnInput() { result = super.getArgument(0) } override CryptographicAlgorithm getAlgorithm() { result = algorithm } @@ -239,6 +245,8 @@ private module NodeJSCrypto { Apply() { this = instantiation.getAMethodCall(any(string m | m = "update" or m = "write")) } + override DataFlow::Node getInitialization() { result = instantiation } + override DataFlow::Node getAnInput() { result = super.getArgument(0) } override CryptographicAlgorithm getAlgorithm() { result = instantiation.getAlgorithm() } @@ -324,7 +332,9 @@ private module CryptoJS { ) } - private API::CallNode getEncryptionApplication(API::Node input, CryptographicAlgorithm algorithm) { + private API::CallNode getEncryptionApplication( + API::Node input, API::Node algorithmNode, CryptographicAlgorithm algorithm + ) { /* * ``` * var CryptoJS = require("crypto-js"); @@ -338,11 +348,14 @@ private module CryptoJS { * Also matches where `CryptoJS.` has been replaced by `require("crypto-js/")` */ - result = getAlgorithmNode(algorithm).getMember("encrypt").getACall() and + algorithmNode = getAlgorithmNode(algorithm) and + result = algorithmNode.getMember("encrypt").getACall() and input = result.getParameter(0) } - private API::CallNode getDirectApplication(API::Node input, CryptographicAlgorithm algorithm) { + private API::CallNode getDirectApplication( + API::Node input, API::Node algorithmNode, CryptographicAlgorithm algorithm + ) { /* * ``` * var CryptoJS = require("crypto-js"); @@ -357,7 +370,8 @@ private module CryptoJS { * Also matches where `CryptoJS.` has been replaced by `require("crypto-js/")` */ - result = getAlgorithmNode(algorithm).getACall() and + algorithmNode = getAlgorithmNode(algorithm) and + result = algorithmNode.getACall() and input = result.getParameter(0) } @@ -389,18 +403,23 @@ private module CryptoJS { private class Apply extends CryptographicOperation::Range instanceof API::CallNode { API::Node input; CryptographicAlgorithm algorithm; // non-functional + DataFlow::Node instantiation; Apply() { - this = getEncryptionApplication(input, algorithm) - or - this = getDirectApplication(input, algorithm) - or - exists(InstantiatedAlgorithm instantiation | - this = getUpdatedApplication(input, instantiation) and - algorithm = instantiation.getAlgorithm() + exists(API::Node algorithmNode | + this = getEncryptionApplication(input, algorithmNode, algorithm) + or + this = getDirectApplication(input, algorithmNode, algorithm) + | + instantiation = algorithmNode.asSource() ) + or + this = getUpdatedApplication(input, instantiation) and + algorithm = instantiation.(InstantiatedAlgorithm).getAlgorithm() } + override DataFlow::Node getInitialization() { result = instantiation } + override DataFlow::Node getAnInput() { result = input.asSink() } override CryptographicAlgorithm getAlgorithm() { result = algorithm } @@ -504,6 +523,8 @@ private module TweetNaCl { ) } + override DataFlow::Node getInitialization() { result = this } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } @@ -539,6 +560,7 @@ private module HashJs { private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode { DataFlow::Node input; CryptographicAlgorithm algorithm; // non-functional + DataFlow::CallNode init; Apply() { /* @@ -554,10 +576,13 @@ private module HashJs { * Also matches where `hash.()` has been replaced by a more specific require a la `require("hash.js/lib/hash/sha/512")` */ - this = getAlgorithmNode(algorithm).getAMemberCall("update") and + init = getAlgorithmNode(algorithm) and + this = init.getAMemberCall("update") and input = super.getArgument(0) } + override DataFlow::Node getInitialization() { result = init } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } @@ -653,6 +678,8 @@ private module Forge { algorithm = cipher.getAlgorithm() } + override DataFlow::Node getInitialization() { result = cipher } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } @@ -715,6 +742,8 @@ private module Md5 { super.getArgument(0) = input } + override DataFlow::Node getInitialization() { result = this } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } @@ -731,17 +760,18 @@ private module Bcrypt { private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode { DataFlow::Node input; CryptographicAlgorithm algorithm; + API::Node init; Apply() { // `require("bcrypt").hash(password);` with minor naming variations algorithm.matchesName("BCRYPT") and - this = - API::moduleImport(["bcrypt", "bcryptjs", "bcrypt-nodejs"]) - .getMember(["hash", "hashSync"]) - .getACall() and + init = API::moduleImport(["bcrypt", "bcryptjs", "bcrypt-nodejs"]) and + this = init.getMember(["hash", "hashSync"]).getACall() and super.getArgument(0) = input } + override DataFlow::Node getInitialization() { result = init.asSource() } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } @@ -769,6 +799,8 @@ private module Hasha { ) } + override DataFlow::Node getInitialization() { result = this } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } diff --git a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll index e86b156e204..135f830e47d 100644 --- a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll +++ b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll @@ -40,6 +40,9 @@ module Cryptography { /** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */ CryptographicAlgorithm getAlgorithm() { result = super.getAlgorithm() } + /** Gets the data-flow node where the cryptographic algorithm used in this operation is configured. */ + DataFlow::Node getInitialization() { result = super.getInitialization() } + /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ DataFlow::Node getAnInput() { result = super.getAnInput() } @@ -65,6 +68,9 @@ module Cryptography { * extend `CryptographicOperation` instead. */ abstract class Range extends DataFlow::Node { + /** Gets the data-flow node where the cryptographic algorithm used in this operation is configured. */ + abstract DataFlow::Node getInitialization(); + /** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */ abstract CryptographicAlgorithm getAlgorithm(); diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/BrokenCryptoAlgorithmCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/BrokenCryptoAlgorithmCustomizations.qll index 38d048da84e..e0db75ab52c 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/BrokenCryptoAlgorithmCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/BrokenCryptoAlgorithmCustomizations.qll @@ -19,7 +19,10 @@ module BrokenCryptoAlgorithm { /** * A data flow sink for sensitive information in broken or weak cryptographic algorithms. */ - abstract class Sink extends DataFlow::Node { } + abstract class Sink extends DataFlow::Node { + /** Gets the data-flow node where the cryptographic algorithm used in this operation is configured. */ + abstract DataFlow::Node getInitialization(); + } /** * A sanitizer for sensitive information in broken or weak cryptographic algorithms. @@ -38,15 +41,17 @@ module BrokenCryptoAlgorithm { * An expression used by a broken or weak cryptographic algorithm. */ class WeakCryptographicOperationSink extends Sink { + CryptographicOperation application; + WeakCryptographicOperationSink() { - exists(CryptographicOperation application | - ( - application.getAlgorithm().isWeak() - or - application.getBlockMode().isWeak() - ) and - this = application.getAnInput() - ) + ( + application.getAlgorithm().isWeak() + or + application.getBlockMode().isWeak() + ) and + this = application.getAnInput() } + + override DataFlow::Node getInitialization() { result = application.getInitialization() } } } diff --git a/javascript/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql b/javascript/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql index 9826ebefe5f..a4dd7ed6372 100644 --- a/javascript/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql +++ b/javascript/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql @@ -16,9 +16,14 @@ import semmle.javascript.security.dataflow.BrokenCryptoAlgorithmQuery import semmle.javascript.security.SensitiveActions import DataFlow::PathGraph -from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink +from + Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Source sourceNode, + Sink sinkNode where cfg.hasFlowPath(source, sink) and - not source.getNode() instanceof CleartextPasswordExpr // flagged by js/insufficient-password-hash -select sink.getNode(), source, sink, "A broken or weak cryptographic algorithm depends on $@.", - source.getNode(), "sensitive data from " + source.getNode().(Source).describe() + sourceNode = source.getNode() and + sinkNode = sink.getNode() and + not sourceNode instanceof CleartextPasswordExpr // flagged by js/insufficient-password-hash +select sinkNode, source, sink, "$@ depends on $@.", sinkNode.getInitialization(), + "A broken or weak cryptographic algorithm", sourceNode, + "sensitive data from " + sourceNode.describe() diff --git a/javascript/ql/test/query-tests/Security/CWE-327/BrokenCryptoAlgorithm.expected b/javascript/ql/test/query-tests/Security/CWE-327/BrokenCryptoAlgorithm.expected index 1938b020355..3b87a7ccd9c 100644 --- a/javascript/ql/test/query-tests/Security/CWE-327/BrokenCryptoAlgorithm.expected +++ b/javascript/ql/test/query-tests/Security/CWE-327/BrokenCryptoAlgorithm.expected @@ -26,8 +26,8 @@ edges | tst.js:19:17:19:24 | password | tst.js:19:17:19:24 | password | | tst.js:22:21:22:30 | secretText | tst.js:22:21:22:30 | secretText | #select -| tst.js:11:17:11:26 | secretText | tst.js:3:18:3:24 | trusted | tst.js:11:17:11:26 | secretText | A broken or weak cryptographic algorithm depends on $@. | tst.js:3:18:3:24 | trusted | sensitive data from an access to trusted | -| tst.js:11:17:11:26 | secretText | tst.js:11:17:11:26 | secretText | tst.js:11:17:11:26 | secretText | A broken or weak cryptographic algorithm depends on $@. | tst.js:11:17:11:26 | secretText | sensitive data from an access to secretText | -| tst.js:17:17:17:25 | o.trusted | tst.js:17:17:17:25 | o.trusted | tst.js:17:17:17:25 | o.trusted | A broken or weak cryptographic algorithm depends on $@. | tst.js:17:17:17:25 | o.trusted | sensitive data from an access to trusted | -| tst.js:22:21:22:30 | secretText | tst.js:3:18:3:24 | trusted | tst.js:22:21:22:30 | secretText | A broken or weak cryptographic algorithm depends on $@. | tst.js:3:18:3:24 | trusted | sensitive data from an access to trusted | -| tst.js:22:21:22:30 | secretText | tst.js:22:21:22:30 | secretText | tst.js:22:21:22:30 | secretText | A broken or weak cryptographic algorithm depends on $@. | tst.js:22:21:22:30 | secretText | sensitive data from an access to secretText | +| tst.js:11:17:11:26 | secretText | tst.js:3:18:3:24 | trusted | tst.js:11:17:11:26 | secretText | $@ depends on $@. | tst.js:5:19:5:49 | crypto. ... ', key) | A broken or weak cryptographic algorithm | tst.js:3:18:3:24 | trusted | sensitive data from an access to trusted | +| tst.js:11:17:11:26 | secretText | tst.js:11:17:11:26 | secretText | tst.js:11:17:11:26 | secretText | $@ depends on $@. | tst.js:5:19:5:49 | crypto. ... ', key) | A broken or weak cryptographic algorithm | tst.js:11:17:11:26 | secretText | sensitive data from an access to secretText | +| tst.js:17:17:17:25 | o.trusted | tst.js:17:17:17:25 | o.trusted | tst.js:17:17:17:25 | o.trusted | $@ depends on $@. | tst.js:5:19:5:49 | crypto. ... ', key) | A broken or weak cryptographic algorithm | tst.js:17:17:17:25 | o.trusted | sensitive data from an access to trusted | +| tst.js:22:21:22:30 | secretText | tst.js:3:18:3:24 | trusted | tst.js:22:21:22:30 | secretText | $@ depends on $@. | tst.js:21:22:21:60 | crypto. ... ', key) | A broken or weak cryptographic algorithm | tst.js:3:18:3:24 | trusted | sensitive data from an access to trusted | +| tst.js:22:21:22:30 | secretText | tst.js:22:21:22:30 | secretText | tst.js:22:21:22:30 | secretText | $@ depends on $@. | tst.js:21:22:21:60 | crypto. ... ', key) | A broken or weak cryptographic algorithm | tst.js:22:21:22:30 | secretText | sensitive data from an access to secretText | diff --git a/python/ql/lib/semmle/python/frameworks/Cryptodome.qll b/python/ql/lib/semmle/python/frameworks/Cryptodome.qll index 1adca253271..f0a309644ad 100644 --- a/python/ql/lib/semmle/python/frameworks/Cryptodome.qll +++ b/python/ql/lib/semmle/python/frameworks/Cryptodome.qll @@ -128,6 +128,8 @@ private module CryptodomeModel { this = newCall.getReturn().getMember(methodName).getACall() } + override DataFlow::Node getInitialization() { result = newCall } + override Cryptography::CryptographicAlgorithm getAlgorithm() { result.matchesName(cipherName) } override DataFlow::Node getAnInput() { @@ -181,21 +183,23 @@ private module CryptodomeModel { class CryptodomeGenericSignatureOperation extends Cryptography::CryptographicOperation::Range, DataFlow::CallCfgNode { + API::CallNode newCall; string methodName; string signatureName; CryptodomeGenericSignatureOperation() { methodName in ["sign", "verify"] and - this = + newCall = API::moduleImport(["Crypto", "Cryptodome"]) .getMember("Signature") .getMember(signatureName) .getMember("new") - .getReturn() - .getMember(methodName) - .getACall() + .getACall() and + this = newCall.getReturn().getMember(methodName).getACall() } + override DataFlow::Node getInitialization() { result = newCall } + override Cryptography::CryptographicAlgorithm getAlgorithm() { result.matchesName(signatureName) } @@ -221,19 +225,23 @@ private module CryptodomeModel { class CryptodomeGenericHashOperation extends Cryptography::CryptographicOperation::Range, DataFlow::CallCfgNode { + API::CallNode newCall; string hashName; CryptodomeGenericHashOperation() { exists(API::Node hashModule | hashModule = - API::moduleImport(["Crypto", "Cryptodome"]).getMember("Hash").getMember(hashName) + API::moduleImport(["Crypto", "Cryptodome"]).getMember("Hash").getMember(hashName) and + newCall = hashModule.getMember("new").getACall() | - this = hashModule.getMember("new").getACall() + this = newCall or - this = hashModule.getMember("new").getReturn().getMember("update").getACall() + this = newCall.getReturn().getMember("update").getACall() ) } + override DataFlow::Node getInitialization() { result = newCall } + override Cryptography::CryptographicAlgorithm getAlgorithm() { result.matchesName(hashName) } override DataFlow::Node getAnInput() { result in [this.getArg(0), this.getArgByName("data")] } diff --git a/python/ql/lib/semmle/python/frameworks/Cryptography.qll b/python/ql/lib/semmle/python/frameworks/Cryptography.qll index 07d80aad7f5..41d69d064d7 100644 --- a/python/ql/lib/semmle/python/frameworks/Cryptography.qll +++ b/python/ql/lib/semmle/python/frameworks/Cryptography.qll @@ -209,18 +209,18 @@ private module CryptographyModel { class CryptographyGenericCipherOperation extends Cryptography::CryptographicOperation::Range, DataFlow::MethodCallNode { + API::CallNode init; string algorithmName; string modeName; CryptographyGenericCipherOperation() { - this = - cipherInstance(algorithmName, modeName) - .getMember(["decryptor", "encryptor"]) - .getReturn() - .getMember(["update", "update_into"]) - .getACall() + init = + cipherInstance(algorithmName, modeName).getMember(["decryptor", "encryptor"]).getACall() and + this = init.getReturn().getMember(["update", "update_into"]).getACall() } + override DataFlow::Node getInitialization() { result = init } + override Cryptography::CryptographicAlgorithm getAlgorithm() { result.matchesName(algorithmName) } @@ -247,19 +247,17 @@ private module CryptographyModel { } /** Gets a reference to a Hash instance using algorithm with `algorithmName`. */ - private API::Node hashInstance(string algorithmName) { - exists(API::CallNode call | result = call.getReturn() | - call = - API::moduleImport("cryptography") - .getMember("hazmat") - .getMember("primitives") - .getMember("hashes") - .getMember("Hash") - .getACall() and - algorithmClassRef(algorithmName).getReturn().getAValueReachableFromSource() in [ - call.getArg(0), call.getArgByName("algorithm") - ] - ) + private API::CallNode hashInstance(string algorithmName) { + result = + API::moduleImport("cryptography") + .getMember("hazmat") + .getMember("primitives") + .getMember("hashes") + .getMember("Hash") + .getACall() and + algorithmClassRef(algorithmName).getReturn().getAValueReachableFromSource() in [ + result.getArg(0), result.getArgByName("algorithm") + ] } /** @@ -268,12 +266,16 @@ private module CryptographyModel { class CryptographyGenericHashOperation extends Cryptography::CryptographicOperation::Range, DataFlow::MethodCallNode { + API::CallNode init; string algorithmName; CryptographyGenericHashOperation() { - this = hashInstance(algorithmName).getMember("update").getACall() + init = hashInstance(algorithmName) and + this = init.getReturn().getMember("update").getACall() } + override DataFlow::Node getInitialization() { result = init } + override Cryptography::CryptographicAlgorithm getAlgorithm() { result.matchesName(algorithmName) } diff --git a/python/ql/lib/semmle/python/frameworks/Rsa.qll b/python/ql/lib/semmle/python/frameworks/Rsa.qll index 1c582eaa799..4b7142177f9 100644 --- a/python/ql/lib/semmle/python/frameworks/Rsa.qll +++ b/python/ql/lib/semmle/python/frameworks/Rsa.qll @@ -37,6 +37,8 @@ private module Rsa { class RsaEncryptCall extends Cryptography::CryptographicOperation::Range, DataFlow::CallCfgNode { RsaEncryptCall() { this = API::moduleImport("rsa").getMember("encrypt").getACall() } + override DataFlow::Node getInitialization() { result = this } + override Cryptography::CryptographicAlgorithm getAlgorithm() { result.getName() = "RSA" } override DataFlow::Node getAnInput() { @@ -54,6 +56,8 @@ private module Rsa { class RsaDecryptCall extends Cryptography::CryptographicOperation::Range, DataFlow::CallCfgNode { RsaDecryptCall() { this = API::moduleImport("rsa").getMember("decrypt").getACall() } + override DataFlow::Node getInitialization() { result = this } + override Cryptography::CryptographicAlgorithm getAlgorithm() { result.getName() = "RSA" } override DataFlow::Node getAnInput() { result in [this.getArg(0), this.getArgByName("crypto")] } @@ -69,6 +73,8 @@ private module Rsa { class RsaSignCall extends Cryptography::CryptographicOperation::Range, DataFlow::CallCfgNode { RsaSignCall() { this = API::moduleImport("rsa").getMember("sign").getACall() } + override DataFlow::Node getInitialization() { result = this } + override Cryptography::CryptographicAlgorithm getAlgorithm() { // signature part result.getName() = "RSA" @@ -96,6 +102,8 @@ private module Rsa { class RsaVerifyCall extends Cryptography::CryptographicOperation::Range, DataFlow::CallCfgNode { RsaVerifyCall() { this = API::moduleImport("rsa").getMember("verify").getACall() } + override DataFlow::Node getInitialization() { result = this } + override Cryptography::CryptographicAlgorithm getAlgorithm() { // note that technically there is also a hashing operation going on but we don't // know what algorithm is used up front, since it is encoded in the signature @@ -121,6 +129,8 @@ private module Rsa { { RsaComputeHashCall() { this = API::moduleImport("rsa").getMember("compute_hash").getACall() } + override DataFlow::Node getInitialization() { result = this } + override Cryptography::CryptographicAlgorithm getAlgorithm() { exists(StrConst str, DataFlow::Node hashNameArg | hashNameArg in [this.getArg(1), this.getArgByName("method_name")] and @@ -144,6 +154,8 @@ private module Rsa { class RsaSignHashCall extends Cryptography::CryptographicOperation::Range, DataFlow::CallCfgNode { RsaSignHashCall() { this = API::moduleImport("rsa").getMember("sign_hash").getACall() } + override DataFlow::Node getInitialization() { result = this } + override Cryptography::CryptographicAlgorithm getAlgorithm() { result.getName() = "RSA" } override DataFlow::Node getAnInput() { diff --git a/python/ql/lib/semmle/python/frameworks/Stdlib.qll b/python/ql/lib/semmle/python/frameworks/Stdlib.qll index ec79a6dfddf..2c8c8bbb6f0 100644 --- a/python/ql/lib/semmle/python/frameworks/Stdlib.qll +++ b/python/ql/lib/semmle/python/frameworks/Stdlib.qll @@ -2747,6 +2747,8 @@ private module StdlibPrivate { exists(this.getParameter(1, "data")) } + override DataFlow::Node getInitialization() { result = this } + override Cryptography::CryptographicAlgorithm getAlgorithm() { result.matchesName(hashName) } override DataFlow::Node getAnInput() { result = this.getParameter(1, "data").asSink() } @@ -2758,12 +2760,16 @@ private module StdlibPrivate { * A hashing operation by using the `update` method on the result of calling the `hashlib.new` function. */ class HashlibNewUpdateCall extends Cryptography::CryptographicOperation::Range, API::CallNode { + API::CallNode init; string hashName; HashlibNewUpdateCall() { - this = hashlibNewCall(hashName).getReturn().getMember("update").getACall() + init = hashlibNewCall(hashName) and + this = init.getReturn().getMember("update").getACall() } + override DataFlow::Node getInitialization() { result = init } + override Cryptography::CryptographicAlgorithm getAlgorithm() { result.matchesName(hashName) } override DataFlow::Node getAnInput() { result = this.getArg(0) } @@ -2802,7 +2808,14 @@ private module StdlibPrivate { * (such as `hashlib.md5`), by calling its' `update` method. */ class HashlibHashClassUpdateCall extends HashlibGenericHashOperation { - HashlibHashClassUpdateCall() { this = hashClass.getReturn().getMember("update").getACall() } + API::CallNode init; + + HashlibHashClassUpdateCall() { + init = hashClass.getACall() and + this = hashClass.getReturn().getMember("update").getACall() + } + + override DataFlow::Node getInitialization() { result = init } override DataFlow::Node getAnInput() { result = this.getArg(0) } } @@ -2819,6 +2832,8 @@ private module StdlibPrivate { exists([this.getArg(0), this.getArgByName("string")]) } + override DataFlow::Node getInitialization() { result = this } + override DataFlow::Node getAnInput() { result = this.getArg(0) or @@ -2865,6 +2880,8 @@ private module StdlibPrivate { exists(this.getParameter(1, "msg").asSink()) } + override DataFlow::Node getInitialization() { result = this } + override API::Node getDigestArg() { result = digestArg } override DataFlow::Node getAnInput() { result = this.getParameter(1, "msg").asSink() } @@ -2876,12 +2893,16 @@ private module StdlibPrivate { * See https://docs.python.org/3.11/library/hmac.html#hmac.HMAC.update */ class HmacUpdateCall extends HmacCryptographicOperation { + API::CallNode init; API::Node digestArg; HmacUpdateCall() { - this = getHmacConstructorCall(digestArg).getReturn().getMember("update").getACall() + init = getHmacConstructorCall(digestArg) and + this = init.getReturn().getMember("update").getACall() } + override DataFlow::Node getInitialization() { result = init } + override API::Node getDigestArg() { result = digestArg } override DataFlow::Node getAnInput() { result = this.getParameter(0, "msg").asSink() } @@ -2895,6 +2916,8 @@ private module StdlibPrivate { class HmacDigestCall extends HmacCryptographicOperation { HmacDigestCall() { this = API::moduleImport("hmac").getMember("digest").getACall() } + override DataFlow::Node getInitialization() { result = this } + override API::Node getDigestArg() { result = this.getParameter(2, "digest") } override DataFlow::Node getAnInput() { result = this.getParameter(1, "msg").asSink() } diff --git a/python/ql/lib/semmle/python/internal/ConceptsShared.qll b/python/ql/lib/semmle/python/internal/ConceptsShared.qll index e86b156e204..135f830e47d 100644 --- a/python/ql/lib/semmle/python/internal/ConceptsShared.qll +++ b/python/ql/lib/semmle/python/internal/ConceptsShared.qll @@ -40,6 +40,9 @@ module Cryptography { /** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */ CryptographicAlgorithm getAlgorithm() { result = super.getAlgorithm() } + /** Gets the data-flow node where the cryptographic algorithm used in this operation is configured. */ + DataFlow::Node getInitialization() { result = super.getInitialization() } + /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ DataFlow::Node getAnInput() { result = super.getAnInput() } @@ -65,6 +68,9 @@ module Cryptography { * extend `CryptographicOperation` instead. */ abstract class Range extends DataFlow::Node { + /** Gets the data-flow node where the cryptographic algorithm used in this operation is configured. */ + abstract DataFlow::Node getInitialization(); + /** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */ abstract CryptographicAlgorithm getAlgorithm(); diff --git a/python/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql b/python/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql index 9b741c3d76b..9da2124e882 100644 --- a/python/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql +++ b/python/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql @@ -13,18 +13,15 @@ import python import semmle.python.Concepts -from - Cryptography::CryptographicOperation operation, Cryptography::CryptographicAlgorithm algorithm, - string msgPrefix +from Cryptography::CryptographicOperation operation, string msgPrefix where - algorithm = operation.getAlgorithm() and // `Cryptography::HashingAlgorithm` and `Cryptography::PasswordHashingAlgorithm` are // handled by `py/weak-sensitive-data-hashing` - algorithm instanceof Cryptography::EncryptionAlgorithm and - ( + exists(Cryptography::EncryptionAlgorithm algorithm | algorithm = operation.getAlgorithm() | algorithm.isWeak() and - msgPrefix = "The cryptographic algorithm " + operation.getAlgorithm().getName() + msgPrefix = "The cryptographic algorithm " + algorithm.getName() ) or operation.getBlockMode().isWeak() and msgPrefix = "The block mode " + operation.getBlockMode() -select operation, msgPrefix + " is broken or weak, and should not be used." +select operation, "$@ is broken or weak, and should not be used.", operation.getInitialization(), + msgPrefix diff --git a/python/ql/test/query-tests/Security/CWE-327-BrokenCryptoAlgorithm/BrokenCryptoAlgorithm.expected b/python/ql/test/query-tests/Security/CWE-327-BrokenCryptoAlgorithm/BrokenCryptoAlgorithm.expected index 4d6ee66ed47..43c6eeb0f77 100644 --- a/python/ql/test/query-tests/Security/CWE-327-BrokenCryptoAlgorithm/BrokenCryptoAlgorithm.expected +++ b/python/ql/test/query-tests/Security/CWE-327-BrokenCryptoAlgorithm/BrokenCryptoAlgorithm.expected @@ -1,4 +1,4 @@ -| test_cryptodome.py:11:13:11:42 | ControlFlowNode for Attribute() | The cryptographic algorithm ARC4 is broken or weak, and should not be used. | -| test_cryptodome.py:16:13:16:42 | ControlFlowNode for Attribute() | The block mode ECB is broken or weak, and should not be used. | -| test_cryptography.py:13:13:13:44 | ControlFlowNode for Attribute() | The cryptographic algorithm ARC4 is broken or weak, and should not be used. | -| test_cryptography.py:22:13:22:58 | ControlFlowNode for Attribute() | The block mode ECB is broken or weak, and should not be used. | +| test_cryptodome.py:11:13:11:42 | ControlFlowNode for Attribute() | $@ is broken or weak, and should not be used. | test_cryptodome.py:10:10:10:22 | ControlFlowNode for Attribute() | The cryptographic algorithm ARC4 | +| test_cryptodome.py:16:13:16:42 | ControlFlowNode for Attribute() | $@ is broken or weak, and should not be used. | test_cryptodome.py:15:10:15:35 | ControlFlowNode for Attribute() | The block mode ECB | +| test_cryptography.py:13:13:13:44 | ControlFlowNode for Attribute() | $@ is broken or weak, and should not be used. | test_cryptography.py:12:13:12:30 | ControlFlowNode for Attribute() | The cryptographic algorithm ARC4 | +| test_cryptography.py:22:13:22:58 | ControlFlowNode for Attribute() | $@ is broken or weak, and should not be used. | test_cryptography.py:21:13:21:30 | ControlFlowNode for Attribute() | The block mode ECB | diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Digest.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Digest.qll index 895ab066d39..52df3feed04 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/core/Digest.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Digest.qll @@ -18,15 +18,21 @@ private API::Node digest(Cryptography::HashingAlgorithm algo) { private class DigestCall extends Cryptography::CryptographicOperation::Range instanceof DataFlow::CallNode { Cryptography::HashingAlgorithm algo; + API::Node digestNode; DigestCall() { - this = digest(algo).getAMethodCall(["hexdigest", "base64digest", "bubblebabble"]) - or - this = digest(algo).getAMethodCall("file") // it's directly hashing the contents of a file, but that's close enough for us. - or - this = digest(algo).getInstance().getAMethodCall(["digest", "update", "<<"]) + digestNode = digest(algo) and + ( + this = digestNode.getAMethodCall(["hexdigest", "base64digest", "bubblebabble"]) + or + this = digestNode.getAMethodCall("file") // it's directly hashing the contents of a file, but that's close enough for us. + or + this = digestNode.getInstance().getAMethodCall(["digest", "update", "<<"]) + ) } + override DataFlow::Node getInitialization() { result = digestNode.asSource() } + override Cryptography::HashingAlgorithm getAlgorithm() { result = algo } override DataFlow::Node getAnInput() { result = super.getArgument(0) } diff --git a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll index e86b156e204..135f830e47d 100644 --- a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll +++ b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll @@ -40,6 +40,9 @@ module Cryptography { /** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */ CryptographicAlgorithm getAlgorithm() { result = super.getAlgorithm() } + /** Gets the data-flow node where the cryptographic algorithm used in this operation is configured. */ + DataFlow::Node getInitialization() { result = super.getInitialization() } + /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ DataFlow::Node getAnInput() { result = super.getAnInput() } @@ -65,6 +68,9 @@ module Cryptography { * extend `CryptographicOperation` instead. */ abstract class Range extends DataFlow::Node { + /** Gets the data-flow node where the cryptographic algorithm used in this operation is configured. */ + abstract DataFlow::Node getInitialization(); + /** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */ abstract CryptographicAlgorithm getAlgorithm(); diff --git a/ruby/ql/lib/codeql/ruby/security/OpenSSL.qll b/ruby/ql/lib/codeql/ruby/security/OpenSSL.qll index 637f7dab04a..cbabdc8782a 100644 --- a/ruby/ql/lib/codeql/ruby/security/OpenSSL.qll +++ b/ruby/ql/lib/codeql/ruby/security/OpenSSL.qll @@ -569,6 +569,8 @@ private class CipherOperation extends Cryptography::CryptographicOperation::Rang this.getMethodName() = "update" } + override DataFlow::Node getInitialization() { result = cipherNode } + override Cryptography::EncryptionAlgorithm getAlgorithm() { result = cipherNode.getCipher().getAlgorithm() } @@ -591,21 +593,21 @@ private module Digest { private class DigestCall extends Cryptography::CryptographicOperation::Range instanceof DataFlow::CallNode { Cryptography::HashingAlgorithm algo; + API::MethodAccessNode call; DigestCall() { - exists(API::MethodAccessNode call | - call = API::getTopLevelMember("OpenSSL").getMember("Digest").getMethod("new") - | - this = call.getReturn().getAMethodCall(["digest", "update", "<<"]) and - algo.matchesName(call.asCall() - .getArgument(0) - .asExpr() - .getExpr() - .getConstantValue() - .getString()) - ) + call = API::getTopLevelMember("OpenSSL").getMember("Digest").getMethod("new") and + this = call.getReturn().getAMethodCall(["digest", "update", "<<"]) and + algo.matchesName(call.asCall() + .getArgument(0) + .asExpr() + .getExpr() + .getConstantValue() + .getString()) } + override DataFlow::Node getInitialization() { result = call.asCall() } + override Cryptography::HashingAlgorithm getAlgorithm() { result = algo } override DataFlow::Node getAnInput() { result = super.getArgument(0) } @@ -617,12 +619,16 @@ private module Digest { private class DigestCallDirect extends Cryptography::CryptographicOperation::Range instanceof DataFlow::CallNode { Cryptography::HashingAlgorithm algo; + API::Node digestNode; DigestCallDirect() { - this = API::getTopLevelMember("OpenSSL").getMember("Digest").getMethod("digest").asCall() and + digestNode = API::getTopLevelMember("OpenSSL").getMember("Digest") and + this = digestNode.getMethod("digest").asCall() and algo.matchesName(this.getArgument(0).asExpr().getExpr().getConstantValue().getString()) } + override DataFlow::Node getInitialization() { result = digestNode.asSource() } + override Cryptography::HashingAlgorithm getAlgorithm() { result = algo } override DataFlow::Node getAnInput() { result = super.getArgument(1) } diff --git a/ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.ql b/ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.ql index b4082c669aa..b8e6400dd42 100644 --- a/ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.ql +++ b/ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.ql @@ -23,4 +23,5 @@ where ) or operation.getBlockMode().isWeak() and msgPrefix = "The block mode " + operation.getBlockMode() -select operation, msgPrefix + " is broken or weak, and should not be used." +select operation, "$@ is broken or weak, and should not be used.", operation.getInitialization(), + msgPrefix diff --git a/ruby/ql/test/query-tests/security/cwe-327/BrokenCryptoAlgorithm.expected b/ruby/ql/test/query-tests/security/cwe-327/BrokenCryptoAlgorithm.expected index 62f621fd8c4..fc7b5b1ba67 100644 --- a/ruby/ql/test/query-tests/security/cwe-327/BrokenCryptoAlgorithm.expected +++ b/ruby/ql/test/query-tests/security/cwe-327/BrokenCryptoAlgorithm.expected @@ -1,19 +1,19 @@ -| broken_crypto.rb:4:8:4:34 | call to new | The cryptographic algorithm DES is broken or weak, and should not be used. | -| broken_crypto.rb:8:1:8:18 | call to update | The cryptographic algorithm DES is broken or weak, and should not be used. | -| broken_crypto.rb:12:8:12:43 | call to new | The block mode ECB is broken or weak, and should not be used. | -| broken_crypto.rb:16:1:16:18 | call to update | The block mode ECB is broken or weak, and should not be used. | -| broken_crypto.rb:28:1:28:35 | call to new | The block mode ECB is broken or weak, and should not be used. | -| broken_crypto.rb:37:1:37:33 | call to new | The block mode ECB is broken or weak, and should not be used. | -| broken_crypto.rb:42:1:42:33 | call to new | The block mode ECB is broken or weak, and should not be used. | -| broken_crypto.rb:47:1:47:33 | call to new | The block mode ECB is broken or weak, and should not be used. | -| broken_crypto.rb:52:1:52:29 | call to new | The block mode ECB is broken or weak, and should not be used. | -| broken_crypto.rb:57:1:57:32 | call to new | The block mode ECB is broken or weak, and should not be used. | -| broken_crypto.rb:60:1:60:24 | call to new | The cryptographic algorithm DES is broken or weak, and should not be used. | -| broken_crypto.rb:62:1:62:30 | call to new | The cryptographic algorithm DES is broken or weak, and should not be used. | -| broken_crypto.rb:67:1:67:31 | call to new | The block mode ECB is broken or weak, and should not be used. | -| broken_crypto.rb:70:1:70:24 | call to new | The cryptographic algorithm RC2 is broken or weak, and should not be used. | -| broken_crypto.rb:72:1:72:30 | call to new | The block mode ECB is broken or weak, and should not be used. | -| broken_crypto.rb:72:1:72:30 | call to new | The cryptographic algorithm RC2 is broken or weak, and should not be used. | -| broken_crypto.rb:75:1:75:24 | call to new | The cryptographic algorithm RC4 is broken or weak, and should not be used. | -| broken_crypto.rb:77:1:77:29 | call to new | The cryptographic algorithm RC4 is broken or weak, and should not be used. | -| broken_crypto.rb:79:1:79:35 | call to new | The cryptographic algorithm RC4 is broken or weak, and should not be used. | +| broken_crypto.rb:4:8:4:34 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:4:8:4:34 | call to new | The cryptographic algorithm DES | +| broken_crypto.rb:8:1:8:18 | call to update | $@ is broken or weak, and should not be used. | broken_crypto.rb:8:1:8:4 | weak | The cryptographic algorithm DES | +| broken_crypto.rb:12:8:12:43 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:12:8:12:43 | call to new | The block mode ECB | +| broken_crypto.rb:16:1:16:18 | call to update | $@ is broken or weak, and should not be used. | broken_crypto.rb:16:1:16:4 | weak | The block mode ECB | +| broken_crypto.rb:28:1:28:35 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:28:1:28:35 | call to new | The block mode ECB | +| broken_crypto.rb:37:1:37:33 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:37:1:37:33 | call to new | The block mode ECB | +| broken_crypto.rb:42:1:42:33 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:42:1:42:33 | call to new | The block mode ECB | +| broken_crypto.rb:47:1:47:33 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:47:1:47:33 | call to new | The block mode ECB | +| broken_crypto.rb:52:1:52:29 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:52:1:52:29 | call to new | The block mode ECB | +| broken_crypto.rb:57:1:57:32 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:57:1:57:32 | call to new | The block mode ECB | +| broken_crypto.rb:60:1:60:24 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:60:1:60:24 | call to new | The cryptographic algorithm DES | +| broken_crypto.rb:62:1:62:30 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:62:1:62:30 | call to new | The cryptographic algorithm DES | +| broken_crypto.rb:67:1:67:31 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:67:1:67:31 | call to new | The block mode ECB | +| broken_crypto.rb:70:1:70:24 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:70:1:70:24 | call to new | The cryptographic algorithm RC2 | +| broken_crypto.rb:72:1:72:30 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:72:1:72:30 | call to new | The block mode ECB | +| broken_crypto.rb:72:1:72:30 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:72:1:72:30 | call to new | The cryptographic algorithm RC2 | +| broken_crypto.rb:75:1:75:24 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:75:1:75:24 | call to new | The cryptographic algorithm RC4 | +| broken_crypto.rb:77:1:77:29 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:77:1:77:29 | call to new | The cryptographic algorithm RC4 | +| broken_crypto.rb:79:1:79:35 | call to new | $@ is broken or weak, and should not be used. | broken_crypto.rb:79:1:79:35 | call to new | The cryptographic algorithm RC4 |