wip

github · Sep 2, 2024 · 36c92e2 · 36c92e2
1 parent 7326aaf
commit 36c92e2
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 54 deletions.
diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll
@@ -631,7 +631,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
       }
 
       additional predicate sinkNode(NodeEx node, FlowState state) {
-        fwdFlow(node) and
+        fwdFlow(pragma[only_bind_into](node)) and
         fwdFlowState(state) and
         Config::isSink(node.asNodeOrImplicitRead())
         or
@@ -2735,76 +2735,49 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
             abstract PathNodeImpl getASuccessorImpl(string label);
 
             pragma[nomagic]
-            private PathNodeImpl getAnImplicitReadSuccessor0(string label, boolean sink) {
+            PathNodeImpl getAnImplicitReadSuccessorAtSink(string label) {
               exists(PathNodeMid readTarget |
                 result = this.getASuccessorImpl(label) and
                 localStep(this, readTarget, _) and
                 readTarget.getNodeEx().isImplicitReadNode(_)
               |
-                // last implicit read into a sink, leaving the access path empty
-                result = readTarget.projectToSink(_) and
-                sink = true
-                or
-                // last implicit read into a non-sink, leaving the access path empty
-                result = readTarget and
-                readTarget.getAp() instanceof ApNil and
-                sink = false
+                // last implicit read, leaving the access path empty
+                result = readTarget.projectToSink(_)
                 or
                 // implicit read, leaving the access path non-empty
-                exists(result.getAnImplicitReadSuccessor0(_, sink)) and
+                exists(result.getAnImplicitReadSuccessorAtSink(_)) and
                 result = readTarget
               )
             }
 
-            /**
-             * Gets an implicit read successor of this node, where we want to exclude
-             * (compound) edges starting from this node and ending at (or going via)
-             * the result, from the final `edges` relation.
-             *
-             * The only case where we do _not_ want to exclude an edge starting at an
-             * implicit read is when it occurs as the output of a subpath, which does
-             * not immediately end in a sink:
-             *
-             * ```
-             * x.Field = taint;
-             * SinkImplicitFieldRead(TaintStepImplicitFieldRead(FlowThrough(x)));
-             * ```
-             *
-             * taint -> [post update] x [Field]
-             *       -> x [Field]
-             *       -> FlowThrough(x) [Field]
-             *       -> TaintStepImplicitFieldRead(FlowThrough(x))
-             *
-             * In contrast:
-             *
-             * ```
-             * x.Field = taint;
-             * SinkImplicitFieldRead(FlowThrough(x));
-             * ```
-             *
-             * taint -> [post update] x [Field]
-             *       -> x [Field]
-             *       -> FlowThrough(x)
-             */
-            pragma[nomagic]
-            PathNodeImpl getAnImplicitReadSuccessor(string label) {
-              exists(boolean sink | result = this.getAnImplicitReadSuccessor0(label, sink) |
-                not subpathsImpl(_, _, _, this) or
-                sink = true
-              )
-            }
-
             private PathNodeImpl getASuccessorIfHidden(string label) {
               this.isHidden() and
               result = this.getASuccessorImpl(label)
               or
-              result = this.getAnImplicitReadSuccessor(label)
+              result = this.getAnImplicitReadSuccessorAtSink(label)
             }
 
             private PathNodeImpl getASuccessorFromNonHidden(string label) {
               result = this.getASuccessorImpl(label) and
               not this.isHidden() and
-              not result = this.getAnImplicitReadSuccessor(label)
+              // In cases like
+              //
+              // ```
+              // x.Field = taint;
+              // Sink(x);
+              // ```
+              //
+              // we only want the direct edge
+              //
+              //  `[post update] x [Field]` -> `x`
+              //
+              // and not the two edges
+              //
+              //  `[post update] x [Field]` -> `x [Field]`
+              //  `x [Field]`               -> `x`
+              //
+              // which the restriction below ensures.
+              not result = this.getAnImplicitReadSuccessorAtSink(label)
               or
               exists(string l1, string l2 |
                 result = this.getASuccessorFromNonHidden(l1).getASuccessorIfHidden(l2) and
@@ -3285,7 +3258,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
               n2 = localStep(n1) and
               n1.isHidden()
               or
-              n2 = n1.getAnImplicitReadSuccessor(_)
+              n2 = n1.getAnImplicitReadSuccessorAtSink(_)
             }
 
             bindingset[par, ret]

diff --git a/swift/ql/test/query-tests/Security/CWE-094/UnsafeJsEval.expected b/swift/ql/test/query-tests/Security/CWE-094/UnsafeJsEval.expected
@@ -38,15 +38,17 @@ edges
 | UnsafeJsEval.swift:279:13:279:13 | string | UnsafeJsEval.swift:280:26:280:26 | string | provenance |  |
 | UnsafeJsEval.swift:285:13:285:13 | string | UnsafeJsEval.swift:286:3:286:10 | .utf16 | provenance |  |
 | UnsafeJsEval.swift:286:3:286:10 | .utf16 | UnsafeJsEval.swift:286:51:286:51 | stringBytes [Collection element] | provenance |  |
-| UnsafeJsEval.swift:286:51:286:51 | stringBytes [Collection element] | UnsafeJsEval.swift:287:60:287:72 | .baseAddress | provenance | Config |
+| UnsafeJsEval.swift:286:51:286:51 | stringBytes [Collection element] | UnsafeJsEval.swift:287:60:287:60 | stringBytes [Collection element] | provenance |  |
 | UnsafeJsEval.swift:287:16:287:98 | call to JSStringRetain(_:) | UnsafeJsEval.swift:291:17:291:17 | jsstr | provenance |  |
 | UnsafeJsEval.swift:287:31:287:97 | call to JSStringCreateWithCharacters(_:_:) | UnsafeJsEval.swift:287:16:287:98 | call to JSStringRetain(_:) | provenance |  |
+| UnsafeJsEval.swift:287:60:287:60 | stringBytes [Collection element] | UnsafeJsEval.swift:287:60:287:72 | .baseAddress | provenance | Config |
 | UnsafeJsEval.swift:287:60:287:72 | .baseAddress | UnsafeJsEval.swift:287:31:287:97 | call to JSStringCreateWithCharacters(_:_:) | provenance |  |
 | UnsafeJsEval.swift:299:13:299:13 | string | UnsafeJsEval.swift:300:3:300:10 | .utf8CString | provenance |  |
 | UnsafeJsEval.swift:300:3:300:10 | .utf8CString | UnsafeJsEval.swift:300:48:300:48 | stringBytes [Collection element] | provenance |  |
-| UnsafeJsEval.swift:300:48:300:48 | stringBytes [Collection element] | UnsafeJsEval.swift:301:61:301:73 | .baseAddress | provenance | Config |
+| UnsafeJsEval.swift:300:48:300:48 | stringBytes [Collection element] | UnsafeJsEval.swift:301:61:301:61 | stringBytes [Collection element] | provenance |  |
 | UnsafeJsEval.swift:301:16:301:85 | call to JSStringRetain(_:) | UnsafeJsEval.swift:305:17:305:17 | jsstr | provenance |  |
 | UnsafeJsEval.swift:301:31:301:84 | call to JSStringCreateWithUTF8CString(_:) | UnsafeJsEval.swift:301:16:301:85 | call to JSStringRetain(_:) | provenance |  |
+| UnsafeJsEval.swift:301:61:301:61 | stringBytes [Collection element] | UnsafeJsEval.swift:301:61:301:73 | .baseAddress | provenance | Config |
 | UnsafeJsEval.swift:301:61:301:73 | .baseAddress | UnsafeJsEval.swift:301:31:301:84 | call to JSStringCreateWithUTF8CString(_:) | provenance |  |
 | UnsafeJsEval.swift:318:24:318:87 | call to String.init(contentsOf:) | UnsafeJsEval.swift:320:44:320:74 | ... .+(_:_:) ... | provenance |  |
 nodes
@@ -76,13 +78,15 @@ nodes
 | UnsafeJsEval.swift:286:51:286:51 | stringBytes [Collection element] | semmle.label | stringBytes [Collection element] |
 | UnsafeJsEval.swift:287:16:287:98 | call to JSStringRetain(_:) | semmle.label | call to JSStringRetain(_:) |
 | UnsafeJsEval.swift:287:31:287:97 | call to JSStringCreateWithCharacters(_:_:) | semmle.label | call to JSStringCreateWithCharacters(_:_:) |
+| UnsafeJsEval.swift:287:60:287:60 | stringBytes [Collection element] | semmle.label | stringBytes [Collection element] |
 | UnsafeJsEval.swift:287:60:287:72 | .baseAddress | semmle.label | .baseAddress |
 | UnsafeJsEval.swift:291:17:291:17 | jsstr | semmle.label | jsstr |
 | UnsafeJsEval.swift:299:13:299:13 | string | semmle.label | string |
 | UnsafeJsEval.swift:300:3:300:10 | .utf8CString | semmle.label | .utf8CString |
 | UnsafeJsEval.swift:300:48:300:48 | stringBytes [Collection element] | semmle.label | stringBytes [Collection element] |
 | UnsafeJsEval.swift:301:16:301:85 | call to JSStringRetain(_:) | semmle.label | call to JSStringRetain(_:) |
 | UnsafeJsEval.swift:301:31:301:84 | call to JSStringCreateWithUTF8CString(_:) | semmle.label | call to JSStringCreateWithUTF8CString(_:) |
+| UnsafeJsEval.swift:301:61:301:61 | stringBytes [Collection element] | semmle.label | stringBytes [Collection element] |
 | UnsafeJsEval.swift:301:61:301:73 | .baseAddress | semmle.label | .baseAddress |
 | UnsafeJsEval.swift:305:17:305:17 | jsstr | semmle.label | jsstr |
 | UnsafeJsEval.swift:318:24:318:87 | call to String.init(contentsOf:) | semmle.label | call to String.init(contentsOf:) |