From d839faf7a30851a172d812137b30635c741870f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri@ericsson.com>
Date: Wed, 8 May 2024 10:10:24 +0200
Subject: [PATCH 01/10] [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 0d87df36ced0e91..d0c0c7a535c6242 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 520286b57c9fdcb..cc954828901af1a 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 4443ffd09293881..45d3788f105dc9c 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 000000000000000..11cc748cb40b16b
--- /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 000000000000000..afacc677ef9d388
--- /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 000000000000000..a8d9ad95a9fe9a1
--- /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.keri@ericsson.com>
Date: Wed, 8 May 2024 10:34:44 +0200
Subject: [PATCH 02/10] 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 11cc748cb40b16b..1a9d0226c320f26 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.keri@ericsson.com>
Date: Wed, 8 May 2024 18:01:16 +0200
Subject: [PATCH 03/10] 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 1a9d0226c320f26..eb0c97217c3996f 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.keri@ericsson.com>
Date: Fri, 10 May 2024 15:01:13 +0200
Subject: [PATCH 04/10] 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 eb0c97217c3996f..4ead0d0b2d529cd 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.keri@ericsson.com>
Date: Fri, 10 May 2024 18:10:46 +0200
Subject: [PATCH 05/10] 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 4ead0d0b2d529cd..695f20410851cbd 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 afacc677ef9d388..000000000000000
--- 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 a8d9ad95a9fe9a1..35c2cdaa947cf4e 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.keri@ericsson.com>
Date: Tue, 14 May 2024 09:28:24 +0200
Subject: [PATCH 06/10] 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 695f20410851cbd..18a8f4455aa332c 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 35c2cdaa947cf4e..1c411aa6a27b5a3 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}}
 }

From 5f97049dcc3e6c4a79a2c61a8ad6bc1820450b6a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri@ericsson.com>
Date: Wed, 15 May 2024 17:01:41 +0200
Subject: [PATCH 07/10] adding NoteTag support

---
 clang/docs/analyzer/checkers.rst              | 25 +++--
 .../clang/StaticAnalyzer/Checkers/Checkers.td |  4 +-
 .../Checkers/SetgidSetuidOrderChecker.cpp     | 98 +++++++++----------
 3 files changed, 70 insertions(+), 57 deletions(-)

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index d0c0c7a535c6242..5cb9deacb125fa8 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1194,19 +1194,32 @@ 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 */
+   // ...
+   // end of section with elevated privileges
+   // reset privileges (user and group) to normal user
+   if (setuid(getuid()) != 0) {
+     handle_error();
+     return;
    }
-   if (setgid(getgid()) == -1) { // warn
-     /* handle error condition */
+   if (setgid(getgid()) != 0) { // warning: A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail
+     handle_error();
+     return;
    }
+   // user-ID and group-ID are reset to normal user now
+   // ...
  }
 
+In the code above the problem is that ``setuid(getuid())`` removes superuser
+privileges before ``setgid(getgid())`` is called. To fix the problem the
+``setgid(getgid())`` should be called first. Further attention is needed to
+avoid code like ``setgid(getuid())`` (this checker does not detect bugs like
+this).
+
+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>`_.
+
 .. _unix-checkers:
 
 unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index cc954828901af1a..8f261a28825550c 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1012,8 +1012,8 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Documentation<HasDocumentation>;
 
 def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
-  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 'setuid(getuid())' (CERT: "
-           "POS36-C)">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and "
+           "'setuid(getuid())' (CERT: POS36-C)">,
   Documentation<HasDocumentation>;
 
 } // end "security"
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
index 18a8f4455aa332c..7740b3bab65cad0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -28,8 +28,7 @@ namespace {
 
 enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
 
-class SetgidSetuidOrderChecker
-    : public Checker<check::PostCall, check::DeadSymbols, eval::Assume> {
+class SetgidSetuidOrderChecker : public Checker<check::PostCall, eval::Assume> {
   const BugType BT{this, "Possible wrong order of privilege revocation"};
 
   const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
@@ -38,24 +37,24 @@ class SetgidSetuidOrderChecker
   const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
   const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
 
-  CallDescriptionSet const OtherSetPrivilegeDesc{
+  const 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:
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
-  void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
   ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
                              bool Assumption) const;
+  const BugType *getBT() const { return &BT; }
 
 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;
+  void processSetuid(ProgramStateRef State, const CallEvent &Call,
+                     CheckerContext &C) const;
+  void processSetgid(ProgramStateRef State, const CallEvent &Call,
+                     CheckerContext &C) const;
+  void 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(const CallDescription &Desc,
@@ -73,36 +72,20 @@ class SetgidSetuidOrderChecker
 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).
+/// (which is the failure branch of the call), and for identification of note
+/// tags.
 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);
+    processSetuid(State, Call, C);
   } else if (SetgidDesc.matches(Call)) {
-    State = processSetgid(State, Call, C);
+    processSetgid(State, Call, C);
   } else if (OtherSetPrivilegeDesc.contains(Call)) {
-    State = processOther(State, Call, C);
+    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);
 }
 
 ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
@@ -129,44 +112,59 @@ ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
       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);
+    State = State->set<LastSetuidCallSVal>(SymbolRef{});
   }
   return State;
 }
 
-ProgramStateRef SetgidSetuidOrderChecker::processSetuid(
-    ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const {
+void SetgidSetuidOrderChecker::processSetuid(ProgramStateRef State,
+                                             const CallEvent &Call,
+                                             CheckerContext &C) const {
   bool IsSetuidWithGetuid = isFunctionCalledInArg(GetuidDesc, Call);
   if (State->get<LastSetPrivilegeCall>() != Setgid && IsSetuidWithGetuid) {
-    State = State->set<LastSetuidCallSVal>(Call.getReturnValue().getAsSymbol());
-    return State->set<LastSetPrivilegeCall>(Setuid);
+    SymbolRef RetSym = Call.getReturnValue().getAsSymbol();
+    State = State->set<LastSetPrivilegeCall>(Setuid);
+    State = State->set<LastSetuidCallSVal>(RetSym);
+    const NoteTag *Note = C.getNoteTag([this,
+                                        RetSym](PathSensitiveBugReport &BR) {
+      if (!BR.isInteresting(RetSym) || &BR.getBugType() != this->getBT())
+        return "";
+      return "Call to 'setuid' found here that removes superuser privileges";
+    });
+    C.addTransition(State, Note);
+    return;
   }
+  State = State->set<LastSetPrivilegeCall>(Irrelevant);
   State = State->set<LastSetuidCallSVal>(SymbolRef{});
-  return State->set<LastSetPrivilegeCall>(Irrelevant);
+  C.addTransition(State);
 }
 
-ProgramStateRef SetgidSetuidOrderChecker::processSetgid(
-    ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const {
+void SetgidSetuidOrderChecker::processSetgid(ProgramStateRef State,
+                                             const CallEvent &Call,
+                                             CheckerContext &C) const {
   bool IsSetgidWithGetgid = isFunctionCalledInArg(GetgidDesc, 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;
     }
-    return State->set<LastSetPrivilegeCall>(Irrelevant);
+    State = State->set<LastSetPrivilegeCall>(Irrelevant);
+  } else {
+    State = State->set<LastSetPrivilegeCall>(IsSetgidWithGetgid ? Setgid
+                                                                : Irrelevant);
   }
-  return State->set<LastSetPrivilegeCall>(IsSetgidWithGetgid ? Setgid
-                                                             : Irrelevant);
+  State = State->set<LastSetuidCallSVal>(SymbolRef{});
+  C.addTransition(State);
 }
 
-ProgramStateRef SetgidSetuidOrderChecker::processOther(
-    ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const {
+void SetgidSetuidOrderChecker::processOther(ProgramStateRef State,
+                                            const CallEvent &Call,
+                                            CheckerContext &C) const {
   State = State->set<LastSetuidCallSVal>(SymbolRef{});
-  return State->set<LastSetPrivilegeCall>(Irrelevant);
+  State = State->set<LastSetPrivilegeCall>(Irrelevant);
+  C.addTransition(State);
 }
 
 bool SetgidSetuidOrderChecker::isFunctionCalledInArg(
@@ -184,7 +182,9 @@ 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, Msg, N));
+    auto Report = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
+    Report->markInteresting(State->get<LastSetuidCallSVal>());
+    C.emitReport(std::move(Report));
   }
 }
 

From 14939785ba71d05fcc68b916c306b3b288d747c8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri@ericsson.com>
Date: Thu, 16 May 2024 10:11:51 +0200
Subject: [PATCH 08/10] added test file, removed getBT

---
 .../Checkers/SetgidSetuidOrderChecker.cpp     |  3 +-
 .../test/Analysis/setgid-setuid-order-notes.c | 75 +++++++++++++++++++
 2 files changed, 76 insertions(+), 2 deletions(-)
 create mode 100644 clang/test/Analysis/setgid-setuid-order-notes.c

diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
index 7740b3bab65cad0..dbe3fd33a6b43fc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -46,7 +46,6 @@ class SetgidSetuidOrderChecker : public Checker<check::PostCall, eval::Assume> {
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
   ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
                              bool Assumption) const;
-  const BugType *getBT() const { return &BT; }
 
 private:
   void processSetuid(ProgramStateRef State, const CallEvent &Call,
@@ -128,7 +127,7 @@ void SetgidSetuidOrderChecker::processSetuid(ProgramStateRef State,
     State = State->set<LastSetuidCallSVal>(RetSym);
     const NoteTag *Note = C.getNoteTag([this,
                                         RetSym](PathSensitiveBugReport &BR) {
-      if (!BR.isInteresting(RetSym) || &BR.getBugType() != this->getBT())
+      if (!BR.isInteresting(RetSym) || &BR.getBugType() != &this->BT)
         return "";
       return "Call to 'setuid' found here that removes superuser privileges";
     });
diff --git a/clang/test/Analysis/setgid-setuid-order-notes.c b/clang/test/Analysis/setgid-setuid-order-notes.c
new file mode 100644
index 000000000000000..28b476cfc2a104c
--- /dev/null
+++ b/clang/test/Analysis/setgid-setuid-order-notes.c
@@ -0,0 +1,75 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder -analyzer-output=text -verify %s
+
+typedef int uid_t;
+typedef int gid_t;
+
+int setuid(uid_t);
+int setgid(gid_t);
+
+uid_t getuid();
+gid_t getgid();
+
+
+
+void test_note_1() {
+  if (setuid(getuid()) == -1) // expected-note{{Assuming the condition is false}} \
+                              // expected-note{{Taking false branch}}
+    return;
+  if (setuid(getuid()) == -1) // expected-note{{Call to 'setuid' found here that removes superuser privileges}} \
+                              // expected-note{{Assuming the condition is false}} \
+                              // expected-note{{Taking false branch}}
+    return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \
+                              // expected-note{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}}
+    return;
+}
+
+void test_note_2() {
+  if (setuid(getuid()) == -1) // expected-note{{Call to 'setuid' found here that removes superuser privileges}} \
+                              // expected-note{{Assuming the condition is false}} \
+                              // expected-note{{Taking false branch}} \
+                              // expected-note{{Assuming the condition is false}} \
+                              // expected-note{{Taking false branch}}
+    return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \
+                              // expected-note{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \
+                              // expected-note{{Assuming the condition is false}} \
+                              // expected-note{{Taking false branch}}
+    return;
+  if (setuid(getuid()) == -1) // expected-note{{Call to 'setuid' found here that removes superuser privileges}} \
+                              // expected-note{{Assuming the condition is false}} \
+                              // expected-note{{Taking false branch}}
+    return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \
+                              // expected-note{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}}
+    return;
+}
+
+int f_setuid() {
+  return setuid(getuid()); // expected-note{{Call to 'setuid' found here that removes superuser privileges}}
+}
+
+int f_setgid() {
+  return setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \
+                           // expected-note{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}}
+}
+
+void test_note_3() {
+  if (f_setuid() == -1) // expected-note{{Assuming the condition is false}} \
+                        // expected-note{{Calling 'f_setuid'}} \
+                        // expected-note{{Returning from 'f_setuid'}} \
+                        // expected-note{{Taking false branch}}
+    return;
+  if (f_setgid() == -1) // expected-note{{Calling 'f_setgid'}}
+    return;
+}
+
+void test_note_4() {
+  if (setuid(getuid()) == 0) {   // expected-note{{Assuming the condition is true}} \
+                                 // expected-note{{Call to 'setuid' found here that removes superuser privileges}} \
+                                 // expected-note{{Taking true branch}}
+    if (setgid(getgid()) == 0) { // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \
+                                 // expected-note{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}}
+    }
+  }
+}

From 96b340a27105640d63fe93a75139578694e969ca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri@ericsson.com>
Date: Thu, 16 May 2024 15:13:27 +0200
Subject: [PATCH 09/10] improved repeated test comments

---
 clang/test/Analysis/setgid-setuid-order-notes.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/clang/test/Analysis/setgid-setuid-order-notes.c b/clang/test/Analysis/setgid-setuid-order-notes.c
index 28b476cfc2a104c..03402413581c640 100644
--- a/clang/test/Analysis/setgid-setuid-order-notes.c
+++ b/clang/test/Analysis/setgid-setuid-order-notes.c
@@ -26,10 +26,8 @@ void test_note_1() {
 
 void test_note_2() {
   if (setuid(getuid()) == -1) // expected-note{{Call to 'setuid' found here that removes superuser privileges}} \
-                              // 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}}
     return;
   if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \
                               // expected-note{{A 'setgid(getgid())' call following a 'setuid(getuid())' call is likely to fail}} \

From 704cba8ae1fa8c2dc64a87634c5edeba05d14e27 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri@ericsson.com>
Date: Wed, 22 May 2024 12:07:47 +0200
Subject: [PATCH 10/10] Update checkers.rst

very small change about checking of return value
---
 clang/docs/analyzer/checkers.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 5cb9deacb125fa8..4d08e9665f686a7 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1216,7 +1216,7 @@ In the code above the problem is that ``setuid(getuid())`` removes superuser
 privileges before ``setgid(getgid())`` is called. To fix the problem the
 ``setgid(getgid())`` should be called first. Further attention is needed to
 avoid code like ``setgid(getuid())`` (this checker does not detect bugs like
-this).
+this) and always check the return value of these calls.
 
 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>`_.