From: Anna Zaks Date: Wed, 18 Jan 2012 02:45:11 +0000 (+0000) Subject: [analyzer] Taint: warn when tainted data is used to specify a buffer X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=4e46221e38b7d434fbecb1cd56b259437206d246;p=clang [analyzer] Taint: warn when tainted data is used to specify a buffer size (Ex: in malloc, memcpy, strncpy..) (Maybe some of this could migrate to the CString checker. One issue with that is that we might want to separate security issues from regular API misuse.) git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@148371 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp index 52e01b619c..07970cfda7 100644 --- a/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp @@ -99,6 +99,12 @@ private: bool checkSystemCall(const CallExpr *CE, StringRef Name, CheckerContext &C) const; + /// Check if tainted data is used as a buffer size ins strn.. functions, + /// and allocators. + static const char MsgTaintedBufferSize[]; + bool checkTaintedBufferSize(const CallExpr *CE, const FunctionDecl *FDecl, + CheckerContext &C) const; + /// Generate a report if the expression is tainted or points to tainted data. bool generateReportIfTainted(const Expr *E, const char Msg[], CheckerContext &C) const; @@ -176,7 +182,13 @@ const char GenericTaintChecker::MsgUncontrolledFormatString[] = const char GenericTaintChecker::MsgSanitizeSystemArgs[] = "Tainted data passed to a system call " "(CERT/STR02-C. Sanitize data passed to complex subsystems)"; -} + +const char GenericTaintChecker::MsgTaintedBufferSize[] = + "Tainted data is used to specify the buffer size " + "(CERT/STR31-C. Guarantee that storage for strings has sufficient space for " + "character data and the null terminator)"; + +} // end of anonymous namespace /// A set which is used to pass information from call pre-visit instruction /// to the call post-visit. The values are unsigned integers, which are either @@ -242,8 +254,8 @@ GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule( else if (C.isCLibraryFunction(FDecl, "strdup") || C.isCLibraryFunction(FDecl, "strdupa")) return TaintPropagationRule(0, ReturnValueIndex); - else if (C.isCLibraryFunction(FDecl, "strndupa")) - return TaintPropagationRule(0, 1, ReturnValueIndex); + else if (C.isCLibraryFunction(FDecl, "wcsdup")) + return TaintPropagationRule(0, ReturnValueIndex); } // Skipping the following functions, since they might be used for cleansing @@ -371,13 +383,17 @@ bool GenericTaintChecker::checkPre(const CallExpr *CE, CheckerContext &C) const{ if (checkUncontrolledFormatString(CE, C)) return true; - StringRef Name = C.getCalleeName(CE); + const FunctionDecl *FDecl = C.getCalleeDecl(CE); + StringRef Name = C.getCalleeName(FDecl); if (Name.empty()) return false; if (checkSystemCall(CE, Name, C)) return true; + if (checkTaintedBufferSize(CE, FDecl, C)) + return true; + return false; } @@ -639,6 +655,48 @@ bool GenericTaintChecker::checkSystemCall(const CallExpr *CE, return false; } +// TODO: Should this check be a part of the CString checker? +// If yes, should taint be a global setting? +bool GenericTaintChecker::checkTaintedBufferSize(const CallExpr *CE, + const FunctionDecl *FDecl, + CheckerContext &C) const { + // If the function has a buffer size argument, set ArgNum. + unsigned ArgNum = InvalidArgIndex; + unsigned BId = 0; + if ( (BId = FDecl->getMemoryFunctionKind()) ) + switch(BId) { + case Builtin::BImemcpy: + case Builtin::BImemmove: + case Builtin::BIstrncpy: + ArgNum = 2; + break; + case Builtin::BIstrndup: + ArgNum = 1; + break; + default: + break; + }; + + if (ArgNum == InvalidArgIndex) { + if (C.isCLibraryFunction(FDecl, "malloc") || + C.isCLibraryFunction(FDecl, "calloc") || + C.isCLibraryFunction(FDecl, "alloca")) + ArgNum = 0; + else if (C.isCLibraryFunction(FDecl, "memccpy")) + ArgNum = 3; + else if (C.isCLibraryFunction(FDecl, "realloc")) + ArgNum = 1; + else if (C.isCLibraryFunction(FDecl, "bcopy")) + ArgNum = 2; + } + + if (ArgNum != InvalidArgIndex && + generateReportIfTainted(CE->getArg(ArgNum), MsgTaintedBufferSize, C)) + return true; + + return false; +} + void ento::registerGenericTaintChecker(CheckerManager &mgr) { mgr.registerChecker(); } diff --git a/test/Analysis/taint-generic.c b/test/Analysis/taint-generic.c index 2d00f4daa9..c50f719344 100644 --- a/test/Analysis/taint-generic.c +++ b/test/Analysis/taint-generic.c @@ -23,6 +23,11 @@ static char *__inline_strcpy_chk (char *dest, const char *src) { char *stpcpy(char *restrict s1, const char *restrict s2); char *strncpy( char * destination, const char * source, size_t num ); char *strndup(const char *s, size_t n); +char *strncat(char *restrict s1, const char *restrict s2, size_t n); + +void *malloc(size_t); +void *calloc(size_t nmemb, size_t size); +void bcopy(void *s1, void *s2, size_t n); #define BUFSIZE 10 @@ -112,6 +117,7 @@ void testTaintSystemCall() { sprintf(buffer, "/bin/mail %s < /tmp/email", addr); system(buffer); // expected-warning {{Tainted data passed to a system call}} } + void testTaintSystemCall2() { // Test that snpintf transfers taint. char buffern[156]; @@ -120,6 +126,7 @@ void testTaintSystemCall2() { __builtin_snprintf(buffern, 10, "/bin/mail %s < /tmp/email", addr); system(buffern); // expected-warning {{Tainted data passed to a system call}} } + void testTaintSystemCall3() { char buffern2[156]; int numt; @@ -128,3 +135,18 @@ void testTaintSystemCall3() { __builtin_snprintf(buffern2, numt, "/bin/mail %s < /tmp/email", "abcd"); system(buffern2); // expected-warning {{Tainted data passed to a system call}} } + +void testTaintedBufferSize() { + size_t ts; + scanf("%zd", &ts); + + int *buf1 = (int*)malloc(ts*sizeof(int)); // expected-warning {{Tainted data is used to specify the buffer size}} + char *dst = (char*)calloc(ts, sizeof(char)); //expected-warning {{Tainted data is used to specify the buffer size}} + bcopy(buf1, dst, ts); // expected-warning {{Tainted data is used to specify the buffer size}} + __builtin_memcpy(dst, buf1, (ts + 4)*sizeof(char)); // expected-warning {{Tainted data is used to specify the buffer size}} + + // If both buffers are trusted, do not issue a warning. + char *dst2 = (char*)malloc(ts*sizeof(char)); // expected-warning {{Tainted data is used to specify the buffer size}} + strncat(dst2, dst, ts); // no-warning + +}