From fb579147517b459b2e324b8e888fead80caef84d Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Tue, 7 Mar 2023 18:21:52 +0100 Subject: [PATCH] C++: Convert a number of data flow based queries to use `ConfigSig` --- cpp/ql/src/Critical/OverflowDestination.ql | 20 +++--- .../Memory Management/NtohlArrayNoBound.ql | 4 +- .../Memory Management/NtohlArrayNoBound.qll | 18 +++++- .../src/Security/CWE/CWE-020/ExternalAPIs.qll | 6 +- .../CWE/CWE-020/ExternalAPIsSpecific.qll | 15 ++++- .../CWE-020/IRUntrustedDataToExternalAPI.ql | 6 +- .../CWE/CWE-020/UntrustedDataToExternalAPI.ql | 6 +- .../Security/CWE/CWE-020/ir/ExternalAPIs.qll | 6 +- .../CWE/CWE-020/ir/ExternalAPIsSpecific.qll | 10 ++- .../CWE-129/ImproperArrayIndexValidation.ql | 18 +++--- .../CWE/CWE-190/ArithmeticUncontrolled.ql | 20 +++--- .../CWE/CWE-295/SSLResultConflation.ql | 20 +++--- .../CWE/CWE-311/CleartextBufferWrite.ql | 20 +++--- .../CWE/CWE-311/CleartextFileWrite.ql | 20 +++--- .../CWE/CWE-311/CleartextTransmission.ql | 62 +++++++++---------- .../CWE/CWE-313/CleartextSqliteDatabase.ql | 22 +++---- cpp/ql/src/Security/CWE/CWE-319/UseOfHttp.ql | 16 ++--- .../CWE/CWE-326/InsufficientKeySize.ql | 18 +++--- .../Security/CWE/CWE-497/ExposedSystemData.ql | 18 +++--- .../Security/CWE/CWE-078/WordexpTainted.ql | 18 +++--- .../CWE-190/AllocMultiplicationOverflow.ql | 16 ++--- 21 files changed, 192 insertions(+), 167 deletions(-) diff --git a/cpp/ql/src/Critical/OverflowDestination.ql b/cpp/ql/src/Critical/OverflowDestination.ql index f38c35c9ab2..39ce527e08a 100644 --- a/cpp/ql/src/Critical/OverflowDestination.ql +++ b/cpp/ql/src/Critical/OverflowDestination.ql @@ -17,7 +17,7 @@ import cpp import semmle.code.cpp.ir.dataflow.TaintTracking import semmle.code.cpp.controlflow.IRGuards import semmle.code.cpp.security.FlowSources -import DataFlow::PathGraph +import OverflowDestination::PathGraph /** * Holds if `fc` is a call to a copy operation where the size argument contains @@ -66,14 +66,12 @@ predicate nodeIsBarrierEqualityCandidate(DataFlow::Node node, Operand access, Va any(IRGuardCondition guard).ensuresEq(access, _, _, node.asInstruction().getBlock(), true) } -class OverflowDestinationConfig extends TaintTracking::Configuration { - OverflowDestinationConfig() { this = "OverflowDestinationConfig" } +module OverflowDestinationConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof FlowSource } - override predicate isSource(DataFlow::Node source) { source instanceof FlowSource } + predicate isSink(DataFlow::Node sink) { sourceSized(_, sink.asIndirectConvertedExpr()) } - override predicate isSink(DataFlow::Node sink) { sourceSized(_, sink.asIndirectConvertedExpr()) } - - override predicate isSanitizer(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { exists(Variable checkedVar | readsVariable(node.asInstruction(), checkedVar) and hasUpperBoundsCheck(checkedVar) @@ -86,11 +84,11 @@ class OverflowDestinationConfig extends TaintTracking::Configuration { } } -from - FunctionCall fc, OverflowDestinationConfig conf, DataFlow::PathNode source, - DataFlow::PathNode sink +module OverflowDestination = TaintTracking::Make; + +from FunctionCall fc, OverflowDestination::PathNode source, OverflowDestination::PathNode sink where - conf.hasFlowPath(source, sink) and + OverflowDestination::hasFlowPath(source, sink) and sourceSized(fc, sink.getNode().asIndirectConvertedExpr()) select fc, source, sink, "To avoid overflow, this operation should be bounded by destination-buffer size, not source-buffer size." diff --git a/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.ql b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.ql index 5d03ccc44ea..c23eda355c4 100644 --- a/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.ql +++ b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.ql @@ -11,6 +11,6 @@ import cpp import NtohlArrayNoBound -from NetworkToBufferSizeConfiguration bufConfig, DataFlow::Node source, DataFlow::Node sink -where bufConfig.hasFlow(source, sink) +from DataFlow::Node source, DataFlow::Node sink +where NetworkToBufferSizeFlow::hasFlow(source, sink) select sink, "Unchecked use of data from network function $@.", source, source.toString() diff --git a/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.qll b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.qll index c772aff233d..60f05bbc293 100644 --- a/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.qll +++ b/cpp/ql/src/Likely Bugs/Memory Management/NtohlArrayNoBound.qll @@ -129,7 +129,7 @@ class NetworkFunctionCall extends FunctionCall { NetworkFunctionCall() { this.getTarget().hasName(["ntohd", "ntohf", "ntohl", "ntohll", "ntohs"]) } } -class NetworkToBufferSizeConfiguration extends DataFlow::Configuration { +deprecated class NetworkToBufferSizeConfiguration extends DataFlow::Configuration { NetworkToBufferSizeConfiguration() { this = "NetworkToBufferSizeConfiguration" } override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof NetworkFunctionCall } @@ -146,3 +146,19 @@ class NetworkToBufferSizeConfiguration extends DataFlow::Configuration { ) } } + +module NetworkToBufferSizeConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { node.asExpr() instanceof NetworkFunctionCall } + + predicate isSink(DataFlow::Node node) { node.asExpr() = any(BufferAccess ba).getAccessedLength() } + + predicate isBarrier(DataFlow::Node node) { + exists(GuardCondition gc, GVN gvn | + gc.getAChild*() = gvn.getAnExpr() and + globalValueNumber(node.asExpr()) = gvn and + gc.controls(node.asExpr().getBasicBlock(), _) + ) + } +} + +module NetworkToBufferSizeFlow = DataFlow::Make; diff --git a/cpp/ql/src/Security/CWE/CWE-020/ExternalAPIs.qll b/cpp/ql/src/Security/CWE/CWE-020/ExternalAPIs.qll index de7d043ad59..6d6f17daf0e 100644 --- a/cpp/ql/src/Security/CWE/CWE-020/ExternalAPIs.qll +++ b/cpp/ql/src/Security/CWE/CWE-020/ExternalAPIs.qll @@ -10,12 +10,10 @@ import ExternalAPIsSpecific /** A node representing untrusted data being passed to an external API. */ class UntrustedExternalApiDataNode extends ExternalApiDataNode { - UntrustedExternalApiDataNode() { any(UntrustedDataToExternalApiConfig c).hasFlow(_, this) } + UntrustedExternalApiDataNode() { UntrustedDataToExternalApiFlow::hasFlow(_, this) } /** Gets a source of untrusted data which is passed to this external API data node. */ - DataFlow::Node getAnUntrustedSource() { - any(UntrustedDataToExternalApiConfig c).hasFlow(result, this) - } + DataFlow::Node getAnUntrustedSource() { UntrustedDataToExternalApiFlow::hasFlow(result, this) } } /** DEPRECATED: Alias for UntrustedExternalApiDataNode */ diff --git a/cpp/ql/src/Security/CWE/CWE-020/ExternalAPIsSpecific.qll b/cpp/ql/src/Security/CWE/CWE-020/ExternalAPIsSpecific.qll index b2df197872c..b4769dd041d 100644 --- a/cpp/ql/src/Security/CWE/CWE-020/ExternalAPIsSpecific.qll +++ b/cpp/ql/src/Security/CWE/CWE-020/ExternalAPIsSpecific.qll @@ -45,7 +45,7 @@ class ExternalApiDataNode extends DataFlow::Node { deprecated class ExternalAPIDataNode = ExternalApiDataNode; /** A configuration for tracking flow from `RemoteFlowSource`s to `ExternalApiDataNode`s. */ -class UntrustedDataToExternalApiConfig extends TaintTracking::Configuration { +deprecated class UntrustedDataToExternalApiConfig extends TaintTracking::Configuration { UntrustedDataToExternalApiConfig() { this = "UntrustedDataToExternalAPIConfig" } override predicate isSource(DataFlow::Node source) { @@ -60,3 +60,16 @@ class UntrustedDataToExternalApiConfig extends TaintTracking::Configuration { /** DEPRECATED: Alias for UntrustedDataToExternalApiConfig */ deprecated class UntrustedDataToExternalAPIConfig = UntrustedDataToExternalApiConfig; + +module UntrustedDataToExternalApiConfig implements DataFlow:ConfigSig { + predicate isSource(DataFlow::Node source) { + exists(RemoteFlowSourceFunction remoteFlow | + remoteFlow = source.asExpr().(Call).getTarget() and + remoteFlow.hasRemoteFlowSource(_, _) + ) + } + + predicate isSink(DataFlow::Node sink) { sink instanceof ExternalApiDataNode } +} + +module UntrustedDataToExternalApiFlow = TaintTracking::Make; diff --git a/cpp/ql/src/Security/CWE/CWE-020/IRUntrustedDataToExternalAPI.ql b/cpp/ql/src/Security/CWE/CWE-020/IRUntrustedDataToExternalAPI.ql index 4860fc5356f..d16ce6376dd 100644 --- a/cpp/ql/src/Security/CWE/CWE-020/IRUntrustedDataToExternalAPI.ql +++ b/cpp/ql/src/Security/CWE/CWE-020/IRUntrustedDataToExternalAPI.ql @@ -13,10 +13,10 @@ import cpp import semmle.code.cpp.ir.dataflow.TaintTracking import ir.ExternalAPIs import semmle.code.cpp.security.FlowSources -import DataFlow::PathGraph +import UntrustedDataToExternalApiFlow::PathGraph -from UntrustedDataToExternalApiConfig config, DataFlow::PathNode source, DataFlow::PathNode sink -where config.hasFlowPath(source, sink) +from UntrustedDataToExternalApiFlow::PathNode source, UntrustedDataToExternalApiFlow::PathNode sink +where UntrustedDataToExternalApiFlow::hasFlowPath(source, sink) select sink, source, sink, "Call to " + sink.getNode().(ExternalApiDataNode).getExternalFunction().toString() + " with untrusted data from $@.", source, source.getNode().(RemoteFlowSource).getSourceType() diff --git a/cpp/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql b/cpp/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql index f7c6a190a5b..a00accd4d29 100644 --- a/cpp/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql +++ b/cpp/ql/src/Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql @@ -12,10 +12,10 @@ import cpp import semmle.code.cpp.ir.dataflow.TaintTracking import ExternalAPIs -import DataFlow::PathGraph +import UntrustedDataToExternalApiFlow::PathGraph -from UntrustedDataToExternalApiConfig config, DataFlow::PathNode source, DataFlow::PathNode sink -where config.hasFlowPath(source, sink) +from UntrustedDataToExternalApiFlow::PathNode source, UntrustedDataToExternalApiFlow::PathNode sink +where UntrustedDataToExternalApiFlow::hasFlowPath(source, sink) select sink, source, sink, "Call to " + sink.getNode().(ExternalApiDataNode).getExternalFunction().toString() + " with untrusted data from $@.", source, source.toString() diff --git a/cpp/ql/src/Security/CWE/CWE-020/ir/ExternalAPIs.qll b/cpp/ql/src/Security/CWE/CWE-020/ir/ExternalAPIs.qll index de7d043ad59..a38802978dd 100644 --- a/cpp/ql/src/Security/CWE/CWE-020/ir/ExternalAPIs.qll +++ b/cpp/ql/src/Security/CWE/CWE-020/ir/ExternalAPIs.qll @@ -10,12 +10,10 @@ import ExternalAPIsSpecific /** A node representing untrusted data being passed to an external API. */ class UntrustedExternalApiDataNode extends ExternalApiDataNode { - UntrustedExternalApiDataNode() { any(UntrustedDataToExternalApiConfig c).hasFlow(_, this) } + UntrustedExternalApiDataNode() { UntrustedDataToExternalApiFlow::hasFlow(_, this) } /** Gets a source of untrusted data which is passed to this external API data node. */ - DataFlow::Node getAnUntrustedSource() { - any(UntrustedDataToExternalApiConfig c).hasFlow(result, this) - } + DataFlow::Node getAnUntrustedSource() { UntrustedDataToExternalApiConfig::hasFlow(result, this) } } /** DEPRECATED: Alias for UntrustedExternalApiDataNode */ diff --git a/cpp/ql/src/Security/CWE/CWE-020/ir/ExternalAPIsSpecific.qll b/cpp/ql/src/Security/CWE/CWE-020/ir/ExternalAPIsSpecific.qll index b0fa685d1a4..45eec3053a6 100644 --- a/cpp/ql/src/Security/CWE/CWE-020/ir/ExternalAPIsSpecific.qll +++ b/cpp/ql/src/Security/CWE/CWE-020/ir/ExternalAPIsSpecific.qll @@ -45,7 +45,7 @@ class ExternalApiDataNode extends DataFlow::Node { deprecated class ExternalAPIDataNode = ExternalApiDataNode; /** A configuration for tracking flow from `RemoteFlowSource`s to `ExternalApiDataNode`s. */ -class UntrustedDataToExternalApiConfig extends TaintTracking::Configuration { +deprecated class UntrustedDataToExternalApiConfig extends TaintTracking::Configuration { UntrustedDataToExternalApiConfig() { this = "UntrustedDataToExternalAPIConfigIR" } override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } @@ -55,3 +55,11 @@ class UntrustedDataToExternalApiConfig extends TaintTracking::Configuration { /** DEPRECATED: Alias for UntrustedDataToExternalApiConfig */ deprecated class UntrustedDataToExternalAPIConfig = UntrustedDataToExternalApiConfig; + +module UntrustedDataToExternalApiConfig implements DataFlow::ConfigSig { + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof ExternalApiDataNode } +} + +module UntrustedDataToExternalApiFlow = TaintTracking::Make; diff --git a/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql b/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql index 2001b1a308c..598e242b0ef 100644 --- a/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql +++ b/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql @@ -17,7 +17,7 @@ import semmle.code.cpp.controlflow.IRGuards import semmle.code.cpp.security.FlowSources import semmle.code.cpp.ir.dataflow.TaintTracking import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils -import DataFlow::PathGraph +import ImproperArrayIndexValidation::PathGraph import semmle.code.cpp.security.Security predicate hasUpperBound(VariableAccess offsetExpr) { @@ -65,12 +65,10 @@ predicate predictableInstruction(Instruction instr) { predictableInstruction(instr.(UnaryInstruction).getUnary()) } -class ImproperArrayIndexValidationConfig extends TaintTracking::Configuration { - ImproperArrayIndexValidationConfig() { this = "ImproperArrayIndexValidationConfig" } +module ImproperArrayIndexValidationConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { isFlowSource(source, _) } - override predicate isSource(DataFlow::Node source) { isFlowSource(source, _) } - - override predicate isSanitizer(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { hasUpperBound(node.asExpr()) or // These barriers are ported from `DefaultTaintTracking` because this query is quite noisy @@ -107,7 +105,7 @@ class ImproperArrayIndexValidationConfig extends TaintTracking::Configuration { ) } - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { exists(ArrayExpr arrayExpr, VariableAccess offsetExpr | offsetExpr = arrayExpr.getArrayOffset() and sink.asExpr() = offsetExpr and @@ -116,11 +114,13 @@ class ImproperArrayIndexValidationConfig extends TaintTracking::Configuration { } } +module ImproperArrayIndexValidation = TaintTracking::Make; + from - ImproperArrayIndexValidationConfig conf, DataFlow::PathNode source, DataFlow::PathNode sink, + ImproperArrayIndexValidation::PathNode source, ImproperArrayIndexValidation::PathNode sink, string sourceType where - conf.hasFlowPath(source, sink) and + ImproperArrayIndexValidation::hasFlowPath(source, sink) and isFlowSource(source.getNode(), sourceType) select sink.getNode(), source, sink, "An array indexing expression depends on $@ that might be outside the bounds of the array.", diff --git a/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql b/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql index 964b2ff33d8..dbae4303df0 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql +++ b/cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql @@ -17,7 +17,7 @@ import semmle.code.cpp.security.Overflow import semmle.code.cpp.security.Security import semmle.code.cpp.security.FlowSources import semmle.code.cpp.ir.dataflow.TaintTracking -import DataFlow::PathGraph +import UncontrolledArith::PathGraph import Bounded /** @@ -90,10 +90,8 @@ predicate missingGuard(VariableAccess va, string effect) { ) } -class UncontrolledArithConfiguration extends TaintTracking::Configuration { - UncontrolledArithConfiguration() { this = "UncontrolledArithConfiguration" } - - override predicate isSource(DataFlow::Node source) { +module UncontrolledArithConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { exists(RandomFunction rand, Call call | call.getTarget() = rand | rand.getFunctionOutput().isReturnValue() and source.asExpr() = call @@ -105,9 +103,9 @@ class UncontrolledArithConfiguration extends TaintTracking::Configuration { ) } - override predicate isSink(DataFlow::Node sink) { missingGuard(sink.asExpr(), _) } + predicate isSink(DataFlow::Node sink) { missingGuard(sink.asExpr(), _) } - override predicate isSanitizer(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { bounded(node.asExpr()) or // If this expression is part of bitwise 'and' or 'or' operation it's likely that the value is @@ -124,14 +122,16 @@ class UncontrolledArithConfiguration extends TaintTracking::Configuration { } } +module UncontrolledArith = TaintTracking::Make; + /** Gets the expression that corresponds to `node`, if any. */ Expr getExpr(DataFlow::Node node) { result = [node.asExpr(), node.asDefiningArgument()] } from - UncontrolledArithConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink, - VariableAccess va, string effect + UncontrolledArith::PathNode source, UncontrolledArith::PathNode sink, VariableAccess va, + string effect where - config.hasFlowPath(source, sink) and + UncontrolledArith::hasFlowPath(source, sink) and sink.getNode().asExpr() = va and missingGuard(va, effect) select sink.getNode(), source, sink, diff --git a/cpp/ql/src/Security/CWE/CWE-295/SSLResultConflation.ql b/cpp/ql/src/Security/CWE/CWE-295/SSLResultConflation.ql index 3ed64eb573e..34d211a53ae 100644 --- a/cpp/ql/src/Security/CWE/CWE-295/SSLResultConflation.ql +++ b/cpp/ql/src/Security/CWE/CWE-295/SSLResultConflation.ql @@ -25,24 +25,22 @@ class SslGetVerifyResultCall extends FunctionCall { * Data flow from a call to `SSL_get_verify_result` to a guard condition * that references the result. */ -class VerifyResultConfig extends DataFlow::Configuration { - VerifyResultConfig() { this = "VerifyResultConfig" } +module VerifyResultConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source.asExpr() instanceof SslGetVerifyResultCall } - override predicate isSource(DataFlow::Node source) { - source.asExpr() instanceof SslGetVerifyResultCall - } - - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { exists(GuardCondition guard | guard.getAChild*() = sink.asExpr()) } } +module VerifyResult = DataFlow::Make; + from - VerifyResultConfig config, DataFlow::Node source, DataFlow::Node sink1, DataFlow::Node sink2, - GuardCondition guard, Expr c1, Expr c2, boolean testIsTrue + DataFlow::Node source, DataFlow::Node sink1, DataFlow::Node sink2, GuardCondition guard, Expr c1, + Expr c2, boolean testIsTrue where - config.hasFlow(source, sink1) and - config.hasFlow(source, sink2) and + VerifyResult::hasFlow(source, sink1) and + VerifyResult::hasFlow(source, sink2) and guard.comparesEq(sink1.asExpr(), c1, 0, false, testIsTrue) and // (value != c1) => testIsTrue guard.comparesEq(sink2.asExpr(), c2, 0, false, testIsTrue) and // (value != c2) => testIsTrue c1.getValue().toInt() = 0 and diff --git a/cpp/ql/src/Security/CWE/CWE-311/CleartextBufferWrite.ql b/cpp/ql/src/Security/CWE/CWE-311/CleartextBufferWrite.ql index b13ba3a3043..88076469fb7 100644 --- a/cpp/ql/src/Security/CWE/CWE-311/CleartextBufferWrite.ql +++ b/cpp/ql/src/Security/CWE/CWE-311/CleartextBufferWrite.ql @@ -16,7 +16,7 @@ import semmle.code.cpp.security.BufferWrite as BufferWrite import semmle.code.cpp.security.SensitiveExprs import semmle.code.cpp.security.FlowSources import semmle.code.cpp.ir.dataflow.TaintTracking -import DataFlow::PathGraph +import ToBufferFlow::PathGraph /** * A buffer write into a sensitive expression. @@ -39,27 +39,27 @@ class SensitiveBufferWrite extends Expr instanceof BufferWrite::BufferWrite { * A taint flow configuration for flow from user input to a buffer write * into a sensitive expression. */ -class ToBufferConfiguration extends TaintTracking::Configuration { - ToBufferConfiguration() { this = "ToBufferConfiguration" } +module ToBufferConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof FlowSource } - override predicate isSource(DataFlow::Node source) { source instanceof FlowSource } - - override predicate isSanitizer(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { node.asExpr().getUnspecifiedType() instanceof IntegralType } - override predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _) } + predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _) } } +module ToBufferFlow = TaintTracking::Make; + predicate isSinkImpl(DataFlow::Node sink, SensitiveBufferWrite w) { w.getASource() = sink.asIndirectExpr() } from - ToBufferConfiguration config, SensitiveBufferWrite w, DataFlow::PathNode sourceNode, - DataFlow::PathNode sinkNode, FlowSource source + SensitiveBufferWrite w, ToBufferFlow::PathNode sourceNode, + ToBufferFlow::PathNode sinkNode, FlowSource source where - config.hasFlowPath(sourceNode, sinkNode) and +ToBufferFlow::hasFlowPath(sourceNode, sinkNode) and sourceNode.getNode() = source and isSinkImpl(sinkNode.getNode(), w) select w, sourceNode, sinkNode, diff --git a/cpp/ql/src/Security/CWE/CWE-311/CleartextFileWrite.ql b/cpp/ql/src/Security/CWE/CWE-311/CleartextFileWrite.ql index ba1c6e78148..7c987840f57 100644 --- a/cpp/ql/src/Security/CWE/CWE-311/CleartextFileWrite.ql +++ b/cpp/ql/src/Security/CWE/CWE-311/CleartextFileWrite.ql @@ -18,23 +18,23 @@ import semmle.code.cpp.security.FileWrite import semmle.code.cpp.ir.dataflow.DataFlow import semmle.code.cpp.valuenumbering.GlobalValueNumbering import semmle.code.cpp.ir.dataflow.TaintTracking -import DataFlow::PathGraph +import FromSensitiveFlow::PathGraph /** * A taint flow configuration for flow from a sensitive expression to a `FileWrite` sink. */ -class FromSensitiveConfiguration extends TaintTracking::Configuration { - FromSensitiveConfiguration() { this = "FromSensitiveConfiguration" } +module FromSensitiveConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { isSourceImpl(source, _) } - override predicate isSource(DataFlow::Node source) { isSourceImpl(source, _) } + predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _, _) } - override predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _, _) } - - override predicate isSanitizer(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { node.asExpr().getUnspecifiedType() instanceof IntegralType } } +module FromSensitiveFlow = TaintTracking::Make; + predicate isSinkImpl(DataFlow::Node sink, FileWrite w, Expr dest) { exists(Expr e | e = [sink.asExpr(), sink.asIndirectExpr()] and @@ -78,10 +78,10 @@ predicate isFileName(GVN gvn) { } from - FromSensitiveConfiguration config, SensitiveExpr source, DataFlow::PathNode sourceNode, - DataFlow::PathNode midNode, FileWrite w, Expr dest + SensitiveExpr source, FromSensitiveFlow::PathNode sourceNode, FromSensitiveFlow::PathNode midNode, + FileWrite w, Expr dest where - config.hasFlowPath(sourceNode, midNode) and + FromSensitiveFlow::hasFlowPath(sourceNode, midNode) and isSourceImpl(sourceNode.getNode(), source) and isSinkImpl(midNode.getNode(), w, dest) select w, sourceNode, midNode, diff --git a/cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql b/cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql index f50b3258ff0..e24496cd1ad 100644 --- a/cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql +++ b/cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql @@ -16,11 +16,9 @@ import cpp import semmle.code.cpp.security.SensitiveExprs import semmle.code.cpp.security.PrivateData import semmle.code.cpp.ir.dataflow.TaintTracking -import semmle.code.cpp.ir.dataflow.TaintTracking2 -import semmle.code.cpp.ir.dataflow.TaintTracking3 import semmle.code.cpp.models.interfaces.FlowSource import semmle.code.cpp.commons.File -import DataFlow::PathGraph +import FromSensitiveFlow::PathGraph class SourceVariable extends Variable { SourceVariable() { @@ -236,72 +234,70 @@ predicate isSourceImpl(DataFlow::Node source) { * A taint flow configuration for flow from a sensitive expression to a network * operation. */ -class FromSensitiveConfiguration extends TaintTracking::Configuration { - FromSensitiveConfiguration() { this = "FromSensitiveConfiguration" } +module FromSensitiveConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { isSourceImpl(source) } - override predicate isSource(DataFlow::Node source) { isSourceImpl(source) } + predicate isSink(DataFlow::Node sink) { isSinkSendRecv(sink, _) } - override predicate isSink(DataFlow::Node sink) { isSinkSendRecv(sink, _) } - - override predicate isSanitizer(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { node.asExpr().getUnspecifiedType() instanceof IntegralType } - override predicate isSanitizerIn(DataFlow::Node node) { + predicate isBarrierIn(DataFlow::Node node) { // As any use of a sensitive variable is a potential source, we need to block flow into // sources to not get path duplication. - this.isSource(node) + isSource(node) } } +module FromSensitiveFlow = TaintTracking::Make; + /** * A taint flow configuration for flow from a sensitive expression to an encryption operation. */ -class ToEncryptionConfiguration extends TaintTracking2::Configuration { - ToEncryptionConfiguration() { this = "ToEncryptionConfiguration" } +module ToEncryptionConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { FromSensitiveFlow::hasFlow(source, _) } - override predicate isSource(DataFlow::Node source) { - any(FromSensitiveConfiguration config).hasFlow(source, _) - } + predicate isSink(DataFlow::Node sink) { isSinkEncrypt(sink, _) } - override predicate isSink(DataFlow::Node sink) { isSinkEncrypt(sink, _) } - - override predicate isSanitizer(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { node.asExpr().getUnspecifiedType() instanceof IntegralType } - override predicate isSanitizerIn(DataFlow::Node node) { + predicate isBarrierIn(DataFlow::Node node) { // As any use of a sensitive variable is a potential source, we need to block flow into // sources to not get path duplication. - this.isSource(node) + isSource(node) } } +module ToEncryptionFlow = TaintTracking::Make; + /** * A taint flow configuration for flow from an encryption operation to a network operation. */ -class FromEncryptionConfiguration extends TaintTracking3::Configuration { - FromEncryptionConfiguration() { this = "FromEncryptionConfiguration" } +module FromEncryptionConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { isSinkEncrypt(source, _) } - override predicate isSource(DataFlow::Node source) { isSinkEncrypt(source, _) } + predicate isSink(DataFlow::Node sink) { FromSensitiveFlow::hasFlowTo(sink) } - override predicate isSink(DataFlow::Node sink) { - any(FromSensitiveConfiguration config).hasFlowTo(sink) - } - - override predicate isSanitizer(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { node.asExpr().getUnspecifiedType() instanceof IntegralType } } -from DataFlow::PathNode source, DataFlow::PathNode sink, NetworkSendRecv networkSendRecv, string msg +module FromEncryptionFlow = TaintTracking::Make; + +from + FromSensitiveFlow::PathNode source, FromSensitiveFlow::PathNode sink, + NetworkSendRecv networkSendRecv, string msg where // flow from sensitive -> network data - any(FromSensitiveConfiguration config).hasFlowPath(source, sink) and + FromSensitiveFlow::hasFlowPath(source, sink) and isSinkSendRecv(sink.getNode(), networkSendRecv) and // no flow from sensitive -> evidence of encryption - not any(ToEncryptionConfiguration config).hasFlow(source.getNode(), _) and - not any(FromEncryptionConfiguration config).hasFlowTo(sink.getNode()) and + not ToEncryptionFlow::hasFlow(source.getNode(), _) and + not FromEncryptionFlow::hasFlowTo(sink.getNode()) and // construct result if networkSendRecv instanceof NetworkSend then diff --git a/cpp/ql/src/Security/CWE/CWE-313/CleartextSqliteDatabase.ql b/cpp/ql/src/Security/CWE/CWE-313/CleartextSqliteDatabase.ql index 29cdaa2c749..3a6b0ee5ae5 100644 --- a/cpp/ql/src/Security/CWE/CWE-313/CleartextSqliteDatabase.ql +++ b/cpp/ql/src/Security/CWE/CWE-313/CleartextSqliteDatabase.ql @@ -14,7 +14,7 @@ import cpp import semmle.code.cpp.security.SensitiveExprs import semmle.code.cpp.ir.dataflow.TaintTracking -import DataFlow::PathGraph +import FromSensitiveFlow::PathGraph class SqliteFunctionCall extends FunctionCall { SqliteFunctionCall() { this.getTarget().getName().matches("sqlite%") } @@ -56,21 +56,19 @@ predicate isSinkImpl(DataFlow::Node sink, SqliteFunctionCall c, Type t) { /** * A taint flow configuration for flow from a sensitive expression to a `SqliteFunctionCall` sink. */ -class FromSensitiveConfiguration extends TaintTracking::Configuration { - FromSensitiveConfiguration() { this = "FromSensitiveConfiguration" } +module FromSensitiveConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { isSourceImpl(source, _) } - override predicate isSource(DataFlow::Node source) { isSourceImpl(source, _) } - - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _, _) and not sqlite_encryption_used() } - override predicate isSanitizer(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { node.asExpr().getUnspecifiedType() instanceof IntegralType } - override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet content) { + predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet content) { // flow out from fields at the sink (only). // constrain `content` to a field inside the node. exists(Type t | @@ -80,11 +78,13 @@ class FromSensitiveConfiguration extends TaintTracking::Configuration { } } +module FromSensitiveFlow = TaintTracking::Make; + from - FromSensitiveConfiguration config, SensitiveExpr sensitive, DataFlow::PathNode source, - DataFlow::PathNode sink, SqliteFunctionCall sqliteCall + SensitiveExpr sensitive, FromSensitiveFlow::PathNode source, FromSensitiveFlow::PathNode sink, + SqliteFunctionCall sqliteCall where - config.hasFlowPath(source, sink) and + FromSensitiveFlow::hasFlowPath(source, sink) and isSourceImpl(source.getNode(), sensitive) and isSinkImpl(sink.getNode(), sqliteCall, _) select sqliteCall, source, sink, diff --git a/cpp/ql/src/Security/CWE/CWE-319/UseOfHttp.ql b/cpp/ql/src/Security/CWE/CWE-319/UseOfHttp.ql index 25a7c585c43..a5c22775c9c 100644 --- a/cpp/ql/src/Security/CWE/CWE-319/UseOfHttp.ql +++ b/cpp/ql/src/Security/CWE/CWE-319/UseOfHttp.ql @@ -14,7 +14,7 @@ import cpp import semmle.code.cpp.ir.dataflow.TaintTracking import semmle.code.cpp.valuenumbering.GlobalValueNumbering -import DataFlow::PathGraph +import HttpStringToUrlOpen::PathGraph /** * A string matching private host names of IPv4 and IPv6, which only matches @@ -54,10 +54,8 @@ class HttpStringLiteral extends StringLiteral { /** * Taint tracking configuration for HTTP connections. */ -class HttpStringToUrlOpenConfig extends TaintTracking::Configuration { - HttpStringToUrlOpenConfig() { this = "HttpStringToUrlOpenConfig" } - - override predicate isSource(DataFlow::Node src) { +module HttpStringToUrlOpenConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { // Sources are strings containing an HTTP URL not in a private domain. src.asIndirectExpr() instanceof HttpStringLiteral and // block taint starting at `strstr`, which is likely testing an existing URL, rather than constructing an HTTP URL. @@ -67,7 +65,7 @@ class HttpStringToUrlOpenConfig extends TaintTracking::Configuration { ) } - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { // Sinks can be anything that demonstrates the string is likely to be // accessed as a URL, for example using it in a network access. Some // URLs are only ever displayed or used for data processing. @@ -91,10 +89,12 @@ class HttpStringToUrlOpenConfig extends TaintTracking::Configuration { } } +module HttpStringToUrlOpen = TaintTracking::Make; + from - HttpStringToUrlOpenConfig config, DataFlow::PathNode source, DataFlow::PathNode sink, +HttpStringToUrlOpen::PathNode source, HttpStringToUrlOpen::PathNode sink, HttpStringLiteral str where - config.hasFlowPath(source, sink) and +HttpStringToUrlOpen::hasFlowPath(source, sink) and str = source.getNode().asIndirectExpr() select str, source, sink, "This URL may be constructed with the HTTP protocol." diff --git a/cpp/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql b/cpp/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql index 202e9fb225d..f1a93845125 100644 --- a/cpp/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql +++ b/cpp/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql @@ -14,7 +14,7 @@ import cpp import semmle.code.cpp.ir.dataflow.DataFlow import semmle.code.cpp.ir.IR -import DataFlow::PathGraph +import KeyStrengthFlow::PathGraph // Gets the recommended minimum key size (in bits) of `func`, the name of an encryption function that accepts a key size as parameter `paramIndex` int getMinimumKeyStrength(string func, int paramIndex) { @@ -28,10 +28,8 @@ int getMinimumKeyStrength(string func, int paramIndex) { result = 2048 } -class KeyStrengthFlow extends DataFlow::Configuration { - KeyStrengthFlow() { this = "KeyStrengthFlow" } - - override predicate isSource(DataFlow::Node node) { +module KeyStrengthFlowConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { exists(int bits | node.asInstruction().(IntegerConstantInstruction).getValue().toInt() = bits and bits < getMinimumKeyStrength(_, _) and @@ -39,7 +37,7 @@ class KeyStrengthFlow extends DataFlow::Configuration { ) } - override predicate isSink(DataFlow::Node node) { + predicate isSink(DataFlow::Node node) { exists(FunctionCall fc, string name, int param | node.asExpr() = fc.getArgument(param) and fc.getTarget().hasGlobalName(name) and @@ -48,11 +46,13 @@ class KeyStrengthFlow extends DataFlow::Configuration { } } +module KeyStrengthFlow = DataFlow::Make; + from - DataFlow::PathNode source, DataFlow::PathNode sink, KeyStrengthFlow conf, FunctionCall fc, - int param, string name, int minimumBits, int bits + KeyStrengthFlow::PathNode source, KeyStrengthFlow::PathNode sink, FunctionCall fc, int param, + string name, int minimumBits, int bits where - conf.hasFlowPath(source, sink) and + KeyStrengthFlow::hasFlowPath(source, sink) and sink.getNode().asExpr() = fc.getArgument(param) and fc.getTarget().hasGlobalName(name) and minimumBits = getMinimumKeyStrength(name, param) and diff --git a/cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql b/cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql index 6b94ff7954b..d44d4252d78 100644 --- a/cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql +++ b/cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql @@ -15,15 +15,13 @@ import cpp import semmle.code.cpp.ir.dataflow.TaintTracking import semmle.code.cpp.models.interfaces.FlowSource -import DataFlow::PathGraph +import ExposedSystemData::PathGraph import SystemData -class ExposedSystemDataConfiguration extends TaintTracking::Configuration { - ExposedSystemDataConfiguration() { this = "ExposedSystemDataConfiguration" } +module ExposedSystemDataConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source = any(SystemData sd).getAnExpr() } - override predicate isSource(DataFlow::Node source) { source = any(SystemData sd).getAnExpr() } - - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { exists(FunctionCall fc, FunctionInput input, int arg | fc.getTarget().(RemoteFlowSinkFunction).hasRemoteFlowSink(input, _) and input.isParameterDeref(arg) and @@ -32,13 +30,15 @@ class ExposedSystemDataConfiguration extends TaintTracking::Configuration { } } -from ExposedSystemDataConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink +module ExposedSystemData = TaintTracking::Make; + +from ExposedSystemData::PathNode source, ExposedSystemData::PathNode sink where - config.hasFlowPath(source, sink) and + ExposedSystemData::hasFlowPath(source, sink) and not exists( DataFlow::Node alt // remove duplicate results on conversions | - config.hasFlow(source.getNode(), alt) and + ExposedSystemData::hasFlow(source.getNode(), alt) and alt.asConvertedExpr() = sink.getNode().asIndirectExpr() and alt != sink.getNode() ) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-078/WordexpTainted.ql b/cpp/ql/src/experimental/Security/CWE/CWE-078/WordexpTainted.ql index f4de1251e8b..290036aeb72 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-078/WordexpTainted.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-078/WordexpTainted.ql @@ -15,7 +15,7 @@ import cpp import semmle.code.cpp.ir.dataflow.TaintTracking import semmle.code.cpp.security.FlowSources -import DataFlow::PathGraph +import WordexpTaint::PathGraph /** * The `wordexp` function, which can perform command substitution. @@ -35,24 +35,24 @@ private predicate isCommandSubstitutionDisabled(FunctionCall fc) { /** * A configuration to track user-supplied data to the `wordexp` function. */ -class WordexpTaintConfiguration extends TaintTracking::Configuration { - WordexpTaintConfiguration() { this = "WordexpTaintConfiguration" } +module WordexpTaintConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof FlowSource } - override predicate isSource(DataFlow::Node source) { source instanceof FlowSource } - - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { exists(FunctionCall fc | fc.getTarget() instanceof WordexpFunction | fc.getArgument(0) = sink.asExpr() and not isCommandSubstitutionDisabled(fc) ) } - override predicate isSanitizer(DataFlow::Node node) { + predicate isBarrier(DataFlow::Node node) { node.asExpr().getUnspecifiedType() instanceof IntegralType } } -from WordexpTaintConfiguration conf, DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode -where conf.hasFlowPath(sourceNode, sinkNode) +module WordexpTaint = TaintTracking::Make; + +from WordexpTaint::PathNode sourceNode, WordexpTaint::PathNode sinkNode +where WordexpTaint::hasFlowPath(sourceNode, sinkNode) select sinkNode.getNode(), sourceNode, sinkNode, "Using user-supplied data in a `wordexp` command, without disabling command substitution, can make code vulnerable to command injection." diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql b/cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql index 15e18c1255c..71958eaad91 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql @@ -15,12 +15,10 @@ import cpp import semmle.code.cpp.models.interfaces.Allocation import semmle.code.cpp.ir.dataflow.DataFlow -import DataFlow::PathGraph +import MultToAlloc::PathGraph -class MultToAllocConfig extends DataFlow::Configuration { - MultToAllocConfig() { this = "MultToAllocConfig" } - - override predicate isSource(DataFlow::Node node) { +module MultToAllocConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { // a multiplication of two non-constant expressions exists(MulExpr me | me = node.asExpr() and @@ -28,14 +26,16 @@ class MultToAllocConfig extends DataFlow::Configuration { ) } - override predicate isSink(DataFlow::Node node) { + predicate isSink(DataFlow::Node node) { // something that affects an allocation size node.asExpr() = any(HeuristicAllocationExpr ae).getSizeExpr().getAChild*() } } -from MultToAllocConfig config, DataFlow::PathNode source, DataFlow::PathNode sink -where config.hasFlowPath(source, sink) +module MultToAlloc = DataFlow::Make; + +from MultToAlloc::PathNode source, MultToAlloc::PathNode sink +where MultToAlloc::hasFlowPath(source, sink) select sink, source, sink, "Potentially overflowing value from $@ is used in the size of this allocation.", source, "multiplication"