Skip to content

Commit 84a74f4

Browse files
rdmarsh2rdmarsh2
authored
Merge pull request github#3002 from theopolis/cpp-linux-drop-privileges-outoforder
CPP: Add query for CWE-273 that detects out-of-order setuid
2 parents 8792d0d + 07605f5 commit 84a74f4

File tree

3 files changed

+274
-0
lines changed

3 files changed

+274
-0
lines changed

‎cpp/ql/src/experimental/Security/CWE/CWE-273/PrivilegeDroppingOutoforder.c

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
#include <stdlib.h>
2+
#include <sys/param.h>
3+
#include <unistd.h>
4+
#include <pwd.h>
5+
6+
void callSetuidAndCheck(int uid) {
7+
if (setuid(uid) != 0) {
8+
exit(1);
9+
}
10+
}
11+
12+
void callSetgidAndCheck(int gid) {
13+
if (setgid(gid) != 0) {
14+
exit(1);
15+
}
16+
}
17+
18+
/// Correct ways to drop priv.
19+
20+
void correctDropPrivInline() {
21+
if (setgroups(0, NULL)) {
22+
exit(1);
23+
}
24+
25+
if (setgid(-2) != 0) {
26+
exit(1);
27+
}
28+
29+
if (setuid(-2) != 0) {
30+
exit(1);
31+
}
32+
}
33+
34+
void correctDropPrivInScope() {
35+
{
36+
if (setgroups(0, NULL)) {
37+
exit(1);
38+
}
39+
}
40+
41+
{
42+
if (setgid(-2) != 0) {
43+
exit(1);
44+
}
45+
}
46+
47+
{
48+
if (setuid(-2) != 0) {
49+
exit(1);
50+
}
51+
}
52+
}
53+
54+
void correctOrderForInitgroups() {
55+
struct passwd *pw = getpwuid(0);
56+
if (pw) {
57+
if (initgroups(pw->pw_name, -2)) {
58+
exit(1);
59+
}
60+
} else {
61+
// Unhandled.
62+
}
63+
int rc = setuid(-2);
64+
if (rc) {
65+
exit(1);
66+
}
67+
}
68+
69+
void correctDropPrivInScopeParent() {
70+
{
71+
callSetgidAndCheck(-2);
72+
}
73+
correctOrderForInitgroups();
74+
}
75+
76+
void incorrectNoReturnCodeCheck() {
77+
int user = -2;
78+
if (user) {
79+
if (user) {
80+
int rc = setgid(user);
81+
(void)rc;
82+
initgroups("nobody", user);
83+
}
84+
if (user) {
85+
setuid(user);
86+
}
87+
}
88+
}
89+
90+
void correctDropPrivInFunctionCall() {
91+
if (setgroups(0, NULL)) {
92+
exit(1);
93+
}
94+
95+
callSetgidAndCheck(-2);
96+
callSetuidAndCheck(-2);
97+
}
98+
99+
/// Incorrect, out of order gid and uid.
100+
/// Calling uid before gid will fail.
101+
102+
void incorrectDropPrivOutOfOrderInline() {
103+
if (setuid(-2) != 0) {
104+
exit(1);
105+
}
106+
107+
if (setgid(-2) != 0) {
108+
exit(1);
109+
}
110+
}
111+
112+
void incorrectDropPrivOutOfOrderInScope() {
113+
{
114+
if (setuid(-2) != 0) {
115+
exit(1);
116+
}
117+
}
118+
119+
setgid(-2);
120+
}
121+
122+
void incorrectDropPrivOutOfOrderWithFunction() {
123+
callSetuidAndCheck(-2);
124+
125+
if (setgid(-2) != 0) {
126+
exit(1);
127+
}
128+
}
129+
130+
void incorrectDropPrivOutOfOrderWithFunction2() {
131+
callSetuidAndCheck(-2);
132+
callSetgidAndCheck(-2);
133+
}
134+
135+
void incorrectDropPrivNoCheck() {
136+
setgid(-2);
137+
setuid(-2);
138+
}

‎cpp/ql/src/experimental/Security/CWE/CWE-273/PrivilegeDroppingOutoforder.qhelp

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>The code attempts to drop privilege in an incorrect order by
7+
erroneous dropping user privilege before groups. This has security
8+
impact if the return codes are not checked.</p>
9+
10+
<p>False positives include code performing negative checks, making
11+
sure that setgid or setgroups does not work, meaning permissions are
12+
dropped. Additionally, other forms of sandboxing may be present removing
13+
any residual risk, for example a dedicated user namespace.</p>
14+
15+
</overview>
16+
<recommendation>
17+
18+
<p>Set the new group ID, then set the target user's intended groups by
19+
dropping previous supplemental source groups and initializing target
20+
groups, and finally set the target user.</p>
21+
22+
</recommendation>
23+
<example>
24+
<p>The following example demonstrates out of order calls.</p>
25+
<sample src="PrivilegeDroppingOutoforder.c" />
26+
27+
</example>
28+
<references>
29+
30+
<li>CERT C Coding Standard:
31+
<a href="https://wiki.sei.cmu.edu/confluence/display/c/POS37-C.+Ensure+that+privilege+relinquishment+is+successful">POS37-C. Ensure that privilege relinquishment is successful</a>.
32+
</li>
33+
34+
</references>
35+
</qhelp>

‎cpp/ql/src/experimental/Security/CWE/CWE-273/PrivilegeDroppingOutoforder.ql

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/**
2+
* @name LinuxPrivilegeDroppingOutoforder
3+
* @description A syscall commonly associated with privilege dropping is being called out of order.
4+
* Normally a process drops group ID and sets supplimental groups for the target user
5+
* before setting the target user ID. This can have security impact if the return code
6+
* from these methods is not checked.
7+
* @kind problem
8+
* @problem.severity recommendation
9+
* @id cpp/drop-linux-privileges-outoforder
10+
* @tags security
11+
* external/cwe/cwe-273
12+
* @precision medium
13+
*/
14+
15+
import cpp
16+
17+
predicate argumentMayBeRoot(Expr e) {
18+
e.getValue() = "0" or
19+
e.(VariableAccess).getTarget().getName().toLowerCase().matches("%root%")
20+
}
21+
22+
class SetuidLikeFunctionCall extends FunctionCall {
23+
SetuidLikeFunctionCall() {
24+
(getTarget().hasGlobalName("setuid") or getTarget().hasGlobalName("setresuid")) and
25+
// setuid/setresuid with the root user are false positives.
26+
not argumentMayBeRoot(getArgument(0))
27+
}
28+
}
29+
30+
class SetuidLikeWrapperCall extends FunctionCall {
31+
SetuidLikeFunctionCall baseCall;
32+
33+
SetuidLikeWrapperCall() {
34+
this = baseCall
35+
or
36+
exists(SetuidLikeWrapperCall fc |
37+
this.getTarget() = fc.getEnclosingFunction() and
38+
baseCall = fc.getBaseCall()
39+
)
40+
}
41+
42+
SetuidLikeFunctionCall getBaseCall() { result = baseCall }
43+
}
44+
45+
class CallBeforeSetuidFunctionCall extends FunctionCall {
46+
CallBeforeSetuidFunctionCall() {
47+
(
48+
getTarget().hasGlobalName("setgid") or
49+
getTarget().hasGlobalName("setresgid") or
50+
// Compatibility may require skipping initgroups and setgroups return checks.
51+
// A stricter best practice is to check the result and errnor for EPERM.
52+
getTarget().hasGlobalName("initgroups") or
53+
getTarget().hasGlobalName("setgroups")
54+
) and
55+
// setgid/setresgid/etc with the root group are false positives.
56+
not argumentMayBeRoot(getArgument(0))
57+
}
58+
}
59+
60+
class CallBeforeSetuidWrapperCall extends FunctionCall {
61+
CallBeforeSetuidFunctionCall baseCall;
62+
63+
CallBeforeSetuidWrapperCall() {
64+
this = baseCall
65+
or
66+
exists(CallBeforeSetuidWrapperCall fc |
67+
this.getTarget() = fc.getEnclosingFunction() and
68+
baseCall = fc.getBaseCall()
69+
)
70+
}
71+
72+
CallBeforeSetuidFunctionCall getBaseCall() { result = baseCall }
73+
}
74+
75+
predicate setuidBeforeSetgid(
76+
SetuidLikeWrapperCall setuidWrapper, CallBeforeSetuidWrapperCall setgidWrapper
77+
) {
78+
setgidWrapper.getAPredecessor+() = setuidWrapper
79+
}
80+
81+
predicate isAccessed(FunctionCall fc) {
82+
exists(Variable v | v.getAnAssignedValue() = fc)
83+
or
84+
exists(Operation c | fc = c.getAChild() | c.isCondition())
85+
or
86+
// ignore pattern where result is intentionally ignored by a cast to void.
87+
fc.hasExplicitConversion()
88+
}
89+
90+
from Function func, CallBeforeSetuidFunctionCall fc, SetuidLikeFunctionCall setuid
91+
where
92+
setuidBeforeSetgid(setuid, fc) and
93+
// Require the call return code to be used in a condition or assigned.
94+
// This introduces false negatives where the return is checked but then
95+
// errno == EPERM allows execution to continue.
96+
not isAccessed(fc) and
97+
func = fc.getEnclosingFunction()
98+
select fc,
99+
"This function is called within " + func + ", and potentially after " +
100+
"$@, and may not succeed. Be sure to check the return code and errno, otherwise permissions " +
101+
"may not be dropped.", setuid, setuid.getTarget().getName()

0 commit comments

Comments
 (0)