Include changes from review

Author: Porcupiney Hairs

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

@@ -13,46 +13,24 @@ import semmle.python.ApiGraphs
 import experimental.semmle.python.Concepts
 import semmle.python.dataflow.new.TaintTracking
 
-private class LibPam extends API::Node {
-  LibPam() {
-    exists(
-      API::Node cdll, API::Node find_library, API::Node libpam, API::CallNode cdll_call,
-      API::CallNode find_lib_call, StrConst str
-    |
-      API::moduleImport("ctypes").getMember("CDLL") = cdll and
-      find_library = API::moduleImport("ctypes.util").getMember("find_library") and
-      cdll_call = cdll.getACall() and
-      find_lib_call = find_library.getACall() and
-      DataFlow::localFlow(DataFlow::exprNode(str), find_lib_call.getArg(0)) and
-      str.getText() = "pam" and
-      cdll_call.getArg(0) = find_lib_call and
-      libpam = cdll_call.getReturn()
-    |
-      libpam = this
-    )
-  }
-
-  override string toString() { result = "libpam" }
-}
-
-class PamAuthCall extends API::Node {
-  PamAuthCall() { exists(LibPam pam | pam.getMember("pam_authenticate") = this) }
-
-  override string toString() { result = "pam_authenticate" }
-}
-
-class PamActMgt extends API::Node {
-  PamActMgt() { exists(LibPam pam | pam.getMember("pam_acct_mgmt") = this) }
-
-  override string toString() { result = "pam_acct_mgmt" }
+API::Node libPam() {
+  exists(API::CallNode findLibCall, API::CallNode cdllCall, StrConst str |
+    findLibCall = API::moduleImport("ctypes.util").getMember("find_library").getACall() and
+    cdllCall = API::moduleImport("ctypes").getMember("CDLL").getACall() and
+    DataFlow::localFlow(DataFlow::exprNode(str), findLibCall.getArg(0)) and
+    str.getText() = "pam" and
+    cdllCall.getArg(0) = findLibCall
+  |
+    result = cdllCall.getReturn()
+  )
 }
 
-from PamAuthCall p, API::CallNode u, Expr handle
+from API::CallNode authenticateCall, DataFlow::Node handle
 where
-  u = p.getACall() and
-  handle = u.asExpr().(Call).getArg(0) and
-  not exists(PamActMgt pam |
-    DataFlow::localFlow(DataFlow::exprNode(handle),
-      DataFlow::exprNode(pam.getACall().asExpr().(Call).getArg(0)))
+  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 u, "This PAM authentication call may be lead to an authorization bypass."
+select authenticateCall, "This PAM authentication call may be lead to an authorization bypass."

python/ql/src/experimental/Security/CWE-285/PamAuthorizationBad.py

@@ -1,11 +1,9 @@
 def authenticate(self, username, password, service='login', encoding='utf-8', resetcreds=True):
   libpam                    = CDLL(find_library("pam"))
   pam_authenticate          = libpam.pam_authenticate
-  pam_acct_mgmt          = libpam.pam_acct_mgmt
   pam_authenticate.restype  = c_int
   pam_authenticate.argtypes = [PamHandle, c_int]
-  pam_acct_mgmt.restype  = c_int
-  pam_acct_mgmt.argtypes = [PamHandle, c_int]
+
 
   handle = PamHandle()
   conv   = PamConv(my_conv, 0)

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

@@ -1 +1 @@
-| bad.py:92:18:92:44 | ControlFlowNode for pam_authenticate() | This PAM authentication call may be lead to an authorization bypass. |
+| pam_test.py:44:18:44:44 | ControlFlowNode for pam_authenticate() | This PAM authentication call may be lead to an authorization bypass. |

python/ql/test/experimental/query-tests/Security/CWE-285/bad.py

@@ -0,0 +1,59 @@
+from ctypes import CDLL, POINTER, Structure, byref
+from ctypes import c_char_p, c_int
+from ctypes.util import find_library
+
+
+class PamHandle(Structure):
+    pass
+
+
+class PamMessage(Structure):
+    pass
+
+
+class PamResponse(Structure):
+    pass
+
+
+class PamConv(Structure):
+    pass
+
+
+libpam = CDLL(find_library("pam"))
+
+pam_start = libpam.pam_start
+pam_start.restype = c_int
+pam_start.argtypes = [c_char_p, c_char_p, POINTER(PamConv), POINTER(PamHandle)]
+
+pam_authenticate = libpam.pam_authenticate
+pam_authenticate.restype = c_int
+pam_authenticate.argtypes = [PamHandle, c_int]
+
+pam_acct_mgmt = libpam.pam_acct_mgmt
+pam_acct_mgmt.restype = c_int
+pam_acct_mgmt.argtypes = [PamHandle, c_int]
+
+
+class pam():
+
+    def authenticate_bad(self, username, service='login'):
+        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
+
+    def authenticate_good(self, username, service='login'):
+        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