Python: Tarslip query; Add sink for members and sanitizers for tarinfo objects.

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

@@ -49,6 +49,9 @@ class TarfileOpen extends TaintSource {
          * this tarfile is safe.
          */
         not this.(CallNode).getAnArg().refersTo(any(StringObject str))
+        and
+        /* Ignore opens within the tarfile module itself */
+        not this.(ControlFlowNode).getLocation().getFile().getBaseName() = "tarfile.py"
     }
 
     override predicate isSourceOf(TaintKind kind) {
@@ -73,13 +76,33 @@ class TarFileInfo extends TaintKind {
 }
 
 
+/* For efficiency we don't want to track the flow of taint
+ * around the tarfile module. */
+class ExcludeTarFilePy extends Sanitizer {
+
+    ExcludeTarFilePy() {
+        this = "Tar sanitizer"
+    }
+
+    override predicate sanitizingNode(TaintKind taint, ControlFlowNode node) {
+        node.getLocation().getFile().getBaseName() = "tarfile.py" and
+        (
+            taint instanceof OpenTarFile
+            or
+            taint instanceof TarFileInfo
+        )
+    }
+
+}
+
 /* Any call to an extractall method */
 class ExtractAllSink extends TaintSink {
 
     CallNode call;
 
     ExtractAllSink() {
-        this = call.getFunction().(AttrNode).getObject("extractall")
+        this = call.getFunction().(AttrNode).getObject("extractall") and
+        count(call.getAnArg()) = 0
     }
 
     override predicate sinks(TaintKind kind) {
@@ -104,6 +127,25 @@ class ExtractSink extends TaintSink {
 
 }
 
+
+/* Members argument to extract method */
+class ExtractMembersSink extends TaintSink {
+
+    CallNode call;
+
+    ExtractMembersSink() {
+        call.getFunction().(AttrNode).getName() = "extractall" and
+        (this = call.getArg(0) or this = call.getArgByName("members"))
+    }
+
+    override predicate sinks(TaintKind kind) {
+        kind.(SequenceKind).getItem() instanceof TarFileInfo
+        or
+        kind instanceof OpenTarFile
+    }
+
+}
+
 class TarFileInfoSanitizer extends Sanitizer {
 
     TarFileInfoSanitizer() {
@@ -114,33 +156,35 @@ class TarFileInfoSanitizer extends Sanitizer {
         path_sanitizing_test(test.getTest()) and
         taint instanceof TarFileInfo
     }
+
+
 }
 
 private predicate path_sanitizing_test(ControlFlowNode test) {
-    checks_not_absolute(test) and
-    test.getAChild+().getNode().(StrConst).getText() = ".."
-}
-
-private predicate checks_not_absolute(ControlFlowNode test) {
-    test.getAChild+().(CallNode).getFunction().pointsTo(Module::named("os.path").attr("absfile"))
+    /* Assume that any test with "path" in it is a sanitizer */
+    test.getAChild+().(AttrNode).getName() = "path"
     or
-    test.getAChild+().getNode().(StrConst).getText() = "/"
+    test.getAChild+().(NameNode).getId() = "path"
 }
 
-
 class TarSlipConfiguration extends TaintTracking::Configuration {
 
     TarSlipConfiguration() { this = "TarSlip configuration" }
 
     override predicate isSource(TaintTracking::Source source) { source instanceof TarfileOpen }
 
-    override predicate isSink(TaintTracking::Sink sink) { 
-        sink instanceof ExtractSink or sink instanceof ExtractAllSink
+    override predicate isSink(TaintTracking::Sink sink) {
+        sink instanceof ExtractSink or
+        sink instanceof ExtractAllSink or
+        sink instanceof ExtractMembersSink
     }
 
     override predicate isSanitizer(Sanitizer sanitizer) {
         sanitizer instanceof TarFileInfoSanitizer
+        or
+        sanitizer instanceof ExcludeTarFilePy
     }
+
 }
 
 
@@ -148,4 +192,3 @@ from TarSlipConfiguration config, TaintedPathSource src, TaintedPathSink sink
 where config.hasFlowPath(src, sink)
 select sink.getSink(), src, sink, "Extraction of tarfile from $@", src.getSource(), "a potentially untrusted source"
 
-

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

@@ -703,7 +703,7 @@ class TaintedNode extends TTaintedNode {
     /** Holds if the underlying CFG node for this node is a vulnerable node
      * and is vulnerable to this node's taint.
      */
-    predicate isVulnerableSink() {
+    predicate isSink() {
         exists(TaintedNode src, TaintSink vuln |
             src.isSource() and
             src.getASuccessor*() = this and
@@ -712,6 +712,13 @@ class TaintedNode extends TTaintedNode {
         )
     }
 
+    /** DEPRECATED -- Use `TaintedNode.isSink()` instead
+     * Sinks are not necessarily vulnerable
+     * For removal 2020-07-01 */
+    deprecated predicate isVulnerableSink() {
+        this.isSink()
+    }
+
     TaintFlowImplementation::TrackedTaint fromAttribute(string name) {
         result = this.getTrackedValue().(TaintFlowImplementation::TrackedAttribute).fromAttribute(name)
     }