Merge pull request github#10967 from MathiasVP/fix-swift-summary

MathiasVP · web-flow · commit 142e50008e6f · 2022-10-28T12:57:52.000+02:00
Swift: Fix flow out of summarized callables
diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowDispatch.qll b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowDispatch.qll
@@ -53,17 +53,19 @@ class ParamReturnKind extends ReturnKind, TParamReturnKind {
  * defined in library code.
  */
 class DataFlowCallable extends TDataFlowCallable {
-  CfgScope scope;
-
-  DataFlowCallable() { this = TDataFlowFunc(scope) }
+  /** Gets the location of this callable. */
+  Location getLocation() { none() } // overridden in subclasses
 
   /** Gets a textual representation of this callable. */
-  string toString() { result = scope.toString() }
+  string toString() { none() } // overridden in subclasses
 
-  /** Gets the location of this callable. */
-  Location getLocation() { result = scope.getLocation() }
+  CfgScope asSourceCallable() { this = TDataFlowFunc(result) }
+
+  FlowSummary::SummarizedCallable asSummarizedCallable() { this = TSummarizedCallable(result) }
 
-  Callable::TypeRange getUnderlyingCallable() { result = scope }
+  Callable::TypeRange getUnderlyingCallable() {
+    result = this.asSummarizedCallable() or result = this.asSourceCallable()
+  }
 }
 
 cached
@@ -230,7 +232,7 @@ cached
 private module Cached {
   cached
   newtype TDataFlowCallable =
-    TDataFlowFunc(CfgScope scope) or
+    TDataFlowFunc(CfgScope scope) { not scope instanceof FlowSummary::SummarizedCallable } or
     TSummarizedCallable(FlowSummary::SummarizedCallable c)
 
   /** Gets a viable run-time target for the call `call`. */
@@ -247,6 +249,26 @@ private module Cached {
     result = TSummarizedCallable(call.asCall().getStaticTarget())
   }
 
+  private class SourceCallable extends DataFlowCallable, TDataFlowFunc {
+    CfgScope scope;
+
+    SourceCallable() { this = TDataFlowFunc(scope) }
+
+    override string toString() { result = scope.toString() }
+
+    override Location getLocation() { result = scope.getLocation() }
+  }
+
+  private class SummarizedCallable extends DataFlowCallable, TSummarizedCallable {
+    FlowSummary::SummarizedCallable sc;
+
+    SummarizedCallable() { this = TSummarizedCallable(sc) }
+
+    override string toString() { result = sc.toString() }
+
+    override Location getLocation() { result = sc.getLocation() }
+  }
+
   cached
   newtype TArgumentPosition =
     TThisArgument() or
diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll
@@ -269,7 +269,7 @@ class SummaryNode extends NodeImpl, TSummaryNode {
 
   SummaryNode() { this = TSummaryNode(c, state) }
 
-  override DataFlowCallable getEnclosingCallable() { result = TDataFlowFunc(c) }
+  override DataFlowCallable getEnclosingCallable() { result.asSummarizedCallable() = c }
 
   override UnknownLocation getLocationImpl() { any() }
 
@@ -430,6 +430,8 @@ private module OutNodes {
   }
 
   class SummaryOutNode extends OutNode, SummaryNode {
+    SummaryOutNode() { FlowSummaryImpl::Private::summaryOutNode(_, this, _) }
+
     override DataFlowCall getCall(ReturnKind kind) {
       FlowSummaryImpl::Private::summaryOutNode(result, this, kind)
     }
diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll
@@ -116,7 +116,7 @@ ExprNode exprNode(DataFlowExpr e) { result.asExpr() = e }
 /**
  * Gets the node corresponding to the value of parameter `p` at function entry.
  */
-ParameterNode parameterNode(DataFlowParameter p) { none() }
+ParameterNode parameterNode(DataFlowParameter p) { result.getParameter() = p }
 
 /**
  * Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local
diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/FlowSummaryImplSpecific.qll b/swift/ql/lib/codeql/swift/dataflow/internal/FlowSummaryImplSpecific.qll
@@ -15,7 +15,7 @@ private import codeql.swift.controlflow.CfgNodes
 
 class SummarizedCallableBase = AbstractFunctionDecl;
 
-DataFlowCallable inject(SummarizedCallable c) { result = TDataFlowFunc(c) }
+DataFlowCallable inject(SummarizedCallable c) { result.getUnderlyingCallable() = c }
 
 /** Gets the parameter position of the instance parameter. */
 ArgumentPosition instanceParameterPosition() { result instanceof ThisArgumentPosition }
diff --git a/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected b/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected
@@ -1,4 +1,5 @@
 edges
+| file://:0:0:0:0 | [summary param] this in signum() :  | file://:0:0:0:0 | [summary] to write: return (return) in signum() :  |
 | file://:0:0:0:0 | self [a, x] :  | file://:0:0:0:0 | .a [x] :  |
 | file://:0:0:0:0 | self [x] :  | file://:0:0:0:0 | .x :  |
 | file://:0:0:0:0 | value :  | file://:0:0:0:0 | [post] self [x] :  |
@@ -100,10 +101,18 @@ edges
 | test.swift:225:14:225:21 | call to source() :  | test.swift:238:13:238:15 | .source_value |
 | test.swift:259:12:259:19 | call to source() :  | test.swift:263:13:263:28 | call to optionalSource() :  |
 | test.swift:263:13:263:28 | call to optionalSource() :  | test.swift:264:15:264:16 | ...! |
+| test.swift:263:13:263:28 | call to optionalSource() :  | test.swift:266:15:266:16 | ...? :  |
+| test.swift:265:15:265:22 | call to source() :  | file://:0:0:0:0 | [summary param] this in signum() :  |
+| test.swift:265:15:265:22 | call to source() :  | test.swift:265:15:265:31 | call to signum() |
+| 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 |
 nodes
 | file://:0:0:0:0 | .a [x] :  | semmle.label | .a [x] :  |
 | file://:0:0:0:0 | .x :  | semmle.label | .x :  |
 | file://:0:0:0:0 | [post] self [x] :  | semmle.label | [post] self [x] :  |
+| file://:0:0:0:0 | [summary param] this in signum() :  | semmle.label | [summary param] this in signum() :  |
+| file://:0:0:0:0 | [summary] to write: return (return) in signum() :  | semmle.label | [summary] to write: return (return) in signum() :  |
 | file://:0:0:0:0 | self [a, x] :  | semmle.label | self [a, x] :  |
 | file://:0:0:0:0 | self [x] :  | semmle.label | self [x] :  |
 | file://:0:0:0:0 | value :  | semmle.label | value :  |
@@ -213,6 +222,11 @@ nodes
 | test.swift:259:12:259:19 | call to source() :  | semmle.label | call to source() :  |
 | test.swift:263:13:263:28 | call to optionalSource() :  | semmle.label | call to optionalSource() :  |
 | test.swift:264:15:264:16 | ...! | semmle.label | ...! |
+| test.swift:265:15:265:22 | call to source() :  | semmle.label | call to source() :  |
+| test.swift:265:15:265:31 | call to signum() | semmle.label | call to signum() |
+| 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() :  |
 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 ... :  |
@@ -239,6 +253,8 @@ subpaths
 | test.swift:218:11:218:18 | call to source() :  | test.swift:169:12:169:22 | value :  | test.swift:170:5:170:5 | [post] self [x] :  | test.swift:218:3:218:5 | [post] getter for .a [x] :  |
 | test.swift:219:13:219:13 | b [a, x] :  | test.swift:185:7:185:7 | self [a, x] :  | file://:0:0:0:0 | .a [x] :  | test.swift:219:13:219:15 | .a [x] :  |
 | test.swift:219:13:219:15 | .a [x] :  | test.swift:163:7:163:7 | self [x] :  | file://:0:0:0:0 | .x :  | test.swift:219:13:219:17 | .x |
+| test.swift:265:15:265:22 | call to source() :  | file://:0:0:0:0 | [summary param] this in signum() :  | file://:0:0:0:0 | [summary] to write: return (return) in signum() :  | test.swift:265:15:265:31 | call to signum() |
+| test.swift:266:15:266:16 | ...? :  | file://:0:0:0:0 | [summary param] this in signum() :  | file://:0:0:0:0 | [summary] to write: return (return) in signum() :  | test.swift:266:15:266:25 | call to signum() :  |
 #select
 | test.swift:7:15:7:15 | t1 | test.swift:6:19:6:26 | call to source() :  | test.swift:7:15:7:15 | t1 | result |
 | test.swift:9:15:9:15 | t1 | test.swift:6:19:6:26 | call to source() :  | test.swift:9:15:9:15 | t1 | result |
@@ -269,3 +285,5 @@ subpaths
 | test.swift:235:13:235:15 | .source_value | test.swift:225:14:225:21 | call to source() :  | test.swift:235:13:235:15 | .source_value | result |
 | test.swift:238:13:238:15 | .source_value | test.swift:225:14:225:21 | call to source() :  | test.swift:238:13:238:15 | .source_value | result |
 | 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 |
diff --git a/swift/ql/test/library-tests/dataflow/dataflow/test.swift b/swift/ql/test/library-tests/dataflow/dataflow/test.swift
@@ -262,8 +262,8 @@ func optionalSource() -> Int? {
 func test_optionals() {
     let x = optionalSource()
     sink(arg: x!) // $ flow=259
-    sink(arg: source().signum()) // $ MISSING: flow=259
-    sink(opt: x?.signum()) // $ MISSING: flow=259
+    sink(arg: source().signum()) // $ flow=265
+    sink(opt: x?.signum()) // $ flow=259
     sink(arg: x ?? 0) // $ MISSING: flow=259
     if let y = x {
         sink(arg: y) // $ MISSING: flow=259
diff --git a/swift/ql/test/library-tests/dataflow/taint/Taint.expected b/swift/ql/test/library-tests/dataflow/taint/Taint.expected
diff --git a/swift/ql/test/query-tests/Security/CWE-079/UnsafeWebViewFetch.expected b/swift/ql/test/query-tests/Security/CWE-079/UnsafeWebViewFetch.expected

Original file line number	Diff line number	Diff line change
`@@ -269,7 +269,7 @@ class SummaryNode extends NodeImpl, TSummaryNode {`
`269`	`269`
`270`	`270`	`SummaryNode() { this = TSummaryNode(c, state) }`
`271`	`271`
`272`		`- override DataFlowCallable getEnclosingCallable() { result = TDataFlowFunc(c) }`
	`272`	`+ override DataFlowCallable getEnclosingCallable() { result.asSummarizedCallable() = c }`
`273`	`273`
`274`	`274`	`override UnknownLocation getLocationImpl() { any() }`
`275`	`275`
`@@ -430,6 +430,8 @@ private module OutNodes {`
`430`	`430`	`}`
`431`	`431`
`432`	`432`	`class SummaryOutNode extends OutNode, SummaryNode {`
	`433`	`+ SummaryOutNode() { FlowSummaryImpl::Private::summaryOutNode(_, this, _) }`
	`434`	`+`
`433`	`435`	`override DataFlowCall getCall(ReturnKind kind) {`
`434`	`436`	`FlowSummaryImpl::Private::summaryOutNode(result, this, kind)`
`435`	`437`	`}`