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

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

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 {

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

clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp

@@ -0,0 +1,197 @@
+//===-- 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 {
+
+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;
+}

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();