From db231a111c3690c6358942a55b78bb5c3669c748 Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Mon, 3 Oct 2022 02:31:43 +0530 Subject: [PATCH 01/10] 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. --- .../change-notes/2022-11-17-py-pam-improve.md | 4 ++ .../security/dataflow/PamAuthorization.qll | 44 +++++++++++++ .../PamAuthorizationCustomizations.qll | 61 +++++++++++++++++++ .../src/Security/CWE-285/PamAuthorization.ql | 31 +++------- .../PamAuthorization.expected | 11 +++- .../Security/CWE-285-PamAuthorization/options | 1 + .../CWE-285-PamAuthorization/pam_test.py | 33 +++++++++- 7 files changed, 158 insertions(+), 27 deletions(-) create mode 100644 python/ql/lib/change-notes/2022-11-17-py-pam-improve.md create mode 100644 python/ql/lib/semmle/python/security/dataflow/PamAuthorization.qll create mode 100644 python/ql/lib/semmle/python/security/dataflow/PamAuthorizationCustomizations.qll create mode 100644 python/ql/test/query-tests/Security/CWE-285-PamAuthorization/options 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..95c5f387103b --- /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 000000000000..82d73cc3e890 --- /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 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/src/Security/CWE-285/PamAuthorization.ql b/python/ql/src/Security/CWE-285/PamAuthorization.ql index affb59ff7db6..9e2eb00d815d 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 54b38f21aa67..7fb5217dbe1f 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 000000000000..28b616e5f19d --- /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 966e13cb9911..f80ce82669ae 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 @@ class PamConv(Structure): 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 @@ def authenticate_good(self, username, service='login'): 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 From 479a9e4156f63abf0ca4bf73292453b7e67b747e Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 28 Nov 2022 16:01:42 +0100 Subject: [PATCH 02/10] Python: Update `.expected` --- .../CWE-285-PamAuthorization/PamAuthorization.expected | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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 7fb5217dbe1f..ddee575f148a 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,10 +1,16 @@ edges +| pam_test.py:0:0:0:0 | ModuleVariableNode for pam_test.request | pam_test.py:70:16:70: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: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: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: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. | +| pam_test.py:75:14:75:40 | ControlFlowNode for pam_authenticate() | pam_test.py:4:26:4:32 | ControlFlowNode for ImportMember | 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. | From c310948521caa6217b2e0e8a531689092ca9b3e6 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 28 Nov 2022 16:02:38 +0100 Subject: [PATCH 03/10] Python: Remove enclosing module for PAM Auth Bypass.qll --- .../security/dataflow/PamAuthorization.qll | 43 ++++++++----------- .../src/Security/CWE-285/PamAuthorization.ql | 2 +- 2 files changed, 20 insertions(+), 25 deletions(-) diff --git a/python/ql/lib/semmle/python/security/dataflow/PamAuthorization.qll b/python/ql/lib/semmle/python/security/dataflow/PamAuthorization.qll index 82d73cc3e890..6abfc999092b 100644 --- a/python/ql/lib/semmle/python/security/dataflow/PamAuthorization.qll +++ b/python/ql/lib/semmle/python/security/dataflow/PamAuthorization.qll @@ -12,33 +12,28 @@ import semmle.python.dataflow.new.TaintTracking import PamAuthorizationCustomizations::PamAuthorizationCustomizations /** - * Provides a taint-tracking configuration for detecting "PAM Authorization" vulnerabilities. + * 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" } +class Configuration extends TaintTracking::Configuration { + Configuration() { this = "RemoteToPam" } - override predicate isSource(DataFlow::Node node) { node instanceof Source } + override predicate isSource(DataFlow::Node node) { node instanceof Source } - override predicate isSink(DataFlow::Node node) { node instanceof Sink } + 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) - } + 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 9e2eb00d815d..44f464b81f55 100644 --- a/python/ql/src/Security/CWE-285/PamAuthorization.ql +++ b/python/ql/src/Security/CWE-285/PamAuthorization.ql @@ -13,7 +13,7 @@ import python import DataFlow::PathGraph import semmle.python.ApiGraphs -import semmle.python.security.dataflow.PamAuthorization::PamAuthorization +import semmle.python.security.dataflow.PamAuthorization from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink where config.hasFlowPath(source, sink) From 3d9556e5a3d255b15b29a70beb5322f670de87d5 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 28 Nov 2022 16:03:17 +0100 Subject: [PATCH 04/10] Python: Use proper Query suffix --- .../{PamAuthorization.qll => PamAuthorizationQuery.qll} | 2 +- python/ql/src/Security/CWE-285/PamAuthorization.ql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename python/ql/lib/semmle/python/security/dataflow/{PamAuthorization.qll => PamAuthorizationQuery.qll} (96%) diff --git a/python/ql/lib/semmle/python/security/dataflow/PamAuthorization.qll b/python/ql/lib/semmle/python/security/dataflow/PamAuthorizationQuery.qll similarity index 96% rename from python/ql/lib/semmle/python/security/dataflow/PamAuthorization.qll rename to python/ql/lib/semmle/python/security/dataflow/PamAuthorizationQuery.qll index 6abfc999092b..18dc29a4a715 100644 --- a/python/ql/lib/semmle/python/security/dataflow/PamAuthorization.qll +++ b/python/ql/lib/semmle/python/security/dataflow/PamAuthorizationQuery.qll @@ -15,7 +15,7 @@ import PamAuthorizationCustomizations::PamAuthorizationCustomizations * A taint-tracking configuration for detecting "PAM Authorization" vulnerabilities. */ class Configuration extends TaintTracking::Configuration { - Configuration() { this = "RemoteToPam" } + Configuration() { this = "PamAuthorization" } override predicate isSource(DataFlow::Node node) { node instanceof Source } diff --git a/python/ql/src/Security/CWE-285/PamAuthorization.ql b/python/ql/src/Security/CWE-285/PamAuthorization.ql index 44f464b81f55..399e98ea3237 100644 --- a/python/ql/src/Security/CWE-285/PamAuthorization.ql +++ b/python/ql/src/Security/CWE-285/PamAuthorization.ql @@ -13,7 +13,7 @@ import python import DataFlow::PathGraph import semmle.python.ApiGraphs -import semmle.python.security.dataflow.PamAuthorization +import semmle.python.security.dataflow.PamAuthorizationQuery from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink where config.hasFlowPath(source, sink) From fef06679e5775cff610dceaae3ffd191376ec75b Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 28 Nov 2022 16:03:32 +0100 Subject: [PATCH 05/10] Python: Remove options file for PAM Auth Bypass Should not be needed --- .../test/query-tests/Security/CWE-285-PamAuthorization/options | 1 - 1 file changed, 1 deletion(-) delete mode 100644 python/ql/test/query-tests/Security/CWE-285-PamAuthorization/options diff --git a/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/options b/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/options deleted file mode 100644 index 28b616e5f19d..000000000000 --- a/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/options +++ /dev/null @@ -1 +0,0 @@ -semmle-extractor-options: --lang=3 --max-import-depth=2 -p ../lib From f8442ccb0ea9141604769a61dec73a9392deb643 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 28 Nov 2022 16:08:44 +0100 Subject: [PATCH 06/10] Python: Adjust PAM Auth bypass test slightly --- .../PamAuthorization.expected | 14 +++---- .../CWE-285-PamAuthorization/pam_test.py | 41 ++++++++++--------- 2 files changed, 29 insertions(+), 26 deletions(-) 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 ddee575f148a..55fdfa9795c3 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,16 +1,16 @@ edges -| pam_test.py:0:0:0:0 | ModuleVariableNode for pam_test.request | pam_test.py:70:16:70:22 | ControlFlowNode for 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: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: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() | +| 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: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() | +| 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:75:14:75:40 | ControlFlowNode for pam_authenticate() | pam_test.py:4:26:4:32 | ControlFlowNode for ImportMember | 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. | +| 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 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/pam_test.py b/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/pam_test.py index f80ce82669ae..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 @@ -39,29 +39,30 @@ 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_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 + return auth_success - 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__) @@ -73,10 +74,12 @@ def bad(): 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', '') @@ -89,4 +92,4 @@ def good(): retval = pam_acct_mgmt(handle, 0) auth_success = retval == 0 - return auth_success \ No newline at end of file + return auth_success From 4e67ec19d05a74ec22bb6fb1608dec561a3bd294 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 28 Nov 2022 16:14:38 +0100 Subject: [PATCH 07/10] Python: Adjust alert text of `py/pam-auth-bypass` --- python/ql/src/Security/CWE-285/PamAuthorization.ql | 3 ++- .../CWE-285-PamAuthorization/PamAuthorization.expected | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/python/ql/src/Security/CWE-285/PamAuthorization.ql b/python/ql/src/Security/CWE-285/PamAuthorization.ql index 399e98ea3237..3093bcf21d5f 100644 --- a/python/ql/src/Security/CWE-285/PamAuthorization.ql +++ b/python/ql/src/Security/CWE-285/PamAuthorization.ql @@ -18,4 +18,5 @@ import semmle.python.security.dataflow.PamAuthorizationQuery 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." + "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 55fdfa9795c3..d2abf9a88eaa 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 @@ -13,4 +13,4 @@ nodes | 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 call may lead to an authorization bypass, since `pam_acct_mgmt` is not called afterwards. | +| 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 | From 8694119c3cccbad423a2622bff77aa2aa258204b Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 28 Nov 2022 16:16:34 +0100 Subject: [PATCH 08/10] Python: Update `py/pam-auth-bypass` change-note wording --- python/ql/lib/change-notes/2022-11-17-py-pam-improve.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 95c5f387103b..ec7f49bd11d6 100644 --- 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 @@ -1,4 +1,4 @@ --- category: majorAnalysis --- -* Converted `py/pam-auth-bypass` to a data-flow query, resulting in significantly lower false positives. +* The _PAM authorization bypass due to incorrect usage_ (`py/pam-auth-bypass`) query has been converted to a tain-tracking query, resulting in significantly fewer false positives. From 544de5232c331fcbd7cb0fe33e9070115f3ad3e7 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 29 Nov 2022 14:47:45 +0100 Subject: [PATCH 09/10] Python: Use ' instead of ` in select text --- python/ql/src/Security/CWE-285/PamAuthorization.ql | 2 +- .../Security/CWE-285-PamAuthorization/PamAuthorization.expected | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/src/Security/CWE-285/PamAuthorization.ql b/python/ql/src/Security/CWE-285/PamAuthorization.ql index 3093bcf21d5f..43cbc33917a0 100644 --- a/python/ql/src/Security/CWE-285/PamAuthorization.ql +++ b/python/ql/src/Security/CWE-285/PamAuthorization.ql @@ -18,5 +18,5 @@ import semmle.python.security.dataflow.PamAuthorizationQuery 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.", + "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 d2abf9a88eaa..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 @@ -13,4 +13,4 @@ nodes | 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 | +| 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 | From 346dd864b5bb09bb3ae71581dbdf0ea4f1fe6b61 Mon Sep 17 00:00:00 2001 From: porcupineyhairs <61983466+porcupineyhairs@users.noreply.github.com> Date: Wed, 30 Nov 2022 05:21:11 +0530 Subject: [PATCH 10/10] Update python/ql/lib/change-notes/2022-11-17-py-pam-improve.md fix typo Co-authored-by: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> --- python/ql/lib/change-notes/2022-11-17-py-pam-improve.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index ec7f49bd11d6..78a267d3caa8 100644 --- 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 @@ -1,4 +1,4 @@ --- category: majorAnalysis --- -* The _PAM authorization bypass due to incorrect usage_ (`py/pam-auth-bypass`) query has been converted to a tain-tracking query, resulting in significantly fewer false positives. +* 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.