Skip to content

Commit 8e1a7fe

Browse files
author
Ted Reed
committed
CPP: Add query for CWE-273 that detects out-of-order setuid
1 parent fb1e993 commit 8e1a7fe

File tree

3 files changed

+270
-0
lines changed

3 files changed

+270
-0
lines changed

‎cpp/ql/src/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/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/Security/CWE/CWE-273/PrivilegeDroppingOutoforder.ql

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
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+
import semmle.code.cpp.dataflow.TaintTracking
17+
18+
class SetuidLikeFunctionCall extends FunctionCall {
19+
SetuidLikeFunctionCall() {
20+
// setuid/setresuid with the root user are false positives.
21+
getTarget().hasGlobalName("setuid") or
22+
getTarget().hasGlobalName("setresuid")
23+
}
24+
}
25+
26+
class SetuidLikeWrapperCall extends FunctionCall {
27+
SetuidLikeFunctionCall baseCall;
28+
29+
SetuidLikeWrapperCall() {
30+
this = baseCall or
31+
exists(SetuidLikeWrapperCall fc |
32+
this.getTarget() = fc.getEnclosingFunction() and
33+
baseCall = fc.getBaseCall()
34+
)
35+
}
36+
37+
SetuidLikeFunctionCall getBaseCall() {
38+
result = baseCall
39+
}
40+
}
41+
42+
class CallBeforeSetuidFunctionCall extends FunctionCall {
43+
CallBeforeSetuidFunctionCall() {
44+
// setgid/setresgid with the root group are false positives.
45+
getTarget().hasGlobalName("setgid") or
46+
getTarget().hasGlobalName("setresgid") or
47+
// Compatibility may require skipping initgroups and setgroups return checks.
48+
// A stricter best practice is to check the result and errnor for EPERM.
49+
getTarget().hasGlobalName("initgroups") or
50+
getTarget().hasGlobalName("setgroups")
51+
}
52+
}
53+
54+
class CallBeforeSetuidWrapperCall extends FunctionCall {
55+
CallBeforeSetuidFunctionCall baseCall;
56+
57+
CallBeforeSetuidWrapperCall() {
58+
this = baseCall or
59+
exists(CallBeforeSetuidWrapperCall fc |
60+
this.getTarget() = fc.getEnclosingFunction() and
61+
baseCall = fc.getBaseCall()
62+
)
63+
}
64+
65+
CallBeforeSetuidFunctionCall getBaseCall() {
66+
result = baseCall
67+
}
68+
}
69+
70+
predicate setuidBeforeSetgid(
71+
SetuidLikeWrapperCall setuidWrapper,
72+
CallBeforeSetuidWrapperCall setgidWrapper) {
73+
setgidWrapper.getAPredecessor+() = setuidWrapper
74+
}
75+
76+
predicate flowsToCondition(Expr fc) {
77+
exists(DataFlow::Node source, DataFlow::Node sink |
78+
TaintTracking::localTaint(source, sink) and
79+
fc = source.asExpr() and
80+
(sink.asExpr().getParent*().(ControlFlowNode).isCondition() or sink.asExpr().isCondition())
81+
)
82+
}
83+
84+
from
85+
Function func,
86+
CallBeforeSetuidFunctionCall fc,
87+
SetuidLikeFunctionCall setuid
88+
where
89+
setuidBeforeSetgid(setuid, fc) and
90+
// Require the call return code to be used in a condition.
91+
// This introduces false negatives where the return is checked but then
92+
// errno == EPERM allows execution to continue.
93+
(not flowsToCondition(fc) or not flowsToCondition(setuid)) and
94+
func = fc.getEnclosingFunction()
95+
select fc, "This function is called within " + func + ", and potentially after " +
96+
"$@, and may not succeed. Be sure to check the return code and errno.",
97+
setuid, setuid.getTarget().getName()

0 commit comments

Comments
 (0)