Index: clang/docs/analyzer/checkers.rst =================================================================== --- clang/docs/analyzer/checkers.rst +++ clang/docs/analyzer/checkers.rst @@ -1993,6 +1993,16 @@ Also warns on misusing the ``strncpy()`` function. +.. _alpha-security-cert-str-50cpp: + +alpha.security.cert.str.50cpp +""""""""""""""""""""""""""""" + +Checker for `STR50-CPP rule`_. + +It warns on misusing ``std::cin`` and ``std::basic_istream::read()`` to +create a ``std::string`` which may not null-terminated. + .. _alpha-security-ArrayBound: alpha.security.ArrayBound (C) Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -871,6 +871,11 @@ Dependencies<[StrCheckerBase]>, Documentation; +def Str50cppChecker : Checker<"50cpp">, + HelpText<"SEI CERT checker of rules defined in STR50-CPP">, + Dependencies<[StrCheckerBase]>, + Documentation; + } // end "alpha.cert.str" let ParentPackage = SecurityAlpha in { Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp @@ -23,9 +23,14 @@ // 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" @@ -38,6 +43,7 @@ using namespace clang; using namespace ento; +using namespace ast_matchers; namespace { @@ -109,6 +115,13 @@ /// 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. @@ -122,6 +135,7 @@ bool EnableStr30cChecker = false; bool EnableStr31cChecker = false; bool EnableStr32cChecker = false; + bool EnableStr50cppChecker = false; private: using StrCheck = std::function>"}, {&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}; @@ -178,6 +200,9 @@ if (!MR) return nullptr; + if (const auto *ER = dyn_cast(MR)) + return ER->getSuperRegion(); + return MR->getBaseRegion(); } @@ -609,6 +634,124 @@ } } +//===----------------------------------------------------------------------===// +// 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(Call.getOriginExpr()); + if (!OCE) + return; + + const auto *DR = + dyn_cast_or_null(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(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(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(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(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(getRegion(Call.getArgSVal(0))); + if (!DR) + return; + + const bool *IsNullTerminated = C.getState()->get(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(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. //===----------------------------------------------------------------------===// @@ -683,3 +826,4 @@ REGISTER_CHECKER(Str30cChecker) REGISTER_CHECKER(Str31cChecker) REGISTER_CHECKER(Str32cChecker) +REGISTER_CHECKER(Str50cppChecker) Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h =================================================================== --- clang/test/Analysis/Inputs/system-header-simulator-cxx.h +++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h @@ -557,6 +557,7 @@ public: basic_string(); basic_string(const CharT *s); + basic_string(const CharT *s, size_t n); ~basic_string(); void clear(); @@ -588,6 +589,28 @@ typedef basic_string u32string; #endif + struct ios_base { + class failure {}; + }; + + template + class basic_istream { + typedef __SIZE_TYPE__ streamsize; + + public: + streamsize gcount() const; + basic_istream &read(CharT *s, streamsize n); + streamsize width(streamsize); + }; + + template + basic_istream &operator>>(basic_istream &is, CharT *c); + template + Istream &&operator>>(Istream &&st, T &&value); + + typedef basic_istream istream; + extern istream cin; + class exception { public: exception() throw(); Index: clang/test/Analysis/cert/str50-cpp-false-positive-suppression.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str50-cpp-false-positive-suppression.cpp @@ -0,0 +1,20 @@ +// 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}} +} Index: clang/test/Analysis/cert/str50-cpp-notes.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str50-cpp-notes.cpp @@ -0,0 +1,33 @@ +// 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 Index: clang/test/Analysis/cert/str50-cpp.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str50-cpp.cpp @@ -0,0 +1,59 @@ +// 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 Index: clang/test/Analysis/diagnostics/explicit-suppression.cpp =================================================================== --- clang/test/Analysis/diagnostics/explicit-suppression.cpp +++ clang/test/Analysis/diagnostics/explicit-suppression.cpp @@ -19,6 +19,6 @@ 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 }