https://github.com/balazske updated
https://github.com/llvm/llvm-project/pull/91445
From d839faf7a30851a172d812137b30635c741870f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com>
Date: Wed, 8 May 2024 10:10:24 +0200
Subject: [PATCH 1/6] [clang][analyzer] Add checker
'Security.SetgidSetuidOrder'.
---
clang/docs/analyzer/checkers.rst | 28 +++
.../clang/StaticAnalyzer/Checkers/Checkers.td | 5 +
.../StaticAnalyzer/Checkers/CMakeLists.txt | 1 +
.../Checkers/SetgidSetuidOrderChecker.cpp | 196 ++++++++++++++++++
.../system-header-simulator-setgid-setuid.h | 15 ++
clang/test/Analysis/setgid-setuid-order.c | 170 +++++++++++++++
6 files changed, 415 insertions(+)
create mode 100644
clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
create mode 100644
clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
create mode 100644 clang/test/Analysis/setgid-setuid-order.c
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0d87df36ced0e..d0c0c7a535c62 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
}
+security.SetgidSetuidOrder (C)
+""""""""""""""""""""""""""""""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C
<https://wiki.sei.cmu.edu/confluence/display/c/POS36-C.+Observe+correct+revocation+order+while+relinquishing+privileges>`_.
+
+.. code-block:: c
+
+ void test1() {
+ if (setuid(getuid()) == -1) {
+ /* handle error condition */
+ }
+ if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+ }
+ }
+
.. _unix-checkers:
unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fd..cc954828901af 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
Dependencies<[SecuritySyntaxChecker]>,
Documentation<HasDocumentation>;
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+ HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and
'setuid(getuid())' (CERT: "
+ "POS36-C)">,
+ Documentation<HasDocumentation>;
+
} // end "security"
let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..45d3788f105dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers
ReturnUndefChecker.cpp
ReturnValueChecker.cpp
RunLoopAutoreleaseLeakChecker.cpp
+ SetgidSetuidOrderChecker.cpp
SimpleStreamChecker.cpp
SmartPtrChecker.cpp
SmartPtrModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
new file mode 100644
index 0000000000000..11cc748cb40b1
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -0,0 +1,196 @@
+//===-- ChrootChecker.cpp - chroot usage checks
---------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines chroot checker, which checks improper use of chroot.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+ : public Checker<check::PostCall, check::DeadSymbols, eval::Assume> {
+ const BugType BT_WrongRevocationOrder{
+ this, "Possible wrong order of privilege revocation"};
+
+ const CallDescription SetuidDesc{{"setuid"}, 1};
+ const CallDescription SetgidDesc{{"setgid"}, 1};
+ const CallDescription SeteuidDesc{{"seteuid"}, 1};
+ const CallDescription SetegidDesc{{"setegid"}, 1};
+ const CallDescription SetreuidDesc{{"setreuid"}, 2};
+ const CallDescription SetregidDesc{{"setregid"}, 2};
+ const CallDescription SetresuidDesc{{"setresuid"}, 3};
+ const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+public:
+ SetgidSetuidOrderChecker() {}
+
+ void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+ void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+ ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+ ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent &Call,
+ CheckerContext &C) const;
+ ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent &Call,
+ CheckerContext &C) const;
+ ProgramStateRef processOther(ProgramStateRef State, const CallEvent &Call,
+ CheckerContext &C) const;
+ /// Check if a function like \c getuid or \c getgid is called directly from
+ /// the first argument of function called from \a Call.
+ bool isFunctionCalledInArg(llvm::StringRef FnName,
+ const CallEvent &Call) const;
+ void emitReport(ProgramStateRef State, CheckerContext &C) const;
+};
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall,
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent &Call,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ if (SetuidDesc.matches(Call)) {
+ State = processSetuid(State, Call, C);
+ } else if (SetgidDesc.matches(Call)) {
+ State = processSetgid(State, Call, C);
+ } else if (matchesAny(Call, SeteuidDesc, SetegidDesc, SetreuidDesc,
+ SetregidDesc, SetresuidDesc, SetresgidDesc)) {
+ State = processOther(State, Call, C);
+ }
+ if (State)
+ C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper &SymReaper,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+
+ SymbolRef LastSetuidSym = State->get<LastSetuidCallSVal>();
+ if (!LastSetuidSym)
+ return;
+
+ if (!SymReaper.isDead(LastSetuidSym))
+ return;
+
+ State = State->set<LastSetuidCallSVal>(SymbolRef{});
+ C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+ bool Assumption) const {
+ SValBuilder &SVB = State->getStateManager().getSValBuilder();
+ SymbolRef LastSetuidSym = State->get<LastSetuidCallSVal>();
+ if (!LastSetuidSym)
+ return State;
+
+ // Check if the most recent call to 'setuid(getuid())' is assumed to be -1.
+ auto FailVal =
+ SVB.evalBinOpNN(State, BO_EQ, nonloc::SymbolVal(LastSetuidSym),
+ SVB.makeIntVal(-1, false), SVB.getConditionType())
+ .getAs<DefinedOrUnknownSVal>();
+ if (!FailVal)
+ return State;
+ auto IsFailBranch = State->assume(*FailVal);
+ if (IsFailBranch.first && !IsFailBranch.second) {
+ // This is the 'setuid(getuid())' == -1 case.
+ // On this branch we do not want to emit warning.
+ State = State->set<LastSetuidCallSVal>(SymbolRef{});
+ State = State->set<LastSetPrivilegeCall>(Irrelevant);
+ }
+ return State;
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::processSetuid(
+ ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const {
+ bool IsSetuidWithGetuid = isFunctionCalledInArg("getuid", Call);
+ if (State->get<LastSetPrivilegeCall>() != Setgid && IsSetuidWithGetuid) {
+ State =
State->set<LastSetuidCallSVal>(Call.getReturnValue().getAsSymbol());
+ return State->set<LastSetPrivilegeCall>(Setuid);
+ }
+ State = State->set<LastSetuidCallSVal>(SymbolRef{});
+ return State->set<LastSetPrivilegeCall>(Irrelevant);
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::processSetgid(
+ ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const {
+ bool IsSetgidWithGetgid = isFunctionCalledInArg("getgid", Call);
+ State = State->set<LastSetuidCallSVal>(SymbolRef{});
+ if (State->get<LastSetPrivilegeCall>() == Setuid) {
+ if (IsSetgidWithGetgid) {
+ State = State->set<LastSetPrivilegeCall>(Irrelevant);
+ emitReport(State, C);
+ // return nullptr to prevent adding transition with the returned state
+ return nullptr;
+ }
+ return State->set<LastSetPrivilegeCall>(Irrelevant);
+ } else
+ return State->set<LastSetPrivilegeCall>(IsSetgidWithGetgid ? Setgid
+ : Irrelevant);
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::processOther(
+ ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const {
+ State = State->set<LastSetuidCallSVal>(SymbolRef{});
+ return State->set<LastSetPrivilegeCall>(Irrelevant);
+}
+
+bool SetgidSetuidOrderChecker::isFunctionCalledInArg(
+ llvm::StringRef FnName, const CallEvent &Call) const {
+ if (const auto *CE = dyn_cast<CallExpr>(Call.getArgExpr(0)))
+ if (const FunctionDecl *FuncD = CE->getDirectCallee())
+ return FuncD->isGlobal() &&
+ FuncD->getASTContext().getSourceManager().isInSystemHeader(
+ FuncD->getCanonicalDecl()->getLocation()) &&
+ FuncD->getNumParams() == 0 && FuncD->getNameAsString() == FnName;
+ return false;
+}
+
+void SetgidSetuidOrderChecker::emitReport(ProgramStateRef State,
+ CheckerContext &C) const {
+ if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
+ StringRef Msg = "A 'setgid(getgid())' call following a 'setuid(getuid())' "
+ "call is likely to fail; Probably order of these "
+ "statements was reversed mistakenly";
+ C.emitReport(std::make_unique<PathSensitiveBugReport>(
+ BT_WrongRevocationOrder, Msg, N));
+ }
+}
+
+void ento::registerSetgidSetuidOrderChecker(CheckerManager &mgr) {
+ mgr.registerChecker<SetgidSetuidOrderChecker>();
+}
+
+bool ento::shouldRegisterSetgidSetuidOrderChecker(const CheckerManager &mgr) {
+ return true;
+}
diff --git a/clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
b/clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
new file mode 100644
index 0000000000000..afacc677ef9d3
--- /dev/null
+++ b/clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
@@ -0,0 +1,15 @@
+#pragma clang system_header
+
+typedef int uid_t;
+typedef int gid_t;
+int setuid(uid_t);
+int setgid(gid_t);
+int seteuid(uid_t);
+int setegid(gid_t);
+int setreuid(uid_t, uid_t);
+int setregid(gid_t, gid_t);
+int setresuid(uid_t, uid_t, uid_t);
+int setresgid(gid_t, gid_t, gid_t);
+
+uid_t getuid();
+gid_t getgid();
diff --git a/clang/test/Analysis/setgid-setuid-order.c
b/clang/test/Analysis/setgid-setuid-order.c
new file mode 100644
index 0000000000000..a8d9ad95a9fe9
--- /dev/null
+++ b/clang/test/Analysis/setgid-setuid-order.c
@@ -0,0 +1,170 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder
-verify %s
+
+#include "Inputs/system-header-simulator-setgid-setuid.h"
+
+void correct_order() {
+ if (setgid(getgid()) == -1)
+ return;
+ if (setuid(getuid()) == -1)
+ return;
+ if (setgid(getgid()) == -1)
+ return;
+}
+
+void incorrect_order() {
+ if (setuid(getuid()) == -1)
+ return;
+ if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
+ return;
+ if (setgid(getgid()) == -1)
+ return;
+}
+
+void warn_at_second_time() {
+ if (setuid(getuid()) == -1)
+ return;
+ if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
+ return;
+ if (setuid(getuid()) == -1)
+ return;
+ if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
+ return;
+}
+
+uid_t f_uid();
+gid_t f_gid();
+
+void setuid_other() {
+ if (setuid(f_uid()) == -1)
+ return;
+ if (setgid(getgid()) == -1)
+ return;
+}
+
+void setgid_other() {
+ if (setuid(getuid()) == -1)
+ return;
+ if (setgid(f_gid()) == -1)
+ return;
+ if (setgid(getgid()) == -1)
+ return;
+}
+
+void setuid_other_between() {
+ if (setuid(getuid()) == -1)
+ return;
+ if (setuid(f_uid()) == -1)
+ return;
+ if (setgid(getgid()) == -1)
+ return;
+}
+
+void setgid_with_getuid() {
+ if (setuid(getuid()) == -1)
+ return;
+ if (setgid(getuid()) == -1)
+ return;
+}
+
+void setuid_with_getgid() {
+ if (setuid(getgid()) == -1)
+ return;
+ if (setgid(getgid()) == -1)
+ return;
+}
+
+int f_setuid() {
+ return setuid(getuid());
+}
+
+int f_setgid() {
+ return setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
+}
+
+void function_calls() {
+ if (f_setuid() == -1)
+ return;
+ if (f_setgid() == -1)
+ return;
+}
+
+void seteuid_between() {
+ if (setuid(getuid()) == -1)
+ return;
+ if (seteuid(getuid()) == -1)
+ return;
+ if (setgid(getgid()) == -1)
+ return;
+}
+
+void setegid_between() {
+ if (setuid(getuid()) == -1)
+ return;
+ if (setegid(getgid()) == -1)
+ return;
+ if (setgid(getgid()) == -1)
+ return;
+}
+
+void setreuid_between() {
+ if (setuid(getuid()) == -1)
+ return;
+ if (setreuid(getuid(), getuid()) == -1)
+ return;
+ if (setgid(getgid()) == -1)
+ return;
+}
+
+void setregid_between() {
+ if (setuid(getuid()) == -1)
+ return;
+ if (setregid(getgid(), getgid()) == -1)
+ return;
+ if (setgid(getgid()) == -1)
+ return;
+}
+
+void setresuid_between() {
+ if (setuid(getuid()) == -1)
+ return;
+ if (setresuid(getuid(), getuid(), getuid()) == -1)
+ return;
+ if (setgid(getgid()) == -1)
+ return;
+}
+
+void setresgid_between() {
+ if (setuid(getuid()) == -1)
+ return;
+ if (setresgid(getgid(), getgid(), getgid()) == -1)
+ return;
+ if (setgid(getgid()) == -1)
+ return;
+}
+
+void other_system_function_between() {
+ if (setuid(getuid()) == -1)
+ return;
+ gid_t g = getgid();
+ if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
+ return;
+}
+
+void f_extern();
+
+void other_unknown_function_between() {
+ if (setuid(getuid()) == -1)
+ return;
+ f_extern();
+ if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
+ return;
+}
+
+void setuid_error_case() {
+ if (setuid(getuid()) == -1) {
+ setgid(getgid());
+ return;
+ }
+ if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
+ return;
+}
From 996c22c0964e51096a229b4cbe7d96d306f2114a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com>
Date: Wed, 8 May 2024 10:34:44 +0200
Subject: [PATCH 2/6] Update checker file header comment
---
.../lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
index 11cc748cb40b1..1a9d0226c320f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -1,4 +1,4 @@
-//===-- ChrootChecker.cpp - chroot usage checks
---------------------------===//
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls
---===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
@@ -6,7 +6,8 @@
//
//===----------------------------------------------------------------------===//
//
-// This file defines chroot checker, which checks improper use of chroot.
+// This file defines a checker to detect possible reversed order of privilege
+// revocations when 'setgid' and 'setuid' is used.
//
//===----------------------------------------------------------------------===//
From 50d996a522659ac80f993c1159b9a2e69f579803 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com>
Date: Wed, 8 May 2024 18:01:16 +0200
Subject: [PATCH 3/6] improved CallDescription usage, updated message
---
.../Checkers/SetgidSetuidOrderChecker.cpp | 21 +++++++++----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
index 1a9d0226c320f..eb0c97217c399 100644
--- a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -33,12 +33,11 @@ class SetgidSetuidOrderChecker
const CallDescription SetuidDesc{{"setuid"}, 1};
const CallDescription SetgidDesc{{"setgid"}, 1};
- const CallDescription SeteuidDesc{{"seteuid"}, 1};
- const CallDescription SetegidDesc{{"setegid"}, 1};
- const CallDescription SetreuidDesc{{"setreuid"}, 2};
- const CallDescription SetregidDesc{{"setregid"}, 2};
- const CallDescription SetresuidDesc{{"setresuid"}, 3};
- const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+ CallDescriptionSet OtherSetPrivilegeDesc{
+ {CDM::CLibrary, {"seteuid"}, 1}, {CDM::CLibrary, {"setegid"}, 1},
+ {CDM::CLibrary, {"setreuid"}, 2}, {CDM::CLibrary, {"setregid"}, 2},
+ {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
public:
SetgidSetuidOrderChecker() {}
@@ -84,8 +83,7 @@ void SetgidSetuidOrderChecker::checkPostCall(const CallEvent
&Call,
State = processSetuid(State, Call, C);
} else if (SetgidDesc.matches(Call)) {
State = processSetgid(State, Call, C);
- } else if (matchesAny(Call, SeteuidDesc, SetegidDesc, SetreuidDesc,
- SetregidDesc, SetresuidDesc, SetresgidDesc)) {
+ } else if (OtherSetPrivilegeDesc.contains(Call)) {
State = processOther(State, Call, C);
}
if (State)
@@ -180,9 +178,10 @@ bool SetgidSetuidOrderChecker::isFunctionCalledInArg(
void SetgidSetuidOrderChecker::emitReport(ProgramStateRef State,
CheckerContext &C) const {
if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
- StringRef Msg = "A 'setgid(getgid())' call following a 'setuid(getuid())' "
- "call is likely to fail; Probably order of these "
- "statements was reversed mistakenly";
+ llvm::StringLiteral Msg =
+ "A 'setgid(getgid())' call following a 'setuid(getuid())' "
+ "call is likely to fail; probably the order of these "
+ "statements is wrong";
C.emitReport(std::make_unique<PathSensitiveBugReport>(
BT_WrongRevocationOrder, Msg, N));
}
From 49b3fe96bc7538c2715c601dc6d1cd92382d4cd1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com>
Date: Fri, 10 May 2024 15:01:13 +0200
Subject: [PATCH 4/6] added missing 'CDM::CLibrary'
---
.../lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
index eb0c97217c399..4ead0d0b2d529 100644
--- a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -31,8 +31,8 @@ class SetgidSetuidOrderChecker
const BugType BT_WrongRevocationOrder{
this, "Possible wrong order of privilege revocation"};
- const CallDescription SetuidDesc{{"setuid"}, 1};
- const CallDescription SetgidDesc{{"setgid"}, 1};
+ const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+ const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
CallDescriptionSet OtherSetPrivilegeDesc{
{CDM::CLibrary, {"seteuid"}, 1}, {CDM::CLibrary, {"setegid"}, 1},
From be329ce28fb75c53389e8ef60f94f9473ab216d4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com>
Date: Fri, 10 May 2024 18:10:46 +0200
Subject: [PATCH 5/6] more review related fixes
---
.../Checkers/SetgidSetuidOrderChecker.cpp | 24 +++++++++----------
.../system-header-simulator-setgid-setuid.h | 15 ------------
clang/test/Analysis/setgid-setuid-order.c | 17 ++++++++++++-
3 files changed, 28 insertions(+), 28 deletions(-)
delete mode 100644
clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
index 4ead0d0b2d529..695f20410851c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -26,6 +26,8 @@ using namespace ento;
namespace {
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
class SetgidSetuidOrderChecker
: public Checker<check::PostCall, check::DeadSymbols, eval::Assume> {
const BugType BT_WrongRevocationOrder{
@@ -34,6 +36,9 @@ class SetgidSetuidOrderChecker
const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+ const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+ const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
CallDescriptionSet OtherSetPrivilegeDesc{
{CDM::CLibrary, {"seteuid"}, 1}, {CDM::CLibrary, {"setegid"}, 1},
{CDM::CLibrary, {"setreuid"}, 2}, {CDM::CLibrary, {"setregid"}, 2},
@@ -56,13 +61,11 @@ class SetgidSetuidOrderChecker
CheckerContext &C) const;
/// Check if a function like \c getuid or \c getgid is called directly from
/// the first argument of function called from \a Call.
- bool isFunctionCalledInArg(llvm::StringRef FnName,
+ bool isFunctionCalledInArg(const CallDescription &Desc,
const CallEvent &Call) const;
void emitReport(ProgramStateRef State, CheckerContext &C) const;
};
-enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
-
} // end anonymous namespace
/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
@@ -132,7 +135,7 @@ ProgramStateRef
SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
ProgramStateRef SetgidSetuidOrderChecker::processSetuid(
ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const {
- bool IsSetuidWithGetuid = isFunctionCalledInArg("getuid", Call);
+ bool IsSetuidWithGetuid = isFunctionCalledInArg(GetuidDesc, Call);
if (State->get<LastSetPrivilegeCall>() != Setgid && IsSetuidWithGetuid) {
State =
State->set<LastSetuidCallSVal>(Call.getReturnValue().getAsSymbol());
return State->set<LastSetPrivilegeCall>(Setuid);
@@ -143,7 +146,7 @@ ProgramStateRef SetgidSetuidOrderChecker::processSetuid(
ProgramStateRef SetgidSetuidOrderChecker::processSetgid(
ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const {
- bool IsSetgidWithGetgid = isFunctionCalledInArg("getgid", Call);
+ bool IsSetgidWithGetgid = isFunctionCalledInArg(GetgidDesc, Call);
State = State->set<LastSetuidCallSVal>(SymbolRef{});
if (State->get<LastSetPrivilegeCall>() == Setuid) {
if (IsSetgidWithGetgid) {
@@ -165,13 +168,10 @@ ProgramStateRef SetgidSetuidOrderChecker::processOther(
}
bool SetgidSetuidOrderChecker::isFunctionCalledInArg(
- llvm::StringRef FnName, const CallEvent &Call) const {
- if (const auto *CE = dyn_cast<CallExpr>(Call.getArgExpr(0)))
- if (const FunctionDecl *FuncD = CE->getDirectCallee())
- return FuncD->isGlobal() &&
- FuncD->getASTContext().getSourceManager().isInSystemHeader(
- FuncD->getCanonicalDecl()->getLocation()) &&
- FuncD->getNumParams() == 0 && FuncD->getNameAsString() == FnName;
+ const CallDescription &Desc, const CallEvent &Call) const {
+ if (const auto *CE =
+ dyn_cast<CallExpr>(Call.getArgExpr(0)->IgnoreParenImpCasts()))
+ return Desc.matchesAsWritten(*CE);
return false;
}
diff --git a/clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
b/clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
deleted file mode 100644
index afacc677ef9d3..0000000000000
--- a/clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
+++ /dev/null
@@ -1,15 +0,0 @@
-#pragma clang system_header
-
-typedef int uid_t;
-typedef int gid_t;
-int setuid(uid_t);
-int setgid(gid_t);
-int seteuid(uid_t);
-int setegid(gid_t);
-int setreuid(uid_t, uid_t);
-int setregid(gid_t, gid_t);
-int setresuid(uid_t, uid_t, uid_t);
-int setresgid(gid_t, gid_t, gid_t);
-
-uid_t getuid();
-gid_t getgid();
diff --git a/clang/test/Analysis/setgid-setuid-order.c
b/clang/test/Analysis/setgid-setuid-order.c
index a8d9ad95a9fe9..35c2cdaa947cf 100644
--- a/clang/test/Analysis/setgid-setuid-order.c
+++ b/clang/test/Analysis/setgid-setuid-order.c
@@ -1,6 +1,21 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder
-verify %s
-#include "Inputs/system-header-simulator-setgid-setuid.h"
+typedef int uid_t;
+typedef int gid_t;
+
+int setuid(uid_t);
+int setgid(gid_t);
+int seteuid(uid_t);
+int setegid(gid_t);
+int setreuid(uid_t, uid_t);
+int setregid(gid_t, gid_t);
+int setresuid(uid_t, uid_t, uid_t);
+int setresgid(gid_t, gid_t, gid_t);
+
+uid_t getuid();
+gid_t getgid();
+
+
void correct_order() {
if (setgid(getgid()) == -1)
From ae31b7bb7c98d69755645e4ab37f2b95326661b3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.k...@ericsson.com>
Date: Tue, 14 May 2024 09:28:24 +0200
Subject: [PATCH 6/6] updates according to new review
---
.../Checkers/SetgidSetuidOrderChecker.cpp | 43 +++++-----
clang/test/Analysis/setgid-setuid-order.c | 80 ++++++++++++++++++-
2 files changed, 98 insertions(+), 25 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
index 695f20410851c..18a8f4455aa33 100644
--- a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -30,8 +30,7 @@ enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
class SetgidSetuidOrderChecker
: public Checker<check::PostCall, check::DeadSymbols, eval::Assume> {
- const BugType BT_WrongRevocationOrder{
- this, "Possible wrong order of privilege revocation"};
+ const BugType BT{this, "Possible wrong order of privilege revocation"};
const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
@@ -39,14 +38,12 @@ class SetgidSetuidOrderChecker
const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
- CallDescriptionSet OtherSetPrivilegeDesc{
+ CallDescriptionSet const OtherSetPrivilegeDesc{
{CDM::CLibrary, {"seteuid"}, 1}, {CDM::CLibrary, {"setegid"}, 1},
{CDM::CLibrary, {"setreuid"}, 2}, {CDM::CLibrary, {"setregid"}, 2},
{CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
public:
- SetgidSetuidOrderChecker() {}
-
void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
@@ -105,7 +102,7 @@ void
SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper &SymReaper,
return;
State = State->set<LastSetuidCallSVal>(SymbolRef{});
- C.addTransition(State, C.getPredecessor());
+ C.addTransition(State);
}
ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
@@ -116,16 +113,21 @@ ProgramStateRef
SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
if (!LastSetuidSym)
return State;
- // Check if the most recent call to 'setuid(getuid())' is assumed to be -1.
- auto FailVal =
- SVB.evalBinOpNN(State, BO_EQ, nonloc::SymbolVal(LastSetuidSym),
- SVB.makeIntVal(-1, false), SVB.getConditionType())
+ // Check if the most recent call to 'setuid(getuid())' is assumed to be != 0.
+ // It should be only -1 at failure, but we want to accept a "!= 0" check too.
+ // (But now an invalid failure check like "!= 1" will be recognized as
correct
+ // too. The "invalid failure check" is a different bug that is not the scope
+ // of this checker.)
+ auto FailComparison =
+ SVB.evalBinOpNN(State, BO_NE, nonloc::SymbolVal(LastSetuidSym),
+ SVB.makeIntVal(0, /*isUnsigned=*/false),
+ SVB.getConditionType())
.getAs<DefinedOrUnknownSVal>();
- if (!FailVal)
+ if (!FailComparison)
return State;
- auto IsFailBranch = State->assume(*FailVal);
- if (IsFailBranch.first && !IsFailBranch.second) {
- // This is the 'setuid(getuid())' == -1 case.
+ if (auto IsFailBranch = State->assume(*FailComparison);
+ IsFailBranch.first && !IsFailBranch.second) {
+ // This is the 'setuid(getuid())' != 0 case.
// On this branch we do not want to emit warning.
State = State->set<LastSetuidCallSVal>(SymbolRef{});
State = State->set<LastSetPrivilegeCall>(Irrelevant);
@@ -156,9 +158,9 @@ ProgramStateRef SetgidSetuidOrderChecker::processSetgid(
return nullptr;
}
return State->set<LastSetPrivilegeCall>(Irrelevant);
- } else
- return State->set<LastSetPrivilegeCall>(IsSetgidWithGetgid ? Setgid
- : Irrelevant);
+ }
+ return State->set<LastSetPrivilegeCall>(IsSetgidWithGetgid ? Setgid
+ : Irrelevant);
}
ProgramStateRef SetgidSetuidOrderChecker::processOther(
@@ -169,9 +171,9 @@ ProgramStateRef SetgidSetuidOrderChecker::processOther(
bool SetgidSetuidOrderChecker::isFunctionCalledInArg(
const CallDescription &Desc, const CallEvent &Call) const {
- if (const auto *CE =
+ if (const auto *CallInArg0 =
dyn_cast<CallExpr>(Call.getArgExpr(0)->IgnoreParenImpCasts()))
- return Desc.matchesAsWritten(*CE);
+ return Desc.matchesAsWritten(*CallInArg0);
return false;
}
@@ -182,8 +184,7 @@ void SetgidSetuidOrderChecker::emitReport(ProgramStateRef
State,
"A 'setgid(getgid())' call following a 'setuid(getuid())' "
"call is likely to fail; probably the order of these "
"statements is wrong";
- C.emitReport(std::make_unique<PathSensitiveBugReport>(
- BT_WrongRevocationOrder, Msg, N));
+ C.emitReport(std::make_unique<PathSensitiveBugReport>(BT, Msg, N));
}
}
diff --git a/clang/test/Analysis/setgid-setuid-order.c
b/clang/test/Analysis/setgid-setuid-order.c
index 35c2cdaa947cf..1c411aa6a27b5 100644
--- a/clang/test/Analysis/setgid-setuid-order.c
+++ b/clang/test/Analysis/setgid-setuid-order.c
@@ -18,14 +18,31 @@ gid_t getgid();
void correct_order() {
+ // A correct revocation sequence starts here.
if (setgid(getgid()) == -1)
return;
if (setuid(getuid()) == -1)
return;
+ // No warning for the following setgid statement.
+ // The previous setgid and setuid calls are a correct privilege revocation
+ // sequence. The checker does not care about the following statements (except
+ // if a wrong setuid-setgid sequence follows again).
if (setgid(getgid()) == -1)
return;
}
+void incorrect_after_correct() {
+ if (setgid(getgid()) == -1)
+ return;
+ if (setuid(getuid()) == -1)
+ return;
+ // Incorrect sequence starts here.
+ if (setuid(getuid()) == -1)
+ return;
+ if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
+ return;
+}
+
void incorrect_order() {
if (setuid(getuid()) == -1)
return;
@@ -77,11 +94,13 @@ void setuid_other_between() {
void setgid_with_getuid() {
if (setuid(getuid()) == -1)
return;
+ // add a clang-tidy check for this case?
if (setgid(getuid()) == -1)
return;
}
void setuid_with_getgid() {
+ // add a clang-tidy check for this case?
if (setuid(getgid()) == -1)
return;
if (setgid(getgid()) == -1)
@@ -157,14 +176,25 @@ void setresgid_between() {
return;
}
-void other_system_function_between() {
+void getgid_getuid_between() {
if (setuid(getuid()) == -1)
return;
- gid_t g = getgid();
+ (void)getgid();
+ (void)getuid();
if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
return;
}
+void stored_getgid_getuid() {
+ // possible future improvement: detect this case
+ uid_t u = getuid();
+ gid_t g = getgid();
+ if (setuid(u) == -1)
+ return;
+ if (setgid(g) == -1) // no warning
+ return;
+}
+
void f_extern();
void other_unknown_function_between() {
@@ -177,9 +207,51 @@ void other_unknown_function_between() {
void setuid_error_case() {
if (setuid(getuid()) == -1) {
- setgid(getgid());
+ // No warning if we know that the first setuid call has failed.
+ (void)setgid(getgid());
return;
}
- if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
+ (void)setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
+}
+
+void setuid_success_case() {
+ if (setuid(getuid()) == 0) {
+ if (setgid(getgid()) == 0) { // expected-warning{{A 'setgid(getgid())'
call following a 'setuid(getuid())' call is likely to fail}}
+ }
+ }
+}
+
+void incorrect_order_compare_zero() {
+ if (setuid(getuid()) != 0)
+ return;
+ (void)setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
+}
+
+void setuid_error_case_compare_zero() {
+ if (setuid(getuid()) != 0) {
+ // No warning if we know that the first setuid call has failed.
+ (void)setgid(getgid());
+ return;
+ }
+}
+
+void incorrect_order_compare_other() {
+ if (setuid(getuid()) == -2) {
+ // This is a case for improvement:
+ // The checker does not recognize that this is an invalid error check,
+ // but this is really another type of bug not related to this checker.
+ (void)setgid(getgid()); // warning should appear here
+ return;
+ }
+ if (setgid(getgid()) == -2) { // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
+ return;
+ }
+}
+
+const int FAIL = -1;
+
+void incorrect_order_compare_var() {
+ if (setuid(getuid()) == FAIL)
return;
+ (void)setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call
following a 'setuid(getuid())' call is likely to fail}}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
