Merge pull request github#3591 from RasmusWL/python-taintkind-fixup

Python: Fix some problems in TaintKind useage

Author: yoff

change-notes/1.25/analysis-python.md

@@ -0,0 +1,22 @@
+# Improvements to Python analysis
+
+The following changes in version 1.25 affect Python analysis in all applications.
+
+## General improvements
+
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+
+
+## Changes to existing queries
+
+| **Query**                  | **Expected impact**    | **Change**                                                       |
+|----------------------------|------------------------|------------------------------------------------------------------|
+
+
+## Changes to libraries
+
+* Importing `semmle.python.web.HttpRequest` will no longer import `UntrustedStringKind` transitively. `UntrustedStringKind` is the most commonly used non-abstract subclass of `ExternalStringKind`. If not imported (by one mean or another), taint-tracking queries that concern `ExternalStringKind` will not produce any results. Please ensure such queries contain an explicit import (`import semmle.python.security.strings.Untrusted`).

python/ql/src/semmle/python/security/strings/Basic.qll

@@ -107,7 +107,11 @@ private predicate os_path_join(ControlFlowNode fromnode, CallNode tonode) {
     tonode.getAnArg() = fromnode
 }
 
-/** A kind of "taint", representing a dictionary mapping str->"taint" */
-class StringDictKind extends DictKind {
+/**
+ * A kind of "taint", representing a dictionary mapping str->"taint"
+ *
+ * DEPRECATED: Use `ExternalStringDictKind` instead.
+ */
+deprecated class StringDictKind extends DictKind {
     StringDictKind() { this.getValue() instanceof StringKind }
 }

python/ql/src/semmle/python/security/strings/External.qll

@@ -60,14 +60,14 @@ class ExternalJsonKind extends TaintKind {
     }
 }
 
-/** A kind of "taint", representing a dictionary mapping str->"taint" */
+/** A kind of "taint", representing a dictionary mapping keys to tainted strings. */
 class ExternalStringDictKind extends DictKind {
     ExternalStringDictKind() { this.getValue() instanceof ExternalStringKind }
 }
 
 /**
- * A kind of "taint", representing a dictionary mapping strings to sequences of
- *  tainted strings
+ * A kind of "taint", representing a dictionary mapping keys to sequences of
+ *  tainted strings.
  */
 class ExternalStringSequenceDictKind extends DictKind {
     ExternalStringSequenceDictKind() { this.getValue() instanceof ExternalStringSequenceKind }

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

@@ -1,6 +1,6 @@
 import python
 import semmle.python.dataflow.TaintTracking
-import semmle.python.security.strings.Untrusted
+import semmle.python.security.strings.External
 import semmle.python.web.Http
 import semmle.python.web.bottle.General
 
@@ -13,7 +13,7 @@ class BottleRequestKind extends TaintKind {
         result instanceof BottleFormsDict and
         (name = "cookies" or name = "query" or name = "form")
         or
-        result instanceof UntrustedStringKind and
+        result instanceof ExternalStringKind and
         (name = "query_string" or name = "url_args")
         or
         result.(DictKind).getValue() instanceof FileUpload and
@@ -34,27 +34,27 @@ class BottleFormsDict extends TaintKind {
         /* Cannot use `getTaintOfAttribute(name)` as it wouldn't bind `name` */
         exists(string name |
             fromnode = tonode.(AttrNode).getObject(name) and
-            result instanceof UntrustedStringKind
+            result instanceof ExternalStringKind
         |
             name != "get" and name != "getunicode" and name != "getall"
         )
     }
 
     override TaintKind getTaintOfMethodResult(string name) {
         (name = "get" or name = "getunicode") and
-        result instanceof UntrustedStringKind
+        result instanceof ExternalStringKind
         or
-        name = "getall" and result.(SequenceKind).getItem() instanceof UntrustedStringKind
+        name = "getall" and result.(SequenceKind).getItem() instanceof ExternalStringKind
     }
 }
 
 class FileUpload extends TaintKind {
     FileUpload() { this = "bottle.FileUpload" }
 
     override TaintKind getTaintOfAttribute(string name) {
-        name = "filename" and result instanceof UntrustedStringKind
+        name = "filename" and result instanceof ExternalStringKind
         or
-        name = "raw_filename" and result instanceof UntrustedStringKind
+        name = "raw_filename" and result instanceof ExternalStringKind
         or
         name = "file" and result instanceof UntrustedFile
     }
@@ -74,7 +74,7 @@ class BottleRequestParameter extends HttpRequestTaintSource {
         exists(BottleRoute route | route.getANamedArgument() = this.(ControlFlowNode).getNode())
     }
 
-    override predicate isSourceOf(TaintKind kind) { kind instanceof UntrustedStringKind }
+    override predicate isSourceOf(TaintKind kind) { kind instanceof ExternalStringKind }
 
     override string toString() { result = "bottle handler function argument" }
 }

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

@@ -1,5 +1,5 @@
 import python
-import semmle.python.security.strings.Untrusted
+import semmle.python.security.strings.External
 import semmle.python.web.Http
 import TurboGears
 
@@ -22,5 +22,5 @@ class UnvalidatedControllerMethodParameter extends HttpRequestTaintSource {
         )
     }
 
-    override predicate isSourceOf(TaintKind kind) { kind instanceof UntrustedStringKind }
+    override predicate isSourceOf(TaintKind kind) { kind instanceof ExternalStringKind }
 }

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

@@ -27,5 +27,5 @@ class ControllerMethodTemplatedReturnValue extends HttpResponseTaintSink {
         )
     }
 
-    override predicate sinks(TaintKind kind) { kind instanceof StringDictKind }
+    override predicate sinks(TaintKind kind) { kind instanceof ExternalStringDictKind }
 }