Merge pull request #11088 from yoff/python/inline-query-tests

Python: Inline query tests

Author: yoff

python/ql/test/experimental/dataflow/TestUtil/DataflowQueryTest.qll

@@ -0,0 +1,58 @@
+import python
+import semmle.python.dataflow.new.DataFlow
+import TestUtilities.InlineExpectationsTest
+private import semmle.python.dataflow.new.internal.PrintNode
+
+class DataFlowQueryTest extends InlineExpectationsTest {
+  DataFlowQueryTest() { this = "DataFlowQueryTest" }
+
+  override string getARelevantTag() { result = "result" }
+
+  override predicate hasActualResult(Location location, string element, string tag, string value) {
+    exists(DataFlow::Configuration cfg, DataFlow::Node sink | cfg.hasFlowTo(sink) |
+      location = sink.getLocation() and
+      tag = "result" and
+      value = "BAD" and
+      element = sink.toString()
+    )
+  }
+
+  // We allow annotating any sink with `result=OK` to signal
+  // safe sinks.
+  // Sometimes a line contains both an alert and a safe sink.
+  // In this situation, the annotation form `OK(safe sink)`
+  // can be useful.
+  override predicate hasOptionalResult(Location location, string element, string tag, string value) {
+    exists(DataFlow::Configuration cfg, DataFlow::Node sink |
+      cfg.isSink(sink) or cfg.isSink(sink, _)
+    |
+      location = sink.getLocation() and
+      tag = "result" and
+      value in ["OK", "OK(" + prettyNode(sink) + ")"] and
+      element = sink.toString()
+    )
+  }
+}
+
+query predicate missingAnnotationOnSink(Location location, string error, string element) {
+  error = "ERROR, you should add `# $ MISSING: result=BAD` or `result=OK` annotation" and
+  exists(DataFlow::Node sink |
+    exists(sink.getLocation().getFile().getRelativePath()) and
+    exists(DataFlow::Configuration cfg | cfg.isSink(sink) or cfg.isSink(sink, _)) and
+    location = sink.getLocation() and
+    element = prettyExpr(sink.asExpr()) and
+    not exists(DataFlow::Configuration cfg | cfg.hasFlowTo(sink)) and
+    not exists(FalseNegativeExpectation missingResult |
+      missingResult.getTag() = "result" and
+      missingResult.getValue() = "BAD" and
+      missingResult.getLocation().getFile() = location.getFile() and
+      missingResult.getLocation().getStartLine() = location.getStartLine()
+    ) and
+    not exists(GoodExpectation okResult |
+      okResult.getTag() = "result" and
+      okResult.getValue() in ["OK", "OK(" + prettyNode(sink) + ")"] and
+      okResult.getLocation().getFile() = location.getFile() and
+      okResult.getLocation().getStartLine() = location.getStartLine()
+    )
+  )
+}

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

@@ -0,0 +1,2 @@
+missingAnnotationOnSink
+failures

python/ql/test/query-tests/Security/CWE-022-PathInjection/DataflowQueryTest.ql

@@ -0,0 +1,3 @@
+import python
+import experimental.dataflow.TestUtil.DataflowQueryTest
+import semmle.python.security.dataflow.PathInjectionQuery

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

@@ -10,12 +10,12 @@
 def download_file():
     filename = request.args.get('filename', '')
     # ok since `send_from_directory` ensure this stays within `STATIC_DIR`
-    return send_from_directory(STATIC_DIR, filename) # OK
+    return send_from_directory(STATIC_DIR, filename) # $result=OK
 
 
 # see https://flask.palletsprojects.com/en/1.1.x/api/#flask.send_from_directory
 @app.route("/also-provide-dirname")
 def download_file():
     dirname = request.args.get('dirname', '')
     filename = request.args.get('filename', '')
-    return send_from_directory(dirname, filename) # NOT OK
+    return send_from_directory(dirname, filename) # $result=BAD result=OK(filename)

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

@@ -10,15 +10,15 @@
 @app.route("/path1")
 def path_injection():
     filename = request.args.get('filename', '')
-    f = open(os.path.join(STATIC_DIR, filename)) # NOT OK
+    f = open(os.path.join(STATIC_DIR, filename)) # $result=BAD
 
 
 @app.route("/path2")
 def path_injection():
     # Normalized, but not checked
     filename = request.args.get('filename', '')
     npath = os.path.normpath(os.path.join(STATIC_DIR, filename))
-    f = open(npath)  # NOT OK
+    f = open(npath)  # $result=BAD
 
 
 @app.route("/path3")
@@ -28,7 +28,7 @@ def unsafe_path_normpath():
     npath = os.path.normpath(os.path.join(STATIC_DIR, filename))
     if npath.startswith(STATIC_DIR):
         pass
-    f = open(npath)  # NOT OK
+    f = open(npath)  # $result=BAD
 
 
 @app.route("/path4")
@@ -37,15 +37,15 @@ def safe_path_normpath():
     filename = request.args.get('filename', '')
     npath = os.path.normpath(os.path.join(STATIC_DIR, filename))
     if npath.startswith(STATIC_DIR):
-        f = open(npath)  # OK
+        f = open(npath)  # $result=OK
 
 
 @app.route("/path5")
 def unsafe_path_realpath():
     # Normalized (by `realpath` that also follows symlinks), but not checked properly
     filename = request.args.get('filename', '')
     npath = os.path.realpath(os.path.join(STATIC_DIR, filename))
-    f = open(npath)  # NOT OK
+    f = open(npath)  # $result=BAD
 
 
 @app.route("/path6")
@@ -54,15 +54,15 @@ def safe_path_realpath():
     filename = request.args.get('filename', '')
     npath = os.path.realpath(os.path.join(STATIC_DIR, filename))
     if npath.startswith(STATIC_DIR):
-        f = open(npath)  # OK
+        f = open(npath)  # $result=OK
 
 
 @app.route("/path6")
 def unsafe_path_abspath():
     # Normalized (by `abspath`), but not checked properly
     filename = request.args.get('filename', '')
     npath = os.path.abspath(os.path.join(STATIC_DIR, filename))
-    f = open(npath)  # NOT OK
+    f = open(npath)  # $result=BAD
 
 
 @app.route("/path7")
@@ -71,7 +71,7 @@ def safe_path_abspath():
     filename = request.args.get('filename', '')
     npath = os.path.abspath(os.path.join(STATIC_DIR, filename))
     if npath.startswith(STATIC_DIR):
-        f = open(npath)  # OK
+        f = open(npath)  # $result=OK
 
 
 @app.route("/abspath_tricky")
@@ -84,22 +84,22 @@ def safe_path_abspath_tricky():
     filename = request.args.get('filename', '')
     possibly_unsafe_path = os.path.join(STATIC_DIR, filename)
     if os.path.abspath(possibly_unsafe_path).startswith(STATIC_DIR):
-        f = open(possibly_unsafe_path)  # OK
+        f = open(possibly_unsafe_path)  # $SPURIOUS: result=BAD
 
 
 @app.route("/int-only/<int:foo_id>")
 def flask_int_only(foo_id):
     # This is OK, since the flask routing ensures that `foo_id` MUST be an integer.
     path = os.path.join(STATIC_DIR, foo_id)
-    f = open(path)  # OK TODO: FP
+    f = open(path)  # $spurious: result=BAD
 
 
 @app.route("/not-path/<foo>")
 def flask_not_path(foo):
     # On UNIX systems, this is OK, since without being marked as `<path:foo>`, flask
     # routing ensures that `foo` cannot contain forward slashes (not by using %2F either).
     path = os.path.join(STATIC_DIR, foo)
-    f = open(path)  # OK if only running on UNIX systems, NOT OK if could be running on windows
+    f = open(path)  # $result=BAD // OK if only running on UNIX systems, NOT OK if could be running on windows
 
 
 @app.route("/no-dot-dot")
@@ -110,7 +110,7 @@ def no_dot_dot():
     # handle if `filename` is an absolute path
     if '../' in path:
         return "not this time"
-    f = open(path)  # NOT OK
+    f = open(path)  # $result=BAD
 
 
 @app.route("/no-dot-dot-with-prefix")
@@ -121,15 +121,15 @@ def no_dot_dot_with_prefix():
     # Therefore, for UNIX-only programs, the `../` check is enough to stop path injections.
     if '../' in path:
         return "not this time"
-    f = open(path)  # OK if only running on UNIX systems, NOT OK if could be running on windows
+    f = open(path)  # $result=BAD // OK if only running on UNIX systems, NOT OK if could be running on windows
 
 
 @app.route("/replace-slash")
 def replace_slash():
     filename = request.args.get('filename', '')
     path = os.path.join(STATIC_DIR, filename)
     sanitized = path.replace("/", "_")
-    f = open(sanitized)  # OK if only running on UNIX systems, NOT OK if could be running on windows
+    f = open(sanitized)  # $result=BAD // OK if only running on UNIX systems, NOT OK if could be running on windows
 
 
 @app.route("/stackoverflow-solution")
@@ -139,7 +139,7 @@ def stackoverflow_solution():
     path = os.path.join(STATIC_DIR, filename)
     if os.path.commonprefix((os.path.realpath(path), STATIC_DIR)) != STATIC_DIR:
         return "not this time"
-    f = open(path) # OK TODO: FP
+    f = open(path) # $SPURIOUS: result=BAD
 
 
 SAFE_FILES = ['foo', 'bar', 'baz']
@@ -149,4 +149,4 @@ def safe_set_of_files():
     filename = request.args.get('filename', '')
     if filename in SAFE_FILES:
         path = os.path.join(STATIC_DIR, filename)
-        f = open(path) # OK TODO: FP
+        f = open(path) # $SPURIOUS: result=BAD

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

@@ -16,34 +16,34 @@ def normalize(x):
 @app.route("/path")
 def simple():
     x = source()
-    open(x)  # NOT OK
+    open(x)  # $result=BAD
 
 
 @app.route("/path")
 def normalization():
     x = source()
     y = normalize(x)
-    open(y)  # NOT OK
+    open(y)  # $result=BAD
 
 
 @app.route("/path")
 def check():
     x = source()
     if x.startswith("subfolder/"):
-        open(x)  # NOT OK
+        open(x)  # $result=BAD
 
 
 @app.route("/path")
 def normalize_then_check():
     x = source()
     y = normalize(x)
     if y.startswith("subfolder/"):
-        open(y)  # OK
+        open(y)  # $result=OK
 
 
 @app.route("/path")
 def check_then_normalize():
     x = source()
     if x.startswith("subfolder/"):
         y = normalize(x)
-        open(y)  # NOT OK
+        open(y)  # $result=BAD

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

@@ -20,7 +20,7 @@ def normalize_then_check():
     x = source()
     y = normalize(x)  # <--- this call...
     if y.startswith("subfolder/"):
-        open(y)  # OK
+        open(y)  # $result=OK
 
 
 @app.route("/path")
@@ -29,7 +29,7 @@ def normalize_check_normalize():
     y = normalize(x)  # (...or this call...)
     if y.startswith("subfolder/"):
         z = normalize(y)  # <--- ...can jump to here, resulting in FP
-        open(z)  # OK
+        open(z)  # $result=OK
 
 
 # The problem does not manifest if we simply define an alias
@@ -42,4 +42,4 @@ def normalize_check_normalize_alias():
     y = normpath(x)
     if y.startswith("subfolder/"):
         z = normpath(y)
-        open(z)  # OK
+        open(z)  # $result=OK

python/ql/test/query-tests/Security/CWE-078-CommandInjection/DataflowQueryTest.expected

@@ -0,0 +1,2 @@
+missingAnnotationOnSink
+failures

python/ql/test/query-tests/Security/CWE-078-CommandInjection/DataflowQueryTest.ql

@@ -0,0 +1,3 @@
+import python
+import experimental.dataflow.TestUtil.DataflowQueryTest
+import semmle.python.security.dataflow.CommandInjectionQuery

python/ql/test/query-tests/Security/CWE-078-CommandInjection/command_injection.py

@@ -10,36 +10,36 @@
 def command_injection1():
     files = request.args.get('files', '')
     # Don't let files be `; rm -rf /`
-    os.system("ls " + files)
+    os.system("ls " + files) # $result=BAD
 
 
 @app.route("/command2")
 def command_injection2():
     files = request.args.get('files', '')
     # Don't let files be `; rm -rf /`
-    subprocess.Popen("ls " + files, shell=True)
+    subprocess.Popen("ls " + files, shell=True) # $result=BAD
 
 
 @app.route("/command3")
 def first_arg_injection():
     cmd = request.args.get('cmd', '')
-    subprocess.Popen([cmd, "param1"])
+    subprocess.Popen([cmd, "param1"]) # $result=BAD
 
 
 @app.route("/other_cases")
 def others():
     files = request.args.get('files', '')
     # Don't let files be `; rm -rf /`
-    os.popen("ls " + files)
+    os.popen("ls " + files) # $result=BAD
 
 
 @app.route("/multiple")
 def multiple():
     command = request.args.get('command', '')
     # We should mark flow to both calls here, which conflicts with removing flow out of
     # a sink due to use-use flow.
-    os.system(command)
-    os.system(command)
+    os.system(command) # $result=BAD
+    os.system(command) # $result=BAD
 
 
 @app.route("/not-into-sink-impl")
@@ -52,11 +52,11 @@ def not_into_sink_impl():
     subprocess.call implementation: https://github.com/python/cpython/blob/fa7ce080175f65d678a7d5756c94f82887fc9803/Lib/subprocess.py#L341
     """
     command = request.args.get('command', '')
-    os.system(command)
-    os.popen(command)
-    subprocess.call(command)
-    subprocess.check_call(command)
-    subprocess.run(command)
+    os.system(command) # $result=BAD
+    os.popen(command) # $result=BAD
+    subprocess.call(command) # $result=BAD
+    subprocess.check_call(command) # $result=BAD
+    subprocess.run(command) # $result=BAD
 
 
 @app.route("/path-exists-not-sanitizer")
@@ -70,11 +70,11 @@ def path_exists_not_sanitizer():
     """
     path = request.args.get('path', '')
     if os.path.exists(path):
-        os.system("ls " + path) # NOT OK
+        os.system("ls " + path) # $result=BAD
 
 
 @app.route("/restricted-characters")
 def restricted_characters():
     path = request.args.get('path', '')
     if re.match(r'^[a-zA-Z0-9_-]+$', path):
-        os.system("ls " + path) # OK (TODO: Currently FP)
+        os.system("ls " + path) # $SPURIOUS: result=BAD