Skip to content

Commit e034c4e

Browse files
vabridgerseinvbristeakhal
authored
[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 <[email protected]> Co-authored-by: Balazs Benics <[email protected]>
1 parent 3b43276 commit e034c4e

File tree

8 files changed

+208
-103
lines changed

8 files changed

+208
-103
lines changed

‎clang/docs/ReleaseNotes.rst

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1024,6 +1024,16 @@ Moved checkers
10241024
original checkers were implemented only using AST matching and make more
10251025
sense as a single clang-tidy check.
10261026

1027+
- The checker ``alpha.unix.Chroot`` was modernized, improved and moved to
1028+
``unix.Chroot``. Testing was done on open source projects that use chroot(),
1029+
and false issues addressed in the improvements based on real use cases. Open
1030+
source projects used for testing include nsjail, lxroot, dive and ruri.
1031+
This checker conforms to SEI Cert C recommendation `POS05-C. Limit access to
1032+
files by creating a jail
1033+
<https://wiki.sei.cmu.edu/confluence/display/c/POS05-C.+Limit+access+to+files+by+creating+a+jail>`_.
1034+
Fixes (#GH34697).
1035+
(#GH117791) [Documentation](https://clang.llvm.org/docs/analyzer/checkers.html#unix-chroot-c).
1036+
10271037
.. _release-notes-sanitizers:
10281038

10291039
Sanitizers

‎clang/docs/analyzer/checkers.rst

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1750,6 +1750,37 @@ Critical section handling functions modeled by this checker:
17501750
}
17511751
}
17521752
1753+
.. _unix-Chroot:
1754+
1755+
unix.Chroot (C)
1756+
"""""""""""""""
1757+
Check improper use of chroot described by SEI Cert C recommendation `POS05-C.
1758+
Limit access to files by creating a jail
1759+
<https://wiki.sei.cmu.edu/confluence/display/c/POS05-C.+Limit+access+to+files+by+creating+a+jail>`_.
1760+
The checker finds usage patterns where ``chdir("/")`` is not called immediately
1761+
after a call to ``chroot(path)``.
1762+
1763+
.. code-block:: c
1764+
1765+
void f();
1766+
1767+
void test_bad() {
1768+
chroot("/usr/local");
1769+
f(); // warn: no call of chdir("/") immediately after chroot
1770+
}
1771+
1772+
void test_bad_path() {
1773+
chroot("/usr/local");
1774+
chdir("/usr"); // warn: no call of chdir("/") immediately after chroot
1775+
f();
1776+
}
1777+
1778+
void test_good() {
1779+
chroot("/usr/local");
1780+
chdir("/"); // no warning
1781+
f();
1782+
}
1783+
17531784
.. _unix-Errno:
17541785
17551786
unix.Errno (C)
@@ -3298,21 +3329,6 @@ SEI CERT checkers which tries to find errors based on their `C coding rules <htt
32983329
alpha.unix
32993330
^^^^^^^^^^
33003331
3301-
.. _alpha-unix-Chroot:
3302-
3303-
alpha.unix.Chroot (C)
3304-
"""""""""""""""""""""
3305-
Check improper use of chroot.
3306-
3307-
.. code-block:: c
3308-
3309-
void f();
3310-
3311-
void test() {
3312-
chroot("/usr/local");
3313-
f(); // warn: no call of chdir("/") immediately after chroot
3314-
}
3315-
33163332
.. _alpha-unix-PthreadLock:
33173333
33183334
alpha.unix.PthreadLock (C)

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -599,14 +599,14 @@ def VforkChecker : Checker<"Vfork">,
599599
HelpText<"Check for proper usage of vfork">,
600600
Documentation<HasDocumentation>;
601601

602-
} // end "unix"
603-
604-
let ParentPackage = UnixAlpha in {
605-
606602
def ChrootChecker : Checker<"Chroot">,
607603
HelpText<"Check improper use of chroot">,
608604
Documentation<HasDocumentation>;
609605

606+
} // end "unix"
607+
608+
let ParentPackage = UnixAlpha in {
609+
610610
def PthreadLockChecker : Checker<"PthreadLock">,
611611
HelpText<"Simple lock -> unlock checker">,
612612
Dependencies<[PthreadLockBase]>,

‎clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp

Lines changed: 112 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,13 @@
77
//===----------------------------------------------------------------------===//
88
//
99
// This file defines chroot checker, which checks improper use of chroot.
10+
// This is described by the SEI Cert C rule POS05-C.
11+
// The checker is a warning not a hard failure since it only checks for a
12+
// recommended rule.
1013
//
1114
//===----------------------------------------------------------------------===//
1215

16+
#include "clang/AST/ASTContext.h"
1317
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
1418
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
1519
#include "clang/StaticAnalyzer/Core/Checker.h"
@@ -25,117 +29,155 @@ using namespace clang;
2529
using namespace ento;
2630

2731
namespace {
32+
enum ChrootKind { NO_CHROOT, ROOT_CHANGED, ROOT_CHANGE_FAILED, JAIL_ENTERED };
33+
} // namespace
2834

29-
// enum value that represent the jail state
30-
enum Kind { NO_CHROOT, ROOT_CHANGED, JAIL_ENTERED };
31-
32-
bool isRootChanged(intptr_t k) { return k == ROOT_CHANGED; }
33-
//bool isJailEntered(intptr_t k) { return k == JAIL_ENTERED; }
35+
// Track chroot state changes for success, failure, state change
36+
// and "jail"
37+
REGISTER_TRAIT_WITH_PROGRAMSTATE(ChrootState, ChrootKind)
38+
namespace {
3439

3540
// This checker checks improper use of chroot.
36-
// The state transition:
41+
// The state transitions
42+
//
43+
// -> ROOT_CHANGE_FAILED
44+
// |
3745
// NO_CHROOT ---chroot(path)--> ROOT_CHANGED ---chdir(/) --> JAIL_ENTERED
3846
// | |
3947
// ROOT_CHANGED<--chdir(..)-- JAIL_ENTERED<--chdir(..)--
4048
// | |
4149
// bug<--foo()-- JAIL_ENTERED<--foo()--
42-
class ChrootChecker : public Checker<eval::Call, check::PreCall> {
43-
// This bug refers to possibly break out of a chroot() jail.
44-
const BugType BT_BreakJail{this, "Break out of jail"};
45-
46-
const CallDescription Chroot{CDM::CLibrary, {"chroot"}, 1},
47-
Chdir{CDM::CLibrary, {"chdir"}, 1};
48-
50+
//
51+
class ChrootChecker final : public Checker<eval::Call, check::PreCall> {
4952
public:
50-
ChrootChecker() {}
51-
52-
static void *getTag() {
53-
static int x;
54-
return &x;
55-
}
56-
5753
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
5854
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
5955

6056
private:
61-
void evalChroot(const CallEvent &Call, CheckerContext &C) const;
62-
void evalChdir(const CallEvent &Call, CheckerContext &C) const;
63-
};
57+
bool evalChroot(const CallEvent &Call, CheckerContext &C) const;
58+
bool evalChdir(const CallEvent &Call, CheckerContext &C) const;
6459

65-
} // end anonymous namespace
60+
const BugType BreakJailBug{this, "Break out of jail"};
61+
const CallDescription Chroot{CDM::CLibrary, {"chroot"}, 1};
62+
const CallDescription Chdir{CDM::CLibrary, {"chdir"}, 1};
63+
};
6664

6765
bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
68-
if (Chroot.matches(Call)) {
69-
evalChroot(Call, C);
70-
return true;
71-
}
72-
if (Chdir.matches(Call)) {
73-
evalChdir(Call, C);
74-
return true;
75-
}
66+
if (Chroot.matches(Call))
67+
return evalChroot(Call, C);
68+
69+
if (Chdir.matches(Call))
70+
return evalChdir(Call, C);
7671

7772
return false;
7873
}
7974

80-
void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
81-
ProgramStateRef state = C.getState();
82-
ProgramStateManager &Mgr = state->getStateManager();
75+
bool ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
76+
BasicValueFactory &BVF = C.getSValBuilder().getBasicValueFactory();
77+
const LocationContext *LCtx = C.getLocationContext();
78+
ProgramStateRef State = C.getState();
79+
const auto *CE = cast<CallExpr>(Call.getOriginExpr());
80+
81+
const QualType IntTy = C.getASTContext().IntTy;
82+
SVal Zero = nonloc::ConcreteInt{BVF.getValue(0, IntTy)};
83+
SVal Minus1 = nonloc::ConcreteInt{BVF.getValue(-1, IntTy)};
8384

84-
// Once encouter a chroot(), set the enum value ROOT_CHANGED directly in
85-
// the GDM.
86-
state = Mgr.addGDM(state, ChrootChecker::getTag(), (void*) ROOT_CHANGED);
87-
C.addTransition(state);
85+
ProgramStateRef ChrootFailed = State->BindExpr(CE, LCtx, Minus1);
86+
C.addTransition(ChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED));
87+
88+
ProgramStateRef ChrootSucceeded = State->BindExpr(CE, LCtx, Zero);
89+
C.addTransition(ChrootSucceeded->set<ChrootState>(ROOT_CHANGED));
90+
return true;
8891
}
8992

90-
void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
91-
ProgramStateRef state = C.getState();
92-
ProgramStateManager &Mgr = state->getStateManager();
93+
bool ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
94+
ProgramStateRef State = C.getState();
9395

94-
// If there are no jail state in the GDM, just return.
95-
const void *k = state->FindGDM(ChrootChecker::getTag());
96-
if (!k)
97-
return;
96+
// If there are no jail state, just return.
97+
if (State->get<ChrootState>() == NO_CHROOT)
98+
return false;
9899

99100
// After chdir("/"), enter the jail, set the enum value JAIL_ENTERED.
100-
const Expr *ArgExpr = Call.getArgExpr(0);
101-
SVal ArgVal = C.getSVal(ArgExpr);
101+
SVal ArgVal = Call.getArgSVal(0);
102102

103103
if (const MemRegion *R = ArgVal.getAsRegion()) {
104104
R = R->StripCasts();
105-
if (const StringRegion* StrRegion= dyn_cast<StringRegion>(R)) {
106-
const StringLiteral* Str = StrRegion->getStringLiteral();
107-
if (Str->getString() == "/")
108-
state = Mgr.addGDM(state, ChrootChecker::getTag(),
109-
(void*) JAIL_ENTERED);
105+
if (const auto *StrRegion = dyn_cast<StringRegion>(R)) {
106+
if (StrRegion->getStringLiteral()->getString() == "/") {
107+
C.addTransition(State->set<ChrootState>(JAIL_ENTERED));
108+
return true;
109+
}
110110
}
111111
}
112-
113-
C.addTransition(state);
112+
return false;
114113
}
115114

115+
class ChrootInvocationVisitor final : public BugReporterVisitor {
116+
public:
117+
explicit ChrootInvocationVisitor(const CallDescription &Chroot)
118+
: Chroot{Chroot} {}
119+
120+
PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
121+
BugReporterContext &BRC,
122+
PathSensitiveBugReport &BR) override {
123+
if (Satisfied)
124+
return nullptr;
125+
126+
auto StmtP = N->getLocation().getAs<StmtPoint>();
127+
if (!StmtP)
128+
return nullptr;
129+
130+
const CallExpr *Call = StmtP->getStmtAs<CallExpr>();
131+
if (!Call)
132+
return nullptr;
133+
134+
if (!Chroot.matchesAsWritten(*Call))
135+
return nullptr;
136+
137+
Satisfied = true;
138+
PathDiagnosticLocation Pos(Call, BRC.getSourceManager(),
139+
N->getLocationContext());
140+
return std::make_shared<PathDiagnosticEventPiece>(Pos, "chroot called here",
141+
/*addPosRange=*/true);
142+
}
143+
144+
void Profile(llvm::FoldingSetNodeID &ID) const override {
145+
static bool Tag;
146+
ID.AddPointer(&Tag);
147+
}
148+
149+
private:
150+
const CallDescription &Chroot;
151+
bool Satisfied = false;
152+
};
153+
116154
// Check the jail state before any function call except chroot and chdir().
117155
void ChrootChecker::checkPreCall(const CallEvent &Call,
118156
CheckerContext &C) const {
119157
// Ignore chroot and chdir.
120158
if (matchesAny(Call, Chroot, Chdir))
121159
return;
122160

123-
// If jail state is ROOT_CHANGED, generate BugReport.
124-
void *const* k = C.getState()->FindGDM(ChrootChecker::getTag());
125-
if (k)
126-
if (isRootChanged((intptr_t) *k))
127-
if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
128-
constexpr llvm::StringLiteral Msg =
129-
"No call of chdir(\"/\") immediately after chroot";
130-
C.emitReport(
131-
std::make_unique<PathSensitiveBugReport>(BT_BreakJail, Msg, N));
132-
}
133-
}
161+
// If jail state is not ROOT_CHANGED just return.
162+
if (C.getState()->get<ChrootState>() != ROOT_CHANGED)
163+
return;
134164

135-
void ento::registerChrootChecker(CheckerManager &mgr) {
136-
mgr.registerChecker<ChrootChecker>();
165+
// Generate bug report.
166+
ExplodedNode *Err =
167+
C.generateNonFatalErrorNode(C.getState(), C.getPredecessor());
168+
if (!Err)
169+
return;
170+
171+
auto R = std::make_unique<PathSensitiveBugReport>(
172+
BreakJailBug, R"(No call of chdir("/") immediately after chroot)", Err);
173+
R->addVisitor<ChrootInvocationVisitor>(Chroot);
174+
C.emitReport(std::move(R));
137175
}
138176

139-
bool ento::shouldRegisterChrootChecker(const CheckerManager &mgr) {
140-
return true;
177+
} // namespace
178+
179+
void ento::registerChrootChecker(CheckerManager &Mgr) {
180+
Mgr.registerChecker<ChrootChecker>();
141181
}
182+
183+
bool ento::shouldRegisterChrootChecker(const CheckerManager &) { return true; }

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
// CHECK-NEXT: security.insecureAPI.vfork
4444
// CHECK-NEXT: unix.API
4545
// CHECK-NEXT: unix.BlockInCriticalSection
46+
// CHECK-NEXT: unix.Chroot
4647
// CHECK-NEXT: unix.cstring.CStringModeling
4748
// CHECK-NEXT: unix.DynamicMemoryModeling
4849
// CHECK-NEXT: unix.Errno

0 commit comments

Comments
 (0)