[analyzer] CERT STR rule checkers: STR51-CPP

[Charusso]

Summary

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

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

It warns on possible std::string construction from a nullptr.

[Charusso]

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

749

We do not really try to model the construction and whether the members are nullable (dataflow, fixed-point, ugh!) so that this assumption is needed to relax this checker. It is still somewhat noisy.

[baloghadamsoftware]

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

67

I support the idea that MemRegionManager should be directly accessible from CheckerContext but I would create a separate micro-patch for that. With a unit test, of course.

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

97

I do not see this function used (even defined) anywhere. Not even EvalCDM which the comment refers to.

749

OK, let us make something clear at the beginning: do we look for constructor arguments that may be null or are null? I would think the latter one is more useful, and then we should not disclose arguments.

clang/docs/analyzer/checkers.rst

 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-cert-str-51cpp:
+
+alpha.security.cert.str.51cpp
+"""""""""""""""""""""""""""""
+
+Checker for `STR51-CPP rule<https://wiki.sei.cmu.edu/confluence/x/E3s-BQ>`_.
+
+It warns on possible ``std::string`` construction from a ``nullptr``.
+
 .. _alpha-security-ArrayBound:
 
 alpha.security.ArrayBound (C)
 """""""""""""""""""""""""""""
 Warn about buffer overflows (older checker).
 
 .. code-block:: c
 

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

def Str32cChecker : Checker<"32c">,
   Dependencies<[StrCheckerBase]>,
   Documentation<HasDocumentation>;
 
 def Str50cppChecker : Checker<"50cpp">,
   HelpText<"SEI CERT checker of rules defined in STR50-CPP">,
   Dependencies<[StrCheckerBase]>,
   Documentation<HasDocumentation>;
 
+def Str51cppChecker : Checker<"51cpp">,
+  HelpText<"SEI CERT checker of rules defined in STR51-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/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

public:
   ConstraintManager &getConstraintManager() {
     return Eng.getConstraintManager();
   }
 
   StoreManager &getStoreManager() {
     return Eng.getStoreManager();
   }
 
+  MemRegionManager &getRegionManager() { return Eng.getRegionManager(); }
+
   /// Returns the previous node in the exploded graph, which includes
   /// the state of the program before the checker ran. Note, checkers should
   /// not retain the node in their state since the nodes might get invalidated.
   ExplodedNode *getPredecessor() { return Pred; }
   const ProgramStateRef &getState() const { return Pred->getState(); }
 
   /// Check if the checker changed the state of the execution; ex: added
   /// a new transition or a bug report.

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

 //  - 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
 //
+//  - STR51-CPP: Do not attempt to create a std::string from a null pointer:
+//     - https://wiki.sei.cmu.edu/confluence/x/E3s-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"
public:
   /// 'c_string[length] = '\0';'. In this case we store the string is being
   /// null-terminated in the 'NullTerminationMap'.
   void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
 
   /// It reports a note on array declarations which may cannot store a string
   /// because of its size, similar to the check in 'checkBind()'.
   void checkPostStmt(const DeclStmt *S, CheckerContext &C) const;
 
+  /// Evalute function calls defined in 'EvalCDM'.
+  bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+
   /// \{
   /// Check the function calls defined in 'CDM'.
   /// STR30-C.
   void checkMemchr(const CallEvent &Call, const CallContext &CallC,
                    CheckerContext &C) const;
   void checkStrchr(const CallEvent &Call, const CallContext &CallC,
                    CheckerContext &C) const;
   void checkStrrchr(const CallEvent &Call, const CallContext &CallC,
public:
   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.
+  /// Creates 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;
+  bool EnableStr51cppChecker = 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 = {
       // char *strncpy(char *dest, const char *src, size_t count)
       {{"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},
+
+      // The following checks STR50-CPP, STR51-CPP rules.
+      {{{"std", "basic_string"}},
        {&StrCheckerBase::checkStringConstructor, {None, 0}}}};
 
   BugType BT{this, "Insecure string handler function call",
              categories::SecurityError};
 };
 } // namespace
 
 /// It stores whether the string is null-terminated.
void StrCheckerBase::checkRead(const CallEvent &Call, const CallContext &CallC,
   // 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)
+  if (Call.getNumArgs() == 0)
     return;
 
+  const auto *MR = getRegion(Call.getArgSVal(0));
   const auto *DR = dyn_cast_or_null<DeclRegion>(getRegion(Call.getArgSVal(0)));
-  if (!DR)
+
+  ProgramStateRef State = C.getState();
+  const Expr *SrcArg = Call.getArgExpr(0);
+
+  // In case of STR51-CPP we need to be sure whether the constructor is
+  // called with a non-null string.
+  SVal SrcV = Call.getArgSVal(0);
+  if (EnableStr51cppChecker && MR) {
+    // We are only interested in possible nullptrs.
+    // Assume that only non-arguments could be nullable.
+    // Assume that only non-methods could be nullable.
+    // FIXME: Add more assumptions to tweak the reports.
+    bool IsNullable = false;
+    if (DR) {
+      IsNullable = !isa<StackArgumentsSpaceRegion>(DR->getMemorySpace());
+    } else if (const auto *CE = dyn_cast<CallExpr>(SrcArg->IgnoreImpCasts())) {
+      IsNullable = !dyn_cast<CXXMemberCallExpr>(CE);
+    }
+
+    // 'SrcV' is problematic iff it is not exactly non-null and nullable.
+    if (IsNullable && MR->getSymbolicBase() &&
+        !State->isNonNull(SrcV).isConstrainedTrue()) {
+
+      auto Report = std::make_unique<PathSensitiveBugReport>(
+          BT, "Constructing from nullptr", C.generateErrorNode());
+      Report->addRange(SrcArg->getSourceRange());
+      Report->markInteresting(MR);
+
+      // 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));
+      return;
+    }
+  }
+
+  // In case of STR50-CPP we need to be sure whether the constructor is
+  // called with a null-terminated string.
+  if (!EnableStr50cppChecker)
     return;
 
-  const bool *IsNullTerminated = C.getState()->get<NullTerminationMap>(DR);
+  // If the second argument is present which is the size of the string
+  // assume that the string will be null-terminated.
+  if (Call.getNumArgs() > 1)
+    return;
+
+  const bool *IsNullTerminated = State->get<NullTerminationMap>(MR);
   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));
 
#define REGISTER_CHECKER(Name)                                                 \
   }                                                                            \
                                                                                \
   bool ento::shouldRegister##Name(const CheckerManager &) { return true; }
 
 REGISTER_CHECKER(Str30cChecker)
 REGISTER_CHECKER(Str31cChecker)
 REGISTER_CHECKER(Str32cChecker)
 REGISTER_CHECKER(Str50cppChecker)
+REGISTER_CHECKER(Str51cppChecker)

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

public:
     basic_string &insert(size_type index, size_type count, CharT ch);
     basic_string &replace(size_type pos, size_type count, const basic_string &str);
     void pop_back();
     void push_back(CharT ch);
     void reserve(size_type new_cap);
     void resize(size_type count);
     void shrink_to_fit();
     void swap(basic_string &other);
+
+    bool empty();
+    size_t size() const;
   };
 
   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

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

+// RUN: %clang_analyze_cc1 -fcxx-exceptions \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.51cpp \
+// RUN:  -verify %s
+
+// See the examples on the page of STR51-CPP:
+// https://wiki.sei.cmu.edu/confluence/x/E3s-BQ
+
+// expected-no-diagnostics
+
+#include "../Inputs/system-header-simulator.h"
+#include "../Inputs/system-header-simulator-cxx.h"
+
+namespace std {
+char *getenv(const char *varname);
+}; // namespace std
+
+class C {
+  std::string S;
+public:
+  C(const std::string &S) : S(S) {} // no-warning: StackArgumentsSpaceRegion
+};
+
+class Slice {
+public:
+  Slice() : data_(""), size_(0) { }
+
+  std::string ToString() const { return std::string(data_, size_); }
+  // no-warning: StackArgumentsSpaceRegion
+
+private:
+  const char* data_;
+  size_t size_;
+};

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

+// RUN: %clang_analyze_cc1 -fcxx-exceptions \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.51cpp \
+// RUN:  -verify %s
+
+// See the examples on the page of STR51-CPP:
+// https://wiki.sei.cmu.edu/confluence/x/E3s-BQ
+
+#include "../Inputs/system-header-simulator-cxx.h"
+
+namespace std {
+char *getenv(const char *varname);
+}; // namespace std
+
+namespace test_getenv_bad {
+void f() {
+  std::string tmp(std::getenv("TMP"));
+  // expected-warning@-1 {{Constructing from nullptr}}
+  if (!tmp.empty()) {
+    // ...
+  }
+}
+} // namespace test_getenv_bad
+
+namespace test_getenv_good {
+void f() {
+  const char *tmpPtrVal = std::getenv("TMP");
+  std::string tmp(tmpPtrVal ? tmpPtrVal : "");
+  if (!tmp.empty()) {
+    // ...
+  }
+}
+} // namespace test_getenv_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:722 {{Called C++ object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:725 {{Called C++ object pointer is null}}
 #endif
 }