Skip to content

Commit d684dbd

Browse files
RasmusWLRasmusWL
authored
Merge pull request #10656 from porcupineyhairs/PyPamImprove
Python: Improve the PAM authentication bypass query
2 parents a6bc9fd + 346dd86 commit d684dbd

File tree

6 files changed

+177
-42
lines changed

6 files changed

+177
-42
lines changed

‎python/ql/lib/change-notes/2022-11-17-py-pam-improve.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: majorAnalysis
3+
---
4+
* 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.

‎python/ql/lib/semmle/python/security/dataflow/PamAuthorizationCustomizations.qll

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for detecting
3+
* "PAM Authorization" vulnerabilities.
4+
*/
5+
6+
import python
7+
import semmle.python.ApiGraphs
8+
import semmle.python.dataflow.new.TaintTracking
9+
import semmle.python.dataflow.new.RemoteFlowSources
10+
11+
/**
12+
* Provides default sources, sinks and sanitizers for detecting
13+
* "PAM Authorization" vulnerabilities.
14+
*/
15+
module PamAuthorizationCustomizations {
16+
/**
17+
* Models a node corresponding to the `pam` library
18+
*/
19+
API::Node libPam() {
20+
exists(API::CallNode findLibCall, API::CallNode cdllCall |
21+
findLibCall =
22+
API::moduleImport("ctypes").getMember("util").getMember("find_library").getACall() and
23+
findLibCall.getParameter(0).getAValueReachingSink().asExpr().(StrConst).getText() = "pam" and
24+
cdllCall = API::moduleImport("ctypes").getMember("CDLL").getACall() and
25+
cdllCall.getParameter(0).getAValueReachingSink() = findLibCall
26+
|
27+
result = cdllCall.getReturn()
28+
)
29+
}
30+
31+
/**
32+
* A data flow source for "PAM Authorization" vulnerabilities.
33+
*/
34+
abstract class Source extends DataFlow::Node { }
35+
36+
/**
37+
* A data flow sink for "PAM Authorization" vulnerabilities.
38+
*/
39+
abstract class Sink extends DataFlow::Node { }
40+
41+
/**
42+
* A source of remote user input, considered as a flow source.
43+
*/
44+
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
45+
46+
/**
47+
* A vulnerable `pam_authenticate` call considered as a flow sink.
48+
*/
49+
class VulnPamAuthCall extends API::CallNode, Sink {
50+
VulnPamAuthCall() {
51+
exists(DataFlow::Node h |
52+
this = libPam().getMember("pam_authenticate").getACall() and
53+
h = this.getArg(0) and
54+
not exists(API::CallNode acctMgmtCall |
55+
acctMgmtCall = libPam().getMember("pam_acct_mgmt").getACall() and
56+
DataFlow::localFlow(h, acctMgmtCall.getArg(0))
57+
)
58+
)
59+
}
60+
}
61+
}

‎python/ql/lib/semmle/python/security/dataflow/PamAuthorizationQuery.qll

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
/**
2+
* Provides a taint-tracking configuration for detecting "PAM Authorization" vulnerabilities.
3+
*
4+
* Note, for performance reasons: only import this file if
5+
* `PamAuthorization::Configuration` is needed, otherwise
6+
* `PamAuthorizationCustomizations` should be imported instead.
7+
*/
8+
9+
import python
10+
import semmle.python.ApiGraphs
11+
import semmle.python.dataflow.new.TaintTracking
12+
import PamAuthorizationCustomizations::PamAuthorizationCustomizations
13+
14+
/**
15+
* A taint-tracking configuration for detecting "PAM Authorization" vulnerabilities.
16+
*/
17+
class Configuration extends TaintTracking::Configuration {
18+
Configuration() { this = "PamAuthorization" }
19+
20+
override predicate isSource(DataFlow::Node node) { node instanceof Source }
21+
22+
override predicate isSink(DataFlow::Node node) { node instanceof Sink }
23+
24+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
25+
// Models flow from a remotely supplied username field to a PAM `handle`.
26+
// `retval = pam_start(service, username, byref(conv), byref(handle))`
27+
exists(API::CallNode pamStart, DataFlow::Node handle, API::CallNode pointer |
28+
pointer = API::moduleImport("ctypes").getMember(["pointer", "byref"]).getACall() and
29+
pamStart = libPam().getMember("pam_start").getACall() and
30+
pointer = pamStart.getArg(3) and
31+
handle = pointer.getArg(0) and
32+
pamStart.getArg(1) = node1 and
33+
handle = node2
34+
)
35+
or
36+
// Flow from handle to the authenticate call in the final step
37+
exists(VulnPamAuthCall c | c.getArg(0) = node1 | node2 = c)
38+
}
39+
}

‎python/ql/src/Security/CWE-285/PamAuthorization.ql

Lines changed: 8 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/**
22
* @name PAM authorization bypass due to incorrect usage
33
* @description Not using `pam_acct_mgmt` after `pam_authenticate` to check the validity of a login can lead to authorization bypass.
4-
* @kind problem
4+
* @kind path-problem
55
* @problem.severity warning
66
* @security-severity 8.1
77
* @precision high
@@ -11,28 +11,12 @@
1111
*/
1212

1313
import python
14+
import DataFlow::PathGraph
1415
import semmle.python.ApiGraphs
15-
import experimental.semmle.python.Concepts
16-
import semmle.python.dataflow.new.TaintTracking
16+
import semmle.python.security.dataflow.PamAuthorizationQuery
1717

18-
API::Node libPam() {
19-
exists(API::CallNode findLibCall, API::CallNode cdllCall |
20-
findLibCall = API::moduleImport("ctypes").getMember("util").getMember("find_library").getACall() and
21-
findLibCall.getParameter(0).getAValueReachingSink().asExpr().(StrConst).getText() = "pam" and
22-
cdllCall = API::moduleImport("ctypes").getMember("CDLL").getACall() and
23-
cdllCall.getParameter(0).getAValueReachingSink() = findLibCall
24-
|
25-
result = cdllCall.getReturn()
26-
)
27-
}
28-
29-
from API::CallNode authenticateCall, DataFlow::Node handle
30-
where
31-
authenticateCall = libPam().getMember("pam_authenticate").getACall() and
32-
handle = authenticateCall.getArg(0) and
33-
not exists(API::CallNode acctMgmtCall |
34-
acctMgmtCall = libPam().getMember("pam_acct_mgmt").getACall() and
35-
DataFlow::localFlow(handle, acctMgmtCall.getArg(0))
36-
)
37-
select authenticateCall,
38-
"This PAM authentication call may lead to an authorization bypass, since 'pam_acct_mgmt' is not called afterwards."
18+
from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
19+
where config.hasFlowPath(source, sink)
20+
select sink.getNode(), source, sink,
21+
"This PAM authentication depends on a $@, and 'pam_acct_mgmt' is not called afterwards.",
22+
source.getNode(), "user-provided value"

‎python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.expected

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,16 @@
1-
| 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. |
1+
edges
2+
| pam_test.py:0:0:0:0 | ModuleVariableNode for pam_test.request | pam_test.py:71:16:71:22 | ControlFlowNode for request |
3+
| pam_test.py:4:26:4:32 | ControlFlowNode for ImportMember | pam_test.py:4:26:4:32 | GSSA Variable request |
4+
| pam_test.py:4:26:4:32 | GSSA Variable request | pam_test.py:0:0:0:0 | ModuleVariableNode for pam_test.request |
5+
| pam_test.py:71:16:71:22 | ControlFlowNode for request | pam_test.py:71:16:71:27 | ControlFlowNode for Attribute |
6+
| pam_test.py:71:16:71:27 | ControlFlowNode for Attribute | pam_test.py:76:14:76:40 | ControlFlowNode for pam_authenticate() |
7+
nodes
8+
| pam_test.py:0:0:0:0 | ModuleVariableNode for pam_test.request | semmle.label | ModuleVariableNode for pam_test.request |
9+
| pam_test.py:4:26:4:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember |
10+
| pam_test.py:4:26:4:32 | GSSA Variable request | semmle.label | GSSA Variable request |
11+
| pam_test.py:71:16:71:22 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
12+
| pam_test.py:71:16:71:27 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
13+
| pam_test.py:76:14:76:40 | ControlFlowNode for pam_authenticate() | semmle.label | ControlFlowNode for pam_authenticate() |
14+
subpaths
15+
#select
16+
| 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 |

‎python/ql/test/query-tests/Security/CWE-285-PamAuthorization/pam_test.py

Lines changed: 49 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
from ctypes import CDLL, POINTER, Structure, byref
22
from ctypes import c_char_p, c_int
33
from ctypes.util import find_library
4+
from flask import Flask, request, redirect
45

56

67
class PamHandle(Structure):
@@ -38,26 +39,57 @@ class PamConv(Structure):
3839
pam_acct_mgmt.argtypes = [PamHandle, c_int]
3940

4041

41-
class pam():
42+
def authenticate_bad_but_no_alert(self, username, service='login'):
43+
# This is not OK, but since we don't have flow from a remote-flow-source, we
44+
# don't give an alert.
45+
handle = PamHandle()
46+
conv = PamConv(None, 0)
47+
retval = pam_start(service, username, byref(conv), byref(handle))
48+
retval = pam_authenticate(handle, 0)
49+
# NOT OK: no call to `pam_acct_mgmt`
50+
auth_success = retval == 0
4251

43-
def authenticate_bad(self, username, service='login'):
44-
handle = PamHandle()
45-
conv = PamConv(None, 0)
46-
retval = pam_start(service, username, byref(conv), byref(handle))
52+
return auth_success
4753

48-
retval = pam_authenticate(handle, 0)
49-
auth_success = retval == 0
5054

51-
return auth_success
55+
def authenticate_good(self, username, service='login'):
56+
handle = PamHandle()
57+
conv = PamConv(None, 0)
58+
retval = pam_start(service, username, byref(conv), byref(handle))
5259

53-
def authenticate_good(self, username, service='login'):
54-
handle = PamHandle()
55-
conv = PamConv(None, 0)
56-
retval = pam_start(service, username, byref(conv), byref(handle))
60+
retval = pam_authenticate(handle, 0)
61+
if retval == 0:
62+
retval = pam_acct_mgmt(handle, 0)
63+
auth_success = retval == 0
5764

58-
retval = pam_authenticate(handle, 0)
59-
if retval == 0:
60-
retval = pam_acct_mgmt(handle, 0)
61-
auth_success = retval == 0
65+
return auth_success
6266

63-
return auth_success
67+
68+
app = Flask(__name__)
69+
@app.route('/bad')
70+
def bad():
71+
username = request.args.get('username', '')
72+
handle = PamHandle()
73+
conv = PamConv(None, 0)
74+
retval = pam_start(service, username, byref(conv), byref(handle))
75+
76+
retval = pam_authenticate(handle, 0)
77+
# NOT OK: no call to `pam_acct_mgmt`
78+
auth_success = retval == 0
79+
80+
return auth_success
81+
82+
83+
@app.route('/good')
84+
def good():
85+
username = request.args.get('username', '')
86+
handle = PamHandle()
87+
conv = PamConv(None, 0)
88+
retval = pam_start(service, username, byref(conv), byref(handle))
89+
90+
retval = pam_authenticate(handle, 0)
91+
if retval == 0:
92+
retval = pam_acct_mgmt(handle, 0)
93+
auth_success = retval == 0
94+
95+
return auth_success

0 commit comments

Comments
 (0)