Python : Improve the PAM authentication bypass query

The current PAM auth bypass query which was contributed by me a few months back, alert on a vulenrable function but does not check if the function is actually function. This leads to a lot of fasle positives.

With this PR, I add a taint-tracking configuration to check if the username parameter can actually be supplied by an attacker.

This should bring the FP's significantly down.

python/ql/lib/change-notes/2022-11-17-py-pam-improve.md

@@ -0,0 +1,4 @@
+---
+ category: majorAnalysis
+---
+* Converted `py/pam-auth-bypass` to a data-flow query, resulting in significantly lower false positives.

python/ql/lib/semmle/python/security/dataflow/PamAuthorization.qll

@@ -0,0 +1,44 @@
+/**
+ * Provides a taint-tracking configuration for detecting "PAM Authorization" vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `PamAuthorization::Configuration` is needed, otherwise
+ * `PamAuthorizationCustomizations` should be imported instead.
+ */
+
+import python
+import semmle.python.ApiGraphs
+import semmle.python.dataflow.new.TaintTracking
+import PamAuthorizationCustomizations::PamAuthorizationCustomizations
+
+/**
+ * Provides a taint-tracking configuration for detecting "PAM Authorization" vulnerabilities.
+ */
+module PamAuthorization {
+  /**
+   * A taint-tracking configuration for detecting "PAM Authorization" vulnerabilities.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "RemoteToPam" }
+
+    override predicate isSource(DataFlow::Node node) { node instanceof Source }
+
+    override predicate isSink(DataFlow::Node node) { node instanceof Sink }
+
+    override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+      // Models flow from a remotely supplied username field to a PAM `handle`.
+      // `retval = pam_start(service, username, byref(conv), byref(handle))`
+      exists(API::CallNode pamStart, DataFlow::Node handle, API::CallNode pointer |
+        pointer = API::moduleImport("ctypes").getMember(["pointer", "byref"]).getACall() and
+        pamStart = libPam().getMember("pam_start").getACall() and
+        pointer = pamStart.getArg(3) and
+        handle = pointer.getArg(0) and
+        pamStart.getArg(1) = node1 and
+        handle = node2
+      )
+      or
+      // Flow from handle to the authenticate call in the final step
+      exists(VulnPamAuthCall c | c.getArg(0) = node1 | node2 = c)
+    }
+  }
+}

python/ql/lib/semmle/python/security/dataflow/PamAuthorizationCustomizations.qll

@@ -0,0 +1,61 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "PAM Authorization" vulnerabilities.
+ */
+
+import python
+import semmle.python.ApiGraphs
+import semmle.python.dataflow.new.TaintTracking
+import semmle.python.dataflow.new.RemoteFlowSources
+
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "PAM Authorization" vulnerabilities.
+ */
+module PamAuthorizationCustomizations {
+  /**
+   * Models a node corresponding to the `pam` library
+   */
+  API::Node libPam() {
+    exists(API::CallNode findLibCall, API::CallNode cdllCall |
+      findLibCall =
+        API::moduleImport("ctypes").getMember("util").getMember("find_library").getACall() and
+      findLibCall.getParameter(0).getAValueReachingSink().asExpr().(StrConst).getText() = "pam" and
+      cdllCall = API::moduleImport("ctypes").getMember("CDLL").getACall() and
+      cdllCall.getParameter(0).getAValueReachingSink() = findLibCall
+    |
+      result = cdllCall.getReturn()
+    )
+  }
+
+  /**
+   * A data flow source for "PAM Authorization" vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for "PAM Authorization" vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A source of remote user input, considered as a flow source.
+   */
+  class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+
+  /**
+   * A vulnerable `pam_authenticate` call considered as a flow sink.
+   */
+  class VulnPamAuthCall extends API::CallNode, Sink {
+    VulnPamAuthCall() {
+      exists(DataFlow::Node h |
+        this = libPam().getMember("pam_authenticate").getACall() and
+        h = this.getArg(0) and
+        not exists(API::CallNode acctMgmtCall |
+          acctMgmtCall = libPam().getMember("pam_acct_mgmt").getACall() and
+          DataFlow::localFlow(h, acctMgmtCall.getArg(0))
+        )
+      )
+    }
+  }
+}

python/ql/src/Security/CWE-285/PamAuthorization.ql

@@ -1,7 +1,7 @@
 /**
  * @name PAM authorization bypass due to incorrect usage
  * @description Not using `pam_acct_mgmt` after `pam_authenticate` to check the validity of a login can lead to authorization bypass.
- * @kind problem
+ * @kind path-problem
  * @problem.severity warning
  * @security-severity 8.1
  * @precision high
@@ -11,28 +11,11 @@
  */
 
 import python
+import DataFlow::PathGraph
 import semmle.python.ApiGraphs
-import experimental.semmle.python.Concepts
-import semmle.python.dataflow.new.TaintTracking
+import semmle.python.security.dataflow.PamAuthorization::PamAuthorization
 
-API::Node libPam() {
-  exists(API::CallNode findLibCall, API::CallNode cdllCall |
-    findLibCall = API::moduleImport("ctypes").getMember("util").getMember("find_library").getACall() and
-    findLibCall.getParameter(0).getAValueReachingSink().asExpr().(StrConst).getText() = "pam" and
-    cdllCall = API::moduleImport("ctypes").getMember("CDLL").getACall() and
-    cdllCall.getParameter(0).getAValueReachingSink() = findLibCall
-  |
-    result = cdllCall.getReturn()
-  )
-}
-
-from API::CallNode authenticateCall, DataFlow::Node handle
-where
-  authenticateCall = libPam().getMember("pam_authenticate").getACall() and
-  handle = authenticateCall.getArg(0) and
-  not exists(API::CallNode acctMgmtCall |
-    acctMgmtCall = libPam().getMember("pam_acct_mgmt").getACall() and
-    DataFlow::localFlow(handle, acctMgmtCall.getArg(0))
-  )
-select authenticateCall,
-  "This PAM authentication call may lead to an authorization bypass, since 'pam_acct_mgmt' is not called afterwards."
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "This PAM authentication call may lead to an authorization bypass, since `pam_acct_mgmt` is not called afterwards."

python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.expected

@@ -1 +1,10 @@
-| pam_test.py:48:18:48:44 | ControlFlowNode for pam_authenticate() | This PAM authentication call may lead to an authorization bypass, since 'pam_acct_mgmt' is not called afterwards. |
+edges
+| pam_test.py:70:16:70:22 | ControlFlowNode for request | pam_test.py:70:16:70:27 | ControlFlowNode for Attribute |
+| pam_test.py:70:16:70:27 | ControlFlowNode for Attribute | pam_test.py:75:14:75:40 | ControlFlowNode for pam_authenticate() |
+nodes
+| pam_test.py:70:16:70:22 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
+| pam_test.py:70:16:70:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
+| pam_test.py:75:14:75:40 | ControlFlowNode for pam_authenticate() | semmle.label | ControlFlowNode for pam_authenticate() |
+subpaths
+#select
+| pam_test.py:75:14:75:40 | ControlFlowNode for pam_authenticate() | pam_test.py:70:16:70:22 | ControlFlowNode for request | pam_test.py:75:14:75:40 | ControlFlowNode for pam_authenticate() | This PAM authentication call may lead to an authorization bypass, since `pam_acct_mgmt` is not called afterwards. |

python/ql/test/query-tests/Security/CWE-285-PamAuthorization/options

@@ -0,0 +1 @@
+semmle-extractor-options: --lang=3 --max-import-depth=2 -p ../lib

python/ql/test/query-tests/Security/CWE-285-PamAuthorization/pam_test.py

@@ -1,6 +1,7 @@
 from ctypes import CDLL, POINTER, Structure, byref
 from ctypes import c_char_p, c_int
 from ctypes.util import find_library
+from flask import Flask, request, redirect
 
 
 class PamHandle(Structure):
@@ -40,11 +41,11 @@ pam_acct_mgmt.argtypes = [PamHandle, c_int]
 
 class pam():
 
-    def authenticate_bad(self, username, service='login'):
+    def authenticate_bad_but_good(self, username, service='login'):
         handle = PamHandle()
         conv = PamConv(None, 0)
         retval = pam_start(service, username, byref(conv), byref(handle))
-
+        # This is not fine but we don't alert here as there is a possibility that the function is not actually used
         retval = pam_authenticate(handle, 0)
         auth_success = retval == 0
 
@@ -61,3 +62,31 @@ class pam():
         auth_success = retval == 0
 
         return auth_success
+
+
+app = Flask(__name__)
+@app.route('/bad')
+def bad():
+    username = request.args.get('username', '')
+    handle = PamHandle()
+    conv = PamConv(None, 0)
+    retval = pam_start(service, username, byref(conv), byref(handle))
+
+    retval = pam_authenticate(handle, 0)
+    auth_success = retval == 0
+
+    return auth_success
+
+@app.route('/good')
+def good():
+    username = request.args.get('username', '')
+    handle = PamHandle()
+    conv = PamConv(None, 0)
+    retval = pam_start(service, username, byref(conv), byref(handle))
+
+    retval = pam_authenticate(handle, 0)
+    if retval == 0:
+        retval = pam_acct_mgmt(handle, 0)
+    auth_success = retval == 0
+
+    return auth_success