Skip to content

Commit 1b073f9

Browse files
author
einvbri
committed
[analyzer] Modernize, improve and promote chroot checker
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.
1 parent cefc1b0 commit 1b073f9

File tree

5 files changed

+172
-54
lines changed

5 files changed

+172
-54
lines changed

‎clang/docs/ReleaseNotes.rst

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -985,6 +985,9 @@ Moved checkers
985985
original checkers were implemented only using AST matching and make more
986986
sense as a single clang-tidy check.
987987

988+
- The checker ``alpha.unix.Chroot`` was modernized, improved and moved from
989+
alpha to a main Unix family checker.
990+
988991
.. _release-notes-sanitizers:
989992

990993
Sanitizers

‎clang/docs/analyzer/checkers.rst

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1750,6 +1750,21 @@ 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.
1758+
1759+
.. code-block:: c
1760+
1761+
void f();
1762+
1763+
void test() {
1764+
chroot("/usr/local");
1765+
f(); // warn: no call of chdir("/") immediately after chroot
1766+
}
1767+
17531768
.. _unix-Errno:
17541769
17551770
unix.Errno (C)
@@ -3275,21 +3290,6 @@ SEI CERT checkers which tries to find errors based on their `C coding rules <htt
32753290
alpha.unix
32763291
^^^^^^^^^^
32773292
3278-
.. _alpha-unix-Chroot:
3279-
3280-
alpha.unix.Chroot (C)
3281-
"""""""""""""""""""""
3282-
Check improper use of chroot.
3283-
3284-
.. code-block:: c
3285-
3286-
void f();
3287-
3288-
void test() {
3289-
chroot("/usr/local");
3290-
f(); // warn: no call of chdir("/") immediately after chroot
3291-
}
3292-
32933293
.. _alpha-unix-PthreadLock:
32943294
32953295
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
@@ -598,14 +598,14 @@ def VforkChecker : Checker<"Vfork">,
598598
HelpText<"Check for proper usage of vfork">,
599599
Documentation<HasDocumentation>;
600600

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

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

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

Lines changed: 117 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010
//
1111
//===----------------------------------------------------------------------===//
1212

13+
#include "clang/AST/ASTContext.h"
14+
#include "clang/Basic/TargetInfo.h"
1315
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
1416
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
1517
#include "clang/StaticAnalyzer/Core/Checker.h"
@@ -24,21 +26,30 @@
2426
using namespace clang;
2527
using namespace ento;
2628

27-
namespace {
28-
2929
// enum value that represent the jail state
30-
enum Kind { NO_CHROOT, ROOT_CHANGED, JAIL_ENTERED };
30+
enum ChrootKind { NO_CHROOT, ROOT_CHANGED, ROOT_CHANGE_FAILED, JAIL_ENTERED };
3131

32-
bool isRootChanged(intptr_t k) { return k == ROOT_CHANGED; }
33-
//bool isJailEntered(intptr_t k) { return k == JAIL_ENTERED; }
32+
// Track chroot state changes for success, failure, state change
33+
// and "jail"
34+
REGISTER_TRAIT_WITH_PROGRAMSTATE(ChrootState, ChrootKind)
35+
36+
// Track the call expression to chroot for accurate
37+
// warning messages
38+
REGISTER_TRAIT_WITH_PROGRAMSTATE(ChrootCall, const Expr *)
39+
40+
namespace {
3441

3542
// This checker checks improper use of chroot.
36-
// The state transition:
43+
// The state transitions
44+
//
45+
// -> ROOT_CHANGE_FAILED
46+
// |
3747
// NO_CHROOT ---chroot(path)--> ROOT_CHANGED ---chdir(/) --> JAIL_ENTERED
3848
// | |
3949
// ROOT_CHANGED<--chdir(..)-- JAIL_ENTERED<--chdir(..)--
4050
// | |
4151
// bug<--foo()-- JAIL_ENTERED<--foo()--
52+
//
4253
class ChrootChecker : public Checker<eval::Call, check::PreCall> {
4354
// This bug refers to possibly break out of a chroot() jail.
4455
const BugType BT_BreakJail{this, "Break out of jail"};
@@ -49,20 +60,17 @@ class ChrootChecker : public Checker<eval::Call, check::PreCall> {
4960
public:
5061
ChrootChecker() {}
5162

52-
static void *getTag() {
53-
static int x;
54-
return &x;
55-
}
56-
5763
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
5864
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
5965

6066
private:
6167
void evalChroot(const CallEvent &Call, CheckerContext &C) const;
6268
void evalChdir(const CallEvent &Call, CheckerContext &C) const;
63-
};
6469

65-
} // end anonymous namespace
70+
/// Searches for the ExplodedNode where chroot was called.
71+
static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
72+
CheckerContext &C);
73+
};
6674

6775
bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
6876
if (Chroot.matches(Call)) {
@@ -80,19 +88,53 @@ bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
8088
void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
8189
ProgramStateRef state = C.getState();
8290
ProgramStateManager &Mgr = state->getStateManager();
91+
const TargetInfo &TI = C.getASTContext().getTargetInfo();
92+
SValBuilder &SVB = C.getSValBuilder();
93+
BasicValueFactory &BVF = SVB.getBasicValueFactory();
94+
ConstraintManager &CM = Mgr.getConstraintManager();
8395

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);
96+
const QualType sIntTy = C.getASTContext().getIntTypeForBitwidth(
97+
/*DestWidth=*/TI.getIntWidth(), /*Signed=*/true);
98+
99+
const Expr *ChrootCE = Call.getOriginExpr();
100+
if (!ChrootCE)
101+
return;
102+
const auto *CE = cast<CallExpr>(Call.getOriginExpr());
103+
104+
const LocationContext *LCtx = C.getLocationContext();
105+
NonLoc RetVal =
106+
C.getSValBuilder()
107+
.conjureSymbolVal(nullptr, ChrootCE, LCtx, sIntTy, C.blockCount())
108+
.castAs<NonLoc>();
109+
110+
ProgramStateRef StateChrootFailed, StateChrootSuccess;
111+
std::tie(StateChrootFailed, StateChrootSuccess) = state->assume(RetVal);
112+
113+
const llvm::APSInt &Zero = BVF.getValue(0, sIntTy);
114+
const llvm::APSInt &Minus1 = BVF.getValue(-1, sIntTy);
115+
116+
if (StateChrootFailed) {
117+
StateChrootFailed = CM.assumeInclusiveRange(StateChrootFailed, RetVal,
118+
Minus1, Minus1, true);
119+
StateChrootFailed = StateChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED);
120+
StateChrootFailed = StateChrootFailed->set<ChrootCall>(ChrootCE);
121+
C.addTransition(StateChrootFailed->BindExpr(CE, LCtx, RetVal));
122+
}
123+
124+
if (StateChrootSuccess) {
125+
StateChrootSuccess =
126+
CM.assumeInclusiveRange(StateChrootSuccess, RetVal, Zero, Zero, true);
127+
StateChrootSuccess = StateChrootSuccess->set<ChrootState>(ROOT_CHANGED);
128+
StateChrootSuccess = StateChrootSuccess->set<ChrootCall>(ChrootCE);
129+
C.addTransition(StateChrootSuccess->BindExpr(CE, LCtx, RetVal));
130+
}
88131
}
89132

90133
void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
91134
ProgramStateRef state = C.getState();
92-
ProgramStateManager &Mgr = state->getStateManager();
93135

94-
// If there are no jail state in the GDM, just return.
95-
const void *k = state->FindGDM(ChrootChecker::getTag());
136+
// If there are no jail state, just return.
137+
const ChrootKind k = C.getState()->get<ChrootState>();
96138
if (!k)
97139
return;
98140

@@ -104,15 +146,35 @@ void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
104146
R = R->StripCasts();
105147
if (const StringRegion* StrRegion= dyn_cast<StringRegion>(R)) {
106148
const StringLiteral* Str = StrRegion->getStringLiteral();
107-
if (Str->getString() == "/")
108-
state = Mgr.addGDM(state, ChrootChecker::getTag(),
109-
(void*) JAIL_ENTERED);
149+
if (Str->getString() == "/") {
150+
state = state->set<ChrootState>(JAIL_ENTERED);
151+
}
110152
}
111153
}
112154

113155
C.addTransition(state);
114156
}
115157

158+
const ExplodedNode *ChrootChecker::getAcquisitionSite(const ExplodedNode *N,
159+
CheckerContext &C) {
160+
ProgramStateRef State = N->getState();
161+
// When bug type is resource leak, exploded node N may not have state info
162+
// for leaked file descriptor, but predecessor should have it.
163+
if (!State->get<ChrootCall>())
164+
N = N->getFirstPred();
165+
166+
const ExplodedNode *Pred = N;
167+
while (N) {
168+
State = N->getState();
169+
if (!State->get<ChrootCall>())
170+
return Pred;
171+
Pred = N;
172+
N = N->getFirstPred();
173+
}
174+
175+
return nullptr;
176+
}
177+
116178
// Check the jail state before any function call except chroot and chdir().
117179
void ChrootChecker::checkPreCall(const CallEvent &Call,
118180
CheckerContext &C) const {
@@ -121,17 +183,40 @@ void ChrootChecker::checkPreCall(const CallEvent &Call,
121183
return;
122184

123185
// 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-
}
186+
const ChrootKind k = C.getState()->get<ChrootState>();
187+
if (k == ROOT_CHANGED) {
188+
ExplodedNode *Err =
189+
C.generateNonFatalErrorNode(C.getState(), C.getPredecessor());
190+
if (!Err)
191+
return;
192+
const Expr *ChrootExpr = C.getState()->get<ChrootCall>();
193+
194+
const ExplodedNode *ChrootCallNode = getAcquisitionSite(Err, C);
195+
assert(ChrootCallNode && "Could not find place of stream opening.");
196+
197+
PathDiagnosticLocation LocUsedForUniqueing;
198+
if (const Stmt *ChrootStmt = ChrootCallNode->getStmtForDiagnostics())
199+
LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
200+
ChrootStmt, C.getSourceManager(),
201+
ChrootCallNode->getLocationContext());
202+
203+
std::unique_ptr<PathSensitiveBugReport> R =
204+
std::make_unique<PathSensitiveBugReport>(
205+
BT_BreakJail, "No call of chdir(\"/\") immediately after chroot",
206+
Err, LocUsedForUniqueing,
207+
ChrootCallNode->getLocationContext()->getDecl());
208+
209+
R->addNote("chroot called here",
210+
PathDiagnosticLocation::create(ChrootCallNode->getLocation(),
211+
C.getSourceManager()),
212+
{ChrootExpr->getSourceRange()});
213+
214+
C.emitReport(std::move(R));
215+
}
133216
}
134217

218+
} // namespace
219+
135220
void ento::registerChrootChecker(CheckerManager &mgr) {
136221
mgr.registerChecker<ChrootChecker>();
137222
}

‎clang/test/Analysis/chroot.c

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Chroot -verify %s
1+
// RUN: %clang_analyze_cc1 -analyzer-checker=unix.Chroot -verify %s
22

33
extern int chroot(const char* path);
44
extern int chdir(const char* path);
@@ -7,7 +7,7 @@ void foo(void) {
77
}
88

99
void f1(void) {
10-
chroot("/usr/local"); // root changed.
10+
chroot("/usr/local"); // expected-note {{chroot called here}}
1111
foo(); // expected-warning {{No call of chdir("/") immediately after chroot}}
1212
}
1313

@@ -18,7 +18,37 @@ void f2(void) {
1818
}
1919

2020
void f3(void) {
21-
chroot("/usr/local"); // root changed.
21+
chroot("/usr/local"); // expected-note {{chroot called here}}
2222
chdir("../"); // change working directory, still out of jail.
2323
foo(); // expected-warning {{No call of chdir("/") immediately after chroot}}
2424
}
25+
26+
void f4(void) {
27+
if (chroot("/usr/local") == 0) {
28+
chdir("../"); // change working directory, still out of jail.
29+
}
30+
}
31+
32+
void f5(void) {
33+
int v = chroot("/usr/local");
34+
if (v == -1) {
35+
foo(); // no warning, chroot failed
36+
chdir("../"); // change working directory, still out of jail.
37+
}
38+
}
39+
40+
void f6(void) {
41+
if (chroot("/usr/local") == -1) {
42+
chdir("../"); // change working directory, still out of jail.
43+
}
44+
}
45+
46+
void f7(void) {
47+
int v = chroot("/usr/local"); // expected-note {{chroot called here}}
48+
if (v == -1) {
49+
foo(); // no warning, chroot failed
50+
chdir("../"); // change working directory, still out of jail.
51+
} else {
52+
foo(); // expected-warning {{No call of chdir("/") immediately after chroot}}
53+
}
54+
}

0 commit comments

Comments
 (0)