From 460eddd7812e2b50592d0679c549ad5f207921d9 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 16 Aug 2022 08:55:49 +0200 Subject: [PATCH 001/174] add ql/override-any --- .../performance/VarUnusedInDisjunctQuery.qll | 27 +++++++++++++ .../performance/VarUnusedInDisjunct.ql | 27 +------------ ql/ql/src/queries/style/OverrideAny.ql | 40 +++++++++++++++++++ 3 files changed, 68 insertions(+), 26 deletions(-) create mode 100644 ql/ql/src/codeql_ql/performance/VarUnusedInDisjunctQuery.qll create mode 100644 ql/ql/src/queries/style/OverrideAny.ql diff --git a/ql/ql/src/codeql_ql/performance/VarUnusedInDisjunctQuery.qll b/ql/ql/src/codeql_ql/performance/VarUnusedInDisjunctQuery.qll new file mode 100644 index 00000000000..503ad6db3ad --- /dev/null +++ b/ql/ql/src/codeql_ql/performance/VarUnusedInDisjunctQuery.qll @@ -0,0 +1,27 @@ +import ql + +/** + * Holds if we assume `t` is a small type, and + * variables of this type are therefore not an issue in cartesian products. + */ +predicate isSmallType(Type t) { + t.getName() = "string" // DataFlow::Configuration and the like + or + exists(NewType newType | newType = t.getDeclaration() | + forex(NewTypeBranch branch | branch = newType.getABranch() | branch.getArity() = 0) + ) + or + t.getName() = "boolean" + or + exists(NewType newType | newType = t.getDeclaration() | + forex(NewTypeBranch branch | branch = newType.getABranch() | + isSmallType(branch.getReturnType()) + ) + ) + or + exists(NewTypeBranch branch | t = branch.getReturnType() | + forall(Type param | param = branch.getParameterType(_) | isSmallType(param)) + ) + or + isSmallType(t.getASuperType()) +} diff --git a/ql/ql/src/queries/performance/VarUnusedInDisjunct.ql b/ql/ql/src/queries/performance/VarUnusedInDisjunct.ql index c26b47554fe..2d85b872153 100644 --- a/ql/ql/src/queries/performance/VarUnusedInDisjunct.ql +++ b/ql/ql/src/queries/performance/VarUnusedInDisjunct.ql @@ -10,6 +10,7 @@ */ import ql +import codeql_ql.performance.VarUnusedInDisjunctQuery /** * Holds if `node` bind `var` in a (transitive) child node. @@ -48,32 +49,6 @@ predicate alwaysBindsVar(VarDef var, AstNode node) { exists(IfFormula ifForm | ifForm = node | alwaysBindsVar(var, ifForm.getCondition())) } -/** - * Holds if we assume `t` is a small type, and - * variables of this type are therefore not an issue in cartesian products. - */ -predicate isSmallType(Type t) { - t.getName() = "string" // DataFlow::Configuration and the like - or - exists(NewType newType | newType = t.getDeclaration() | - forex(NewTypeBranch branch | branch = newType.getABranch() | branch.getArity() = 0) - ) - or - t.getName() = "boolean" - or - exists(NewType newType | newType = t.getDeclaration() | - forex(NewTypeBranch branch | branch = newType.getABranch() | - isSmallType(branch.getReturnType()) - ) - ) - or - exists(NewTypeBranch branch | t = branch.getReturnType() | - forall(Type param | param = branch.getParameterType(_) | isSmallType(param)) - ) - or - isSmallType(t.getASuperType()) -} - /** * Holds if `pred` is inlined. */ diff --git a/ql/ql/src/queries/style/OverrideAny.ql b/ql/ql/src/queries/style/OverrideAny.ql new file mode 100644 index 00000000000..0c7d82fd3c8 --- /dev/null +++ b/ql/ql/src/queries/style/OverrideAny.ql @@ -0,0 +1,40 @@ +/** + * @name Override with unmentioned parameter + * @description A predicate that overrides the default behavior but doesn't mention a parameter is suspicious. + * @kind problem + * @problem.severity warning + * @id ql/override-any + * @precision very-high + */ + +import ql +import codeql_ql.performance.VarUnusedInDisjunctQuery + +AstNode param(Predicate pred, string name, Type t) { + result = pred.getParameter(_) and + result.(VarDecl).getName() = name and + result.(VarDecl).getType() = t + or + result = pred.getReturnTypeExpr() and + name = "result" and + t = pred.getReturnType() +} + +predicate hasAccess(Predicate pred, string name) { + exists(param(pred, name, _).(VarDecl).getAnAccess()) + or + name = "result" and + exists(param(pred, name, _)) and + exists(ResultAccess res | res.getEnclosingPredicate() = pred) +} + +from Predicate pred, AstNode param, string name, Type paramType +where + pred.hasAnnotation("override") and + param = param(pred, name, paramType) and + not hasAccess(pred, name) and + not pred.getBody() instanceof NoneCall and + exists(pred.getBody()) and + not isSmallType(pred.getParent().(Class).getType()) and + not isSmallType(paramType) +select pred, "Override predicate doesn't mention $@.", param, name From 1e3adcd14e172aff3a0a03c89181ca31ca091080 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 2 Nov 2022 11:37:37 +0100 Subject: [PATCH 002/174] Revert "Revert "SSA: Turn consistency predicates into `query` predicates"" --- .../ql/consistency-queries/SsaConsistency.ql | 12 ++--------- ruby/ql/consistency-queries/SsaConsistency.ql | 12 ++--------- shared/ssa/codeql/ssa/Ssa.qll | 20 +++++++++++-------- 3 files changed, 16 insertions(+), 28 deletions(-) diff --git a/csharp/ql/consistency-queries/SsaConsistency.ql b/csharp/ql/consistency-queries/SsaConsistency.ql index 71f88bf2ab0..bd666a71e49 100644 --- a/csharp/ql/consistency-queries/SsaConsistency.ql +++ b/csharp/ql/consistency-queries/SsaConsistency.ql @@ -1,8 +1,8 @@ import csharp -import semmle.code.csharp.dataflow.internal.SsaImpl::Consistency as Consistency +import semmle.code.csharp.dataflow.internal.SsaImpl::Consistency import Ssa -class MyRelevantDefinition extends Consistency::RelevantDefinition, Ssa::Definition { +class MyRelevantDefinition extends RelevantDefinition, Ssa::Definition { override predicate hasLocationInfo( string filepath, int startline, int startcolumn, int endline, int endcolumn ) { @@ -10,14 +10,6 @@ class MyRelevantDefinition extends Consistency::RelevantDefinition, Ssa::Definit } } -query predicate nonUniqueDef = Consistency::nonUniqueDef/4; - -query predicate readWithoutDef = Consistency::readWithoutDef/3; - -query predicate deadDef = Consistency::deadDef/2; - -query predicate notDominatedByDef = Consistency::notDominatedByDef/4; - query predicate localDeclWithSsaDef(LocalVariableDeclExpr d) { // Local variables in C# must be initialized before every use, so uninitialized // local variables should not have an SSA definition, as that would imply that diff --git a/ruby/ql/consistency-queries/SsaConsistency.ql b/ruby/ql/consistency-queries/SsaConsistency.ql index 54c1b149ab2..7ba9262baa4 100644 --- a/ruby/ql/consistency-queries/SsaConsistency.ql +++ b/ruby/ql/consistency-queries/SsaConsistency.ql @@ -1,18 +1,10 @@ import codeql.ruby.dataflow.SSA -import codeql.ruby.dataflow.internal.SsaImpl::Consistency as Consistency +import codeql.ruby.dataflow.internal.SsaImpl::Consistency -class MyRelevantDefinition extends Consistency::RelevantDefinition, Ssa::Definition { +class MyRelevantDefinition extends RelevantDefinition, Ssa::Definition { override predicate hasLocationInfo( string filepath, int startline, int startcolumn, int endline, int endcolumn ) { this.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) } } - -query predicate nonUniqueDef = Consistency::nonUniqueDef/4; - -query predicate readWithoutDef = Consistency::readWithoutDef/3; - -query predicate deadDef = Consistency::deadDef/2; - -query predicate notDominatedByDef = Consistency::notDominatedByDef/4; diff --git a/shared/ssa/codeql/ssa/Ssa.qll b/shared/ssa/codeql/ssa/Ssa.qll index 886e4128e26..19f31f7c8bb 100644 --- a/shared/ssa/codeql/ssa/Ssa.qll +++ b/shared/ssa/codeql/ssa/Ssa.qll @@ -9,7 +9,10 @@ signature module InputSig { * A basic block, that is, a maximal straight-line sequence of control flow nodes * without branches or joins. */ - class BasicBlock; + class BasicBlock { + /** Gets a textual representation of this basic block. */ + string toString(); + } /** * Gets the basic block that immediately dominates basic block `bb`, if any. @@ -43,7 +46,10 @@ signature module InputSig { class ExitBasicBlock extends BasicBlock; /** A variable that can be SSA converted. */ - class SourceVariable; + class SourceVariable { + /** Gets a textual representation of this variable. */ + string toString(); + } /** * Holds if the `i`th node of basic block `bb` is a (potential) write to source @@ -846,8 +852,6 @@ module Make { } /** Provides a set of consistency queries. */ - // TODO: Make these `query` predicates once class signatures are supported - // (`SourceVariable` and `BasicBlock` must have `toString`) module Consistency { /** A definition that is relevant for the consistency queries. */ abstract class RelevantDefinition extends Definition { @@ -858,19 +862,19 @@ module Make { } /** Holds if a read can be reached from multiple definitions. */ - predicate nonUniqueDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) { + query predicate nonUniqueDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) { ssaDefReachesRead(v, def, bb, i) and not exists(unique(Definition def0 | ssaDefReachesRead(v, def0, bb, i))) } /** Holds if a read cannot be reached from a definition. */ - predicate readWithoutDef(SourceVariable v, BasicBlock bb, int i) { + query predicate readWithoutDef(SourceVariable v, BasicBlock bb, int i) { variableRead(bb, i, v, _) and not ssaDefReachesRead(v, _, bb, i) } /** Holds if a definition cannot reach a read. */ - predicate deadDef(RelevantDefinition def, SourceVariable v) { + query predicate deadDef(RelevantDefinition def, SourceVariable v) { v = def.getSourceVariable() and not ssaDefReachesRead(_, def, _, _) and not phiHasInputFromBlock(_, def, _) and @@ -878,7 +882,7 @@ module Make { } /** Holds if a read is not dominated by a definition. */ - predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) { + query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) { exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) | ssaDefReachesReadWithinBlock(v, def, bb, i) and (bb != bbDef or i < iDef) From dc6f60a5015bf6b81a8c98c537cf449ea90bc5bf Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 2 Nov 2022 16:10:29 +0100 Subject: [PATCH 003/174] Add new XXE query Only XMLParser sinks for the time being --- .../codeql/swift/dataflow/ExternalFlow.qll | 2 + .../swift/frameworks/StandardLibrary/Data.qll | 6 ++ .../StandardLibrary/InputStream.qll | 8 +++ swift/ql/lib/codeql/swift/security/XXE.qll | 67 +++++++++++++++++++ .../ql/lib/codeql/swift/security/XXEQuery.qll | 27 ++++++++ .../ql/src/queries/Security/CWE-611/XXE.qhelp | 57 ++++++++++++++++ swift/ql/src/queries/Security/CWE-611/XXE.ql | 24 +++++++ .../src/queries/Security/CWE-611/XXEBad.swift | 2 + .../queries/Security/CWE-611/XXEGood.swift | 2 + .../Security/CWE-611/XXETest.expected | 0 .../query-tests/Security/CWE-611/XXETest.ql | 20 ++++++ .../Security/CWE-611/testXXE.swift | 58 ++++++++++++++++ 12 files changed, 273 insertions(+) create mode 100644 swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Data.qll create mode 100644 swift/ql/lib/codeql/swift/frameworks/StandardLibrary/InputStream.qll create mode 100644 swift/ql/lib/codeql/swift/security/XXE.qll create mode 100644 swift/ql/lib/codeql/swift/security/XXEQuery.qll create mode 100644 swift/ql/src/queries/Security/CWE-611/XXE.qhelp create mode 100644 swift/ql/src/queries/Security/CWE-611/XXE.ql create mode 100644 swift/ql/src/queries/Security/CWE-611/XXEBad.swift create mode 100644 swift/ql/src/queries/Security/CWE-611/XXEGood.swift create mode 100644 swift/ql/test/query-tests/Security/CWE-611/XXETest.expected create mode 100644 swift/ql/test/query-tests/Security/CWE-611/XXETest.ql create mode 100644 swift/ql/test/query-tests/Security/CWE-611/testXXE.swift diff --git a/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll b/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll index 28e783c8047..4d563525830 100644 --- a/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll +++ b/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll @@ -79,6 +79,8 @@ private import internal.FlowSummaryImplSpecific */ private module Frameworks { private import codeql.swift.frameworks.StandardLibrary.CustomUrlSchemes + private import codeql.swift.frameworks.StandardLibrary.Data + private import codeql.swift.frameworks.StandardLibrary.InputStream private import codeql.swift.frameworks.StandardLibrary.String private import codeql.swift.frameworks.StandardLibrary.Url private import codeql.swift.frameworks.StandardLibrary.UrlSession diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Data.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Data.qll new file mode 100644 index 00000000000..740fc8874b6 --- /dev/null +++ b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Data.qll @@ -0,0 +1,6 @@ +import swift +private import codeql.swift.dataflow.ExternalFlow + +private class DataSummaries extends SummaryModelCsv { + override predicate row(string row) { row = ";Data;true;init(_:);;;Argument[0];ReturnValue;taint" } +} diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/InputStream.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/InputStream.qll new file mode 100644 index 00000000000..60b61bec294 --- /dev/null +++ b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/InputStream.qll @@ -0,0 +1,8 @@ +import swift +private import codeql.swift.dataflow.ExternalFlow + +private class InputStreamSummaries extends SummaryModelCsv { + override predicate row(string row) { + row = ";InputStream;true;init(data:);;;Argument[0];ReturnValue;taint" + } +} diff --git a/swift/ql/lib/codeql/swift/security/XXE.qll b/swift/ql/lib/codeql/swift/security/XXE.qll new file mode 100644 index 00000000000..eecbd147041 --- /dev/null +++ b/swift/ql/lib/codeql/swift/security/XXE.qll @@ -0,0 +1,67 @@ +/** Provides classes and predicates to reason about XML external entities (XXE) vulnerabilities. */ + +import swift +private import codeql.swift.dataflow.DataFlow +private import codeql.swift.dataflow.internal.DataFlowPrivate + +/** A data flow sink for XML external entities (XXE) vulnerabilities. */ +abstract class XxeSink extends DataFlow::Node { } + +/** A sanitizer for XML external entities (XXE) vulnerabilities. */ +abstract class XxeSanitizer extends DataFlow::Node { } + +/** + * A unit class for adding additional taint steps. + * + * Extend this class to add additional taint steps that should apply to paths related to + * XML external entities (XXE) vulnerabilities. + */ +class XxeAdditionalTaintStep extends Unit { + abstract predicate step(DataFlow::Node n1, DataFlow::Node n2); +} + +/** The XML argument of a `XMLParser` vulnerable to XXE. */ +private class DefaultXxeSink extends XxeSink { + DefaultXxeSink() { + this.asExpr() = any(Argument a | a.getApplyExpr() instanceof VulnerableParser).getExpr() + } +} + +/** The construction of a `XMLParser` that enables external entities. */ +private class VulnerableParser extends CallExpr { + VulnerableParser() { + resolvesExternalEntities(this) and this.getFunction() instanceof ConstructorRefCallExpr + } +} + +/** Holds if there is an access of `ref` that sets `shouldResolveExternalEntities` to `true`. */ +private predicate resolvesExternalEntities(XmlParserRef ref) { + exists(XmlParserRef base | + DataFlow::localExprFlow(ref, base) or DataFlow::localExprFlow(base, ref) + | + exists(AssignExpr assign, ShouldResolveExternalEntities s, BooleanLiteralExpr b | + s.getBase() = base and + assign.getDest() = s and + b.getValue() = true and + DataFlow::localExprFlow(b, assign.getSource()) + ) + ) +} + +/** A reference to the field `XMLParser.shouldResolveExternalEntities`. */ +private class ShouldResolveExternalEntities extends MemberRefExpr { + ShouldResolveExternalEntities() { + this.getMember().(FieldDecl).getName() = "shouldResolveExternalEntities" and + this.getBase() instanceof XmlParserRef + } +} + +/** An expression of type `XMLParser`. */ +private class XmlParserRef extends Expr { + XmlParserRef() { this.getType() instanceof XmlParserType } +} + +/** The type `XMLParser`. */ +private class XmlParserType extends NominalType { + XmlParserType() { this.getFullName() = "XMLParser" } +} diff --git a/swift/ql/lib/codeql/swift/security/XXEQuery.qll b/swift/ql/lib/codeql/swift/security/XXEQuery.qll new file mode 100644 index 00000000000..2b5dc5ea3ea --- /dev/null +++ b/swift/ql/lib/codeql/swift/security/XXEQuery.qll @@ -0,0 +1,27 @@ +/** + * Provides a taint-tracking configuration for reasoning about XML external entities + * (XXE) vulnerabilities. + */ + +import swift +import codeql.swift.dataflow.DataFlow +import codeql.swift.dataflow.FlowSources +import codeql.swift.dataflow.TaintTracking +import codeql.swift.security.XXE + +/** + * A taint-tracking configuration for XML external entities (XXE) vulnerabilities. + */ +class XxeConfiguration extends TaintTracking::Configuration { + XxeConfiguration() { this = "XxeConfiguration" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof XxeSink } + + override predicate isSanitizer(DataFlow::Node sanitizer) { sanitizer instanceof XxeSanitizer } + + override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) { + any(XxeAdditionalTaintStep s).step(n1, n2) + } +} diff --git a/swift/ql/src/queries/Security/CWE-611/XXE.qhelp b/swift/ql/src/queries/Security/CWE-611/XXE.qhelp new file mode 100644 index 00000000000..0aca8ac474b --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-611/XXE.qhelp @@ -0,0 +1,57 @@ + + + + +

+Parsing untrusted XML files with a weakly configured XML parser may lead to an XML External Entity (XXE) attack. This type of attack +uses external entity references to access arbitrary files on a system, carry out denial of service, or server side +request forgery. Even when the result of parsing is not returned to the user, out-of-band +data retrieval techniques may allow attackers to steal sensitive data. Denial of services can also be +carried out in this situation. +

+ + + +

+The easiest way to prevent XXE attacks is to disable external entity handling when +parsing untrusted data. How this is done depends on the library being used. Note that some +libraries, such as recent versions of XMLParser, disable entity expansion by default, +so unless you have explicitly enabled entity expansion, no further action needs to be taken. +

+ + + +

+The following example uses the XMLParser class to parse a string data. +If that string is from an untrusted source, this code may be vulnerable to an XXE attack, since +the parser is also setting its shouldResolveExternalEntities option to true: +

+ + +

+To guard against XXE attacks, the shouldResolveExternalEntities option should be +left unset or explicitly set to false. +

+ + + + + +
  • +OWASP: +XML External Entity (XXE) Processing. +
    • +
  • +OWASP: +XML External Entity Prevention Cheat Sheet. +
    • +
  • +Timothy Morgen: +XML Schema, DTD, and Entity Attacks. +
    • +
  • +Timur Yunusov, Alexey Osipov: +XML Out-Of-Band Data Retrieval. +
    • +     +     diff --git a/swift/ql/src/queries/Security/CWE-611/XXE.ql b/swift/ql/src/queries/Security/CWE-611/XXE.ql new file mode 100644 index 00000000000..288da0aad9d --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-611/XXE.ql @@ -0,0 +1,24 @@ +/** + * @name Resolving XML external entity in user-controlled data + * @description Parsing user-controlled XML documents and allowing expansion of external entity + * references may lead to disclosure of confidential data or denial of service. + * @kind path-problem + * @problem.severity error + * @security-severity 9.1 + * @precision high + * @id swift/xxe + * @tags security + * external/cwe/cwe-611 + * external/cwe/cwe-776 + * external/cwe/cwe-827 + */ + +import swift +import codeql.swift.dataflow.DataFlow +import codeql.swift.security.XXEQuery +import DataFlow::PathGraph + +from DataFlow::PathNode source, DataFlow::PathNode sink +where any(XxeConfiguration c).hasFlowPath(source, sink) +select sink.getNode(), source, sink, "XML parser with enabled external entities depends on $@.", + source.getNode(), "user input" diff --git a/swift/ql/src/queries/Security/CWE-611/XXEBad.swift b/swift/ql/src/queries/Security/CWE-611/XXEBad.swift new file mode 100644 index 00000000000..19ff4a7383d --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-611/XXEBad.swift @@ -0,0 +1,2 @@ +let parser = XMLParser(data: remoteData) // BAD (parser explicitly enables external entities) +parser.shouldResolveExternalEntities = true diff --git a/swift/ql/src/queries/Security/CWE-611/XXEGood.swift b/swift/ql/src/queries/Security/CWE-611/XXEGood.swift new file mode 100644 index 00000000000..5552186c759 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-611/XXEGood.swift @@ -0,0 +1,2 @@ +let parser = XMLParser(data: remoteData) // GOOD (parser explicitly disables external entities) +parser.shouldResolveExternalEntities = false diff --git a/swift/ql/test/query-tests/Security/CWE-611/XXETest.expected b/swift/ql/test/query-tests/Security/CWE-611/XXETest.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/swift/ql/test/query-tests/Security/CWE-611/XXETest.ql b/swift/ql/test/query-tests/Security/CWE-611/XXETest.ql new file mode 100644 index 00000000000..817f55678ad --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-611/XXETest.ql @@ -0,0 +1,20 @@ +import swift +import codeql.swift.security.XXEQuery +import TestUtilities.InlineExpectationsTest + +class XxeTest extends InlineExpectationsTest { + XxeTest() { this = "XxeTest" } + + override string getARelevantTag() { result = "hasXXE" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + exists(XxeConfiguration config, DataFlow::Node source, DataFlow::Node sink, Expr sinkExpr | + config.hasFlow(source, sink) and + sinkExpr = sink.asExpr() and + location = sinkExpr.getLocation() and + element = sinkExpr.toString() and + tag = "hasXXE" and + value = source.asExpr().getLocation().getStartLine().toString() + ) + } +} diff --git a/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift b/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift new file mode 100644 index 00000000000..b98385463ab --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift @@ -0,0 +1,58 @@ +// --- stubs --- + +class Data { + init(_ elements: S) {} +} + +struct URL { + init?(string: String) {} +} + +class InputStream { + init(data: Data) {} +} + +extension String { + init(contentsOf: URL) { + let data = "" + self.init(data) + } +} + +class XMLParser { + var shouldResolveExternalEntities: Bool { get { return false } set {} } + init?(contentsOf: URL) {} + init(data: Data) {} + init(stream: InputStream) {} +} + +// --- tests --- + +func testData() { + let remoteString = String(contentsOf: URL(string: "http://example.com/")!) + let remoteData = Data(remoteString) + let parser = XMLParser(data: remoteData) // $ hasXXE=32 + parser.shouldResolveExternalEntities = true +} + +func testInputStream() { + let remoteString = String(contentsOf: URL(string: "http://example.com/")!) + let remoteData = Data(remoteString) + let remoteStream = InputStream(data: remoteData) + let parser = XMLParser(stream: remoteStream) // $ hasXXE=39 + parser.shouldResolveExternalEntities = true + +} + +func testDataSafe() { + let remoteString = String(contentsOf: URL(string: "http://example.com/")!) + let remoteData = Data(remoteString) + let _ = XMLParser(data: remoteData) // NO XXE: parser doesn't enable external entities +} + +func testInputStreamSafe() { + let remoteString = String(contentsOf: URL(string: "http://example.com/")!) + let remoteData = Data(remoteString) + let remoteStream = InputStream(data: remoteData) + let _ = XMLParser(stream: remoteStream) // NO XXE: parser doesn't enable external entities +} \ No newline at end of file From f4047e016cb5831769081c6def3a0cd11fad886b Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 2 Nov 2022 16:21:17 +0100 Subject: [PATCH 004/174] Address QL-for-QL alert Use an alert message consistent with the other languages --- swift/ql/src/queries/Security/CWE-611/XXE.ql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/swift/ql/src/queries/Security/CWE-611/XXE.ql b/swift/ql/src/queries/Security/CWE-611/XXE.ql index 288da0aad9d..04c27949c11 100644 --- a/swift/ql/src/queries/Security/CWE-611/XXE.ql +++ b/swift/ql/src/queries/Security/CWE-611/XXE.ql @@ -20,5 +20,6 @@ import DataFlow::PathGraph from DataFlow::PathNode source, DataFlow::PathNode sink where any(XxeConfiguration c).hasFlowPath(source, sink) -select sink.getNode(), source, sink, "XML parser with enabled external entities depends on $@.", - source.getNode(), "user input" +select sink.getNode(), source, sink, + "XML parsing depends on a $@ without guarding against external entity expansion.", + source.getNode(), "user-provided value" From 0c6957ea785be63235d1cb8f61d86526681ec172 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 2 Nov 2022 17:05:55 +0100 Subject: [PATCH 005/174] Adjust test expectations of a query affected by new summaries --- .../Security/CWE-311/CleartextTransmission.expected | 11 +++++++++++ .../test/query-tests/Security/CWE-311/testSend.swift | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected b/swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected index de02d0db461..5580af94d22 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected +++ b/swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.expected @@ -1,4 +1,8 @@ edges +| testSend.swift:5:5:5:29 | [summary param] 0 in init(_:) : | file://:0:0:0:0 | [summary] to write: return (return) in init(_:) : | +| testSend.swift:33:14:33:32 | call to init(_:) : | testSend.swift:37:19:37:19 | data2 | +| testSend.swift:33:19:33:19 | passwordPlain : | testSend.swift:5:5:5:29 | [summary param] 0 in init(_:) : | +| testSend.swift:33:19:33:19 | passwordPlain : | testSend.swift:33:14:33:32 | call to init(_:) : | | testSend.swift:41:10:41:18 | data : | testSend.swift:41:45:41:45 | data : | | testSend.swift:45:13:45:13 | password : | testSend.swift:52:27:52:27 | str1 | | testSend.swift:46:13:46:13 | password : | testSend.swift:53:27:53:27 | str2 | @@ -8,7 +12,12 @@ edges | testURL.swift:13:54:13:54 | passwd : | testURL.swift:13:22:13:54 | ... .+(_:_:) ... | | testURL.swift:16:55:16:55 | credit_card_no : | testURL.swift:16:22:16:55 | ... .+(_:_:) ... | nodes +| file://:0:0:0:0 | [summary] to write: return (return) in init(_:) : | semmle.label | [summary] to write: return (return) in init(_:) : | +| testSend.swift:5:5:5:29 | [summary param] 0 in init(_:) : | semmle.label | [summary param] 0 in init(_:) : | | testSend.swift:29:19:29:19 | passwordPlain | semmle.label | passwordPlain | +| testSend.swift:33:14:33:32 | call to init(_:) : | semmle.label | call to init(_:) : | +| testSend.swift:33:19:33:19 | passwordPlain : | semmle.label | passwordPlain : | +| testSend.swift:37:19:37:19 | data2 | semmle.label | data2 | | testSend.swift:41:10:41:18 | data : | semmle.label | data : | | testSend.swift:41:45:41:45 | data : | semmle.label | data : | | testSend.swift:45:13:45:13 | password : | semmle.label | password : | @@ -24,9 +33,11 @@ nodes | testURL.swift:16:55:16:55 | credit_card_no : | semmle.label | credit_card_no : | | testURL.swift:20:22:20:22 | passwd | semmle.label | passwd | subpaths +| testSend.swift:33:19:33:19 | passwordPlain : | testSend.swift:5:5:5:29 | [summary param] 0 in init(_:) : | file://:0:0:0:0 | [summary] to write: return (return) in init(_:) : | testSend.swift:33:14:33:32 | call to init(_:) : | | testSend.swift:47:17:47:17 | password : | testSend.swift:41:10:41:18 | data : | testSend.swift:41:45:41:45 | data : | testSend.swift:47:13:47:25 | call to pad(_:) : | #select | testSend.swift:29:19:29:19 | passwordPlain | testSend.swift:29:19:29:19 | passwordPlain | testSend.swift:29:19:29:19 | passwordPlain | This operation transmits 'passwordPlain', which may contain unencrypted sensitive data from $@. | testSend.swift:29:19:29:19 | passwordPlain | passwordPlain | +| testSend.swift:37:19:37:19 | data2 | testSend.swift:33:19:33:19 | passwordPlain : | testSend.swift:37:19:37:19 | data2 | This operation transmits 'data2', which may contain unencrypted sensitive data from $@. | testSend.swift:33:19:33:19 | passwordPlain : | passwordPlain | | testSend.swift:52:27:52:27 | str1 | testSend.swift:45:13:45:13 | password : | testSend.swift:52:27:52:27 | str1 | This operation transmits 'str1', which may contain unencrypted sensitive data from $@. | testSend.swift:45:13:45:13 | password : | password | | testSend.swift:53:27:53:27 | str2 | testSend.swift:46:13:46:13 | password : | testSend.swift:53:27:53:27 | str2 | This operation transmits 'str2', which may contain unencrypted sensitive data from $@. | testSend.swift:46:13:46:13 | password : | password | | testSend.swift:54:27:54:27 | str3 | testSend.swift:47:17:47:17 | password : | testSend.swift:54:27:54:27 | str3 | This operation transmits 'str3', which may contain unencrypted sensitive data from $@. | testSend.swift:47:17:47:17 | password : | password | diff --git a/swift/ql/test/query-tests/Security/CWE-311/testSend.swift b/swift/ql/test/query-tests/Security/CWE-311/testSend.swift index 506b3a921e3..aaf2e3487f2 100644 --- a/swift/ql/test/query-tests/Security/CWE-311/testSend.swift +++ b/swift/ql/test/query-tests/Security/CWE-311/testSend.swift @@ -34,7 +34,7 @@ func test1(passwordPlain : String, passwordHash : String) { let data3 = Data(passwordHash) nw.send(content: data1, completion: .idempotent) // GOOD (not sensitive) - nw.send(content: data2, completion: .idempotent) // BAD [NOT DETECTED] + nw.send(content: data2, completion: .idempotent) // BAD nw.send(content: data3, completion: .idempotent) // GOOD (not sensitive) } From fe138dc0a176bc8535ecb00bd00d5ad4139dd4d2 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 3 Nov 2022 09:29:57 +0100 Subject: [PATCH 006/174] Add explicitly safe test cases --- .../query-tests/Security/CWE-611/testXXE.swift | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift b/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift index b98385463ab..63244e082e9 100644 --- a/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift +++ b/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift @@ -50,9 +50,25 @@ func testDataSafe() { let _ = XMLParser(data: remoteData) // NO XXE: parser doesn't enable external entities } +func testDataSafeExplicit() { + let remoteString = String(contentsOf: URL(string: "http://example.com/")!) + let remoteData = Data(remoteString) + let parser = XMLParser(data: remoteData) // NO XXE: parser disables external entities + parser.shouldResolveExternalEntities = false + +} + func testInputStreamSafe() { let remoteString = String(contentsOf: URL(string: "http://example.com/")!) let remoteData = Data(remoteString) let remoteStream = InputStream(data: remoteData) let _ = XMLParser(stream: remoteStream) // NO XXE: parser doesn't enable external entities +} + +func testInputStreamSafeExplicit() { + let remoteString = String(contentsOf: URL(string: "http://example.com/")!) + let remoteData = Data(remoteString) + let remoteStream = InputStream(data: remoteData) + let parser = XMLParser(stream: remoteStream) // NO XXE: parser disables external entities + parser.shouldResolveExternalEntities = false } \ No newline at end of file From 3e1819f25de28bf4bf44d8ccddd3f44a60206b00 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 3 Nov 2022 09:49:13 +0100 Subject: [PATCH 007/174] Model XMLParser constructor init(contentsOf:) --- swift/ql/lib/codeql/swift/security/XXE.qll | 5 ++++- .../Security/CWE-611/testXXE.swift | 20 ++++++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/swift/ql/lib/codeql/swift/security/XXE.qll b/swift/ql/lib/codeql/swift/security/XXE.qll index eecbd147041..c3139ed974b 100644 --- a/swift/ql/lib/codeql/swift/security/XXE.qll +++ b/swift/ql/lib/codeql/swift/security/XXE.qll @@ -58,7 +58,10 @@ private class ShouldResolveExternalEntities extends MemberRefExpr { /** An expression of type `XMLParser`. */ private class XmlParserRef extends Expr { - XmlParserRef() { this.getType() instanceof XmlParserType } + XmlParserRef() { + this.getType() instanceof XmlParserType or + this.getType() = any(OptionalType t | t.getBaseType() instanceof XmlParserType) + } } /** The type `XMLParser`. */ diff --git a/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift b/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift index 63244e082e9..75538f014f9 100644 --- a/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift +++ b/swift/ql/test/query-tests/Security/CWE-611/testXXE.swift @@ -41,7 +41,13 @@ func testInputStream() { let remoteStream = InputStream(data: remoteData) let parser = XMLParser(stream: remoteStream) // $ hasXXE=39 parser.shouldResolveExternalEntities = true +} +func testUrl() { + let remoteString = String(contentsOf: URL(string: "http://example.com/")!) + let remoteUrl = URL(string: remoteString)! + let parser = XMLParser(contentsOf: remoteUrl) // $ hasXXE=47 + parser?.shouldResolveExternalEntities = true } func testDataSafe() { @@ -55,7 +61,6 @@ func testDataSafeExplicit() { let remoteData = Data(remoteString) let parser = XMLParser(data: remoteData) // NO XXE: parser disables external entities parser.shouldResolveExternalEntities = false - } func testInputStreamSafe() { @@ -71,4 +76,17 @@ func testInputStreamSafeExplicit() { let remoteStream = InputStream(data: remoteData) let parser = XMLParser(stream: remoteStream) // NO XXE: parser disables external entities parser.shouldResolveExternalEntities = false +} + +func testUrlSafe() { + let remoteString = String(contentsOf: URL(string: "http://example.com/")!) + let remoteUrl = URL(string: remoteString)! + let _ = XMLParser(contentsOf: remoteUrl) // NO XXE: parser doesn't enable external entities +} + +func testUrlSafeExplicit() { + let remoteString = String(contentsOf: URL(string: "http://example.com/")!) + let remoteUrl = URL(string: remoteString)! + let parser = XMLParser(contentsOf: remoteUrl) // NO XXE: parser disables external entities + parser?.shouldResolveExternalEntities = false } \ No newline at end of file From da67b1059cca6fa83849252ca8fc2ab56587f8fc Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 3 Nov 2022 12:38:45 +0100 Subject: [PATCH 008/174] Remove (now unnecessary) import --- swift/ql/lib/codeql/swift/security/XXE.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/swift/ql/lib/codeql/swift/security/XXE.qll b/swift/ql/lib/codeql/swift/security/XXE.qll index c3139ed974b..506225f54a1 100644 --- a/swift/ql/lib/codeql/swift/security/XXE.qll +++ b/swift/ql/lib/codeql/swift/security/XXE.qll @@ -2,7 +2,6 @@ import swift private import codeql.swift.dataflow.DataFlow -private import codeql.swift.dataflow.internal.DataFlowPrivate /** A data flow sink for XML external entities (XXE) vulnerabilities. */ abstract class XxeSink extends DataFlow::Node { } From a7ecdef2a6ea66d2acf42d62b6180c0c6fafbd32 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 3 Nov 2022 11:23:03 +0000 Subject: [PATCH 009/174] Swift: Add dataflow tests for tuples. --- .../dataflow/dataflow/LocalFlow.expected | 30 +++++++++++++ .../dataflow/dataflow/test.swift | 43 ++++++++++++++++++- 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected b/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected index b0e95ee6946..2e945f96294 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected +++ b/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected @@ -192,3 +192,33 @@ | test.swift:266:15:266:15 | x | test.swift:267:15:267:15 | x | | test.swift:266:15:266:25 | call to signum() | test.swift:266:15:266:25 | OptionalEvaluationExpr | | test.swift:267:15:267:15 | x | test.swift:268:16:268:16 | x | +| test.swift:277:9:277:9 | WriteDef | test.swift:279:15:279:15 | t1 | +| test.swift:277:14:277:26 | (...) | test.swift:277:9:277:9 | WriteDef | +| test.swift:279:15:279:15 | t1 | test.swift:280:15:280:15 | t1 | +| test.swift:280:15:280:15 | t1 | test.swift:281:15:281:15 | t1 | +| test.swift:281:15:281:15 | t1 | test.swift:283:5:283:5 | t1 | +| test.swift:283:5:283:5 | t1 | test.swift:285:15:285:15 | t1 | +| test.swift:285:15:285:15 | t1 | test.swift:286:15:286:15 | t1 | +| test.swift:286:15:286:15 | t1 | test.swift:287:15:287:15 | t1 | +| test.swift:287:15:287:15 | t1 | test.swift:289:5:289:5 | t1 | +| test.swift:289:5:289:5 | t1 | test.swift:291:15:291:15 | t1 | +| test.swift:291:15:291:15 | t1 | test.swift:292:15:292:15 | t1 | +| test.swift:292:15:292:15 | t1 | test.swift:293:15:293:15 | t1 | +| test.swift:297:9:297:9 | WriteDef | test.swift:298:14:298:14 | t1 | +| test.swift:297:14:297:45 | (...) | test.swift:297:9:297:9 | WriteDef | +| test.swift:298:9:298:9 | WriteDef | test.swift:305:15:305:15 | t2 | +| test.swift:298:14:298:14 | t1 | test.swift:298:9:298:9 | WriteDef | +| test.swift:298:14:298:14 | t1 | test.swift:299:21:299:21 | t1 | +| test.swift:299:9:299:17 | WriteDef | test.swift:309:15:309:15 | a | +| test.swift:299:9:299:17 | WriteDef | test.swift:310:15:310:15 | b | +| test.swift:299:9:299:17 | WriteDef | test.swift:311:15:311:15 | c | +| test.swift:299:21:299:21 | t1 | test.swift:299:9:299:17 | WriteDef | +| test.swift:299:21:299:21 | t1 | test.swift:299:9:299:17 | WriteDef | +| test.swift:299:21:299:21 | t1 | test.swift:299:9:299:17 | WriteDef | +| test.swift:299:21:299:21 | t1 | test.swift:301:15:301:15 | t1 | +| test.swift:301:15:301:15 | t1 | test.swift:302:15:302:15 | t1 | +| test.swift:302:15:302:15 | t1 | test.swift:303:15:303:15 | t1 | +| test.swift:303:15:303:15 | t1 | test.swift:304:15:304:15 | t1 | +| test.swift:305:15:305:15 | t2 | test.swift:306:15:306:15 | t2 | +| test.swift:306:15:306:15 | t2 | test.swift:307:15:307:15 | t2 | +| test.swift:307:15:307:15 | t2 | test.swift:308:15:308:15 | t2 | diff --git a/swift/ql/test/library-tests/dataflow/dataflow/test.swift b/swift/ql/test/library-tests/dataflow/dataflow/test.swift index 95a95145140..e1f4f41c5c2 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/test.swift +++ b/swift/ql/test/library-tests/dataflow/dataflow/test.swift @@ -124,7 +124,7 @@ func forwarder() { return i }) sink(arg: z) // $ flow=122 - + var clean: Int = forward(arg: source(), lambda: { (i: Int) -> Int in return 0 @@ -269,3 +269,44 @@ func test_optionals() { sink(arg: y) // $ MISSING: flow=259 } } + +func sink(arg: (Int, Int)) {} +func sink(arg: (Int, Int, Int)) {} + +func testTuples() { + var t1 = (1, source()) + + sink(arg: t1) + sink(arg: t1.0) + sink(arg: t1.1) // $ MISSING: flow=277 + + t1.1 = 2 + + sink(arg: t1) + sink(arg: t1.0) + sink(arg: t1.1) + + t1.0 = source() + + sink(arg: t1) + sink(arg: t1.0) // $ MISSING: flow=289 + sink(arg: t1.1) +} + +func testTuples2() { + let t1 = (x: source(), y: source(), z: 0) + let t2 = t1 + let (a, b, c) = t1 + + sink(arg: t1) + sink(arg: t1.x) // $ MISSING: flow=297 + sink(arg: t1.y) // $ MISSING: flow=297 + sink(arg: t1.z) + sink(arg: t2) + sink(arg: t2.x) // $ MISSING: flow=297 + sink(arg: t2.y) // $ MISSING: flow=297 + sink(arg: t2.z) + sink(arg: a) // $ MISSING: flow=297 + sink(arg: b) // $ MISSING: flow=297 + sink(arg: c) +} From 472ece45e7a7d70d8eac84107413b76be5b4a0f8 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 3 Nov 2022 15:11:03 +0000 Subject: [PATCH 010/174] Swift: Basic content flow through tuples. --- .../dataflow/internal/DataFlowPrivate.qll | 16 ++++++- .../dataflow/internal/DataFlowPublic.qll | 12 +++++ .../dataflow/dataflow/DataFlow.expected | 44 +++++++++++++++++++ .../dataflow/dataflow/test.swift | 14 +++--- 4 files changed, 78 insertions(+), 8 deletions(-) diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll index 892eea699aa..554f2eb7454 100644 --- a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll +++ b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll @@ -177,7 +177,9 @@ private module Cached { newtype TContentSet = TSingletonContent(Content c) cached - newtype TContent = TFieldContent(FieldDecl f) + newtype TContent = + TFieldContent(FieldDecl f) or + TTupleContent(int index) { exists(any(TupleExpr tn).getElement(index)) } } /** @@ -505,6 +507,12 @@ predicate storeStep(Node node1, ContentSet c, Node node2) { c.isSingleton(any(Content::FieldContent ct | ct.getField() = ref.getMember())) ) or + exists(TupleExpr tuple, int pos | + node1.asExpr() = tuple.getElement(pos) and + node2.asExpr() = tuple and + c.isSingleton(any(Content::TupleContent ct | ct.getIndex() = pos)) + ) + or FlowSummaryImpl::Private::Steps::summaryStoreStep(node1, c, node2) } @@ -517,6 +525,12 @@ predicate readStep(Node node1, ContentSet c, Node node2) { node2.asExpr() = ref and c.isSingleton(any(Content::FieldContent ct | ct.getField() = ref.getMember())) ) + or + exists(TupleElementExpr tuple | + node1.asExpr() = tuple.getSubExpr() and + node2.asExpr() = tuple and + c.isSingleton(any(Content::TupleContent ct | ct.getIndex() = tuple.getIndex())) + ) } /** diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll index be569f18314..9257c8ccb05 100644 --- a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll +++ b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll @@ -159,6 +159,18 @@ module Content { override string toString() { result = f.toString() } } + + /** An element of a tuple at a specific index. */ + class TupleContent extends Content, TTupleContent { + private int index; + + TupleContent() { this = TTupleContent(index) } + + /** Gets the index for this tuple element. */ + int getIndex() { result = index } + + override string toString() { result = "Tuple element at index " + index.toString() } + } } /** diff --git a/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected b/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected index 3cf1784e010..8862798d4e4 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected +++ b/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected @@ -107,6 +107,23 @@ edges | test.swift:266:15:266:16 | ...? : | file://:0:0:0:0 | [summary param] this in signum() : | | test.swift:266:15:266:16 | ...? : | test.swift:266:15:266:25 | call to signum() : | | test.swift:266:15:266:25 | call to signum() : | test.swift:266:15:266:25 | OptionalEvaluationExpr | +| test.swift:277:14:277:26 | (...) [Tuple element at index 1] : | test.swift:281:15:281:15 | t1 [Tuple element at index 1] : | +| test.swift:277:14:277:26 | (...) [Tuple element at index 1] : | test.swift:287:15:287:15 | t1 [Tuple element at index 1] : | +| test.swift:277:14:277:26 | (...) [Tuple element at index 1] : | test.swift:293:15:293:15 | t1 [Tuple element at index 1] : | +| test.swift:277:18:277:25 | call to source() : | test.swift:277:14:277:26 | (...) [Tuple element at index 1] : | +| test.swift:281:15:281:15 | t1 [Tuple element at index 1] : | test.swift:281:15:281:18 | .1 | +| test.swift:287:15:287:15 | t1 [Tuple element at index 1] : | test.swift:287:15:287:18 | .1 | +| test.swift:293:15:293:15 | t1 [Tuple element at index 1] : | test.swift:293:15:293:18 | .1 | +| test.swift:297:14:297:45 | (...) [Tuple element at index 0] : | test.swift:302:15:302:15 | t1 [Tuple element at index 0] : | +| test.swift:297:14:297:45 | (...) [Tuple element at index 0] : | test.swift:306:15:306:15 | t2 [Tuple element at index 0] : | +| test.swift:297:14:297:45 | (...) [Tuple element at index 1] : | test.swift:303:15:303:15 | t1 [Tuple element at index 1] : | +| test.swift:297:14:297:45 | (...) [Tuple element at index 1] : | test.swift:307:15:307:15 | t2 [Tuple element at index 1] : | +| test.swift:297:18:297:25 | call to source() : | test.swift:297:14:297:45 | (...) [Tuple element at index 0] : | +| test.swift:297:31:297:38 | call to source() : | test.swift:297:14:297:45 | (...) [Tuple element at index 1] : | +| test.swift:302:15:302:15 | t1 [Tuple element at index 0] : | test.swift:302:15:302:18 | .0 | +| test.swift:303:15:303:15 | t1 [Tuple element at index 1] : | test.swift:303:15:303:18 | .1 | +| test.swift:306:15:306:15 | t2 [Tuple element at index 0] : | test.swift:306:15:306:18 | .0 | +| test.swift:307:15:307:15 | t2 [Tuple element at index 1] : | test.swift:307:15:307:18 | .1 | nodes | file://:0:0:0:0 | .a [x] : | semmle.label | .a [x] : | | file://:0:0:0:0 | .x : | semmle.label | .x : | @@ -227,6 +244,26 @@ nodes | test.swift:266:15:266:16 | ...? : | semmle.label | ...? : | | test.swift:266:15:266:25 | OptionalEvaluationExpr | semmle.label | OptionalEvaluationExpr | | test.swift:266:15:266:25 | call to signum() : | semmle.label | call to signum() : | +| test.swift:277:14:277:26 | (...) [Tuple element at index 1] : | semmle.label | (...) [Tuple element at index 1] : | +| test.swift:277:18:277:25 | call to source() : | semmle.label | call to source() : | +| test.swift:281:15:281:15 | t1 [Tuple element at index 1] : | semmle.label | t1 [Tuple element at index 1] : | +| test.swift:281:15:281:18 | .1 | semmle.label | .1 | +| test.swift:287:15:287:15 | t1 [Tuple element at index 1] : | semmle.label | t1 [Tuple element at index 1] : | +| test.swift:287:15:287:18 | .1 | semmle.label | .1 | +| test.swift:293:15:293:15 | t1 [Tuple element at index 1] : | semmle.label | t1 [Tuple element at index 1] : | +| test.swift:293:15:293:18 | .1 | semmle.label | .1 | +| test.swift:297:14:297:45 | (...) [Tuple element at index 0] : | semmle.label | (...) [Tuple element at index 0] : | +| test.swift:297:14:297:45 | (...) [Tuple element at index 1] : | semmle.label | (...) [Tuple element at index 1] : | +| test.swift:297:18:297:25 | call to source() : | semmle.label | call to source() : | +| test.swift:297:31:297:38 | call to source() : | semmle.label | call to source() : | +| test.swift:302:15:302:15 | t1 [Tuple element at index 0] : | semmle.label | t1 [Tuple element at index 0] : | +| test.swift:302:15:302:18 | .0 | semmle.label | .0 | +| test.swift:303:15:303:15 | t1 [Tuple element at index 1] : | semmle.label | t1 [Tuple element at index 1] : | +| test.swift:303:15:303:18 | .1 | semmle.label | .1 | +| test.swift:306:15:306:15 | t2 [Tuple element at index 0] : | semmle.label | t2 [Tuple element at index 0] : | +| test.swift:306:15:306:18 | .0 | semmle.label | .0 | +| test.swift:307:15:307:15 | t2 [Tuple element at index 1] : | semmle.label | t2 [Tuple element at index 1] : | +| test.swift:307:15:307:18 | .1 | semmle.label | .1 | subpaths | test.swift:75:21:75:22 | &... : | test.swift:65:16:65:28 | arg1 : | test.swift:65:1:70:1 | arg2[return] : | test.swift:75:31:75:32 | [post] &... : | | test.swift:114:19:114:19 | arg : | test.swift:109:9:109:14 | arg : | test.swift:110:12:110:12 | arg : | test.swift:114:12:114:22 | call to ... : | @@ -287,3 +324,10 @@ subpaths | test.swift:264:15:264:16 | ...! | test.swift:259:12:259:19 | call to source() : | test.swift:264:15:264:16 | ...! | result | | test.swift:265:15:265:31 | call to signum() | test.swift:265:15:265:22 | call to source() : | test.swift:265:15:265:31 | call to signum() | result | | test.swift:266:15:266:25 | OptionalEvaluationExpr | test.swift:259:12:259:19 | call to source() : | test.swift:266:15:266:25 | OptionalEvaluationExpr | result | +| test.swift:281:15:281:18 | .1 | test.swift:277:18:277:25 | call to source() : | test.swift:281:15:281:18 | .1 | result | +| test.swift:287:15:287:18 | .1 | test.swift:277:18:277:25 | call to source() : | test.swift:287:15:287:18 | .1 | result | +| test.swift:293:15:293:18 | .1 | test.swift:277:18:277:25 | call to source() : | test.swift:293:15:293:18 | .1 | result | +| test.swift:302:15:302:18 | .0 | test.swift:297:18:297:25 | call to source() : | test.swift:302:15:302:18 | .0 | result | +| test.swift:303:15:303:18 | .1 | test.swift:297:31:297:38 | call to source() : | test.swift:303:15:303:18 | .1 | result | +| test.swift:306:15:306:18 | .0 | test.swift:297:18:297:25 | call to source() : | test.swift:306:15:306:18 | .0 | result | +| test.swift:307:15:307:18 | .1 | test.swift:297:31:297:38 | call to source() : | test.swift:307:15:307:18 | .1 | result | diff --git a/swift/ql/test/library-tests/dataflow/dataflow/test.swift b/swift/ql/test/library-tests/dataflow/dataflow/test.swift index e1f4f41c5c2..15635efab78 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/test.swift +++ b/swift/ql/test/library-tests/dataflow/dataflow/test.swift @@ -278,19 +278,19 @@ func testTuples() { sink(arg: t1) sink(arg: t1.0) - sink(arg: t1.1) // $ MISSING: flow=277 + sink(arg: t1.1) // $ flow=277 t1.1 = 2 sink(arg: t1) sink(arg: t1.0) - sink(arg: t1.1) + sink(arg: t1.1) // $ SPURIOUS: flow=277 t1.0 = source() sink(arg: t1) sink(arg: t1.0) // $ MISSING: flow=289 - sink(arg: t1.1) + sink(arg: t1.1) // $ SPURIOUS: flow=277 } func testTuples2() { @@ -299,12 +299,12 @@ func testTuples2() { let (a, b, c) = t1 sink(arg: t1) - sink(arg: t1.x) // $ MISSING: flow=297 - sink(arg: t1.y) // $ MISSING: flow=297 + sink(arg: t1.x) // $ flow=297 + sink(arg: t1.y) // $ flow=297 sink(arg: t1.z) sink(arg: t2) - sink(arg: t2.x) // $ MISSING: flow=297 - sink(arg: t2.y) // $ MISSING: flow=297 + sink(arg: t2.x) // $ flow=297 + sink(arg: t2.y) // $ flow=297 sink(arg: t2.z) sink(arg: a) // $ MISSING: flow=297 sink(arg: b) // $ MISSING: flow=297 From 6dc51edb4c7903136a257202540f0d386d3afbe9 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 3 Nov 2022 15:23:30 +0000 Subject: [PATCH 011/174] Swift: Assigning to tuple values. --- .../codeql/swift/dataflow/internal/DataFlowPrivate.qll | 7 +++++++ .../library-tests/dataflow/dataflow/DataFlow.expected | 8 ++++++++ swift/ql/test/library-tests/dataflow/dataflow/test.swift | 2 +- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll index 554f2eb7454..35782564306 100644 --- a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll +++ b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll @@ -513,6 +513,13 @@ predicate storeStep(Node node1, ContentSet c, Node node2) { c.isSingleton(any(Content::TupleContent ct | ct.getIndex() = pos)) ) or + exists(TupleElementExpr tuple, AssignExpr assign | + tuple = assign.getDest() and + node1.asExpr() = assign.getSource() and + node2/*.(PostUpdateNode).getPreUpdateNode()*/.asExpr() = tuple.getSubExpr() and + c.isSingleton(any(Content::TupleContent ct | ct.getIndex() = tuple.getIndex())) + ) + or FlowSummaryImpl::Private::Steps::summaryStoreStep(node1, c, node2) } diff --git a/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected b/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected index 8862798d4e4..2b5638078e1 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected +++ b/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected @@ -113,6 +113,9 @@ edges | test.swift:277:18:277:25 | call to source() : | test.swift:277:14:277:26 | (...) [Tuple element at index 1] : | | test.swift:281:15:281:15 | t1 [Tuple element at index 1] : | test.swift:281:15:281:18 | .1 | | test.swift:287:15:287:15 | t1 [Tuple element at index 1] : | test.swift:287:15:287:18 | .1 | +| test.swift:289:5:289:5 | t1 [Tuple element at index 0] : | test.swift:292:15:292:15 | t1 [Tuple element at index 0] : | +| test.swift:289:12:289:19 | call to source() : | test.swift:289:5:289:5 | t1 [Tuple element at index 0] : | +| test.swift:292:15:292:15 | t1 [Tuple element at index 0] : | test.swift:292:15:292:18 | .0 | | test.swift:293:15:293:15 | t1 [Tuple element at index 1] : | test.swift:293:15:293:18 | .1 | | test.swift:297:14:297:45 | (...) [Tuple element at index 0] : | test.swift:302:15:302:15 | t1 [Tuple element at index 0] : | | test.swift:297:14:297:45 | (...) [Tuple element at index 0] : | test.swift:306:15:306:15 | t2 [Tuple element at index 0] : | @@ -250,6 +253,10 @@ nodes | test.swift:281:15:281:18 | .1 | semmle.label | .1 | | test.swift:287:15:287:15 | t1 [Tuple element at index 1] : | semmle.label | t1 [Tuple element at index 1] : | | test.swift:287:15:287:18 | .1 | semmle.label | .1 | +| test.swift:289:5:289:5 | t1 [Tuple element at index 0] : | semmle.label | t1 [Tuple element at index 0] : | +| test.swift:289:12:289:19 | call to source() : | semmle.label | call to source() : | +| test.swift:292:15:292:15 | t1 [Tuple element at index 0] : | semmle.label | t1 [Tuple element at index 0] : | +| test.swift:292:15:292:18 | .0 | semmle.label | .0 | | test.swift:293:15:293:15 | t1 [Tuple element at index 1] : | semmle.label | t1 [Tuple element at index 1] : | | test.swift:293:15:293:18 | .1 | semmle.label | .1 | | test.swift:297:14:297:45 | (...) [Tuple element at index 0] : | semmle.label | (...) [Tuple element at index 0] : | @@ -326,6 +333,7 @@ subpaths | test.swift:266:15:266:25 | OptionalEvaluationExpr | test.swift:259:12:259:19 | call to source() : | test.swift:266:15:266:25 | OptionalEvaluationExpr | result | | test.swift:281:15:281:18 | .1 | test.swift:277:18:277:25 | call to source() : | test.swift:281:15:281:18 | .1 | result | | test.swift:287:15:287:18 | .1 | test.swift:277:18:277:25 | call to source() : | test.swift:287:15:287:18 | .1 | result | +| test.swift:292:15:292:18 | .0 | test.swift:289:12:289:19 | call to source() : | test.swift:292:15:292:18 | .0 | result | | test.swift:293:15:293:18 | .1 | test.swift:277:18:277:25 | call to source() : | test.swift:293:15:293:18 | .1 | result | | test.swift:302:15:302:18 | .0 | test.swift:297:18:297:25 | call to source() : | test.swift:302:15:302:18 | .0 | result | | test.swift:303:15:303:18 | .1 | test.swift:297:31:297:38 | call to source() : | test.swift:303:15:303:18 | .1 | result | diff --git a/swift/ql/test/library-tests/dataflow/dataflow/test.swift b/swift/ql/test/library-tests/dataflow/dataflow/test.swift index 15635efab78..522a58aac02 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/test.swift +++ b/swift/ql/test/library-tests/dataflow/dataflow/test.swift @@ -289,7 +289,7 @@ func testTuples() { t1.0 = source() sink(arg: t1) - sink(arg: t1.0) // $ MISSING: flow=289 + sink(arg: t1.0) // $ flow=289 sink(arg: t1.1) // $ SPURIOUS: flow=277 } From 24f0eeb6dfe5acaa6465ee24e906836fefb27aa0 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 3 Nov 2022 15:36:18 +0000 Subject: [PATCH 012/174] Swift: Better assigning to tuple values. --- .../swift/dataflow/internal/DataFlowPrivate.qll | 4 ++-- .../dataflow/dataflow/DataFlow.expected | 16 +++------------- .../dataflow/dataflow/DataFlowInline.expected | 2 ++ .../dataflow/dataflow/LocalFlow.expected | 11 +++++++++++ .../library-tests/dataflow/dataflow/test.swift | 4 ++-- 5 files changed, 20 insertions(+), 17 deletions(-) diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll index 35782564306..996fcd773b3 100644 --- a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll +++ b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll @@ -86,7 +86,7 @@ private module Cached { hasExprNode(n, [ any(Argument arg | modifiable(arg)).getExpr(), any(MemberRefExpr ref).getBase(), - any(ApplyExpr apply).getQualifier() + any(ApplyExpr apply).getQualifier(), any(TupleElementExpr te).getSubExpr() ]) } @@ -516,7 +516,7 @@ predicate storeStep(Node node1, ContentSet c, Node node2) { exists(TupleElementExpr tuple, AssignExpr assign | tuple = assign.getDest() and node1.asExpr() = assign.getSource() and - node2/*.(PostUpdateNode).getPreUpdateNode()*/.asExpr() = tuple.getSubExpr() and + node2.(PostUpdateNode).getPreUpdateNode().asExpr() = tuple.getSubExpr() and c.isSingleton(any(Content::TupleContent ct | ct.getIndex() = tuple.getIndex())) ) or diff --git a/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected b/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected index 2b5638078e1..c4e0a7a01e6 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected +++ b/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected @@ -108,15 +108,11 @@ edges | test.swift:266:15:266:16 | ...? : | test.swift:266:15:266:25 | call to signum() : | | test.swift:266:15:266:25 | call to signum() : | test.swift:266:15:266:25 | OptionalEvaluationExpr | | test.swift:277:14:277:26 | (...) [Tuple element at index 1] : | test.swift:281:15:281:15 | t1 [Tuple element at index 1] : | -| test.swift:277:14:277:26 | (...) [Tuple element at index 1] : | test.swift:287:15:287:15 | t1 [Tuple element at index 1] : | -| test.swift:277:14:277:26 | (...) [Tuple element at index 1] : | test.swift:293:15:293:15 | t1 [Tuple element at index 1] : | | test.swift:277:18:277:25 | call to source() : | test.swift:277:14:277:26 | (...) [Tuple element at index 1] : | | test.swift:281:15:281:15 | t1 [Tuple element at index 1] : | test.swift:281:15:281:18 | .1 | -| test.swift:287:15:287:15 | t1 [Tuple element at index 1] : | test.swift:287:15:287:18 | .1 | -| test.swift:289:5:289:5 | t1 [Tuple element at index 0] : | test.swift:292:15:292:15 | t1 [Tuple element at index 0] : | -| test.swift:289:12:289:19 | call to source() : | test.swift:289:5:289:5 | t1 [Tuple element at index 0] : | +| test.swift:289:5:289:5 | [post] t1 [Tuple element at index 0] : | test.swift:292:15:292:15 | t1 [Tuple element at index 0] : | +| test.swift:289:12:289:19 | call to source() : | test.swift:289:5:289:5 | [post] t1 [Tuple element at index 0] : | | test.swift:292:15:292:15 | t1 [Tuple element at index 0] : | test.swift:292:15:292:18 | .0 | -| test.swift:293:15:293:15 | t1 [Tuple element at index 1] : | test.swift:293:15:293:18 | .1 | | test.swift:297:14:297:45 | (...) [Tuple element at index 0] : | test.swift:302:15:302:15 | t1 [Tuple element at index 0] : | | test.swift:297:14:297:45 | (...) [Tuple element at index 0] : | test.swift:306:15:306:15 | t2 [Tuple element at index 0] : | | test.swift:297:14:297:45 | (...) [Tuple element at index 1] : | test.swift:303:15:303:15 | t1 [Tuple element at index 1] : | @@ -251,14 +247,10 @@ nodes | test.swift:277:18:277:25 | call to source() : | semmle.label | call to source() : | | test.swift:281:15:281:15 | t1 [Tuple element at index 1] : | semmle.label | t1 [Tuple element at index 1] : | | test.swift:281:15:281:18 | .1 | semmle.label | .1 | -| test.swift:287:15:287:15 | t1 [Tuple element at index 1] : | semmle.label | t1 [Tuple element at index 1] : | -| test.swift:287:15:287:18 | .1 | semmle.label | .1 | -| test.swift:289:5:289:5 | t1 [Tuple element at index 0] : | semmle.label | t1 [Tuple element at index 0] : | +| test.swift:289:5:289:5 | [post] t1 [Tuple element at index 0] : | semmle.label | [post] t1 [Tuple element at index 0] : | | test.swift:289:12:289:19 | call to source() : | semmle.label | call to source() : | | test.swift:292:15:292:15 | t1 [Tuple element at index 0] : | semmle.label | t1 [Tuple element at index 0] : | | test.swift:292:15:292:18 | .0 | semmle.label | .0 | -| test.swift:293:15:293:15 | t1 [Tuple element at index 1] : | semmle.label | t1 [Tuple element at index 1] : | -| test.swift:293:15:293:18 | .1 | semmle.label | .1 | | test.swift:297:14:297:45 | (...) [Tuple element at index 0] : | semmle.label | (...) [Tuple element at index 0] : | | test.swift:297:14:297:45 | (...) [Tuple element at index 1] : | semmle.label | (...) [Tuple element at index 1] : | | test.swift:297:18:297:25 | call to source() : | semmle.label | call to source() : | @@ -332,9 +324,7 @@ subpaths | test.swift:265:15:265:31 | call to signum() | test.swift:265:15:265:22 | call to source() : | test.swift:265:15:265:31 | call to signum() | result | | test.swift:266:15:266:25 | OptionalEvaluationExpr | test.swift:259:12:259:19 | call to source() : | test.swift:266:15:266:25 | OptionalEvaluationExpr | result | | test.swift:281:15:281:18 | .1 | test.swift:277:18:277:25 | call to source() : | test.swift:281:15:281:18 | .1 | result | -| test.swift:287:15:287:18 | .1 | test.swift:277:18:277:25 | call to source() : | test.swift:287:15:287:18 | .1 | result | | test.swift:292:15:292:18 | .0 | test.swift:289:12:289:19 | call to source() : | test.swift:292:15:292:18 | .0 | result | -| test.swift:293:15:293:18 | .1 | test.swift:277:18:277:25 | call to source() : | test.swift:293:15:293:18 | .1 | result | | test.swift:302:15:302:18 | .0 | test.swift:297:18:297:25 | call to source() : | test.swift:302:15:302:18 | .0 | result | | test.swift:303:15:303:18 | .1 | test.swift:297:31:297:38 | call to source() : | test.swift:303:15:303:18 | .1 | result | | test.swift:306:15:306:18 | .0 | test.swift:297:18:297:25 | call to source() : | test.swift:306:15:306:18 | .0 | result | diff --git a/swift/ql/test/library-tests/dataflow/dataflow/DataFlowInline.expected b/swift/ql/test/library-tests/dataflow/dataflow/DataFlowInline.expected index e69de29bb2d..0ecaa82d583 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/DataFlowInline.expected +++ b/swift/ql/test/library-tests/dataflow/dataflow/DataFlowInline.expected @@ -0,0 +1,2 @@ +| test.swift:287:21:288:1 | // $ flow=277\n | Missing result:flow=277 | +| test.swift:293:21:294:1 | // $ flow=277\n | Missing result:flow=277 | diff --git a/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected b/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected index 2e945f96294..987d7073b87 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected +++ b/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected @@ -195,14 +195,21 @@ | test.swift:277:9:277:9 | WriteDef | test.swift:279:15:279:15 | t1 | | test.swift:277:14:277:26 | (...) | test.swift:277:9:277:9 | WriteDef | | test.swift:279:15:279:15 | t1 | test.swift:280:15:280:15 | t1 | +| test.swift:280:15:280:15 | [post] t1 | test.swift:281:15:281:15 | t1 | | test.swift:280:15:280:15 | t1 | test.swift:281:15:281:15 | t1 | +| test.swift:281:15:281:15 | [post] t1 | test.swift:283:5:283:5 | t1 | | test.swift:281:15:281:15 | t1 | test.swift:283:5:283:5 | t1 | +| test.swift:283:5:283:5 | [post] t1 | test.swift:285:15:285:15 | t1 | | test.swift:283:5:283:5 | t1 | test.swift:285:15:285:15 | t1 | | test.swift:285:15:285:15 | t1 | test.swift:286:15:286:15 | t1 | +| test.swift:286:15:286:15 | [post] t1 | test.swift:287:15:287:15 | t1 | | test.swift:286:15:286:15 | t1 | test.swift:287:15:287:15 | t1 | +| test.swift:287:15:287:15 | [post] t1 | test.swift:289:5:289:5 | t1 | | test.swift:287:15:287:15 | t1 | test.swift:289:5:289:5 | t1 | +| test.swift:289:5:289:5 | [post] t1 | test.swift:291:15:291:15 | t1 | | test.swift:289:5:289:5 | t1 | test.swift:291:15:291:15 | t1 | | test.swift:291:15:291:15 | t1 | test.swift:292:15:292:15 | t1 | +| test.swift:292:15:292:15 | [post] t1 | test.swift:293:15:293:15 | t1 | | test.swift:292:15:292:15 | t1 | test.swift:293:15:293:15 | t1 | | test.swift:297:9:297:9 | WriteDef | test.swift:298:14:298:14 | t1 | | test.swift:297:14:297:45 | (...) | test.swift:297:9:297:9 | WriteDef | @@ -217,8 +224,12 @@ | test.swift:299:21:299:21 | t1 | test.swift:299:9:299:17 | WriteDef | | test.swift:299:21:299:21 | t1 | test.swift:301:15:301:15 | t1 | | test.swift:301:15:301:15 | t1 | test.swift:302:15:302:15 | t1 | +| test.swift:302:15:302:15 | [post] t1 | test.swift:303:15:303:15 | t1 | | test.swift:302:15:302:15 | t1 | test.swift:303:15:303:15 | t1 | +| test.swift:303:15:303:15 | [post] t1 | test.swift:304:15:304:15 | t1 | | test.swift:303:15:303:15 | t1 | test.swift:304:15:304:15 | t1 | | test.swift:305:15:305:15 | t2 | test.swift:306:15:306:15 | t2 | +| test.swift:306:15:306:15 | [post] t2 | test.swift:307:15:307:15 | t2 | | test.swift:306:15:306:15 | t2 | test.swift:307:15:307:15 | t2 | +| test.swift:307:15:307:15 | [post] t2 | test.swift:308:15:308:15 | t2 | | test.swift:307:15:307:15 | t2 | test.swift:308:15:308:15 | t2 | diff --git a/swift/ql/test/library-tests/dataflow/dataflow/test.swift b/swift/ql/test/library-tests/dataflow/dataflow/test.swift index 522a58aac02..77d503dec59 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/test.swift +++ b/swift/ql/test/library-tests/dataflow/dataflow/test.swift @@ -284,13 +284,13 @@ func testTuples() { sink(arg: t1) sink(arg: t1.0) - sink(arg: t1.1) // $ SPURIOUS: flow=277 + sink(arg: t1.1) // $ flow=277 t1.0 = source() sink(arg: t1) sink(arg: t1.0) // $ flow=289 - sink(arg: t1.1) // $ SPURIOUS: flow=277 + sink(arg: t1.1) // $ flow=277 } func testTuples2() { From 86cbf1b82cd178f8604473203fb83de391c8d655 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 3 Nov 2022 18:45:43 +0000 Subject: [PATCH 013/174] Swift: Add comments. --- .../lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll index 996fcd773b3..4b7193df8e5 100644 --- a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll +++ b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll @@ -500,6 +500,7 @@ predicate jumpStep(Node pred, Node succ) { } predicate storeStep(Node node1, ContentSet c, Node node2) { + // assignment to a member variable `obj.member = value` exists(MemberRefExpr ref, AssignExpr assign | ref = assign.getDest() and node1.asExpr() = assign.getSource() and @@ -507,12 +508,14 @@ predicate storeStep(Node node1, ContentSet c, Node node2) { c.isSingleton(any(Content::FieldContent ct | ct.getField() = ref.getMember())) ) or + // creation of a tuple `(v1, v2)` exists(TupleExpr tuple, int pos | node1.asExpr() = tuple.getElement(pos) and node2.asExpr() = tuple and c.isSingleton(any(Content::TupleContent ct | ct.getIndex() = pos)) ) or + // assignment to a tuple member `tuple.index = value` exists(TupleElementExpr tuple, AssignExpr assign | tuple = assign.getDest() and node1.asExpr() = assign.getSource() and @@ -526,6 +529,7 @@ predicate storeStep(Node node1, ContentSet c, Node node2) { predicate isLValue(Expr e) { any(AssignExpr assign).getDest() = e } predicate readStep(Node node1, ContentSet c, Node node2) { + // read of a member variable `obj.member` exists(MemberRefExpr ref | not isLValue(ref) and node1.asExpr() = ref.getBase() and @@ -533,6 +537,7 @@ predicate readStep(Node node1, ContentSet c, Node node2) { c.isSingleton(any(Content::FieldContent ct | ct.getField() = ref.getMember())) ) or + // read of a tuple member `tuple.index` exists(TupleElementExpr tuple | node1.asExpr() = tuple.getSubExpr() and node2.asExpr() = tuple and From 20147e87b2a1b267bf684443d326356239efee62 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 4 Nov 2022 09:38:12 +0000 Subject: [PATCH 014/174] Swift: Correct var names. --- .../codeql/swift/dataflow/internal/DataFlowPrivate.qll | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll index 4b7193df8e5..4e828bfaf27 100644 --- a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll +++ b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll @@ -179,7 +179,7 @@ private module Cached { cached newtype TContent = TFieldContent(FieldDecl f) or - TTupleContent(int index) { exists(any(TupleExpr tn).getElement(index)) } + TTupleContent(int index) { exists(any(TupleExpr te).getElement(index)) } } /** @@ -512,7 +512,7 @@ predicate storeStep(Node node1, ContentSet c, Node node2) { exists(TupleExpr tuple, int pos | node1.asExpr() = tuple.getElement(pos) and node2.asExpr() = tuple and - c.isSingleton(any(Content::TupleContent ct | ct.getIndex() = pos)) + c.isSingleton(any(Content::TupleContent tc | tc.getIndex() = pos)) ) or // assignment to a tuple member `tuple.index = value` @@ -520,7 +520,7 @@ predicate storeStep(Node node1, ContentSet c, Node node2) { tuple = assign.getDest() and node1.asExpr() = assign.getSource() and node2.(PostUpdateNode).getPreUpdateNode().asExpr() = tuple.getSubExpr() and - c.isSingleton(any(Content::TupleContent ct | ct.getIndex() = tuple.getIndex())) + c.isSingleton(any(Content::TupleContent tc | tc.getIndex() = tuple.getIndex())) ) or FlowSummaryImpl::Private::Steps::summaryStoreStep(node1, c, node2) @@ -541,7 +541,7 @@ predicate readStep(Node node1, ContentSet c, Node node2) { exists(TupleElementExpr tuple | node1.asExpr() = tuple.getSubExpr() and node2.asExpr() = tuple and - c.isSingleton(any(Content::TupleContent ct | ct.getIndex() = tuple.getIndex())) + c.isSingleton(any(Content::TupleContent tc | tc.getIndex() = tuple.getIndex())) ) } From 3c07ff592ac37ba905187a96f0bff89592cda9f6 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 4 Nov 2022 09:44:48 +0000 Subject: [PATCH 015/174] Swift: Fix result expectations. --- .../library-tests/dataflow/dataflow/DataFlowInline.expected | 2 -- swift/ql/test/library-tests/dataflow/dataflow/test.swift | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/swift/ql/test/library-tests/dataflow/dataflow/DataFlowInline.expected b/swift/ql/test/library-tests/dataflow/dataflow/DataFlowInline.expected index 0ecaa82d583..e69de29bb2d 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/DataFlowInline.expected +++ b/swift/ql/test/library-tests/dataflow/dataflow/DataFlowInline.expected @@ -1,2 +0,0 @@ -| test.swift:287:21:288:1 | // $ flow=277\n | Missing result:flow=277 | -| test.swift:293:21:294:1 | // $ flow=277\n | Missing result:flow=277 | diff --git a/swift/ql/test/library-tests/dataflow/dataflow/test.swift b/swift/ql/test/library-tests/dataflow/dataflow/test.swift index 77d503dec59..23c8d138a2d 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/test.swift +++ b/swift/ql/test/library-tests/dataflow/dataflow/test.swift @@ -284,13 +284,13 @@ func testTuples() { sink(arg: t1) sink(arg: t1.0) - sink(arg: t1.1) // $ flow=277 + sink(arg: t1.1) t1.0 = source() sink(arg: t1) sink(arg: t1.0) // $ flow=289 - sink(arg: t1.1) // $ flow=277 + sink(arg: t1.1) } func testTuples2() { From f0b505876068c2c2e4f15ca1f634e9d817cd4689 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 2 Nov 2022 10:58:30 +0100 Subject: [PATCH 016/174] C#: Remove filename from telemetry output. --- csharp/ql/src/Telemetry/ExternalApi.qll | 38 +++++++------------ .../ql/src/Telemetry/ExternalLibraryUsage.ql | 20 +++++----- 2 files changed, 24 insertions(+), 34 deletions(-) diff --git a/csharp/ql/src/Telemetry/ExternalApi.qll b/csharp/ql/src/Telemetry/ExternalApi.qll index d4cc0ae43cc..cf201b08737 100644 --- a/csharp/ql/src/Telemetry/ExternalApi.qll +++ b/csharp/ql/src/Telemetry/ExternalApi.qll @@ -42,22 +42,12 @@ class ExternalApi extends DotNet::Callable { /** * Gets the namespace of this API. */ - private string getNamespace() { this.getDeclaringType().hasQualifiedName(result, _) } + string getNamespace() { this.getDeclaringType().hasQualifiedName(result, _) } /** - * Gets the assembly file name containing this API. + * Gets the namespace and signature of this API. */ - private string getAssembly() { result = this.getFile().getBaseName() } - - /** - * Gets the assembly file name and namespace of this API. - */ - string getInfoPrefix() { result = this.getAssembly() + "#" + this.getNamespace() } - - /** - * Gets the assembly file name, namespace and signature of this API. - */ - string getInfo() { result = this.getInfoPrefix() + "#" + this.getSignature() } + string getApiName() { result = this.getNamespace() + "#" + this.getSignature() } /** Gets a call to this API callable. */ DispatchCall getACall() { @@ -125,30 +115,30 @@ signature predicate relevantApi(ExternalApi api); * for restricting the number or returned results based on a certain limit. */ module Results { - private int getUsages(string apiInfo) { + private int getUsages(string apiName) { result = strictcount(DispatchCall c, ExternalApi api | c = api.getACall() and - apiInfo = api.getInfo() and + apiName = api.getApiName() and getRelevantUsages(api) ) } - private int getOrder(string apiInfo) { - apiInfo = - rank[result](string info, int usages | - usages = getUsages(info) + private int getOrder(string apiName) { + apiName = + rank[result](string name, int usages | + usages = getUsages(name) | - info order by usages desc, info + name order by usages desc, name ) } /** - * Holds if there exists an API with `apiInfo` that is being used `usages` times + * Holds if there exists an API with `apiName` that is being used `usages` times * and if it is in the top results (guarded by resultLimit). */ - predicate restrict(string apiInfo, int usages) { - usages = getUsages(apiInfo) and - getOrder(apiInfo) <= resultLimit() + predicate restrict(string apiName, int usages) { + usages = getUsages(apiName) and + getOrder(apiName) <= resultLimit() } } diff --git a/csharp/ql/src/Telemetry/ExternalLibraryUsage.ql b/csharp/ql/src/Telemetry/ExternalLibraryUsage.ql index dfc1a8e062c..a9f70b84394 100644 --- a/csharp/ql/src/Telemetry/ExternalLibraryUsage.ql +++ b/csharp/ql/src/Telemetry/ExternalLibraryUsage.ql @@ -1,6 +1,6 @@ /** * @name External libraries - * @description A list of external libraries used in the code + * @description A list of external libraries used in the code given by their namespace. * @kind metric * @tags summary telemetry * @id csharp/telemetry/external-libs @@ -10,23 +10,23 @@ private import csharp private import semmle.code.csharp.dispatch.Dispatch private import ExternalApi -private predicate getRelevantUsages(string info, int usages) { +private predicate getRelevantUsages(string namespace, int usages) { usages = strictcount(DispatchCall c, ExternalApi api | c = api.getACall() and - api.getInfoPrefix() = info and + api.getNamespace() = namespace and not api.isUninteresting() ) } -private int getOrder(string info) { - info = +private int getOrder(string namespace) { + namespace = rank[result](string i, int usages | getRelevantUsages(i, usages) | i order by usages desc, i) } -from ExternalApi api, string info, int usages +from ExternalApi api, string namespace, int usages where - info = api.getInfoPrefix() and - getRelevantUsages(info, usages) and - getOrder(info) <= resultLimit() -select info, usages order by usages desc + namespace = api.getNamespace() and + getRelevantUsages(namespace, usages) and + getOrder(namespace) <= resultLimit() +select namespace, usages order by usages desc From fec4d1992d33a01956e569ebf0872e7eaf55ee67 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 2 Nov 2022 11:00:53 +0100 Subject: [PATCH 017/174] C#: Update telemetry expected output. --- .../Telemetry/LibraryUsage/ExternalLibraryUsage.expected | 4 ++-- .../Telemetry/LibraryUsage/SupportedExternalTaint.expected | 2 +- .../SupportedExternalSinks/SupportedExternalSinks.expected | 4 ++-- .../SupportedExternalSources.expected | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/csharp/ql/test/query-tests/Telemetry/LibraryUsage/ExternalLibraryUsage.expected b/csharp/ql/test/query-tests/Telemetry/LibraryUsage/ExternalLibraryUsage.expected index e64b827f137..7b5c4c57661 100644 --- a/csharp/ql/test/query-tests/Telemetry/LibraryUsage/ExternalLibraryUsage.expected +++ b/csharp/ql/test/query-tests/Telemetry/LibraryUsage/ExternalLibraryUsage.expected @@ -1,2 +1,2 @@ -| System.Private.CoreLib.dll#System | 5 | -| System.Private.CoreLib.dll#System.Collections.Generic | 2 | +| System | 5 | +| System.Collections.Generic | 2 | diff --git a/csharp/ql/test/query-tests/Telemetry/LibraryUsage/SupportedExternalTaint.expected b/csharp/ql/test/query-tests/Telemetry/LibraryUsage/SupportedExternalTaint.expected index e9be925c286..b386e4acb38 100644 --- a/csharp/ql/test/query-tests/Telemetry/LibraryUsage/SupportedExternalTaint.expected +++ b/csharp/ql/test/query-tests/Telemetry/LibraryUsage/SupportedExternalTaint.expected @@ -1 +1 @@ -| System.Private.CoreLib.dll#System.Collections.Generic#List<>.Add(T) | 2 | +| System.Collections.Generic#List<>.Add(T) | 2 | diff --git a/csharp/ql/test/query-tests/Telemetry/SupportedExternalSinks/SupportedExternalSinks.expected b/csharp/ql/test/query-tests/Telemetry/SupportedExternalSinks/SupportedExternalSinks.expected index b85e33ddae7..b3acebc02e0 100644 --- a/csharp/ql/test/query-tests/Telemetry/SupportedExternalSinks/SupportedExternalSinks.expected +++ b/csharp/ql/test/query-tests/Telemetry/SupportedExternalSinks/SupportedExternalSinks.expected @@ -1,2 +1,2 @@ -| System.Web.cs#System.Web#HttpResponse.Write(System.Object) | 2 | -| System.Web.cs#System.Web#HttpResponse.WriteFile(System.String) | 1 | +| System.Web#HttpResponse.Write(System.Object) | 2 | +| System.Web#HttpResponse.WriteFile(System.String) | 1 | diff --git a/csharp/ql/test/query-tests/Telemetry/SupportedExternalSources/SupportedExternalSources.expected b/csharp/ql/test/query-tests/Telemetry/SupportedExternalSources/SupportedExternalSources.expected index 0f125b04b94..93c23f159a6 100644 --- a/csharp/ql/test/query-tests/Telemetry/SupportedExternalSources/SupportedExternalSources.expected +++ b/csharp/ql/test/query-tests/Telemetry/SupportedExternalSources/SupportedExternalSources.expected @@ -1,2 +1,2 @@ -| System.Console.dll#System#Console.ReadLine() | 2 | -| System.Console.dll#System#Console.Read() | 1 | +| System#Console.ReadLine() | 2 | +| System#Console.Read() | 1 | From be1129e782bef1a47bee305bf2fa3982f1471fe7 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 3 Nov 2022 09:36:23 +0100 Subject: [PATCH 018/174] C#: Only consider effectively public methods. --- csharp/ql/src/Telemetry/ExternalApi.qll | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/csharp/ql/src/Telemetry/ExternalApi.qll b/csharp/ql/src/Telemetry/ExternalApi.qll index cf201b08737..98678946761 100644 --- a/csharp/ql/src/Telemetry/ExternalApi.qll +++ b/csharp/ql/src/Telemetry/ExternalApi.qll @@ -28,7 +28,11 @@ class TestLibrary extends RefType { * An external API from either the C# Standard Library or a 3rd party library. */ class ExternalApi extends DotNet::Callable { - ExternalApi() { this.isUnboundDeclaration() and this.fromLibrary() } + ExternalApi() { + this.isUnboundDeclaration() and + this.fromLibrary() and + this.(Modifiable).isEffectivelyPublic() + } /** * Gets the unbound type, name and parameter types of this API. From e0d7e277fb318719728a072bc984333f0d6f7653 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 3 Nov 2022 13:17:07 +0100 Subject: [PATCH 019/174] C#: Align counting with Java and only count calls and not all possible dispatch calls. --- csharp/ql/src/Telemetry/ExternalApi.qll | 26 +++++++++---------- .../ql/src/Telemetry/ExternalLibraryUsage.ql | 4 +-- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/csharp/ql/src/Telemetry/ExternalApi.qll b/csharp/ql/src/Telemetry/ExternalApi.qll index 98678946761..d3611fc0a2a 100644 --- a/csharp/ql/src/Telemetry/ExternalApi.qll +++ b/csharp/ql/src/Telemetry/ExternalApi.qll @@ -53,24 +53,24 @@ class ExternalApi extends DotNet::Callable { */ string getApiName() { result = this.getNamespace() + "#" + this.getSignature() } - /** Gets a call to this API callable. */ - DispatchCall getACall() { - this = result.getADynamicTarget().getUnboundDeclaration() - or - this = result.getAStaticTarget().getUnboundDeclaration() - } - /** Gets a node that is an input to a call to this API. */ private ArgumentNode getAnInput() { - result.getCall().(DataFlowDispatch::NonDelegateDataFlowCall).getDispatchCall() = this.getACall() + result + .getCall() + .(DataFlowDispatch::NonDelegateDataFlowCall) + .getATarget(_) + .getUnboundDeclaration() = this } /** Gets a node that is an output from a call to this API. */ private DataFlow::Node getAnOutput() { - exists(DataFlowDispatch::NonDelegateDataFlowCall call, DataFlowImplCommon::ReturnKindExt ret | - result = ret.getAnOutNode(call) + exists( + Call c, DataFlowDispatch::NonDelegateDataFlowCall dc, DataFlowImplCommon::ReturnKindExt ret | - this.getACall() = call.getDispatchCall() + dc.getDispatchCall().getCall() = c and + c.getTarget().getUnboundDeclaration() = this + | + result = ret.getAnOutNode(dc) ) } @@ -121,8 +121,8 @@ signature predicate relevantApi(ExternalApi api); module Results { private int getUsages(string apiName) { result = - strictcount(DispatchCall c, ExternalApi api | - c = api.getACall() and + strictcount(Call c, ExternalApi api | + c.getTarget().getUnboundDeclaration() = api and apiName = api.getApiName() and getRelevantUsages(api) ) diff --git a/csharp/ql/src/Telemetry/ExternalLibraryUsage.ql b/csharp/ql/src/Telemetry/ExternalLibraryUsage.ql index a9f70b84394..f06a18054dc 100644 --- a/csharp/ql/src/Telemetry/ExternalLibraryUsage.ql +++ b/csharp/ql/src/Telemetry/ExternalLibraryUsage.ql @@ -12,8 +12,8 @@ private import ExternalApi private predicate getRelevantUsages(string namespace, int usages) { usages = - strictcount(DispatchCall c, ExternalApi api | - c = api.getACall() and + strictcount(Call c, ExternalApi api | + c.getTarget().getUnboundDeclaration() = api and api.getNamespace() = namespace and not api.isUninteresting() ) From 366b94addc3dc24cea933f135abf7ef498fe14de Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 2 Nov 2022 14:55:35 +0100 Subject: [PATCH 020/174] C#: Implement override for getAPrimaryQlClass for AnonymousClass. --- csharp/ql/lib/semmle/code/csharp/Type.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/csharp/ql/lib/semmle/code/csharp/Type.qll b/csharp/ql/lib/semmle/code/csharp/Type.qll index f95ded84771..dfabea580a4 100644 --- a/csharp/ql/lib/semmle/code/csharp/Type.qll +++ b/csharp/ql/lib/semmle/code/csharp/Type.qll @@ -824,6 +824,8 @@ class RecordClass extends RecordType, Class { */ class AnonymousClass extends Class { AnonymousClass() { anonymous_types(this) } + + override string getAPrimaryQlClass() { result = "AnonymousClass" } } /** From 187ece610b2fb5539fef80db6cd9d9ef7c5c57aa Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 2 Nov 2022 15:09:31 +0100 Subject: [PATCH 021/174] C#: Only evaluate api name and namespace strings if they are needed. --- csharp/ql/src/Telemetry/ExternalApi.qll | 3 +++ 1 file changed, 3 insertions(+) diff --git a/csharp/ql/src/Telemetry/ExternalApi.qll b/csharp/ql/src/Telemetry/ExternalApi.qll index d3611fc0a2a..653eb419b7f 100644 --- a/csharp/ql/src/Telemetry/ExternalApi.qll +++ b/csharp/ql/src/Telemetry/ExternalApi.qll @@ -37,6 +37,7 @@ class ExternalApi extends DotNet::Callable { /** * Gets the unbound type, name and parameter types of this API. */ + bindingset[this] private string getSignature() { result = this.getDeclaringType().getUnboundDeclaration() + "." + this.getName() + "(" + @@ -46,11 +47,13 @@ class ExternalApi extends DotNet::Callable { /** * Gets the namespace of this API. */ + bindingset[this] string getNamespace() { this.getDeclaringType().hasQualifiedName(result, _) } /** * Gets the namespace and signature of this API. */ + bindingset[this] string getApiName() { result = this.getNamespace() + "#" + this.getSignature() } /** Gets a node that is an input to a call to this API. */ From d58072216461907262b6fce6975c590ba82fb274 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 3 Nov 2022 15:04:19 +0100 Subject: [PATCH 022/174] C#: Modify unsupported external library meta query to use call instead of dispatchcall. --- csharp/ql/src/meta/frameworks/UnsupportedExternalAPIs.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/csharp/ql/src/meta/frameworks/UnsupportedExternalAPIs.ql b/csharp/ql/src/meta/frameworks/UnsupportedExternalAPIs.ql index 6332dfc515c..1934fdc72cc 100644 --- a/csharp/ql/src/meta/frameworks/UnsupportedExternalAPIs.ql +++ b/csharp/ql/src/meta/frameworks/UnsupportedExternalAPIs.ql @@ -14,9 +14,9 @@ private import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSumma private import semmle.code.csharp.dataflow.internal.NegativeSummary private import Telemetry.ExternalApi -from DispatchCall c, ExternalApi api +from Call c, ExternalApi api where - c = api.getACall() and + c.getTarget().getUnboundDeclaration() = api and not api.isUninteresting() and not api.isSupported() and not api instanceof FlowSummaryImpl::Public::NegativeSummarizedCallable From 5ec22bc180da321822f694b00a5f9614e6220071 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 1 Nov 2022 09:52:31 +0100 Subject: [PATCH 023/174] add a shared regex pack --- .../2022-09-26-initial-version.md | 4 + shared/regex/codeql-pack.lock.yml | 4 + .../codeql/regex/OverlyLargeRangeQuery.qll | 300 ++++ shared/regex/codeql/regex/RegexTreeView.qll | 451 ++++++ .../codeql/regex/nfa/BadTagFilterQuery.qll | 177 +++ .../regex/nfa/ExponentialBackTracking.qll | 355 +++++ shared/regex/codeql/regex/nfa/NfaUtils.qll | 1378 +++++++++++++++++ .../regex/codeql/regex/nfa/RegexpMatching.qll | 176 +++ .../regex/nfa/SuperlinearBackTracking.qll | 440 ++++++ shared/regex/qlpack.yml | 5 + 10 files changed, 3290 insertions(+) create mode 100644 shared/regex/change-notes/2022-09-26-initial-version.md create mode 100644 shared/regex/codeql-pack.lock.yml create mode 100644 shared/regex/codeql/regex/OverlyLargeRangeQuery.qll create mode 100644 shared/regex/codeql/regex/RegexTreeView.qll create mode 100644 shared/regex/codeql/regex/nfa/BadTagFilterQuery.qll create mode 100644 shared/regex/codeql/regex/nfa/ExponentialBackTracking.qll create mode 100644 shared/regex/codeql/regex/nfa/NfaUtils.qll create mode 100644 shared/regex/codeql/regex/nfa/RegexpMatching.qll create mode 100644 shared/regex/codeql/regex/nfa/SuperlinearBackTracking.qll create mode 100644 shared/regex/qlpack.yml diff --git a/shared/regex/change-notes/2022-09-26-initial-version.md b/shared/regex/change-notes/2022-09-26-initial-version.md new file mode 100644 index 00000000000..e4d6e0490c2 --- /dev/null +++ b/shared/regex/change-notes/2022-09-26-initial-version.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Initial release. Extracted common regex related code, including the ReDoS analysis, into a library pack to share code between languages. diff --git a/shared/regex/codeql-pack.lock.yml b/shared/regex/codeql-pack.lock.yml new file mode 100644 index 00000000000..a046f6d9786 --- /dev/null +++ b/shared/regex/codeql-pack.lock.yml @@ -0,0 +1,4 @@ +--- +dependencies: {} +compiled: false +lockVersion: 1.0.0 \ No newline at end of file diff --git a/shared/regex/codeql/regex/OverlyLargeRangeQuery.qll b/shared/regex/codeql/regex/OverlyLargeRangeQuery.qll new file mode 100644 index 00000000000..8d3a0b9c0ff --- /dev/null +++ b/shared/regex/codeql/regex/OverlyLargeRangeQuery.qll @@ -0,0 +1,300 @@ +/** + * Classes and predicates for working with suspicious character ranges. + */ + +private import RegexTreeView + +/** + * Classes and predicates implementing an analysis detecting suspicious character ranges. + */ +module Make { + private import TreeImpl + + /** + * Gets a rank for `range` that is unique for ranges in the same file. + * Prioritizes ranges that match more characters. + */ + int rankRange(RegExpCharacterRange range) { + range = + rank[result](RegExpCharacterRange r, int startline, int startcolumn, int low, int high | + r.hasLocationInfo(_, startline, startcolumn, _, _) and + isRange(r, low, high) + | + r order by (high - low) desc, startline, startcolumn + ) + } + + /** Holds if `range` spans from the unicode code points `low` to `high` (both inclusive). */ + predicate isRange(RegExpCharacterRange range, int low, int high) { + exists(string lowc, string highc | + range.isRange(lowc, highc) and + low.toUnicode() = lowc and + high.toUnicode() = highc + ) + } + + /** Holds if `char` is an alpha-numeric character. */ + predicate isAlphanumeric(string char) { + // written like this to avoid having a bindingset for the predicate + char = [[48 .. 57], [65 .. 90], [97 .. 122]].toUnicode() // 0-9, A-Z, a-z + } + + /** + * Holds if the given ranges are from the same character class + * and there exists at least one character matched by both ranges. + */ + predicate overlap(RegExpCharacterRange a, RegExpCharacterRange b) { + exists(RegExpCharacterClass clz | + a = clz.getAChild() and + b = clz.getAChild() and + a != b + | + exists(int alow, int ahigh, int blow, int bhigh | + isRange(a, alow, ahigh) and + isRange(b, blow, bhigh) and + alow <= bhigh and + blow <= ahigh + ) + ) + } + + /** + * Holds if `range` overlaps with the char class `escape` from the same character class. + */ + predicate overlapsWithCharEscape(RegExpCharacterRange range, RegExpCharacterClassEscape escape) { + exists(RegExpCharacterClass clz, string low, string high | + range = clz.getAChild() and + escape = clz.getAChild() and + range.isRange(low, high) + | + escape.getValue() = "w" and + getInRange(low, high).regexpMatch("\\w") + or + escape.getValue() = "d" and + getInRange(low, high).regexpMatch("\\d") + or + escape.getValue() = "s" and + getInRange(low, high).regexpMatch("\\s") + ) + } + + /** Gets the unicode code point for a `char`. */ + bindingset[char] + int toCodePoint(string char) { result.toUnicode() = char } + + /** A character range that appears to be overly wide. */ + class OverlyWideRange instanceof RegExpCharacterRange { + OverlyWideRange() { + exists(int low, int high, int numChars | + isRange(this, low, high) and + numChars = (1 + high - low) and + this.getRootTerm().isUsedAsRegExp() and + numChars >= 10 + | + // across the Z-a range (which includes backticks) + toCodePoint("Z") >= low and + toCodePoint("a") <= high + or + // across the 9-A range (which includes e.g. ; and ?) + toCodePoint("9") >= low and + toCodePoint("A") <= high + or + // a non-alphanumeric char as part of the range boundaries + exists(int bound | bound = [low, high] | not isAlphanumeric(bound.toUnicode())) and + // while still being ascii + low < 128 and + high < 128 + ) and + // allowlist for known ranges + not this = allowedWideRanges() + } + + /** Gets a string representation of a character class that matches the same chars as this range. */ + string printEquivalent() { result = RangePrinter::printEquivalentCharClass(this) } + + /** Gets a string representation of this range. */ + string toString() { result = super.toString() } + + /** Holds if `lo` is the lower bound of this character range and `hi` the upper bound. */ + predicate isRange(string lo, string hi) { super.isRange(lo, hi) } + } + + /** Gets a range that should not be reported as an overly wide range. */ + RegExpCharacterRange allowedWideRanges() { + // ~ is the last printable ASCII character, it's used right in various wide ranges. + result.isRange(_, "~") + or + // the same with " " and "!". " " is the first printable character, and "!" is the first non-white-space printable character. + result.isRange([" ", "!"], _) + or + // the `[@-_]` range is intentional + result.isRange("@", "_") + or + // starting from the zero byte is a good indication that it's purposely matching a large range. + result.isRange(0.toUnicode(), _) + } + + /** Gets a char between (and including) `low` and `high`. */ + bindingset[low, high] + private string getInRange(string low, string high) { + result = [toCodePoint(low) .. toCodePoint(high)].toUnicode() + } + + /** A module computing an equivalent character class for an overly wide range. */ + module RangePrinter { + bindingset[char] + bindingset[result] + private string next(string char) { + exists(int prev, int next | + prev.toUnicode() = char and + next.toUnicode() = result and + next = prev + 1 + ) + } + + /** Gets the points where the parts of the pretty printed range should be cut off. */ + private string cutoffs() { result = ["A", "Z", "a", "z", "0", "9"] } + + /** Gets the char to use in the low end of a range for a given `cut` */ + private string lowCut(string cut) { + cut = ["A", "a", "0"] and + result = cut + or + cut = ["Z", "z", "9"] and + result = next(cut) + } + + /** Gets the char to use in the high end of a range for a given `cut` */ + private string highCut(string cut) { + cut = ["Z", "z", "9"] and + result = cut + or + cut = ["A", "a", "0"] and + next(result) = cut + } + + /** Gets the cutoff char used for a given `part` of a range when pretty-printing it. */ + private string cutoff(OverlyWideRange range, int part) { + exists(int low, int high | isRange(range, low, high) | + result = + rank[part + 1](string cut | + cut = cutoffs() and low < toCodePoint(cut) and toCodePoint(cut) < high + | + cut order by toCodePoint(cut) + ) + ) + } + + /** Gets the number of parts we should print for a given `range`. */ + private int parts(OverlyWideRange range) { result = 1 + count(cutoff(range, _)) } + + /** Holds if the given part of a range should span from `low` to `high`. */ + private predicate part(OverlyWideRange range, int part, string low, string high) { + // first part. + part = 0 and + ( + range.isRange(low, high) and + parts(range) = 1 + or + parts(range) >= 2 and + range.isRange(low, _) and + high = highCut(cutoff(range, part)) + ) + or + // middle + part >= 1 and + part < parts(range) - 1 and + low = lowCut(cutoff(range, part - 1)) and + high = highCut(cutoff(range, part)) + or + // last. + part = parts(range) - 1 and + low = lowCut(cutoff(range, part - 1)) and + range.isRange(_, high) + } + + /** Gets an escaped `char` for use in a character class. */ + bindingset[char] + private string escape(string char) { + exists(string reg | reg = "(\\[|\\]|\\\\|-|/)" | + if char.regexpMatch(reg) then result = "\\" + char else result = char + ) + } + + /** Gets a part of the equivalent range. */ + private string printEquivalentCharClass(OverlyWideRange range, int part) { + exists(string low, string high | part(range, part, low, high) | + if + isAlphanumeric(low) and + isAlphanumeric(high) + then result = low + "-" + high + else + result = + strictconcat(string char | char = getInRange(low, high) | escape(char) order by char) + ) + } + + /** Gets the entire pretty printed equivalent range. */ + string printEquivalentCharClass(OverlyWideRange range) { + result = + strictconcat(string r, int part | + r = "[" and part = -1 and exists(range) + or + r = printEquivalentCharClass(range, part) + or + r = "]" and part = parts(range) + | + r order by part + ) + } + } + + /** Gets a char range that is overly large because of `reason`. */ + RegExpCharacterRange getABadRange(string reason, int priority) { + result instanceof OverlyWideRange and + priority = 0 and + exists(string equiv | equiv = result.(OverlyWideRange).printEquivalent() | + if equiv.length() <= 50 + then reason = "is equivalent to " + equiv + else reason = "is equivalent to " + equiv.substring(0, 50) + "..." + ) + or + priority = 1 and + exists(RegExpCharacterRange other | + reason = "overlaps with " + other + " in the same character class" and + rankRange(result) < rankRange(other) and + overlap(result, other) + ) + or + priority = 2 and + exists(RegExpCharacterClassEscape escape | + reason = "overlaps with " + escape + " in the same character class" and + overlapsWithCharEscape(result, escape) + ) + or + reason = "is empty" and + priority = 3 and + exists(int low, int high | + isRange(result, low, high) and + low > high + ) + } + + /** Holds if `range` matches suspiciously many characters. */ + predicate problem(RegExpCharacterRange range, string reason) { + reason = + strictconcat(string m, int priority | + range = getABadRange(m, priority) + | + m, ", and " order by priority desc + ) and + // specifying a range using an escape is usually OK. + not range.getAChild() instanceof RegExpEscape and + // Unicode escapes in strings are interpreted before it turns into a regexp, + // so e.g. [\u0001-\uFFFF] will just turn up as a range between two constants. + // We therefore exclude these ranges. + range.getRootTerm().getParent() instanceof RegExpLiteral and + // is used as regexp (mostly for JS where regular expressions are parsed eagerly) + range.getRootTerm().isUsedAsRegExp() + } +} diff --git a/shared/regex/codeql/regex/RegexTreeView.qll b/shared/regex/codeql/regex/RegexTreeView.qll new file mode 100644 index 00000000000..f805bd83185 --- /dev/null +++ b/shared/regex/codeql/regex/RegexTreeView.qll @@ -0,0 +1,451 @@ +/** + * This file contains a `RegexTreeViewSig` module describing the syntax tree of regular expressions. + */ + +/** + * A signature describing the syntax tree of regular expressions. + */ +signature module RegexTreeViewSig { + /** + * An element used in some way as or in a regular expression. + * This class exists to have a common supertype that all languages can agree on. + */ + class Top; + + /** + * An element containing a regular expression term, that is, either + * a string literal (parsed as a regular expression; the root of the parse tree) + * or another regular expression term (a descendant of the root). + */ + class RegExpParent extends Top; + + /** + * A regular expression literal. + * + * Note that this class does not cover regular expressions constructed by calling the built-in + * `RegExp` function. + * + * Example: + * + * ``` + * /(?i)ab*c(d|e)$/ + * ``` + */ + class RegExpLiteral extends RegExpParent; + + /** + * A regular expression term, that is, a syntactic part of a regular expression. + * These are the tree nodes that form the parse tree of a regular expression literal. + */ + class RegExpTerm extends Top { + /** Gets a child term of this term. */ + RegExpTerm getAChild(); + + /** + * Holds if this is the root term of a regular expression. + */ + predicate isRootTerm(); + + /** + * Gets the parent term of this regular expression term, or the + * regular expression literal if this is the root term. + */ + RegExpParent getParent(); + + /** + * Holds if this term is part of a regular expression literal, or a string literal + * that is interpreted as a regular expression. + */ + predicate isUsedAsRegExp(); + + /** Gets the outermost term of this regular expression. */ + RegExpTerm getRootTerm(); + + /** Gets the raw source text of this term. */ + string getRawValue(); + + /** Gets the `i`th child term of this term. */ + RegExpTerm getChild(int i); + + /** Gets the number of child terms of this term. */ + int getNumChild(); + + /** Gets the regular expression term that is matched (textually) after this one, if any. */ + RegExpTerm getSuccessor(); + + string toString(); + + predicate hasLocationInfo( + string filepath, int startline, int startcolumn, int endline, int endcolumn + ); + } + + /** + * A quantified regular expression term. + * + * Example: + * + * ``` + * ((ECMA|Java)[sS]cript)* + * ``` + */ + class RegExpQuantifier extends RegExpTerm; + + /** + * A star-quantified term. + * + * Example: + * + * ``` + * \w* + * ``` + */ + class RegExpStar extends RegExpQuantifier; + + /** + * An optional term. + * + * Example: + * + * ``` + * ;? + * ``` + */ + class RegExpOpt extends RegExpQuantifier; + + /** + * A plus-quantified term. + * + * Example: + * + * ``` + * \w+ + * ``` + */ + class RegExpPlus extends RegExpQuantifier; + + /** + * A range-quantified term + * + * Examples: + * + * ``` + * \w{2,4} + * \w{2,} + * \w{2} + * ``` + */ + class RegExpRange extends RegExpQuantifier { + /** Gets the lower bound of the range. */ + int getLowerBound(); + + /** + * Gets the upper bound of the range, if any. + * + * If there is no upper bound, any number of repetitions is allowed. + * For a term of the form `r{lo}`, both the lower and the upper bound + * are `lo`. + */ + int getUpperBound(); + } + + /** + * An escaped regular expression term, that is, a regular expression + * term starting with a backslash. + * + * Example: + * + * ``` + * \. + * \w + * ``` + */ + class RegExpEscape extends RegExpTerm; + + /** + * A character class escape in a regular expression. + * + * Examples: + * + * ``` + * \w + * \S + * ``` + */ + class RegExpCharacterClassEscape extends RegExpEscape { + /** Gets the name of the character class; for example, `w` for `\w`. */ + string getValue(); + } + + /** + * An alternative term, that is, a term of the form `a|b`. + * + * Example: + * + * ``` + * ECMA|Java + * ``` + */ + class RegExpAlt extends RegExpTerm; + + /** + * A grouped regular expression. + * + * Examples: + * + * ``` + * (ECMA|Java) + * (?:ECMA|Java) + * (?['"]) + * ``` + */ + class RegExpGroup extends RegExpTerm { + /** + * Gets the index of this capture group within the enclosing regular + * expression literal. + * + * For example, in the regular expression `/((a?).)(?:b)/`, the + * group `((a?).)` has index 1, the group `(a?)` nested inside it + * has index 2, and the group `(?:b)` has no index, since it is + * not a capture group. + */ + int getNumber(); + } + + /** + * A back reference, that is, a term of the form `\i` or `\k` + * in a regular expression. + * + * Examples: + * + * ``` + * \1 + * \k + * ``` + */ + class RegExpBackRef extends RegExpTerm { + /** Gets the capture group this back reference refers to. */ + RegExpGroup getGroup(); + } + + /** + * A sequence term. + * + * Example: + * + * ``` + * (ECMA|Java)Script + * ``` + * + * This is a sequence with the elements `(ECMA|Java)` and `Script`. + */ + class RegExpSequence extends RegExpTerm; + + /** + * A zero-width lookahead or lookbehind assertion. + * + * Examples: + * + * ``` + * (?=\w) + * (?!\n) + * (?<=\.) + * (?&] + * ``` + */ + class RegExpCharacterClass extends RegExpTerm { + /** + * Holds if this character class matches any character. + */ + predicate isUniversalClass(); + + /** Holds if this is an inverted character class, that is, a term of the form `[^...]`. */ + predicate isInverted(); + } + + /** + * A character range in a character class in a regular expression. + * + * Example: + * + * ``` + * a-z + * ``` + */ + class RegExpCharacterRange extends RegExpTerm { + /** Holds if `lo` is the lower bound of this character range and `hi` the upper bound. */ + predicate isRange(string lo, string hi); + } + + /** + * A dot regular expression. + * + * Example: + * + * ``` + * . + * ``` + */ + class RegExpDot extends RegExpTerm; + + /** + * A dollar assertion `$` matching the end of a line. + * + * Example: + * + * ``` + * $ + * ``` + */ + class RegExpDollar extends RegExpTerm; + + /** + * A caret assertion `^` matching the beginning of a line. + * + * Example: + * + * ``` + * ^ + * ``` + */ + class RegExpCaret extends RegExpTerm; + + /** + * A word boundary assertion. + * + * Example: + * + * ``` + * \b + * ``` + */ + class RegExpWordBoundary extends RegExpTerm; + + /** + * A regular expression term that permits unlimited repetitions. + */ + class InfiniteRepetitionQuantifier extends RegExpQuantifier; + + /** + * Holds if the regular expression should not be considered. + * + * For javascript we make the pragmatic performance optimization to ignore minified files. + */ + predicate isExcluded(RegExpParent parent); + + /** + * Holds if `term` is a possessive quantifier. + * As javascript's regexes do not support possessive quantifiers, this never holds, but is used by the shared library. + */ + predicate isPossessive(RegExpQuantifier term); + + /** + * Holds if the regex that `term` is part of is used in a way that ignores any leading prefix of the input it's matched against. + * Not yet implemented for JavaScript. + */ + predicate matchesAnyPrefix(RegExpTerm term); + + /** + * Holds if the regex that `term` is part of is used in a way that ignores any trailing suffix of the input it's matched against. + * Not yet implemented for JavaScript. + */ + predicate matchesAnySuffix(RegExpTerm term); + + /** + * Holds if `term` is an escape class representing e.g. `\d`. + * `clazz` is which character class it represents, e.g. "d" for `\d`. + */ + predicate isEscapeClass(RegExpTerm term, string clazz); + + /** + * Holds if `root` has the `i` flag for case-insensitive matching. + */ + predicate isIgnoreCase(RegExpTerm root); + + /** + * Holds if `root` has the `s` flag for multi-line matching. + */ + predicate isDotAll(RegExpTerm root); +} diff --git a/shared/regex/codeql/regex/nfa/BadTagFilterQuery.qll b/shared/regex/codeql/regex/nfa/BadTagFilterQuery.qll new file mode 100644 index 00000000000..c9c254fe990 --- /dev/null +++ b/shared/regex/codeql/regex/nfa/BadTagFilterQuery.qll @@ -0,0 +1,177 @@ +/** + * Provides predicates for reasoning about bad tag filter vulnerabilities. + */ + +private import NfaUtils as NfaUtils +private import RegexpMatching as RM +private import codeql.regex.RegexTreeView + +/** + * Module implementing classes and predicates reasoing about bad tag filter vulnerabilities. + */ +module Make { + private import TreeImpl + import RM::Make + + /** + * Holds if the regexp `root` should be tested against `str`. + * Implements the `isRegexpMatchingCandidateSig` signature from `RegexpMatching`. + * `ignorePrefix` toggles whether the regular expression should be treated as accepting any prefix if it's unanchored. + * `testWithGroups` toggles whether it's tested which groups are filled by a given input string. + */ + private predicate isBadTagFilterCandidate( + RootTerm root, string str, boolean ignorePrefix, boolean testWithGroups + ) { + // the regexp must mention "<" and ">" explicitly. + forall(string angleBracket | angleBracket = ["<", ">"] | + any(RegExpConstant term | term.getValue().matches("%" + angleBracket + "%")).getRootTerm() = + root + ) and + ignorePrefix = true and + ( + str = ["", "", "", "", "", + "", "", "", "", + "", "", + "", "", "", + "", "", + "", "", + "") and + regexp.matches("") and + not regexp.matches("") and + ( + not regexp.matches("") and + msg = "This regular expression matches , but not " + or + not regexp.matches("") and + msg = "This regular expression matches , but not " + ) + or + regexp.matches("") and + regexp.matches("") and + not regexp.matches("") and + not regexp.matches("") and + msg = + "This regular expression does not match script tags where the attribute uses single-quotes." + or + regexp.matches("") and + regexp.matches("") and + not regexp.matches("") and + not regexp.matches("") and + msg = + "This regular expression does not match script tags where the attribute uses double-quotes." + or + regexp.matches("") and + regexp.matches("") and + not regexp.matches("") and + not regexp.matches("") and + not regexp.matches("") and + msg = + "This regular expression does not match script tags where tabs are used between attributes." + or + regexp.matches("") and + not isIgnoreCase(regexp) and + not regexp.matches("") and + not regexp.matches("") and + ( + not regexp.matches("") and + msg = "This regular expression does not match upper case ") and + regexp.matches("") and + msg = "This regular expression does not match mixed case ") and + not regexp.matches("") and + not regexp.matches("") and + ( + not regexp.matches("") and + msg = "This regular expression does not match script end tags like ." + or + not regexp.matches("") and + msg = "This regular expression does not match script end tags like ." + or + not regexp.matches("", - "", "", "", "", - "", "", - "", "", "", - "", "", "", - "", "") and - regexp.matches("") and - not regexp.matches("") and - ( - not regexp.matches("") and - msg = "This regular expression matches , but not " - or - not regexp.matches("") and - msg = "This regular expression matches , but not " - ) - or - regexp.matches("") and - regexp.matches("") and - not regexp.matches("") and - not regexp.matches("") and - msg = "This regular expression does not match script tags where the attribute uses single-quotes." - or - regexp.matches("") and - regexp.matches("") and - not regexp.matches("") and - not regexp.matches("") and - msg = "This regular expression does not match script tags where the attribute uses double-quotes." - or - regexp.matches("") and - regexp.matches("") and - not regexp.matches("") and - not regexp.matches("") and - not regexp.matches("") and - msg = "This regular expression does not match script tags where tabs are used between attributes." - or - regexp.matches("") and - not RegExpFlags::isIgnoreCase(regexp) and - not regexp.matches("") and - not regexp.matches("") and - ( - not regexp.matches("") and - msg = "This regular expression does not match upper case ") and - regexp.matches("") and - msg = "This regular expression does not match mixed case ") and - not regexp.matches("") and - not regexp.matches("") and - ( - not regexp.matches("") and - msg = "This regular expression does not match script end tags like ." - or - not regexp.matches("") and - msg = "This regular expression does not match script end tags like ." - or - not regexp.matches("