[analyzer] CERT: STR37-C

[zukatsinadze]

Summary

This patch introduces a new checker:
alpha.security.cert.str.37c

This checker is implemented based on the following rule:
https://wiki.sei.cmu.edu/confluence/x/BNcxBQ
The check warns if the argument of a character handling
function is not representable as unsigned char.

[Szelethus]

Adding @martong, because I fear that this is colliding with StdLibraryFunctionsChecker. The warnings added here seem to be, in essence, identical to D73898.

[zukatsinadze]

In D79358#2019755, @Szelethus wrote:

Adding @martong, because I fear that this is colliding with StdLibraryFunctionsChecker. The warnings added here seem to be, in essence, identical to D73898.

Indeed. I think the best way forward is to add missing functions (I think these are toascii, toupper, tolower) to @martong's checker to guarantee that CERT rule is covered completely.

[Szelethus]

I'm afraid so. The patch otherwise looked really clean, sorry to ruin the day! :)

clang/docs/analyzer/checkers.rst

reminderText =
      [NSString stringWithFormat:
        NSLocalizedString(@"%@ Reminders", @"Indicates multiple reminders"),
          reminderCount];
  }
 
 alpha.security
 ^^^^^^^^^^^^^^
 
-
-alpha.security.cert
-^^^^^^^^^^^^^^^^^^^
-
-SEI CERT checkers which tries to find errors based on their `C coding rules <https://wiki.sei.cmu.edu/confluence/display/c/2+Rules>`_.
-
-.. _alpha-security-cert-pos-checkers:
-
-alpha.security.cert.pos
-^^^^^^^^^^^^^^^^^^^^^^^
-
-SEI CERT checkers of `POSIX C coding rules <https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152405>`_.
-
 .. _alpha-security-cert-pos-34c:
 
 alpha.security.cert.pos.34c
 """""""""""""""""""""""""""
 Finds calls to the ``putenv`` function which pass a pointer to an automatic variable as the argument.
 
 .. code-block:: c
 
   int func(const char *var) {
     char env[1024];
     int retval = snprintf(env, sizeof(env),"TEST=%s", var);
     if (retval < 0 || (size_t)retval >= sizeof(env)) {
         /* Handle error */
     }
  
     return putenv(env); // putenv function should not be called with auto variables
   }
-  
+
+.. _alpha-security-cert-str-37c:
+
+alpha.security.cert.str.37c
+"""""""""""""""""""""""""""
+
+Finds calls of character-handling functions with arguments which are not representable as an unsigned char.
+Rule applies to the following functions: ``isalnum``, ``isalpha``, ``isascii``, ``isblank``, ``iscntrl``, ``isdigit``, ``isgraph``, 
+``islower``, ``isprint``, ``ispunct``, ``isspace``, ``isupper``, ``isxdigit``, ``toascii``, ``toupper``, ``tolower``.
+
+.. code-block:: c
+
+  size_t count_preceding_whitespace(const char *s) {
+    const char *t = s;
+    size_t length = strlen(s) + 1;
+    while (isspace(*t) && (t - s < length)) { 
+      // The argument to isspace() must be EOF or representable as an unsigned char; otherwise, the result is undefined.
+      ++t;
+    }
+    return t - s;
+}
+
 .. _alpha-security-ArrayBound:
 
 alpha.security.ArrayBound (C)
 """""""""""""""""""""""""""""
 Warn about buffer overflows (older checker).
 
 .. code-block:: c
 

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 
 def Security : Package <"security">;
 def InsecureAPI : Package<"insecureAPI">, ParentPackage<Security>;
 def SecurityAlpha : Package<"security">, ParentPackage<Alpha>;
 def Taint : Package<"taint">, ParentPackage<SecurityAlpha>;
 
 def CERT : Package<"cert">, ParentPackage<SecurityAlpha>;
 def POS : Package<"pos">, ParentPackage<CERT>;
+def STR : Package<"str">, ParentPackage<CERT>;
 
 def Unix : Package<"unix">;
 def UnixAlpha : Package<"unix">, ParentPackage<Alpha>;
 def CString : Package<"cstring">, ParentPackage<Unix>;
 def CStringAlpha : Package<"cstring">, ParentPackage<UnixAlpha>;
 
 def OSX : Package<"osx">;
 def OSXAlpha : Package<"osx">, ParentPackage<Alpha>;
let ParentPackage = POS in {
 
   def PutenvWithAuto : Checker<"34c">,
   HelpText<"Finds calls to the 'putenv' function which pass a pointer to "
            "an automatic variable as the argument.">,
   Documentation<HasDocumentation>;
 
 } // end "alpha.cert.pos"
 
+let ParentPackage = STR in {
+
+  def Str37 : Checker<"37c">,
+  HelpText<"Arguments to character-handling functions must be "
+           "representable as an unsigned char">,
+  Documentation<HasDocumentation>;
+
+} // end "alpha.cert.str"
+
 let ParentPackage = SecurityAlpha in {
 
 def ArrayBoundChecker : Checker<"ArrayBound">,
   HelpText<"Warn about buffer overflows (older checker)">,
   Documentation<HasAlphaDocumentation>;
 
 def ArrayBoundCheckerV2 : Checker<"ArrayBoundV2">,
   HelpText<"Warn about buffer overflows (newer checker)">,

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   PthreadLockChecker.cpp
   cert/PutenvWithAutoChecker.cpp
   RetainCountChecker/RetainCountChecker.cpp
   RetainCountChecker/RetainCountDiagnostics.cpp
   ReturnPointerRangeChecker.cpp
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
+  cert/Str37Checker.cpp
   SimpleStreamChecker.cpp
   SmartPtrModeling.cpp
   StackAddrEscapeChecker.cpp
   StdLibraryFunctionsChecker.cpp
   STLAlgorithmModeling.cpp
   StreamChecker.cpp
   Taint.cpp
   TaintTesterChecker.cpp

clang/lib/StaticAnalyzer/Checkers/cert/Str37Checker.cpp

+//== Str37Checker.cpp --------------------------------- -*- C++ -*--=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines Str37Checker which finds calls of character-handling
+// functions with arguments which are not representable as an unsigned char.
+// https://wiki.sei.cmu.edu/confluence/x/BNcxBQ
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class Str37Checker : public Checker<check::PreCall> {
+private:
+  BugType BT{this,
+             "arguments to character-handling functions must be representable "
+             "as an unsigned char",
+             categories::SecurityError};
+
+  using ArgCheck = std::function<void(const Str37Checker *, const CallEvent &,
+                                      CheckerContext &)>;
+  // The pairs are in the following form:
+  // {{function-name, argument-count}, {callback, index-of-argument}}
+  const CallDescriptionMap<std::pair<ArgCheck, unsigned>> CDM = {
+      {{"isalnum", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"isalpha", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"isascii", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"isblank", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"iscntrl", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"isdigit", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"isgraph", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"islower", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"isprint", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"ispunct", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"isspace", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"isupper", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"isxdigit", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"toascii", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"toupper", 1}, {&Str37Checker::checkPreCall, 0}},
+      {{"tolower", 1}, {&Str37Checker::checkPreCall, 0}}};
+
+public:
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+};
+} // namespace
+
+void Str37Checker::checkPreCall(const CallEvent &Call,
+                                CheckerContext &C) const {
+  const auto *Lookup = CDM.lookup(Call);
+  if (!Lookup)
+    return;
+
+  unsigned ArgIndex = (*Lookup).second;
+  SValBuilder &SVB = C.getSValBuilder();
+  ProgramStateRef State = C.getState();
+
+  const Expr *ArgExpr = Call.getArgExpr(ArgIndex);
+  QualType Ty = ArgExpr->IgnoreImpCasts()->getType();
+  if (Ty->isPointerType())
+    Ty = Ty->getPointeeType();
+  const auto *B = dyn_cast<BuiltinType>(Ty);
+  if (!B)
+    return;
+  if (B->getKind() == BuiltinType::UChar)
+    return;
+
+  SVal ArgV = Call.getArgSVal(ArgIndex);
+  if (const auto AcutalVal = SVB.getKnownValue(State, ArgV))
+    // -1 is included because of EOF
+    if (-1 <= *AcutalVal && *AcutalVal <= 255)
+      return;
+
+  StringRef CallName = Call.getCalleeIdentifier()->getName();
+  SmallString<128> Msg;
+  llvm::raw_svector_ostream Out(Msg);
+  Out << '\'' << "arguments to character-handling function `" << CallName
+      << "` must be representable as an unsigned char";
+  std::string ReportMsg = Out.str().str();
+
+  ExplodedNode *N = C.generateErrorNode();
+  auto Report = std::make_unique<PathSensitiveBugReport>(BT, ReportMsg, N);
+
+  C.emitReport(std::move(Report));
+}
+
+void ento::registerStr37(CheckerManager &Mgr) {
+  Mgr.registerChecker<Str37Checker>();
+}
+
+bool ento::shouldRegisterStr37(const CheckerManager &) { return true; }

clang/test/Analysis/cert/str37-c-fp-suppression.cpp

+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.str.37c\
+// RUN:  -verify %s
+
+#include "../Inputs/system-header-simulator.h"
+
+int isspace(int c);
+int toupper(int c);
+int isalnum(int c);
+int isalpha(int c);
+int isascii(int c);
+int isblank(int c);
+int iscntrl(int c);
+int isdigit(int c);
+int isgraph(int c);
+int islower(int c);
+int isprint(int c);
+int ispunct(int c);
+int isupper(int c);
+int isxdigit(int c);
+int toascii(int c);
+int tolower(int c);
+
+namespace correct_use {
+
+int test_tolower(const char *c) {
+  char lowA = tolower('A'); // no warning
+  return tolower(97);       // no warning
+}
+
+int test_toascii(const char *c) {
+  return toascii((unsigned char)*c); // no warning
+}
+
+void test_toupper(const char *s) {
+  char c = 98;
+  c = toupper(c); // no warning
+  char b = -2;
+  b = toupper((unsigned char)b); // no warning
+}
+
+int test_isalnum() {
+  int a = 50;
+  int testEOF = isalnum(EOF); // no warning
+  return isalnum(a);          // no warning
+}
+
+int test_isalpha() {
+  char c = 'a';
+  int b = isalpha(255); // no warning
+  return isalpha(c);    // no warning
+}
+
+int test_isascii() {
+  char c = 'a';
+  int b = isascii(200); // no warning
+  int A = isascii(65);  // no warning
+  return isascii(c);    // no warning
+}
+
+int test_isblank() {
+  char c = ' ';
+  bool isEOFBlank = isblank(EOF); // no warning
+  return isblank(c);              // no warning
+}
+
+int test_iscntrl() {
+  char c = 2;
+  return iscntrl(c); // no warning
+}
+
+int test_isdigit() {
+  char c = '2';
+  return isdigit(c); // no warning
+}
+
+int test_isgraph() {
+  char c = '9';
+  return isgraph(c); // no warning
+}
+
+int test_islower() {
+  char c = 'a';
+  return islower(c); // no warning
+}
+
+int test_isprint(char c) {
+  return isprint((unsigned char)c); // no warning
+}
+
+int test_ispunct() {
+  char c = 'a';
+  char b = -2;
+  bool test_unsigned = ispunct((unsigned char)b); // no warning
+  bool not_true = ispunct(c);                     // no warning
+  return ispunct(2);                              // no warning
+}
+
+int test_isupper(const char *b) {
+  char c = 'A';
+  bool A_is_upper = isupper(c);      // no warning
+  return isupper((unsigned char)*b); // no warning
+}
+
+int test_isxdigit() {
+  char hexa = '9';
+  char not_hexa = 'M';
+  return isxdigit(hexa) || isxdigit(not_hexa); // no warning
+}
+} // namespace correct_use
+
+namespace incorrect_use {
+
+int test_tolower() {
+  int a = 5;
+  int b = 7;
+  char c = a - b;
+  return tolower(c);
+  // expected-warning@-1 {{arguments to character-handling function `tolower` must be representable as an unsigned char}}
+}
+
+int test_toascii(const char *c) {
+  return toascii(*c);
+  // expected-warning@-1 {{arguments to character-handling function `toascii` must be representable as an unsigned char}}
+}
+
+void test_toupper(const char *s) {
+  char c = -2;
+  c = toupper(c);
+  // expected-warning@-1 {{arguments to character-handling function `toupper` must be representable as an unsigned char}}
+}
+
+int test_isalnum() {
+  char c = -2;
+  return isalnum(c);
+  // expected-warning@-1 {{arguments to character-handling function `isalnum` must be representable as an unsigned char}}
+}
+
+int test_isalpha() {
+  return isalpha(256);
+  // expected-warning@-1 {{arguments to character-handling function `isalpha` must be representable as an unsigned char}}
+}
+
+int test_isascii() {
+  int b = isascii(269);
+  // expected-warning@-1 {{arguments to character-handling function `isascii` must be representable as an unsigned char}}
+  return isascii(100); // no warning
+}
+
+int test_isblank() {
+  char c = -2;
+  return isblank(c);
+  // expected-warning@-1 {{arguments to character-handling function `isblank` must be representable as an unsigned char}}
+}
+
+int test_iscntrl() {
+  char c = -22;
+  return iscntrl(c);
+  // expected-warning@-1 {{arguments to character-handling function `iscntrl` must be representable as an unsigned char}}
+}
+
+int test_isdigit() {
+  char c = -2;
+  return isdigit(c);
+  // expected-warning@-1 {{arguments to character-handling function `isdigit` must be representable as an unsigned char}}
+}
+
+int test_isgraph() {
+  return isgraph(269);
+  // expected-warning@-1 {{arguments to character-handling function `isgraph` must be representable as an unsigned char}}
+}
+
+int test_islower() {
+  return islower(269);
+  // expected-warning@-1 {{arguments to character-handling function `islower` must be representable as an unsigned char}}
+}
+
+int test_isprint(char c) {
+  return isprint(c);
+  // expected-warning@-1 {{arguments to character-handling function `isprint` must be representable as an unsigned char}}
+}
+
+int test_ispunct() {
+  char c;
+  return ispunct(c);
+  // expected-warning@-1 {{arguments to character-handling function `ispunct` must be representable as an unsigned char}}
+}
+
+int test_isupper(const char *b) {
+  return isupper(*b);
+  // expected-warning@-1 {{arguments to character-handling function `isupper` must be representable as an unsigned char}}
+}
+
+int test_isxdigit() {
+  char hexa = -10;
+  return isxdigit(hexa);
+  // expected-warning@-1 {{arguments to character-handling function `isxdigit` must be representable as an unsigned char}}
+}
+
+} // namespace incorrect_use

clang/test/Analysis/cert/str37-c.cpp

+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.str.37c\
+// RUN:  -verify %s
+
+// Examples from the CERT rule's page.
+// https://wiki.sei.cmu.edu/confluence/x/BNcxBQ
+
+#include "../Inputs/system-header-simulator.h"
+
+int isspace(int c);
+
+size_t bad_usage(const char *s) {
+  const char *t = s;
+  size_t length = strlen(s) + 1;
+  while (isspace(*t) && (t - s < length)) {
+    // expected-warning@-1 {{arguments to character-handling function `isspace` must be representable as an unsigned char}}
+    ++t;
+  }
+  return t - s;
+}
+
+size_t good_usage(const char *s) {
+  const char *t = s;
+  size_t length = strlen(s) + 1;
+  while (isspace((unsigned char)*t) && (t - s < length)) { // no warning
+    ++t;
+  }
+  return t - s;
+}