[analyzer] CERT STR rule checkers: STR50-CPP

[Charusso]

Summary

This patch introduces a new experimental checker:
alpha.security.cert.str.50cpp

This checker is implemented based on the following rule:
https://wiki.sei.cmu.edu/confluence/x/i3w-BQ

It warns on misusing std::cin and std::basic_istream<T>::read() to
create a std::string which may not null-terminated.

[baloghadamsoftware]

Why limiting the checker to std::cin?

Anyway, this checker is far from ready yet (the essence is missing), see my comment inside.

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

658

Hmm, let us imagine that I write the following code:

TypeT cin;
AnyClassWithInstanceGTGTOperatorTakingTypeT >> cin;

Your code will probably crash when trying to retrieve arg 1 because you neither check for std::cin nor for InstanceCall.

669

Do we need such heavyweight thing here? In my experience, AST matchers are expensive to create.

673

Here should be the check for the size of the buffer and the comparison to cin.width(). It seems that it is not ready yet. This should be the most essential part of this checker.

676

std::cin does not write. It is just a stream from where we read. operator>> writes. Please rephrase the error message.

clang/test/Analysis/cert/str50-cpp.cpp

24

We should get warning for bufTwo and no warning for bufOne. operator>> reads cin.width()-1 characters to the buffer plus a null-terminator, which is exactly 12 bytes. So no overflow happens. However, it also resets the width, so the next read is unlimited. Please check STR50-CPP. It describes exactly the same as above.

[whisperity]

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

671

IIRC the matches are of SmallVector which should have an empty() method, prefer it to size() <op> 0.

clang/docs/analyzer/checkers.rst

 
 Checker for `STR32-C rule<https://wiki.sei.cmu.edu/confluence/x/r9UxBQ>`_.
 
 It warns on reading non-null-terminated strings. This warning is restricted to
 the allocations which the Static Analyzer models with :ref:`unix.Malloc<unix-Malloc>`_.
 
 Also warns on misusing the ``strncpy()`` function.
 
+.. _alpha-security-cert-str-50cpp:
+
+alpha.security.cert.str.50cpp
+"""""""""""""""""""""""""""""
+
+Checker for `STR50-CPP rule<https://wiki.sei.cmu.edu/confluence/x/i3w-BQ>`_.
+
+It warns on misusing ``std::cin`` and ``std::basic_istream<T>::read()`` to
+create a ``std::string`` which may not null-terminated.
+
 .. _alpha-security-ArrayBound:
 
 alpha.security.ArrayBound (C)
 """""""""""""""""""""""""""""
 Warn about buffer overflows (older checker).
 
 .. code-block:: c
 

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

def Str31cChecker : Checker<"31c">,
   Dependencies<[StrCheckerBase]>,
   Documentation<HasDocumentation>;
 
 def Str32cChecker : Checker<"32c">,
   HelpText<"SEI CERT checker of rules defined in STR32-C">,
   Dependencies<[StrCheckerBase]>,
   Documentation<HasDocumentation>;
 
+def Str50cppChecker : Checker<"50cpp">,
+  HelpText<"SEI CERT checker of rules defined in STR50-CPP">,
+  Dependencies<[StrCheckerBase]>,
+  Documentation<HasDocumentation>;
+
 } // end "alpha.cert.str"
 
 let ParentPackage = SecurityAlpha in {
 
 def ArrayBoundChecker : Checker<"ArrayBound">,
   HelpText<"Warn about buffer overflows (older checker)">,
   Documentation<HasAlphaDocumentation>;
 

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

 //  - STR31-C: Guarantee that storage for strings has sufficient space for
 //    character data and the null terminator:
 //     - https://wiki.sei.cmu.edu/confluence/x/sNUxBQ
 //
 //  - STR32-C: Do not pass a non-null-terminated character sequence to a
 //    library function that expects a string:
 //     - https://wiki.sei.cmu.edu/confluence/x/r9UxBQ
 //
+//  - STR50-CPP: Guarantee that storage for strings has sufficient space for
+//    character data and the null terminator:
+//     - https://wiki.sei.cmu.edu/confluence/x/i3w-BQ
+//
 //===----------------------------------------------------------------------===//
 
 #include "AllocationState.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 #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"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
 #include <utility>
 
 using namespace clang;
 using namespace ento;
+using namespace ast_matchers;
 
 namespace {
 
 /// Helper struct for defining the function call signatures of 'CDM'.
 /// The fields are defining what is the index of the interesting arguments.
 struct CallContext {
   CallContext(Optional<unsigned> DestinationPos,
               Optional<unsigned> SourcePos = None,
void checkSprintf(const CallEvent &Call, const CallContext &CallC,
                     CheckerContext &C) const;
   void checkFscanf(const CallEvent &Call, const CallContext &CallC,
                    CheckerContext &C) const;
   void checkStrcpy(const CallEvent &Call, const CallContext &CallC,
                    CheckerContext &C) const;
   /// STR32-C.
   void checkStrncpy(const CallEvent &Call, const CallContext &CallC,
                     CheckerContext &C) const;
+  /// STR50-CPP.
+  void checkInStream(const CallEvent &Call, const CallContext &CallC,
+                     CheckerContext &C) const;
+  void checkRead(const CallEvent &Call, const CallContext &CallC,
+                 CheckerContext &C) const;
+  void checkStringConstructor(const CallEvent &Call, const CallContext &CallC,
+                              CheckerContext &C) const;
   /// \}
 
   /// Create a report on a function call which could cause a buffer overflow.
   ///
   /// In case of STR31-C emit an error report.
   /// In case of STR32-C emit a note and mark the buffer as possibly not
   /// null-terminated.
   void createCallOverflowReport(const CallEvent &Call, const CallContext &CallC,
                                 CheckerContext &C) const;
 
   bool EnableStr30cChecker = false;
   bool EnableStr31cChecker = false;
   bool EnableStr32cChecker = false;
+  bool EnableStr50cppChecker = false;
 
 private:
   using StrCheck = std::function<void(const StrCheckerBase *, const CallEvent &,
                                       const CallContext &, CheckerContext &)>;
 
   const CallDescriptionMap<std::pair<StrCheck, CallContext>> CDM = {
       // The following checks STR30-C rules.
       // void *memchr(const void *buffer, int c, size_t count);
const CallDescriptionMap<std::pair<StrCheck, CallContext>> CDM = {
       {{"sprintf", 3, 2}, {&StrCheckerBase::checkSprintf, {0}}},
       // int fscanf(FILE *stream, const char *format, ... [char *dest]);
       {{"fscanf", 3, 2}, {&StrCheckerBase::checkFscanf, {2}}},
       // char *strcpy(char *dest, const char *src);
       {{"strcpy", 2}, {&StrCheckerBase::checkStrcpy, {0, 1}}},
 
       // The following checks STR32-C rules.
       // char *strncpy(char *dest, const char *src, size_t count)
-      {{"strncpy", 3}, {&StrCheckerBase::checkStrncpy, {0, 1, 2}}}};
+      {{"strncpy", 3}, {&StrCheckerBase::checkStrncpy, {0, 1, 2}}},
+
+      // The following checks STR50-CPP rules.
+      {{"operator>>"}, {&StrCheckerBase::checkInStream, {None, 1}}},
+      // istream& read (char *src, streamsize n);
+      {{{"std", "basic_istream", "read"}, 2},
+       {&StrCheckerBase::checkRead, {None, 0, 1}}},
+      {{{"std", "basic_string"}, 1},
+       {&StrCheckerBase::checkStringConstructor, {None, 0}}}};
 
   BugType BT{this, "Insecure string handler function call",
              categories::SecurityError};
 };
 } // namespace
 
 /// It stores whether the string is null-terminated.
 REGISTER_MAP_WITH_PROGRAMSTATE(NullTerminationMap, const MemRegion *, bool)

 //===----------------------------------------------------------------------===//
 
 /// Returns the interesting region of \p V.
 static const MemRegion *getRegion(SVal V) {
   const MemRegion *MR = V.getAsRegion();
   if (!MR)
     return nullptr;
 
+  if (const auto *ER = dyn_cast<ElementRegion>(MR))
+    return ER->getSuperRegion();
+
   return MR->getBaseRegion();
 }
 
 /// Print pretty the expression \p E.
 std::string printPretty(const Expr *E, CheckerContext &C) {
   assert(E);
   SmallString<128> Str;
   llvm::raw_svector_ostream Out(Str);
if (VD->getType()->getAsArrayTypeUnsafe()) {
 
       std::string ArrayName = VD->getNameAsString();
       createAllocationOverflowReport(VR, ArrayName, VD->getInit(), C);
     }
   }
 }
 
 //===----------------------------------------------------------------------===//
+// The following checks STR50-CPP rules.
+//===----------------------------------------------------------------------===//
+
+void StrCheckerBase::checkInStream(const CallEvent &Call,
+                                   const CallContext &CallC,
+                                   CheckerContext &C) const {
+  if (!EnableStr50cppChecker)
+    return;
+
+  // Check whether the first argument is 'std::cin'.
+  const auto *OCE = dyn_cast<CXXOperatorCallExpr>(Call.getOriginExpr());
+  if (!OCE)
+    return;
+
+  const auto *DR =
+      dyn_cast_or_null<DeclRegion>(getRegion(C.getSVal(OCE->getArg(0))));
+  if (!DR)
+    return;
+
+  if (DR->getDecl()->getName() != "cin")
+    return;
+
+  const Expr *DestArg = OCE->getArg(1);
+  DR = dyn_cast_or_null<DeclRegion>(getRegion(C.getSVal(DestArg)));
+  if (!DR)
+    return;
+
+  // Writing into a string is safe.
+  const auto MatchStringTy =
+      match(qualType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(
+                cxxRecordDecl(hasName("::std::basic_string")))))),
+            *DR->getValueType(), C.getASTContext());
+
+  if (MatchStringTy.size() != 0)
+    return;
+
+  SmallString<256> Msg;
+  llvm::raw_svector_ostream Out(Msg);
+  Out << "std::cin could write outside of '" << DR->getDecl()->getName()
+      << '\'';
+
+  auto Report = std::make_unique<PathSensitiveBugReport>(BT, Out.str(),
+                                                         C.generateErrorNode());
+  Report->addRange(DestArg->getSourceRange());
+  Report->markInteresting(DR);
+
+  // Track the allocation.
+  bugreporter::trackExpressionValue(Report->getErrorNode(), DestArg, *Report);
+  if (const SymbolRef Sym = C.getSVal(DestArg).getAsSymbol())
+    Report->addVisitor(allocation_state::getMallocBRVisitor(Sym));
+
+  C.emitReport(std::move(Report));
+}
+
+void StrCheckerBase::checkRead(const CallEvent &Call, const CallContext &CallC,
+                               CheckerContext &C) const {
+  if (!EnableStr50cppChecker)
+    return;
+
+  const auto *DR = dyn_cast_or_null<DeclRegion>(getRegion(Call.getArgSVal(0)));
+  if (!DR)
+    return;
+
+  ProgramStateRef State = C.getState();
+  const NoteTag *Tag = C.getNoteTag(
+      [=](PathSensitiveBugReport &BR) -> std::string {
+        if (!BR.isInteresting(DR))
+          return "";
+
+        SmallString<128> Msg;
+        llvm::raw_svector_ostream Out(Msg);
+
+        Out << "Reading a not null-terminated input to '"
+            << DR->getDecl()->getName() << '\'';
+
+        return Out.str().str();
+      },
+      /*IsPrunable=*/false);
+
+  // Mark the allocation have arbitrary input data, may not null terminated.
+  State = State->set<NullTerminationMap>(DR, false);
+  C.addTransition(State, Tag);
+}
+
+void StrCheckerBase::checkStringConstructor(const CallEvent &Call,
+                                            const CallContext &CallC,
+                                            CheckerContext &C) const {
+  if (!EnableStr50cppChecker)
+    return;
+
+  const auto *DR = dyn_cast_or_null<DeclRegion>(getRegion(Call.getArgSVal(0)));
+  if (!DR)
+    return;
+
+  const bool *IsNullTerminated = C.getState()->get<NullTerminationMap>(DR);
+  if (!IsNullTerminated || *IsNullTerminated)
+    return;
+
+  SmallString<256> Msg;
+  llvm::raw_svector_ostream Out(Msg);
+  Out << '\'' << DR->getDecl()->getName() << "' is not null-terminated";
+  std::string ReportMsg = Out.str().str();
+
+  auto Report = std::make_unique<PathSensitiveBugReport>(BT, Out.str(),
+                                                         C.generateErrorNode());
+  const Expr *SrcArg = Call.getArgExpr(0);
+  Report->addRange(SrcArg->getSourceRange());
+  Report->markInteresting(DR);
+
+  // Track the allocation.
+  bugreporter::trackExpressionValue(Report->getErrorNode(), SrcArg, *Report);
+  if (const SymbolRef Sym = C.getSVal(SrcArg).getAsSymbol())
+    Report->addVisitor(allocation_state::getMallocBRVisitor(Sym));
+
+  C.emitReport(std::move(Report));
+}
+
+//===----------------------------------------------------------------------===//
 // Main logic to check a function call.
 //===----------------------------------------------------------------------===//
 
 void StrCheckerBase::checkPostCall(const CallEvent &Call,
                                    CheckerContext &C) const {
   if (const auto *Lookup = CDM.lookup(Call)) {
     const StrCheck &Check = Lookup->first;
     Check(this, Call, Lookup->second, C);
void ento::register##Name(CheckerManager &Mgr) {                             \
     Checker->Enable##Name = true;                                              \
   }                                                                            \
                                                                                \
   bool ento::shouldRegister##Name(const CheckerManager &) { return true; }
 
 REGISTER_CHECKER(Str30cChecker)
 REGISTER_CHECKER(Str31cChecker)
 REGISTER_CHECKER(Str32cChecker)
+REGISTER_CHECKER(Str50cppChecker)

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

public:
     const T& front() const { return *begin(); }
   };
 
   template <typename CharT>
   class basic_string {
   public:
     basic_string();
     basic_string(const CharT *s);
+    basic_string(const CharT *s, size_t n);
 
     ~basic_string();
     void clear();
 
     basic_string &operator=(const basic_string &str);
     basic_string &operator+=(const basic_string &str);
 
     const CharT *c_str() const;
namespace std {
 
   typedef basic_string<char> string;
   typedef basic_string<wchar_t> wstring;
 #if __cplusplus >= 201103L
   typedef basic_string<char16_t> u16string;
   typedef basic_string<char32_t> u32string;
 #endif
 
+  struct ios_base {
+    class failure {};
+  };
+
+  template <typename CharT>
+  class basic_istream {
+    typedef __SIZE_TYPE__ streamsize;
+
+  public:
+    streamsize gcount() const;
+    basic_istream &read(CharT *s, streamsize n);
+    streamsize width(streamsize);
+  };
+
+  template <class CharT>
+  basic_istream<CharT> &operator>>(basic_istream<CharT> &is, CharT *c);
+  template <class Istream, class T>
+  Istream &&operator>>(Istream &&st, T &&value);
+
+  typedef basic_istream<char> istream;
+  extern istream cin;
+
   class exception {
   public:
     exception() throw();
     virtual ~exception() throw();
     virtual const char *what() const throw() {
       return 0;
     }
   };

clang/test/Analysis/cert/str50-cpp-false-positive-suppression.cpp

+// RUN: %clang_analyze_cc1 -fcxx-exceptions \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.50cpp \
+// RUN:  -verify %s
+
+// See the examples on the page of STR50-CPP:
+// https://wiki.sei.cmu.edu/confluence/x/i3w-BQ
+
+#include "../Inputs/system-header-simulator-cxx.h"
+
+void test_member(std::istream &in) {
+  struct S {
+    char buffer[32];
+    void read(std::istream &in) { in.read(buffer, sizeof(buffer)); }
+  };
+
+  S s;
+  s.read(in);
+  std::string str(s.buffer);
+  // expected-warning@-1 {{'buffer' is not null-terminated}}
+}

clang/test/Analysis/cert/str50-cpp-notes.cpp

+// RUN: %clang_analyze_cc1 -fcxx-exceptions \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.50cpp \
+// RUN:  -analyzer-output=text -verify %s
+
+// See the examples on the page of STR50-CPP:
+// https://wiki.sei.cmu.edu/confluence/x/i3w-BQ
+
+#include "../Inputs/system-header-simulator-cxx.h"
+
+namespace test_cin_bad {
+void f() {
+  char buf[12]; // expected-note {{'buf' initialized here}}
+  std::cin >> buf;
+  // expected-note@-1 {{std::cin could write outside of 'buf'}}
+  // expected-warning@-2 {{std::cin could write outside of 'buf'}}
+}
+} // namespace test_cin_bad
+
+namespace test_read_bad {
+void f(std::istream &in) {
+  char buffer[32]; // expected-note {{'buffer' initialized here}}
+  try {
+    in.read(buffer, sizeof(buffer));
+    // expected-note@-1 {{Reading a not null-terminated input to 'buffer'}}
+  } catch (std::ios_base::failure &e) {
+    // Handle error
+  }
+
+  std::string str(buffer);
+  // expected-note@-1 {{'buffer' is not null-terminated}}
+  // expected-warning@-2 {{'buffer' is not null-terminated}}
+}
+} // namespace test_read_bad

clang/test/Analysis/cert/str50-cpp.cpp

+// RUN: %clang_analyze_cc1 -fcxx-exceptions \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.50cpp \
+// RUN:  -verify %s
+
+// See the examples on the page of STR50-CPP:
+// https://wiki.sei.cmu.edu/confluence/x/i3w-BQ
+
+#include "../Inputs/system-header-simulator-cxx.h"
+
+namespace test_cin_bad {
+void f() {
+  char buf[12];
+  std::cin >> buf;
+  // expected-warning@-1 {{std::cin could write outside of 'buf'}}
+}
+
+void g() {
+  char bufOne[12];
+  char bufTwo[12];
+  std::cin.width(12);
+  std::cin >> bufOne;
+  // expected-warning@-1 {{std::cin could write outside of 'bufOne'}}
+  std::cin >> bufTwo;
+}
+} // namespace test_cin_bad
+
+namespace test_cin_good {
+void f() {
+  std::string input;
+  std::string stringOne, stringTwo;
+  std::cin >> stringOne >> stringTwo;
+}
+} // namespace test_cin_good
+
+namespace test_read_bad {
+void f(std::istream &in) {
+  char buffer[32];
+  try {
+    in.read(buffer, sizeof(buffer));
+  } catch (std::ios_base::failure &e) {
+    // Handle error
+  }
+
+  std::string str(buffer);
+  // expected-warning@-1 {{'buffer' is not null-terminated}}
+}
+} // namespace test_read_bad
+
+namespace test_read_good {
+void f(std::istream &in) {
+  char buffer[32];
+  try {
+    in.read(buffer, sizeof(buffer));
+  } catch (std::ios_base::failure &e) {
+    // Handle error
+  }
+  std::string str(buffer, in.gcount());
+}
+} // namespace test_read_good

clang/test/Analysis/diagnostics/explicit-suppression.cpp

class C {
   // The virtual function is to make C not trivially copy assignable so that we call the
   // variant of std::copy() that does not defer to memmove().
   virtual int f();
 };
 
 void testCopyNull(C *I, C *E) {
   std::copy(I, E, (C *)0);
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:699 {{Called C++ object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:722 {{Called C++ object pointer is null}}
 #endif
 }