Merge branch 'master' into python-autoformat-almost-everything

python/ql/src/Security/CWE-022/TarSlip.ql

@@ -121,9 +121,19 @@ class ExtractMembersSink extends TaintSink {
 class TarFileInfoSanitizer extends Sanitizer {
     TarFileInfoSanitizer() { this = "TarInfo sanitizer" }
 
+    /** The test `if <path_sanitizing_test>:` clears taint on its `false` edge. */
     override predicate sanitizingEdge(TaintKind taint, PyEdgeRefinement test) {
-        path_sanitizing_test(test.getTest()) and
-        taint instanceof TarFileInfo
+        taint instanceof TarFileInfo and
+        clears_taint_on_false_edge(test.getTest(), test.getSense())
+    }
+
+    private predicate clears_taint_on_false_edge(ControlFlowNode test, boolean sense) {
+        path_sanitizing_test(test) and
+        sense = false
+        or
+        // handle `not` (also nested)
+        test.(UnaryExprNode).getNode().getOp() instanceof Not and
+        clears_taint_on_false_edge(test.(UnaryExprNode).getOperand(), sense.booleanNot())
     }
 }
 

python/ql/src/Security/CWE-078/CommandInjection.ql

@@ -29,8 +29,7 @@ class CommandInjectionConfiguration extends TaintTracking::Configuration {
     }
 
     override predicate isSink(TaintTracking::Sink sink) {
-        sink instanceof OsCommandFirstArgument or
-        sink instanceof ShellCommand
+        sink instanceof CommandSink
     }
 
     override predicate isExtension(TaintTracking::Extension extension) {

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

@@ -710,33 +710,16 @@ private class EssaTaintTracking extends string {
         TaintTrackingNode src, MultiAssignmentDefinition defn, TaintTrackingContext context,
         AttributePath path, TaintKind kind
     ) {
-        exists(DataFlow::Node srcnode, TaintKind srckind, Assign assign |
+        exists(DataFlow::Node srcnode, TaintKind srckind, Assign assign, int depth |
             src = TTaintTrackingNode_(srcnode, context, path, srckind, this) and
             path.noAttribute()
         |
             assign.getValue().getAFlowNode() = srcnode.asCfgNode() and
-            kind =
-                iterable_unpacking_descent(assign.getATarget().getAFlowNode(), defn.getDefiningNode(),
-                    srckind)
+            depth = iterable_unpacking_descent(assign.getATarget().getAFlowNode(), defn.getDefiningNode()) and
+            kind = taint_at_depth(srckind, depth)
         )
     }
 
-    /** `((x,y), ...) = value` with any nesting on LHS */
-    private TaintKind iterable_unpacking_descent(
-        SequenceNode left_parent, ControlFlowNode left_defn, CollectionKind parent_kind
-    ) {
-        //TODO: Fix the cartesian product in this predicate
-        none() and
-        left_parent.getAnElement() = left_defn and
-        // Handle `a, *b = some_iterable`
-        if left_defn instanceof StarredNode
-        then result = parent_kind
-        else result = parent_kind.getMember()
-        or
-        result =
-            iterable_unpacking_descent(left_parent.getAnElement(), left_defn, parent_kind.getMember())
-    }
-
     pragma[noinline]
     private predicate taintedAttributeAssignment(
         TaintTrackingNode src, AttributeAssignment defn, TaintTrackingContext context,
@@ -966,6 +949,46 @@ private predicate piNodeTestAndUse(PyEdgeRefinement defn, ControlFlowNode test,
     test = defn.getTest() and use = defn.getInput().getASourceUse() and test.getAChild*() = use
 }
 
+/** Helper predicate for taintedMultiAssignment */
+private TaintKind taint_at_depth(SequenceKind parent_kind, int depth) {
+    depth >= 0 and
+    (
+        // base-case #0
+        depth = 0 and
+        result = parent_kind
+        or
+        // base-case #1
+        depth = 1 and
+        result = parent_kind.getMember()
+        or
+        // recursive case
+        depth > 1 and
+        result = taint_at_depth(parent_kind.getMember(), depth-1)
+    )
+}
+
+/** Helper predicate for taintedMultiAssignment
+ *
+ * Returns the `depth` the elements that are assigned to `left_defn` with iterable unpacking has,
+ * compared to `left_parent`. Special care is taken for `StarredNode` that is assigned a sequence of items.
+ *
+ * For example, `((x, *y), ...) = value` with any nesting on LHS
+ * - with `left_defn` = `x`, `left_parent` = `(x, *y)`, result = 1
+ * - with `left_defn` = `x`, `left_parent` = `((x, *y), ...)`, result = 2
+ * - with `left_defn` = `*y`, `left_parent` = `(x, *y)`, result = 0
+ * - with `left_defn` = `*y`, `left_parent` = `((x, *y), ...)`, result = 1
+ */
+int iterable_unpacking_descent(SequenceNode left_parent, ControlFlowNode left_defn) {
+    exists(Assign a | a.getATarget().getASubExpression*().getAFlowNode() = left_parent) and
+    left_parent.getAnElement() = left_defn and
+    // Handle `a, *b = some_iterable`
+    if left_defn instanceof StarredNode
+    then result = 0
+    else result = 1
+    or
+    result = 1 + iterable_unpacking_descent(left_parent.getAnElement(), left_defn)
+}
+
 module Implementation {
     /* A call that returns a copy (or similar) of the argument */
     predicate copyCall(ControlFlowNode fromnode, CallNode tonode) {

python/ql/src/semmle/python/security/injection/Command.qll

@@ -10,6 +10,9 @@ import python
 import semmle.python.security.TaintTracking
 import semmle.python.security.strings.Untrusted
 
+/** Abstract taint sink that is potentially vulnerable to malicious shell commands. */
+abstract class CommandSink extends TaintSink { }
+
 private ModuleObject osOrPopenModule() {
     result.getName() = "os" or
     result.getName() = "popen2"
@@ -47,7 +50,7 @@ class FirstElementFlow extends DataFlowExtension::DataFlowNode {
  * A taint sink that is potentially vulnerable to malicious shell commands.
  * The `vuln` in `subprocess.call(shell=vuln)` and similar calls.
  */
-class ShellCommand extends TaintSink {
+class ShellCommand extends CommandSink {
     override string toString() { result = "shell command" }
 
     ShellCommand() {
@@ -86,7 +89,7 @@ class ShellCommand extends TaintSink {
  * A taint sink that is potentially vulnerable to malicious shell commands.
  * The `vuln` in `subprocess.call(vuln, ...)` and similar calls.
  */
-class OsCommandFirstArgument extends TaintSink {
+class OsCommandFirstArgument extends CommandSink {
     override string toString() { result = "OS command first argument" }
 
     OsCommandFirstArgument() {
@@ -105,3 +108,126 @@ class OsCommandFirstArgument extends TaintSink {
         kind instanceof FirstElementKind
     }
 }
+
+// -------------------------------------------------------------------------- //
+// Modeling of the 'invoke' package and 'fabric' package (v 2.x)
+//
+// Since fabric build so closely upon invoke, we model them together to avoid
+// duplication
+// -------------------------------------------------------------------------- //
+/**
+ * A taint sink that is potentially vulnerable to malicious shell commands.
+ * The `vuln` in `invoke.run(vuln, ...)` and similar calls.
+ */
+class InvokeRun extends CommandSink {
+    InvokeRun() {
+        this = Value::named("invoke.run").(FunctionValue).getArgumentForCall(_, 0)
+        or
+        this = Value::named("invoke.sudo").(FunctionValue).getArgumentForCall(_, 0)
+    }
+
+    override string toString() { result = "InvokeRun" }
+
+    override predicate sinks(TaintKind kind) { kind instanceof ExternalStringKind }
+}
+
+/**
+ * Internal TaintKind to track the invoke.Context instance passed to functions
+ * marked with @invoke.task
+ */
+private class InvokeContextArg extends TaintKind {
+    InvokeContextArg() { this = "InvokeContextArg" }
+}
+
+/** Internal TaintSource to track the context passed to functions marked with @invoke.task */
+private class InvokeContextArgSource extends TaintSource {
+    InvokeContextArgSource() {
+        exists(Function f, Expr decorator |
+            count(f.getADecorator()) = 1 and
+            (
+                decorator = f.getADecorator() and not decorator instanceof Call
+                or
+                decorator = f.getADecorator().(Call).getFunc()
+            ) and
+            (
+                decorator.pointsTo(Value::named("invoke.task"))
+                or
+                decorator.pointsTo(Value::named("fabric.task"))
+            )
+        |
+            this.(ControlFlowNode).getNode() = f.getArg(0)
+        )
+    }
+
+    override predicate isSourceOf(TaintKind kind) { kind instanceof InvokeContextArg }
+}
+
+/**
+ * A taint sink that is potentially vulnerable to malicious shell commands.
+ * The `vuln` in `invoke.Context().run(vuln, ...)` and similar calls.
+ */
+class InvokeContextRun extends CommandSink {
+    InvokeContextRun() {
+        exists(CallNode call |
+            any(InvokeContextArg k).taints(call.getFunction().(AttrNode).getObject("run"))
+            or
+            call = Value::named("invoke.Context").(ClassValue).lookup("run").getACall()
+            or
+            // fabric.connection.Connection is a subtype of invoke.context.Context
+            // since fabric.Connection.run has a decorator, it doesn't work with FunctionValue :|
+            // and `Value::named("fabric.Connection").(ClassValue).lookup("run").getACall()` returned no results,
+            // so here is the hacky solution that works :\
+            call.getFunction().(AttrNode).getObject("run").pointsTo().getClass() =
+                Value::named("fabric.Connection")
+        |
+            this = call.getArg(0)
+            or
+            this = call.getArgByName("command")
+        )
+    }
+
+    override string toString() { result = "InvokeContextRun" }
+
+    override predicate sinks(TaintKind kind) { kind instanceof ExternalStringKind }
+}
+
+/**
+ * A taint sink that is potentially vulnerable to malicious shell commands.
+ * The `vuln` in `fabric.Group().run(vuln, ...)` and similar calls.
+ */
+class FabricGroupRun extends CommandSink {
+    FabricGroupRun() {
+        exists(ClassValue cls |
+            cls.getASuperType() = Value::named("fabric.Group") and
+            this = cls.lookup("run").(FunctionValue).getArgumentForCall(_, 1)
+        )
+    }
+
+    override string toString() { result = "FabricGroupRun" }
+
+    override predicate sinks(TaintKind kind) { kind instanceof ExternalStringKind }
+}
+
+// -------------------------------------------------------------------------- //
+// Modeling of the 'invoke' package and 'fabric' package (v 1.x)
+// -------------------------------------------------------------------------- //
+class FabricV1Commands extends CommandSink {
+    FabricV1Commands() {
+        // since `run` and `sudo` are decorated, we can't use FunctionValue's :(
+        exists(CallNode call |
+            call = Value::named("fabric.api.local").getACall()
+            or
+            call = Value::named("fabric.api.run").getACall()
+            or
+            call = Value::named("fabric.api.sudo").getACall()
+        |
+            this = call.getArg(0)
+            or
+            this = call.getArgByName("command")
+        )
+    }
+
+    override string toString() { result = "FabricV1Commands" }
+
+    override predicate sinks(TaintKind kind) { kind instanceof ExternalStringKind }
+}

python/ql/src/semmle/python/web/bottle/General.qll

@@ -36,7 +36,7 @@ class BottleRoute extends ControlFlowNode {
 
     Function getFunction() { bottle_route(this, _, result) }
 
-    Parameter getNamedArgument() {
+    Parameter getANamedArgument() {
         exists(string name, Function func |
             func = this.getFunction() and
             func.getArgByName(name) = result and

python/ql/src/semmle/python/web/bottle/Request.qll

@@ -71,7 +71,7 @@ class UntrustedFile extends TaintKind {
 /** Parameter to a bottle request handler function */
 class BottleRequestParameter extends HttpRequestTaintSource {
     BottleRequestParameter() {
-        exists(BottleRoute route | route.getNamedArgument() = this.(ControlFlowNode).getNode())
+        exists(BottleRoute route | route.getANamedArgument() = this.(ControlFlowNode).getNode())
     }
 
     override predicate isSourceOf(TaintKind kind) { kind instanceof UntrustedStringKind }

python/ql/src/semmle/python/web/django/General.qll

@@ -2,39 +2,142 @@ import python
 import semmle.python.regex
 import semmle.python.web.Http
 
-predicate django_route(CallNode call, ControlFlowNode regex, FunctionValue view) {
-    exists(FunctionValue url |
-        Value::named("django.conf.urls.url") = url and
-        url.getArgumentForCall(call, 0) = regex and
-        url.getArgumentForCall(call, 1).pointsTo(view)
-    )
-}
-
-class DjangoRouteRegex extends RegexString {
-    DjangoRouteRegex() { django_route(_, this.getAFlowNode(), _) }
-}
-
-class DjangoRoute extends CallNode {
-    DjangoRoute() { django_route(this, _, _) }
-
-    FunctionValue getViewFunction() { django_route(this, _, result) }
-
-    string getNamedArgument() {
-        exists(DjangoRouteRegex regex |
-            django_route(this, regex.getAFlowNode(), _) and
-            regex.getGroupName(_, _) = result
-        )
+// TODO: Since django uses `path = partial(...)`, our analysis doesn't understand this is
+// a FunctionValue, so we can't use `FunctionValue.getArgumentForCall`
+// https://github.com/django/django/blob/master/django/urls/conf.py#L76
+abstract class DjangoRoute extends CallNode {
+    DjangoViewHandler getViewHandler() {
+        result = view_handler_from_view_arg(this.getArg(1))
+        or
+        result = view_handler_from_view_arg(this.getArgByName("view"))
     }
 
+    abstract string getANamedArgument();
+
     /**
      * Get the number of positional arguments that will be passed to the view.
      * Will only return a result if there are no named arguments.
      */
-    int getNumPositionalArguments() {
-        exists(DjangoRouteRegex regex |
-            django_route(this, regex.getAFlowNode(), _) and
-            not exists(string s | s = regex.getGroupName(_, _)) and
+    abstract int getNumPositionalArguments();
+}
+
+/**
+ * For function based views -- also see `DjangoClassBasedViewHandler`
+ * https://docs.djangoproject.com/en/1.11/topics/http/views/
+ * https://docs.djangoproject.com/en/3.0/topics/http/views/
+ */
+class DjangoViewHandler extends PythonFunctionValue {
+
+    /** Gets the index of the 'request' argument */
+    int getRequestArgIndex() {
+        result = 0
+    }
+}
+
+/**
+ * Class based views
+ * https://docs.djangoproject.com/en/1.11/topics/class-based-views/
+ * https://docs.djangoproject.com/en/3.0/topics/class-based-views/
+ */
+private class DjangoViewClass extends ClassValue {
+    DjangoViewClass() {
+        Value::named("django.views.generic.View") = this.getASuperType()
+        or
+        Value::named("django.views.View") = this.getASuperType()
+    }
+}
+
+class DjangoClassBasedViewHandler extends DjangoViewHandler {
+    DjangoClassBasedViewHandler() {
+        exists(DjangoViewClass cls |
+            cls.lookup(httpVerbLower()) = this
+        )
+    }
+
+    override int getRequestArgIndex() {
+        // due to `self` being the first parameter
+        result = 1
+    }
+}
+
+/**
+ * Gets the function that will handle requests when `view_arg` is used as the view argument to a
+ * django route. That is, this methods handles Class-based Views and its `as_view()` function.
+ */
+private DjangoViewHandler view_handler_from_view_arg(ControlFlowNode view_arg) {
+    // Function-based view
+    result = view_arg.pointsTo()
+    or
+    // Class-based view
+    exists(ClassValue cls |
+        cls = view_arg.(CallNode).getFunction().(AttrNode).getObject("as_view").pointsTo() and
+        result = cls.lookup(httpVerbLower())
+    )
+}
+
+// We need this "dummy" class, since otherwise the regex argument would not be considered
+// a regex (RegexString is abstract)
+class DjangoRouteRegex extends RegexString {
+    DjangoRouteRegex() { exists(DjangoRegexRoute route | route.getRouteArg() = this.getAFlowNode()) }
+}
+
+class DjangoRegexRoute extends DjangoRoute {
+    ControlFlowNode route;
+
+    DjangoRegexRoute() {
+        exists(FunctionValue route_maker |
+            // Django 1.x: https://docs.djangoproject.com/en/1.11/ref/urls/#django.conf.urls.url
+            Value::named("django.conf.urls.url") = route_maker and
+            route_maker.getArgumentForCall(this, 0) = route
+        )
+        or
+        // Django 2.x and 3.x: https://docs.djangoproject.com/en/3.0/ref/urls/#re-path
+        this = Value::named("django.urls.re_path").getACall() and
+        (
+            route = this.getArg(0)
+            or
+            route = this.getArgByName("route")
+        )
+    }
+
+    ControlFlowNode getRouteArg() { result = route }
+
+    override string getANamedArgument() {
+        exists(DjangoRouteRegex regex | regex.getAFlowNode() = route |
+            result = regex.getGroupName(_, _)
+        )
+    }
+
+    override int getNumPositionalArguments() {
+        not exists(this.getANamedArgument()) and
+        exists(DjangoRouteRegex regex | regex.getAFlowNode() = route |
             result = count(regex.getGroupNumber(_, _))
         )
     }
 }
+
+class DjangoPathRoute extends DjangoRoute {
+    ControlFlowNode route;
+
+    DjangoPathRoute() {
+        // Django 2.x and 3.x: https://docs.djangoproject.com/en/3.0/ref/urls/#path
+        this = Value::named("django.urls.path").getACall() and
+        (
+            route = this.getArg(0)
+            or
+            route = this.getArgByName("route")
+        )
+    }
+
+    override string getANamedArgument() {
+        // regexp taken from django:
+        // https://github.com/django/django/blob/7d1bf29977bb368d7c28e7c6eb146db3b3009ae7/django/urls/resolvers.py#L199
+        exists(StrConst route_str, string match |
+            route_str = route.getNode() and
+            match = route_str.getText().regexpFind("<(?:(?<converter>[^>:]+):)?(?<parameter>\\w+)>", _, _) and
+            result = match.regexpCapture("<(?:(?<converter>[^>:]+):)?(?<parameter>\\w+)>", 2)
+        )
+    }
+
+    override int getNumPositionalArguments() { none() }
+}

python/ql/src/semmle/python/web/django/Redirect.qll

@@ -17,9 +17,6 @@ class DjangoRedirect extends HttpRedirectTaintSink {
     override string toString() { result = "django.redirect" }
 
     DjangoRedirect() {
-        exists(CallNode call |
-            redirect().getACall() = call and
-            this = call.getAnArg()
-        )
+        this = redirect().getACall().getAnArg()
     }
 }

python/ql/src/semmle/python/web/django/Request.qll

@@ -39,53 +39,35 @@ class DjangoQueryDict extends TaintKind {
     }
 }
 
-abstract class DjangoRequestSource extends HttpRequestTaintSource {
+/** A Django request parameter */
+class DjangoRequestSource extends HttpRequestTaintSource {
+    DjangoRequestSource() {
+        exists(DjangoRoute route, DjangoViewHandler view, int request_arg_index |
+            route.getViewHandler() = view and
+            request_arg_index = view.getRequestArgIndex() and
+            this = view.getScope().getArg(request_arg_index).asName().getAFlowNode()
+        )
+    }
+
     override string toString() { result = "Django request source" }
 
     override predicate isSourceOf(TaintKind kind) { kind instanceof DjangoRequest }
 }
 
-/**
- * Function based views
- * https://docs.djangoproject.com/en/1.11/topics/http/views/
- */
-private class DjangoFunctionBasedViewRequestArgument extends DjangoRequestSource {
-    DjangoFunctionBasedViewRequestArgument() {
-        exists(FunctionValue view |
-            django_route(_, _, view) and
-            this = view.getScope().getArg(0).asName().getAFlowNode()
-        )
-    }
-}
-
-/**
- * Class based views
- * https://docs.djangoproject.com/en/1.11/topics/class-based-views/
- */
-private class DjangoView extends ClassValue {
-    DjangoView() { Value::named("django.views.generic.View") = this.getASuperType() }
-}
-
-private FunctionValue djangoViewHttpMethod() {
-    exists(DjangoView view | view.lookup(httpVerbLower()) = result)
-}
-
-class DjangoClassBasedViewRequestArgument extends DjangoRequestSource {
-    DjangoClassBasedViewRequestArgument() {
-        this = djangoViewHttpMethod().getScope().getArg(1).asName().getAFlowNode()
-    }
-}
-
 /** An argument specified in a url routing table */
 class DjangoRequestParameter extends HttpRequestTaintSource {
     DjangoRequestParameter() {
-        exists(DjangoRoute route, Function f | f = route.getViewFunction().getScope() |
-            this.(ControlFlowNode).getNode() = f.getArgByName(route.getNamedArgument())
+        exists(DjangoRoute route, Function f, DjangoViewHandler view, int request_arg_index |
+            route.getViewHandler() = view and
+            request_arg_index = view.getRequestArgIndex() and
+            f = view.getScope()
+        |
+            this.(ControlFlowNode).getNode() = f.getArgByName(route.getANamedArgument())
             or
             exists(int i | i >= 0 |
                 i < route.getNumPositionalArguments() and
                 // +1 because first argument is always the request
-                this.(ControlFlowNode).getNode() = f.getArg(i + 1)
+                this.(ControlFlowNode).getNode() = f.getArg(request_arg_index + 1 + i)
             )
         )
     }

python/ql/src/semmle/python/web/django/Response.qll

@@ -14,7 +14,15 @@ class DjangoResponse extends TaintKind {
 }
 
 private ClassValue theDjangoHttpResponseClass() {
-    result = Value::named("django.http.response.HttpResponse") and
+    (
+        // version 1.x
+        result = Value::named("django.http.response.HttpResponse")
+        or
+        // version 2.x
+        // https://docs.djangoproject.com/en/2.2/ref/request-response/#httpresponse-objects
+        result = Value::named("django.http.HttpResponse")
+    ) and
+    // TODO: does this do anything? when could they be the same???
     not result = theDjangoHttpRedirectClass()
 }
 

python/ql/src/semmle/python/web/django/Shared.qll

@@ -4,5 +4,9 @@ import python
 FunctionValue redirect() { result = Value::named("django.shortcuts.redirect") }
 
 ClassValue theDjangoHttpRedirectClass() {
+    // version 1.x
     result = Value::named("django.http.response.HttpResponseRedirectBase")
+    or
+    // version 2.x
+    result = Value::named("django.http.HttpResponseRedirectBase")
 }