[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,31 @@ 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().
+
+.. code-block:: c
+
+ void f();
+
+ void test() {
+   chroot("/usr/local");
+   f(); // warn: no call of chdir("/") immediately after chroot
+ }
+
+ void test() {
+   chroot("/usr/local");
+   chdir("/"); // no warning
+   f();
+ }
+
 .. _unix-Errno:
 
 unix.Errno (C)
@@ -3275,21 +3300,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

@@ -10,6 +10,8 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/ASTContext.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
@@ -24,21 +26,30 @@
 using namespace clang;
 using namespace ento;
 
-namespace {
-
 // enum value that represent the jail state
-enum Kind { NO_CHROOT, ROOT_CHANGED, JAIL_ENTERED };
+enum ChrootKind { NO_CHROOT, ROOT_CHANGED, ROOT_CHANGE_FAILED, JAIL_ENTERED };
+
+// Track chroot state changes for success, failure, state change
+// and "jail"
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ChrootState, ChrootKind)
+
+// Track the call expression to chroot for accurate
+// warning messages
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ChrootCall, const Expr *)
 
-bool isRootChanged(intptr_t k) { return k == ROOT_CHANGED; }
-//bool isJailEntered(intptr_t k) { return k == JAIL_ENTERED; }
+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"};
@@ -49,20 +60,17 @@ class ChrootChecker : 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;
-};
 
-} // end anonymous namespace
+  /// Searches for the ExplodedNode where chroot was called.
+  static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
+                                                CheckerContext &C);
+};
 
 bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
   if (Chroot.matches(Call)) {
@@ -79,20 +87,46 @@ bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
 
 void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
   ProgramStateRef state = C.getState();
-  ProgramStateManager &Mgr = state->getStateManager();
+  SValBuilder &SVB = C.getSValBuilder();
+  BasicValueFactory &BVF = SVB.getBasicValueFactory();
 
-  // 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);
+  const QualType IntTy = C.getASTContext().IntTy;
+
+  const Expr *ChrootCE = Call.getOriginExpr();
+  if (!ChrootCE)
+    return;
+  const auto *CE = cast<CallExpr>(Call.getOriginExpr());
+
+  const LocationContext *LCtx = C.getLocationContext();
+  NonLoc RetVal = SVB.conjureSymbolVal(/*SymbolTag=*/nullptr, ChrootCE, LCtx,
+                                       IntTy, C.blockCount())
+                      .castAs<NonLoc>();
+
+  auto [StateChrootFailed, StateChrootSuccess] = state->assume(RetVal);
+
+  const llvm::APSInt &Zero = BVF.getValue(0, IntTy);
+  const llvm::APSInt &Minus1 = BVF.getValue(-1, IntTy);
+
+  if (StateChrootFailed) {
+    StateChrootFailed = StateChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED);
+    StateChrootFailed = StateChrootFailed->set<ChrootCall>(ChrootCE);
+    C.addTransition(
+        StateChrootFailed->BindExpr(CE, LCtx, nonloc::ConcreteInt{Minus1}));
+  }
+
+  if (StateChrootSuccess) {
+    StateChrootSuccess = StateChrootSuccess->set<ChrootState>(ROOT_CHANGED);
+    StateChrootSuccess = StateChrootSuccess->set<ChrootCall>(ChrootCE);
+    C.addTransition(
+        StateChrootSuccess->BindExpr(CE, LCtx, nonloc::ConcreteInt{Zero}));
+  }
 }
 
 void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
   ProgramStateRef state = C.getState();
-  ProgramStateManager &Mgr = state->getStateManager();
 
-  // If there are no jail state in the GDM, just return.
-  const void *k = state->FindGDM(ChrootChecker::getTag());
+  // If there are no jail state, just return.
+  const ChrootKind k = C.getState()->get<ChrootState>();
   if (!k)
     return;
 
@@ -104,15 +138,36 @@ void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
     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 (Str->getString() == "/") {
+        state = state->set<ChrootState>(JAIL_ENTERED);
+      }
     }
   }
 
   C.addTransition(state);
 }
 
+const ExplodedNode *ChrootChecker::getAcquisitionSite(const ExplodedNode *N,
+                                                      CheckerContext &C) {
+  ProgramStateRef State = N->getState();
+  // When a chroot() issue is found, the current exploded node may not
+  // have state info for where chroot() was called for the bug report but
+  // a predecessor should have it.
+  if (!State->get<ChrootCall>())
+    N = N->getFirstPred();
+
+  const ExplodedNode *Pred = N;
+  while (N) {
+    State = N->getState();
+    if (!State->get<ChrootCall>())
+      return Pred;
+    Pred = N;
+    N = N->getFirstPred();
+  }
+
+  return nullptr;
+}
+
 // Check the jail state before any function call except chroot and chdir().
 void ChrootChecker::checkPreCall(const CallEvent &Call,
                                  CheckerContext &C) const {
@@ -121,17 +176,40 @@ void ChrootChecker::checkPreCall(const CallEvent &Call,
     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));
-      }
+  const ChrootKind k = C.getState()->get<ChrootState>();
+  if (k == ROOT_CHANGED) {
+    ExplodedNode *Err =
+        C.generateNonFatalErrorNode(C.getState(), C.getPredecessor());
+    if (!Err)
+      return;
+    const Expr *ChrootExpr = C.getState()->get<ChrootCall>();
+
+    const ExplodedNode *ChrootCallNode = getAcquisitionSite(Err, C);
+    assert(ChrootCallNode && "Could not find place of stream opening.");
+
+    PathDiagnosticLocation LocUsedForUniqueing;
+    if (const Stmt *ChrootStmt = ChrootCallNode->getStmtForDiagnostics())
+      LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
+          ChrootStmt, C.getSourceManager(),
+          ChrootCallNode->getLocationContext());
+
+    std::unique_ptr<PathSensitiveBugReport> R =
+        std::make_unique<PathSensitiveBugReport>(
+            BT_BreakJail, R"(No call of chdir("/") immediately after chroot)",
+            Err, LocUsedForUniqueing,
+            ChrootCallNode->getLocationContext()->getDecl());
+
+    R->addNote("chroot called here",
+               PathDiagnosticLocation::create(ChrootCallNode->getLocation(),
+                                              C.getSourceManager()),
+               {ChrootExpr->getSourceRange()});
+
+    C.emitReport(std::move(R));
+  }
 }
 
+} // namespace
+
 void ento::registerChrootChecker(CheckerManager &mgr) {
   mgr.registerChecker<ChrootChecker>();
 }

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

clang/test/Analysis/chroot.c

@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Chroot -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.Chroot -verify %s
 
 extern int chroot(const char* path);
 extern int chdir(const char* path);
@@ -7,7 +7,7 @@ void foo(void) {
 }
 
 void f1(void) {
-  chroot("/usr/local"); // root changed.
+  chroot("/usr/local"); // expected-note {{chroot called here}}
   foo(); // expected-warning {{No call of chdir("/") immediately after chroot}}
 }
 
@@ -18,7 +18,37 @@ void f2(void) {
 }
 
 void f3(void) {
-  chroot("/usr/local"); // root changed.
+  chroot("/usr/local"); // expected-note {{chroot called here}}
   chdir("../"); // change working directory, still out of jail.
   foo(); // expected-warning {{No call of chdir("/") immediately after chroot}}
 }
+
+void f4(void) {
+  if (chroot("/usr/local") == 0) {
+      chdir("../"); // change working directory, still out of jail.
+  }
+}
+
+void f5(void) {
+  int v = chroot("/usr/local");
+  if (v == -1) {
+      foo();        // no warning, chroot failed
+      chdir("../"); // change working directory, still out of jail.
+  }
+}
+
+void f6(void) {
+  if (chroot("/usr/local") == -1) {
+      chdir("../"); // change working directory, still out of jail.
+  }
+}
+
+void f7(void) {
+  int v = chroot("/usr/local"); // expected-note {{chroot called here}}
+  if (v == -1) {
+      foo();        // no warning, chroot failed
+      chdir("../"); // change working directory, still out of jail.
+  } else {
+      foo();        // expected-warning {{No call of chdir("/") immediately after chroot}}
+  }
+}

clang/test/Analysis/show-checker-list.c

@@ -24,10 +24,6 @@
 // RUN:   -analyzer-checker-help-developer \
 // RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-STABLE-ALPHA-DEVELOPER
 
-// CHECK-STABLE-NOT:    alpha.unix.Chroot
-// CHECK-DEVELOPER-NOT: alpha.unix.Chroot
-// CHECK-ALPHA:         alpha.unix.Chroot
-
 // Note that alpha.cplusplus.IteratorModeling is not only an alpha, but also a
 // hidden checker. In this case, we'd only like to see it in the developer list.
 // CHECK-ALPHA-NOT: alpha.cplusplus.IteratorModeling
@@ -42,10 +38,6 @@
 // CHECK-ALPHA-NOT:  debug.ConfigDumper
 
 
-// CHECK-STABLE-ALPHA:         alpha.unix.Chroot
-// CHECK-DEVELOPER-ALPHA:      alpha.unix.Chroot
-// CHECK-STABLE-DEVELOPER-NOT: alpha.unix.Chroot
-
 // CHECK-STABLE-ALPHA:        core.DivideZero
 // CHECK-DEVELOPER-ALPHA-NOT: core.DivideZero
 // CHECK-STABLE-DEVELOPER:    core.DivideZero
@@ -55,6 +47,5 @@
 // CHECK-STABLE-DEVELOPER: debug.ConfigDumper
 
 
-// CHECK-STABLE-ALPHA-DEVELOPER: alpha.unix.Chroot
 // CHECK-STABLE-ALPHA-DEVELOPER: core.DivideZero
 // CHECK-STABLE-ALPHA-DEVELOPER: debug.ConfigDumper