From 0537ad2e82db02bd0676812bd8556ed141b86408 Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Tue, 12 Apr 2022 14:46:43 +0100 Subject: [PATCH 01/17] Initial CSharp Crypto queries --- csharp/CWE-760/HardcodedSalt.ql | 72 ++++++++++++++++++++++++++++++++ csharp/CWE-916/WeakIterations.ql | 72 ++++++++++++++++++++++++++++++++ csharp/github/crypto.qll | 34 +++++++++++++++ csharp/github/hardcoded.qll | 27 ++++++++++++ 4 files changed, 205 insertions(+) create mode 100644 csharp/CWE-760/HardcodedSalt.ql create mode 100644 csharp/CWE-916/WeakIterations.ql create mode 100644 csharp/github/crypto.qll create mode 100644 csharp/github/hardcoded.qll diff --git a/csharp/CWE-760/HardcodedSalt.ql b/csharp/CWE-760/HardcodedSalt.ql new file mode 100644 index 0000000000..5ed6f5bb36 --- /dev/null +++ b/csharp/CWE-760/HardcodedSalt.ql @@ -0,0 +1,72 @@ +/** + * @name Hardcoded Salt + * @description Hardcoded Salt + * @kind path-problem + * @problem.severity error + * @security-severity 6.1 + * @precision medium + * @id cs/hardcoded-salt + * @tags security + * external/cwe/cwe-760 + */ + +import csharp +private import semmle.code.csharp.frameworks.Moq +private import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph +// import semmle.code.csharp.frameworks.system.security.Cryptography +private import github.hardcoded +private import github.crypto + +module HardcodedSalt { + abstract class Source extends DataFlow::ExprNode { } + + abstract class Sink extends DataFlow::ExprNode { } + + abstract class Sanitizer extends DataFlow::ExprNode { } + + abstract class SanitizerGuard extends DataFlow::BarrierGuard { } + + /* + * Sources + */ + + class Hardcoded extends Source { + Hardcoded() { this instanceof HardcodedValues } + } + + /* + * Sinks + */ + + class HashAlgSalts extends Sink { + HashAlgSalts() { exists(Crypto::HashingAlgorithms hash | this = hash.getSalt()) } + } + + /* + * Config + */ + + class TaintTrackingConfiguration extends TaintTracking::Configuration { + TaintTrackingConfiguration() { this = "HardcodedSalt" } + + override predicate isSource(DataFlow::Node source) { source instanceof HardcodedSalt::Source } + + override predicate isSink(DataFlow::Node sink) { + sink instanceof HardcodedSalt::Sink and + not any(ReturnedByMockObject mock).getAMemberInitializationValue() = sink.asExpr() and + not any(ReturnedByMockObject mock).getAnArgument() = sink.asExpr() + } + + override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer } + + override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { + guard instanceof SanitizerGuard + } + } +} + +from + HardcodedSalt::TaintTrackingConfiguration config, DataFlow::PathNode source, + DataFlow::PathNode sink +where config.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "Use of $@.", source.getNode(), "hardcoded salt" diff --git a/csharp/CWE-916/WeakIterations.ql b/csharp/CWE-916/WeakIterations.ql new file mode 100644 index 0000000000..cde06b1a50 --- /dev/null +++ b/csharp/CWE-916/WeakIterations.ql @@ -0,0 +1,72 @@ +/** + * @name Hardcoded Salt + * @description Hardcoded Salt + * @kind path-problem + * @problem.severity error + * @security-severity 6.1 + * @precision medium + * @id cs/hardcoded-salt + * @tags security + * external/cwe/cwe-760 + */ + +import csharp +private import semmle.code.csharp.frameworks.Moq +private import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph +// import semmle.code.csharp.frameworks.system.security.Cryptography +private import github.hardcoded +private import github.crypto + +module HardcodedSalt { + abstract class Source extends DataFlow::ExprNode { } + + abstract class Sink extends DataFlow::ExprNode { } + + abstract class Sanitizer extends DataFlow::ExprNode { } + + abstract class SanitizerGuard extends DataFlow::BarrierGuard { } + + /* + * Sources + */ + + class Hardcoded extends Source { + Hardcoded() { this.getExpr().(IntLiteral).getValue().toInt() < 1000 } + } + + /* + * Sinks + */ + + class HashAlgSalts extends Sink { + HashAlgSalts() { exists(Crypto::HashingAlgorithms hash | this = hash.getIterations()) } + } + + /* + * Config + */ + + class TaintTrackingConfiguration extends TaintTracking::Configuration { + TaintTrackingConfiguration() { this = "HardcodedSalt" } + + override predicate isSource(DataFlow::Node source) { source instanceof HardcodedSalt::Source } + + override predicate isSink(DataFlow::Node sink) { + sink instanceof HardcodedSalt::Sink and + not any(ReturnedByMockObject mock).getAMemberInitializationValue() = sink.asExpr() and + not any(ReturnedByMockObject mock).getAnArgument() = sink.asExpr() + } + + override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer } + + override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { + guard instanceof SanitizerGuard + } + } +} + +from + HardcodedSalt::TaintTrackingConfiguration config, DataFlow::PathNode source, + DataFlow::PathNode sink +where config.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "Use of $@.", source.getNode(), "hardcoded salt" diff --git a/csharp/github/crypto.qll b/csharp/github/crypto.qll new file mode 100644 index 0000000000..2ffdfb9866 --- /dev/null +++ b/csharp/github/crypto.qll @@ -0,0 +1,34 @@ +import csharp + +module Crypto { + class HashingAlgorithm extends DataFlow::ExprNode { + abstract DataFlow::ExprNode getHashValue(); + + abstract DataFlow::ExprNode getSalt(); + + abstract DataFlow::ExprNode getIterations(); + } + + abstract class HashingAlgorithms extends HashingAlgorithm { } + + class CryptoRfc2898DeriveBytes extends HashingAlgorithms { + CryptoRfc2898DeriveBytes() { + exists(ObjectCreation object | + object.getType().getQualifiedName() = "System.Security.Cryptography.Rfc2898DeriveBytes" and + this.asExpr() = object + ) + } + + override DataFlow::ExprNode getHashValue() { + result.asExpr() = this.asExpr().(ObjectCreation).getArgument(0) + } + + override DataFlow::ExprNode getSalt() { + result.asExpr() = this.asExpr().(ObjectCreation).getArgument(1) + } + + override DataFlow::ExprNode getIterations() { + result.asExpr() = this.asExpr().(ObjectCreation).getArgument(2) + } + } +} diff --git a/csharp/github/hardcoded.qll b/csharp/github/hardcoded.qll new file mode 100644 index 0000000000..63856db305 --- /dev/null +++ b/csharp/github/hardcoded.qll @@ -0,0 +1,27 @@ +import csharp + +abstract class HardcodedValues extends DataFlow::ExprNode { } + +class NonEmptyStringLiteral extends HardcodedValues { + NonEmptyStringLiteral() { this.getExpr().(StringLiteral).getValue().length() > 1 } +} + +class ByteArrayLiteral extends HardcodedValues { + ByteArrayLiteral() { + this.getExpr() = + any(ArrayCreation ac | + ac.getArrayType().getElementType() instanceof ByteType and + ac.hasInitializer() + ) + } +} + +class CharArrayLiteral extends HardcodedValues { + CharArrayLiteral() { + this.getExpr() = + any(ArrayCreation ac | + ac.getArrayType().getElementType() instanceof CharType and + ac.hasInitializer() + ) + } +} From b005b05da70f16c49b3c83c8e52a6e89ae4ee724 Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Tue, 12 Apr 2022 14:48:02 +0100 Subject: [PATCH 02/17] Fix metadata --- csharp/CWE-916/WeakIterations.ql | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/csharp/CWE-916/WeakIterations.ql b/csharp/CWE-916/WeakIterations.ql index cde06b1a50..ed9e030bfa 100644 --- a/csharp/CWE-916/WeakIterations.ql +++ b/csharp/CWE-916/WeakIterations.ql @@ -1,13 +1,13 @@ /** - * @name Hardcoded Salt - * @description Hardcoded Salt + * @name Use of Password Hash With Insufficient Computational Effort + * @description Use of Password Hash With Insufficient Computational Effort * @kind path-problem - * @problem.severity error - * @security-severity 6.1 + * @problem.severity warning + * @security-severity 4.0 * @precision medium - * @id cs/hardcoded-salt + * @id cs/weak-interations * @tags security - * external/cwe/cwe-760 + * external/cwe/cwe-916 */ import csharp From 8dd26cffc0a73fedd82dfbff8bef17fb3eb59f2e Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Tue, 12 Apr 2022 14:53:17 +0100 Subject: [PATCH 03/17] Update suite to include custom queries --- csharp/suites/codeql-csharp.qls | 2 ++ 1 file changed, 2 insertions(+) diff --git a/csharp/suites/codeql-csharp.qls b/csharp/suites/codeql-csharp.qls index 2ab0ad73ad..2bda14f1dc 100644 --- a/csharp/suites/codeql-csharp.qls +++ b/csharp/suites/codeql-csharp.qls @@ -3,6 +3,8 @@ - description: "GitHub's Field Team CodeQL CSharp extended Suite" +- qlpack: github-queries-csharp + - import: codeql-suites/csharp-security-extended.qls from: codeql/csharp-queries From 93afe9b66547fd935086286198a25753ed404019 Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Tue, 12 Apr 2022 14:54:43 +0100 Subject: [PATCH 04/17] Fix other hardcoded metadata and query info --- csharp/CWE-916/WeakIterations.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/csharp/CWE-916/WeakIterations.ql b/csharp/CWE-916/WeakIterations.ql index ed9e030bfa..348c5fc245 100644 --- a/csharp/CWE-916/WeakIterations.ql +++ b/csharp/CWE-916/WeakIterations.ql @@ -47,7 +47,7 @@ module HardcodedSalt { */ class TaintTrackingConfiguration extends TaintTracking::Configuration { - TaintTrackingConfiguration() { this = "HardcodedSalt" } + TaintTrackingConfiguration() { this = "WeakInteractions" } override predicate isSource(DataFlow::Node source) { source instanceof HardcodedSalt::Source } @@ -69,4 +69,4 @@ from HardcodedSalt::TaintTrackingConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink where config.hasFlowPath(source, sink) -select sink.getNode(), source, sink, "Use of $@.", source.getNode(), "hardcoded salt" +select sink.getNode(), source, sink, "Use of $@.", source.getNode(), "hardcoded weak iterations" From 34f12873da714372fbc1ca849bcd9e4768f83174 Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Tue, 12 Apr 2022 15:12:55 +0100 Subject: [PATCH 05/17] Update class name --- csharp/CWE-916/WeakIterations.ql | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/csharp/CWE-916/WeakIterations.ql b/csharp/CWE-916/WeakIterations.ql index 348c5fc245..65330c4ef6 100644 --- a/csharp/CWE-916/WeakIterations.ql +++ b/csharp/CWE-916/WeakIterations.ql @@ -17,7 +17,7 @@ private import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph private import github.hardcoded private import github.crypto -module HardcodedSalt { +module WeakIterations { abstract class Source extends DataFlow::ExprNode { } abstract class Sink extends DataFlow::ExprNode { } @@ -49,10 +49,10 @@ module HardcodedSalt { class TaintTrackingConfiguration extends TaintTracking::Configuration { TaintTrackingConfiguration() { this = "WeakInteractions" } - override predicate isSource(DataFlow::Node source) { source instanceof HardcodedSalt::Source } + override predicate isSource(DataFlow::Node source) { source instanceof WeakIterations::Source } override predicate isSink(DataFlow::Node sink) { - sink instanceof HardcodedSalt::Sink and + sink instanceof WeakIterations::Sink and not any(ReturnedByMockObject mock).getAMemberInitializationValue() = sink.asExpr() and not any(ReturnedByMockObject mock).getAnArgument() = sink.asExpr() } @@ -66,7 +66,7 @@ module HardcodedSalt { } from - HardcodedSalt::TaintTrackingConfiguration config, DataFlow::PathNode source, + WeakIterations::TaintTrackingConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink where config.hasFlowPath(source, sink) select sink.getNode(), source, sink, "Use of $@.", source.getNode(), "hardcoded weak iterations" From 7c2cd85afc2135e775b4ec4233b1259b7d1cc15d Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Tue, 12 Apr 2022 19:53:17 +0100 Subject: [PATCH 06/17] Better support for CSharp Crypto --- .../CWE-326/EncryptionInsufficientKeySize.ql | 20 ++++ csharp/CWE-326/EncryptionKeySizeToLarge.ql | 20 ++++ csharp/CWE-326/EncryptionKeySizeUseGetter.ql | 22 +++++ csharp/CWE-916/WeakIterations.ql | 2 +- csharp/github/crypto.qll | 97 +++++++++++++++++++ csharp/suites/codeql-csharp.qls | 3 + 6 files changed, 163 insertions(+), 1 deletion(-) create mode 100644 csharp/CWE-326/EncryptionInsufficientKeySize.ql create mode 100644 csharp/CWE-326/EncryptionKeySizeToLarge.ql create mode 100644 csharp/CWE-326/EncryptionKeySizeUseGetter.ql diff --git a/csharp/CWE-326/EncryptionInsufficientKeySize.ql b/csharp/CWE-326/EncryptionInsufficientKeySize.ql new file mode 100644 index 0000000000..8aab6ff7ab --- /dev/null +++ b/csharp/CWE-326/EncryptionInsufficientKeySize.ql @@ -0,0 +1,20 @@ +/** + * @name Weak encryption: Insufficient key size + * @description Finds uses of encryption algorithms with too small a key size + * @kind problem + * @problem.severity warning + * @security-severity 7.5 + * @precision high + * @id cs/insufficient-key-size + * @tags security + * external/cwe/cwe-326 + */ + +import csharp +import github.crypto + +from Crypto::SymmetricAlgorithms aglms, int key_size +where + key_size = aglms.getKeySize() and + key_size < aglms.minKeySize() +select aglms, "Key size " + key_size.toString() + " is to small (min: " + aglms.minKeySize() + ")" diff --git a/csharp/CWE-326/EncryptionKeySizeToLarge.ql b/csharp/CWE-326/EncryptionKeySizeToLarge.ql new file mode 100644 index 0000000000..9d01069672 --- /dev/null +++ b/csharp/CWE-326/EncryptionKeySizeToLarge.ql @@ -0,0 +1,20 @@ +/** + * @name Weak encryption: Insufficient key size + * @description Finds uses of encryption algorithms with too small a key size + * @kind problem + * @problem.severity note + * @precision high + * @id cs/insufficient-key-size + * @tags security + * external/cwe/cwe-326 + */ + +import csharp +import github.crypto + +from Crypto::SymmetricAlgorithms aglms, int key_size +where + key_size = aglms.getKeySize() and + key_size > aglms.maxKeySize() +select aglms, + "Key size " + key_size.toString() + " is to large for algorithm (max: " + aglms.maxKeySize() + ")" diff --git a/csharp/CWE-326/EncryptionKeySizeUseGetter.ql b/csharp/CWE-326/EncryptionKeySizeUseGetter.ql new file mode 100644 index 0000000000..ee6646d4c2 --- /dev/null +++ b/csharp/CWE-326/EncryptionKeySizeUseGetter.ql @@ -0,0 +1,22 @@ +/** + * @name Weak encryption: Insufficient key size + * @description Finds uses of encryption algorithms with too small a key size + * @kind problem + * @problem.severity warning + * @security-severity 2.5 + * @precision high + * @id cs/encryption-keysize-use-getter + * @tags security + * external/cwe/cwe-326 + */ + +import csharp +import github.crypto + +from Crypto::SymmetricAlgorithms aglms, PropertyAccess props, Expr expr +where + aglms.getType().hasName(["DSACryptoServiceProvider", "RSACryptoServiceProvider"]) and + props.getTarget().getDeclaringType() = aglms.getType() and + props.getTarget().getName() = "KeySize" and + expr = props.getParent().(Assignment).getRValue() +select expr, "Cannot use Getter to set key size" diff --git a/csharp/CWE-916/WeakIterations.ql b/csharp/CWE-916/WeakIterations.ql index 65330c4ef6..54042d8bc7 100644 --- a/csharp/CWE-916/WeakIterations.ql +++ b/csharp/CWE-916/WeakIterations.ql @@ -31,7 +31,7 @@ module WeakIterations { */ class Hardcoded extends Source { - Hardcoded() { this.getExpr().(IntLiteral).getValue().toInt() < 1000 } + Hardcoded() { this.getExpr().(IntLiteral).getValue().toInt() < 100000 } } /* diff --git a/csharp/github/crypto.qll b/csharp/github/crypto.qll index 2ffdfb9866..a706acb531 100644 --- a/csharp/github/crypto.qll +++ b/csharp/github/crypto.qll @@ -9,8 +9,20 @@ module Crypto { abstract DataFlow::ExprNode getIterations(); } + class SymmetricAlgorithm extends DataFlow::ExprNode { + abstract int maxKeySize(); + + abstract int minKeySize(); + + abstract int getKeySize(); + } + + // Abstraction classes abstract class HashingAlgorithms extends HashingAlgorithm { } + abstract class SymmetricAlgorithms extends SymmetricAlgorithm { } + + // Content class CryptoRfc2898DeriveBytes extends HashingAlgorithms { CryptoRfc2898DeriveBytes() { exists(ObjectCreation object | @@ -31,4 +43,89 @@ module Crypto { result.asExpr() = this.asExpr().(ObjectCreation).getArgument(2) } } + + class DSACryptoServiceProvider extends SymmetricAlgorithms { + DSACryptoServiceProvider() { + exists(ObjectCreation object | + object + .getType() + .hasQualifiedName("System.Security.Cryptography", "DSACryptoServiceProvider") and + this.asExpr() = object + ) + } + + override int maxKeySize() { result = 1024 } + + override int minKeySize() { result = 1024 } + + override int getKeySize() { + this.asExpr().(ObjectCreation).hasNoArguments() and + result = 1024 + or + // https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.dsacryptoserviceprovider.-ctor?view=net-6.0#system-security-cryptography-dsacryptoserviceprovider-ctor(system-int32) + result = this.asExpr().(ObjectCreation).getArgument(0).getValue().toInt() + } + } + + class RC2CryptoServiceProvider extends SymmetricAlgorithms { + RC2CryptoServiceProvider() { + exists(ObjectCreation object | + object + .getType() + .hasQualifiedName("System.Security.Cryptography", "RC2CryptoServiceProvider") and + this.asExpr() = object + ) + } + + override int maxKeySize() { result = 128 } + + override int minKeySize() { result = 128 } + + override int getKeySize() { + this.asExpr().(ObjectCreation).hasNoArguments() and + result = 128 // default + or + // https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.dsacryptoserviceprovider.-ctor?view=net-6.0#system-security-cryptography-dsacryptoserviceprovider-ctor(system-int32) + result = this.asExpr().(ObjectCreation).getArgument(0).getValue().toInt() + } + } + + class RSA extends SymmetricAlgorithms { + RSA() { + exists(ObjectCreation object | + object + .getType() + .hasQualifiedName("System.Security.Cryptography", ["RSACryptoServiceProvider", "RSACng"]) and + this.asExpr() = object + ) + or + exists(MethodCall call | + call.getType().hasQualifiedName("System.Security.Cryptography", ["RSA"]) and + call.getTarget().hasName("Create") and + this.asExpr() = call + ) + } + + override int maxKeySize() { result = 2048 } + + override int minKeySize() { result = 2048 } + + override int getKeySize() { + ( + // https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.dsacryptoserviceprovider.-ctor?view=net-6.0#system-security-cryptography-dsacryptoserviceprovider-ctor(system-int32) + this.asExpr().(ObjectCreation).hasNoArguments() and + result = 1024 + or + result = this.asExpr().(ObjectCreation).getArgument(0).getValue().toInt() + ) + or + ( + // https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.rsa.create?view=net-6.0#system-security-cryptography-rsa-create(system-int32) + this.asExpr().(MethodCall).hasNoArguments() and + result = 1024 + or + result = this.asExpr().(MethodCall).getArgument(0).getValue().toInt() + ) + } + } } diff --git a/csharp/suites/codeql-csharp.qls b/csharp/suites/codeql-csharp.qls index 2bda14f1dc..0f0db355bc 100644 --- a/csharp/suites/codeql-csharp.qls +++ b/csharp/suites/codeql-csharp.qls @@ -4,6 +4,9 @@ - description: "GitHub's Field Team CodeQL CSharp extended Suite" - qlpack: github-queries-csharp +- exclude: + tags contain: + - static - import: codeql-suites/csharp-security-extended.qls from: codeql/csharp-queries From 9d4c4bd2dd61212370ebba50ed4e193c54c5d580 Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Tue, 12 Apr 2022 20:23:51 +0100 Subject: [PATCH 07/17] Fix big typo --- csharp/CWE-326/EncryptionInsufficientKeySize.ql | 2 +- csharp/CWE-326/EncryptionKeySizeToLarge.ql | 2 +- csharp/CWE-326/EncryptionKeySizeUseGetter.ql | 2 +- csharp/github/crypto.qll | 10 +++++----- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/csharp/CWE-326/EncryptionInsufficientKeySize.ql b/csharp/CWE-326/EncryptionInsufficientKeySize.ql index 8aab6ff7ab..fc6b4b8b24 100644 --- a/csharp/CWE-326/EncryptionInsufficientKeySize.ql +++ b/csharp/CWE-326/EncryptionInsufficientKeySize.ql @@ -13,7 +13,7 @@ import csharp import github.crypto -from Crypto::SymmetricAlgorithms aglms, int key_size +from Crypto::AsymmetricAlgorithms aglms, int key_size where key_size = aglms.getKeySize() and key_size < aglms.minKeySize() diff --git a/csharp/CWE-326/EncryptionKeySizeToLarge.ql b/csharp/CWE-326/EncryptionKeySizeToLarge.ql index 9d01069672..6803977795 100644 --- a/csharp/CWE-326/EncryptionKeySizeToLarge.ql +++ b/csharp/CWE-326/EncryptionKeySizeToLarge.ql @@ -12,7 +12,7 @@ import csharp import github.crypto -from Crypto::SymmetricAlgorithms aglms, int key_size +from Crypto::AsymmetricAlgorithms aglms, int key_size where key_size = aglms.getKeySize() and key_size > aglms.maxKeySize() diff --git a/csharp/CWE-326/EncryptionKeySizeUseGetter.ql b/csharp/CWE-326/EncryptionKeySizeUseGetter.ql index ee6646d4c2..b8a42537c5 100644 --- a/csharp/CWE-326/EncryptionKeySizeUseGetter.ql +++ b/csharp/CWE-326/EncryptionKeySizeUseGetter.ql @@ -13,7 +13,7 @@ import csharp import github.crypto -from Crypto::SymmetricAlgorithms aglms, PropertyAccess props, Expr expr +from Crypto::AsymmetricAlgorithms aglms, PropertyAccess props, Expr expr where aglms.getType().hasName(["DSACryptoServiceProvider", "RSACryptoServiceProvider"]) and props.getTarget().getDeclaringType() = aglms.getType() and diff --git a/csharp/github/crypto.qll b/csharp/github/crypto.qll index a706acb531..8994211a94 100644 --- a/csharp/github/crypto.qll +++ b/csharp/github/crypto.qll @@ -9,7 +9,7 @@ module Crypto { abstract DataFlow::ExprNode getIterations(); } - class SymmetricAlgorithm extends DataFlow::ExprNode { + class AsymmetricAlgorithm extends DataFlow::ExprNode { abstract int maxKeySize(); abstract int minKeySize(); @@ -20,7 +20,7 @@ module Crypto { // Abstraction classes abstract class HashingAlgorithms extends HashingAlgorithm { } - abstract class SymmetricAlgorithms extends SymmetricAlgorithm { } + abstract class AsymmetricAlgorithms extends AsymmetricAlgorithm { } // Content class CryptoRfc2898DeriveBytes extends HashingAlgorithms { @@ -44,7 +44,7 @@ module Crypto { } } - class DSACryptoServiceProvider extends SymmetricAlgorithms { + class DSACryptoServiceProvider extends AsymmetricAlgorithms { DSACryptoServiceProvider() { exists(ObjectCreation object | object @@ -67,7 +67,7 @@ module Crypto { } } - class RC2CryptoServiceProvider extends SymmetricAlgorithms { + class RC2CryptoServiceProvider extends AsymmetricAlgorithms { RC2CryptoServiceProvider() { exists(ObjectCreation object | object @@ -90,7 +90,7 @@ module Crypto { } } - class RSA extends SymmetricAlgorithms { + class RSA extends AsymmetricAlgorithms { RSA() { exists(ObjectCreation object | object From 9c215ef5e982401c88e13b4ece581b38a6537441 Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Wed, 13 Apr 2022 12:21:27 +0100 Subject: [PATCH 08/17] Initial tests --- tests/.gitignore | 1 + .../csharp-tests/CWE-760/HardcodedSalt.expected | 0 tests/csharp-tests/CWE-760/HardcodedSalt.qlref | 1 + tests/csharp-tests/CWE-760/Test.cs | 16 ++++++++++++++++ tests/csharp-tests/CWE-760/options | 1 + tests/csharp-tests/qlpack.lock.yml | 4 ++++ tests/csharp-tests/qlpack.yml | 8 ++++++++ 7 files changed, 31 insertions(+) create mode 100644 tests/.gitignore create mode 100644 tests/csharp-tests/CWE-760/HardcodedSalt.expected create mode 100644 tests/csharp-tests/CWE-760/HardcodedSalt.qlref create mode 100644 tests/csharp-tests/CWE-760/Test.cs create mode 100644 tests/csharp-tests/CWE-760/options create mode 100644 tests/csharp-tests/qlpack.lock.yml create mode 100644 tests/csharp-tests/qlpack.yml diff --git a/tests/.gitignore b/tests/.gitignore new file mode 100644 index 0000000000..16a637d4a9 --- /dev/null +++ b/tests/.gitignore @@ -0,0 +1 @@ +**/*.testproj \ No newline at end of file diff --git a/tests/csharp-tests/CWE-760/HardcodedSalt.expected b/tests/csharp-tests/CWE-760/HardcodedSalt.expected new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/csharp-tests/CWE-760/HardcodedSalt.qlref b/tests/csharp-tests/CWE-760/HardcodedSalt.qlref new file mode 100644 index 0000000000..f8e5d2cdc8 --- /dev/null +++ b/tests/csharp-tests/CWE-760/HardcodedSalt.qlref @@ -0,0 +1 @@ +CWE-760/HardcodedSalt.ql \ No newline at end of file diff --git a/tests/csharp-tests/CWE-760/Test.cs b/tests/csharp-tests/CWE-760/Test.cs new file mode 100644 index 0000000000..66d99c3378 --- /dev/null +++ b/tests/csharp-tests/CWE-760/Test.cs @@ -0,0 +1,16 @@ +using System; +using System.IO; +using System.Text; +using System.Runtime.Serialization; +using System.Security.Cryptography; +using System.Security.Permissions; + +public class StaticSalt { + public void Test1() { + string password = "TestPassword"; + + // BAD: Static String + var salt = Encoding.UTF8.GetBytes("Hardcoded Salt"); + var hash = new Rfc2898DeriveBytes(password, salt); + } +} \ No newline at end of file diff --git a/tests/csharp-tests/CWE-760/options b/tests/csharp-tests/CWE-760/options new file mode 100644 index 0000000000..f2f776d118 --- /dev/null +++ b/tests/csharp-tests/CWE-760/options @@ -0,0 +1 @@ +semmle-extractor-options: /r:System.Security.Cryptography.Csp.dll /r:System.Security.Cryptography.Algorithms.dll diff --git a/tests/csharp-tests/qlpack.lock.yml b/tests/csharp-tests/qlpack.lock.yml new file mode 100644 index 0000000000..a046f6d978 --- /dev/null +++ b/tests/csharp-tests/qlpack.lock.yml @@ -0,0 +1,4 @@ +--- +dependencies: {} +compiled: false +lockVersion: 1.0.0 \ No newline at end of file diff --git a/tests/csharp-tests/qlpack.yml b/tests/csharp-tests/qlpack.yml new file mode 100644 index 0000000000..76b4d626f6 --- /dev/null +++ b/tests/csharp-tests/qlpack.yml @@ -0,0 +1,8 @@ +name: github-queries-csharp-tests +groups: [csharp, test] +dependencies: + codeql/csharp-all: "*" + github-queries-csharp: "*" + +extractor: csharp +tests: . \ No newline at end of file From e0b6a3d7e3f17bb25be5c53db0e7ca6f231f621d Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Wed, 13 Apr 2022 13:34:19 +0100 Subject: [PATCH 09/17] Additional Tests and updates --- tests/.gitignore | 3 ++- .../CWE-760/HardcodedSalt.expected | 18 +++++++++++++ tests/csharp-tests/CWE-760/Test.cs | 16 ++++++++++++ tests/csharp-tests/CWE-916/Test.cs | 26 +++++++++++++++++++ .../CWE-916/WeakIterations.expected | 6 +++++ .../csharp-tests/CWE-916/WeakIterations.qlref | 1 + tests/csharp-tests/CWE-916/options | 1 + 7 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 tests/csharp-tests/CWE-916/Test.cs create mode 100644 tests/csharp-tests/CWE-916/WeakIterations.expected create mode 100644 tests/csharp-tests/CWE-916/WeakIterations.qlref create mode 100644 tests/csharp-tests/CWE-916/options diff --git a/tests/.gitignore b/tests/.gitignore index 16a637d4a9..42070f13d9 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -1 +1,2 @@ -**/*.testproj \ No newline at end of file +**/*.testproj +**/*.actual diff --git a/tests/csharp-tests/CWE-760/HardcodedSalt.expected b/tests/csharp-tests/CWE-760/HardcodedSalt.expected index e69de29bb2..bdd849652e 100644 --- a/tests/csharp-tests/CWE-760/HardcodedSalt.expected +++ b/tests/csharp-tests/CWE-760/HardcodedSalt.expected @@ -0,0 +1,18 @@ +edges +| Test.cs:13:16:13:55 | call to method GetBytes : Byte[] | Test.cs:14:49:14:52 | access to local variable salt | +| Test.cs:13:39:13:54 | "Hardcoded Salt" : String | Test.cs:13:16:13:55 | call to method GetBytes : Byte[] | +| Test.cs:23:12:23:28 | "Hardcoded Salt2" : String | Test.cs:29:39:29:63 | call to method generateSalt : String | +| Test.cs:29:16:29:64 | call to method GetBytes : Byte[] | Test.cs:30:49:30:52 | access to local variable salt | +| Test.cs:29:39:29:63 | call to method generateSalt : String | Test.cs:29:16:29:64 | call to method GetBytes : Byte[] | +nodes +| Test.cs:13:16:13:55 | call to method GetBytes : Byte[] | semmle.label | call to method GetBytes : Byte[] | +| Test.cs:13:39:13:54 | "Hardcoded Salt" : String | semmle.label | "Hardcoded Salt" : String | +| Test.cs:14:49:14:52 | access to local variable salt | semmle.label | access to local variable salt | +| Test.cs:23:12:23:28 | "Hardcoded Salt2" : String | semmle.label | "Hardcoded Salt2" : String | +| Test.cs:29:16:29:64 | call to method GetBytes : Byte[] | semmle.label | call to method GetBytes : Byte[] | +| Test.cs:29:39:29:63 | call to method generateSalt : String | semmle.label | call to method generateSalt : String | +| Test.cs:30:49:30:52 | access to local variable salt | semmle.label | access to local variable salt | +subpaths +#select +| Test.cs:14:49:14:52 | access to local variable salt | Test.cs:13:39:13:54 | "Hardcoded Salt" : String | Test.cs:14:49:14:52 | access to local variable salt | Use of $@. | Test.cs:13:39:13:54 | "Hardcoded Salt" | hardcoded salt | +| Test.cs:30:49:30:52 | access to local variable salt | Test.cs:23:12:23:28 | "Hardcoded Salt2" : String | Test.cs:30:49:30:52 | access to local variable salt | Use of $@. | Test.cs:23:12:23:28 | "Hardcoded Salt2" | hardcoded salt | diff --git a/tests/csharp-tests/CWE-760/Test.cs b/tests/csharp-tests/CWE-760/Test.cs index 66d99c3378..a68507404f 100644 --- a/tests/csharp-tests/CWE-760/Test.cs +++ b/tests/csharp-tests/CWE-760/Test.cs @@ -12,5 +12,21 @@ public void Test1() { // BAD: Static String var salt = Encoding.UTF8.GetBytes("Hardcoded Salt"); var hash = new Rfc2898DeriveBytes(password, salt); + + // Good: Randomly generated byte array + var randonSalt = new byte[16]; + RandomNumberGenerator.Create().GetBytes(randonSalt); + var hash_safe = new Rfc2898DeriveBytes(password, randonSalt); + } + + public static string generateSalt() { + return "Hardcoded Salt2"; + } + public void Test2() { + string password = "TestPassword2"; + + // BAD: Static String + var salt = Encoding.UTF8.GetBytes(StaticSalt.generateSalt()); + var hash = new Rfc2898DeriveBytes(password, salt); } } \ No newline at end of file diff --git a/tests/csharp-tests/CWE-916/Test.cs b/tests/csharp-tests/CWE-916/Test.cs new file mode 100644 index 0000000000..e15c5785b4 --- /dev/null +++ b/tests/csharp-tests/CWE-916/Test.cs @@ -0,0 +1,26 @@ +using System; +using System.IO; +using System.Text; +using System.Runtime.Serialization; +using System.Security.Cryptography; +using System.Security.Permissions; + +public class StaticSalt { + public void Test1() { + string password = "TestPassword"; + var randonSalt = new byte[16]; + RandomNumberGenerator.Create().GetBytes(randonSalt); + + + // BAD: Default (1000) + var hash = new Rfc2898DeriveBytes(password, randonSalt); + + // BAD: Static int + var hash2 = new Rfc2898DeriveBytes(password, randonSalt, 1000); + + + + // Good: High interations + var hash_safe = new Rfc2898DeriveBytes(password, randonSalt, 100000); + } +} \ No newline at end of file diff --git a/tests/csharp-tests/CWE-916/WeakIterations.expected b/tests/csharp-tests/CWE-916/WeakIterations.expected new file mode 100644 index 0000000000..c41066ae3a --- /dev/null +++ b/tests/csharp-tests/CWE-916/WeakIterations.expected @@ -0,0 +1,6 @@ +edges +nodes +| Test.cs:19:62:19:65 | 1000 | semmle.label | 1000 | +subpaths +#select +| Test.cs:19:62:19:65 | 1000 | Test.cs:19:62:19:65 | 1000 | Test.cs:19:62:19:65 | 1000 | Use of $@. | Test.cs:19:62:19:65 | 1000 | hardcoded weak iterations | diff --git a/tests/csharp-tests/CWE-916/WeakIterations.qlref b/tests/csharp-tests/CWE-916/WeakIterations.qlref new file mode 100644 index 0000000000..194cb7eaf3 --- /dev/null +++ b/tests/csharp-tests/CWE-916/WeakIterations.qlref @@ -0,0 +1 @@ +CWE-916/WeakIterations.ql \ No newline at end of file diff --git a/tests/csharp-tests/CWE-916/options b/tests/csharp-tests/CWE-916/options new file mode 100644 index 0000000000..f2f776d118 --- /dev/null +++ b/tests/csharp-tests/CWE-916/options @@ -0,0 +1 @@ +semmle-extractor-options: /r:System.Security.Cryptography.Csp.dll /r:System.Security.Cryptography.Algorithms.dll From 62ac79e05ec4efb916c5c8019133eed9e133b589 Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Wed, 13 Apr 2022 14:03:11 +0100 Subject: [PATCH 10/17] Add tests for CWE-326 queries --- .../EncryptionInsuffientKeySize/KeySize.cs | 42 +++++++++++++++++++ .../KeySize.expected | 4 ++ .../EncryptionInsuffientKeySize/KeySize.qlref | 1 + .../EncryptionInsuffientKeySize/options | 1 + .../EncryptionKeySizeToLarge/KeySizeLarge.cs | 18 ++++++++ .../KeySizeLarge.expected | 1 + .../KeySizeLarge.qlref | 1 + .../CWE-326/EncryptionKeySizeToLarge/options | 1 + .../KeySizeGetter.cs | 23 ++++++++++ .../KeySizeGetter.expected | 2 + .../KeySizeGetter.qlref | 1 + .../EncryptionKeySizeUseGetter/options | 1 + 12 files changed, 96 insertions(+) create mode 100644 tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/KeySize.cs create mode 100644 tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/KeySize.expected create mode 100644 tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/KeySize.qlref create mode 100644 tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/options create mode 100644 tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/KeySizeLarge.cs create mode 100644 tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/KeySizeLarge.expected create mode 100644 tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/KeySizeLarge.qlref create mode 100644 tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/options create mode 100644 tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/KeySizeGetter.cs create mode 100644 tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/KeySizeGetter.expected create mode 100644 tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/KeySizeGetter.qlref create mode 100644 tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/options diff --git a/tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/KeySize.cs b/tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/KeySize.cs new file mode 100644 index 0000000000..11c7c291b8 --- /dev/null +++ b/tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/KeySize.cs @@ -0,0 +1,42 @@ +using System; +using System.IO; +using System.Text; +using System.Runtime.Serialization; +using System.Security.Cryptography; + +public class StaticSalt +{ + // public void Test1() + // { + // // + // var dsa1 = new DSACryptoServiceProvider(); + + // // + // var dsa2 = new DSACryptoServiceProvider(1024); + + // } + + public void TestRSA() + { + // BAD: Default is insecure + var rsa1 = new RSACryptoServiceProvider(); + var rsa2 = new RSACryptoServiceProvider(1024); + + int key_size = 1024; + var rsa3 = new RSACryptoServiceProvider(key_size); + + // Good + var rsa_good = new RSACryptoServiceProvider(2048); + } + + public void TestRSACng() + { + + // BAD: Default is insecure + var rsacng = new RSACng(); + var rsacng2 = new RSACng(1024); + + // GOOD: High + var rsacng_good2 = new RSACng(2048); + } +} \ No newline at end of file diff --git a/tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/KeySize.expected b/tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/KeySize.expected new file mode 100644 index 0000000000..3eecc9384d --- /dev/null +++ b/tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/KeySize.expected @@ -0,0 +1,4 @@ +| KeySize.cs:22:20:22:49 | object creation of type RSACryptoServiceProvider | Key size 1024 is to small (min: 2048) | +| KeySize.cs:23:20:23:53 | object creation of type RSACryptoServiceProvider | Key size 1024 is to small (min: 2048) | +| KeySize.cs:36:22:36:33 | object creation of type RSACng | Key size 1024 is to small (min: 2048) | +| KeySize.cs:37:23:37:38 | object creation of type RSACng | Key size 1024 is to small (min: 2048) | diff --git a/tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/KeySize.qlref b/tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/KeySize.qlref new file mode 100644 index 0000000000..92bf21e7b2 --- /dev/null +++ b/tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/KeySize.qlref @@ -0,0 +1 @@ +CWE-326/EncryptionInsufficientKeySize.ql \ No newline at end of file diff --git a/tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/options b/tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/options new file mode 100644 index 0000000000..be5e69bbe8 --- /dev/null +++ b/tests/csharp-tests/CWE-326/EncryptionInsuffientKeySize/options @@ -0,0 +1 @@ +semmle-extractor-options: /r:System.Security.Cryptography.Csp.dll /r:System.Security.Cryptography.Algorithms.dll /r:System.Security.Cryptography.Cng.dll diff --git a/tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/KeySizeLarge.cs b/tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/KeySizeLarge.cs new file mode 100644 index 0000000000..3c72fa046b --- /dev/null +++ b/tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/KeySizeLarge.cs @@ -0,0 +1,18 @@ +using System; +using System.IO; +using System.Text; +using System.Runtime.Serialization; +using System.Security.Cryptography; + +public class StaticSalt +{ + public void Test1() + { + // BAD + var dsa1 = new DSACryptoServiceProvider(2042); + + // GOOD + var dsa2 = new DSACryptoServiceProvider(1024); + + } +} \ No newline at end of file diff --git a/tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/KeySizeLarge.expected b/tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/KeySizeLarge.expected new file mode 100644 index 0000000000..3cdece03d8 --- /dev/null +++ b/tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/KeySizeLarge.expected @@ -0,0 +1 @@ +| KeySizeLarge.cs:12:20:12:53 | object creation of type DSACryptoServiceProvider | Key size 2042 is to large for algorithm (max: 1024) | diff --git a/tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/KeySizeLarge.qlref b/tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/KeySizeLarge.qlref new file mode 100644 index 0000000000..d68a4a4b46 --- /dev/null +++ b/tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/KeySizeLarge.qlref @@ -0,0 +1 @@ +CWE-326/EncryptionKeySizeToLarge.ql \ No newline at end of file diff --git a/tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/options b/tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/options new file mode 100644 index 0000000000..be5e69bbe8 --- /dev/null +++ b/tests/csharp-tests/CWE-326/EncryptionKeySizeToLarge/options @@ -0,0 +1 @@ +semmle-extractor-options: /r:System.Security.Cryptography.Csp.dll /r:System.Security.Cryptography.Algorithms.dll /r:System.Security.Cryptography.Cng.dll diff --git a/tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/KeySizeGetter.cs b/tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/KeySizeGetter.cs new file mode 100644 index 0000000000..4d7b621ee3 --- /dev/null +++ b/tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/KeySizeGetter.cs @@ -0,0 +1,23 @@ +using System; +using System.IO; +using System.Text; +using System.Runtime.Serialization; +using System.Security.Cryptography; + +public class StaticSalt +{ + public void Test1() + { + // BAD + var dsa1 = new DSACryptoServiceProvider(); + dsa1.KeySize = 2048; + + // BAD + var rsa1 = new RSACryptoServiceProvider(); + rsa1.KeySize = 2048; + + // GOOD + var dsa2 = new DSACryptoServiceProvider(1024); + + } +} \ No newline at end of file diff --git a/tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/KeySizeGetter.expected b/tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/KeySizeGetter.expected new file mode 100644 index 0000000000..b3bd1fa3f1 --- /dev/null +++ b/tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/KeySizeGetter.expected @@ -0,0 +1,2 @@ +| KeySizeGetter.cs:13:24:13:27 | 2048 | Cannot use Getter to set key size | +| KeySizeGetter.cs:17:24:17:27 | 2048 | Cannot use Getter to set key size | diff --git a/tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/KeySizeGetter.qlref b/tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/KeySizeGetter.qlref new file mode 100644 index 0000000000..e3d861365f --- /dev/null +++ b/tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/KeySizeGetter.qlref @@ -0,0 +1 @@ +CWE-326/EncryptionKeySizeUseGetter.ql \ No newline at end of file diff --git a/tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/options b/tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/options new file mode 100644 index 0000000000..be5e69bbe8 --- /dev/null +++ b/tests/csharp-tests/CWE-326/EncryptionKeySizeUseGetter/options @@ -0,0 +1 @@ +semmle-extractor-options: /r:System.Security.Cryptography.Csp.dll /r:System.Security.Cryptography.Algorithms.dll /r:System.Security.Cryptography.Cng.dll From c048d0f6b2dde21ca1a457229eaa9f7cdf5d998f Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Wed, 13 Apr 2022 14:16:05 +0100 Subject: [PATCH 11/17] Update CWE-916 --- csharp/CWE-916/WeakIterations.ql | 2 +- tests/csharp-tests/CWE-916/Test.cs | 32 +++++++++++-------- .../CWE-916/WeakIterations.expected | 8 +++-- 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/csharp/CWE-916/WeakIterations.ql b/csharp/CWE-916/WeakIterations.ql index 54042d8bc7..0f73b92ca6 100644 --- a/csharp/CWE-916/WeakIterations.ql +++ b/csharp/CWE-916/WeakIterations.ql @@ -47,7 +47,7 @@ module WeakIterations { */ class TaintTrackingConfiguration extends TaintTracking::Configuration { - TaintTrackingConfiguration() { this = "WeakInteractions" } + TaintTrackingConfiguration() { this = "WeakIteration" } override predicate isSource(DataFlow::Node source) { source instanceof WeakIterations::Source } diff --git a/tests/csharp-tests/CWE-916/Test.cs b/tests/csharp-tests/CWE-916/Test.cs index e15c5785b4..dbccc1bebf 100644 --- a/tests/csharp-tests/CWE-916/Test.cs +++ b/tests/csharp-tests/CWE-916/Test.cs @@ -5,22 +5,28 @@ using System.Security.Cryptography; using System.Security.Permissions; -public class StaticSalt { - public void Test1() { - string password = "TestPassword"; - var randonSalt = new byte[16]; - RandomNumberGenerator.Create().GetBytes(randonSalt); +public class StaticSalt +{ + public void Test1() + { + string password = "TestPassword"; + var randonSalt = new byte[16]; + RandomNumberGenerator.Create().GetBytes(randonSalt); - // BAD: Default (1000) - var hash = new Rfc2898DeriveBytes(password, randonSalt); + // BAD: Default (1000) + var hash = new Rfc2898DeriveBytes(password, randonSalt); - // BAD: Static int - var hash2 = new Rfc2898DeriveBytes(password, randonSalt, 1000); + // BAD: Static int + var hash2 = new Rfc2898DeriveBytes(password, randonSalt, 1000); - + // BAD: Static int + int interations = 1000; + var hash3 = new Rfc2898DeriveBytes(password, randonSalt, interations); - // Good: High interations - var hash_safe = new Rfc2898DeriveBytes(password, randonSalt, 100000); - } + + + // Good: High interations + var hash_safe = new Rfc2898DeriveBytes(password, randonSalt, 100000); + } } \ No newline at end of file diff --git a/tests/csharp-tests/CWE-916/WeakIterations.expected b/tests/csharp-tests/CWE-916/WeakIterations.expected index c41066ae3a..1bddf48af0 100644 --- a/tests/csharp-tests/CWE-916/WeakIterations.expected +++ b/tests/csharp-tests/CWE-916/WeakIterations.expected @@ -1,6 +1,10 @@ edges +| Test.cs:24:27:24:30 | 1000 : Int32 | Test.cs:25:66:25:76 | access to local variable interations | nodes -| Test.cs:19:62:19:65 | 1000 | semmle.label | 1000 | +| Test.cs:21:66:21:69 | 1000 | semmle.label | 1000 | +| Test.cs:24:27:24:30 | 1000 : Int32 | semmle.label | 1000 : Int32 | +| Test.cs:25:66:25:76 | access to local variable interations | semmle.label | access to local variable interations | subpaths #select -| Test.cs:19:62:19:65 | 1000 | Test.cs:19:62:19:65 | 1000 | Test.cs:19:62:19:65 | 1000 | Use of $@. | Test.cs:19:62:19:65 | 1000 | hardcoded weak iterations | +| Test.cs:21:66:21:69 | 1000 | Test.cs:21:66:21:69 | 1000 | Test.cs:21:66:21:69 | 1000 | Use of $@. | Test.cs:21:66:21:69 | 1000 | hardcoded weak iterations | +| Test.cs:25:66:25:76 | access to local variable interations | Test.cs:24:27:24:30 | 1000 : Int32 | Test.cs:25:66:25:76 | access to local variable interations | Use of $@. | Test.cs:24:27:24:30 | 1000 | hardcoded weak iterations | From c48db6f4d79210415a058fb55ed5025f8e605518 Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Wed, 13 Apr 2022 18:53:35 +0100 Subject: [PATCH 12/17] Update Weak Iterations query --- csharp/CWE-916/WeakIterations.ql | 13 ++++++++++++- csharp/github/crypto.qll | 10 ++++++++++ tests/csharp-tests/CWE-916/WeakIterations.expected | 2 ++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/csharp/CWE-916/WeakIterations.ql b/csharp/CWE-916/WeakIterations.ql index 0f73b92ca6..7e19777fa4 100644 --- a/csharp/CWE-916/WeakIterations.ql +++ b/csharp/CWE-916/WeakIterations.ql @@ -5,7 +5,7 @@ * @problem.severity warning * @security-severity 4.0 * @precision medium - * @id cs/weak-interations + * @id cs/weak-hash-algorithms-iterations * @tags security * external/cwe/cwe-916 */ @@ -34,6 +34,17 @@ module WeakIterations { Hardcoded() { this.getExpr().(IntLiteral).getValue().toInt() < 100000 } } + class DefaultSettings extends Source { + // TODO: This needs to be FAR better + DefaultSettings() { + exists(Crypto::HashingAlgorithms hash | + hash.getExpr().(ObjectCreation).getNumberOfArguments() <= 2 and + hash.defaultIterations() < 10000 and + this.asExpr() = hash.getExpr() + ) + } + } + /* * Sinks */ diff --git a/csharp/github/crypto.qll b/csharp/github/crypto.qll index 8994211a94..78b2fd06d4 100644 --- a/csharp/github/crypto.qll +++ b/csharp/github/crypto.qll @@ -6,6 +6,8 @@ module Crypto { abstract DataFlow::ExprNode getSalt(); + abstract int defaultIterations(); + abstract DataFlow::ExprNode getIterations(); } @@ -39,8 +41,16 @@ module Crypto { result.asExpr() = this.asExpr().(ObjectCreation).getArgument(1) } + override int defaultIterations() { result = 1000 } + override DataFlow::ExprNode getIterations() { result.asExpr() = this.asExpr().(ObjectCreation).getArgument(2) + or + // TODO: It this the best way? We need a better way of determinding + // iterations isn't set. + this.getExpr().(ObjectCreation).getNumberOfArguments() <= 2 and + this.defaultIterations() < 100000 and + result.asExpr() = this.getExpr() } } diff --git a/tests/csharp-tests/CWE-916/WeakIterations.expected b/tests/csharp-tests/CWE-916/WeakIterations.expected index 1bddf48af0..f8cc1d094d 100644 --- a/tests/csharp-tests/CWE-916/WeakIterations.expected +++ b/tests/csharp-tests/CWE-916/WeakIterations.expected @@ -1,10 +1,12 @@ edges | Test.cs:24:27:24:30 | 1000 : Int32 | Test.cs:25:66:25:76 | access to local variable interations | nodes +| Test.cs:18:20:18:63 | object creation of type Rfc2898DeriveBytes | semmle.label | object creation of type Rfc2898DeriveBytes | | Test.cs:21:66:21:69 | 1000 | semmle.label | 1000 | | Test.cs:24:27:24:30 | 1000 : Int32 | semmle.label | 1000 : Int32 | | Test.cs:25:66:25:76 | access to local variable interations | semmle.label | access to local variable interations | subpaths #select +| Test.cs:18:20:18:63 | object creation of type Rfc2898DeriveBytes | Test.cs:18:20:18:63 | object creation of type Rfc2898DeriveBytes | Test.cs:18:20:18:63 | object creation of type Rfc2898DeriveBytes | Use of $@. | Test.cs:18:20:18:63 | object creation of type Rfc2898DeriveBytes | hardcoded weak iterations | | Test.cs:21:66:21:69 | 1000 | Test.cs:21:66:21:69 | 1000 | Test.cs:21:66:21:69 | 1000 | Use of $@. | Test.cs:21:66:21:69 | 1000 | hardcoded weak iterations | | Test.cs:25:66:25:76 | access to local variable interations | Test.cs:24:27:24:30 | 1000 : Int32 | Test.cs:25:66:25:76 | access to local variable interations | Use of $@. | Test.cs:24:27:24:30 | 1000 | hardcoded weak iterations | From 1a513bc4a312478563bd11d000c462aafe2a26ec Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Wed, 13 Apr 2022 19:36:34 +0100 Subject: [PATCH 13/17] Add Weak HMac query --- csharp/CWE-327/WeakHMac.ql | 20 ++++++++++++++ csharp/github/crypto.qll | 28 +++++++++++++++++++ tests/csharp-tests/CWE-327/WeakHmac.cs | 29 ++++++++++++++++++++ tests/csharp-tests/CWE-327/WeakHmac.expected | 8 ++++++ tests/csharp-tests/CWE-327/WeakHmac.qlref | 1 + tests/csharp-tests/CWE-327/options | 1 + 6 files changed, 87 insertions(+) create mode 100644 csharp/CWE-327/WeakHMac.ql create mode 100644 tests/csharp-tests/CWE-327/WeakHmac.cs create mode 100644 tests/csharp-tests/CWE-327/WeakHmac.expected create mode 100644 tests/csharp-tests/CWE-327/WeakHmac.qlref create mode 100644 tests/csharp-tests/CWE-327/options diff --git a/csharp/CWE-327/WeakHMac.ql b/csharp/CWE-327/WeakHMac.ql new file mode 100644 index 0000000000..e7b3daed63 --- /dev/null +++ b/csharp/CWE-327/WeakHMac.ql @@ -0,0 +1,20 @@ +/** + * @name Use of Weak Hmac Algorithms + * @description Use of Weak Hmac Algorithms + * @kind problem + * @problem.severity warning + * @security-severity 4.0 + * @precision medium + * @id cs/weak-hmac-algorithm + * @tags security + * external/cwe/cwe-327 + */ + +import csharp +private import semmle.code.csharp.frameworks.Moq +private import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph +private import github.crypto + +from Crypto::HMacSigningAlgorithms algorithms +where algorithms.algorithm() = ["MD5", "SHA1"] +select algorithms, "Weak Hmac Algorithms" diff --git a/csharp/github/crypto.qll b/csharp/github/crypto.qll index 78b2fd06d4..5fb4fb5c5f 100644 --- a/csharp/github/crypto.qll +++ b/csharp/github/crypto.qll @@ -11,6 +11,12 @@ module Crypto { abstract DataFlow::ExprNode getIterations(); } + class HMacSigningAlgorithm extends DataFlow::ExprNode { + abstract string algorithm(); + + abstract DataFlow::ExprNode key(); + } + class AsymmetricAlgorithm extends DataFlow::ExprNode { abstract int maxKeySize(); @@ -22,6 +28,8 @@ module Crypto { // Abstraction classes abstract class HashingAlgorithms extends HashingAlgorithm { } + abstract class HMacSigningAlgorithms extends HMacSigningAlgorithm { } + abstract class AsymmetricAlgorithms extends AsymmetricAlgorithm { } // Content @@ -138,4 +146,24 @@ module Crypto { ) } } + + class HMac extends HMacSigningAlgorithms { + HMac() { + exists(ObjectCreation object | + object + .getType() + .hasQualifiedName("System.Security.Cryptography", + ["HMACMD5", "HMACSHA1", "HMACSHA256", "HMACSHA384", "HMACSHA512", "HMACRIPEMD160"]) and + this.asExpr() = object + ) + } + + override string algorithm() { + result = this.getType().getName().toUpperCase().replaceAll("HMAC", "") + } + + override DataFlow::ExprNode key() { + result.asExpr() = this.asExpr().(ObjectCreation).getArgument(0) + } + } } diff --git a/tests/csharp-tests/CWE-327/WeakHmac.cs b/tests/csharp-tests/CWE-327/WeakHmac.cs new file mode 100644 index 0000000000..68a926e2df --- /dev/null +++ b/tests/csharp-tests/CWE-327/WeakHmac.cs @@ -0,0 +1,29 @@ +using System; +using System.IO; +using System.Text; +using System.Runtime.Serialization; +using System.Security.Cryptography; +using System.Security.Permissions; + +public class StaticSalt +{ + public void Test1() + { + var key = Encoding.UTF8.GetBytes("TestPassword"); + + // BAD: MD5 + var md5 = new HMACMD5(); + var md5_1 = new HMACMD5(key); + + // BAD: SHA1 (not really but worth reporting) + var sha1 = new HMACSHA1(); + var sha1_2 = new HMACSHA1(key); + + // GOOD: SHA256 + var sha2 = new HMACSHA256(key); + // GOOD: SHA384 + var sha3 = new HMACSHA384(key); + // GOOD: SHA512 + var sha5 = new HMACSHA512(key); + } +} \ No newline at end of file diff --git a/tests/csharp-tests/CWE-327/WeakHmac.expected b/tests/csharp-tests/CWE-327/WeakHmac.expected new file mode 100644 index 0000000000..54a473dd41 --- /dev/null +++ b/tests/csharp-tests/CWE-327/WeakHmac.expected @@ -0,0 +1,8 @@ +edges +nodes +subpaths +#select +| WeakHmac.cs:15:19:15:31 | object creation of type HMACMD5 | Algs | +| WeakHmac.cs:16:21:16:36 | object creation of type HMACMD5 | Algs | +| WeakHmac.cs:19:20:19:33 | object creation of type HMACSHA1 | Algs | +| WeakHmac.cs:20:22:20:38 | object creation of type HMACSHA1 | Algs | diff --git a/tests/csharp-tests/CWE-327/WeakHmac.qlref b/tests/csharp-tests/CWE-327/WeakHmac.qlref new file mode 100644 index 0000000000..4e13afb2e2 --- /dev/null +++ b/tests/csharp-tests/CWE-327/WeakHmac.qlref @@ -0,0 +1 @@ +CWE-327/WeakHMac.ql \ No newline at end of file diff --git a/tests/csharp-tests/CWE-327/options b/tests/csharp-tests/CWE-327/options new file mode 100644 index 0000000000..f2f776d118 --- /dev/null +++ b/tests/csharp-tests/CWE-327/options @@ -0,0 +1 @@ +semmle-extractor-options: /r:System.Security.Cryptography.Csp.dll /r:System.Security.Cryptography.Algorithms.dll From 7f61526ec06f03bf444161cc2cfe8486c2c80d62 Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Wed, 13 Apr 2022 19:53:14 +0100 Subject: [PATCH 14/17] Fix metadata --- csharp/CWE-326/EncryptionKeySizeToLarge.ql | 3 ++- csharp/CWE-326/EncryptionKeySizeUseGetter.ql | 2 +- csharp/CWE-327/WeakHMac.ql | 1 - 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/csharp/CWE-326/EncryptionKeySizeToLarge.ql b/csharp/CWE-326/EncryptionKeySizeToLarge.ql index 6803977795..36366a6e62 100644 --- a/csharp/CWE-326/EncryptionKeySizeToLarge.ql +++ b/csharp/CWE-326/EncryptionKeySizeToLarge.ql @@ -2,7 +2,8 @@ * @name Weak encryption: Insufficient key size * @description Finds uses of encryption algorithms with too small a key size * @kind problem - * @problem.severity note + * @problem.severity warning + * @security-severity 1.0 * @precision high * @id cs/insufficient-key-size * @tags security diff --git a/csharp/CWE-326/EncryptionKeySizeUseGetter.ql b/csharp/CWE-326/EncryptionKeySizeUseGetter.ql index b8a42537c5..4ed71c66bf 100644 --- a/csharp/CWE-326/EncryptionKeySizeUseGetter.ql +++ b/csharp/CWE-326/EncryptionKeySizeUseGetter.ql @@ -3,7 +3,7 @@ * @description Finds uses of encryption algorithms with too small a key size * @kind problem * @problem.severity warning - * @security-severity 2.5 + * @security-severity 1.0 * @precision high * @id cs/encryption-keysize-use-getter * @tags security diff --git a/csharp/CWE-327/WeakHMac.ql b/csharp/CWE-327/WeakHMac.ql index e7b3daed63..a40c9b69bd 100644 --- a/csharp/CWE-327/WeakHMac.ql +++ b/csharp/CWE-327/WeakHMac.ql @@ -11,7 +11,6 @@ */ import csharp -private import semmle.code.csharp.frameworks.Moq private import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph private import github.crypto From d3af1c05cd23dc0178299bbc576bf0fc668e91ac Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Wed, 13 Apr 2022 20:05:23 +0100 Subject: [PATCH 15/17] Fix imports --- csharp/CWE-327/WeakHMac.ql | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/csharp/CWE-327/WeakHMac.ql b/csharp/CWE-327/WeakHMac.ql index a40c9b69bd..b0fee4dd82 100644 --- a/csharp/CWE-327/WeakHMac.ql +++ b/csharp/CWE-327/WeakHMac.ql @@ -11,8 +11,7 @@ */ import csharp -private import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph -private import github.crypto +import github.crypto from Crypto::HMacSigningAlgorithms algorithms where algorithms.algorithm() = ["MD5", "SHA1"] From 69433aca4fe218e242109f2e15d27f09f3fd120e Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Wed, 13 Apr 2022 20:23:02 +0100 Subject: [PATCH 16/17] Add experimental query to suite --- csharp/suites/codeql-csharp.qls | 1 + 1 file changed, 1 insertion(+) diff --git a/csharp/suites/codeql-csharp.qls b/csharp/suites/codeql-csharp.qls index 0f0db355bc..40f9e50570 100644 --- a/csharp/suites/codeql-csharp.qls +++ b/csharp/suites/codeql-csharp.qls @@ -18,6 +18,7 @@ - cs/webclient-path-injection - cs/web/cookie-httponly-not-set - cs/web/cookie-secure-not-set + - cs/hash-without-salt # Backdoor queries - cs/backdoor/dangerous-native-functions - cs/backdoor/potential-time-bomb From 688e3ef5920cb58f7f05900f968d91b38295027f Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Thu, 14 Apr 2022 10:42:06 +0100 Subject: [PATCH 17/17] Update name and description --- csharp/CWE-326/EncryptionKeySizeToLarge.ql | 4 ++-- csharp/CWE-326/EncryptionKeySizeUseGetter.ql | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/csharp/CWE-326/EncryptionKeySizeToLarge.ql b/csharp/CWE-326/EncryptionKeySizeToLarge.ql index 36366a6e62..dffe0e89ae 100644 --- a/csharp/CWE-326/EncryptionKeySizeToLarge.ql +++ b/csharp/CWE-326/EncryptionKeySizeToLarge.ql @@ -1,6 +1,6 @@ /** - * @name Weak encryption: Insufficient key size - * @description Finds uses of encryption algorithms with too small a key size + * @name Weak encryption: Key size is too large for algorithm + * @description Finds uses of encryption algorithms with too large of a key size * @kind problem * @problem.severity warning * @security-severity 1.0 diff --git a/csharp/CWE-326/EncryptionKeySizeUseGetter.ql b/csharp/CWE-326/EncryptionKeySizeUseGetter.ql index 4ed71c66bf..9a9f9402a2 100644 --- a/csharp/CWE-326/EncryptionKeySizeUseGetter.ql +++ b/csharp/CWE-326/EncryptionKeySizeUseGetter.ql @@ -1,6 +1,6 @@ /** - * @name Weak encryption: Insufficient key size - * @description Finds uses of encryption algorithms with too small a key size + * @name Weak encryption: Key size set using a Getter and failing to set key size + * @description Finds uses of encryption algorithms using a Getter and not setting the size of the key * @kind problem * @problem.severity warning * @security-severity 1.0