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 00000000000..95c5f387103 --- /dev/null +++ b/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. diff --git a/python/ql/lib/semmle/python/security/dataflow/PamAuthorization.qll b/python/ql/lib/semmle/python/security/dataflow/PamAuthorization.qll new file mode 100644 index 00000000000..82d73cc3e89 --- /dev/null +++ b/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) + } + } +} 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 00000000000..b3acdef6ef5 --- /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/src/Security/CWE-285/PamAuthorization.ql b/python/ql/src/Security/CWE-285/PamAuthorization.ql index affb59ff7db..9e2eb00d815 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,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." 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 54b38f21aa6..7fb5217dbe1 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,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. | diff --git a/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/options b/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/options new file mode 100644 index 00000000000..28b616e5f19 --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/options @@ -0,0 +1 @@ +semmle-extractor-options: --lang=3 --max-import-depth=2 -p ../lib 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 966e13cb991..f80ce82669a 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): @@ -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 \ No newline at end of file