[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 52e01b619c15aab0ab13822dace5f20f278cca0c..07970cfda78387f02b127df713dbe47980d54637 100644 (file)
--- 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<GenericTaintChecker>();
 }

diff --git a/test/Analysis/taint-generic.c b/test/Analysis/taint-generic.c
index 2d00f4daa9184af1e61f29ec1a470111d4f9eb89..c50f7193445bed843dc5ac76c86ad584d6699a8e 100644 (file)
--- 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
+
+}