[analyzer] Modernize, improve and promote chroot checker

clang/docs/ReleaseNotes.rst

@@ -985,6 +985,16 @@ Moved checkers
   original checkers were implemented only using AST matching and make more
   sense as a single clang-tidy check.
 
+- The checker ``alpha.unix.Chroot`` was modernized, improved and moved to
+  ``unix.Chroot``. Testing was done on open source projects that use chroot(),
+  and false issues addressed in the improvements based on real use cases. Open
+  source projects used for testing include nsjail, lxroot, dive and ruri.
+  This checker conforms to SEI Cert C recommendation `POS05-C. Limit access to
+  files by creating a jail
+  <https://wiki.sei.cmu.edu/confluence/display/c/POS05-C.+Limit+access+to+files+by+creating+a+jail>`_.
+  Fixes (#GH34697).
+  (#GH117791) [Documentation](https://clang.llvm.org/docs/analyzer/checkers.html#unix-chroot-c).
+
 .. _release-notes-sanitizers:
 
 Sanitizers

clang/docs/analyzer/checkers.rst

@@ -1750,6 +1750,37 @@ Critical section handling functions modeled by this checker:
    }
  }
 
+.. _unix-Chroot:
+
+unix.Chroot (C)
+"""""""""""""""
+Check improper use of chroot described by SEI Cert C recommendation `POS05-C.
+Limit access to files by creating a jail
+<https://wiki.sei.cmu.edu/confluence/display/c/POS05-C.+Limit+access+to+files+by+creating+a+jail>`_.
+The checker finds usage patterns where ``chdir("/")`` is not called immediately
+after a call to ``chroot(path)``.
+
+.. code-block:: c
+
+ void f();
+
+ void test_bad() {
+   chroot("/usr/local");
+   f(); // warn: no call of chdir("/") immediately after chroot
+ }
+
+  void test_bad_path() {
+    chroot("/usr/local");
+    chdir("/usr"); // warn: no call of chdir("/") immediately after chroot
+    f();
+  }
+
+ void test_good() {
+   chroot("/usr/local");
+   chdir("/"); // no warning
+   f();
+ }
+
 .. _unix-Errno:
 
 unix.Errno (C)
@@ -3275,21 +3306,6 @@ SEI CERT checkers which tries to find errors based on their `C coding rules <htt
 alpha.unix
 ^^^^^^^^^^
 
-.. _alpha-unix-Chroot:
-
-alpha.unix.Chroot (C)
-"""""""""""""""""""""
-Check improper use of chroot.
-
-.. code-block:: c
-
- void f();
-
- void test() {
-   chroot("/usr/local");
-   f(); // warn: no call of chdir("/") immediately after chroot
- }
-
 .. _alpha-unix-PthreadLock:
 
 alpha.unix.PthreadLock (C)

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

@@ -598,14 +598,14 @@ def VforkChecker : Checker<"Vfork">,
   HelpText<"Check for proper usage of vfork">,
   Documentation<HasDocumentation>;
 
-} // end "unix"
-
-let ParentPackage = UnixAlpha in {
-
 def ChrootChecker : Checker<"Chroot">,
   HelpText<"Check improper use of chroot">,
   Documentation<HasDocumentation>;
 
+} // end "unix"
+
+let ParentPackage = UnixAlpha in {
+
 def PthreadLockChecker : Checker<"PthreadLock">,
   HelpText<"Simple lock -> unlock checker">,
   Dependencies<[PthreadLockBase]>,

clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp

@@ -7,9 +7,13 @@
 //===----------------------------------------------------------------------===//
 //
 //  This file defines chroot checker, which checks improper use of chroot.
+//  This is described by the SEI Cert C rule POS05-C.
+//  The checker is a warning not a hard failure since it only checks for a
+//  recommended rule.
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/ASTContext.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
@@ -25,117 +29,155 @@ using namespace clang;
 using namespace ento;
 
 namespace {
+enum ChrootKind { NO_CHROOT, ROOT_CHANGED, ROOT_CHANGE_FAILED, JAIL_ENTERED };
+} // namespace
 
-// enum value that represent the jail state
-enum Kind { NO_CHROOT, ROOT_CHANGED, JAIL_ENTERED };
-
-bool isRootChanged(intptr_t k) { return k == ROOT_CHANGED; }
-//bool isJailEntered(intptr_t k) { return k == JAIL_ENTERED; }
+// Track chroot state changes for success, failure, state change
+// and "jail"
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ChrootState, ChrootKind)
+namespace {
 
 // This checker checks improper use of chroot.
-// The state transition:
+// The state transitions
+//
+//                          -> ROOT_CHANGE_FAILED
+//                          |
 // NO_CHROOT ---chroot(path)--> ROOT_CHANGED ---chdir(/) --> JAIL_ENTERED
 //                                  |                               |
 //         ROOT_CHANGED<--chdir(..)--      JAIL_ENTERED<--chdir(..)--
 //                                  |                               |
 //                      bug<--foo()--          JAIL_ENTERED<--foo()--
-class ChrootChecker : public Checker<eval::Call, check::PreCall> {
-  // This bug refers to possibly break out of a chroot() jail.
-  const BugType BT_BreakJail{this, "Break out of jail"};
-
-  const CallDescription Chroot{CDM::CLibrary, {"chroot"}, 1},
-      Chdir{CDM::CLibrary, {"chdir"}, 1};
-
+//
+class ChrootChecker final : public Checker<eval::Call, check::PreCall> {
 public:
-  ChrootChecker() {}
-
-  static void *getTag() {
-    static int x;
-    return &x;
-  }
-
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
 
 private:
-  void evalChroot(const CallEvent &Call, CheckerContext &C) const;
-  void evalChdir(const CallEvent &Call, CheckerContext &C) const;
-};
+  bool evalChroot(const CallEvent &Call, CheckerContext &C) const;
+  bool evalChdir(const CallEvent &Call, CheckerContext &C) const;
 
-} // end anonymous namespace
+  const BugType BreakJailBug{this, "Break out of jail"};
+  const CallDescription Chroot{CDM::CLibrary, {"chroot"}, 1};
+  const CallDescription Chdir{CDM::CLibrary, {"chdir"}, 1};
+};
 
 bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
-  if (Chroot.matches(Call)) {
-    evalChroot(Call, C);
-    return true;
-  }
-  if (Chdir.matches(Call)) {
-    evalChdir(Call, C);
-    return true;
-  }
+  if (Chroot.matches(Call))
+    return evalChroot(Call, C);
+
+  if (Chdir.matches(Call))
+    return evalChdir(Call, C);
 
   return false;
 }
 
-void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
-  ProgramStateRef state = C.getState();
-  ProgramStateManager &Mgr = state->getStateManager();
+bool ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
+  BasicValueFactory &BVF = C.getSValBuilder().getBasicValueFactory();
+  const LocationContext *LCtx = C.getLocationContext();
+  ProgramStateRef State = C.getState();
+  const auto *CE = cast<CallExpr>(Call.getOriginExpr());
+
+  const QualType IntTy = C.getASTContext().IntTy;
+  SVal Zero = nonloc::ConcreteInt{BVF.getValue(0, IntTy)};
+  SVal Minus1 = nonloc::ConcreteInt{BVF.getValue(-1, IntTy)};
 
-  // Once encouter a chroot(), set the enum value ROOT_CHANGED directly in
-  // the GDM.
-  state = Mgr.addGDM(state, ChrootChecker::getTag(), (void*) ROOT_CHANGED);
-  C.addTransition(state);
+  ProgramStateRef ChrootFailed = State->BindExpr(CE, LCtx, Minus1);
+  C.addTransition(ChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED));
+
+  ProgramStateRef ChrootSucceeded = State->BindExpr(CE, LCtx, Zero);
+  C.addTransition(ChrootSucceeded->set<ChrootState>(ROOT_CHANGED));
+  return true;
 }
 
-void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
-  ProgramStateRef state = C.getState();
-  ProgramStateManager &Mgr = state->getStateManager();
+bool ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
 
-  // If there are no jail state in the GDM, just return.
-  const void *k = state->FindGDM(ChrootChecker::getTag());
-  if (!k)
-    return;
+  // If there are no jail state, just return.
+  if (State->get<ChrootState>() == NO_CHROOT)
+    return false;
 
   // After chdir("/"), enter the jail, set the enum value JAIL_ENTERED.
-  const Expr *ArgExpr = Call.getArgExpr(0);
-  SVal ArgVal = C.getSVal(ArgExpr);
+  SVal ArgVal = Call.getArgSVal(0);
 
   if (const MemRegion *R = ArgVal.getAsRegion()) {
     R = R->StripCasts();
-    if (const StringRegion* StrRegion= dyn_cast<StringRegion>(R)) {
-      const StringLiteral* Str = StrRegion->getStringLiteral();
-      if (Str->getString() == "/")
-        state = Mgr.addGDM(state, ChrootChecker::getTag(),
-                           (void*) JAIL_ENTERED);
+    if (const auto *StrRegion = dyn_cast<StringRegion>(R)) {
+      if (StrRegion->getStringLiteral()->getString() == "/") {
+        C.addTransition(State->set<ChrootState>(JAIL_ENTERED));
+        return true;
+      }
     }
   }
-
-  C.addTransition(state);
+  return false;
 }
 
+class ChrootInvocationVisitor final : public BugReporterVisitor {
+public:
+  explicit ChrootInvocationVisitor(const CallDescription &Chroot)
+      : Chroot{Chroot} {}
+
+  PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
+                                   BugReporterContext &BRC,
+                                   PathSensitiveBugReport &BR) override {
+    if (Satisfied)
+      return nullptr;
+
+    auto StmtP = N->getLocation().getAs<StmtPoint>();
+    if (!StmtP)
+      return nullptr;
+
+    const CallExpr *Call = StmtP->getStmtAs<CallExpr>();
+    if (!Call)
+      return nullptr;
+
+    if (!Chroot.matchesAsWritten(*Call))
+      return nullptr;
+
+    Satisfied = true;
+    PathDiagnosticLocation Pos(Call, BRC.getSourceManager(),
+                               N->getLocationContext());
+    return std::make_shared<PathDiagnosticEventPiece>(Pos, "chroot called here",
+                                                      /*addPosRange=*/true);
+  }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    static bool Tag;
+    ID.AddPointer(&Tag);
+  }
+
+private:
+  const CallDescription &Chroot;
+  bool Satisfied = false;
+};
+
 // Check the jail state before any function call except chroot and chdir().
 void ChrootChecker::checkPreCall(const CallEvent &Call,
                                  CheckerContext &C) const {
   // Ignore chroot and chdir.
   if (matchesAny(Call, Chroot, Chdir))
     return;
 
-  // If jail state is ROOT_CHANGED, generate BugReport.
-  void *const* k = C.getState()->FindGDM(ChrootChecker::getTag());
-  if (k)
-    if (isRootChanged((intptr_t) *k))
-      if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
-        constexpr llvm::StringLiteral Msg =
-            "No call of chdir(\"/\") immediately after chroot";
-        C.emitReport(
-            std::make_unique<PathSensitiveBugReport>(BT_BreakJail, Msg, N));
-      }
-}
+  // If jail state is not ROOT_CHANGED just return.
+  if (C.getState()->get<ChrootState>() != ROOT_CHANGED)
+    return;
 
-void ento::registerChrootChecker(CheckerManager &mgr) {
-  mgr.registerChecker<ChrootChecker>();
+  // Generate bug report.
+  ExplodedNode *Err =
+      C.generateNonFatalErrorNode(C.getState(), C.getPredecessor());
+  if (!Err)
+    return;
+
+  auto R = std::make_unique<PathSensitiveBugReport>(
+      BreakJailBug, R"(No call of chdir("/") immediately after chroot)", Err);
+  R->addVisitor<ChrootInvocationVisitor>(Chroot);
+  C.emitReport(std::move(R));
 }
 
-bool ento::shouldRegisterChrootChecker(const CheckerManager &mgr) {
-  return true;
+} // namespace
+
+void ento::registerChrootChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<ChrootChecker>();
 }
+
+bool ento::shouldRegisterChrootChecker(const CheckerManager &) { return true; }

clang/test/Analysis/analyzer-enabled-checkers.c

@@ -43,6 +43,7 @@
 // CHECK-NEXT: security.insecureAPI.vfork
 // CHECK-NEXT: unix.API
 // CHECK-NEXT: unix.BlockInCriticalSection
+// CHECK-NEXT: unix.Chroot
 // CHECK-NEXT: unix.cstring.CStringModeling
 // CHECK-NEXT: unix.DynamicMemoryModeling
 // CHECK-NEXT: unix.Errno