From 85c751cb7fc44f2d993c109b5b94f0a17265ae78 Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Sun, 3 Apr 2022 19:02:26 +0530 Subject: [PATCH 1/2] CPP: PAM Authorization Bypass This PR is similar to my other PRs for [Python](https://github.com/github/codeql/pull/8595) and [Golang](https://github.com/github/codeql-go/pull/709). This PR aims to detect instances were an initiated PAM Transaction invokes the `pam_authenticate` method but does not invoke a call to the pam_acct_mgmt` method. This is bad as a call to `pam_authenticate` only verifies the users credentials. It does not check if the user account is still is a valid state. If only a call to `pam_authenticate` is used to verify the user, a user with an expired account password would still be able to login. This can be prevented by calling the `pam_acct_mgmt` function after a `pam_authenticate` function. --- .../CWE/CWE-285/PamAuthorization.qhelp | 49 +++++++++++++++ .../Security/CWE/CWE-285/PamAuthorization.ql | 33 +++++++++++ .../CWE/CWE-285/PamAuthorizationBad.cpp | 20 +++++++ .../CWE/CWE-285/PamAuthorizationGood.cpp | 24 ++++++++ .../CWE/CWE-285/PamAuthorization.expected | 1 + .../CWE/CWE-285/PamAuthorization.qlref | 1 + .../query-tests/Security/CWE/CWE-285/test.cpp | 59 +++++++++++++++++++ 7 files changed, 187 insertions(+) create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.qhelp create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.ql create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorizationBad.cpp create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorizationGood.cpp create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-285/PamAuthorization.expected create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-285/PamAuthorization.qlref create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-285/test.cpp diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.qhelp new file mode 100644 index 000000000000..6880596e3b73 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.qhelp @@ -0,0 +1,49 @@ + + + +

+ Using only a call to + pam_authenticate + to check the validity of a login can lead to authorization bypass vulnerabilities. +

+

+ A + pam_authenticate + only verifies the credentials of a user. It does not check if a user has an appropriate authorization to actually login. This means a user with a expired login or a password can still access the system. +

+ + + + +

+ A call to + pam_authenticate + should be followed by a call to + pam_acct_mgmt + to check if a user is allowed to login. +

+ + + +

+ In the following example, the code only checks the credentials of a user. Hence, in this case, a user expired with expired creds can still login. This can be verified by creating a new user account, expiring it with + chage -E0 `username` + and then trying to log in. +

+ + +

+ This can be avoided by calling + pam_acct_mgmt + call to verify access as has been done in the snippet shown below. +

+ + + + +
  • + Man-Page: + pam_acct_mgmt +
    • +     +     \ No newline at end of file diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.ql b/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.ql new file mode 100644 index 000000000000..1b26774d6a0a --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.ql @@ -0,0 +1,33 @@ +/** + * @name PAM Authorization bypass + * @description Only using `pam_authenticate` call to authenticate users can lead to authorization vulnerabilities. + * @kind problem + * @problem.severity error + * @id cpp/pam-auth-bypass + * @tags security + * external/cwe/cwe-285 + */ +import cpp +import semmle.code.cpp.dataflow.DataFlow +import semmle.code.cpp.valuenumbering.GlobalValueNumbering + +private class PamAuthCall extends FunctionCall { + PamAuthCall() { + exists(Function f | f.hasName("pam_authenticate") | f.getACallToThisFunction() = this) + } +} + +private class PamActMgmtCall extends FunctionCall { + PamActMgmtCall() { + exists(Function f | f.hasName("pam_acct_mgmt") | f.getACallToThisFunction() = this) + } +} + +from PamAuthCall pa, Expr handle +where + pa.getArgument(0) = handle and + not exists(PamActMgmtCall pac | + globalValueNumber(handle) = globalValueNumber(pac.getArgument(0)) or + DataFlow::localExprFlow(handle, pac.getArgument(0)) + ) +select pa, "This PAM authentication call may be lead to an authorization bypass." diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorizationBad.cpp b/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorizationBad.cpp new file mode 100644 index 000000000000..cde024cde343 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorizationBad.cpp @@ -0,0 +1,20 @@ +bool PamAuthGood(const std::string &username_in, + const std::string &password_in, + std::string &authenticated_username) +{ + + struct pam_handle *pamh = nullptr; /* pam session handle */ + + const char *username = username_in.c_str(); + int err = pam_start("test", username, + 0, &pamh); + if (err != PAM_SUCCESS) + { + return false; + } + + err = pam_authenticate(pamh, 0); // BAD + if (err != PAM_SUCCESS) + return err; + return true; +} diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorizationGood.cpp b/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorizationGood.cpp new file mode 100644 index 000000000000..e4a2ce008c54 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorizationGood.cpp @@ -0,0 +1,24 @@ +bool PamAuthGood(const std::string &username_in, + const std::string &password_in, + std::string &authenticated_username) +{ + + struct pam_handle *pamh = nullptr; /* pam session handle */ + + const char *username = username_in.c_str(); + int err = pam_start("test", username, + 0, &pamh); + if (err != PAM_SUCCESS) + { + return false; + } + + err = pam_authenticate(pamh, 0); + if (err != PAM_SUCCESS) + return err; + + err = pam_acct_mgmt(pamh, 0); // GOOD + if (err != PAM_SUCCESS) + return err; + return true; +} diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-285/PamAuthorization.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-285/PamAuthorization.expected new file mode 100644 index 000000000000..af34f888df2e --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-285/PamAuthorization.expected @@ -0,0 +1 @@ +| test.cpp:29:11:29:26 | call to pam_authenticate | This PAM authentication call may be lead to an authorization bypass. | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-285/PamAuthorization.qlref b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-285/PamAuthorization.qlref new file mode 100644 index 000000000000..f1135f7d536a --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-285/PamAuthorization.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-285/PamAuthorization.ql diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-285/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-285/test.cpp new file mode 100644 index 000000000000..e2753f10775e --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-285/test.cpp @@ -0,0 +1,59 @@ +#include "../../../../../library-tests/dataflow/taint-tests/stl.h" + +using namespace std; + +#define PAM_SUCCESS 1 + +typedef struct pam_handle +{ +}; +int pam_start(std::string servicename, std::string username, int a, struct pam_handle **); +int pam_authenticate(struct pam_handle *, int e); +int pam_acct_mgmt(struct pam_handle *, int e); + +bool PamAuthBad(const std::string &username_in, + const std::string &password_in, + std::string &authenticated_username) +{ + + struct pam_handle *pamh = nullptr; /* pam session handle */ + + const char *username = username_in.c_str(); + int err = pam_start("test", username, + 0, &pamh); + if (err != PAM_SUCCESS) + { + return false; + } + + err = pam_authenticate(pamh, 0); + if (err != PAM_SUCCESS) + return err; + + return true; +} + +bool PamAuthGood(const std::string &username_in, + const std::string &password_in, + std::string &authenticated_username) +{ + + struct pam_handle *pamh = nullptr; /* pam session handle */ + + const char *username = username_in.c_str(); + int err = pam_start("test", username, + 0, &pamh); + if (err != PAM_SUCCESS) + { + return false; + } + + err = pam_authenticate(pamh, 0); + if (err != PAM_SUCCESS) + return err; + + err = pam_acct_mgmt(pamh, 0); + if (err != PAM_SUCCESS) + return err; + return true; +} From 06edb3f3a1e4714c9d4b7074f98425fb3faa03ee Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Thu, 21 Apr 2022 00:23:49 +0530 Subject: [PATCH 2/2] fix formatting issues --- .../experimental/Security/CWE/CWE-285/PamAuthorization.qhelp | 4 ++-- .../src/experimental/Security/CWE/CWE-285/PamAuthorization.ql | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.qhelp index 6880596e3b73..debf30dfa65d 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.qhelp +++ b/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.qhelp @@ -30,14 +30,14 @@ chage -E0 `username` and then trying to log in.

    - +

    This can be avoided by calling pam_acct_mgmt call to verify access as has been done in the snippet shown below.

    - + diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.ql b/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.ql index 1b26774d6a0a..5292a705d93b 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-285/PamAuthorization.ql @@ -7,6 +7,7 @@ * @tags security * external/cwe/cwe-285 */ + import cpp import semmle.code.cpp.dataflow.DataFlow import semmle.code.cpp.valuenumbering.GlobalValueNumbering