[PATCH] ChrootChecker: Bind chroot's result zero and reduce verbose warning
Hiroo MATSUMOTO
hiroom2.mail at gmail.com
Wed Mar 5 09:41:46 PST 2014
Updating ChrootChecker's patch.
Improve warning:
- Convert evalCall to checkPostCall.
- Assume chroot's return value false state and True state. False
state is treated as returning zero.
- Add new state 'IMPROPER_USE' for checking whether ChrootChecker
detects or not.
Clean up:
- Convert checkPostStmt to checkPostCall.
- Convert direct using GDM to using ProgramStateRef's get and set.
Index: lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/ChrootChecker.cpp (revision 202679)
+++ lib/StaticAnalyzer/Checkers/ChrootChecker.cpp (working copy)
@@ -16,6 +16,7 @@
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
@@ -25,95 +26,107 @@
namespace {
-// enum value that represent the jail state
-enum Kind { NO_CHROOT, ROOT_CHANGED, JAIL_ENTERED };
+struct ChrootState {
+private:
+ // enum value that represent the jail state
+ enum Kind { ROOT_CHANGED, JAIL_ENTERED, IMPROPER_USE } K;
+ ChrootState(Kind InK) : K(InK) {}
-bool isRootChanged(intptr_t k) { return k == ROOT_CHANGED; }
-//bool isJailEntered(intptr_t k) { return k == JAIL_ENTERED; }
+public:
+ bool isRootChanged() const { return K == ROOT_CHANGED; }
+ bool isJailEntered() const { return K == JAIL_ENTERED; }
+ bool isImproperUse() const { return K == IMPROPER_USE; }
+ static ChrootState getRootChanged() { return ChrootState(ROOT_CHANGED); }
+ static ChrootState getJailEntered() { return ChrootState(JAIL_ENTERED); }
+ static ChrootState getImproperUse() { return ChrootState(IMPROPER_USE); }
+
+ bool operator==(const ChrootState &CS) const { return K == CS.K; }
+ void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(K); }
+};
// This checker checks improper use of chroot.
// The state transition:
// 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::PreStmt<CallExpr>
> {
+// IMPROPER_USE<--foo()-- JAIL_ENTERED<--foo()--
+class ChrootChecker : public Checker< check::PreCall, check::PostCall > {
+private:
mutable IdentifierInfo *II_chroot, *II_chdir;
// This bug refers to possibly break out of a chroot() jail.
mutable OwningPtr<BuiltinBug> BT_BreakJail;
+ void initIdentifierInfo(ASTContext &Ctx) const {
+ if (!II_chroot)
+ II_chroot = &Ctx.Idents.get("chroot");
+ if (!II_chdir)
+ II_chdir = &Ctx.Idents.get("chdir");
+ }
+
public:
- ChrootChecker() : II_chroot(0), II_chdir(0) {}
-
+ ChrootChecker() : II_chroot(0), II_chdir(0) {
+ BT_BreakJail.reset(new BuiltinBug(this, "Break out of jail",
+ "No call of chdir(\"/\") immediately
"
+ "after chroot"));
+ }
+
static void *getTag() {
static int x;
return &x;
}
- bool evalCall(const CallExpr *CE, CheckerContext &C) const;
- void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
+ void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+ void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
private:
- void Chroot(CheckerContext &C, const CallExpr *CE) const;
- void Chdir(CheckerContext &C, const CallExpr *CE) const;
+ void Chroot(const CallEvent &Call, CheckerContext &C) const;
+ void Chdir(const CallEvent &Call, CheckerContext &C) const;
};
} // end anonymous namespace
+REGISTER_MAP_WITH_PROGRAMSTATE(ChrootMap, void *, ChrootState)
-bool ChrootChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {
- const FunctionDecl *FD = C.getCalleeDecl(CE);
- if (!FD)
- return false;
-
- ASTContext &Ctx = C.getASTContext();
- if (!II_chroot)
- II_chroot = &Ctx.Idents.get("chroot");
- if (!II_chdir)
- II_chdir = &Ctx.Idents.get("chdir");
-
- if (FD->getIdentifier() == II_chroot) {
- Chroot(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_chdir) {
- Chdir(C, CE);
- return true;
- }
-
- return false;
+void ChrootChecker::checkPostCall(const CallEvent &Call,
+ CheckerContext &C) const {
+ initIdentifierInfo(C.getASTContext());
+ const IdentifierInfo *II = Call.getCalleeIdentifier();
+ if (II == II_chroot)
+ Chroot(Call, C);
+ else if (II == II_chdir)
+ Chdir(Call, C);
}
-void ChrootChecker::Chroot(CheckerContext &C, const CallExpr *CE) const {
+void ChrootChecker::Chroot(const CallEvent &Call, CheckerContext &C) const
{
ProgramStateRef state = C.getState();
- ProgramStateManager &Mgr = state->getStateManager();
+ DefinedOrUnknownSVal RetVal
+ = Call.getReturnValue().castAs<DefinedOrUnknownSVal>();
+ ProgramStateRef Fail, Success;
+ std::tie(Fail, Success) = state->assume(RetVal);
- // 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);
+ // Once encouter a chroot(), set the enum value ROOT_CHANGED
+ Success = Success->set<ChrootMap>(ChrootChecker::getTag(),
+ ChrootState::getRootChanged());
+ C.addTransition(Success);
+ C.addTransition(Fail);
}
-void ChrootChecker::Chdir(CheckerContext &C, const CallExpr *CE) const {
+void ChrootChecker::Chdir(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 (!k)
+ // If jail state is not ROOT_CHANGED, just return.
+ const ChrootState *CS = state->get<ChrootMap>(ChrootChecker::getTag());
+ if (!CS || CS->isJailEntered())
return;
// After chdir("/"), enter the jail, set the enum value JAIL_ENTERED.
- const Expr *ArgExpr = CE->getArg(0);
- SVal ArgVal = state->getSVal(ArgExpr, C.getLocationContext());
-
+ 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);
+ state = state->set<ChrootMap>(ChrootChecker::getTag(),
+ ChrootState::getJailEntered());
}
}
@@ -121,36 +134,27 @@
}
// Check the jail state before any function call except chroot and chdir().
-void ChrootChecker::checkPreStmt(const CallExpr *CE, CheckerContext &C)
const {
- const FunctionDecl *FD = C.getCalleeDecl(CE);
- if (!FD)
- return;
-
- ASTContext &Ctx = C.getASTContext();
- if (!II_chroot)
- II_chroot = &Ctx.Idents.get("chroot");
- if (!II_chdir)
- II_chdir = &Ctx.Idents.get("chdir");
-
+void ChrootChecker::checkPreCall(const CallEvent &Call,
+ CheckerContext &C) const {
+ initIdentifierInfo(C.getASTContext());
+ const IdentifierInfo *II = Call.getCalleeIdentifier();
// Ingnore chroot and chdir.
- if (FD->getIdentifier() == II_chroot || FD->getIdentifier() == II_chdir)
+ if (II == II_chroot || II == II_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.addTransition()) {
- if (!BT_BreakJail)
- BT_BreakJail.reset(new BuiltinBug(
- this, "Break out of jail", "No call of chdir(\"/\")
immediately "
- "after chroot"));
- BugReport *R = new BugReport(*BT_BreakJail,
- BT_BreakJail->getDescription(), N);
- C.emitReport(R);
- }
-
- return;
+ ProgramStateRef state = C.getState();
+ const ChrootState *CS = state->get<ChrootMap>(ChrootChecker::getTag());
+ if (!CS || !CS->isRootChanged())
+ return;
+
+ state = state->set<ChrootMap>(ChrootChecker::getTag(),
+ ChrootState::getImproperUse());
+ if (ExplodedNode *N = C.addTransition(state)) {
+ BugReport *R = new BugReport(*BT_BreakJail,
+ BT_BreakJail->getDescription(), N);
+ C.emitReport(R);
+ }
}
void ento::registerChrootChecker(CheckerManager &mgr) {
2014-03-04 15:31 GMT+09:00 Hiroo MATSUMOTO <hiroom2.mail at gmail.com>:
> Hi, Jordan
>
>
> As you say, this patch negatively effects other checker's behavior
> like missing malloc free.
> I'll try ways of SimpleStreamChecker and MallocChecker.
>
>
> Regards.
> Hiroo MATSUMOTO
>
>
>
> 2014-03-04 11:28 GMT+09:00 Jordan Rose <jordan_rose at apple.com>:
>
> I don't think we want to assume the chroot always succeeds, and we
>> definitely don't want to stop processing if there's an error (because there
>> could be unrelated mistakes). Instead, we should check when we later access
>> the chroot-state that it hasn't already failed.
>>
>> That is, this test case should still warn:
>>
>> int *buf = malloc(4);
>> if (chroot("/") < 0)
>> return; // forgot to free buf!
>>
>> There's precedent for this in SimpleStreamChecker, which checks if a
>> symbol has been constrained to null before declaring that it's leaked, and
>> in MallocChecker, which tracks realloc failures to warn that the original
>> buffer is still live. Neither of those are quite what you'd want here, but
>> they're a good start.
>>
>> This checker isn't written very well anyway (for example, using FindGDM
>> and addGDM directly instead of going through ProgramState's get and set).
>> If you're interested, cleaning it up would be a nice improvement.
>>
>> Thanks for pointing out the errors. If you're not interested in solving
>> them yourself right now, please file a bug at http://llvm.org/bugs/.
>>
>> Jordan
>>
>>
>> On Mar 3, 2014, at 3:19 , Hiroo MATSUMOTO <hiroom2.mail at gmail.com> wrote:
>>
>> > ChrootChecker tracks a chroot failed case. It will generate warning
>> > even though chroot is used properly.
>> >
>> > When finding improper using chroot, ChrootChecker doesn't stop
>> > tracking. It will generate verbose warning.
>> >
>> > For example, ChrootChecker will generate warnings from below code
>> > which can switch proper using and improper using with IMPROPER_USE.
>> >
>> > When IMPROPER_USE is not defined, 1 warning will be generated.
>> > When IMPROPER_USE is defined, 3 warnings will be generated.
>> >
>> >
>> > #include <stdio.h>
>> > #include <stdlib.h>
>> > #include <unistd.h>
>> >
>> > int main(int argc, char *argv[])
>> > {
>> > if (argc < 2) {
>> > fprintf(stderr, "usage: %s newroot\n", argv[0]);
>> > return 1;
>> > }
>> >
>> > if (chroot(argv[1]) < 0) {
>> > perror("chroot"); /** proper using and improper using */
>> > return 1;
>> > }
>> >
>> > #ifndef IMPROPER_USE
>> > if (chdir("/") < 0) {
>> > perror("chdir");
>> > return 1;
>> > }
>> > #endif
>> >
>> > if (execv("/bin/sh", argv) < 0) { /** improper using */
>> > perror("execv"); /** improper using */
>> > return 1;
>> > }
>> >
>> > return 0;
>> > }
>> >
>> >
>> > This patch will bind return value of chroot to zero. And this patch
>> > will stop tracking when finding improper using chroot.
>> >
>> >
>> > Index: lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
>> > ===================================================================
>> > --- lib/StaticAnalyzer/Checkers/ChrootChecker.cpp (revision 202679)
>> > +++ lib/StaticAnalyzer/Checkers/ChrootChecker.cpp (working copy)
>> > @@ -87,11 +87,13 @@
>> > void ChrootChecker::Chroot(CheckerContext &C, const CallExpr *CE)
>> const {
>> > ProgramStateRef state = C.getState();
>> > ProgramStateManager &Mgr = state->getStateManager();
>> > + SValBuilder &svalBuilder = C.getSValBuilder();
>> > + SVal success =
>> svalBuilder.makeZeroVal(svalBuilder.getContext().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);
>> > + C.addTransition(state->BindExpr(CE, C.getLocationContext(),
>> success));
>> > }
>> >
>> > void ChrootChecker::Chdir(CheckerContext &C, const CallExpr *CE) const
>> {
>> > @@ -140,7 +142,7 @@
>> > void *const* k = C.getState()->FindGDM(ChrootChecker::getTag());
>> > if (k)
>> > if (isRootChanged((intptr_t) *k))
>> > - if (ExplodedNode *N = C.addTransition()) {
>> > + if (ExplodedNode *N = C.generateSink()) {
>> > if (!BT_BreakJail)
>> > BT_BreakJail.reset(new BuiltinBug(
>> > this, "Break out of jail", "No call of chdir(\"/\")
>> immediately "
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140306/e368c03b/attachment.html>
More information about the cfe-commits
mailing list