[clang][analyzer] Add checker 'security.SetgidSetuidOrder' (#91445)

balazske · web-flow · commit 11b97da83141 · 2024-05-22T12:11:18.000+02:00
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,47 @@ 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.
+
+.. code-block:: c
+
+ void test1() {
+   // ...
+   // end of section with elevated privileges
+   // reset privileges (user and group) to normal user
+   if (setuid(getuid()) != 0) {
+     handle_error();
+     return;
+   }
+   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) 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>`_.
+
 .. _unix-checkers:
 
 unix
diff --git 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
@@ -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
@@ -0,0 +1,196 @@
+//===-- 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.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===----------------------------------------------------------------------===//
+
+#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 {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+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};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  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;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+                             bool Assumption) const;
+
+private:
+  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,
+                             const CallEvent &Call) const;
+  void emitReport(ProgramStateRef State, CheckerContext &C) const;
+};
+
+} // 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), 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)) {
+    processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+    processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+    processOther(State, Call, C);
+  }
+}
+
+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 != 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 (!FailComparison)
+    return State;
+  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<LastSetPrivilegeCall>(Irrelevant);
+    State = State->set<LastSetuidCallSVal>(SymbolRef{});
+  }
+  return State;
+}
+
+void SetgidSetuidOrderChecker::processSetuid(ProgramStateRef State,
+                                             const CallEvent &Call,
+                                             CheckerContext &C) const {
+  bool IsSetuidWithGetuid = isFunctionCalledInArg(GetuidDesc, Call);
+  if (State->get<LastSetPrivilegeCall>() != Setgid && IsSetuidWithGetuid) {
+    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->BT)
+        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{});
+  C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::processSetgid(ProgramStateRef State,
+                                             const CallEvent &Call,
+                                             CheckerContext &C) const {
+  bool IsSetgidWithGetgid = isFunctionCalledInArg(GetgidDesc, Call);
+  if (State->get<LastSetPrivilegeCall>() == Setuid) {
+    if (IsSetgidWithGetgid) {
+      State = State->set<LastSetPrivilegeCall>(Irrelevant);
+      emitReport(State, C);
+      return;
+    }
+    State = State->set<LastSetPrivilegeCall>(Irrelevant);
+  } else {
+    State = State->set<LastSetPrivilegeCall>(IsSetgidWithGetgid ? Setgid
+                                                                : Irrelevant);
+  }
+  State = State->set<LastSetuidCallSVal>(SymbolRef{});
+  C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::processOther(ProgramStateRef State,
+                                            const CallEvent &Call,
+                                            CheckerContext &C) const {
+  State = State->set<LastSetuidCallSVal>(SymbolRef{});
+  State = State->set<LastSetPrivilegeCall>(Irrelevant);
+  C.addTransition(State);
+}
+
+bool SetgidSetuidOrderChecker::isFunctionCalledInArg(
+    const CallDescription &Desc, const CallEvent &Call) const {
+  if (const auto *CallInArg0 =
+          dyn_cast<CallExpr>(Call.getArgExpr(0)->IgnoreParenImpCasts()))
+    return Desc.matchesAsWritten(*CallInArg0);
+  return false;
+}
+
+void SetgidSetuidOrderChecker::emitReport(ProgramStateRef State,
+                                          CheckerContext &C) const {
+  if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
+    llvm::StringLiteral Msg =
+        "A 'setgid(getgid())' call following a 'setuid(getuid())' "
+        "call is likely to fail; probably the order of these "
+        "statements is wrong";
+    auto Report = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
+    Report->markInteresting(State->get<LastSetuidCallSVal>());
+    C.emitReport(std::move(Report));
+  }
+}
+
+void ento::registerSetgidSetuidOrderChecker(CheckerManager &mgr) {
+  mgr.registerChecker<SetgidSetuidOrderChecker>();
+}
+
+bool ento::shouldRegisterSetgidSetuidOrderChecker(const CheckerManager &mgr) {
+  return true;
+}
diff --git a/clang/test/Analysis/setgid-setuid-order-notes.c b/clang/test/Analysis/setgid-setuid-order-notes.c
@@ -0,0 +1,73 @@
+// 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 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}} \
+                              // 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}}
+    }
+  }
+}
diff --git a/clang/test/Analysis/setgid-setuid-order.c b/clang/test/Analysis/setgid-setuid-order.c
