From 0c534444ad4fd31efc6359a46679bc798ea569e1 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 10 May 2022 17:59:21 +0200 Subject: [PATCH 1/8] Python: Format .qhelp file 99% of our .qhelp files have manually wrapped lines, so just wanted to keep things consistent --- .../Security/CWE-285/PamAuthorization.qhelp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-285/PamAuthorization.qhelp b/python/ql/src/experimental/Security/CWE-285/PamAuthorization.qhelp index 8e0f829f33ee..faf8f8ab896b 100644 --- a/python/ql/src/experimental/Security/CWE-285/PamAuthorization.qhelp +++ b/python/ql/src/experimental/Security/CWE-285/PamAuthorization.qhelp @@ -9,7 +9,9 @@

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. + 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.

@@ -26,7 +28,9 @@

- 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 + 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.

@@ -46,4 +50,4 @@ pam_acct_mgmt - \ No newline at end of file + From c84f693151f3ce723992c2b816be136a0e2259e4 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 10 May 2022 18:00:20 +0200 Subject: [PATCH 2/8] Python: Adjust PamAuthorization examples They did not have proper formatting (only 2 spaces), and I restructured them a bit more so they look like code in the wild --- .../Security/CWE-285/PamAuthorizationBad.py | 32 +++++++++------ .../Security/CWE-285/PamAuthorizationGood.py | 40 +++++++++++-------- 2 files changed, 43 insertions(+), 29 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-285/PamAuthorizationBad.py b/python/ql/src/experimental/Security/CWE-285/PamAuthorizationBad.py index 257f9b997292..15e952ab6b28 100644 --- a/python/ql/src/experimental/Security/CWE-285/PamAuthorizationBad.py +++ b/python/ql/src/experimental/Security/CWE-285/PamAuthorizationBad.py @@ -1,13 +1,19 @@ -def authenticate(self, username, password, service='login', encoding='utf-8', resetcreds=True): - libpam = CDLL(find_library("pam")) - pam_authenticate = libpam.pam_authenticate - pam_authenticate.restype = c_int - pam_authenticate.argtypes = [PamHandle, c_int] - - - handle = PamHandle() - conv = PamConv(my_conv, 0) - retval = pam_start(service, username, byref(conv), byref(handle)) - - retval = pam_authenticate(handle, 0) - return retval == 0 \ No newline at end of file +libpam = CDLL(find_library("pam")) + +pam_authenticate = libpam.pam_authenticate +pam_authenticate.restype = c_int +pam_authenticate.argtypes = [PamHandle, c_int] + +def authenticate(username, password, service='login'): + def my_conv(n_messages, messages, p_response, app_data): + """ + Simple conversation function that responds to any prompt where the echo is off with the supplied password + """ + ... + + handle = PamHandle() + conv = PamConv(my_conv, 0) + retval = pam_start(service, username, byref(conv), byref(handle)) + + retval = pam_authenticate(handle, 0) + return retval == 0 diff --git a/python/ql/src/experimental/Security/CWE-285/PamAuthorizationGood.py b/python/ql/src/experimental/Security/CWE-285/PamAuthorizationGood.py index 0f047c6ac659..eecb5b94a0e6 100644 --- a/python/ql/src/experimental/Security/CWE-285/PamAuthorizationGood.py +++ b/python/ql/src/experimental/Security/CWE-285/PamAuthorizationGood.py @@ -1,17 +1,25 @@ -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) - retval = pam_start(service, username, byref(conv), byref(handle)) +libpam = CDLL(find_library("pam")) - retval = pam_authenticate(handle, 0) - if retval == 0: - retval = pam_acct_mgmt(handle, 0) - return retval == 0 \ No newline at end of file +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] + +def authenticate(username, password, service='login'): + def my_conv(n_messages, messages, p_response, app_data): + """ + Simple conversation function that responds to any prompt where the echo is off with the supplied password + """ + ... + + handle = PamHandle() + conv = PamConv(my_conv, 0) + retval = pam_start(service, username, byref(conv), byref(handle)) + + retval = pam_authenticate(handle, 0) + if retval == 0: + retval = pam_acct_mgmt(handle, 0) + return retval == 0 From 7e87e18b32fe5d2486e60caebe377939357a1db5 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 10 May 2022 18:02:17 +0200 Subject: [PATCH 3/8] Python: Adjust name/description/select of `PamAuthorization.ql` Thought that calling out the actual vulnerability would make things easier for our end users :) --- .../src/experimental/Security/CWE-285/PamAuthorization.ql | 7 ++++--- .../query-tests/Security/CWE-285/PamAuthorization.expected | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/python/ql/src/experimental/Security/CWE-285/PamAuthorization.ql b/python/ql/src/experimental/Security/CWE-285/PamAuthorization.ql index 7561dec7f67d..3f80e0992c1a 100644 --- a/python/ql/src/experimental/Security/CWE-285/PamAuthorization.ql +++ b/python/ql/src/experimental/Security/CWE-285/PamAuthorization.ql @@ -1,6 +1,6 @@ /** - * @name Authorization bypass due to incorrect usage of PAM - * @description Using only the `pam_authenticate` call to check the validity of a login can lead to a authorization bypass. + * @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 * @problem.severity warning * @precision high @@ -33,4 +33,5 @@ where acctMgmtCall = libPam().getMember("pam_acct_mgmt").getACall() and DataFlow::localFlow(handle, acctMgmtCall.getArg(0)) ) -select authenticateCall, "This PAM authentication call may be lead to an authorization bypass." +select authenticateCall, + "This PAM authentication call may be lead to an authorization bypass, since 'pam_acct_mgmt' is not called afterwards." diff --git a/python/ql/test/experimental/query-tests/Security/CWE-285/PamAuthorization.expected b/python/ql/test/experimental/query-tests/Security/CWE-285/PamAuthorization.expected index 1b6c23291be9..e22e5bd61302 100644 --- a/python/ql/test/experimental/query-tests/Security/CWE-285/PamAuthorization.expected +++ b/python/ql/test/experimental/query-tests/Security/CWE-285/PamAuthorization.expected @@ -1 +1 @@ -| pam_test.py:48:18:48:44 | ControlFlowNode for pam_authenticate() | This PAM authentication call may be lead to an authorization bypass. | +| pam_test.py:48:18:48:44 | ControlFlowNode for pam_authenticate() | This PAM authentication call may be lead to an authorization bypass, since 'pam_acct_mgmt' is not called afterwards. | From f68b281762a329749b3c7b9519f071b2a3263e7a Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 10 May 2022 18:04:52 +0200 Subject: [PATCH 4/8] Python: Add change-note --- .../change-notes/2022-05-10-promote-pam-authentication-bypass.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 python/ql/src/change-notes/2022-05-10-promote-pam-authentication-bypass.md diff --git a/python/ql/src/change-notes/2022-05-10-promote-pam-authentication-bypass.md b/python/ql/src/change-notes/2022-05-10-promote-pam-authentication-bypass.md new file mode 100644 index 000000000000..e6379d8fc4bb --- /dev/null +++ b/python/ql/src/change-notes/2022-05-10-promote-pam-authentication-bypass.md @@ -0,0 +1 @@ +* The query "PAM authorization bypass due to incorrect usage" (`py/pam-auth-bypass`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @porcupineyhairs](https://github.com/github/codeql/pull/8595). From c890f9c4ac90791f72092483df356ec66451ca48 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 10 May 2022 18:08:43 +0200 Subject: [PATCH 5/8] Python: Fix change-note --- .../2022-05-10-promote-pam-authentication-bypass.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/python/ql/src/change-notes/2022-05-10-promote-pam-authentication-bypass.md b/python/ql/src/change-notes/2022-05-10-promote-pam-authentication-bypass.md index e6379d8fc4bb..e87e717f6099 100644 --- a/python/ql/src/change-notes/2022-05-10-promote-pam-authentication-bypass.md +++ b/python/ql/src/change-notes/2022-05-10-promote-pam-authentication-bypass.md @@ -1 +1,4 @@ +--- +category: newQuery +--- * The query "PAM authorization bypass due to incorrect usage" (`py/pam-auth-bypass`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @porcupineyhairs](https://github.com/github/codeql/pull/8595). From 0956d506deca527915c71588236c085e3820834f Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Wed, 11 May 2022 13:44:47 +0200 Subject: [PATCH 6/8] Python: Actually promote `py/pam-auth-bypass` :facepalm: --- .../{experimental => }/Security/CWE-285/PamAuthorization.qhelp | 0 .../src/{experimental => }/Security/CWE-285/PamAuthorization.ql | 0 .../{experimental => }/Security/CWE-285/PamAuthorizationBad.py | 0 .../{experimental => }/Security/CWE-285/PamAuthorizationGood.py | 0 .../query-tests/Security/CWE-285/PamAuthorization.qlref | 1 - .../Security/CWE-285-PamAuthorization}/PamAuthorization.expected | 0 .../Security/CWE-285-PamAuthorization/PamAuthorization.qlref | 1 + .../Security/CWE-285-PamAuthorization}/pam_test.py | 0 8 files changed, 1 insertion(+), 1 deletion(-) rename python/ql/src/{experimental => }/Security/CWE-285/PamAuthorization.qhelp (100%) rename python/ql/src/{experimental => }/Security/CWE-285/PamAuthorization.ql (100%) rename python/ql/src/{experimental => }/Security/CWE-285/PamAuthorizationBad.py (100%) rename python/ql/src/{experimental => }/Security/CWE-285/PamAuthorizationGood.py (100%) delete mode 100644 python/ql/test/experimental/query-tests/Security/CWE-285/PamAuthorization.qlref rename python/ql/test/{experimental/query-tests/Security/CWE-285 => query-tests/Security/CWE-285-PamAuthorization}/PamAuthorization.expected (100%) create mode 100644 python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.qlref rename python/ql/test/{experimental/query-tests/Security/CWE-285 => query-tests/Security/CWE-285-PamAuthorization}/pam_test.py (100%) diff --git a/python/ql/src/experimental/Security/CWE-285/PamAuthorization.qhelp b/python/ql/src/Security/CWE-285/PamAuthorization.qhelp similarity index 100% rename from python/ql/src/experimental/Security/CWE-285/PamAuthorization.qhelp rename to python/ql/src/Security/CWE-285/PamAuthorization.qhelp diff --git a/python/ql/src/experimental/Security/CWE-285/PamAuthorization.ql b/python/ql/src/Security/CWE-285/PamAuthorization.ql similarity index 100% rename from python/ql/src/experimental/Security/CWE-285/PamAuthorization.ql rename to python/ql/src/Security/CWE-285/PamAuthorization.ql diff --git a/python/ql/src/experimental/Security/CWE-285/PamAuthorizationBad.py b/python/ql/src/Security/CWE-285/PamAuthorizationBad.py similarity index 100% rename from python/ql/src/experimental/Security/CWE-285/PamAuthorizationBad.py rename to python/ql/src/Security/CWE-285/PamAuthorizationBad.py diff --git a/python/ql/src/experimental/Security/CWE-285/PamAuthorizationGood.py b/python/ql/src/Security/CWE-285/PamAuthorizationGood.py similarity index 100% rename from python/ql/src/experimental/Security/CWE-285/PamAuthorizationGood.py rename to python/ql/src/Security/CWE-285/PamAuthorizationGood.py diff --git a/python/ql/test/experimental/query-tests/Security/CWE-285/PamAuthorization.qlref b/python/ql/test/experimental/query-tests/Security/CWE-285/PamAuthorization.qlref deleted file mode 100644 index 38fac298b1e3..000000000000 --- a/python/ql/test/experimental/query-tests/Security/CWE-285/PamAuthorization.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/Security/CWE-285/PamAuthorization.ql diff --git a/python/ql/test/experimental/query-tests/Security/CWE-285/PamAuthorization.expected b/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.expected similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-285/PamAuthorization.expected rename to python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.expected diff --git a/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.qlref b/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.qlref new file mode 100644 index 000000000000..81915461d7ad --- /dev/null +++ b/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.qlref @@ -0,0 +1 @@ +Security/CWE-285/PamAuthorization.ql diff --git a/python/ql/test/experimental/query-tests/Security/CWE-285/pam_test.py b/python/ql/test/query-tests/Security/CWE-285-PamAuthorization/pam_test.py similarity index 100% rename from python/ql/test/experimental/query-tests/Security/CWE-285/pam_test.py rename to python/ql/test/query-tests/Security/CWE-285-PamAuthorization/pam_test.py From 044829c3bbbf0bc57ea61c61d44906105989a04f Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Wed, 11 May 2022 14:56:21 +0200 Subject: [PATCH 7/8] Python: Add `@security-severity` to `py/pam-auth-bypass` The value 8.1 was calculated by our internal tool. This corresponds to a 'High' severity, which from my gut feeling seems reasonable for authorization bypass. --- python/ql/src/Security/CWE-285/PamAuthorization.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/python/ql/src/Security/CWE-285/PamAuthorization.ql b/python/ql/src/Security/CWE-285/PamAuthorization.ql index 3f80e0992c1a..c92ea89972e7 100644 --- a/python/ql/src/Security/CWE-285/PamAuthorization.ql +++ b/python/ql/src/Security/CWE-285/PamAuthorization.ql @@ -3,6 +3,7 @@ * @description Not using `pam_acct_mgmt` after `pam_authenticate` to check the validity of a login can lead to authorization bypass. * @kind problem * @problem.severity warning + * @security-severity 8.1 * @precision high * @id py/pam-auth-bypass * @tags security From b54de13d973d7f1e21066076aaee1745047922ae Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Wed, 18 May 2022 10:30:29 +0200 Subject: [PATCH 8/8] Python: Apply suggestions from code review Co-authored-by: Taus --- python/ql/src/Security/CWE-285/PamAuthorization.qhelp | 4 ++-- python/ql/src/Security/CWE-285/PamAuthorization.ql | 2 +- .../CWE-285-PamAuthorization/PamAuthorization.expected | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/python/ql/src/Security/CWE-285/PamAuthorization.qhelp b/python/ql/src/Security/CWE-285/PamAuthorization.qhelp index faf8f8ab896b..63af4c2f22df 100644 --- a/python/ql/src/Security/CWE-285/PamAuthorization.qhelp +++ b/python/ql/src/Security/CWE-285/PamAuthorization.qhelp @@ -10,7 +10,7 @@ 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 + appropriate authorization to actually login. This means a user with an expired login or a password can still access the system.

@@ -29,7 +29,7 @@

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 + in this case, a user with expired credentials 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. diff --git a/python/ql/src/Security/CWE-285/PamAuthorization.ql b/python/ql/src/Security/CWE-285/PamAuthorization.ql index c92ea89972e7..33f5a69513b2 100644 --- a/python/ql/src/Security/CWE-285/PamAuthorization.ql +++ b/python/ql/src/Security/CWE-285/PamAuthorization.ql @@ -35,4 +35,4 @@ where DataFlow::localFlow(handle, acctMgmtCall.getArg(0)) ) select authenticateCall, - "This PAM authentication call may be lead to an authorization bypass, since 'pam_acct_mgmt' is not called afterwards." + "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 e22e5bd61302..54b38f21aa67 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 @@ -| pam_test.py:48:18:48:44 | ControlFlowNode for pam_authenticate() | This PAM authentication call may be lead to an authorization bypass, since 'pam_acct_mgmt' is not called afterwards. | +| 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. |