diff --git a/python/ql/lib/change-notes/2022-11-17-py-pam-improve.md b/python/ql/lib/change-notes/2022-11-17-py-pam-improve.md new file mode 100644 index 000000000000..78a267d3caa8 --- /dev/null +++ b/python/ql/lib/change-notes/2022-11-17-py-pam-improve.md @@ -0,0 +1,4 @@ +--- + category: majorAnalysis +--- +* The _PAM authorization bypass due to incorrect usage_ (`py/pam-auth-bypass`) query has been converted to a taint-tracking query, resulting in significantly fewer false positives. diff --git a/python/ql/lib/semmle/python/security/dataflow/PamAuthorizationCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/PamAuthorizationCustomizations.qll new file mode 100644 index 000000000000..b3acdef6ef5c --- /dev/null +++ b/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)) + ) + ) + } + } +} diff --git a/python/ql/lib/semmle/python/security/dataflow/PamAuthorizationQuery.qll b/python/ql/lib/semmle/python/security/dataflow/PamAuthorizationQuery.qll new file mode 100644 index 000000000000..18dc29a4a715 --- /dev/null +++ b/python/ql/lib/semmle/python/security/dataflow/PamAuthorizationQuery.qll @@ -0,0 +1,39 @@ +/** + * 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 + +/** + * A taint-tracking configuration for detecting "PAM Authorization" vulnerabilities. + */ +class Configuration extends TaintTracking::Configuration { + Configuration() { this = "PamAuthorization" } + + 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) + } +} diff --git a/python/ql/src/Security/CWE-285/PamAuthorization.ql b/python/ql/src/Security/CWE-285/PamAuthorization.ql index affb59ff7db6..43cbc33917a0 100644 --- a/python/ql/src/Security/CWE-285/PamAuthorization.ql +++ b/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,12 @@ */ 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.PamAuthorizationQuery -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 depends on a $@, and 'pam_acct_mgmt' is not called afterwards.", + source.getNode(), "user-provided value" diff --git a/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.expected b/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.expected index 54b38f21aa67..3cb2cf2782d3 100644 --- a/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.expected +++ b/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.expected @@ -1 +1,16 @@ -| 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:0:0:0:0 | ModuleVariableNode for pam_test.request | pam_test.py:71:16:71:22 | ControlFlowNode for request | +| pam_test.py:4:26:4:32 | ControlFlowNode for ImportMember | pam_test.py:4:26:4:32 | GSSA Variable request | +| pam_test.py:4:26:4:32 | GSSA Variable request | pam_test.py:0:0:0:0 | ModuleVariableNode for pam_test.request | +| pam_test.py:71:16:71:22 | ControlFlowNode for request | pam_test.py:71:16:71:27 | ControlFlowNode for Attribute | +| pam_test.py:71:16:71:27 | ControlFlowNode for Attribute | pam_test.py:76:14:76:40 | ControlFlowNode for pam_authenticate() | +nodes +| pam_test.py:0:0:0:0 | ModuleVariableNode for pam_test.request | semmle.label | ModuleVariableNode for pam_test.request | +| pam_test.py:4:26:4:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember | +| pam_test.py:4:26:4:32 | GSSA Variable request | semmle.label | GSSA Variable request | +| pam_test.py:71:16:71:22 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | +| pam_test.py:71:16:71:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | +| pam_test.py:76:14:76:40 | ControlFlowNode for pam_authenticate() | semmle.label | ControlFlowNode for pam_authenticate() | +subpaths +#select +| pam_test.py:76:14:76:40 | ControlFlowNode for pam_authenticate() | pam_test.py:4:26:4:32 | ControlFlowNode for ImportMember | pam_test.py:76:14:76:40 | ControlFlowNode for pam_authenticate() | This PAM authentication depends on a $@, and 'pam_acct_mgmt' is not called afterwards. | pam_test.py:4:26:4:32 | ControlFlowNode for ImportMember | user-provided value | diff --git a/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/pam_test.py b/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/pam_test.py index 966e13cb9911..f16e3c9941ea 100644 --- a/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/pam_test.py +++ b/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): @@ -38,26 +39,57 @@ class PamConv(Structure): pam_acct_mgmt.argtypes = [PamHandle, c_int] -class pam(): +def authenticate_bad_but_no_alert(self, username, service='login'): + # This is not OK, but since we don't have flow from a remote-flow-source, we + # don't give an alert. + handle = PamHandle() + conv = PamConv(None, 0) + retval = pam_start(service, username, byref(conv), byref(handle)) + retval = pam_authenticate(handle, 0) + # NOT OK: no call to `pam_acct_mgmt` + auth_success = retval == 0 - def authenticate_bad(self, username, service='login'): - handle = PamHandle() - conv = PamConv(None, 0) - retval = pam_start(service, username, byref(conv), byref(handle)) + return auth_success - 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)) - 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 - retval = pam_authenticate(handle, 0) - if retval == 0: - retval = pam_acct_mgmt(handle, 0) - auth_success = retval == 0 + return auth_success - 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) + # NOT OK: no call to `pam_acct_mgmt` + 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