Add security syntax checker for strcpy() which causes the Static Analyzer to generate a warning any time the strcpy() function is used with a note suggesting to use a function which provides bounded buffers.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@128679 91177308-0d34-0410-b5e6-96231b3b80d8

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

@@ -66,6 +66,7 @@ public:
   void CheckCall_gets(const CallExpr *CE, const FunctionDecl *FD);
   void CheckCall_getpw(const CallExpr *CE, const FunctionDecl *FD);
   void CheckCall_mktemp(const CallExpr *CE, const FunctionDecl *FD);
+  void CheckCall_strcpy(const CallExpr *CE, const FunctionDecl *FD);
   void CheckCall_rand(const CallExpr *CE, const FunctionDecl *FD);
   void CheckCall_random(const CallExpr *CE, const FunctionDecl *FD);
   void CheckUncheckedReturnValue(CallExpr *CE);
@@ -98,6 +99,7 @@ void WalkAST::VisitCallExpr(CallExpr *CE) {
     CheckCall_gets(CE, FD);
     CheckCall_getpw(CE, FD);
     CheckCall_mktemp(CE, FD);
+    CheckCall_strcpy(CE, FD);
     if (CheckRand) {
       CheckCall_rand(CE, FD);
       CheckCall_random(CE, FD);
@@ -348,6 +350,58 @@ void WalkAST::CheckCall_mktemp(const CallExpr *CE, const FunctionDecl *FD) {
                      CE->getLocStart(), &R, 1);
 }
 
+//===----------------------------------------------------------------------===//
+// Check: Any use of 'strcpy' is insecure.
+//
+// CWE-119: Improper Restriction of Operations within 
+// the Bounds of a Memory Buffer 
+//===----------------------------------------------------------------------===//
+void WalkAST::CheckCall_strcpy(const CallExpr *CE, const FunctionDecl *FD) {
+  IdentifierInfo *II = FD->getIdentifier();
+  if (!II)   // if no identifier, not a simple C function
+    return;
+  llvm::StringRef Name = II->getName();
+  if (Name.startswith("__builtin_"))
+    Name = Name.substr(10);
+
+  if ((Name != "strcpy") &&
+      (Name != "__strcpy_chk"))
+    return;
+
+  const FunctionProtoType *FPT
+    = dyn_cast<FunctionProtoType>(FD->getType().IgnoreParens());
+  if (!FPT)
+    return;
+
+  // Verify the function takes two arguments
+  int numArgs = FPT->getNumArgs();
+  if (numArgs != 2 && numArgs != 3)
+    return;
+
+  // Verify the type for both arguments
+  for (int i = 0; i < 2; i++) {
+    // Verify that the arguments are pointers
+    const PointerType *PT = dyn_cast<PointerType>(FPT->getArgType(i));
+    if (!PT)
+      return;
+
+    // Verify that the argument is a 'char*'.
+    if (PT->getPointeeType().getUnqualifiedType() != BR.getContext().CharTy)
+      return;
+  }
+
+  // Issue a warning
+  SourceRange R = CE->getCallee()->getSourceRange();
+  BR.EmitBasicReport("Potential insecure memory buffer bounds restriction in "
+		     "call 'strcpy'",
+		     "Security",
+		     "Call to function 'strcpy' is insecure as it does not "
+		     "provide bounding of the memory buffer. Replace "
+		     "unbounded copy functions with analogous functions that "
+		     "support length arguments such as 'strncpy'. CWE-119.",
+                     CE->getLocStart(), &R, 1);
+}
+
 //===----------------------------------------------------------------------===//
 // Check: Linear congruent random number generators should not be used
 // Originally: <rdar://problem/63371000>

test/Analysis/security-syntax-checks.m

@@ -1,4 +1,16 @@
 // RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze -analyzer-checker=security.experimental.SecuritySyntactic %s -verify
+// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze -DUSE_BUILTINS -analyzer-checker=security.experimental.SecuritySyntactic %s -verify
+// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze -DVARIANT -analyzer-checker=security.experimental.SecuritySyntactic %s -verify
+// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze -DUSE_BUILTINS -DVARIANT -analyzer-checker=security.experimental.SecuritySyntactic %s -verify
+
+#ifdef USE_BUILTINS
+# define BUILTIN(f) __builtin_ ## f
+#else /* USE_BUILTINS */
+# define BUILTIN(f) f
+#endif /* USE_BUILTINS */
+
+typedef typeof(sizeof(int)) size_t;
+
 
 // <rdar://problem/6336718> rule request: floating point used as loop 
 //  condition (FLP30-C, FLP-30-CPP)
@@ -103,3 +115,28 @@ char *mktemp(char *buf);
 void test_mktemp() {
   char *x = mktemp("/tmp/zxcv"); // expected-warning{{Call to function 'mktemp' is insecure as it always creates or uses insecure temporary file}}
 }
+
+
+//===----------------------------------------------------------------------===
+// strcpy()
+//===----------------------------------------------------------------------===
+#ifdef VARIANT
+
+#define __strcpy_chk BUILTIN(__strcpy_chk)
+char *__strcpy_chk(char *restrict s1, const char *restrict s2, size_t destlen);
+
+#define strcpy(a,b) __strcpy_chk(a,b,(size_t)-1)
+
+#else /* VARIANT */
+
+#define strcpy BUILTIN(strcpy)
+char *strcpy(char *restrict s1, const char *restrict s2);
+
+#endif /* VARIANT */
+
+void test_strcpy() {
+  char x[4];
+  char *y;
+
+  strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strncpy'. CWE-119.}}
+}