From 61095b3c5895700bca5223413a50076dc6ec1300 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 2 Feb 2023 20:27:05 +0000 Subject: [PATCH 01/20] ConceptsShared: Add deprecated DataFlow::Node CryptographicOperation#getInput() predicate --- .../ql/lib/semmle/javascript/internal/ConceptsShared.qll | 6 ++++++ python/ql/lib/semmle/python/internal/ConceptsShared.qll | 6 ++++++ ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll | 6 ++++++ 3 files changed, 18 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll index 2f6c8bb8b29b..5be626877ccd 100644 --- a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll +++ b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll @@ -43,6 +43,9 @@ module Cryptography { /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ DataFlow::Node getAnInput() { result = super.getAnInput() } + /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ + deprecated final DataFlow::Node getInput() { result = super.getInput() } + /** * Gets the block mode used to perform this cryptographic operation. * This may have no result - for example if the `CryptographicAlgorithm` used @@ -67,6 +70,9 @@ module Cryptography { /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ abstract DataFlow::Node getAnInput(); + /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ + deprecated final DataFlow::Node getInput() { result = this.getAnInput() } + /** * Gets the block mode used to perform this cryptographic operation. * This may have no result - for example if the `CryptographicAlgorithm` used diff --git a/python/ql/lib/semmle/python/internal/ConceptsShared.qll b/python/ql/lib/semmle/python/internal/ConceptsShared.qll index 2f6c8bb8b29b..5be626877ccd 100644 --- a/python/ql/lib/semmle/python/internal/ConceptsShared.qll +++ b/python/ql/lib/semmle/python/internal/ConceptsShared.qll @@ -43,6 +43,9 @@ module Cryptography { /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ DataFlow::Node getAnInput() { result = super.getAnInput() } + /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ + deprecated final DataFlow::Node getInput() { result = super.getInput() } + /** * Gets the block mode used to perform this cryptographic operation. * This may have no result - for example if the `CryptographicAlgorithm` used @@ -67,6 +70,9 @@ module Cryptography { /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ abstract DataFlow::Node getAnInput(); + /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ + deprecated final DataFlow::Node getInput() { result = this.getAnInput() } + /** * Gets the block mode used to perform this cryptographic operation. * This may have no result - for example if the `CryptographicAlgorithm` used diff --git a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll index 2f6c8bb8b29b..5be626877ccd 100644 --- a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll +++ b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll @@ -43,6 +43,9 @@ module Cryptography { /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ DataFlow::Node getAnInput() { result = super.getAnInput() } + /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ + deprecated final DataFlow::Node getInput() { result = super.getInput() } + /** * Gets the block mode used to perform this cryptographic operation. * This may have no result - for example if the `CryptographicAlgorithm` used @@ -67,6 +70,9 @@ module Cryptography { /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ abstract DataFlow::Node getAnInput(); + /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ + deprecated final DataFlow::Node getInput() { result = this.getAnInput() } + /** * Gets the block mode used to perform this cryptographic operation. * This may have no result - for example if the `CryptographicAlgorithm` used From e5dfbe2c8ddd8620e0c70c51715e15e8e1d13f6c Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 2 Feb 2023 20:27:52 +0000 Subject: [PATCH 02/20] ConceptsShared: Add BlockMode#matchesString(string) predicate --- .../ql/lib/semmle/javascript/internal/ConceptsShared.qll | 4 ++++ python/ql/lib/semmle/python/internal/ConceptsShared.qll | 4 ++++ ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll index 5be626877ccd..23b345928520 100644 --- a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll +++ b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll @@ -91,6 +91,10 @@ module Cryptography { /** Holds if this block mode is considered to be insecure. */ predicate isWeak() { this = "ECB" } + + /** Holds if the given string appears to match this block mode. */ + bindingset[s] + predicate matchesString(string s) { s.toUpperCase().matches("%" + this + "%") } } } diff --git a/python/ql/lib/semmle/python/internal/ConceptsShared.qll b/python/ql/lib/semmle/python/internal/ConceptsShared.qll index 5be626877ccd..23b345928520 100644 --- a/python/ql/lib/semmle/python/internal/ConceptsShared.qll +++ b/python/ql/lib/semmle/python/internal/ConceptsShared.qll @@ -91,6 +91,10 @@ module Cryptography { /** Holds if this block mode is considered to be insecure. */ predicate isWeak() { this = "ECB" } + + /** Holds if the given string appears to match this block mode. */ + bindingset[s] + predicate matchesString(string s) { s.toUpperCase().matches("%" + this + "%") } } } diff --git a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll index 5be626877ccd..23b345928520 100644 --- a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll +++ b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll @@ -91,6 +91,10 @@ module Cryptography { /** Holds if this block mode is considered to be insecure. */ predicate isWeak() { this = "ECB" } + + /** Holds if the given string appears to match this block mode. */ + bindingset[s] + predicate matchesString(string s) { s.toUpperCase().matches("%" + this + "%") } } } From 983055b8f9628181ddb19882df6c60ea5f3544bb Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 2 Feb 2023 20:28:37 +0000 Subject: [PATCH 03/20] JS: Use shared CryptographicOperation concept and implement BlockMode getBlockMode() --- .../EndpointCharacteristics.qll | 2 +- .../ql/lib/semmle/javascript/Concepts.qll | 7 + .../javascript/frameworks/CryptoLibraries.qll | 158 ++++++++++++------ .../BrokenCryptoAlgorithmCustomizations.qll | 2 +- ...InsufficientPasswordHashCustomizations.qll | 2 +- .../CryptoLibraries/CryptographicOperation.ql | 2 +- 6 files changed, 121 insertions(+), 52 deletions(-) diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll index e95b2785ceb1..fe3286032ad8 100644 --- a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll +++ b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll @@ -387,7 +387,7 @@ private class CryptographicOperationFlowCharacteristic extends NotASinkCharacter CryptographicOperationFlowCharacteristic() { this = "CryptographicOperationFlow" } override predicate appliesToEndpoint(DataFlow::Node n) { - any(CryptographicOperation op).getInput() = n + any(CryptographicOperation op).getAnInput() = n } } diff --git a/javascript/ql/lib/semmle/javascript/Concepts.qll b/javascript/ql/lib/semmle/javascript/Concepts.qll index e3c3f0d23571..a760e746030f 100644 --- a/javascript/ql/lib/semmle/javascript/Concepts.qll +++ b/javascript/ql/lib/semmle/javascript/Concepts.qll @@ -110,3 +110,10 @@ abstract class PersistentWriteAccess extends DataFlow::Node { */ abstract DataFlow::Node getValue(); } + +/** + * Provides models for cryptographic things. + */ +module Cryptography { + import semmle.javascript.internal.ConceptsShared::Cryptography +} diff --git a/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll b/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll index 9cf4dcfaacea..8d2921e48cb9 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll @@ -3,22 +3,7 @@ */ import javascript -import semmle.javascript.security.CryptoAlgorithms - -/** - * 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 DataFlow::Node getInput(); - - /** - * Gets the applied algorithm. - */ - abstract CryptographicAlgorithm getAlgorithm(); -} +import semmle.javascript.Concepts::Cryptography /** * A key used in a cryptographic algorithm. @@ -52,13 +37,20 @@ class CryptographicKeyCredentialsExpr extends CredentialsNode instanceof Cryptog override string getCredentialsKind() { result = "key" } } +// Holds if `algorithm` is an `EncryptionAlgorithm` that uses a block cipher +private predicate isBlockEncryptionAlgorithm(CryptographicAlgorithm algorithm) { + algorithm instanceof EncryptionAlgorithm and + not algorithm.(EncryptionAlgorithm).isStreamCipher() +} + /** * A model of the asmCrypto library. */ private module AsmCrypto { - private class Apply extends CryptographicOperation instanceof DataFlow::CallNode { + private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode { DataFlow::Node input; CryptographicAlgorithm algorithm; // non-functional + private string algorithmName; Apply() { /* @@ -71,17 +63,22 @@ private module AsmCrypto { * ``` */ - exists(DataFlow::SourceNode asmCrypto, string algorithmName | + exists(DataFlow::SourceNode asmCrypto | asmCrypto = DataFlow::globalVarRef("asmCrypto") and algorithm.matchesName(algorithmName) and this = asmCrypto.getAPropertyRead(algorithmName).getAMemberCall(_) and - input = this.getAnArgument() + input = this.getArgument(0) ) } - override DataFlow::Node getInput() { result = input } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } + + override BlockMode getBlockMode() { + isBlockEncryptionAlgorithm(this.getAlgorithm()) and + result.matchesString(algorithmName) + } } } @@ -93,7 +90,7 @@ private module BrowserIdCrypto { Key() { this = any(Apply apply).getKey() } } - private class Apply extends CryptographicOperation instanceof DataFlow::MethodCallNode { + private class Apply extends CryptographicOperation::Range instanceof DataFlow::MethodCallNode { CryptographicAlgorithm algorithm; // non-functional Apply() { @@ -126,10 +123,13 @@ private module BrowserIdCrypto { ) } - override DataFlow::Node getInput() { result = super.getArgument(0) } + override DataFlow::Node getAnInput() { result = super.getArgument(0) } override CryptographicAlgorithm getAlgorithm() { result = algorithm } + // not relevant for browserid-crypto + override BlockMode getBlockMode() { none() } + DataFlow::Node getKey() { result = super.getArgument(1) } } } @@ -140,6 +140,7 @@ private module BrowserIdCrypto { private module NodeJSCrypto { private class InstantiatedAlgorithm extends DataFlow::CallNode { CryptographicAlgorithm algorithm; // non-functional + private string algorithmName; InstantiatedAlgorithm() { /* @@ -158,11 +159,25 @@ private module NodeJSCrypto { exists(DataFlow::SourceNode mod | mod = DataFlow::moduleImport("crypto") and this = mod.getAMemberCall("create" + ["Hash", "Hmac", "Sign", "Cipher"]) and - algorithm.matchesName(this.getArgument(0).getStringValue()) + algorithmName = this.getArgument(0).getStringValue() and + algorithm.matchesName(algorithmName) ) } CryptographicAlgorithm getAlgorithm() { result = algorithm } + + private BlockMode getExplicitBlockMode() { result.matchesString(algorithmName) } + + BlockMode getBlockMode() { + isBlockEncryptionAlgorithm(this.getAlgorithm()) and + ( + if exists(this.getExplicitBlockMode()) + then result = this.getExplicitBlockMode() + else + // CBC is the default if not explicitly specified + result = "CBC" + ) + } } private class CreateKey extends CryptographicKeyCreation, DataFlow::CallNode { @@ -211,14 +226,16 @@ private module NodeJSCrypto { override predicate isSymmetricKey() { none() } } - private class Apply extends CryptographicOperation instanceof DataFlow::MethodCallNode { + private class Apply extends CryptographicOperation::Range instanceof DataFlow::MethodCallNode { InstantiatedAlgorithm instantiation; Apply() { this = instantiation.getAMethodCall(any(string m | m = "update" or m = "write")) } - override DataFlow::Node getInput() { result = super.getArgument(0) } + override DataFlow::Node getAnInput() { result = super.getArgument(0) } override CryptographicAlgorithm getAlgorithm() { result = instantiation.getAlgorithm() } + + override BlockMode getBlockMode() { result = instantiation.getBlockMode() } } private class Key extends CryptographicKey { @@ -307,7 +324,7 @@ private module CryptoJS { input = result.getArgument(0) } - private class Apply extends CryptographicOperation { + private class Apply extends CryptographicOperation::Range, DataFlow::CallNode { DataFlow::Node input; CryptographicAlgorithm algorithm; // non-functional @@ -316,9 +333,31 @@ private module CryptoJS { this = getDirectApplication(input, algorithm) } - override DataFlow::Node getInput() { result = input } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } + + // e.g. CryptoJS.AES.encrypt("msg", "key", { mode: CryptoJS.mode. }) + private BlockMode getExplicitBlockMode() { + exists(DataFlow::ObjectLiteralNode o, DataFlow::SourceNode modeNode, string modeString | + modeNode = API::moduleImport("crypto-js").getMember("mode").getMember(modeString).asSource() and + o.flowsTo(this.getArgument(2)) and + modeNode = o.getAPropertySource("mode") + | + result.matchesString(modeString) + ) + } + + override BlockMode getBlockMode() { + isBlockEncryptionAlgorithm(this.getAlgorithm()) and + ( + if exists(this.getExplicitBlockMode()) + then result = this.getExplicitBlockMode() + else + // CBC is the default if not explicitly specified + result = "CBC" + ) + } } private class Key extends CryptographicKey { @@ -374,7 +413,7 @@ private module CryptoJS { * A model of the TweetNaCl library. */ private module TweetNaCl { - private class Apply extends CryptographicOperation instanceof DataFlow::CallNode { + private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode { DataFlow::Node input; CryptographicAlgorithm algorithm; @@ -401,9 +440,12 @@ private module TweetNaCl { ) } - override DataFlow::Node getInput() { result = input } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } + + // No block ciphers implemented + override BlockMode getBlockMode() { none() } } } @@ -434,7 +476,7 @@ private module HashJs { ) } - private class Apply extends CryptographicOperation instanceof DataFlow::CallNode { + private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode { DataFlow::Node input; CryptographicAlgorithm algorithm; // non-functional @@ -456,9 +498,12 @@ private module HashJs { input = super.getArgument(0) } - override DataFlow::Node getInput() { result = input } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } + + // not relevant for hash.js + override BlockMode getBlockMode() { none() } } } @@ -478,19 +523,20 @@ private module Forge { private class KeyCipher extends Cipher { DataFlow::Node key; CryptographicAlgorithm algorithm; // non-functional + private string blockModeString; KeyCipher() { exists(DataFlow::SourceNode mod, string algorithmName | mod = getAnImportNode() and algorithm.matchesName(algorithmName) | - exists(string createName, string cipherName, string cipherPrefix, string cipherSuffix | + exists(string createName, string cipherName, string cipherPrefix | // `require('forge').cipher.createCipher("3DES-CBC").update("secret", "key");` (createName = "createCipher" or createName = "createDecipher") and this = mod.getAPropertyRead("cipher").getAMemberCall(createName) and this.getArgument(0).mayHaveStringValue(cipherName) and - cipherName = cipherPrefix + "-" + cipherSuffix and - cipherSuffix = ["CBC", "CFB", "CTR", "ECB", "GCM", "OFB"] and + cipherName = cipherPrefix + "-" + blockModeString and + blockModeString = ["CBC", "CFB", "CTR", "ECB", "GCM", "OFB"] and algorithmName = cipherPrefix and key = this.getArgument(1) ) @@ -500,7 +546,8 @@ private module Forge { createName = "createEncryptionCipher" or createName = "createDecryptionCipher" | this = mod.getAPropertyRead(algorithmName).getAMemberCall(createName) and - key = this.getArgument(0) + key = this.getArgument(0) and + blockModeString = algorithmName ) ) } @@ -508,6 +555,11 @@ private module Forge { override CryptographicAlgorithm getAlgorithm() { result = algorithm } DataFlow::Node getKey() { result = key } + + BlockMode getBlockMode() { + isBlockEncryptionAlgorithm(this.getAlgorithm()) and + result.matchesString(blockModeString) + } } private class NonKeyCipher extends Cipher { @@ -527,21 +579,22 @@ private module Forge { override CryptographicAlgorithm getAlgorithm() { result = algorithm } } - private class Apply extends CryptographicOperation instanceof DataFlow::CallNode { + private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode { DataFlow::Node input; CryptographicAlgorithm algorithm; // non-functional + private Cipher cipher; Apply() { - exists(Cipher cipher | - this = cipher.getAMemberCall("update") and - super.getArgument(0) = input and - algorithm = cipher.getAlgorithm() - ) + this = cipher.getAMemberCall("update") and + super.getArgument(0) = input and + algorithm = cipher.getAlgorithm() } - override DataFlow::Node getInput() { result = input } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } + + override BlockMode getBlockMode() { result = cipher.(KeyCipher).getBlockMode() } } private class Key extends CryptographicKey { @@ -586,7 +639,7 @@ private module Forge { * A model of the md5 library. */ private module Md5 { - private class Apply extends CryptographicOperation instanceof DataFlow::CallNode { + private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode { DataFlow::Node input; CryptographicAlgorithm algorithm; @@ -600,9 +653,12 @@ private module Md5 { ) } - override DataFlow::Node getInput() { result = input } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } + + // not relevant for md5 + override BlockMode getBlockMode() { none() } } } @@ -610,7 +666,7 @@ private module Md5 { * A model of the bcrypt, bcryptjs, bcrypt-nodejs libraries. */ private module Bcrypt { - private class Apply extends CryptographicOperation instanceof DataFlow::CallNode { + private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode { DataFlow::Node input; CryptographicAlgorithm algorithm; @@ -633,9 +689,12 @@ private module Bcrypt { ) } - override DataFlow::Node getInput() { result = input } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } + + // not relevant for bcrypt + override BlockMode getBlockMode() { none() } } } @@ -643,7 +702,7 @@ private module Bcrypt { * A model of the hasha library. */ private module Hasha { - private class Apply extends CryptographicOperation instanceof DataFlow::CallNode { + private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode { DataFlow::Node input; CryptographicAlgorithm algorithm; @@ -659,9 +718,12 @@ private module Hasha { ) } - override DataFlow::Node getInput() { result = input } + override DataFlow::Node getAnInput() { result = input } override CryptographicAlgorithm getAlgorithm() { result = algorithm } + + // not relevant for hasha + override BlockMode getBlockMode() { none() } } } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/BrokenCryptoAlgorithmCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/BrokenCryptoAlgorithmCustomizations.qll index 832f811f67b9..01a5b1b260b1 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/BrokenCryptoAlgorithmCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/BrokenCryptoAlgorithmCustomizations.qll @@ -41,7 +41,7 @@ module BrokenCryptoAlgorithm { WeakCryptographicOperationSink() { exists(CryptographicOperation application | application.getAlgorithm().isWeak() and - this = application.getInput() + this = application.getAnInput() ) } } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/InsufficientPasswordHashCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/InsufficientPasswordHashCustomizations.qll index 1697d55fe0b1..8901be9962f8 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/InsufficientPasswordHashCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/InsufficientPasswordHashCustomizations.qll @@ -47,7 +47,7 @@ module InsufficientPasswordHash { application.getAlgorithm().isWeak() or not application.getAlgorithm() instanceof PasswordHashingAlgorithm | - this = application.getInput() + this = application.getAnInput() ) } } diff --git a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.ql b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.ql index d085113b9c70..10e5ae851f45 100644 --- a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.ql +++ b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.ql @@ -1,4 +1,4 @@ import javascript from CryptographicOperation operation -select operation, operation.getAlgorithm().getName(), operation.getInput() +select operation, operation.getAlgorithm().getName(), operation.getAnInput() From 1435ef186293ff4296931405cc9e50cc53aab905 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 2 Feb 2023 20:20:03 +0000 Subject: [PATCH 04/20] CryptoAlgorithms: make CryptographicAlgorithm#matchesName split on underscores --- .../ql/lib/semmle/javascript/security/CryptoAlgorithms.qll | 4 ++-- python/ql/lib/semmle/python/concepts/CryptoAlgorithms.qll | 4 ++-- ruby/ql/lib/codeql/ruby/security/CryptoAlgorithms.qll | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/CryptoAlgorithms.qll b/javascript/ql/lib/semmle/javascript/security/CryptoAlgorithms.qll index 22a2d1c1eb29..766f99c61da6 100644 --- a/javascript/ql/lib/semmle/javascript/security/CryptoAlgorithms.qll +++ b/javascript/ql/lib/semmle/javascript/security/CryptoAlgorithms.qll @@ -40,12 +40,12 @@ abstract class CryptographicAlgorithm extends TCryptographicAlgorithm { /** * Holds if the name of this algorithm matches `name` modulo case, - * white space, dashes, underscores, and anything after a dash in the name + * white space, dashes, underscores, and anything after a dash or underscore in the name * (to ignore modes of operation, such as CBC or ECB). */ bindingset[name] predicate matchesName(string name) { - [name.toUpperCase(), name.toUpperCase().regexpCapture("^(\\w+)(?:-.*)?$", 1)] + [name.toUpperCase(), name.toUpperCase().regexpCapture("^([A-Z0-9]+)(?:(-|_).*)?$", 1)] .regexpReplaceAll("[-_ ]", "") = getName() } diff --git a/python/ql/lib/semmle/python/concepts/CryptoAlgorithms.qll b/python/ql/lib/semmle/python/concepts/CryptoAlgorithms.qll index 22a2d1c1eb29..766f99c61da6 100644 --- a/python/ql/lib/semmle/python/concepts/CryptoAlgorithms.qll +++ b/python/ql/lib/semmle/python/concepts/CryptoAlgorithms.qll @@ -40,12 +40,12 @@ abstract class CryptographicAlgorithm extends TCryptographicAlgorithm { /** * Holds if the name of this algorithm matches `name` modulo case, - * white space, dashes, underscores, and anything after a dash in the name + * white space, dashes, underscores, and anything after a dash or underscore in the name * (to ignore modes of operation, such as CBC or ECB). */ bindingset[name] predicate matchesName(string name) { - [name.toUpperCase(), name.toUpperCase().regexpCapture("^(\\w+)(?:-.*)?$", 1)] + [name.toUpperCase(), name.toUpperCase().regexpCapture("^([A-Z0-9]+)(?:(-|_).*)?$", 1)] .regexpReplaceAll("[-_ ]", "") = getName() } diff --git a/ruby/ql/lib/codeql/ruby/security/CryptoAlgorithms.qll b/ruby/ql/lib/codeql/ruby/security/CryptoAlgorithms.qll index 22a2d1c1eb29..766f99c61da6 100644 --- a/ruby/ql/lib/codeql/ruby/security/CryptoAlgorithms.qll +++ b/ruby/ql/lib/codeql/ruby/security/CryptoAlgorithms.qll @@ -40,12 +40,12 @@ abstract class CryptographicAlgorithm extends TCryptographicAlgorithm { /** * Holds if the name of this algorithm matches `name` modulo case, - * white space, dashes, underscores, and anything after a dash in the name + * white space, dashes, underscores, and anything after a dash or underscore in the name * (to ignore modes of operation, such as CBC or ECB). */ bindingset[name] predicate matchesName(string name) { - [name.toUpperCase(), name.toUpperCase().regexpCapture("^(\\w+)(?:-.*)?$", 1)] + [name.toUpperCase(), name.toUpperCase().regexpCapture("^([A-Z0-9]+)(?:(-|_).*)?$", 1)] .regexpReplaceAll("[-_ ]", "") = getName() } From c25dc978df7cb6a74ac4175940ea853803fd9ef3 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 2 Feb 2023 16:16:44 +0000 Subject: [PATCH 05/20] JS: add blockMode to CryptographicOperation tests --- .../CryptographicOperation.expected | 62 +++++++++---------- .../CryptoLibraries/CryptographicOperation.ql | 14 ++++- 2 files changed, 44 insertions(+), 32 deletions(-) diff --git a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.expected b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.expected index 09134c235bc0..6408535c0f4d 100644 --- a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.expected +++ b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.expected @@ -1,31 +1,31 @@ -| tst.js:1:1:1:27 | asmCryp ... (input) | SHA256 | tst.js:1:22:1:26 | input | -| tst.js:5:5:5:43 | jwcrypt ... retKey) | DSA | tst.js:5:19:5:23 | input | -| tst.js:10:18:10:55 | cipher. ... 'hex') | AES192 | tst.js:10:32:10:39 | 'input1' | -| tst.js:11:18:11:54 | cipher. ... 'hex') | AES192 | tst.js:11:31:11:38 | 'input2' | -| tst.js:15:1:15:21 | hash.up ... nput1') | SHA256 | tst.js:15:13:15:20 | 'input1' | -| tst.js:16:1:16:20 | hash.write('input2') | SHA256 | tst.js:16:12:16:19 | 'input2' | -| tst.js:20:1:20:21 | hmac.up ... nput1') | SHA256 | tst.js:20:13:20:20 | 'input1' | -| tst.js:21:1:21:20 | hmac.write('input2') | SHA256 | tst.js:21:12:21:19 | 'input2' | -| tst.js:25:1:25:21 | sign.up ... nput1') | SHA256 | tst.js:25:13:25:20 | 'input1' | -| tst.js:26:1:26:20 | sign.write('input2') | SHA256 | tst.js:26:12:26:19 | 'input2' | -| tst.js:29:1:29:52 | CryptoJ ... y 123') | AES | tst.js:29:22:29:33 | 'my message' | -| tst.js:32:1:32:31 | CryptoJ ... "Key") | SHA1 | tst.js:32:15:32:23 | "Message" | -| tst.js:35:1:35:35 | CryptoJ ... "Key") | SHA1 | tst.js:35:19:35:27 | "Message" | -| tst.js:37:1:37:64 | require ... y 123') | AES | tst.js:37:34:37:45 | 'my message' | -| tst.js:39:1:39:43 | require ... "Key") | SHA1 | tst.js:39:27:39:35 | "Message" | -| tst.js:41:1:41:34 | require ... ssage') | ED25519 | tst.js:41:22:41:33 | 'my message' | -| tst.js:43:1:43:34 | require ... ssage') | SHA512 | tst.js:43:22:43:33 | 'my message' | -| tst.js:45:1:45:39 | require ... ssage') | ED25519 | tst.js:45:27:45:38 | 'my message' | -| tst.js:47:1:47:39 | require ... ssage') | SHA512 | tst.js:47:27:47:38 | 'my message' | -| tst.js:49:1:49:41 | require ... ('abc') | SHA256 | tst.js:49:36:49:40 | 'abc' | -| tst.js:51:1:51:51 | require ... ('abc') | SHA512 | tst.js:51:46:51:50 | 'abc' | -| tst.js:53:1:53:86 | require ... y dog') | MD5 | tst.js:53:41:53:85 | 'The qu ... zy dog' | -| tst.js:55:1:55:91 | require ... y dog') | MD5 | tst.js:55:46:55:90 | 'The qu ... zy dog' | -| tst.js:57:1:57:65 | require ... ecret") | RC2 | tst.js:57:57:57:64 | "secret" | -| tst.js:59:1:59:70 | require ... ecret") | 3DES | tst.js:59:62:59:69 | "secret" | -| tst.js:61:1:61:25 | require ... ssage") | MD5 | tst.js:61:16:61:24 | "message" | -| tst.js:63:1:63:32 | require ... ssword) | BCRYPT | tst.js:63:24:63:31 | password | -| tst.js:65:1:65:36 | require ... ssword) | BCRYPT | tst.js:65:28:65:35 | password | -| tst.js:67:1:67:34 | require ... ssword) | BCRYPT | tst.js:67:26:67:33 | password | -| tst.js:69:1:69:39 | require ... ssword) | BCRYPT | tst.js:69:31:69:38 | password | -| tst.js:71:1:71:49 | require ... md5" }) | MD5 | tst.js:71:18:71:26 | 'unicorn' | +| tst.js:1:1:1:27 | asmCryp ... (input) | SHA256 | tst.js:1:22:1:26 | input | | +| tst.js:5:5:5:43 | jwcrypt ... retKey) | DSA | tst.js:5:19:5:23 | input | | +| tst.js:10:18:10:55 | cipher. ... 'hex') | AES192 | tst.js:10:32:10:39 | 'input1' | CBC | +| tst.js:11:18:11:54 | cipher. ... 'hex') | AES192 | tst.js:11:31:11:38 | 'input2' | CBC | +| tst.js:15:1:15:21 | hash.up ... nput1') | SHA256 | tst.js:15:13:15:20 | 'input1' | | +| tst.js:16:1:16:20 | hash.write('input2') | SHA256 | tst.js:16:12:16:19 | 'input2' | | +| tst.js:20:1:20:21 | hmac.up ... nput1') | SHA256 | tst.js:20:13:20:20 | 'input1' | | +| tst.js:21:1:21:20 | hmac.write('input2') | SHA256 | tst.js:21:12:21:19 | 'input2' | | +| tst.js:25:1:25:21 | sign.up ... nput1') | SHA256 | tst.js:25:13:25:20 | 'input1' | | +| tst.js:26:1:26:20 | sign.write('input2') | SHA256 | tst.js:26:12:26:19 | 'input2' | | +| tst.js:29:1:29:52 | CryptoJ ... y 123') | AES | tst.js:29:22:29:33 | 'my message' | CBC | +| tst.js:32:1:32:31 | CryptoJ ... "Key") | SHA1 | tst.js:32:15:32:23 | "Message" | | +| tst.js:35:1:35:35 | CryptoJ ... "Key") | SHA1 | tst.js:35:19:35:27 | "Message" | | +| tst.js:37:1:37:64 | require ... y 123') | AES | tst.js:37:34:37:45 | 'my message' | CBC | +| tst.js:39:1:39:43 | require ... "Key") | SHA1 | tst.js:39:27:39:35 | "Message" | | +| tst.js:41:1:41:34 | require ... ssage') | ED25519 | tst.js:41:22:41:33 | 'my message' | | +| tst.js:43:1:43:34 | require ... ssage') | SHA512 | tst.js:43:22:43:33 | 'my message' | | +| tst.js:45:1:45:39 | require ... ssage') | ED25519 | tst.js:45:27:45:38 | 'my message' | | +| tst.js:47:1:47:39 | require ... ssage') | SHA512 | tst.js:47:27:47:38 | 'my message' | | +| tst.js:49:1:49:41 | require ... ('abc') | SHA256 | tst.js:49:36:49:40 | 'abc' | | +| tst.js:51:1:51:51 | require ... ('abc') | SHA512 | tst.js:51:46:51:50 | 'abc' | | +| tst.js:53:1:53:86 | require ... y dog') | MD5 | tst.js:53:41:53:85 | 'The qu ... zy dog' | | +| tst.js:55:1:55:91 | require ... y dog') | MD5 | tst.js:55:46:55:90 | 'The qu ... zy dog' | | +| tst.js:57:1:57:65 | require ... ecret") | RC2 | tst.js:57:57:57:64 | "secret" | | +| tst.js:59:1:59:70 | require ... ecret") | 3DES | tst.js:59:62:59:69 | "secret" | CBC | +| tst.js:61:1:61:25 | require ... ssage") | MD5 | tst.js:61:16:61:24 | "message" | | +| tst.js:63:1:63:32 | require ... ssword) | BCRYPT | tst.js:63:24:63:31 | password | | +| tst.js:65:1:65:36 | require ... ssword) | BCRYPT | tst.js:65:28:65:35 | password | | +| tst.js:67:1:67:34 | require ... ssword) | BCRYPT | tst.js:67:26:67:33 | password | | +| tst.js:69:1:69:39 | require ... ssword) | BCRYPT | tst.js:69:31:69:38 | password | | +| tst.js:71:1:71:49 | require ... md5" }) | MD5 | tst.js:71:18:71:26 | 'unicorn' | | diff --git a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.ql b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.ql index 10e5ae851f45..33b56f278a0a 100644 --- a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.ql +++ b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.ql @@ -1,4 +1,16 @@ import javascript +string getBlockMode(CryptographicOperation operation) { + if + operation.getAlgorithm() instanceof EncryptionAlgorithm and + not operation.getAlgorithm().(EncryptionAlgorithm).isStreamCipher() + then + if exists(operation.getBlockMode()) + then result = operation.getBlockMode() + else result = "" + else result = "" +} + from CryptographicOperation operation -select operation, operation.getAlgorithm().getName(), operation.getAnInput() +select operation, operation.getAlgorithm().getName(), operation.getAnInput(), + getBlockMode(operation) From aa2c532a7892cfedfc9f3d5d91470ce384efbf3c Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 2 Feb 2023 20:22:02 +0000 Subject: [PATCH 06/20] JS: adjust test whitespace --- .../CryptographicOperation.expected | 60 +++++++++---------- .../test/library-tests/CryptoLibraries/tst.js | 11 ++++ 2 files changed, 41 insertions(+), 30 deletions(-) diff --git a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.expected b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.expected index 6408535c0f4d..cd01b1d1b424 100644 --- a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.expected +++ b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.expected @@ -1,31 +1,31 @@ | tst.js:1:1:1:27 | asmCryp ... (input) | SHA256 | tst.js:1:22:1:26 | input | | -| tst.js:5:5:5:43 | jwcrypt ... retKey) | DSA | tst.js:5:19:5:23 | input | | -| tst.js:10:18:10:55 | cipher. ... 'hex') | AES192 | tst.js:10:32:10:39 | 'input1' | CBC | -| tst.js:11:18:11:54 | cipher. ... 'hex') | AES192 | tst.js:11:31:11:38 | 'input2' | CBC | -| tst.js:15:1:15:21 | hash.up ... nput1') | SHA256 | tst.js:15:13:15:20 | 'input1' | | -| tst.js:16:1:16:20 | hash.write('input2') | SHA256 | tst.js:16:12:16:19 | 'input2' | | -| tst.js:20:1:20:21 | hmac.up ... nput1') | SHA256 | tst.js:20:13:20:20 | 'input1' | | -| tst.js:21:1:21:20 | hmac.write('input2') | SHA256 | tst.js:21:12:21:19 | 'input2' | | -| tst.js:25:1:25:21 | sign.up ... nput1') | SHA256 | tst.js:25:13:25:20 | 'input1' | | -| tst.js:26:1:26:20 | sign.write('input2') | SHA256 | tst.js:26:12:26:19 | 'input2' | | -| tst.js:29:1:29:52 | CryptoJ ... y 123') | AES | tst.js:29:22:29:33 | 'my message' | CBC | -| tst.js:32:1:32:31 | CryptoJ ... "Key") | SHA1 | tst.js:32:15:32:23 | "Message" | | -| tst.js:35:1:35:35 | CryptoJ ... "Key") | SHA1 | tst.js:35:19:35:27 | "Message" | | -| tst.js:37:1:37:64 | require ... y 123') | AES | tst.js:37:34:37:45 | 'my message' | CBC | -| tst.js:39:1:39:43 | require ... "Key") | SHA1 | tst.js:39:27:39:35 | "Message" | | -| tst.js:41:1:41:34 | require ... ssage') | ED25519 | tst.js:41:22:41:33 | 'my message' | | -| tst.js:43:1:43:34 | require ... ssage') | SHA512 | tst.js:43:22:43:33 | 'my message' | | -| tst.js:45:1:45:39 | require ... ssage') | ED25519 | tst.js:45:27:45:38 | 'my message' | | -| tst.js:47:1:47:39 | require ... ssage') | SHA512 | tst.js:47:27:47:38 | 'my message' | | -| tst.js:49:1:49:41 | require ... ('abc') | SHA256 | tst.js:49:36:49:40 | 'abc' | | -| tst.js:51:1:51:51 | require ... ('abc') | SHA512 | tst.js:51:46:51:50 | 'abc' | | -| tst.js:53:1:53:86 | require ... y dog') | MD5 | tst.js:53:41:53:85 | 'The qu ... zy dog' | | -| tst.js:55:1:55:91 | require ... y dog') | MD5 | tst.js:55:46:55:90 | 'The qu ... zy dog' | | -| tst.js:57:1:57:65 | require ... ecret") | RC2 | tst.js:57:57:57:64 | "secret" | | -| tst.js:59:1:59:70 | require ... ecret") | 3DES | tst.js:59:62:59:69 | "secret" | CBC | -| tst.js:61:1:61:25 | require ... ssage") | MD5 | tst.js:61:16:61:24 | "message" | | -| tst.js:63:1:63:32 | require ... ssword) | BCRYPT | tst.js:63:24:63:31 | password | | -| tst.js:65:1:65:36 | require ... ssword) | BCRYPT | tst.js:65:28:65:35 | password | | -| tst.js:67:1:67:34 | require ... ssword) | BCRYPT | tst.js:67:26:67:33 | password | | -| tst.js:69:1:69:39 | require ... ssword) | BCRYPT | tst.js:69:31:69:38 | password | | -| tst.js:71:1:71:49 | require ... md5" }) | MD5 | tst.js:71:18:71:26 | 'unicorn' | | +| tst.js:7:5:7:43 | jwcrypt ... retKey) | DSA | tst.js:7:19:7:23 | input | | +| tst.js:12:18:12:55 | cipher. ... 'hex') | AES192 | tst.js:12:32:12:39 | 'input1' | CBC | +| tst.js:13:18:13:54 | cipher. ... 'hex') | AES192 | tst.js:13:31:13:38 | 'input2' | CBC | +| tst.js:17:1:17:21 | hash.up ... nput1') | SHA256 | tst.js:17:13:17:20 | 'input1' | | +| tst.js:18:1:18:20 | hash.write('input2') | SHA256 | tst.js:18:12:18:19 | 'input2' | | +| tst.js:22:1:22:21 | hmac.up ... nput1') | SHA256 | tst.js:22:13:22:20 | 'input1' | | +| tst.js:23:1:23:20 | hmac.write('input2') | SHA256 | tst.js:23:12:23:19 | 'input2' | | +| tst.js:27:1:27:21 | sign.up ... nput1') | SHA256 | tst.js:27:13:27:20 | 'input1' | | +| tst.js:28:1:28:20 | sign.write('input2') | SHA256 | tst.js:28:12:28:19 | 'input2' | | +| tst.js:36:1:36:52 | CryptoJ ... y 123') | AES | tst.js:36:22:36:33 | 'my message' | CBC | +| tst.js:39:1:39:31 | CryptoJ ... "Key") | SHA1 | tst.js:39:15:39:23 | "Message" | | +| tst.js:42:1:42:35 | CryptoJ ... "Key") | SHA1 | tst.js:42:19:42:27 | "Message" | | +| tst.js:44:1:44:64 | require ... y 123') | AES | tst.js:44:34:44:45 | 'my message' | CBC | +| tst.js:46:1:46:43 | require ... "Key") | SHA1 | tst.js:46:27:46:35 | "Message" | | +| tst.js:52:1:52:34 | require ... ssage') | ED25519 | tst.js:52:22:52:33 | 'my message' | | +| tst.js:54:1:54:34 | require ... ssage') | SHA512 | tst.js:54:22:54:33 | 'my message' | | +| tst.js:56:1:56:39 | require ... ssage') | ED25519 | tst.js:56:27:56:38 | 'my message' | | +| tst.js:58:1:58:39 | require ... ssage') | SHA512 | tst.js:58:27:58:38 | 'my message' | | +| tst.js:60:1:60:41 | require ... ('abc') | SHA256 | tst.js:60:36:60:40 | 'abc' | | +| tst.js:62:1:62:51 | require ... ('abc') | SHA512 | tst.js:62:46:62:50 | 'abc' | | +| tst.js:64:1:64:86 | require ... y dog') | MD5 | tst.js:64:41:64:85 | 'The qu ... zy dog' | | +| tst.js:66:1:66:91 | require ... y dog') | MD5 | tst.js:66:46:66:90 | 'The qu ... zy dog' | | +| tst.js:68:1:68:65 | require ... ecret") | RC2 | tst.js:68:57:68:64 | "secret" | | +| tst.js:70:1:70:70 | require ... ecret") | 3DES | tst.js:70:62:70:69 | "secret" | CBC | +| tst.js:72:1:72:25 | require ... ssage") | MD5 | tst.js:72:16:72:24 | "message" | | +| tst.js:74:1:74:32 | require ... ssword) | BCRYPT | tst.js:74:24:74:31 | password | | +| tst.js:76:1:76:36 | require ... ssword) | BCRYPT | tst.js:76:28:76:35 | password | | +| tst.js:78:1:78:34 | require ... ssword) | BCRYPT | tst.js:78:26:78:33 | password | | +| tst.js:80:1:80:39 | require ... ssword) | BCRYPT | tst.js:80:31:80:38 | password | | +| tst.js:82:1:82:49 | require ... md5" }) | MD5 | tst.js:82:18:82:26 | 'unicorn' | | diff --git a/javascript/ql/test/library-tests/CryptoLibraries/tst.js b/javascript/ql/test/library-tests/CryptoLibraries/tst.js index 5efd150bd167..a7aa47359d5c 100644 --- a/javascript/ql/test/library-tests/CryptoLibraries/tst.js +++ b/javascript/ql/test/library-tests/CryptoLibraries/tst.js @@ -1,5 +1,7 @@ asmCrypto.SHA256.hex(input); + + var jwcrypto = require("browserid-crypto"); jwcrypto.generateKeypair({algorithm: 'DSA'}, function(err, keypair) { jwcrypto.sign(input, keypair.secretKey); @@ -25,6 +27,11 @@ const sign = crypto.createSign('SHA256'); sign.update('input1'); sign.write('input2'); + + + + + var CryptoJS = require("crypto-js"); CryptoJS.AES.encrypt('my message', 'secret key 123'); @@ -38,6 +45,10 @@ require("crypto-js/aes").encrypt('my message', 'secret key 123'); require("crypto-js/sha1")("Message", "Key"); + + + + require("nacl").sign('my message'); require("nacl").hash('my message'); From b0b8f8725ea6376cc1c796eca3077110bf17ebad Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 2 Feb 2023 20:22:45 +0000 Subject: [PATCH 07/20] JS: add some CryptographicOperation#getBlockMode() tests --- .../CryptographicOperation.expected | 4 ++++ .../ql/test/library-tests/CryptoLibraries/tst.js | 16 ++++++++-------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.expected b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.expected index cd01b1d1b424..0844cb2c5778 100644 --- a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.expected +++ b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicOperation.expected @@ -1,4 +1,5 @@ | tst.js:1:1:1:27 | asmCryp ... (input) | SHA256 | tst.js:1:22:1:26 | input | | +| tst.js:3:1:3:41 | asmCryp ... ey, iv) | AES | tst.js:3:27:3:31 | input | OFB | | tst.js:7:5:7:43 | jwcrypt ... retKey) | DSA | tst.js:7:19:7:23 | input | | | tst.js:12:18:12:55 | cipher. ... 'hex') | AES192 | tst.js:12:32:12:39 | 'input1' | CBC | | tst.js:13:18:13:54 | cipher. ... 'hex') | AES192 | tst.js:13:31:13:38 | 'input2' | CBC | @@ -8,11 +9,14 @@ | tst.js:23:1:23:20 | hmac.write('input2') | SHA256 | tst.js:23:12:23:19 | 'input2' | | | tst.js:27:1:27:21 | sign.up ... nput1') | SHA256 | tst.js:27:13:27:20 | 'input1' | | | tst.js:28:1:28:20 | sign.write('input2') | SHA256 | tst.js:28:12:28:19 | 'input2' | | +| tst.js:32:1:32:38 | cipher. ... 'hex') | AES | tst.js:32:15:32:22 | 'input1' | ECB | +| tst.js:33:1:33:37 | cipher. ... 'hex') | AES | tst.js:33:14:33:21 | 'input2' | ECB | | tst.js:36:1:36:52 | CryptoJ ... y 123') | AES | tst.js:36:22:36:33 | 'my message' | CBC | | tst.js:39:1:39:31 | CryptoJ ... "Key") | SHA1 | tst.js:39:15:39:23 | "Message" | | | tst.js:42:1:42:35 | CryptoJ ... "Key") | SHA1 | tst.js:42:19:42:27 | "Message" | | | tst.js:44:1:44:64 | require ... y 123') | AES | tst.js:44:34:44:45 | 'my message' | CBC | | tst.js:46:1:46:43 | require ... "Key") | SHA1 | tst.js:46:27:46:35 | "Message" | | +| tst.js:50:1:50:40 | CryptoJ ... , opts) | AES | tst.js:50:22:50:26 | "msg" | CFB | | tst.js:52:1:52:34 | require ... ssage') | ED25519 | tst.js:52:22:52:33 | 'my message' | | | tst.js:54:1:54:34 | require ... ssage') | SHA512 | tst.js:54:22:54:33 | 'my message' | | | tst.js:56:1:56:39 | require ... ssage') | ED25519 | tst.js:56:27:56:38 | 'my message' | | diff --git a/javascript/ql/test/library-tests/CryptoLibraries/tst.js b/javascript/ql/test/library-tests/CryptoLibraries/tst.js index a7aa47359d5c..97f373d805b6 100644 --- a/javascript/ql/test/library-tests/CryptoLibraries/tst.js +++ b/javascript/ql/test/library-tests/CryptoLibraries/tst.js @@ -1,6 +1,6 @@ asmCrypto.SHA256.hex(input); - +asmCrypto.AES_OFB.encrypt(input, key, iv) var jwcrypto = require("browserid-crypto"); jwcrypto.generateKeypair({algorithm: 'DSA'}, function(err, keypair) { @@ -27,10 +27,10 @@ const sign = crypto.createSign('SHA256'); sign.update('input1'); sign.write('input2'); - - - - +var crypto = require('crypto'); +var cipher = crypto.createCipher('aes-192-ecb', 'a password'); +cipher.update('input1', 'utf8', 'hex'); +cipher.write('input2', 'utf8', 'hex'); var CryptoJS = require("crypto-js"); CryptoJS.AES.encrypt('my message', 'secret key 123'); @@ -45,9 +45,9 @@ require("crypto-js/aes").encrypt('my message', 'secret key 123'); require("crypto-js/sha1")("Message", "Key"); - - - +var CryptoJS = require("crypto-js"); +var opts = { mode: CryptoJS.mode.CFB } +CryptoJS.AES.encrypt("msg", "key", opts) require("nacl").sign('my message'); From 6b2a92a7ca368a8e64fea94e553066c930ba5ed7 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Fri, 3 Feb 2023 12:12:47 +0000 Subject: [PATCH 08/20] JS: update CryptographicKey.expected --- .../CryptoLibraries/CryptographicKey.expected | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicKey.expected b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicKey.expected index 4be374d96aeb..55b59cb16ad7 100644 --- a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicKey.expected +++ b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicKey.expected @@ -1,10 +1,11 @@ -| tst.js:5:26:5:42 | keypair.secretKey | -| tst.js:19:42:19:51 | 'a secret' | -| tst.js:29:36:29:51 | 'secret key 123' | -| tst.js:32:26:32:30 | "Key" | -| tst.js:35:30:35:34 | "Key" | -| tst.js:37:48:37:63 | 'secret key 123' | -| tst.js:39:38:39:42 | "Key" | -| tst.js:57:45:57:47 | key | -| tst.js:59:50:59:52 | key | -| tst.js:73:32:73:39 | "secret" | +| tst.js:7:26:7:42 | keypair.secretKey | +| tst.js:21:42:21:51 | 'a secret' | +| tst.js:36:36:36:51 | 'secret key 123' | +| tst.js:39:26:39:30 | "Key" | +| tst.js:42:30:42:34 | "Key" | +| tst.js:44:48:44:63 | 'secret key 123' | +| tst.js:46:38:46:42 | "Key" | +| tst.js:50:29:50:33 | "key" | +| tst.js:68:45:68:47 | key | +| tst.js:70:50:70:52 | key | +| tst.js:84:32:84:39 | "secret" | From e17b3d975d985cf6c6fd95495aa3aa269f91a2e1 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Fri, 3 Feb 2023 12:16:25 +0000 Subject: [PATCH 09/20] JS: pick up CryptographicKeys used in asmCrypto encrypt/decrypt calls --- .../semmle/javascript/frameworks/CryptoLibraries.qll | 12 +++++++++++- .../CryptoLibraries/CryptographicKey.expected | 1 + 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll b/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll index 8d2921e48cb9..2356e82ec958 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 { DataFlow::Node input; CryptographicAlgorithm algorithm; // non-functional private string algorithmName; + private string methodName; Apply() { /* @@ -66,7 +67,7 @@ private module AsmCrypto { exists(DataFlow::SourceNode asmCrypto | asmCrypto = DataFlow::globalVarRef("asmCrypto") and algorithm.matchesName(algorithmName) and - this = asmCrypto.getAPropertyRead(algorithmName).getAMemberCall(_) and + this = asmCrypto.getAPropertyRead(algorithmName).getAMemberCall(methodName) and input = this.getArgument(0) ) } @@ -79,6 +80,15 @@ private module AsmCrypto { isBlockEncryptionAlgorithm(this.getAlgorithm()) and result.matchesString(algorithmName) } + + DataFlow::Node getKey() { + methodName = ["encrypt", "decrypt"] and + result = super.getArgument(1) + } + } + + private class Key extends CryptographicKey { + Key() { this = any(Apply apply).getKey() } } } diff --git a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicKey.expected b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicKey.expected index 55b59cb16ad7..1b714e688df7 100644 --- a/javascript/ql/test/library-tests/CryptoLibraries/CryptographicKey.expected +++ b/javascript/ql/test/library-tests/CryptoLibraries/CryptographicKey.expected @@ -1,3 +1,4 @@ +| tst.js:3:34:3:36 | key | | tst.js:7:26:7:42 | keypair.secretKey | | tst.js:21:42:21:51 | 'a secret' | | tst.js:36:36:36:51 | 'secret key 123' | From b968b59afc07bf64043c368645bd1cb2c5ce2f70 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Fri, 3 Feb 2023 14:15:32 +0000 Subject: [PATCH 10/20] CryptoAlgorithms: make CryptographicAlgorithm#matchesName hold only if that algorithm is the most specific match --- .../javascript/security/CryptoAlgorithms.qll | 30 ++++++++++++++----- .../python/concepts/CryptoAlgorithms.qll | 30 ++++++++++++++----- .../codeql/ruby/security/CryptoAlgorithms.qll | 30 ++++++++++++++----- 3 files changed, 69 insertions(+), 21 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/security/CryptoAlgorithms.qll b/javascript/ql/lib/semmle/javascript/security/CryptoAlgorithms.qll index 766f99c61da6..79dd19dd9725 100644 --- a/javascript/ql/lib/semmle/javascript/security/CryptoAlgorithms.qll +++ b/javascript/ql/lib/semmle/javascript/security/CryptoAlgorithms.qll @@ -26,6 +26,26 @@ private newtype TCryptographicAlgorithm = isWeakPasswordHashingAlgorithm(name) and isWeak = true } +/** + * Gets the most specific `CryptographicAlgorithm` that matches the given `name`. + * A matching algorithm is one where the name of the algorithm matches the start of name, with allowances made for different name formats. + * In the case that multiple `CryptographicAlgorithm`s match the given `name`, the algorithm(s) with the longest name will be selected. This is intended to select more specific versions of algorithms when multiple versions could match - for example "SHA3_224" matches against both "SHA3" and "SHA3224", but the latter is a more precise match. + */ +bindingset[name] +private CryptographicAlgorithm getBestAlgorithmForName(string name) { + result = + max(CryptographicAlgorithm algorithm | + algorithm.getName() = + [ + name.toUpperCase(), // the full name + name.toUpperCase().regexpCapture("^([\\w]+)(?:-.*)?$", 1), // the name prior to any dashes or spaces + name.toUpperCase().regexpCapture("^([A-Z0-9]+)(?:(-|_).*)?$", 1) // the name prior to any dashes, spaces, or underscores + ].regexpReplaceAll("[-_ ]", "") // strip dashes, underscores, and spaces + | + algorithm order by algorithm.getName().length() + ) +} + /** * A cryptographic algorithm. */ @@ -39,15 +59,11 @@ abstract class CryptographicAlgorithm extends TCryptographicAlgorithm { abstract string getName(); /** - * Holds if the name of this algorithm matches `name` modulo case, - * white space, dashes, underscores, and anything after a dash or underscore in the name - * (to ignore modes of operation, such as CBC or ECB). + * Holds if the name of this algorithm is the most specific match for `name`. + * This predicate matches quite liberally to account for different ways of formatting algorithm names, e.g. using dashes, underscores, or spaces as separators, including or not including block modes of operation, etc. */ bindingset[name] - predicate matchesName(string name) { - [name.toUpperCase(), name.toUpperCase().regexpCapture("^([A-Z0-9]+)(?:(-|_).*)?$", 1)] - .regexpReplaceAll("[-_ ]", "") = getName() - } + predicate matchesName(string name) { this = getBestAlgorithmForName(name) } /** * Holds if this algorithm is weak. diff --git a/python/ql/lib/semmle/python/concepts/CryptoAlgorithms.qll b/python/ql/lib/semmle/python/concepts/CryptoAlgorithms.qll index 766f99c61da6..79dd19dd9725 100644 --- a/python/ql/lib/semmle/python/concepts/CryptoAlgorithms.qll +++ b/python/ql/lib/semmle/python/concepts/CryptoAlgorithms.qll @@ -26,6 +26,26 @@ private newtype TCryptographicAlgorithm = isWeakPasswordHashingAlgorithm(name) and isWeak = true } +/** + * Gets the most specific `CryptographicAlgorithm` that matches the given `name`. + * A matching algorithm is one where the name of the algorithm matches the start of name, with allowances made for different name formats. + * In the case that multiple `CryptographicAlgorithm`s match the given `name`, the algorithm(s) with the longest name will be selected. This is intended to select more specific versions of algorithms when multiple versions could match - for example "SHA3_224" matches against both "SHA3" and "SHA3224", but the latter is a more precise match. + */ +bindingset[name] +private CryptographicAlgorithm getBestAlgorithmForName(string name) { + result = + max(CryptographicAlgorithm algorithm | + algorithm.getName() = + [ + name.toUpperCase(), // the full name + name.toUpperCase().regexpCapture("^([\\w]+)(?:-.*)?$", 1), // the name prior to any dashes or spaces + name.toUpperCase().regexpCapture("^([A-Z0-9]+)(?:(-|_).*)?$", 1) // the name prior to any dashes, spaces, or underscores + ].regexpReplaceAll("[-_ ]", "") // strip dashes, underscores, and spaces + | + algorithm order by algorithm.getName().length() + ) +} + /** * A cryptographic algorithm. */ @@ -39,15 +59,11 @@ abstract class CryptographicAlgorithm extends TCryptographicAlgorithm { abstract string getName(); /** - * Holds if the name of this algorithm matches `name` modulo case, - * white space, dashes, underscores, and anything after a dash or underscore in the name - * (to ignore modes of operation, such as CBC or ECB). + * Holds if the name of this algorithm is the most specific match for `name`. + * This predicate matches quite liberally to account for different ways of formatting algorithm names, e.g. using dashes, underscores, or spaces as separators, including or not including block modes of operation, etc. */ bindingset[name] - predicate matchesName(string name) { - [name.toUpperCase(), name.toUpperCase().regexpCapture("^([A-Z0-9]+)(?:(-|_).*)?$", 1)] - .regexpReplaceAll("[-_ ]", "") = getName() - } + predicate matchesName(string name) { this = getBestAlgorithmForName(name) } /** * Holds if this algorithm is weak. diff --git a/ruby/ql/lib/codeql/ruby/security/CryptoAlgorithms.qll b/ruby/ql/lib/codeql/ruby/security/CryptoAlgorithms.qll index 766f99c61da6..79dd19dd9725 100644 --- a/ruby/ql/lib/codeql/ruby/security/CryptoAlgorithms.qll +++ b/ruby/ql/lib/codeql/ruby/security/CryptoAlgorithms.qll @@ -26,6 +26,26 @@ private newtype TCryptographicAlgorithm = isWeakPasswordHashingAlgorithm(name) and isWeak = true } +/** + * Gets the most specific `CryptographicAlgorithm` that matches the given `name`. + * A matching algorithm is one where the name of the algorithm matches the start of name, with allowances made for different name formats. + * In the case that multiple `CryptographicAlgorithm`s match the given `name`, the algorithm(s) with the longest name will be selected. This is intended to select more specific versions of algorithms when multiple versions could match - for example "SHA3_224" matches against both "SHA3" and "SHA3224", but the latter is a more precise match. + */ +bindingset[name] +private CryptographicAlgorithm getBestAlgorithmForName(string name) { + result = + max(CryptographicAlgorithm algorithm | + algorithm.getName() = + [ + name.toUpperCase(), // the full name + name.toUpperCase().regexpCapture("^([\\w]+)(?:-.*)?$", 1), // the name prior to any dashes or spaces + name.toUpperCase().regexpCapture("^([A-Z0-9]+)(?:(-|_).*)?$", 1) // the name prior to any dashes, spaces, or underscores + ].regexpReplaceAll("[-_ ]", "") // strip dashes, underscores, and spaces + | + algorithm order by algorithm.getName().length() + ) +} + /** * A cryptographic algorithm. */ @@ -39,15 +59,11 @@ abstract class CryptographicAlgorithm extends TCryptographicAlgorithm { abstract string getName(); /** - * Holds if the name of this algorithm matches `name` modulo case, - * white space, dashes, underscores, and anything after a dash or underscore in the name - * (to ignore modes of operation, such as CBC or ECB). + * Holds if the name of this algorithm is the most specific match for `name`. + * This predicate matches quite liberally to account for different ways of formatting algorithm names, e.g. using dashes, underscores, or spaces as separators, including or not including block modes of operation, etc. */ bindingset[name] - predicate matchesName(string name) { - [name.toUpperCase(), name.toUpperCase().regexpCapture("^([A-Z0-9]+)(?:(-|_).*)?$", 1)] - .regexpReplaceAll("[-_ ]", "") = getName() - } + predicate matchesName(string name) { this = getBestAlgorithmForName(name) } /** * Holds if this algorithm is weak. From 6c35feaa984d070765ef621733b31d40a17a6e54 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Fri, 3 Feb 2023 14:39:32 +0000 Subject: [PATCH 11/20] ConceptsShared: add a default implementation of BlockMode CryptographicOperation#getBlockMode() for compatibility with external code --- javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll | 2 +- python/ql/lib/semmle/python/internal/ConceptsShared.qll | 2 +- ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll index 23b345928520..522c883e0b4a 100644 --- a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll +++ b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll @@ -78,7 +78,7 @@ module Cryptography { * This may have no result - for example if the `CryptographicAlgorithm` used * is a stream cipher rather than a block cipher. */ - abstract BlockMode getBlockMode(); + BlockMode getBlockMode() { none() } } } diff --git a/python/ql/lib/semmle/python/internal/ConceptsShared.qll b/python/ql/lib/semmle/python/internal/ConceptsShared.qll index 23b345928520..522c883e0b4a 100644 --- a/python/ql/lib/semmle/python/internal/ConceptsShared.qll +++ b/python/ql/lib/semmle/python/internal/ConceptsShared.qll @@ -78,7 +78,7 @@ module Cryptography { * This may have no result - for example if the `CryptographicAlgorithm` used * is a stream cipher rather than a block cipher. */ - abstract BlockMode getBlockMode(); + BlockMode getBlockMode() { none() } } } diff --git a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll index 23b345928520..522c883e0b4a 100644 --- a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll +++ b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll @@ -78,7 +78,7 @@ module Cryptography { * This may have no result - for example if the `CryptographicAlgorithm` used * is a stream cipher rather than a block cipher. */ - abstract BlockMode getBlockMode(); + BlockMode getBlockMode() { none() } } } From ecafce81914e8d77599ba7a66463711f2e43d3a2 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Fri, 3 Feb 2023 21:44:23 +0100 Subject: [PATCH 12/20] improve the CryptoJS model by using API::Node --- .../javascript/frameworks/CryptoLibraries.qll | 46 ++++++++----------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll b/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll index 2356e82ec958..8ebae47d840d 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll @@ -279,23 +279,18 @@ private module CryptoJS { /** * Matches `CryptoJS.` and `require("crypto-js/")` */ - private DataFlow::SourceNode getAlgorithmNode(CryptographicAlgorithm algorithm) { + private API::Node getAlgorithmNode(CryptographicAlgorithm algorithm) { exists(string algorithmName | algorithm.matchesName(algorithmName) | - exists(DataFlow::SourceNode mod | mod = DataFlow::moduleImport("crypto-js") | - result = mod.getAPropertyRead(algorithmName) or - result = mod.getAPropertyRead("Hmac" + algorithmName) // they prefix Hmac + exists(API::Node mod | mod = API::moduleImport("crypto-js") | + result = mod.getMember(algorithmName) or + result = mod.getMember("Hmac" + algorithmName) // they prefix Hmac ) or - exists(DataFlow::SourceNode mod | - mod = DataFlow::moduleImport("crypto-js/" + algorithmName) and - result = mod - ) + result = API::moduleImport("crypto-js/" + algorithmName) ) } - private DataFlow::CallNode getEncryptionApplication( - DataFlow::Node input, CryptographicAlgorithm algorithm - ) { + private API::CallNode getEncryptionApplication(API::Node input, CryptographicAlgorithm algorithm) { /* * ``` * var CryptoJS = require("crypto-js"); @@ -309,13 +304,11 @@ private module CryptoJS { * Also matches where `CryptoJS.` has been replaced by `require("crypto-js/")` */ - result = getAlgorithmNode(algorithm).getAMemberCall("encrypt") and - input = result.getArgument(0) + result = getAlgorithmNode(algorithm).getMember("encrypt").getACall() and + input = result.getParameter(0) } - private DataFlow::CallNode getDirectApplication( - DataFlow::Node input, CryptographicAlgorithm algorithm - ) { + private API::CallNode getDirectApplication(API::Node input, CryptographicAlgorithm algorithm) { /* * ``` * var CryptoJS = require("crypto-js"); @@ -331,11 +324,11 @@ private module CryptoJS { */ result = getAlgorithmNode(algorithm).getACall() and - input = result.getArgument(0) + input = result.getParameter(0) } - private class Apply extends CryptographicOperation::Range, DataFlow::CallNode { - DataFlow::Node input; + private class Apply extends CryptographicOperation::Range instanceof API::CallNode { + API::Node input; CryptographicAlgorithm algorithm; // non-functional Apply() { @@ -343,16 +336,15 @@ private module CryptoJS { this = getDirectApplication(input, algorithm) } - override DataFlow::Node getAnInput() { result = input } + override DataFlow::Node getAnInput() { result = input.asSink() } override CryptographicAlgorithm getAlgorithm() { result = algorithm } // e.g. CryptoJS.AES.encrypt("msg", "key", { mode: CryptoJS.mode. }) private BlockMode getExplicitBlockMode() { - exists(DataFlow::ObjectLiteralNode o, DataFlow::SourceNode modeNode, string modeString | - modeNode = API::moduleImport("crypto-js").getMember("mode").getMember(modeString).asSource() and - o.flowsTo(this.getArgument(2)) and - modeNode = o.getAPropertySource("mode") + exists(string modeString | + API::moduleImport("crypto-js").getMember("mode").getMember(modeString).asSource() = + super.getParameter(2).getMember("mode").asSink() | result.matchesString(modeString) ) @@ -372,15 +364,13 @@ private module CryptoJS { private class Key extends CryptographicKey { Key() { - exists(DataFlow::SourceNode e, CryptographicAlgorithm algorithm | - e = getAlgorithmNode(algorithm) - | + exists(API::Node e, CryptographicAlgorithm algorithm | e = getAlgorithmNode(algorithm) | exists(string name | name = "encrypt" or name = "decrypt" | algorithm instanceof EncryptionAlgorithm and - this = e.getAMemberCall(name).getArgument(1) + this = e.getMember(name).getACall().getArgument(1) ) or algorithm instanceof HashingAlgorithm and From 8d90c02a6726bf7c05a21fce12f7258838153e96 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Tue, 14 Feb 2023 15:24:22 +0000 Subject: [PATCH 13/20] JS: remove unused field --- .../ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll b/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll index 8ebae47d840d..21d23f517f3a 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll @@ -149,7 +149,6 @@ private module BrowserIdCrypto { */ private module NodeJSCrypto { private class InstantiatedAlgorithm extends DataFlow::CallNode { - CryptographicAlgorithm algorithm; // non-functional private string algorithmName; InstantiatedAlgorithm() { @@ -169,12 +168,11 @@ private module NodeJSCrypto { exists(DataFlow::SourceNode mod | mod = DataFlow::moduleImport("crypto") and this = mod.getAMemberCall("create" + ["Hash", "Hmac", "Sign", "Cipher"]) and - algorithmName = this.getArgument(0).getStringValue() and - algorithm.matchesName(algorithmName) + algorithmName = this.getArgument(0).getStringValue() ) } - CryptographicAlgorithm getAlgorithm() { result = algorithm } + CryptographicAlgorithm getAlgorithm() { result.matchesName(algorithmName) } private BlockMode getExplicitBlockMode() { result.matchesString(algorithmName) } From c7aaad9ed0e7bda3f9847b6443df7b64d343f2dc Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Tue, 14 Feb 2023 16:41:28 +0000 Subject: [PATCH 14/20] JS: avoid adding a deprecated CryptographicOperation#getInput to py/ruby --- .../ql/lib/semmle/javascript/Concepts.qll | 19 ++++++++++++++++++- .../javascript/internal/ConceptsShared.qll | 6 ------ .../semmle/python/internal/ConceptsShared.qll | 6 ------ .../codeql/ruby/internal/ConceptsShared.qll | 6 ------ 4 files changed, 18 insertions(+), 19 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/Concepts.qll b/javascript/ql/lib/semmle/javascript/Concepts.qll index a760e746030f..14308a3d17b9 100644 --- a/javascript/ql/lib/semmle/javascript/Concepts.qll +++ b/javascript/ql/lib/semmle/javascript/Concepts.qll @@ -115,5 +115,22 @@ abstract class PersistentWriteAccess extends DataFlow::Node { * Provides models for cryptographic things. */ module Cryptography { - import semmle.javascript.internal.ConceptsShared::Cryptography + private import semmle.javascript.internal.ConceptsShared::Cryptography as SC + + class CryptographicOperation extends SC::CryptographicOperation instanceof CryptographicOperation::Range { + /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ + deprecated final DataFlow::Node getInput() { result = this.getAnInput() } + } + + class EncryptionAlgorithm = SC::EncryptionAlgorithm; + + class HashingAlgorithm = SC::HashingAlgorithm; + + class PasswordHashingAlgorithm = SC::PasswordHashingAlgorithm; + + module CryptographicOperation = SC::CryptographicOperation; + + class BlockMode = SC::BlockMode; + + class CryptographicAlgorithm = SC::CryptographicAlgorithm; } diff --git a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll index 522c883e0b4a..449aca6e5c4a 100644 --- a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll +++ b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll @@ -43,9 +43,6 @@ module Cryptography { /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ DataFlow::Node getAnInput() { result = super.getAnInput() } - /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ - deprecated final DataFlow::Node getInput() { result = super.getInput() } - /** * Gets the block mode used to perform this cryptographic operation. * This may have no result - for example if the `CryptographicAlgorithm` used @@ -70,9 +67,6 @@ module Cryptography { /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ abstract DataFlow::Node getAnInput(); - /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ - deprecated final DataFlow::Node getInput() { result = this.getAnInput() } - /** * Gets the block mode used to perform this cryptographic operation. * This may have no result - for example if the `CryptographicAlgorithm` used diff --git a/python/ql/lib/semmle/python/internal/ConceptsShared.qll b/python/ql/lib/semmle/python/internal/ConceptsShared.qll index 522c883e0b4a..449aca6e5c4a 100644 --- a/python/ql/lib/semmle/python/internal/ConceptsShared.qll +++ b/python/ql/lib/semmle/python/internal/ConceptsShared.qll @@ -43,9 +43,6 @@ module Cryptography { /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ DataFlow::Node getAnInput() { result = super.getAnInput() } - /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ - deprecated final DataFlow::Node getInput() { result = super.getInput() } - /** * Gets the block mode used to perform this cryptographic operation. * This may have no result - for example if the `CryptographicAlgorithm` used @@ -70,9 +67,6 @@ module Cryptography { /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ abstract DataFlow::Node getAnInput(); - /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ - deprecated final DataFlow::Node getInput() { result = this.getAnInput() } - /** * Gets the block mode used to perform this cryptographic operation. * This may have no result - for example if the `CryptographicAlgorithm` used diff --git a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll index 522c883e0b4a..449aca6e5c4a 100644 --- a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll +++ b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll @@ -43,9 +43,6 @@ module Cryptography { /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ DataFlow::Node getAnInput() { result = super.getAnInput() } - /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ - deprecated final DataFlow::Node getInput() { result = super.getInput() } - /** * Gets the block mode used to perform this cryptographic operation. * This may have no result - for example if the `CryptographicAlgorithm` used @@ -70,9 +67,6 @@ module Cryptography { /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */ abstract DataFlow::Node getAnInput(); - /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ - deprecated final DataFlow::Node getInput() { result = this.getAnInput() } - /** * Gets the block mode used to perform this cryptographic operation. * This may have no result - for example if the `CryptographicAlgorithm` used From d4d0b910853b3609bae33c966adf65f904afdbe4 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 15 Feb 2023 15:22:49 +0000 Subject: [PATCH 15/20] dynamic: switch CryptographicOperation::Range#getBlockMode() back to being an abstract predicate --- javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll | 2 +- python/ql/lib/semmle/python/internal/ConceptsShared.qll | 2 +- ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll index 449aca6e5c4a..660899884196 100644 --- a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll +++ b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll @@ -72,7 +72,7 @@ module Cryptography { * This may have no result - for example if the `CryptographicAlgorithm` used * is a stream cipher rather than a block cipher. */ - BlockMode getBlockMode() { none() } + abstract BlockMode getBlockMode(); } } diff --git a/python/ql/lib/semmle/python/internal/ConceptsShared.qll b/python/ql/lib/semmle/python/internal/ConceptsShared.qll index 449aca6e5c4a..660899884196 100644 --- a/python/ql/lib/semmle/python/internal/ConceptsShared.qll +++ b/python/ql/lib/semmle/python/internal/ConceptsShared.qll @@ -72,7 +72,7 @@ module Cryptography { * This may have no result - for example if the `CryptographicAlgorithm` used * is a stream cipher rather than a block cipher. */ - BlockMode getBlockMode() { none() } + abstract BlockMode getBlockMode(); } } diff --git a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll index 449aca6e5c4a..660899884196 100644 --- a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll +++ b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll @@ -72,7 +72,7 @@ module Cryptography { * This may have no result - for example if the `CryptographicAlgorithm` used * is a stream cipher rather than a block cipher. */ - BlockMode getBlockMode() { none() } + abstract BlockMode getBlockMode(); } } From 925b4a3fa88caca6c4f48c0eab3d88fa11528f55 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 15 Feb 2023 16:20:08 +0000 Subject: [PATCH 16/20] JS: improve documentation on deprecated CryptographicOperation#getInput() predicate --- javascript/ql/lib/semmle/javascript/Concepts.qll | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/Concepts.qll b/javascript/ql/lib/semmle/javascript/Concepts.qll index 14308a3d17b9..c70a6f3804f4 100644 --- a/javascript/ql/lib/semmle/javascript/Concepts.qll +++ b/javascript/ql/lib/semmle/javascript/Concepts.qll @@ -118,7 +118,13 @@ module Cryptography { private import semmle.javascript.internal.ConceptsShared::Cryptography as SC class CryptographicOperation extends SC::CryptographicOperation instanceof CryptographicOperation::Range { - /** DEPRECATED. This predicate has been renamed to `getAnInput`. */ + /** + * DEPRECATED. This predicate has been renamed to `getAnInput`. + * + * To implement `CryptographicOperation`, please extend + * `CryptographicOperation::Range` and implement `getAnInput` instead of + * extending this class directly. + */ deprecated final DataFlow::Node getInput() { result = this.getAnInput() } } From e8cbf7287d66538d89c19c2bd9cf1532f45a24e9 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 15 Feb 2023 16:50:24 +0000 Subject: [PATCH 17/20] JS: breaking change note for CryptographicOperation sync --- ...23-02-15-cryptographic-operation-getinput-deprecated.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 javascript/ql/lib/change-notes/2023-02-15-cryptographic-operation-getinput-deprecated.md diff --git a/javascript/ql/lib/change-notes/2023-02-15-cryptographic-operation-getinput-deprecated.md b/javascript/ql/lib/change-notes/2023-02-15-cryptographic-operation-getinput-deprecated.md new file mode 100644 index 000000000000..246d23739c18 --- /dev/null +++ b/javascript/ql/lib/change-notes/2023-02-15-cryptographic-operation-getinput-deprecated.md @@ -0,0 +1,7 @@ +--- +category: breaking +--- +* The `CryptographicOperation` concept has been changed to use a range pattern. This is a breaking change and existing implementations of `CryptographicOperation` will need to be updated in order to compile. These implementations can be updated by: + 1. Extending `CryptographicOperation::Range` rather than `CryptographicOperation` + 2. Renaming the `getInput()` member predicate as `getAnInput()` + 3. Implementing the `BlockMode getBlockMode()` member predicate. The implementation for this can be `none()` if the operation is a hashing operation or an encryption operation using a stream cipher. From 43af306d602fcb63a22772b90b4214d3ad8bed10 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 15 Feb 2023 16:55:18 +0000 Subject: [PATCH 18/20] dynamic: more detailed qldoc for CryptographicOperation#getBlockMode() --- .../javascript/internal/ConceptsShared.qll | 16 ++++++++++++---- .../semmle/python/internal/ConceptsShared.qll | 16 ++++++++++++---- .../lib/codeql/ruby/internal/ConceptsShared.qll | 16 ++++++++++++---- 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll index 660899884196..def6e7da1039 100644 --- a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll +++ b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll @@ -45,8 +45,12 @@ module Cryptography { /** * Gets the block mode used to perform this cryptographic operation. - * This may have no result - for example if the `CryptographicAlgorithm` used - * is a stream cipher rather than a block cipher. + * + * This predicate is only expected to have a result if two conditions hold: + * 1. The operation is an encryption operation, i.e. the algorithm used is an `EncryptionAlgorithm`, and + * 2. The algorithm used is a block cipher (not a stream cipher). + * + * If either of these conditions do not hold, then this predicate should have no result. */ BlockMode getBlockMode() { result = super.getBlockMode() } } @@ -69,8 +73,12 @@ module Cryptography { /** * Gets the block mode used to perform this cryptographic operation. - * This may have no result - for example if the `CryptographicAlgorithm` used - * is a stream cipher rather than a block cipher. + * + * This predicate is only expected to have a result if two conditions hold: + * 1. The operation is an encryption operation, i.e. the algorithm used is an `EncryptionAlgorithm`, and + * 2. The algorithm used is a block cipher (not a stream cipher). + * + * If either of these conditions do not hold, then this predicate should have no result. */ abstract BlockMode getBlockMode(); } diff --git a/python/ql/lib/semmle/python/internal/ConceptsShared.qll b/python/ql/lib/semmle/python/internal/ConceptsShared.qll index 660899884196..def6e7da1039 100644 --- a/python/ql/lib/semmle/python/internal/ConceptsShared.qll +++ b/python/ql/lib/semmle/python/internal/ConceptsShared.qll @@ -45,8 +45,12 @@ module Cryptography { /** * Gets the block mode used to perform this cryptographic operation. - * This may have no result - for example if the `CryptographicAlgorithm` used - * is a stream cipher rather than a block cipher. + * + * This predicate is only expected to have a result if two conditions hold: + * 1. The operation is an encryption operation, i.e. the algorithm used is an `EncryptionAlgorithm`, and + * 2. The algorithm used is a block cipher (not a stream cipher). + * + * If either of these conditions do not hold, then this predicate should have no result. */ BlockMode getBlockMode() { result = super.getBlockMode() } } @@ -69,8 +73,12 @@ module Cryptography { /** * Gets the block mode used to perform this cryptographic operation. - * This may have no result - for example if the `CryptographicAlgorithm` used - * is a stream cipher rather than a block cipher. + * + * This predicate is only expected to have a result if two conditions hold: + * 1. The operation is an encryption operation, i.e. the algorithm used is an `EncryptionAlgorithm`, and + * 2. The algorithm used is a block cipher (not a stream cipher). + * + * If either of these conditions do not hold, then this predicate should have no result. */ abstract BlockMode getBlockMode(); } diff --git a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll index 660899884196..def6e7da1039 100644 --- a/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll +++ b/ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll @@ -45,8 +45,12 @@ module Cryptography { /** * Gets the block mode used to perform this cryptographic operation. - * This may have no result - for example if the `CryptographicAlgorithm` used - * is a stream cipher rather than a block cipher. + * + * This predicate is only expected to have a result if two conditions hold: + * 1. The operation is an encryption operation, i.e. the algorithm used is an `EncryptionAlgorithm`, and + * 2. The algorithm used is a block cipher (not a stream cipher). + * + * If either of these conditions do not hold, then this predicate should have no result. */ BlockMode getBlockMode() { result = super.getBlockMode() } } @@ -69,8 +73,12 @@ module Cryptography { /** * Gets the block mode used to perform this cryptographic operation. - * This may have no result - for example if the `CryptographicAlgorithm` used - * is a stream cipher rather than a block cipher. + * + * This predicate is only expected to have a result if two conditions hold: + * 1. The operation is an encryption operation, i.e. the algorithm used is an `EncryptionAlgorithm`, and + * 2. The algorithm used is a block cipher (not a stream cipher). + * + * If either of these conditions do not hold, then this predicate should have no result. */ abstract BlockMode getBlockMode(); } From 1958b9dcd548261fe089de26d7089c1f11bcf952 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 15 Feb 2023 16:59:03 +0000 Subject: [PATCH 19/20] JS: add missing qldoc --- javascript/ql/lib/semmle/javascript/Concepts.qll | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/Concepts.qll b/javascript/ql/lib/semmle/javascript/Concepts.qll index c70a6f3804f4..831fd0fe145e 100644 --- a/javascript/ql/lib/semmle/javascript/Concepts.qll +++ b/javascript/ql/lib/semmle/javascript/Concepts.qll @@ -117,6 +117,13 @@ abstract class PersistentWriteAccess extends DataFlow::Node { module Cryptography { private import semmle.javascript.internal.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 model new APIs. If you want to refine existing API models, + * extend `CryptographicOperation` instead. + */ class CryptographicOperation extends SC::CryptographicOperation instanceof CryptographicOperation::Range { /** * DEPRECATED. This predicate has been renamed to `getAnInput`. From 9cfd0f5f469bb8b0f74f8f17f08ab878305e4847 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 16 Feb 2023 11:00:37 +0000 Subject: [PATCH 20/20] JS: fix qldoc --- javascript/ql/lib/semmle/javascript/Concepts.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/Concepts.qll b/javascript/ql/lib/semmle/javascript/Concepts.qll index 831fd0fe145e..67cf325eb11d 100644 --- a/javascript/ql/lib/semmle/javascript/Concepts.qll +++ b/javascript/ql/lib/semmle/javascript/Concepts.qll @@ -121,8 +121,8 @@ module Cryptography { * A data-flow node that is an application of a cryptographic algorithm. For example, * encryption, decryption, signature-validation. * - * Extend this class to model new APIs. If you want to refine existing API models, - * extend `CryptographicOperation` instead. + * Extend this class to refine existing API models. If you want to model new APIs, + * extend `CryptographicOperation::Range` instead. */ class CryptographicOperation extends SC::CryptographicOperation instanceof CryptographicOperation::Range { /**