Merge pull request github#5118 from yoff/python-port-stacktrace-exosure

RasmusWL · web-flow · commit fbbec5d2b90f · 2021-03-16T14:52:44.000+01:00
Python: Port stack trace exposure
diff --git a/python/change-notes/2021-02-25-port-stactrace-exposure-query.md b/python/change-notes/2021-02-25-port-stactrace-exposure-query.md
@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Updated _Information exposure through an exception_ (`py/stack-trace-exposure`) query to use the new data-flow library and type-tracking approach instead of points-to analysis. You may see differences in the results found by the query, but overall this change should result in a more robust and accurate analysis.
diff --git a/python/ql/src/Security/CWE-209/StackTraceExposure.ql b/python/ql/src/Security/CWE-209/StackTraceExposure.ql
@@ -13,19 +13,10 @@
  */
 
 import python
-import semmle.python.security.Paths
-import semmle.python.security.Exceptions
-import semmle.python.web.HttpResponse
+import semmle.python.security.dataflow.StackTraceExposure
+import DataFlow::PathGraph
 
-class StackTraceExposureConfiguration extends TaintTracking::Configuration {
-  StackTraceExposureConfiguration() { this = "Stack trace exposure configuration" }
-
-  override predicate isSource(TaintTracking::Source source) { source instanceof ErrorInfoSource }
-
-  override predicate isSink(TaintTracking::Sink sink) { sink instanceof HttpResponseTaintSink }
-}
-
-from StackTraceExposureConfiguration config, TaintedPathSource src, TaintedPathSink sink
-where config.hasFlowPath(src, sink)
-select sink.getSink(), src, sink, "$@ may be exposed to an external user", src.getSource(),
+from StackTraceExposureConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "$@ may be exposed to an external user", source.getNode(),
   "Error information"
diff --git a/python/ql/src/semmle/python/security/dataflow/ExceptionInfo.qll b/python/ql/src/semmle/python/security/dataflow/ExceptionInfo.qll
@@ -0,0 +1,39 @@
+/** Provides classes representing various sources of information about raised exceptions. */
+
+import python
+import semmle.python.dataflow.new.DataFlow
+private import semmle.python.ApiGraphs
+
+/**
+ * INTERNAL: Do not use.
+ *
+ * A data-flow node that carries information about a raised exception.
+ * Such information should rarely be exposed directly to the user.
+ */
+abstract class ExceptionInfo extends DataFlow::Node { }
+
+/** A call to a function from the `traceback` module revealing information about a raised exception. */
+private class TracebackFunctionCall extends ExceptionInfo, DataFlow::CallCfgNode {
+  TracebackFunctionCall() {
+    this =
+      API::moduleImport("traceback")
+          .getMember([
+              "extract_tb", "extract_stack", "format_list", "format_exception_only",
+              "format_exception", "format_exc", "format_tb", "format_stack"
+            ])
+          .getACall()
+  }
+}
+
+/** A caught exception. */
+private class CaughtException extends ExceptionInfo {
+  CaughtException() {
+    this.asVar().getDefinition().(EssaNodeDefinition).getDefiningNode().getNode() =
+      any(ExceptStmt s).getName()
+  }
+}
+
+/** A call to `sys.exc_info`. */
+private class SysExcInfoCall extends ExceptionInfo, DataFlow::CallCfgNode {
+  SysExcInfoCall() { this = API::moduleImport("sys").getMember("exc_info").getACall() }
+}
diff --git a/python/ql/src/semmle/python/security/dataflow/StackTraceExposure.qll b/python/ql/src/semmle/python/security/dataflow/StackTraceExposure.qll
@@ -0,0 +1,33 @@
+/**
+ * Provides a taint-tracking configuration for detecting stack trace exposure
+ * vulnerabilities.
+ */
+
+import python
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.dataflow.new.TaintTracking
+import semmle.python.Concepts
+import semmle.python.dataflow.new.internal.Attributes
+private import ExceptionInfo
+
+/**
+ * A taint-tracking configuration for detecting stack trace exposure.
+ */
+class StackTraceExposureConfiguration extends TaintTracking::Configuration {
+  StackTraceExposureConfiguration() { this = "StackTraceExposureConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof ExceptionInfo }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink = any(HTTP::Server::HttpResponse response).getBody()
+  }
+
+  // A stack trace is accessible as the `__traceback__` attribute of a caught exception.
+  //  seehttps://docs.python.org/3/reference/datamodel.html#traceback-objects
+  override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    exists(AttrRead attr | attr.getAttributeName() = "__traceback__" |
+      nodeFrom = attr.getObject() and
+      nodeTo = attr
+    )
+  }
+}
diff --git a/python/ql/test/query-tests/Security/CWE-209/ExceptionInfo.expected b/python/ql/test/query-tests/Security/CWE-209/ExceptionInfo.expected
diff --git a/python/ql/test/query-tests/Security/CWE-209/ExceptionInfo.ql b/python/ql/test/query-tests/Security/CWE-209/ExceptionInfo.ql
@@ -0,0 +1,20 @@
+import python
+import semmle.python.dataflow.new.DataFlow
+import TestUtilities.InlineExpectationsTest
+import semmle.python.security.dataflow.ExceptionInfo
+
+class ExceptionInfoTest extends InlineExpectationsTest {
+  ExceptionInfoTest() { this = "ExceptionInfoTest" }
+
+  override string getARelevantTag() { result = "exceptionInfo" }
+
+  override predicate hasActualResult(Location location, string element, string tag, string value) {
+    exists(location.getFile().getRelativePath()) and
+    exists(ExceptionInfo e |
+      location = e.getLocation() and
+      element = e.toString() and
+      value = "" and
+      tag = "exceptionInfo"
+    )
+  }
+}
diff --git a/python/ql/test/query-tests/Security/CWE-209/Exceptions.py b/python/ql/test/query-tests/Security/CWE-209/Exceptions.py
@@ -0,0 +1,10 @@
+try:
+    1+2
+except Exception as e:  #$ exceptionInfo
+    e
+
+def test_exception():
+    try:
+        1+2
+    except Exception as e:  #$ exceptionInfo
+        e
diff --git a/python/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected b/python/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected
@@ -1,8 +1,19 @@
 edges
-| test.py:33:15:33:36 | exception info | test.py:34:29:34:31 | exception info |
-| test.py:33:15:33:36 | exception info | test.py:34:29:34:31 | exception info |
-| test.py:34:29:34:31 | exception info | test.py:34:16:34:32 | exception info |
-| test.py:34:29:34:31 | exception info | test.py:34:16:34:32 | exception info |
+| test.py:23:25:23:25 | SSA variable e | test.py:24:16:24:16 | ControlFlowNode for e |
+| test.py:31:25:31:25 | SSA variable e | test.py:32:16:32:30 | ControlFlowNode for Attribute |
+| test.py:49:15:49:36 | ControlFlowNode for Attribute() | test.py:50:29:50:31 | ControlFlowNode for err |
+| test.py:50:29:50:31 | ControlFlowNode for err | test.py:50:16:50:32 | ControlFlowNode for format_error() |
+nodes
+| test.py:16:16:16:37 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
+| test.py:23:25:23:25 | SSA variable e | semmle.label | SSA variable e |
+| test.py:24:16:24:16 | ControlFlowNode for e | semmle.label | ControlFlowNode for e |
+| test.py:31:25:31:25 | SSA variable e | semmle.label | SSA variable e |
+| test.py:32:16:32:30 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
+| test.py:49:15:49:36 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
+| test.py:50:16:50:32 | ControlFlowNode for format_error() | semmle.label | ControlFlowNode for format_error() |
+| test.py:50:29:50:31 | ControlFlowNode for err | semmle.label | ControlFlowNode for err |
 #select
-| test.py:16:16:16:37 | Attribute() | test.py:16:16:16:37 | exception info | test.py:16:16:16:37 | exception info | $@ may be exposed to an external user | test.py:16:16:16:37 | Attribute() | Error information |
-| test.py:34:16:34:32 | format_error() | test.py:33:15:33:36 | exception info | test.py:34:16:34:32 | exception info | $@ may be exposed to an external user | test.py:33:15:33:36 | Attribute() | Error information |
+| test.py:16:16:16:37 | ControlFlowNode for Attribute() | test.py:16:16:16:37 | ControlFlowNode for Attribute() | test.py:16:16:16:37 | ControlFlowNode for Attribute() | $@ may be exposed to an external user | test.py:16:16:16:37 | ControlFlowNode for Attribute() | Error information |
+| test.py:24:16:24:16 | ControlFlowNode for e | test.py:23:25:23:25 | SSA variable e | test.py:24:16:24:16 | ControlFlowNode for e | $@ may be exposed to an external user | test.py:23:25:23:25 | SSA variable e | Error information |
+| test.py:32:16:32:30 | ControlFlowNode for Attribute | test.py:31:25:31:25 | SSA variable e | test.py:32:16:32:30 | ControlFlowNode for Attribute | $@ may be exposed to an external user | test.py:31:25:31:25 | SSA variable e | Error information |
+| test.py:50:16:50:32 | ControlFlowNode for format_error() | test.py:49:15:49:36 | ControlFlowNode for Attribute() | test.py:50:16:50:32 | ControlFlowNode for format_error() | $@ may be exposed to an external user | test.py:49:15:49:36 | ControlFlowNode for Attribute() | Error information |
diff --git a/python/ql/test/query-tests/Security/CWE-209/Stacktrace.py b/python/ql/test/query-tests/Security/CWE-209/Stacktrace.py
@@ -0,0 +1,15 @@
+import sys, traceback
+
+try:
+    1/0
+except:
+    exc_type, exc_value, exc_traceback = sys.exc_info() #$ exceptionInfo
+
+    tb = traceback.extract_tb(exc_traceback) #$ exceptionInfo
+    stack = traceback.extract_stack() #$ exceptionInfo
+    print(traceback.format_exc(1, tb)) #$ exceptionInfo
+    print(traceback.format_exception(exc_type, exc_value, exc_traceback)) #$ exceptionInfo
+    print(traceback.format_exception_only(None, exc_value)) #$ exceptionInfo
+    print(traceback.format_list(stack)) #$ exceptionInfo
+    print(traceback.format_stack()) #$ exceptionInfo
+    print(traceback.format_tb(exc_traceback)) #$ exceptionInfo
diff --git a/python/ql/test/query-tests/Security/CWE-209/test.py b/python/ql/test/query-tests/Security/CWE-209/test.py
@@ -12,25 +12,41 @@ def do_computation():
 def server_bad():
     try:
         do_computation()
-    except Exception as e:
-        return traceback.format_exc()
+    except Exception:
+        return traceback.format_exc()  #$ exceptionInfo
+
+# BAD
+@app.route('/bad/direct')
+def server_bad_direct():
+    try:
+        do_computation()
+    except Exception as e:  #$ exceptionInfo
+        return e
+
+# BAD
+@app.route('/bad/traceback')
+def server_bad_traceback():
+    try:
+        do_computation()
+    except Exception as e:  #$ exceptionInfo
+        return e.__traceback__
 
 # GOOD
 @app.route('/good')
 def server_good():
     try:
         do_computation()
-    except Exception as e:
-        log(traceback.format_exc())
+    except Exception:
+        log(traceback.format_exc())  #$ exceptionInfo
         return "An internal error has occurred!"
 
 #BAD
 @app.route('/bad/with-flow')
 def server_bad_flow():
     try:
         do_computation()
-    except Exception as e:
-        err = traceback.format_exc()
+    except Exception:
+        err = traceback.format_exc()  #$ exceptionInfo
         return format_error(err)
 
 def format_error(msg):

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+lgtm,codescanning`
	`2`	+* Updated _Information exposure through an exception_ (`py/stack-trace-exposure`) query to use the new data-flow library and type-tracking approach instead of points-to analysis. You may see differences in the results found by the query, but overall this change should result in a more robust and accurate analysis.