https://github.com/balazske created
https://github.com/llvm/llvm-project/pull/91445
None
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] [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;
+}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
