[clang][analyzer] Add checker 'security.SetgidSetuidOrder'

[balazske]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/91445.diff

6 Files Affected:

• (modified) clang/docs/analyzer/checkers.rst (+28)
• (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+5)
• (modified) clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt (+1)
• (added) clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp (+196)
• (added) clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h (+15)
• (added) clang/test/Analysis/setgid-setuid-order.c (+170)

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;
+}

[NagyDonat]

It's good to see that this checker is finished. I added several inline comments, but they are not serious issues -- most are connected to CallDescriptions where I'm now very familiar with the available options (and I refactored the code, so others are not so familiar with it...).

[NagyDonat]

Suggested change

	"call is likely to fail; Probably order of these "
	"statements was reversed mistakenly";
	"call is likely to fail; this order is probably a mistake";

[NagyDonat]

StringRef has a subclass StringLiteral that's optimized for compile-time constant strings (this is llvm::StringLiteral -- while clang::StringLiteral is an unrelated AST node class). It would be better to use that (or perhaps const char * if you prefer a lower-level style).

[NagyDonat]

Please use the MatchAs argument of the CallDescription constructor, which was recently introduced by my changes. In this case you'd probably use the matching mode CDM::CLibrary.

Without this these call descriptions would match even a C++ method if it has the right name and number of arguments.

[NagyDonat]

matchesAny is a nice tool, but it would be even better to use a CallDescriptionSet and its contains method, because then you wouldn't need to introduce individual variable names for these call descriptions.

[NagyDonat]

It is very strange that this evalAssume callback doesn't use the parameters Cond and Assumption, but I see that this is apparently a normal thing that also happens in other checkers. Perhaps add a comment that explains this (e.g. something similar to the FIXME in RetainCountChecker if you agree with it).

@haoNoQ @Xazax-hun @steakhal Do you happen to have architectural insight about this situation? How did we end up with a callback where the arguments are mostly unused?

[balazske]

In this case it would be possible to examine what is inside Cond and check for (conj_$11{int, LC2, S1335, #1}) == -1 like things (if possible, the conj_$11 should be equal to the saved symbol of the setuid call), but the test with assumption is much more simple. And I see that the Cond is often just a simple 1 or 0 integer value, this looks interesting and not useful.

[NagyDonat]

@balazske Thanks for the explanation.

This question is still somewhat relevant as an architectural curiosity, but there is no need to change this part of this commit.

[steakhal]

I agree that it has limited usability. We eagerly fold expressions (see eagerly-assume), and it's really hard to recover meaningful context (if possible) from that callback. This holds back the users of this callback.

Now that I grepped for users, they are usually interested in if a symbol gets perfectly constrained; after which they do two things: cleanup some map and/or set some traits.

I think a callback for perfect constraints makes sense, but not for arbitrary assumptions.
On this note: The ExprEngine::processAssume is invoked once for every assume call. However, one assume call might lead to a sequence of re-assume calls, which could individually perfectly constrain a sequence of symbols (due to the merger eqclasses) like here.

[NagyDonat]

Consider using CallDescription::matchesAsWritten(const CallExpr &CE) with the matching mode CDM::CLibrary. It is a bit more permissive than your checks (e.g. it accepts a function that wasn't declared in a system header), but I think it is a good "sane default" heuristic for recognizing C library functions.

[balazske]

I did not observe that CallDescription can be used with CallExpr, it is much simple now. The considered functions are moved into the test file instead of using a new header only for this checker (it was added because system header requirement).

[NagyDonat]

Perhaps place this enum declaration above the checker class. (When I was looking for its definition, I automatically scrolled to the top, and I was surprised that I didn't see it there.)

(This is minor bikeshedding, feel free to ignore it if you feel that it's better to keep this declaration here.)

[NagyDonat]

You forgot to add CDM::CLibrary in the definition of SetuidDesc and SetgidDesc (see the new inline comment).

There are also several inline comments from my previous review where I'm expecting an answer (not necessarily a code change -- in each case I can accept the current solution if you would prefer it).

[NagyDonat]

Suggested change

	const CallDescription SetuidDesc{{"setuid"}, 1};
	const CallDescription SetgidDesc{{"setgid"}, 1};
	const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
	const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};

[NagyDonat]

@balazske Thanks for the explanation.

This question is still somewhat relevant as an architectural curiosity, but there is no need to change this part of this commit.

[balazske]

Is it useful to add a note tag to the previous setuid(getuid()) call? It can be (theoretically) in another function or otherwise in a remote place in the source code.

[gamesh411]

Emitting a note to the location where the first part of the detected pattern (the setuid(getuid()) call) seems like useful information, but this patch is great as it is.
You could also add it in another patch if it is not trivial.

[NagyDonat]

Suggested change

	if (const auto *CE =
	dyn_cast<CallExpr>(Call.getArgExpr(0)->IgnoreParenImpCasts()))
	return Desc.matchesAsWritten(*CE);
	if (const auto *CExpr =
	dyn_cast<CallExpr>(Call.getArgExpr(0)->IgnoreParenImpCasts()))
	return Desc.matchesAsWritten(*CExpr);

I was confused by this for a moment because the name CE is used to denote a CallEvent object by a significant minority of checkers (9 source files, while there are 60 files that use CallEvent &Call like this one). There is also minimal precedent for using CE to denote a CallExpr, but that appears just once, in RetainCountDiagnostics.cpp.

[steakhal]

I wasn't confused as the type was already spelled in the line; and also CallEvents are usually taken by ref, not by a pointer. I have no strong preference.

[steakhal]

On this logic: I'm expecting some tests covering the case when the getuid() call is not directly nested as the argument of setuid(), but rather we have some intermediate variable in between.

[NagyDonat]

Thanks for updating your commit! Now there are only two remaining issues and they are both very minor (marked by inline comments: renaming CallExpr *CE and explaining the reason why "trying to set the gid again" appears as a special case in the SEI-CERT standard).

[steakhal]

I agree that it has limited usability. We eagerly fold expressions (see eagerly-assume), and it's really hard to recover meaningful context (if possible) from that callback. This holds back the users of this callback.

Now that I grepped for users, they are usually interested in if a symbol gets perfectly constrained; after which they do two things: cleanup some map and/or set some traits.

I think a callback for perfect constraints makes sense, but not for arbitrary assumptions.
On this note: The ExprEngine::processAssume is invoked once for every assume call. However, one assume call might lead to a sequence of re-assume calls, which could individually perfectly constrain a sequence of symbols (due to the merger eqclasses) like here.

[steakhal]

Give that the alpha.unix.chroot checker does something similar, I wonder if this checker should share the same parent package with that one to aid discoverability.
WDYT?

[balazske]

Move the new checker into unix, or move the chroot checker into security (or alpha.security)?

[steakhal]

I don't have a preference, I was just asking.
I just felt this is a natural question, and you seem to care about alpha checkers and documentation/discoverability, and this seemed like a subject to discuss. @NagyDonat WDYT?

[steakhal]

I think this looks good now.
I think to really reach the full potential of this checker, we must have a visitor/note tag for highlighting the places of the last setgid or setuid call in the report.

I also think that the docs could be improved.

Anyways. I think it's already pretty good, and I don't plan to do another round of reviews.

[steakhal]

I think the docs should explicitly mention how to fix/suppress this issue.
Like: One should call setgid(getgid()) first and then setuid(getuid()).
Maybe also mention to pay attention to not mix up the APIs so that user ID and group IDs are not swapped, etc. Also add a remark to not forget to check the return value of these APIs.

And this example looks suspiciously similar to the one present in the CERT docs.
Does it raise license and copy-right issues? If that's a concern, please rework the example.

[steakhal]

I was expecting a visitor highlighting the previous setuid call.
Without a visitor, or note tag, the call wouldn't be mentioned in the report.

I think the lack of visitors or note tags was not resolved.

[balazske]

I added the NoteTag support now (instead of a next PR). The checkDeadSymbols is removed, it does really not matter if the data remains in the GDM and this way it is used to display the note tag only for the last setuid call.

[steakhal]

NoteTags, yeey.

Please add tests for the Notes added by the NoteTag callbacks. Inother words, add a test with the text diagnostic output.

[steakhal]

LGTM now, modulo the license concern mentioned here.
I approve this, given that is checked/resolved.

[steakhal]

Suggested change

	// expected-note{{Assuming the condition is false}} \
	// expected-note{{Taking false branch}} \
	// expected-note{{Assuming the condition is false}} \
	// expected-note{{Taking false branch}}
	// expected-note 2 {{Assuming the condition is false}} \
	// expected-note 2 {{Taking false branch}}