diff --git a/swift/ql/lib/change-notes/2023-10-27-variable-capture.md b/swift/ql/lib/change-notes/2023-10-27-variable-capture.md new file mode 100644 index 000000000000..94c7201c30ba --- /dev/null +++ b/swift/ql/lib/change-notes/2023-10-27-variable-capture.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Improved support for flow through captured variables that properly adheres to inter-procedural control flow. \ No newline at end of file diff --git a/swift/ql/lib/codeql/swift/controlflow/CfgNodes.qll b/swift/ql/lib/codeql/swift/controlflow/CfgNodes.qll index b8467b098f2f..eab8385be366 100644 --- a/swift/ql/lib/codeql/swift/controlflow/CfgNodes.qll +++ b/swift/ql/lib/codeql/swift/controlflow/CfgNodes.qll @@ -173,7 +173,7 @@ class ApplyExprCfgNode extends ExprCfgNode { Callable getStaticTarget() { result = e.getStaticTarget() } - Expr getFunction() { result = e.getFunction() } + CfgNode getFunction() { result.getAst() = e.getFunction() } } class CallExprCfgNode extends ApplyExprCfgNode { diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll index ae25463b21ac..554869409182 100644 --- a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll +++ b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll @@ -10,6 +10,7 @@ private import codeql.swift.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl private import codeql.swift.frameworks.StandardLibrary.PointerTypes private import codeql.swift.frameworks.StandardLibrary.Array private import codeql.swift.frameworks.StandardLibrary.Dictionary +private import codeql.dataflow.VariableCapture as VariableCapture /** Gets the callable in which this node occurs. */ DataFlowCallable nodeGetEnclosingCallable(Node n) { result = n.(NodeImpl).getEnclosingCallable() } @@ -98,6 +99,16 @@ private class SsaDefinitionNodeImpl extends SsaDefinitionNode, NodeImpl { } } +private class CaptureNodeImpl extends CaptureNode, NodeImpl { + override Location getLocationImpl() { result = this.getSynthesizedCaptureNode().getLocation() } + + override string toStringImpl() { result = this.getSynthesizedCaptureNode().toString() } + + override DataFlowCallable getEnclosingCallable() { + result.asSourceCallable() = this.getSynthesizedCaptureNode().getEnclosingCallable() + } +} + private predicate localFlowSsaInput(Node nodeFrom, Ssa::Definition def, Ssa::Definition next) { exists(BasicBlock bb, int i | def.lastRefRedef(bb, i, next) | def.definesAt(_, bb, i) and @@ -140,19 +151,22 @@ private module Cached { [ any(Argument arg | modifiable(arg)).getExpr(), any(MemberRefExpr ref).getBase(), any(ApplyExpr apply).getQualifier(), any(TupleElementExpr te).getSubExpr(), - any(SubscriptExpr se).getBase() + any(SubscriptExpr se).getBase(), + any(ApplyExpr apply | not exists(apply.getStaticTarget())).getFunction() ]) } or TDictionarySubscriptNode(SubscriptExpr e) { e.getBase().getType().getCanonicalType() instanceof CanonicalDictionaryType - } + } or + TCaptureNode(CaptureFlow::SynthesizedCaptureNode cn) or + TClosureSelfParameterNode(ClosureExpr closure) private predicate localSsaFlowStepUseUse(Ssa::Definition def, Node nodeFrom, Node nodeTo) { def.adjacentReadPair(nodeFrom.getCfgNode(), nodeTo.getCfgNode()) and ( - nodeTo instanceof InoutReturnNode + nodeTo instanceof InoutReturnNodeImpl implies - nodeTo.(InoutReturnNode).getParameter() = def.getSourceVariable().asVarDecl() + nodeTo.(InoutReturnNodeImpl).getParameter() = def.getSourceVariable().asVarDecl() ) } @@ -167,7 +181,7 @@ private module Cached { * Holds if `nodeFrom` is a parameter node, and `nodeTo` is a corresponding SSA node. */ private predicate localFlowSsaParamInput(Node nodeFrom, Node nodeTo) { - nodeTo = getParameterDefNode(nodeFrom.(ParameterNode).getParameter()) + nodeTo = getParameterDefNode(nodeFrom.asParameter()) } private predicate localFlowStepCommon(Node nodeFrom, Node nodeTo) { @@ -180,9 +194,9 @@ private module Cached { nodeFrom.asDefinition() = def and nodeTo.getCfgNode() = def.getAFirstRead() and ( - nodeTo instanceof InoutReturnNode + nodeTo instanceof InoutReturnNodeImpl implies - nodeTo.(InoutReturnNode).getParameter() = def.getSourceVariable().asVarDecl() + nodeTo.(InoutReturnNodeImpl).getParameter() = def.getSourceVariable().asVarDecl() ) or // use-use flow @@ -279,6 +293,9 @@ private module Cached { // flow through a flow summary (extension of `SummaryModelCsv`) FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom.(FlowSummaryNode).getSummaryNode(), nodeTo.(FlowSummaryNode).getSummaryNode(), true) + or + // flow step according to the closure capture library + captureValueStep(nodeFrom, nodeTo) } /** @@ -305,7 +322,8 @@ private module Cached { TFieldContent(FieldDecl f) or TTupleContent(int index) { exists(any(TupleExpr te).getElement(index)) } or TEnumContent(ParamDecl f) { exists(EnumElementDecl d | d.getAParam() = f) } or - TCollectionContent() + TCollectionContent() or + TCapturedVariableContent(CapturedVariable v) } /** @@ -342,7 +360,9 @@ private predicate hasPatternNode(PatternCfgNode n, Pattern p) { import Cached /** Holds if `n` should be hidden from path explanations. */ -predicate nodeIsHidden(Node n) { n instanceof FlowSummaryNode } +predicate nodeIsHidden(Node n) { + n instanceof FlowSummaryNode or n instanceof ClosureSelfParameterNode +} /** * The intermediate node for a dictionary subscript operation `dict[key]`. In a write, this is used @@ -396,6 +416,25 @@ private module ParameterNodes { override ParamDecl getParameter() { result = param } } + class ClosureSelfParameterNode extends ParameterNodeImpl, TClosureSelfParameterNode { + ClosureExpr closure; + + ClosureSelfParameterNode() { this = TClosureSelfParameterNode(closure) } + + override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) { + c.asSourceCallable() = closure and + pos instanceof TThisParameter + } + + override Location getLocationImpl() { result = closure.getLocation() } + + override string toStringImpl() { result = "closure self parameter" } + + override DataFlowCallable getEnclosingCallable() { this.isParameterOf(result, _) } + + ClosureExpr getClosure() { result = closure } + } + class SummaryParameterNode extends ParameterNodeImpl, FlowSummaryNode { SummaryParameterNode() { FlowSummaryImpl::Private::summaryParameterNode(this.getSummaryNode(), _) @@ -610,6 +649,18 @@ private module ArgumentNodes { pos = TPositionalArgument(0) } } + + class SelfClosureArgumentNode extends ExprNode, ArgumentNode { + ApplyExprCfgNode apply; + + SelfClosureArgumentNode() { n = apply.getFunction() } + + override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) { + apply = call.asCall() and + not exists(apply.getStaticTarget()) and + pos instanceof ThisArgumentPosition + } + } } import ArgumentNodes @@ -785,6 +836,175 @@ private module OutNodes { import OutNodes +/** + * Holds if there is a data flow step from `e1` to `e2` that only steps from + * child to parent in the AST. + */ +private predicate simpleAstFlowStep(Expr e1, Expr e2) { + e2.(IfExpr).getBranch(_) = e1 + or + e2.(AssignExpr).getSource() = e1 + or + e2.(ArrayExpr).getAnElement() = e1 +} + +private predicate closureFlowStep(CaptureInput::Expr e1, CaptureInput::Expr e2) { + simpleAstFlowStep(e1, e2) + or + exists(Ssa::WriteDefinition def | + def.getARead().getNode().asAstNode() = e2 and + def.assigns(any(CfgNode cfg | cfg.getNode().asAstNode() = e1)) + ) + or + e2.(Pattern).getImmediateMatchingExpr() = e1 +} + +private module CaptureInput implements VariableCapture::InputSig { + private import swift as S + private import codeql.swift.controlflow.BasicBlocks as B + + class Location = S::Location; + + class BasicBlock instanceof B::BasicBlock { + string toString() { result = super.toString() } + + Callable getEnclosingCallable() { result = super.getScope() } + + Location getLocation() { result = super.getLocation() } + } + + BasicBlock getImmediateBasicBlockDominator(BasicBlock bb) { + result.(B::BasicBlock).immediatelyDominates(bb) + } + + BasicBlock getABasicBlockSuccessor(BasicBlock bb) { result = bb.(B::BasicBlock).getASuccessor() } + + class CapturedVariable instanceof S::VarDecl { + CapturedVariable() { + any(S::CapturedDecl capturedDecl).getDecl() = this and + exists(this.getEnclosingCallable()) + } + + string toString() { result = super.toString() } + + Callable getCallable() { result = super.getEnclosingCallable() } + + Location getLocation() { result = super.getLocation() } + } + + class CapturedParameter extends CapturedVariable instanceof S::ParamDecl { } + + class Expr instanceof S::AstNode { + string toString() { result = super.toString() } + + Location getLocation() { result = super.getLocation() } + + predicate hasCfgNode(BasicBlock bb, int i) { + this = bb.(B::BasicBlock).getNode(i).getNode().asAstNode() + } + } + + class VariableWrite extends Expr { + CapturedVariable variable; + Expr source; + + VariableWrite() { + exists(S::Assignment a | this = a | + a.getDest().(DeclRefExpr).getDecl() = variable and + source = a.getSource() + ) + or + exists(S::NamedPattern np | this = np | + variable = np.getVarDecl() and + source = np.getMatchingExpr() + ) + } + + CapturedVariable getVariable() { result = variable } + + Expr getSource() { result = source } + } + + class VariableRead extends Expr instanceof S::DeclRefExpr { + CapturedVariable v; + + VariableRead() { this.getDecl() = v and not isLValue(this) } + + CapturedVariable getVariable() { result = v } + } + + class ClosureExpr extends Expr instanceof S::Callable { + ClosureExpr() { any(S::CapturedDecl c).getScope() = this } + + predicate hasBody(Callable body) { this = body } + + predicate hasAliasedAccess(Expr f) { closureFlowStep+(this, f) and not closureFlowStep(f, _) } + } + + class Callable extends S::Callable { + predicate isConstructor() { + // A class declaration cannot capture a variable in Swift. Consider this hypothetical example: + // ``` + // protocol Interface { } + // func foo() -> Interface { + // let y = 42 + // class Impl : Interface { + // let x : Int + // init() { + // x = y + // } + // } + // let object = Impl() + // return object + // } + // ``` + // The Swift compiler will reject this with an error message such as + // ``` + // error: class declaration cannot close over value 'y' defined in outer scope + // x = y + // ^ + // ``` + none() + } + } +} + +class CapturedVariable = CaptureInput::CapturedVariable; + +class CapturedParameter = CaptureInput::CapturedParameter; + +module CaptureFlow = VariableCapture::Flow; + +private CaptureFlow::ClosureNode asClosureNode(Node n) { + result = n.(CaptureNode).getSynthesizedCaptureNode() + or + result.(CaptureFlow::ExprNode).getExpr() = n.asExpr() + or + result.(CaptureFlow::ExprPostUpdateNode).getExpr() = + n.(PostUpdateNode).getPreUpdateNode().asExpr() + or + result.(CaptureFlow::ParameterNode).getParameter() = n.asParameter() + or + result.(CaptureFlow::ThisParameterNode).getCallable() = n.(ClosureSelfParameterNode).getClosure() + or + exists(CaptureInput::VariableWrite write | + result.(CaptureFlow::VariableWriteSourceNode).getVariableWrite() = write and + n.asExpr() = write.getSource() + ) +} + +private predicate captureStoreStep(Node node1, Content::CapturedVariableContent c, Node node2) { + CaptureFlow::storeStep(asClosureNode(node1), c.getVariable(), asClosureNode(node2)) +} + +private predicate captureReadStep(Node node1, Content::CapturedVariableContent c, Node node2) { + CaptureFlow::readStep(asClosureNode(node1), c.getVariable(), asClosureNode(node2)) +} + +predicate captureValueStep(Node node1, Node node2) { + CaptureFlow::localFlowStep(asClosureNode(node1), asClosureNode(node2)) +} + predicate jumpStep(Node pred, Node succ) { FlowSummaryImpl::Private::Steps::summaryJumpStep(pred.(FlowSummaryNode).getSummaryNode(), succ.(FlowSummaryNode).getSummaryNode()) @@ -897,6 +1117,8 @@ predicate storeStep(Node node1, ContentSet c, Node node2) { or FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(), c, node2.(FlowSummaryNode).getSummaryNode()) + or + captureStoreStep(node1, any(Content::CapturedVariableContent cvc | c.isSingleton(cvc)), node2) } predicate isLValue(Expr e) { any(AssignExpr assign).getDest() = e } @@ -1001,6 +1223,8 @@ predicate readStep(Node node1, ContentSet c, Node node2) { or FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(), c, node2.(FlowSummaryNode).getSummaryNode()) + or + captureReadStep(node1, any(Content::CapturedVariableContent cvc | c.isSingleton(cvc)), node2) } /** @@ -1094,6 +1318,17 @@ private module PostUpdateNodes { result.(FlowSummaryNode).getSummaryNode()) } } + + class CapturePostUpdateNode extends PostUpdateNodeImpl, CaptureNode { + private CaptureNode pre; + + CapturePostUpdateNode() { + CaptureFlow::capturePostUpdateNode(this.getSynthesizedCaptureNode(), + pre.getSynthesizedCaptureNode()) + } + + override Node getPreUpdateNode() { result = pre } + } } private import PostUpdateNodes @@ -1152,7 +1387,12 @@ predicate additionalLambdaFlowStep(Node nodeFrom, Node nodeTo, boolean preserves * One example would be to allow flow like `p.foo = p.bar;`, which is disallowed * by default as a heuristic. */ -predicate allowParameterReturnInSelf(ParameterNode p) { none() } +predicate allowParameterReturnInSelf(ParameterNode p) { + exists(Callable c | + c = p.(ParameterNodeImpl).getEnclosingCallable().asSourceCallable() and + CaptureFlow::heuristicAllowInstanceParameterReturnInSelf(c) + ) +} /** An approximated `Content`. */ class ContentApprox = Unit; diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll index f8887071451e..abbb400904a6 100644 --- a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll +++ b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll @@ -51,6 +51,11 @@ class Node extends TNode { * Gets this node's underlying SSA definition, if any. */ Ssa::Definition asDefinition() { none() } + + /** + * Gets the parameter that corresponds to this node, if any. + */ + ParamDecl asParameter() { none() } } /** @@ -96,7 +101,7 @@ class ParameterNode extends Node instanceof ParameterNodeImpl { result = this.(ParameterNodeImpl).getEnclosingCallable() } - ParamDecl getParameter() { result = this.(ParameterNodeImpl).getParameter() } + override ParamDecl asParameter() { result = this.(ParameterNodeImpl).getParameter() } } /** @@ -109,9 +114,7 @@ class SsaDefinitionNode extends Node, TSsaDefinitionNode { override Ssa::Definition asDefinition() { result = def } } -class InoutReturnNode extends Node instanceof InoutReturnNodeImpl { - ParamDecl getParameter() { result = super.getParameter() } -} +class InoutReturnNode extends Node instanceof InoutReturnNodeImpl { } /** * A node associated with an object after an operation that might have @@ -129,6 +132,22 @@ class PostUpdateNode extends Node instanceof PostUpdateNodeImpl { Node getPreUpdateNode() { result = super.getPreUpdateNode() } } +/** + * A synthesized data flow node representing a closure object that tracks + * captured variables. + */ +class CaptureNode extends Node, TCaptureNode { + private CaptureFlow::SynthesizedCaptureNode cn; + + CaptureNode() { this = TCaptureNode(cn) } + + /** + * Gets the underlying synthesized capture node that is created by the + * variable capture library. + */ + CaptureFlow::SynthesizedCaptureNode getSynthesizedCaptureNode() { result = cn } +} + /** * Gets a node corresponding to expression `e`. */ @@ -137,7 +156,7 @@ ExprNode exprNode(DataFlowExpr e) { result.asExpr() = e } /** * Gets the node corresponding to the value of parameter `p` at function entry. */ -ParameterNode parameterNode(ParamDecl p) { result.getParameter() = p } +ParameterNode parameterNode(ParamDecl p) { result.asParameter() = p } /** * Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local @@ -234,6 +253,18 @@ module Content { * DEPRECATED: An element of a collection. This is an alias for the general CollectionContent. */ deprecated class ArrayContent = CollectionContent; + + /** A captured variable. */ + class CapturedVariableContent extends Content, TCapturedVariableContent { + CapturedVariable v; + + CapturedVariableContent() { this = TCapturedVariableContent(v) } + + /** Gets the underlying captured variable. */ + CapturedVariable getVariable() { result = v } + + override string toString() { result = v.toString() } + } } /** diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/CInterop.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/CInterop.qll index 35a7cafe1e94..6310512cabe5 100644 --- a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/CInterop.qll +++ b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/CInterop.qll @@ -7,6 +7,10 @@ private import codeql.swift.dataflow.ExternalFlow private class CInteropSummaries extends SummaryModelCsv { override predicate row(string row) { - row = ";;false;getVaList(_:);;;Argument[0].CollectionElement;ReturnValue;value" + row = + [ + ";;false;getVaList(_:);;;Argument[0].CollectionElement;ReturnValue;value", + ";;false;withVaList(_:_:);;;Argument[0];Argument[1].Parameter[0];value" + ] } } diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/WebView.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/WebView.qll index b845ee811049..da54683029f5 100644 --- a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/WebView.qll +++ b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/WebView.qll @@ -66,7 +66,7 @@ private class WKNavigationDelegateSource extends RemoteFlowSource { ] and p.getDeclaringFunction() = f and p.getIndex() = 1 and - this.(DataFlow::ParameterNode).getParameter() = p + this.asParameter() = p ) } @@ -173,7 +173,7 @@ private class JsExportedSource extends RemoteFlowSource { base.getEnclosingDecl().asNominalTypeDecl() instanceof JsExportedProto and adopter.getEnclosingDecl().asNominalTypeDecl() instanceof JsExportedType | - this.(DataFlow::ParameterNode).getParameter().getDeclaringFunction() = adopter and + this.asParameter().getDeclaringFunction() = adopter and pragma[only_bind_out](adopter.getName()) = pragma[only_bind_out](base.getName()) ) or diff --git a/swift/ql/test/TestUtilities/InlineFlowTest.qll b/swift/ql/test/TestUtilities/InlineFlowTest.qll new file mode 100644 index 000000000000..4ab2a7cdf38f --- /dev/null +++ b/swift/ql/test/TestUtilities/InlineFlowTest.qll @@ -0,0 +1,117 @@ +/** + * Provides a simple base test for flow-related tests using inline expectations. + * + * Example for a test.ql: + * ```ql + * import swift + * import TestUtilities.InlineFlowTest + * import DefaultFlowTest + * import PathGraph + * + * from PathNode source, PathNode sink + * where flowPath(source, sink) + * select sink, source, sink, "$@", source, source.toString() + * ``` + * + * To declare expectations, you can use the $hasTaintFlow or $hasValueFlow comments within the test source files. + * Example of the corresponding test file, e.g. Test.java + * ```swift + * func source() -> Any { return nil } + * func taint() -> Any { return nil } + * func sink(_ o: Any) { } + * + * func test() { + * let s = source() + * sink(s) // $ hasValueFlow + * let t = "foo" + taint() + * sink(t); // $ hasTaintFlow + * } + * ``` + * + * If you are only interested in value flow, then instead of importing `DefaultFlowTest`, you can import + * `ValueFlowTest`. Similarly, if you are only interested in taint flow, then instead of + * importing `DefaultFlowTest`, you can import `TaintFlowTest`. In both cases + * `DefaultFlowConfig` can be replaced by another implementation of `DataFlow::ConfigSig`. + * + * If you need more fine-grained tuning, consider implementing a test using `InlineExpectationsTest`. + */ + +import codeql.swift.dataflow.DataFlow +import codeql.swift.dataflow.ExternalFlow +import codeql.swift.dataflow.TaintTracking +import TestUtilities.InlineExpectationsTest + +private predicate defaultSource(DataFlow::Node source) { + source.asExpr().(CallExpr).getStaticTarget().(Function).getShortName() = ["source", "taint"] +} + +private predicate defaultSink(DataFlow::Node sink) { + exists(CallExpr ca | ca.getStaticTarget().(Function).getShortName() = "sink" | + sink.asExpr() = ca.getAnArgument().getExpr() + ) +} + +module DefaultFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { defaultSource(source) } + + predicate isSink(DataFlow::Node sink) { defaultSink(sink) } + + int fieldFlowBranchLimit() { result = 1000 } +} + +private module NoFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { none() } + + predicate isSink(DataFlow::Node sink) { none() } +} + +private string getSourceArgString(DataFlow::Node src) { + defaultSource(src) and + src.asExpr().(CallExpr).getAnArgument().getExpr().(StringLiteralExpr).getValue() = result +} + +module FlowTest { + module ValueFlow = DataFlow::Global; + + module TaintFlow = TaintTracking::Global; + + private module InlineTest implements TestSig { + string getARelevantTag() { result = ["hasValueFlow", "hasTaintFlow"] } + + predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasValueFlow" and + exists(DataFlow::Node src, DataFlow::Node sink | ValueFlow::flow(src, sink) | + sink.getLocation() = location and + element = sink.toString() and + if exists(getSourceArgString(src)) then value = getSourceArgString(src) else value = "" + ) + or + tag = "hasTaintFlow" and + exists(DataFlow::Node src, DataFlow::Node sink | + TaintFlow::flow(src, sink) and not ValueFlow::flow(src, sink) + | + sink.getLocation() = location and + element = sink.toString() and + if exists(getSourceArgString(src)) then value = getSourceArgString(src) else value = "" + ) + } + } + + import MakeTest + import DataFlow::MergePathGraph + + predicate flowPath(PathNode source, PathNode sink) { + ValueFlow::flowPath(source.asPathNode1(), sink.asPathNode1()) or + TaintFlow::flowPath(source.asPathNode2(), sink.asPathNode2()) + } +} + +module DefaultFlowTest = FlowTest; + +module ValueFlowTest { + import FlowTest +} + +module TaintFlowTest { + import FlowTest +} diff --git a/swift/ql/test/library-tests/dataflow/capture/closures.swift b/swift/ql/test/library-tests/dataflow/capture/closures.swift new file mode 100644 index 000000000000..948a46b9b989 --- /dev/null +++ b/swift/ql/test/library-tests/dataflow/capture/closures.swift @@ -0,0 +1,191 @@ +func sink(_ value: T) { print("sink:", value) } +func source(_ label: String, _ value: T) -> T { return value } +func taint(_ label: String, _ value: T) -> T { return value } + +func hello() -> String { + let value = "Hello world!" + return source("hello", value) +} + +func captureList() { + let y: Int = source("captureList", 123); + { [x = hello()] () in + sink(x) // $ MISSING: hasValueFlow=hello + sink(y) // $ MISSING: hasValueFlow=captureList + }() +} + +func withoutCaptureList() { + let y: Int = source("withoutCaptureList", 124); + { [] () in + sink(y) // $ hasValueFlow=withoutCaptureList + }() +} + +func setAndCallEscape() { + let x = source("setAndCallEscape", 0) + + let escape = { + sink(x) // $ hasValueFlow=setAndCallEscape + return x + 1 + } + + sink(escape()) // $ hasTaintFlow=setAndCallEscape +} + +var escape: (() -> Int)? = nil + +func setEscape() { + let x = source("setEscape", 0) + escape = { + sink(x) // $ MISSING: hasValueFlow=setEscape + return x + 1 + } +} + +func callEscape() { + setEscape() + sink(escape?()) // $ MISSING: hasTaintFlow=setEscape +} + +func logical() -> Bool { + let f: ((Int) -> Int)? = { x in + sink(x) // $ hasValueFlow=logical + return x + 1 + } + + let x: Int? = source("logical", 42) + return f != nil + && (x != nil + && f!(x!) == 43) +} + +func asyncTest() { + func withCallback(_ callback: @escaping (Int) async -> Int) { + @Sendable + func wrapper(_ x: Int) async -> Int { + return await callback(x + 1) // $ MISSING: hasValueFlow=asyncTest + } + Task { + print("asyncTest():", await wrapper(source("asyncTest", 40))) + } + } + withCallback { x in + x + 1 // $ MISSING: hasTaintFlow=asyncTest + } +} + +func foo() { + var x = 1 + let f = { y in x += y } + x = source("foo", 41) + let r = { x } + sink(r()) // $ hasValueFlow=foo + f(1) + sink(r()) // $ hasValueFlow=foo +} + +func sharedCapture() { + let (incrX, getX) = { + var x = source("sharedCapture", 0) + return ({ x += 1 }, { x }) + }() + + let doubleIncrX = { + incrX() + incrX() + } + + sink(getX()) // $ MISSING: hasValueFlow=sharedCapture + doubleIncrX() + sink(getX()) // $ MISSING: hasTaintFlow=sharedCapture +} + +func sharedCaptureMultipleWriters() { + var x = 123 + + let callSink1 = { sink(x) } // $ MISSING: hasValueFlow=setter1 + let callSink2 = { sink(x) } // $ MISSING: hasValueFlow=setter2 + + let makeSetter = { y in + let setter = { x = y } + return setter + } + + let setter1 = makeSetter(source("setter1", 1)) + let setter2 = makeSetter(source("setter2", 2)) + + setter1() + callSink1() + + setter2() + callSink2() +} + +func taintCollections(array: inout Array) { + array[0] = source("array", 0) + sink(array) // $ hasTaintFlow=array + sink(array[0]) // $ hasValueFlow=array + array.withContiguousStorageIfAvailable({ + buffer in + sink(array) // $ hasTaintFlow=array + sink(array[0]) // $ hasValueFlow=array + }) +} + +func simplestTest() { + let x = source("simplestTest", 0) + sink(x) // $ hasValueFlow=simplestTest +} + +func sideEffects() { + var x = 0 + var f = { () in x = source("sideEffects", 1) } + f() + sink(x) // $ hasValueFlow=sideEffects +} + +class S { + var bf1 = 0 + var bf2 = 0 + func captureOther() { + var other = S() + var f = { x in + other.bf1 = x; + }; + + // no flow + sink(bf1); + sink(other.bf1); + sink(other.bf2); + + f(source("captureOther", 2)); + + sink(other.bf1); // $ hasValueFlow=captureOther + sink(other.bf2); + } + + func captureThis() { + var f = { [self] x in + self.bf1 = x; + bf2 = x; + }; + + // no flow + sink(bf1); + sink(self.bf2); + + f(source("captureThis", 2)); + + sink(bf1); // $ MISSING: hasValueFlow + sink(self.bf2); // $ MISSING: hasValueFlow + } +} + +func multi() { + var x = 0 + var y = source("multi", 1) + var f = { () in x = y } + f() + sink(x) // $ hasValueFlow=multi +} \ No newline at end of file diff --git a/swift/ql/test/library-tests/dataflow/capture/inlinetest.expected b/swift/ql/test/library-tests/dataflow/capture/inlinetest.expected new file mode 100644 index 000000000000..8ec8033d086e --- /dev/null +++ b/swift/ql/test/library-tests/dataflow/capture/inlinetest.expected @@ -0,0 +1,2 @@ +testFailures +failures diff --git a/swift/ql/test/library-tests/dataflow/capture/inlinetest.ql b/swift/ql/test/library-tests/dataflow/capture/inlinetest.ql new file mode 100644 index 000000000000..50e3f8d2f7de --- /dev/null +++ b/swift/ql/test/library-tests/dataflow/capture/inlinetest.ql @@ -0,0 +1,2 @@ +import TestUtilities.InlineFlowTest +import DefaultFlowTest diff --git a/swift/ql/test/library-tests/dataflow/dataflow/DataFlowInline.expected b/swift/ql/test/library-tests/dataflow/dataflow/DataFlowInline.expected index 48de9172b362..8ec8033d086e 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/DataFlowInline.expected +++ b/swift/ql/test/library-tests/dataflow/dataflow/DataFlowInline.expected @@ -1,2 +1,2 @@ -failures testFailures +failures diff --git a/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected b/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected index 5ec7f6dc2aad..8615571b864c 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected +++ b/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected @@ -131,12 +131,14 @@ | test.swift:147:9:147:9 | SSA def(lambdaSource) | test.swift:151:15:151:15 | lambdaSource | | test.swift:147:9:147:9 | lambdaSource | test.swift:147:9:147:9 | SSA def(lambdaSource) | | test.swift:147:24:150:5 | { ... } | test.swift:147:9:147:9 | lambdaSource | +| test.swift:151:15:151:15 | [post] lambdaSource | test.swift:159:16:159:16 | lambdaSource | | test.swift:151:15:151:15 | lambdaSource | test.swift:159:16:159:16 | lambdaSource | | test.swift:153:9:153:9 | SSA def(lambdaSink) | test.swift:157:5:157:5 | lambdaSink | | test.swift:153:9:153:9 | lambdaSink | test.swift:153:9:153:9 | SSA def(lambdaSink) | | test.swift:153:22:156:5 | { ... } | test.swift:153:9:153:9 | lambdaSink | | test.swift:154:10:154:13 | SSA def(i) | test.swift:155:19:155:19 | i | | test.swift:154:10:154:13 | i | test.swift:154:10:154:13 | SSA def(i) | +| test.swift:157:5:157:5 | [post] lambdaSink | test.swift:159:5:159:5 | lambdaSink | | test.swift:157:5:157:5 | lambdaSink | test.swift:159:5:159:5 | lambdaSink | | test.swift:162:7:162:7 | SSA def(self) | test.swift:162:7:162:7 | self[return] | | test.swift:162:7:162:7 | self | test.swift:162:7:162:7 | SSA def(self) | @@ -1131,12 +1133,18 @@ | test.swift:888:9:888:9 | SSA def(stream) | test.swift:898:24:898:24 | stream | | test.swift:888:9:888:9 | stream | test.swift:888:9:888:9 | SSA def(stream) | | test.swift:888:18:896:6 | call to AsyncStream.init(_:bufferingPolicy:_:) | test.swift:888:9:888:9 | stream | +| test.swift:889:9:889:9 | continuation | test.swift:890:27:895:13 | continuation | +| test.swift:890:27:895:13 | closure self parameter | test.swift:891:17:891:17 | phi(this) | | test.swift:891:17:891:17 | $generator | test.swift:891:17:891:17 | &... | | test.swift:891:17:891:17 | &... | test.swift:891:17:891:17 | $generator | | test.swift:891:17:891:17 | [post] $generator | test.swift:891:17:891:17 | &... | +| test.swift:891:17:891:17 | phi(this) | test.swift:892:21:892:21 | this | +| test.swift:891:17:891:17 | phi(this) | test.swift:894:17:894:17 | this | | test.swift:891:26:891:26 | $generator | test.swift:891:26:891:26 | SSA def($generator) | | test.swift:891:26:891:26 | SSA def($generator) | test.swift:891:17:891:17 | $generator | | test.swift:891:26:891:30 | call to makeIterator() | test.swift:891:26:891:26 | $generator | +| test.swift:892:21:892:21 | this | test.swift:891:17:891:17 | phi(this) | +| test.swift:892:21:892:21 | this | test.swift:891:17:891:17 | phi(this) | | test.swift:898:5:898:5 | $i$generator | test.swift:898:5:898:5 | &... | | test.swift:898:5:898:5 | &... | test.swift:898:5:898:5 | $i$generator | | test.swift:898:5:898:5 | [post] $i$generator | test.swift:898:5:898:5 | &... | diff --git a/swift/ql/test/library-tests/dataflow/dataflow/test.swift b/swift/ql/test/library-tests/dataflow/dataflow/test.swift index 26dcb621ebb4..735d1490e9c3 100644 --- a/swift/ql/test/library-tests/dataflow/dataflow/test.swift +++ b/swift/ql/test/library-tests/dataflow/dataflow/test.swift @@ -129,7 +129,7 @@ func forwarder() { (i: Int) -> Int in return 0 }) - sink(arg: clean) + sink(arg: clean) // clean } func lambdaFlows() { diff --git a/swift/ql/test/library-tests/dataflow/flowsources/FlowSourcesInline.expected b/swift/ql/test/library-tests/dataflow/flowsources/FlowSourcesInline.expected index 48de9172b362..8ec8033d086e 100644 --- a/swift/ql/test/library-tests/dataflow/flowsources/FlowSourcesInline.expected +++ b/swift/ql/test/library-tests/dataflow/flowsources/FlowSourcesInline.expected @@ -1,2 +1,2 @@ -failures testFailures +failures diff --git a/swift/ql/test/library-tests/dataflow/taint/core/TaintInline.expected b/swift/ql/test/library-tests/dataflow/taint/core/TaintInline.expected index 48de9172b362..8ec8033d086e 100644 --- a/swift/ql/test/library-tests/dataflow/taint/core/TaintInline.expected +++ b/swift/ql/test/library-tests/dataflow/taint/core/TaintInline.expected @@ -1,2 +1,2 @@ -failures testFailures +failures diff --git a/swift/ql/test/library-tests/dataflow/taint/libraries/TaintInline.expected b/swift/ql/test/library-tests/dataflow/taint/libraries/TaintInline.expected index 48de9172b362..8ec8033d086e 100644 --- a/swift/ql/test/library-tests/dataflow/taint/libraries/TaintInline.expected +++ b/swift/ql/test/library-tests/dataflow/taint/libraries/TaintInline.expected @@ -1,2 +1,2 @@ -failures testFailures +failures diff --git a/swift/ql/test/library-tests/dataflow/taint/libraries/int.swift b/swift/ql/test/library-tests/dataflow/taint/libraries/int.swift index 45b6f7515878..3ea65132e870 100644 --- a/swift/ql/test/library-tests/dataflow/taint/libraries/int.swift +++ b/swift/ql/test/library-tests/dataflow/taint/libraries/int.swift @@ -103,8 +103,8 @@ func taintThroughMutablePointer() { ptr4 in let return5 = myArray5.withUnsafeBytes({ ptr5 in - sink(arg: ptr5) - sink(arg: ptr5[0]) // $ MISSING: tainted=97 + sink(arg: ptr5) // $ tainted=97 + sink(arg: ptr5[0]) // $ tainted=97 ptr4.copyBytes(from: ptr5) sink(arg: ptr4) sink(arg: ptr4[0]) // $ MISSING: tainted=97 @@ -146,8 +146,8 @@ func taintCollections(array: inout Array, contiguousArray: inout Contiguous buffer in sink(arg: buffer) // $ tainted=142 sink(arg: buffer[0]) // $ tainted=142 - sink(arg: array) - sink(arg: array[0]) // $ MISSING: tainted=142 + sink(arg: array) // $ tainted=142 + sink(arg: array[0]) // $ tainted=142 }) contiguousArray[0] = source2() diff --git a/swift/ql/test/query-tests/Security/CWE-134/UncontrolledFormatString.expected b/swift/ql/test/query-tests/Security/CWE-134/UncontrolledFormatString.expected index 3a2d4eb80c6a..dd2e31475b42 100644 --- a/swift/ql/test/query-tests/Security/CWE-134/UncontrolledFormatString.expected +++ b/swift/ql/test/query-tests/Security/CWE-134/UncontrolledFormatString.expected @@ -1,4 +1,8 @@ edges +| UncontrolledFormatString.swift:57:12:57:22 | format | UncontrolledFormatString.swift:58:22:60:5 | format | +| UncontrolledFormatString.swift:58:22:60:5 | format | UncontrolledFormatString.swift:58:22:60:5 | { ... } [format] | +| UncontrolledFormatString.swift:58:22:60:5 | { ... } [format] | UncontrolledFormatString.swift:59:16:59:16 | this [format] | +| UncontrolledFormatString.swift:59:16:59:16 | this [format] | UncontrolledFormatString.swift:59:16:59:16 | format | | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | UncontrolledFormatString.swift:70:28:70:28 | tainted | | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | UncontrolledFormatString.swift:73:28:73:28 | tainted | | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | UncontrolledFormatString.swift:74:28:74:28 | tainted | @@ -11,12 +15,19 @@ edges | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | UncontrolledFormatString.swift:84:54:84:54 | tainted | | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | UncontrolledFormatString.swift:85:72:85:72 | tainted | | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | UncontrolledFormatString.swift:88:11:88:11 | tainted | +| UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | UncontrolledFormatString.swift:89:11:89:11 | tainted | | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | UncontrolledFormatString.swift:91:61:91:61 | tainted | | UncontrolledFormatString.swift:81:47:81:47 | tainted | UncontrolledFormatString.swift:81:30:81:54 | call to NSString.init(string:) | | UncontrolledFormatString.swift:82:65:82:65 | tainted | UncontrolledFormatString.swift:82:48:82:72 | call to NSString.init(string:) | | UncontrolledFormatString.swift:84:54:84:54 | tainted | UncontrolledFormatString.swift:84:37:84:61 | call to NSString.init(string:) | | UncontrolledFormatString.swift:85:72:85:72 | tainted | UncontrolledFormatString.swift:85:55:85:79 | call to NSString.init(string:) | +| UncontrolledFormatString.swift:89:11:89:11 | tainted | UncontrolledFormatString.swift:57:12:57:22 | format | nodes +| UncontrolledFormatString.swift:57:12:57:22 | format | semmle.label | format | +| UncontrolledFormatString.swift:58:22:60:5 | format | semmle.label | format | +| UncontrolledFormatString.swift:58:22:60:5 | { ... } [format] | semmle.label | { ... } [format] | +| UncontrolledFormatString.swift:59:16:59:16 | format | semmle.label | format | +| UncontrolledFormatString.swift:59:16:59:16 | this [format] | semmle.label | this [format] | | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | semmle.label | call to String.init(contentsOf:) | | UncontrolledFormatString.swift:70:28:70:28 | tainted | semmle.label | tainted | | UncontrolledFormatString.swift:73:28:73:28 | tainted | semmle.label | tainted | @@ -34,9 +45,11 @@ nodes | UncontrolledFormatString.swift:85:55:85:79 | call to NSString.init(string:) | semmle.label | call to NSString.init(string:) | | UncontrolledFormatString.swift:85:72:85:72 | tainted | semmle.label | tainted | | UncontrolledFormatString.swift:88:11:88:11 | tainted | semmle.label | tainted | +| UncontrolledFormatString.swift:89:11:89:11 | tainted | semmle.label | tainted | | UncontrolledFormatString.swift:91:61:91:61 | tainted | semmle.label | tainted | subpaths #select +| UncontrolledFormatString.swift:59:16:59:16 | format | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | UncontrolledFormatString.swift:59:16:59:16 | format | This format string depends on $@. | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | this user-provided value | | UncontrolledFormatString.swift:70:28:70:28 | tainted | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | UncontrolledFormatString.swift:70:28:70:28 | tainted | This format string depends on $@. | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | this user-provided value | | UncontrolledFormatString.swift:73:28:73:28 | tainted | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | UncontrolledFormatString.swift:73:28:73:28 | tainted | This format string depends on $@. | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | this user-provided value | | UncontrolledFormatString.swift:74:28:74:28 | tainted | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | UncontrolledFormatString.swift:74:28:74:28 | tainted | This format string depends on $@. | UncontrolledFormatString.swift:64:24:64:77 | call to String.init(contentsOf:) | this user-provided value | diff --git a/swift/ql/test/query-tests/Security/CWE-134/UncontrolledFormatString.swift b/swift/ql/test/query-tests/Security/CWE-134/UncontrolledFormatString.swift index e5665eedeac7..05fc1cb25648 100644 --- a/swift/ql/test/query-tests/Security/CWE-134/UncontrolledFormatString.swift +++ b/swift/ql/test/query-tests/Security/CWE-134/UncontrolledFormatString.swift @@ -56,7 +56,7 @@ func getVaList(_ args: [CVarArg]) -> CVaListPointer { return (nil as CVaListPoin func MyLog(_ format: String, _ args: CVarArg...) { withVaList(args) { arglist in - NSLogv(format, arglist) // BAD [NOT DETECTED] + NSLogv(format, arglist) // BAD } }