Merge pull request github#2888 from RasmusWL/python-tarslip-sanitizer

Python: Improve tarslip sanitizer

Author: tausbn

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/test/query-tests/Security/CWE-022/TarSlip.expected

@@ -15,8 +15,15 @@ edges
 | tarslip.py:34:14:34:16 | tarfile.open | tarslip.py:34:1:34:17 | tarfile.entry |
 | tarslip.py:40:7:40:39 | tarfile.open | tarslip.py:41:24:41:26 | tarfile.open |
 | tarslip.py:40:7:40:39 | tarfile.open | tarslip.py:41:24:41:26 | tarfile.open |
+| tarslip.py:56:7:56:39 | tarfile.open | tarslip.py:57:14:57:16 | tarfile.open |
+| tarslip.py:56:7:56:39 | tarfile.open | tarslip.py:57:14:57:16 | tarfile.open |
+| tarslip.py:57:1:57:17 | tarfile.entry | tarslip.py:59:21:59:25 | tarfile.entry |
+| tarslip.py:57:1:57:17 | tarfile.entry | tarslip.py:59:21:59:25 | tarfile.entry |
+| tarslip.py:57:14:57:16 | tarfile.open | tarslip.py:57:1:57:17 | tarfile.entry |
+| tarslip.py:57:14:57:16 | tarfile.open | tarslip.py:57:1:57:17 | tarfile.entry |
 #select
 | tarslip.py:13:1:13:3 | tar | 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 | Attribute() | a potentially untrusted source |
 | tarslip.py:18:17:18:21 | entry | 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 | Attribute() | a potentially untrusted source |
 | tarslip.py:37:17:37:21 | entry | 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 | Attribute() | a potentially untrusted source |
 | tarslip.py:41:24:41:26 | tar | tarslip.py:40:7:40:39 | tarfile.open | tarslip.py:41:24:41:26 | tarfile.open | Extraction of tarfile from $@ | tarslip.py:40:7:40:39 | Attribute() | a potentially untrusted source |
+| tarslip.py:59:21:59:25 | entry | tarslip.py:56:7:56:39 | tarfile.open | tarslip.py:59:21:59:25 | tarfile.entry | Extraction of tarfile from $@ | tarslip.py:56:7:56:39 | Attribute() | a potentially untrusted source |

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

@@ -50,3 +50,33 @@ def safemembers(members):
 
 tar = tarfile.open(unsafe_filename_tar)
 tar.extractall(members=safemembers(tar))
+
+
+# Wrong sanitizer (is missing not)
+tar = tarfile.open(unsafe_filename_tar)
+for entry in tar:
+    if os.path.isabs(entry.name) or ".." in entry.name:
+        tar.extract(entry, "/tmp/unpack/")
+
+
+# OK Sanitized using not
+tar = tarfile.open(unsafe_filename_tar)
+for entry in tar:
+    if not (os.path.isabs(entry.name) or ".." in entry.name):
+        tar.extract(entry, "/tmp/unpack/")
+
+# The following two variants are included by purpose, since by default there is a
+# difference in handling `not x` and `not (x or False)` when overriding
+# Sanitizer.sanitizingEdge. We want to ensure we handle both consistently.
+
+# Not reported, although vulnerable to '..'
+tar = tarfile.open(unsafe_filename_tar)
+for entry in tar:
+    if not (os.path.isabs(entry.name) or False):
+        tar.extract(entry, "/tmp/unpack/")
+
+# Not reported, although vulnerable to '..'
+tar = tarfile.open(unsafe_filename_tar)
+for entry in tar:
+    if not os.path.isabs(entry.name):
+        tar.extract(entry, "/tmp/unpack/")