Author: vabridgers Date: 2024-11-29T08:23:08+01:00 New Revision: e034c4ef7b52635bb9cc78b6d3f97a4af5002f92
URL: https://github.com/llvm/llvm-project/commit/e034c4ef7b52635bb9cc78b6d3f97a4af5002f92 DIFF: https://github.com/llvm/llvm-project/commit/e034c4ef7b52635bb9cc78b6d3f97a4af5002f92.diff LOG: [analyzer] Modernize, improve and promote chroot checker (#117791) This change modernizes, improves and promotes the chroot checker from alpha to the Unix family of checkers. This checker covers the POS05 recommendations for use of chroot. The improvements included modeling of a success or failure from chroot and not falsely reporting a warning along an error path. This was made possible through modernizing the checker to be flow sensitive. --------- Co-authored-by: einvbri <vince.a.bridg...@ericsson.com> Co-authored-by: Balazs Benics <benicsbal...@gmail.com> Added: Modified: clang/docs/ReleaseNotes.rst clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp clang/test/Analysis/analyzer-enabled-checkers.c clang/test/Analysis/chroot.c clang/test/Analysis/show-checker-list.c clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 758a08e6309360..102ffb56fec35f 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -1024,6 +1024,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 diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 401d8dcb4e7f02..cbbdc6345c116b 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 +<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) @@ -3298,21 +3329,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) diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 20ef69efdce62b..6dcdcd6177292b 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -599,14 +599,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]>, diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp index 3a0a01c23de03e..99fc0a953ef178 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<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 { @@ -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<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; } diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c index a84a0c2211fde0..160e35c77462d1 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 eb512c05f86f72..34f460b0f01540 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 3d354c338b9b3a..99d7d4ac8dae0b 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 3d1d3c561a5580..0910f030b0f072 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
