From c720fb2c341a456503ada2a7fff5ba45aec385a1 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Tue, 16 Apr 2024 15:25:56 +0200 Subject: [PATCH 1/6] C#: Add HtmlString test, which is supported as it is a known sink defined in QL. --- .../Telemetry/SupportedExternalApis/SupportedExternalApis.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/csharp/ql/test/query-tests/Telemetry/SupportedExternalApis/SupportedExternalApis.cs b/csharp/ql/test/query-tests/Telemetry/SupportedExternalApis/SupportedExternalApis.cs index 96488fa19a77..3c2c3161ec14 100644 --- a/csharp/ql/test/query-tests/Telemetry/SupportedExternalApis/SupportedExternalApis.cs +++ b/csharp/ql/test/query-tests/Telemetry/SupportedExternalApis/SupportedExternalApis.cs @@ -45,4 +45,9 @@ public void M5() Console.SetError(Console.Out); // Has no flow summary, supported as neutral summary model var x = Console.Read(); // Known source } + + public void M6() + { + var html = new HtmlString("html"); // Supported HtmlSink defined in QL. + } } From f69737b407432caf988b51338bb85ed684cecd20 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Tue, 16 Apr 2024 12:38:38 +0200 Subject: [PATCH 2/6] C#: Move parallelsink to the library qlpack. --- .../dataflow/flowsinks/ParallelSink.qll | 24 ++++++++++++++++ csharp/ql/src/Likely Bugs/ParallelSink.qll | 28 ++++--------------- .../ThreadUnsafeICryptoTransformLambda.ql | 2 +- 3 files changed, 30 insertions(+), 24 deletions(-) create mode 100644 csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/ParallelSink.qll diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/ParallelSink.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/ParallelSink.qll new file mode 100644 index 000000000000..9048013b208c --- /dev/null +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/ParallelSink.qll @@ -0,0 +1,24 @@ +import csharp + +abstract class ParallelSink extends DataFlow::Node { } + +class LambdaParallelSink extends ParallelSink { + LambdaParallelSink() { + exists(Class c, Method m, MethodCall mc, Expr e | e = this.asExpr() | + c.getABaseType*().hasFullyQualifiedName("System.Threading.Tasks", "Parallel") and + c.getAMethod() = m and + m.getName() = "Invoke" and + m.getACall() = mc and + mc.getAnArgument() = e + ) + } +} + +class ThreadStartParallelSink extends ParallelSink { + ThreadStartParallelSink() { + exists(DelegateCreation dc, Expr e | e = this.asExpr() | + dc.getArgument() = e and + dc.getType().getName().matches("%Start") + ) + } +} diff --git a/csharp/ql/src/Likely Bugs/ParallelSink.qll b/csharp/ql/src/Likely Bugs/ParallelSink.qll index 9048013b208c..99503c77417e 100644 --- a/csharp/ql/src/Likely Bugs/ParallelSink.qll +++ b/csharp/ql/src/Likely Bugs/ParallelSink.qll @@ -1,24 +1,6 @@ -import csharp - -abstract class ParallelSink extends DataFlow::Node { } +/** + * DEPRECATED: Use `ParallelSink` from `flowsinks.ParallelSink` instead. + */ -class LambdaParallelSink extends ParallelSink { - LambdaParallelSink() { - exists(Class c, Method m, MethodCall mc, Expr e | e = this.asExpr() | - c.getABaseType*().hasFullyQualifiedName("System.Threading.Tasks", "Parallel") and - c.getAMethod() = m and - m.getName() = "Invoke" and - m.getACall() = mc and - mc.getAnArgument() = e - ) - } -} - -class ThreadStartParallelSink extends ParallelSink { - ThreadStartParallelSink() { - exists(DelegateCreation dc, Expr e | e = this.asExpr() | - dc.getArgument() = e and - dc.getType().getName().matches("%Start") - ) - } -} +import csharp +deprecated import semmle.code.csharp.security.dataflow.flowsinks.ParallelSink diff --git a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransformLambda.ql b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransformLambda.ql index 77151d0ba309..9f70760ba602 100644 --- a/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransformLambda.ql +++ b/csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransformLambda.ql @@ -15,7 +15,7 @@ */ import csharp -import ParallelSink +import semmle.code.csharp.security.dataflow.flowsinks.ParallelSink import ICryptoTransform module NotThreadSafeCryptoUsageIntoParallelInvokeConfig implements DataFlow::ConfigSig { From 543032a3de43f6a0e1be23b4dbab9f779bb4653f Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Tue, 16 Apr 2024 13:41:05 +0200 Subject: [PATCH 3/6] C#: Add ParallelSink QL Doc. --- .../security/dataflow/flowsinks/ParallelSink.qll | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/ParallelSink.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/ParallelSink.qll index 9048013b208c..5e53c9bd8fd2 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/ParallelSink.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/ParallelSink.qll @@ -1,7 +1,17 @@ +/** + * Provides class representing parallel sink nodes. + */ + import csharp +/** + * A data flow sink node for parallel execution. + */ abstract class ParallelSink extends DataFlow::Node { } +/** + * A data flow sink node for lambda parallel sink. + */ class LambdaParallelSink extends ParallelSink { LambdaParallelSink() { exists(Class c, Method m, MethodCall mc, Expr e | e = this.asExpr() | @@ -14,6 +24,9 @@ class LambdaParallelSink extends ParallelSink { } } +/** + * A data flow sink node for thread start parallel sink. + */ class ThreadStartParallelSink extends ParallelSink { ThreadStartParallelSink() { exists(DelegateCreation dc, Expr e | e = this.asExpr() | From e7bfd7df62b9d7b8287721549483a0ca96661bed Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 15 Apr 2024 16:16:29 +0200 Subject: [PATCH 4/6] C#: Take more sources and sinks into account when reporting in the telemetry queries. --- .../security/dataflow/flowsinks/AllSinks.qll | 84 +++++++++++++++++++ .../dataflow/flowsources/AllSources.qll | 77 +++++++++++++++++ csharp/ql/src/Telemetry/ExternalApi.qll | 9 +- 3 files changed, 165 insertions(+), 5 deletions(-) create mode 100644 csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/AllSinks.qll create mode 100644 csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/AllSources.qll diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/AllSinks.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/AllSinks.qll new file mode 100644 index 000000000000..b0af48947479 --- /dev/null +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/AllSinks.qll @@ -0,0 +1,84 @@ +/** Provides classes representing various flow sinks for data flow / taint tracking. */ + +private import semmle.code.csharp.dataflow.internal.ExternalFlow + +/** + * Module that adds all sinks to `SinkNode`, excluding sinks for cryptography based + * queries, and queries where sinks are not succifiently explicit. + */ +private module AllSinks { + private import ParallelSink as ParallelSink + private import Remote as Remote + private import semmle.code.csharp.security.dataflow.CodeInjectionQuery as CodeInjectionQuery + private import semmle.code.csharp.security.dataflow.ConditionalBypassQuery as ConditionalBypassQuery + private import semmle.code.csharp.security.dataflow.ExposureOfPrivateInformationQuery as ExposureOfPrivateInformationQuery + private import semmle.code.csharp.security.dataflow.HardcodedCredentialsQuery as HardcodedCredentialsQuery + private import semmle.code.csharp.security.dataflow.LDAPInjectionQuery as LdapInjectionQuery + private import semmle.code.csharp.security.dataflow.LogForgingQuery as LogForgingQuery + private import semmle.code.csharp.security.dataflow.MissingXMLValidationQuery as MissingXmlValidationQuery + private import semmle.code.csharp.security.dataflow.ReDoSQuery as ReDosQuery + private import semmle.code.csharp.security.dataflow.RegexInjectionQuery as RegexInjectionQuery + private import semmle.code.csharp.security.dataflow.ResourceInjectionQuery as ResourceInjectionQuery + private import semmle.code.csharp.security.dataflow.SqlInjectionQuery as SqlInjectionQuery + private import semmle.code.csharp.security.dataflow.TaintedPathQuery as TaintedPathQuery + private import semmle.code.csharp.security.dataflow.UnsafeDeserializationQuery as UnsafeDeserializationQuery + private import semmle.code.csharp.security.dataflow.UrlRedirectQuery as UrlRedirectQuery + private import semmle.code.csharp.security.dataflow.XMLEntityInjectionQuery as XmlEntityInjectionQuery + private import semmle.code.csharp.security.dataflow.XPathInjectionQuery as XpathInjectionQuery + private import semmle.code.csharp.security.dataflow.XSSSinks as XssSinks + private import semmle.code.csharp.security.dataflow.ZipSlipQuery as ZipSlipQuery + + private class ParallelSink extends SinkNode instanceof ParallelSink::ParallelSink { } + + private class RemoteSinkFlowSinks extends SinkNode instanceof Remote::RemoteFlowSink { } + + private class CodeInjectionSink extends SinkNode instanceof CodeInjectionQuery::Sink { } + + private class ConditionalBypassSink extends SinkNode instanceof ConditionalBypassQuery::Sink { } + + private class ExposureOfPrivateInformationSink extends SinkNode instanceof ExposureOfPrivateInformationQuery::Sink + { } + + private class HardcodedCredentialsSink extends SinkNode instanceof HardcodedCredentialsQuery::Sink + { } + + private class LdapInjectionSink extends SinkNode instanceof LdapInjectionQuery::Sink { } + + private class LogForgingSink extends SinkNode instanceof LogForgingQuery::Sink { } + + private class MissingXmlValidationSink extends SinkNode instanceof MissingXmlValidationQuery::Sink + { } + + private class ReDosSink extends SinkNode instanceof ReDosQuery::Sink { } + + private class RegexInjectionSink extends SinkNode instanceof RegexInjectionQuery::Sink { } + + private class ResourceInjectionSink extends SinkNode instanceof ResourceInjectionQuery::Sink { } + + private class SqlInjectionSink extends SinkNode instanceof SqlInjectionQuery::Sink { } + + private class TaintedPathSink extends SinkNode instanceof TaintedPathQuery::Sink { } + + private class UnsafeDeserializationSink extends SinkNode instanceof UnsafeDeserializationQuery::Sink + { } + + private class UrlRedirectSink extends SinkNode instanceof UrlRedirectQuery::Sink { } + + private class XmlEntityInjectionSink extends SinkNode instanceof XmlEntityInjectionQuery::Sink { } + + private class XpathInjectionSink extends SinkNode instanceof XpathInjectionQuery::Sink { } + + private class XssSink extends SinkNode instanceof XssSinks::Sink { } + + /** + * Add all models as data sinks. + */ + private class SinkNodeExternal extends SinkNode { + SinkNodeExternal() { sinkNode(this, _) } + } +} + +/** + * A data flow sink node. + */ +abstract class SinkNode extends DataFlow::Node { } diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/AllSources.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/AllSources.qll new file mode 100644 index 000000000000..11ff5ae2a553 --- /dev/null +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/AllSources.qll @@ -0,0 +1,77 @@ +/** Provides classes representing various flow sources for data flow / taint tracking. */ + +private import semmle.code.csharp.dataflow.internal.ExternalFlow + +/** + * Module that adds all sources to `SourceNode`, excluding source for cryptography based + * queries, and queries where sources are not succifiently explicit or mainly hardcoded constants. + */ +private module AllSources { + private import FlowSources as FlowSources + private import semmle.code.csharp.security.cryptography.HardcodedSymmetricEncryptionKey + private import semmle.code.csharp.security.dataflow.CleartextStorageQuery as CleartextStorageQuery + private import semmle.code.csharp.security.dataflow.CodeInjectionQuery as CodeInjectionQuery + private import semmle.code.csharp.security.dataflow.ConditionalBypassQuery as ConditionalBypassQuery + private import semmle.code.csharp.security.dataflow.ExposureOfPrivateInformationQuery as ExposureOfPrivateInformationQuery + private import semmle.code.csharp.security.dataflow.HardcodedCredentialsQuery as HardcodedCredentialsQuery + private import semmle.code.csharp.security.dataflow.LDAPInjectionQuery as LdapInjectionQuery + private import semmle.code.csharp.security.dataflow.LogForgingQuery as LogForgingQuery + private import semmle.code.csharp.security.dataflow.MissingXMLValidationQuery as MissingXmlValidationQuery + private import semmle.code.csharp.security.dataflow.ReDoSQuery as ReDosQuery + private import semmle.code.csharp.security.dataflow.RegexInjectionQuery as RegexInjectionQuery + private import semmle.code.csharp.security.dataflow.ResourceInjectionQuery as ResourceInjectionQuery + private import semmle.code.csharp.security.dataflow.SqlInjectionQuery as SqlInjectionQuery + private import semmle.code.csharp.security.dataflow.TaintedPathQuery as TaintedPathQuery + private import semmle.code.csharp.security.dataflow.UnsafeDeserializationQuery as UnsafeDeserializationQuery + private import semmle.code.csharp.security.dataflow.UrlRedirectQuery as UrlRedirectQuery + private import semmle.code.csharp.security.dataflow.XMLEntityInjectionQuery as XmlEntityInjectionQuery + private import semmle.code.csharp.security.dataflow.XPathInjectionQuery as XpathInjectionQuery + private import semmle.code.csharp.security.dataflow.ZipSlipQuery as ZipSlipQuery + + private class FlowSourcesSources extends SourceNode instanceof FlowSources::SourceNode { } + + private class CodeInjectionSource extends SourceNode instanceof CodeInjectionQuery::Source { } + + private class ConditionalBypassSource extends SourceNode instanceof ConditionalBypassQuery::Source + { } + + private class LdapInjectionSource extends SourceNode instanceof LdapInjectionQuery::Source { } + + private class LogForgingSource extends SourceNode instanceof LogForgingQuery::Source { } + + private class MissingXmlValidationSource extends SourceNode instanceof MissingXmlValidationQuery::Source + { } + + private class ReDosSource extends SourceNode instanceof ReDosQuery::Source { } + + private class RegexInjectionSource extends SourceNode instanceof RegexInjectionQuery::Source { } + + private class ResourceInjectionSource extends SourceNode instanceof ResourceInjectionQuery::Source + { } + + private class SqlInjectionSource extends SourceNode instanceof SqlInjectionQuery::Source { } + + private class TaintedPathSource extends SourceNode instanceof TaintedPathQuery::Source { } + + private class UnsafeDeserializationSource extends SourceNode instanceof UnsafeDeserializationQuery::Source + { } + + private class UrlRedirectSource extends SourceNode instanceof UrlRedirectQuery::Source { } + + private class XmlEntityInjectionSource extends SourceNode instanceof XmlEntityInjectionQuery::Source + { } + + private class XpathInjectionSource extends SourceNode instanceof XpathInjectionQuery::Source { } + + /** + * Add all models as data sources. + */ + private class SourceNodeExternal extends SourceNode { + SourceNodeExternal() { sourceNode(this, _) } + } +} + +/** + * A data flow source node. + */ +abstract class SourceNode extends DataFlow::Node { } diff --git a/csharp/ql/src/Telemetry/ExternalApi.qll b/csharp/ql/src/Telemetry/ExternalApi.qll index 8919b67cf2d2..a710cdf7cfde 100644 --- a/csharp/ql/src/Telemetry/ExternalApi.qll +++ b/csharp/ql/src/Telemetry/ExternalApi.qll @@ -8,7 +8,8 @@ private import semmle.code.csharp.dataflow.internal.DataFlowDispatch as DataFlow private import semmle.code.csharp.dataflow.internal.ExternalFlow private import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl private import semmle.code.csharp.dataflow.internal.TaintTrackingPrivate -private import semmle.code.csharp.security.dataflow.flowsources.Remote +private import semmle.code.csharp.security.dataflow.flowsources.AllSources +private import semmle.code.csharp.security.dataflow.flowsinks.AllSinks private import TestLibrary /** Holds if the given callable is not worth supporting. */ @@ -84,13 +85,11 @@ class ExternalApi extends Callable { /** Holds if this API is a known source. */ pragma[nomagic] - predicate isSource() { - this.getAnOutput() instanceof RemoteFlowSource or sourceNode(this.getAnOutput(), _) - } + predicate isSource() { this.getAnOutput() instanceof SourceNode } /** Holds if this API is a known sink. */ pragma[nomagic] - predicate isSink() { sinkNode(this.getAnInput(), _) } + predicate isSink() { this.getAnInput() instanceof SinkNode } /** Holds if this API is a known neutral. */ pragma[nomagic] From 4a4f9b39427380eabebe265188dd8487772d3eb9 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Tue, 16 Apr 2024 15:30:00 +0200 Subject: [PATCH 5/6] C#: Update expected test output. --- .../SupportedExternalApis/SupportedExternalApis.expected | 1 + 1 file changed, 1 insertion(+) diff --git a/csharp/ql/test/query-tests/Telemetry/SupportedExternalApis/SupportedExternalApis.expected b/csharp/ql/test/query-tests/Telemetry/SupportedExternalApis/SupportedExternalApis.expected index 0fc5fc61a5df..2096193b27d6 100644 --- a/csharp/ql/test/query-tests/Telemetry/SupportedExternalApis/SupportedExternalApis.expected +++ b/csharp/ql/test/query-tests/Telemetry/SupportedExternalApis/SupportedExternalApis.expected @@ -8,4 +8,5 @@ | System#DateTime.AddDays(System.Double) | 1 | | System#DateTime.DateTime(System.Int32,System.Int32,System.Int32) | 1 | | System#Guid.Parse(System.String) | 1 | +| System.Web#HtmlString.HtmlString(System.String) | 1 | | System.Web#HttpResponse.WriteFile(System.String) | 1 | From bc0e580683831c2847e08b88b6223a1685f8e483 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 17 Apr 2024 09:34:27 +0200 Subject: [PATCH 6/6] C#: Address review comments. --- .../csharp/security/dataflow/flowsinks/AllSinks.qll | 10 +++++----- .../security/dataflow/flowsources/AllSources.qll | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/AllSinks.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/AllSinks.qll index b0af48947479..bf601bdf9b66 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/AllSinks.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/AllSinks.qll @@ -2,6 +2,11 @@ private import semmle.code.csharp.dataflow.internal.ExternalFlow +/** + * A data flow sink node. + */ +abstract class SinkNode extends DataFlow::Node { } + /** * Module that adds all sinks to `SinkNode`, excluding sinks for cryptography based * queries, and queries where sinks are not succifiently explicit. @@ -77,8 +82,3 @@ private module AllSinks { SinkNodeExternal() { sinkNode(this, _) } } } - -/** - * A data flow sink node. - */ -abstract class SinkNode extends DataFlow::Node { } diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/AllSources.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/AllSources.qll index 11ff5ae2a553..7d05500446a4 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/AllSources.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/AllSources.qll @@ -2,6 +2,11 @@ private import semmle.code.csharp.dataflow.internal.ExternalFlow +/** + * A data flow source node. + */ +abstract class SourceNode extends DataFlow::Node { } + /** * Module that adds all sources to `SourceNode`, excluding source for cryptography based * queries, and queries where sources are not succifiently explicit or mainly hardcoded constants. @@ -70,8 +75,3 @@ private module AllSources { SourceNodeExternal() { sourceNode(this, _) } } } - -/** - * A data flow source node. - */ -abstract class SourceNode extends DataFlow::Node { }