Python: Tarslip query; track info objects and handle sanitization.

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

@@ -15,12 +15,28 @@ import python
 import semmle.python.security.Paths
 
 import semmle.python.security.TaintTracking
+import semmle.python.security.strings.Basic
 
 /** A TaintKind to represent open tarfile objects. That is, the result of calling `tarfile.open(...)` */
 class OpenTarFile extends TaintKind {
     OpenTarFile() {
         this = "tarfile.open"
     }
+
+    override TaintKind getTaintOfMethodResult(string name) {
+        name = "getmember" and result instanceof TarFileInfo
+        or
+        name = "getmembers" and result.(SequenceKind).getItem() instanceof TarFileInfo
+    }
+
+    override ClassValue getType() {
+        result = Module::named("tarfile").attr("TarFile")
+    }
+
+    override TaintKind getTaintForIteration() {
+        result instanceof TarFileInfo
+    }
+
 }
 
 /** The source of open tarfile objects. That is, any call to `tarfile.open(...)` */
@@ -41,13 +57,29 @@ class TarfileOpen extends TaintSource {
 
 }
 
-/* Any call to an extract method */
-class ExtractionSink extends TaintSink {
+class TarFileInfo extends TaintKind {
+
+    TarFileInfo() {
+        this = "tarfile.entry"
+    }
+
+    override TaintKind getTaintOfMethodResult(string name) {
+        name = "next" and result = this
+    }
+
+    override TaintKind getTaintOfAttribute(string name) {
+        name = "name" and result instanceof TarFileInfo
+    }
+}
+
+
+/* Any call to an extractall method */
+class ExtractAllSink extends TaintSink {
 
     CallNode call;
 
-    ExtractionSink() {
-        this = call.getFunction().(AttrNode).getObject(extract())
+    ExtractAllSink() {
+        this = call.getFunction().(AttrNode).getObject("extractall")
     }
 
     override predicate sinks(TaintKind kind) {
@@ -56,12 +88,45 @@ class ExtractionSink extends TaintSink {
 
 }
 
-private string extract() {
-    result = "extract" or result = "extractall"
+/* Argument to extract method */
+class ExtractSink extends TaintSink {
+
+    CallNode call;
+
+    ExtractSink() {
+        call.getFunction().(AttrNode).getName() = "extract" and
+        this = call.getArg(0)
+    }
+
+    override predicate sinks(TaintKind kind) {
+        kind instanceof TarFileInfo
+    }
+
 }
 
+class TarFileInfoSanitizer extends Sanitizer {
+
+    TarFileInfoSanitizer() {
+        this = "TarInfo sanitizer"
+    }
+
+    override predicate sanitizingEdge(TaintKind taint, PyEdgeRefinement test) {
+        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"))
+    or
+    test.getAChild+().getNode().(StrConst).getText() = "/"
+}
 
-//evil = [e for e in members if os.path.relpath(e).startswith(('/', '..'))]
 
 class TarSlipConfiguration extends TaintTracking::Configuration {
 
@@ -69,8 +134,13 @@ class TarSlipConfiguration extends TaintTracking::Configuration {
 
     override predicate isSource(TaintTracking::Source source) { source instanceof TarfileOpen }
 
-    override predicate isSink(TaintTracking::Sink sink) { sink instanceof ExtractionSink }
+    override predicate isSink(TaintTracking::Sink sink) { 
+        sink instanceof ExtractSink or sink instanceof ExtractAllSink
+    }
 
+    override predicate isSanitizer(Sanitizer sanitizer) {
+        sanitizer instanceof TarFileInfoSanitizer
+    }
 }
 
 

python/ql/test/query-tests/Security/CWE-022/TarSlip.expected

@@ -1,9 +1,13 @@
 edges
 | tarslip.py:12:7:12:39 | tarfile.open | tarslip.py:13:1:13:3 | tarfile.open |
-| tarslip.py:12:7:12:39 | tarfile.open | tarslip.py:14:1:14:3 | tarfile.open |
 | tarslip.py:16:7:16:39 | tarfile.open | tarslip.py:17:14:17:16 | tarfile.open |
-| tarslip.py:16:7:16:39 | tarfile.open | tarslip.py:18:5:18:7 | tarfile.open |
+| tarslip.py:17:5:17:9 | tarfile.entry | tarslip.py:18:17:18:21 | tarfile.entry |
+| tarslip.py:17:14:17:16 | tarfile.open | tarslip.py:17:5:17:9 | tarfile.entry |
+| tarslip.py:33:7:33:39 | tarfile.open | tarslip.py:34:14:34:16 | tarfile.open |
+| tarslip.py:34:5:34:9 | tarfile.entry | tarslip.py:37:17:37:21 | tarfile.entry |
+| tarslip.py:34:14:34:16 | tarfile.open | tarslip.py:34:5:34:9 | tarfile.entry |
 parents
 #select
 | tarslip.py:13:1:13:3 | Taint sink | tarslip.py:12:7:12:39 | tarfile.open | tarslip.py:13:1:13:3 | tarfile.open | Extraction of tarfile from $@ | tarslip.py:12:7:12:39 | Taint source | a potentially untrusted source |
-| tarslip.py:18:5:18:7 | Taint sink | tarslip.py:16:7:16:39 | tarfile.open | tarslip.py:18:5:18:7 | tarfile.open | Extraction of tarfile from $@ | tarslip.py:16:7:16:39 | Taint source | a potentially untrusted source |
+| tarslip.py:18:17:18:21 | Taint sink | tarslip.py:16:7:16:39 | tarfile.open | tarslip.py:18:17:18:21 | tarfile.entry | Extraction of tarfile from $@ | tarslip.py:16:7:16:39 | Taint source | a potentially untrusted source |
+| tarslip.py:37:17:37:21 | Taint sink | tarslip.py:33:7:33:39 | tarfile.open | tarslip.py:37:17:37:21 | tarfile.entry | Extraction of tarfile from $@ | tarslip.py:33:7:33:39 | Taint source | a potentially untrusted source |

python/ql/test/query-tests/Security/CWE-022/tarslip.py

@@ -20,3 +20,18 @@ for entry in tar:
 tar = tarfile.open(safe_filename_tar)
 tar.extractall()
 tar.close()
+
+
+#Sanitized
+tar = tarfile.open(unsafe_filename_tar)
+for entry in tar:
+    if os.path.isabs(entry.name) or ".." in entry.name:
+        raise ValueError("Illegal tar archive entry")
+    tar.extract(entry, "/tmp/unpack/")
+
+#Part Sanitized
+tar = tarfile.open(unsafe_filename_tar)
+for entry in tar:
+    if ".." in entry.name:
+        raise ValueError("Illegal tar archive entry")
+    tar.extract(entry, "/tmp/unpack/")