Python taint-tracking: Avoid ambiguous flows through calls. Fix up tests.

python/ql/src/Security/CWE-209/StackTraceExposure.ql

@@ -33,3 +33,6 @@ class StackTraceExposureConfiguration extends TaintTracking::Configuration {
 from StackTraceExposureConfiguration config, TaintedPathSource src, TaintedPathSink sink
 where config.hasFlowPath(src, sink)
 select sink.getSink(), src, sink, "$@ may be exposed to an external user", src.getSource(), "Error information"
+
+
+

python/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql

@@ -16,7 +16,9 @@ class BrokenCryptoConfiguration extends TaintTracking::Configuration {
 
     BrokenCryptoConfiguration() { this = "Broken crypto configuration" }
 
-    override predicate isSource(TaintTracking::Source source) { source instanceof SensitiveDataSource }
+    override predicate isSource(TaintTracking::Source source) { 
+        source instanceof SensitiveDataSource
+    }
 
     override predicate isSink(TaintTracking::Sink sink) {
         sink instanceof WeakCryptoSink

python/ql/src/semmle/python/dataflow/Implementation.qll

@@ -2,6 +2,7 @@ import python
 import semmle.python.security.TaintTracking
 private import semmle.python.objects.ObjectInternal
 private import semmle.python.pointsto.Filters as Filters
+private import semmle.python.dataflow.Presentation
 
 newtype TTaintTrackingContext =
     TNoParam()
@@ -175,24 +176,6 @@ class TaintTrackingNode extends TTaintTrackingNode {
         result = this.getNode().getLocation()
     }
 
-    TaintTrackingNode getASuccessor() {
-        result = this.getASuccessor(_)
-    }
-
-    TaintTrackingNode getASuccessor(string edgeLabel) {
-        this.isVisible() and
-        result = this.getAnUnlabeledSuccessor*().getALabeledSuccessor(edgeLabel)
-    }
-
-    private TaintTrackingNode getAnUnlabeledSuccessor() {
-        this.getConfiguration().(TaintTrackingImplementation).flowStep(this, result, "")
-    }
-
-    private TaintTrackingNode getALabeledSuccessor(string label) {
-        not label = "" and
-        this.getConfiguration().(TaintTrackingImplementation).flowStep(this, result, label)
-    }
-
     predicate isSource() {
         this.getConfiguration().(TaintTrackingImplementation).isPathSource(this)
     }
@@ -210,13 +193,21 @@ class TaintTrackingNode extends TTaintTrackingNode {
         result = this.getCfgNode().getNode()
     }
 
-    /** Holds if this node should be presented to the user as part of a path */
-    predicate isVisible() {
-        this.isSource() or
-        exists(string label |
-            not label = "" |
-            this.getConfiguration().(TaintTrackingImplementation).flowStep(_, this, label)
-        )
+    TaintTrackingNode getASuccessor(string edgeLabel) {
+        result = this.unlabeledSuccessor*().labeledSuccessor(edgeLabel)
+    }
+
+    TaintTrackingNode getASuccessor() {
+        result = this.getASuccessor(_)
+    }
+
+    private TaintTrackingNode unlabeledSuccessor() {
+        this.getConfiguration().(TaintTrackingImplementation).flowStep(this, result, "")
+    }
+
+    private TaintTrackingNode labeledSuccessor(string label) {
+        not label = "" and
+        this.getConfiguration().(TaintTrackingImplementation).flowStep(this, result, label)
     }
 
 }
@@ -231,7 +222,7 @@ class TaintTrackingImplementation extends string {
     predicate hasFlowPath(TaintTrackingNode source, TaintTrackingNode sink) {
         this.isPathSource(source) and
         this.isPathSink(sink) and
-        sink = source.getASuccessor*()
+        this.flowReaches(source, sink)
     }
 
     predicate flowSource(DataFlow::Node node, TaintTrackingContext context, AttributePath path, TaintKind kind) {
@@ -282,6 +273,15 @@ class TaintTrackingImplementation extends string {
         )
     }
 
+    predicate flowReaches(TaintTrackingNode src, TaintTrackingNode dest) {
+        this = src.getConfiguration() and dest = src
+        or
+        exists(TaintTrackingNode mid |
+            this.flowReaches(src, mid) and
+            this.flowStep(mid, dest, _)
+        )
+    }
+
     predicate flowBarrier(DataFlow::Node node, TaintKind kind) {
         this.(TaintTracking::Configuration).isBarrier(node, kind)
         or
@@ -397,6 +397,8 @@ class TaintTrackingImplementation extends string {
         or
         this.returnFlowStep(src, node, context, path, kind, edgeLabel)
         or
+        this.callFlowStep(src, node, context, path, kind, edgeLabel)
+        or
         this.iterationStep(src, node, context, path, kind, edgeLabel)
         or
         this.yieldStep(src, node, context, path, kind, edgeLabel)
@@ -508,27 +510,39 @@ class TaintTrackingImplementation extends string {
     //    // TO DO... named parameters
     //}
 
+    /* If the return value is tainted without context, then it always flows back to the caller */
     pragma [noinline]
     predicate returnFlowStep(TaintTrackingNode src, DataFlow::Node node, TaintTrackingContext context, AttributePath path, TaintKind kind, string edgeLabel) {
-        exists(CallNode call, PythonFunctionObjectInternal pyfunc, TaintTrackingContext callee, DataFlow::Node retval |
-            this.callContexts(call, pyfunc, context, callee) and
-            src = TTaintTrackingNode_(retval, callee, path, kind, this) and
+        exists(CallNode call, PythonFunctionObjectInternal pyfunc, DataFlow::Node retval |
+            pyfunc.getACall() = call and
+            context = TNoParam() and
+            src = TTaintTrackingNode_(retval, TNoParam(), path, kind, this) and
             node.asCfgNode() = call and
             retval.asCfgNode() = any(Return ret | ret.getScope() = pyfunc.getScope()).getValue().getAFlowNode()
         ) and
         edgeLabel = "return"
     }
 
+    /* Avoid taint flow from return value to caller as it can produce imprecise flow graphs
+     * Step directly from tainted argument to call result.
+     */
     pragma [noinline]
-    predicate callContexts(CallNode call, PythonFunctionObjectInternal pyfunc, TaintTrackingContext caller, TaintTrackingContext callee) {
+    predicate callFlowStep(TaintTrackingNode src, DataFlow::Node node, TaintTrackingContext context, AttributePath path, TaintKind kind, string edgeLabel) {
+        exists(CallNode call, PythonFunctionObjectInternal pyfunc, TaintTrackingContext callee, DataFlow::Node retval, TaintTrackingNode retnode |
+            this.callContexts(call, src, pyfunc, context, callee) and
+            retnode = TTaintTrackingNode_(retval, callee, path, kind, this) and
+            node.asCfgNode() = call and
+            retval.asCfgNode() = any(Return ret | ret.getScope() = pyfunc.getScope()).getValue().getAFlowNode()
+        ) and
+        edgeLabel = "call"
+    }
+
+    pragma [noinline]
+    predicate callContexts(CallNode call, TaintTrackingNode argnode, PythonFunctionObjectInternal pyfunc, TaintTrackingContext caller, TaintTrackingContext callee) {
         exists(int arg, TaintKind callerKind, AttributePath callerPath |
-            this.callWithTaintedArgument(_, call, caller, pyfunc, arg, callerPath, callerKind) and
+            this.callWithTaintedArgument(argnode, call, caller, pyfunc, arg, callerPath, callerKind) and
             callee = TParamContext(callerKind, callerPath, arg)
         )
-        or
-        pyfunc.getACall() = call and
-        callee = TNoParam() and
-        caller = TNoParam()
     }
 
     predicate callWithTaintedArgument(TaintTrackingNode src, CallNode call, TaintTrackingContext caller, CallableValue pyfunc, int arg, AttributePath path, TaintKind kind) {
@@ -633,6 +647,8 @@ class TaintTrackingImplementation extends string {
         this.taintedArgument(src, defn, context, path, kind)
         or
         this.taintedExceptionCapture(src, defn, context, path, kind)
+        or
+        this.taintedScopeEntryDefinition(src, defn, context, path, kind)
     }
 
     pragma [noinline]
@@ -729,6 +745,14 @@ class TaintTrackingImplementation extends string {
         )
     }
 
+    pragma [noinline]
+    predicate taintedScopeEntryDefinition(TaintTrackingNode src, ScopeEntryDefinition defn, TaintTrackingContext context, AttributePath path, TaintKind kind) {
+        exists(EssaVariable var |
+            BaseFlow::scope_entry_value_transfer_from_earlier(var, _, defn, _) and
+            this.taintedDefinition(src, var.getDefinition(), context, path, kind)
+        )
+    }
+
     predicate moduleAttributeTainted(ModuleValue m, string name, TaintTrackingNode taint) {
         exists(DataFlow::Node srcnode, EssaVariable var |
             taint = TTaintTrackingNode_(srcnode, TNoParam(), _, _, this) and
@@ -769,6 +793,12 @@ class TaintTrackingImplementation extends string {
         )
     }
 
+    predicate crossCallFlow(TaintTrackingNode taintedArg, TaintTrackingNode call) {
+        this.parameterStep(taintedArg, _, _, _, _, _) and
+        this.flowReaches(taintedArg, call) and
+        call.getNode().asCfgNode().(CallNode).getArg(_) = taintedArg.getNode().asCfgNode()
+    }
+
 }
 
 /* Backwards compatibility with config-less taint-tracking */

python/ql/src/semmle/python/security/Crypto.qll

@@ -192,4 +192,16 @@ module Cryptography {
 
 }
 
+private class CipherConfig extends TaintTracking::Configuration {
+
+    CipherConfig() { this = "Crypto cipher config" }
+
+    override predicate isSource(TaintTracking::Source source) {
+        source instanceof Pycrypto::CipherInstanceSource
+        or
+        source instanceof Cryptography::CipherSource
+    }
+
+}
+
 

python/ql/src/semmle/python/security/TaintTracking.qll

@@ -431,7 +431,7 @@ abstract class TaintSource extends @py_flow_node {
         exists(TaintedNode src, TaintedNode tsink |
             src = this.getATaintNode() and
             src.getTaintKind() = srckind and
-            src.getASuccessor*() = tsink and
+            src.getConfiguration().(TaintTrackingImplementation).flowReaches(src, tsink) and
             this.isSourceOf(srckind, _) and
             sink = tsink.getCfgNode() and
             sink.sinks(tsink.getTaintKind()) and
@@ -624,7 +624,7 @@ class TaintedPathSource extends TaintTrackingNode {
 
     /** Holds if taint can flow from this source to sink `sink` */
     final predicate flowsTo(TaintedPathSink sink) {
-        this.getASuccessor*() = sink
+        this.getConfiguration().(TaintTrackingImplementation).flowReaches(this, sink)
     }
 
     DataFlow::Node getSource() {
@@ -676,7 +676,7 @@ module DataFlow {
         private predicate hasFlowPath(TaintedNode source, TaintedNode sink) {
             this.isSource(source.getCfgNode()) and
             this.isSink(sink.getCfgNode()) and
-            source.getASuccessor*() = sink
+            source.getConfiguration().(TaintTrackingImplementation).flowReaches(source, sink)
         }
 
         predicate hasFlow(ControlFlowNode source, ControlFlowNode sink) {

python/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected

@@ -1,9 +1,6 @@
 edges
 | test.py:33:15:33:36 | exception info | test.py:34:29:34:31 | exception info |
-| test.py:34:29:34:31 | exception info | test.py:36:18:36:20 | exception info |
-| test.py:36:18:36:20 | exception info | test.py:37:25:37:27 | exception info |
-| test.py:37:12:37:27 | exception info | test.py:34:16:34:32 | exception info |
-| test.py:37:25:37:27 | exception info | test.py:37:12:37:27 | exception info |
+| test.py:34:29:34:31 | exception info | test.py:34:16:34:32 | exception info |
 #select
 | test.py:16:16:16:37 | Attribute() | test.py:16:16:16:37 | exception info | test.py:16:16:16:37 | exception info | $@ may be exposed to an external user | test.py:16:16:16:37 | Attribute() | Error information |
 | test.py:34:16:34:32 | format_error() | test.py:33:15:33:36 | exception info | test.py:34:16:34:32 | exception info | $@ may be exposed to an external user | test.py:33:15:33:36 | Attribute() | Error information |

python/ql/test/query-tests/Security/CWE-327/TestNode.expected

@@ -6,9 +6,3 @@ WARNING: Predicate getNode has been deprecated and may be removed in future (Tes
 | Taint cryptography.encryptor.RC4 | test_cryptography.py:6:17:6:34 | test_cryptography.py:6 | test_cryptography.py:6:17:6:34 | Attribute() |  |
 | Taint cryptography.encryptor.RC4 | test_cryptography.py:7:12:7:20 | test_cryptography.py:7 | test_cryptography.py:7:12:7:20 | encryptor |  |
 | Taint cryptography.encryptor.RC4 | test_cryptography.py:7:42:7:50 | test_cryptography.py:7 | test_cryptography.py:7:42:7:50 | encryptor |  |
-| Taint sensitive.data | test_cryptography.py:4:17:4:28 | test_cryptography.py:4 | test_cryptography.py:4:17:4:28 | get_password |  |
-| Taint sensitive.data | test_cryptography.py:4:17:4:30 | test_cryptography.py:4 | test_cryptography.py:4:17:4:30 | get_password() |  |
-| Taint sensitive.data | test_cryptography.py:7:29:7:37 | test_cryptography.py:7 | test_cryptography.py:7:29:7:37 | dangerous |  |
-| Taint sensitive.data | test_pycrypto.py:4:17:4:28 | test_pycrypto.py:4 | test_pycrypto.py:4:17:4:28 | get_password |  |
-| Taint sensitive.data | test_pycrypto.py:4:17:4:30 | test_pycrypto.py:4 | test_pycrypto.py:4:17:4:30 | get_password() |  |
-| Taint sensitive.data | test_pycrypto.py:6:27:6:35 | test_pycrypto.py:6 | test_pycrypto.py:6:27:6:35 | dangerous |  |

python/ql/test/query-tests/Security/CWE-601/UrlRedirect.expected

@@ -1,7 +1,5 @@
 edges
 | test.py:7:22:7:33 | dict of externally controlled string | test.py:7:22:7:51 | externally controlled string |
 | test.py:7:22:7:51 | externally controlled string | test.py:8:21:8:26 | externally controlled string |
-| test.py:15:17:15:28 | dict of externally controlled string | test.py:15:17:15:42 | externally controlled string |
-| test.py:15:17:15:42 | externally controlled string | test.py:17:13:17:21 | externally controlled string |
 #select
-| test.py:8:21:8:26 | flask.redirect | test.py:7:22:7:33 | dict of externally controlled string | test.py:8:21:8:26 | externally controlled string | Untrusted URL redirection due to $@. | test.py:7:22:7:33 | flask.request.args | a user-provided value |
+| test.py:8:21:8:26 | target | test.py:7:22:7:33 | dict of externally controlled string | test.py:8:21:8:26 | externally controlled string | Untrusted URL redirection due to $@. | test.py:7:22:7:33 | Attribute | a user-provided value |