<div dir="ltr"><div>Updating ChrootChecker's patch.</div><div><br></div><div>Improve warning:</div><div> - Convert evalCall to checkPostCall.</div><div> - Assume chroot's return value false state and True state. False</div>
<div> state is treated as returning zero.</div><div> - Add new state 'IMPROPER_USE' for checking whether ChrootChecker</div><div> detects or not.</div><div>Clean up:</div><div> - Convert checkPostStmt to checkPostCall.</div>
<div> - Convert direct using GDM to using ProgramStateRef's get and set.</div><div><br></div><div><br></div><div>Index: lib/StaticAnalyzer/Checkers/ChrootChecker.cpp</div><div>===================================================================</div>
<div>--- lib/StaticAnalyzer/Checkers/ChrootChecker.cpp<span class="" style="white-space:pre"> </span>(revision 202679)</div><div>+++ lib/StaticAnalyzer/Checkers/ChrootChecker.cpp<span class="" style="white-space:pre"> </span>(working copy)</div>
<div>@@ -16,6 +16,7 @@</div><div> #include "clang/StaticAnalyzer/Core/Checker.h"</div><div> #include "clang/StaticAnalyzer/Core/CheckerManager.h"</div><div> #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"</div>
<div>+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"</div><div> #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"</div><div> #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"</div>
<div> #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"</div><div>@@ -25,95 +26,107 @@</div><div> </div><div> namespace {</div><div> </div><div>-// enum value that represent the jail state</div>
<div>-enum Kind { NO_CHROOT, ROOT_CHANGED, JAIL_ENTERED };</div><div>+struct ChrootState {</div><div>+private:</div><div>+ // enum value that represent the jail state</div><div>+ enum Kind { ROOT_CHANGED, JAIL_ENTERED, IMPROPER_USE } K;</div>
<div>+ ChrootState(Kind InK) : K(InK) {}</div><div> </div><div>-bool isRootChanged(intptr_t k) { return k == ROOT_CHANGED; }</div><div>-//bool isJailEntered(intptr_t k) { return k == JAIL_ENTERED; }</div><div>+public:</div>
<div>+ bool isRootChanged() const { return K == ROOT_CHANGED; }</div><div>+ bool isJailEntered() const { return K == JAIL_ENTERED; }</div><div>+ bool isImproperUse() const { return K == IMPROPER_USE; }</div><div> </div>
<div>+ static ChrootState getRootChanged() { return ChrootState(ROOT_CHANGED); }</div><div>+ static ChrootState getJailEntered() { return ChrootState(JAIL_ENTERED); }</div><div>+ static ChrootState getImproperUse() { return ChrootState(IMPROPER_USE); }</div>
<div>+</div><div>+ bool operator==(const ChrootState &CS) const { return K == CS.K; }</div><div>+ void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(K); }</div><div>+};</div><div> // This checker checks improper use of chroot.</div>
<div> // The state transition:</div><div> // NO_CHROOT ---chroot(path)--> ROOT_CHANGED ---chdir(/) --> JAIL_ENTERED</div><div> // | |</div><div> // ROOT_CHANGED<--chdir(..)-- JAIL_ENTERED<--chdir(..)--</div>
<div> // | |</div><div>-// bug<--foo()-- JAIL_ENTERED<--foo()--</div><div>-class ChrootChecker : public Checker<eval::Call, check::PreStmt<CallExpr> > {</div>
<div>+// IMPROPER_USE<--foo()-- JAIL_ENTERED<--foo()--</div><div>+class ChrootChecker : public Checker< check::PreCall, check::PostCall > {</div><div>+private:</div><div> mutable IdentifierInfo *II_chroot, *II_chdir;</div>
<div> // This bug refers to possibly break out of a chroot() jail.</div><div> mutable OwningPtr<BuiltinBug> BT_BreakJail;</div><div> </div><div>+ void initIdentifierInfo(ASTContext &Ctx) const {</div><div>+ if (!II_chroot)</div>
<div>+ II_chroot = &Ctx.Idents.get("chroot");</div><div>+ if (!II_chdir)</div><div>+ II_chdir = &Ctx.Idents.get("chdir");</div><div>+ }</div><div>+</div><div> public:</div><div>- ChrootChecker() : II_chroot(0), II_chdir(0) {}</div>
<div>- </div><div>+ ChrootChecker() : II_chroot(0), II_chdir(0) {</div><div>+ BT_BreakJail.reset(new BuiltinBug(this, "Break out of jail",</div><div>+ "No call of chdir(\"/\") immediately "</div>
<div>+ "after chroot"));</div><div>+ }</div><div>+</div><div> static void *getTag() {</div><div> static int x;</div><div> return &x;</div><div> }</div><div> </div>
<div>- bool evalCall(const CallExpr *CE, CheckerContext &C) const;</div><div>- void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;</div><div>+ void checkPreCall(const CallEvent &Call, CheckerContext &C) const;</div>
<div>+ void checkPostCall(const CallEvent &Call, CheckerContext &C) const;</div><div> </div><div> private:</div><div>- void Chroot(CheckerContext &C, const CallExpr *CE) const;</div><div>- void Chdir(CheckerContext &C, const CallExpr *CE) const;</div>
<div>+ void Chroot(const CallEvent &Call, CheckerContext &C) const;</div><div>+ void Chdir(const CallEvent &Call, CheckerContext &C) const;</div><div> };</div><div> </div><div> } // end anonymous namespace</div>
<div>+REGISTER_MAP_WITH_PROGRAMSTATE(ChrootMap, void *, ChrootState)</div><div> </div><div>-bool ChrootChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {</div><div>- const FunctionDecl *FD = C.getCalleeDecl(CE);</div>
<div>- if (!FD)</div><div>- return false;</div><div>-</div><div>- ASTContext &Ctx = C.getASTContext();</div><div>- if (!II_chroot)</div><div>- II_chroot = &Ctx.Idents.get("chroot");</div><div>- if (!II_chdir)</div>
<div>- II_chdir = &Ctx.Idents.get("chdir");</div><div>-</div><div>- if (FD->getIdentifier() == II_chroot) {</div><div>- Chroot(C, CE);</div><div>- return true;</div><div>- }</div><div>- if (FD->getIdentifier() == II_chdir) {</div>
<div>- Chdir(C, CE);</div><div>- return true;</div><div>- }</div><div>-</div><div>- return false;</div><div>+void ChrootChecker::checkPostCall(const CallEvent &Call,</div><div>+ CheckerContext &C) const {</div>
<div>+ initIdentifierInfo(C.getASTContext());</div><div>+ const IdentifierInfo *II = Call.getCalleeIdentifier();</div><div>+ if (II == II_chroot)</div><div>+ Chroot(Call, C);</div><div>+ else if (II == II_chdir)</div>
<div>+ Chdir(Call, C);</div><div> }</div><div> </div><div>-void ChrootChecker::Chroot(CheckerContext &C, const CallExpr *CE) const {</div><div>+void ChrootChecker::Chroot(const CallEvent &Call, CheckerContext &C) const {</div>
<div> ProgramStateRef state = C.getState();</div><div>- ProgramStateManager &Mgr = state->getStateManager();</div><div>+ DefinedOrUnknownSVal RetVal</div><div>+ = Call.getReturnValue().castAs<DefinedOrUnknownSVal>();</div>
<div>+ ProgramStateRef Fail, Success;</div><div>+ std::tie(Fail, Success) = state->assume(RetVal);</div><div> </div><div>- // Once encouter a chroot(), set the enum value ROOT_CHANGED directly in </div><div>- // the GDM.</div>
<div>- state = Mgr.addGDM(state, ChrootChecker::getTag(), (void*) ROOT_CHANGED);</div><div>- C.addTransition(state);</div><div>+ // Once encouter a chroot(), set the enum value ROOT_CHANGED</div><div>+ Success = Success->set<ChrootMap>(ChrootChecker::getTag(),</div>
<div>+ ChrootState::getRootChanged());</div><div>+ C.addTransition(Success);</div><div>+ C.addTransition(Fail);</div><div> }</div><div> </div><div>-void ChrootChecker::Chdir(CheckerContext &C, const CallExpr *CE) const {</div>
<div>+void ChrootChecker::Chdir(const CallEvent &Call, CheckerContext &C) const {</div><div> ProgramStateRef state = C.getState();</div><div>- ProgramStateManager &Mgr = state->getStateManager();</div><div>
-</div><div>- // If there are no jail state in the GDM, just return.</div><div>- const void *k = state->FindGDM(ChrootChecker::getTag());</div><div>- if (!k)</div><div>+ // If jail state is not ROOT_CHANGED, just return.</div>
<div>+ const ChrootState *CS = state->get<ChrootMap>(ChrootChecker::getTag());</div><div>+ if (!CS || CS->isJailEntered())</div><div> return;</div><div> </div><div> // After chdir("/"), enter the jail, set the enum value JAIL_ENTERED.</div>
<div>- const Expr *ArgExpr = CE->getArg(0);</div><div>- SVal ArgVal = state->getSVal(ArgExpr, C.getLocationContext());</div><div>- </div><div>+ SVal ArgVal = Call.getArgSVal(0);</div><div> if (const MemRegion *R = ArgVal.getAsRegion()) {</div>
<div> R = R->StripCasts();</div><div> if (const StringRegion* StrRegion= dyn_cast<StringRegion>(R)) {</div><div> const StringLiteral* Str = StrRegion->getStringLiteral();</div><div> if (Str->getString() == "/")</div>
<div>- state = Mgr.addGDM(state, ChrootChecker::getTag(),</div><div>- (void*) JAIL_ENTERED);</div><div>+ state = state->set<ChrootMap>(ChrootChecker::getTag(),</div><div>+ ChrootState::getJailEntered());</div>
<div> }</div><div> }</div><div> </div><div>@@ -121,36 +134,27 @@</div><div> }</div><div> </div><div> // Check the jail state before any function call except chroot and chdir().</div><div>-void ChrootChecker::checkPreStmt(const CallExpr *CE, CheckerContext &C) const {</div>
<div>- const FunctionDecl *FD = C.getCalleeDecl(CE);</div><div>- if (!FD)</div><div>- return;</div><div>-</div><div>- ASTContext &Ctx = C.getASTContext();</div><div>- if (!II_chroot)</div><div>- II_chroot = &Ctx.Idents.get("chroot");</div>
<div>- if (!II_chdir)</div><div>- II_chdir = &Ctx.Idents.get("chdir");</div><div>-</div><div>+void ChrootChecker::checkPreCall(const CallEvent &Call,</div><div>+ CheckerContext &C) const {</div>
<div>+ initIdentifierInfo(C.getASTContext());</div><div>+ const IdentifierInfo *II = Call.getCalleeIdentifier();</div><div> // Ingnore chroot and chdir.</div><div>- if (FD->getIdentifier() == II_chroot || FD->getIdentifier() == II_chdir)</div>
<div>+ if (II == II_chroot || II == II_chdir)</div><div> return;</div><div> </div><div> // If jail state is ROOT_CHANGED, generate BugReport.</div><div>- void *const* k = C.getState()->FindGDM(ChrootChecker::getTag());</div>
<div>- if (k)</div><div>- if (isRootChanged((intptr_t) *k))</div><div>- if (ExplodedNode *N = C.addTransition()) {</div><div>- if (!BT_BreakJail)</div><div>- BT_BreakJail.reset(new BuiltinBug(</div>
<div>- this, "Break out of jail", "No call of chdir(\"/\") immediately "</div><div>- "after chroot"));</div><div>- BugReport *R = new BugReport(*BT_BreakJail, </div>
<div>- BT_BreakJail->getDescription(), N);</div><div>- C.emitReport(R);</div><div>- }</div><div>- </div><div>- return;</div><div>+ ProgramStateRef state = C.getState();</div>
<div>+ const ChrootState *CS = state->get<ChrootMap>(ChrootChecker::getTag());</div><div>+ if (!CS || !CS->isRootChanged())</div><div>+ return;</div><div>+</div><div>+ state = state->set<ChrootMap>(ChrootChecker::getTag(),</div>
<div>+ ChrootState::getImproperUse());</div><div>+ if (ExplodedNode *N = C.addTransition(state)) {</div><div>+ BugReport *R = new BugReport(*BT_BreakJail, </div><div>+ BT_BreakJail->getDescription(), N);</div>
<div>+ C.emitReport(R);</div><div>+ }</div><div> }</div><div> </div><div> void ento::registerChrootChecker(CheckerManager &mgr) {</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
2014-03-04 15:31 GMT+09:00 Hiroo MATSUMOTO <span dir="ltr"><<a href="mailto:hiroom2.mail@gmail.com" target="_blank">hiroom2.mail@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div>Hi, Jordan</div><div><br></div><div><br></div><div>As you say, this patch negatively effects other checker's behavior</div><div>like missing malloc free.</div><div>I'll try ways of SimpleStreamChecker and MallocChecker.</div>
<div><br></div><div><br></div><div>Regards.</div><div>Hiroo MATSUMOTO</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-03-04 11:28 GMT+09:00 Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span>:<div>
<div class="h5"><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br>
<br>
That is, this test case should still warn:<br>
<br>
int *buf = malloc(4);<br>
if (chroot("/") < 0)<br>
return; // forgot to free buf!<br>
<br>
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.<br>
<br>
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.<br>
<br>
Thanks for pointing out the errors. If you're not interested in solving them yourself right now, please file a bug at <a href="http://llvm.org/bugs/" target="_blank">http://llvm.org/bugs/</a>.<br>
<br>
Jordan<br>
<div><div><br>
<br>
On Mar 3, 2014, at 3:19 , Hiroo MATSUMOTO <<a href="mailto:hiroom2.mail@gmail.com" target="_blank">hiroom2.mail@gmail.com</a>> wrote:<br>
<br>
> ChrootChecker tracks a chroot failed case. It will generate warning<br>
> even though chroot is used properly.<br>
><br>
> When finding improper using chroot, ChrootChecker doesn't stop<br>
> tracking. It will generate verbose warning.<br>
><br>
> For example, ChrootChecker will generate warnings from below code<br>
> which can switch proper using and improper using with IMPROPER_USE.<br>
><br>
> When IMPROPER_USE is not defined, 1 warning will be generated.<br>
> When IMPROPER_USE is defined, 3 warnings will be generated.<br>
><br>
><br>
> #include <stdio.h><br>
> #include <stdlib.h><br>
> #include <unistd.h><br>
><br>
> int main(int argc, char *argv[])<br>
> {<br>
> if (argc < 2) {<br>
> fprintf(stderr, "usage: %s newroot\n", argv[0]);<br>
> return 1;<br>
> }<br>
><br>
> if (chroot(argv[1]) < 0) {<br>
> perror("chroot"); /** proper using and improper using */<br>
> return 1;<br>
> }<br>
><br>
> #ifndef IMPROPER_USE<br>
> if (chdir("/") < 0) {<br>
> perror("chdir");<br>
> return 1;<br>
> }<br>
> #endif<br>
><br>
> if (execv("/bin/sh", argv) < 0) { /** improper using */<br>
> perror("execv"); /** improper using */<br>
> return 1;<br>
> }<br>
><br>
> return 0;<br>
> }<br>
><br>
><br>
> This patch will bind return value of chroot to zero. And this patch<br>
> will stop tracking when finding improper using chroot.<br>
><br>
><br>
> Index: lib/StaticAnalyzer/Checkers/ChrootChecker.cpp<br>
> ===================================================================<br>
> --- lib/StaticAnalyzer/Checkers/ChrootChecker.cpp (revision 202679)<br>
> +++ lib/StaticAnalyzer/Checkers/ChrootChecker.cpp (working copy)<br>
> @@ -87,11 +87,13 @@<br>
> void ChrootChecker::Chroot(CheckerContext &C, const CallExpr *CE) const {<br>
> ProgramStateRef state = C.getState();<br>
> ProgramStateManager &Mgr = state->getStateManager();<br>
> + SValBuilder &svalBuilder = C.getSValBuilder();<br>
> + SVal success = svalBuilder.makeZeroVal(svalBuilder.getContext().IntTy);<br>
><br>
> // Once encouter a chroot(), set the enum value ROOT_CHANGED directly in<br>
> // the GDM.<br>
> state = Mgr.addGDM(state, ChrootChecker::getTag(), (void*) ROOT_CHANGED);<br>
> - C.addTransition(state);<br>
> + C.addTransition(state->BindExpr(CE, C.getLocationContext(), success));<br>
> }<br>
><br>
> void ChrootChecker::Chdir(CheckerContext &C, const CallExpr *CE) const {<br>
> @@ -140,7 +142,7 @@<br>
> void *const* k = C.getState()->FindGDM(ChrootChecker::getTag());<br>
> if (k)<br>
> if (isRootChanged((intptr_t) *k))<br>
> - if (ExplodedNode *N = C.addTransition()) {<br>
> + if (ExplodedNode *N = C.generateSink()) {<br>
> if (!BT_BreakJail)<br>
> BT_BreakJail.reset(new BuiltinBug(<br>
> this, "Break out of jail", "No call of chdir(\"/\") immediately "<br>
><br>
</div></div>> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</blockquote></div></div></div><br></div>
</blockquote></div><br></div>