[analyzer] Add security checks for bcmp(), bcopy(), bzero().

These functions are obsolete. The analyzer would advice to replace them with
memcmp(), memcpy() or memmove(), or memset().

Patch by Tom Rix!

Differential Revision: https://reviews.llvm.org/D41881


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

include/clang/StaticAnalyzer/Checkers/Checkers.td

@@ -373,6 +373,15 @@ def PaddingChecker : Checker<"Padding">,
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = InsecureAPI in {
+  def bcmp : Checker<"bcmp">,
+    HelpText<"Warn on uses of the 'bcmp' function">,
+    DescFile<"CheckSecuritySyntaxOnly.cpp">;
+  def bcopy : Checker<"bcopy">,
+    HelpText<"Warn on uses of the 'bcopy' function">,
+    DescFile<"CheckSecuritySyntaxOnly.cpp">;
+  def bzero : Checker<"bzero">,
+    HelpText<"Warn on uses of the 'bzero' function">,
+    DescFile<"CheckSecuritySyntaxOnly.cpp">;
   def gets : Checker<"gets">,
     HelpText<"Warn on uses of the 'gets' function">,
     DescFile<"CheckSecuritySyntaxOnly.cpp">;

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

@@ -37,6 +37,9 @@ static bool isArc4RandomAvailable(const ASTContext &Ctx) {
 
 namespace {
 struct ChecksFilter {
+  DefaultBool check_bcmp;
+  DefaultBool check_bcopy;
+  DefaultBool check_bzero;
   DefaultBool check_gets;
   DefaultBool check_getpw;
   DefaultBool check_mktemp;
@@ -47,6 +50,9 @@ struct ChecksFilter {
   DefaultBool check_FloatLoopCounter;
   DefaultBool check_UncheckedReturn;
 
+  CheckName checkName_bcmp;
+  CheckName checkName_bcopy;
+  CheckName checkName_bzero;
   CheckName checkName_gets;
   CheckName checkName_getpw;
   CheckName checkName_mktemp;
@@ -89,6 +95,9 @@ public:
 
   // Checker-specific methods.
   void checkLoopConditionForFloat(const ForStmt *FS);
+  void checkCall_bcmp(const CallExpr *CE, const FunctionDecl *FD);
+  void checkCall_bcopy(const CallExpr *CE, const FunctionDecl *FD);
+  void checkCall_bzero(const CallExpr *CE, const FunctionDecl *FD);
   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);
@@ -129,6 +138,9 @@ void WalkAST::VisitCallExpr(CallExpr *CE) {
 
   // Set the evaluation function by switching on the callee name.
   FnCheck evalFunction = llvm::StringSwitch<FnCheck>(Name)
+    .Case("bcmp", &WalkAST::checkCall_bcmp)
+    .Case("bcopy", &WalkAST::checkCall_bcopy)
+    .Case("bzero", &WalkAST::checkCall_bzero)
     .Case("gets", &WalkAST::checkCall_gets)
     .Case("getpw", &WalkAST::checkCall_getpw)
     .Case("mktemp", &WalkAST::checkCall_mktemp)
@@ -295,6 +307,132 @@ void WalkAST::checkLoopConditionForFloat(const ForStmt *FS) {
                      FSLoc, ranges);
 }
 
+//===----------------------------------------------------------------------===//
+// Check: Any use of bcmp.
+// CWE-477: Use of Obsolete Functions
+// bcmp was deprecated in POSIX.1-2008
+//===----------------------------------------------------------------------===//
+
+void WalkAST::checkCall_bcmp(const CallExpr *CE, const FunctionDecl *FD) {
+  if (!filter.check_bcmp)
+    return;
+
+  const FunctionProtoType *FPT = FD->getType()->getAs<FunctionProtoType>();
+  if (!FPT)
+    return;
+
+  // Verify that the function takes three arguments.
+  if (FPT->getNumParams() != 3)
+    return;
+
+  for (int i = 0; i < 2; i++) {
+    // Verify the first and second argument type is void*.
+    const PointerType *PT = FPT->getParamType(i)->getAs<PointerType>();
+    if (!PT)
+      return;
+
+    if (PT->getPointeeType().getUnqualifiedType() != BR.getContext().VoidTy)
+      return;
+  }
+
+  // Verify the third argument type is integer.
+  if (!FPT->getParamType(2)->isIntegralOrUnscopedEnumerationType())
+    return;
+
+  // Issue a warning.
+  PathDiagnosticLocation CELoc =
+    PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
+  BR.EmitBasicReport(AC->getDecl(), filter.checkName_bcmp,
+                     "Use of deprecated function in call to 'bcmp()'",
+                     "Security",
+                     "The bcmp() function is obsoleted by memcmp().",
+                     CELoc, CE->getCallee()->getSourceRange());
+}
+
+//===----------------------------------------------------------------------===//
+// Check: Any use of bcopy.
+// CWE-477: Use of Obsolete Functions
+// bcopy was deprecated in POSIX.1-2008
+//===----------------------------------------------------------------------===//
+
+void WalkAST::checkCall_bcopy(const CallExpr *CE, const FunctionDecl *FD) {
+  if (!filter.check_bcopy)
+    return;
+
+  const FunctionProtoType *FPT = FD->getType()->getAs<FunctionProtoType>();
+  if (!FPT)
+    return;
+
+  // Verify that the function takes three arguments.
+  if (FPT->getNumParams() != 3)
+    return;
+
+  for (int i = 0; i < 2; i++) {
+    // Verify the first and second argument type is void*.
+    const PointerType *PT = FPT->getParamType(i)->getAs<PointerType>();
+    if (!PT)
+      return;
+
+    if (PT->getPointeeType().getUnqualifiedType() != BR.getContext().VoidTy)
+      return;
+  }
+
+  // Verify the third argument type is integer.
+  if (!FPT->getParamType(2)->isIntegralOrUnscopedEnumerationType())
+    return;
+
+  // Issue a warning.
+  PathDiagnosticLocation CELoc =
+    PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
+  BR.EmitBasicReport(AC->getDecl(), filter.checkName_bcopy,
+                     "Use of deprecated function in call to 'bcopy()'",
+                     "Security",
+                     "The bcopy() function is obsoleted by memcpy() "
+		     "or memmove().",
+                     CELoc, CE->getCallee()->getSourceRange());
+}
+
+//===----------------------------------------------------------------------===//
+// Check: Any use of bzero.
+// CWE-477: Use of Obsolete Functions
+// bzero was deprecated in POSIX.1-2008
+//===----------------------------------------------------------------------===//
+
+void WalkAST::checkCall_bzero(const CallExpr *CE, const FunctionDecl *FD) {
+  if (!filter.check_bzero)
+    return;
+
+  const FunctionProtoType *FPT = FD->getType()->getAs<FunctionProtoType>();
+  if (!FPT)
+    return;
+
+  // Verify that the function takes two arguments.
+  if (FPT->getNumParams() != 2)
+    return;
+
+  // Verify the first argument type is void*.
+  const PointerType *PT = FPT->getParamType(0)->getAs<PointerType>();
+  if (!PT)
+    return;
+
+  if (PT->getPointeeType().getUnqualifiedType() != BR.getContext().VoidTy)
+    return;
+
+  // Verify the second argument type is integer.
+  if (!FPT->getParamType(1)->isIntegralOrUnscopedEnumerationType())
+    return;
+
+  // Issue a warning.
+  PathDiagnosticLocation CELoc =
+    PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
+  BR.EmitBasicReport(AC->getDecl(), filter.checkName_bzero,
+                     "Use of deprecated function in call to 'bzero()'",
+                     "Security",
+                     "The bzero() function is obsoleted by memset().",
+                     CELoc, CE->getCallee()->getSourceRange());
+}
+
+
 //===----------------------------------------------------------------------===//
 // Check: Any use of 'gets' is insecure.
 // Originally: <rdar://problem/6335715>
@@ -775,6 +913,9 @@ public:
     checker->filter.checkName_##name = mgr.getCurrentCheckName();              \
   }
 
+REGISTER_CHECKER(bcmp)
+REGISTER_CHECKER(bcopy)
+REGISTER_CHECKER(bzero)
 REGISTER_CHECKER(gets)
 REGISTER_CHECKER(getpw)
 REGISTER_CHECKER(mkstemp)

test/Analysis/security-syntax-checks.m

@@ -37,6 +37,27 @@ void test_float_condition() {
   for (FooType x = 100000001.0f; x <= 100000010.0f; x++ ) {} // expected-warning{{Variable 'x' with floating point type 'FooType'}}
 }
 
+// Obsolete function bcmp
+int bcmp(void *, void *, size_t);
+
+int test_bcmp(void *a, void *b, size_t n) {
+  return bcmp(a, b, n); // expected-warning{{The bcmp() function is obsoleted by memcmp()}}
+}
+
+// Obsolete function bcopy
+void bcopy(void *, void *, size_t);
+
+void test_bcopy(void *a, void *b, size_t n) {
+  bcopy(a, b, n); // expected-warning{{The bcopy() function is obsoleted by memcpy() or memmove(}}
+}
+
+// Obsolete function bzero
+void bzero(void *, size_t);
+
+void test_bzero(void *a, size_t n) {
+  bzero(a, n); // expected-warning{{The bzero() function is obsoleted by memset()}}
+}
+
 // <rdar://problem/6335715> rule request: gets() buffer overflow
 // Part of recommendation: 300-BSI (buildsecurityin.us-cert.gov)
 char* gets(char *buf);

www/analyzer/available_checks.html

@@ -1172,6 +1172,40 @@ void test() {
 </pre></div></div></td></tr>
 
 
+<tr><td><div class="namedescr expandable"><span class="name">
+security.insecureAPI.bcmp</span><span class="lang">
+(C)</span><div class="descr">
+Warn on uses of the <code>bcmp</code> function.</div></div></td>
+<td><div class="exampleContainer expandable">
+<div class="example"><pre>
+void test() {
+  bcmp(ptr0, ptr1, n); // warn
+}
+</pre></div></div></td></tr>
+
+<tr><td><div class="namedescr expandable"><span class="name">
+security.insecureAPI.bcopy</span><span class="lang">
+(C)</span><div class="descr">
+Warn on uses of the <code>bcopy</code> function.</div></div></td>
+<td><div class="exampleContainer expandable">
+<div class="example"><pre>
+void test() {
+  bcopy(src, dst, n); // warn
+}
+</pre></div></div></td></tr>
+
+<tr><td><div class="namedescr expandable"><span class="name">
+security.insecureAPI.bzero</span><span class="lang">
+(C)</span><div class="descr">
+Warn on uses of the <code>bzero</code> function.</div></div></td>
+<td><div class="exampleContainer expandable">
+<div class="example"><pre>
+void test() {
+  bzero(ptr, n); // warn
+}
+</pre></div></div></td></tr>
+
+
 <tr><td><div class="namedescr expandable"><span class="name">
 security.insecureAPI.getpw</span><span class="lang">
 (C)</span><div class="descr">