From: Anna Zaks Date: Sat, 7 Jan 2012 02:33:10 +0000 (+0000) Subject: [analyzer] Add basic format string vulnerability checking. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=9f03b62036a7abc0a227b17f4a49b9eefced9450;p=clang [analyzer] Add basic format string vulnerability checking. We already have a more conservative check in the compiler (if the format string is not a literal, we warn). Still adding it here for completeness and since this check is stronger - only triggered if the format string is tainted. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@147714 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h b/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h index c3396d4eb1..7c9ea98698 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h @@ -308,7 +308,7 @@ public: bool isTainted(const Stmt *S, const LocationContext *LCtx, TaintTagType Kind = TaintTagGeneric) const; bool isTainted(SVal V, TaintTagType Kind = TaintTagGeneric) const; - bool isTainted(const SymExpr* Sym, TaintTagType Kind = TaintTagGeneric) const; + bool isTainted(SymbolRef Sym, TaintTagType Kind = TaintTagGeneric) const; bool isTainted(const MemRegion *Reg, TaintTagType Kind=TaintTagGeneric) const; //==---------------------------------------------------------------------==// diff --git a/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp index 9e67e21cc3..da57a190b4 100644 --- a/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp @@ -43,6 +43,15 @@ private: mutable llvm::OwningPtr BT; void initBugType() const; + /// Add/propagate taint on a post visit. + void taintPost(const CallExpr *CE, CheckerContext &C) const; + /// Add/propagate taint on a pre visit. + void taintPre(const CallExpr *CE, CheckerContext &C) const; + + /// Catch taint related bugs. Check if tainted data is passed to a system + /// call etc. + bool checkPre(const CallExpr *CE, CheckerContext &C) const; + /// Given a pointer argument, get the symbol of the value it contains /// (points to). SymbolRef getPointedToSymbol(CheckerContext &C, @@ -66,6 +75,10 @@ private: /// and thus, is tainted. bool isStdin(const Expr *E, CheckerContext &C) const; + /// Check for CWE-134: Uncontrolled Format String. + bool checkUncontrolledFormatString(const CallExpr *CE, + CheckerContext &C) const; + public: static void *getTag() { static int Tag; return &Tag; } @@ -94,13 +107,26 @@ namespace ento { inline void GenericTaintChecker::initBugType() const { if (!BT) - BT.reset(new BugType("Tainted data checking", "General")); + BT.reset(new BugType("Taint Analysis", "General")); } void GenericTaintChecker::checkPreStmt(const CallExpr *CE, CheckerContext &C) const { - const ProgramState *State = C.getState(); + // Check for errors first. + if (checkPre(CE, C)) + return; + // Add taint second. + taintPre(CE, C); +} + +void GenericTaintChecker::checkPostStmt(const CallExpr *CE, + CheckerContext &C) const { + taintPost(CE, C); +} + +void GenericTaintChecker::taintPre(const CallExpr *CE, + CheckerContext &C) const { // Set the evaluation function by switching on the callee name. StringRef Name = C.getCalleeName(CE); FnCheck evalFunction = llvm::StringSwitch(Name) @@ -111,6 +137,7 @@ void GenericTaintChecker::checkPreStmt(const CallExpr *CE, .Default(0); // Check and evaluate the call. + const ProgramState *State = 0; if (evalFunction) State = (this->*evalFunction)(CE, C); if (!State) @@ -119,10 +146,8 @@ void GenericTaintChecker::checkPreStmt(const CallExpr *CE, C.addTransition(State); } -void GenericTaintChecker::checkPostStmt(const CallExpr *CE, - CheckerContext &C) const { - const ProgramState *State = C.getState(); - +void GenericTaintChecker::taintPost(const CallExpr *CE, + CheckerContext &C) const { // Define the attack surface. // Set the evaluation function by switching on the callee name. StringRef Name = C.getCalleeName(CE); @@ -140,6 +165,7 @@ void GenericTaintChecker::checkPostStmt(const CallExpr *CE, // If the callee isn't defined, it is not of security concern. // Check and evaluate the call. + const ProgramState *State = 0; if (evalFunction) State = (this->*evalFunction)(CE, C); if (!State) @@ -150,6 +176,15 @@ void GenericTaintChecker::checkPostStmt(const CallExpr *CE, C.addTransition(State); } +bool GenericTaintChecker::checkPre(const CallExpr *CE, CheckerContext &C) const{ + + if (checkUncontrolledFormatString(CE, C)) + return true; + + StringRef Name = C.getCalleeName(CE); + return false; +} + SymbolRef GenericTaintChecker::getPointedToSymbol(CheckerContext &C, const Expr* Arg, bool IssueWarning) const { @@ -301,6 +336,57 @@ bool GenericTaintChecker::isStdin(const Expr *E, return false; } +static bool getPrintfFormatArgumentNum(const CallExpr *CE, + const CheckerContext &C, + unsigned int &ArgNum) { + // Find if the function contains a format string argument. + // Handles: fprintf, printf, sprintf, snprintf, vfprintf, vprintf, vsprintf, + // vsnprintf, syslog, custom annotated functions. + const FunctionDecl *FDecl = C.getCalleeDecl(CE); + if (!FDecl) + return false; + for (specific_attr_iterator + i = FDecl->specific_attr_begin(), + e = FDecl->specific_attr_end(); i != e ; ++i) { + + const FormatAttr *Format = *i; + ArgNum = Format->getFormatIdx() - 1; + if ((Format->getType() == "printf") && CE->getNumArgs() > ArgNum) + return true; + } + + // Or if a function is named setproctitle (this is a heuristic). + if (C.getCalleeName(CE).find("setproctitle") != StringRef::npos) { + ArgNum = 0; + return true; + } + + return false; +} + +bool GenericTaintChecker::checkUncontrolledFormatString(const CallExpr *CE, + CheckerContext &C) const{ + // Check if the function contains a format string argument. + unsigned int ArgNum = 0; + if (!getPrintfFormatArgumentNum(CE, C, ArgNum)) + return false; + + // If either the format string content or the pointer itself are tainted, warn. + const ProgramState *State = C.getState(); + const Expr *Arg = CE->getArg(ArgNum); + if (State->isTainted(getPointedToSymbol(C, Arg, false)) || + State->isTainted(Arg, C.getLocationContext())) + if (ExplodedNode *N = C.addTransition()) { + initBugType(); + BugReport *report = new BugReport(*BT, + "Tainted format string (CWE-134: Uncontrolled Format String)", N); + report->addRange(Arg->getSourceRange()); + C.EmitReport(report); + return true; + } + return false; +} + void ento::registerGenericTaintChecker(CheckerManager &mgr) { mgr.registerChecker(); } diff --git a/lib/StaticAnalyzer/Core/ProgramState.cpp b/lib/StaticAnalyzer/Core/ProgramState.cpp index 8a287e9e16..86a9ac0357 100644 --- a/lib/StaticAnalyzer/Core/ProgramState.cpp +++ b/lib/StaticAnalyzer/Core/ProgramState.cpp @@ -640,7 +640,7 @@ bool ProgramState::isTainted(const MemRegion *Reg, TaintTagType K) const { return false; } -bool ProgramState::isTainted(const SymExpr* Sym, TaintTagType Kind) const { +bool ProgramState::isTainted(SymbolRef Sym, TaintTagType Kind) const { if (!Sym) return false; diff --git a/test/Analysis/taint-generic.c b/test/Analysis/taint-generic.c index 431fad4fe7..a23d20f79f 100644 --- a/test/Analysis/taint-generic.c +++ b/test/Analysis/taint-generic.c @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -analyze -analyzer-checker=experimental.security.taint,experimental.security.ArrayBoundV2 -verify %s +// RUN: %clang_cc1 -analyze -analyzer-checker=experimental.security.taint,experimental.security.ArrayBoundV2 -Wno-format-security -verify %s int scanf(const char *restrict format, ...); int getchar(void); @@ -46,3 +46,17 @@ void bufferGetchar(int x) { int m = getchar(); Buffer[m] = 1; //expected-warning {{Out of bound memory access }} } + +typedef struct _FILE FILE; +extern FILE *stdin; +int fscanf(FILE *restrict stream, const char *restrict format, ...); +int sprintf(char *str, const char *format, ...); +void setproctitle(const char *fmt, ...); + +void testUncontrolledFormatString() { + char s[80]; + fscanf(stdin, "%s", s); + char buf[128]; + sprintf(buf,s); // expected-warning {{Uncontrolled Format String}} + setproctitle(s, 3); // expected-warning {{Uncontrolled Format String}} +}