diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 6c40e48e2f49b..5e5ef571ac67f 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/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 + `_. + Fixes (#GH34697). + (#GH117791) [Documentation](https://clang.llvm.org/docs/analyzer/checkers.html#unix-chroot-c). + .. _release-notes-sanitizers: Sanitizers diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index f34b25cd04bd1..e56b2493d564b 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/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 +`_. +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 , HelpText<"Check for proper usage of vfork">, Documentation; -} // end "unix" - -let ParentPackage = UnixAlpha in { - def ChrootChecker : Checker<"Chroot">, HelpText<"Check improper use of chroot">, Documentation; +} // end "unix" + +let ParentPackage = UnixAlpha in { + def PthreadLockChecker : Checker<"PthreadLock">, HelpText<"Simple lock -> unlock checker">, Dependencies<[PthreadLockBase]>, diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp index 3a0a01c23de03..99fc0a953ef17 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp +++ b/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,94 +29,128 @@ 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 { - // 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 { 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(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(ROOT_CHANGE_FAILED)); + + ProgramStateRef ChrootSucceeded = State->BindExpr(CE, LCtx, Zero); + C.addTransition(ChrootSucceeded->set(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() == 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(R)) { - const StringLiteral* Str = StrRegion->getStringLiteral(); - if (Str->getString() == "/") - state = Mgr.addGDM(state, ChrootChecker::getTag(), - (void*) JAIL_ENTERED); + if (const auto *StrRegion = dyn_cast(R)) { + if (StrRegion->getStringLiteral()->getString() == "/") { + C.addTransition(State->set(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(); + if (!StmtP) + return nullptr; + + const CallExpr *Call = StmtP->getStmtAs(); + if (!Call) + return nullptr; + + if (!Chroot.matchesAsWritten(*Call)) + return nullptr; + + Satisfied = true; + PathDiagnosticLocation Pos(Call, BRC.getSourceManager(), + N->getLocationContext()); + return std::make_shared(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 { @@ -120,22 +158,26 @@ void ChrootChecker::checkPreCall(const CallEvent &Call, 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(BT_BreakJail, Msg, N)); - } -} + // If jail state is not ROOT_CHANGED just return. + if (C.getState()->get() != ROOT_CHANGED) + return; -void ento::registerChrootChecker(CheckerManager &mgr) { - mgr.registerChecker(); + // Generate bug report. + ExplodedNode *Err = + C.generateNonFatalErrorNode(C.getState(), C.getPredecessor()); + if (!Err) + return; + + auto R = std::make_unique( + BreakJailBug, R"(No call of chdir("/") immediately after chroot)", Err); + R->addVisitor(Chroot); + C.emitReport(std::move(R)); } -bool ento::shouldRegisterChrootChecker(const CheckerManager &mgr) { - return true; +} // namespace + +void ento::registerChrootChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); } + +bool ento::shouldRegisterChrootChecker(const CheckerManager &) { return true; } diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c index e605c62a66ad0..78a5daab98b0c 100644 --- a/clang/test/Analysis/analyzer-enabled-checkers.c +++ b/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 diff --git a/clang/test/Analysis/chroot.c b/clang/test/Analysis/chroot.c index eb512c05f86f7..34f460b0f0154 100644 --- a/clang/test/Analysis/chroot.c +++ b/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 -analyzer-output=text -verify %s extern int chroot(const char* path); extern int chdir(const char* path); @@ -7,8 +7,10 @@ void foo(void) { } void f1(void) { - chroot("/usr/local"); // root changed. - foo(); // expected-warning {{No call of chdir("/") immediately after chroot}} + chroot("/usr/local"); // expected-note {{chroot called here}} + foo(); + // expected-warning@-1 {{No call of chdir("/") immediately after chroot}} + // expected-note@-2 {{No call of chdir("/") immediately after chroot}} } void f2(void) { @@ -18,7 +20,49 @@ 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}} + foo(); + // expected-warning@-1 {{No call of chdir("/") immediately after chroot}} + // expected-note@-2 {{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) { // expected-note {{Taking false branch}} + foo(); // no warning, chroot failed + chdir("../"); // change working directory, still out of jail. + } else { + foo(); + // expected-warning@-1 {{No call of chdir("/") immediately after chroot}} + // expected-note@-2 {{No call of chdir("/") immediately after chroot}} + } +} + +void f8() { + chroot("/usr/local"); // expected-note {{chroot called here}} + chdir("/usr"); // This chdir was ineffective because it's not exactly `chdir("/")`. + foo(); + // expected-warning@-1 {{No call of chdir("/") immediately after chroot}} + // expected-note@-2 {{No call of chdir("/") immediately after chroot}} } diff --git a/clang/test/Analysis/show-checker-list.c b/clang/test/Analysis/show-checker-list.c index 3d354c338b9b3..99d7d4ac8dae0 100644 --- a/clang/test/Analysis/show-checker-list.c +++ b/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 diff --git a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c index 345a4e8f44efd..dad15806197d8 100644 --- a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c +++ b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c @@ -51,6 +51,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