Charusso updated this revision to Diff 265959. Charusso retitled this revision from "[analyzer] CERT: STR31-C" to "[analyzer] CERT STR rule checkers: STR31-C". Charusso added a comment.
- Refactor. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70411/new/ https://reviews.llvm.org/D70411 Files: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/lib/StaticAnalyzer/Checkers/AllocationState.h clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp clang/test/Analysis/Inputs/system-header-simulator.h clang/test/Analysis/cert/str31-c-false-positive-suppression.cpp clang/test/Analysis/cert/str31-c-notes.cpp clang/test/Analysis/cert/str31-c.cpp
Index: clang/test/Analysis/cert/str31-c.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str31-c.cpp @@ -0,0 +1,184 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.31c \ +// RUN: -verify %s + +// See the examples on the page of STR31-C: +// https://wiki.sei.cmu.edu/confluence/x/sNUxBQ + +#include "../Inputs/system-header-simulator.h" + +typedef __SIZE_TYPE__ size_t; + +void free(void *memblock); +void *malloc(size_t size); + +namespace test_gets_bad { +#define BUFFER_SIZE 1024 + +void func(void) { + char buf[BUFFER_SIZE]; + if (gets(buf)) { + // expected-warning@-1 {{'gets' could write outside of 'buf'}} + } +} +} // namespace test_gets_bad + +namespace test_gets_good { +enum { BUFFERSIZE = 32 }; + +void func(void) { + char buff[BUFFERSIZE]; + + if (fgets(buff, sizeof(buff), stdin)) { + } +} +} // namespace test_gets_good + +namespace test_sprintf_bad { +void func(const char *name) { + char buf[128]; + sprintf(buf, "%s.txt", name); + // expected-warning@-1 {{'sprintf' could write outside of 'buf'}} +} +} // namespace test_sprintf_bad + +namespace test_sprintf_good { +void func(const char *name) { + char buff[128]; + snprintf(buff, sizeof(buff), "%s.txt", name); +} +} // namespace test_sprintf_good + +namespace test_fscanf_bad { +enum { BUF_LENGTH = 1024 }; + +void get_data(void) { + char buf[BUF_LENGTH]; + fscanf(stdin, "%s", buf); + // expected-warning@-1 {{'fscanf' could write outside of 'buf'}} +} +} // namespace test_fscanf_bad + +namespace test_fscanf_good { +enum { BUF_LENGTH = 1024 }; + +void get_data(void) { + char buff[BUF_LENGTH]; + fscanf(stdin, "%1023s", buff); +} +} // namespace test_fscanf_good + +namespace test_strcpy_bad { +int main(int argc, char *argv[]) { + const char *const name = (argc && argv[0]) ? argv[0] : ""; + char prog_name[128]; + strcpy(prog_name, name); + // expected-warning@-1 {{'strcpy' could write outside of 'prog_name'}} + return 0; +} + +void func(void) { + char buff[256]; + char *editor = getenv("EDITOR"); + if (editor != NULL) { + strcpy(buff, editor); + // expected-warning@-1 {{'strcpy' could write outside of 'buff'}} + } +} +} // namespace test_strcpy_bad + +namespace test_strcpy_good { +int main(int argc, char *argv[]) { + const char *const name = (argc && argv[0]) ? argv[0] : ""; + char *prog_name2 = (char *)malloc(strlen(name) + 1); + if (prog_name2 != NULL) { + strcpy(prog_name2, name); + } + + free(prog_name2); + return 0; +} + +void func(void) { + char *buff2; + char *editor = getenv("EDITOR"); + if (editor != NULL) { + size_t len = strlen(editor) + 1; + buff2 = (char *)malloc(len); + if (buff2 != NULL) { + strcpy(buff2, editor); + } + + free(buff2); + } +} +} // namespace test_strcpy_good + +//===----------------------------------------------------------------------===// +// The following are from the rule's page which we do not handle yet. +//===----------------------------------------------------------------------===// + +namespace test_loop_index_bad { +void copy(size_t n, char src[n], char dest[n]) { + size_t i; + + for (i = 0; src[i] && (i < n); ++i) { + dest[i] = src[i]; + } + dest[i] = '\0'; +} +} // namespace test_loop_index_bad + +namespace test_loop_index_good { +void copy(size_t n, char src[n], char dest[n]) { + size_t i; + + for (i = 0; src[i] && (i < n - 1); ++i) { + dest[i] = src[i]; + } + dest[i] = '\0'; +} +} // namespace test_loop_index_good + +namespace test_getchar_bad { +enum { BUFFERSIZE = 32 }; + +void func(void) { + char buf[BUFFERSIZE]; + char *p; + int ch; + p = buf; + while ((ch = getchar()) != '\n' && ch != EOF) { + *p++ = (char)ch; + } + *p++ = 0; + if (ch == EOF) { + /* Handle EOF or error */ + } +} +} // namespace test_getchar_bad + +namespace test_getchar_good { +enum { BUFFERSIZE = 32 }; + +void func(void) { + char buf[BUFFERSIZE]; + int ch; + size_t index = 0; + size_t chars_read = 0; + + while ((ch = getchar()) != '\n' && ch != EOF) { + if (index < sizeof(buf) - 1) { + buf[index++] = (char)ch; + } + chars_read++; + } + buf[index] = '\0'; /* Terminate string */ + if (ch == EOF) { + /* Handle EOF or error */ + } + if (chars_read > index) { + /* Handle truncation */ + } +} +} // namespace test_getchar_good Index: clang/test/Analysis/cert/str31-c-notes.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str31-c-notes.cpp @@ -0,0 +1,47 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.31c \ +// RUN: -analyzer-output=text -verify %s + +// See the examples on the page of STR31-C: +// https://wiki.sei.cmu.edu/confluence/x/sNUxBQ + +#include "../Inputs/system-header-simulator.h" + +void *malloc(size_t size); +void free(void *memblock); + +void test_simple_size(unsigned size) { + size = 13; + + char *buf = (char *)malloc(size); + // expected-note@-1 {{Memory is allocated}} + // expected-note@-2 {{'buf' initialized here}} + + if (gets(buf)) { + // expected-note@-1 {{'gets' could write outside of 'buf'}} + // expected-warning@-2 {{'gets' could write outside of 'buf'}} + } + + free(buf); +} + +void test_strlen_without_null_terminator(const char *src) { + char *buff2; + char *editor = getenv("EDITOR"); + if (editor != NULL) { + // expected-note@-1 {{Assuming 'editor' is not equal to NULL}} + // expected-note@-2 {{Taking true branch}} + size_t len = strlen(editor); + buff2 = (char *)malloc(len); + // expected-note@-1 {{Memory is allocated}} + // expected-note@-2 {{Value assigned to 'buff2'}} + if (buff2 != NULL) { + // expected-note@-1 {{Assuming 'buff2' is not equal to NULL}} + // expected-note@-2 {{Taking true branch}} + strcpy(buff2, editor); + // expected-note@-1 {{'strcpy' could write outside of 'buff2'}} + // expected-warning@-2 {{'strcpy' could write outside of 'buff2'}} + } + free(buff2); + } +} Index: clang/test/Analysis/cert/str31-c-false-positive-suppression.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str31-c-false-positive-suppression.cpp @@ -0,0 +1,70 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.31c \ +// RUN: -verify %s + +// See the examples on the page of STR31-C: +// https://wiki.sei.cmu.edu/confluence/x/sNUxBQ + +#include "../Inputs/system-header-simulator.h" + +void free(void *memblock); +void *malloc(size_t size); + +// FIXME: The following tests could fail if the destination array has an offset, +// which we do not model yet. Also we cannot reason about complex sized +// allocations which makes this checker a little bit noisy. + +void test_known_size_cannot_overflow() { + char dest[128]; + strcpy(dest, "src"); // no-warning +} + +void test_everything_known() { + char *dest = (char *)malloc(strlen("Foo") + 1); + if (dest) + strcpy(dest, "Foo"); // no-warning + free(dest); +} + +void test_complex_size_may_false_positive(const char *src) { + char *dest; + size_t size; + size = strlen(src); + + dest = (char *)malloc(size * 2 + 2); + + strcpy(dest, &src[13]); + // expected-warning@-1 {{'strcpy' could write outside of 'dest'}} + free(dest); +} + +void test_complex_size_wrong_size(const char *src) { + char *dest; + size_t size = strlen(src); + + // FIXME: '+ 2' missing so there is not enough space for the null terminator. + dest = (char *)malloc(2 * size + 1); + + strcpy(dest, &src[13]); + // expected-warning@-1 {{'strcpy' could write outside of 'dest'}} + free(dest); +} + +void test_char_by_char_may_false_positive(const char *src, size_t size) { + const char *s; + unsigned char c; + char *escape = (char *)malloc(size); + char *e = escape; + for (s = src; (c = *s) != '\0'; ++s) { + if (c == '\n') { + strcpy(e, "\\n"); + // expected-warning@-1 {{'strcpy' could write outside of 'e'}} + e += 2; + } else { + *e++ = c; + } + } + + *e = '\0'; + free(escape); +} Index: clang/test/Analysis/Inputs/system-header-simulator.h =================================================================== --- clang/test/Analysis/Inputs/system-header-simulator.h +++ clang/test/Analysis/Inputs/system-header-simulator.h @@ -32,6 +32,12 @@ int printf(const char *restrict format, ...); int fprintf(FILE *restrict, const char *restrict, ...); int getchar(void); +char *gets(char *buffer); +char *fgets(char *str, int n, FILE *stream); +int sprintf(char *buffer, const char *format, ...); +int snprintf(char *buffer, size_t count, const char *format, ...); +int fscanf(FILE *stream, const char *format, ...); +char *getenv(const char *varname); void setbuf(FILE *restrict, char *restrict); int setvbuf(FILE *restrict, char *restrict, int, size_t); Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp @@ -0,0 +1,257 @@ +//===- StrCheckerBase - SEI CERT rules checker defined in STR ---*- 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 StrCheckerBase which tries to find the SEI CERT string +// handler function issues. +// The rules can be found in section 'Rule 07. Characters and Strings (STR)': +// - https://wiki.sei.cmu.edu/confluence/x/ptUxBQ +// +// This checker is a base checker which consist of the following checkers: +// - 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 +// +//===----------------------------------------------------------------------===// + +#include "AllocationState.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 "llvm/ADT/Optional.h" +#include <utility> + +using namespace clang; +using namespace ento; + +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) + : DestinationPos(DestinationPos), SourcePos(SourcePos) {} + + Optional<unsigned> DestinationPos; + Optional<unsigned> SourcePos; +}; + +class StrCheckerBase : public Checker<check::PostCall> { +public: + /// Check the function calls defined in 'CDM'. + /// Function calls of 'CDM' could cause a not null-terminated string. + /// + /// In case of STR31-C we want to emit an error immediately on such calls. + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + + /// \{ + /// Check the function calls defined in 'CDM'. + void checkGets(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; + 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; + /// \} + + /// In case of STR31-C emit an error report of possible buffer overflow. + void createCallOverflowReport(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; + + bool EnableStr31cChecker = false; + +private: + using StrCheck = std::function<void(const StrCheckerBase *, const CallEvent &, + const CallContext &, CheckerContext &)>; + + const CallDescriptionMap<std::pair<StrCheck, CallContext>> CDM = { + // The following checks STR31-C rules. + // char *gets(char *dest); + {{"gets", 1}, {&StrCheckerBase::checkGets, {0}}}, + // int sprintf(char *dest, const char *format, ... [const char *source]); + {{"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}}}}; + + BugType BT{this, "Insecure string handler function call", + categories::SecurityError}; +}; +} // namespace + +//===----------------------------------------------------------------------===// +// Helper functions. +//===----------------------------------------------------------------------===// + +/// Returns the interesting region of \p V. +static const MemRegion *getRegion(SVal V) { + const MemRegion *MR = V.getAsRegion(); + if (!MR) + return nullptr; + + 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); + E->printPretty(Out, /*Helper=*/nullptr, + C.getASTContext().getPrintingPolicy()); + return Out.str().str(); +} + +// FIXME: Make it static by make the BugType copyable. +void StrCheckerBase::createCallOverflowReport(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + unsigned DestPos = *CallC.DestinationPos; + SVal DestV = Call.getArgSVal(DestPos); + const MemRegion *MR = getRegion(DestV); + if (!MR) + return; + + ProgramStateRef State = C.getState(); + + StringRef CallName = Call.getCalleeIdentifier()->getName(); + + const Expr *Arg = Call.getArgExpr(DestPos); + + SmallString<256> Msg; + llvm::raw_svector_ostream Out(Msg); + Out << '\'' << CallName << "' could write outside of '" << printPretty(Arg, C) + << '\''; + + auto Report = std::make_unique<PathSensitiveBugReport>(BT, Out.str(), + C.generateErrorNode()); + Report->addRange(Arg->getSourceRange()); + + // Track the allocation. + bugreporter::trackExpressionValue(Report->getErrorNode(), Arg, *Report); + if (const SymbolRef Sym = DestV.getAsSymbol()) + Report->addVisitor(allocation_state::getMallocBRVisitor(Sym)); + + C.emitReport(std::move(Report)); +} + +//===----------------------------------------------------------------------===// +// The following checks STR31-C rules. +//===----------------------------------------------------------------------===// + +void StrCheckerBase::checkGets(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr31cChecker) + createCallOverflowReport(Call, CallC, C); +} + +void StrCheckerBase::checkSprintf(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr31cChecker) + createCallOverflowReport(Call, CallC, C); +} + +void StrCheckerBase::checkFscanf(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (!EnableStr31cChecker) + return; + + const auto *FormatExpr = + dyn_cast<StringLiteral>(Call.getArgExpr(1)->IgnoreImpCasts()); + if (!FormatExpr) + return; + + // FIXME: Handle multiple buffers. + if (FormatExpr->getString() != "%s") + return; + + createCallOverflowReport(Call, CallC, C); +} + +void StrCheckerBase::checkStrcpy(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (!EnableStr31cChecker) + return; + + ProgramStateRef State = C.getState(); + SValBuilder &SVB = C.getSValBuilder(); + SVal SrcV = Call.getArgSVal(*CallC.SourcePos); + SVal DestV = Call.getArgSVal(*CallC.DestinationPos); + + // Check the size of the allocation 'DestSize' to prevent false alarms. + const MemRegion *SrcMR = getRegion(SrcV); + const MemRegion *DestMR = getRegion(DestV); + if (!SrcMR || !DestMR) + return; + + DefinedOrUnknownSVal SrcSize = getDynamicSize(State, SrcMR, SVB); + DefinedOrUnknownSVal DestSize = getDynamicSize(State, DestMR, SVB); + + // Only care about allocations we could model. + // FIXME: Use heuristics what is an allocator and try to obtain its size. + if (DestSize.isUnknown()) + return; + + if (SrcSize == DestSize) + return; + + if (const llvm::APSInt *SrcSizeInt = SVB.getKnownValue(State, SrcSize)) + if (const llvm::APSInt *DestSizeInt = SVB.getKnownValue(State, DestSize)) + if (SrcSizeInt->getZExtValue() <= DestSizeInt->getZExtValue()) + return; + + // Allocating size of 'strlen(src)' is most likely fine. + // FIXME: Make sure we catch 'strlen()' in complex SVals. + // FIXME: Calcualte the appropriate size for holding the string. + if (const SymExpr *SE = DestSize.getAsSymExpr()) + if (const auto *SIE = dyn_cast<SymIntExpr>(SE)) + if (const auto *SM = dyn_cast<SymbolMetadata>(SIE->getLHS())) + if (SM->getRegion() == SrcMR) + return; + + createCallOverflowReport(Call, CallC, C); +} + +//===----------------------------------------------------------------------===// +// 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::registerStrCheckerBase(CheckerManager &Mgr) { + Mgr.registerChecker<StrCheckerBase>(); +} + +bool ento::shouldRegisterStrCheckerBase(const CheckerManager &) { return true; } + +#define REGISTER_CHECKER(Name) \ + void ento::register##Name(CheckerManager &Mgr) { \ + auto *Checker = Mgr.getChecker<StrCheckerBase>(); \ + Checker->Enable##Name = true; \ + } \ + \ + bool ento::shouldRegister##Name(const CheckerManager &) { return true; } + +REGISTER_CHECKER(Str31cChecker) Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -3268,6 +3268,10 @@ namespace ento { namespace allocation_state { +std::unique_ptr<BugReporterVisitor> getMallocBRVisitor(SymbolRef Sym) { + return std::make_unique<MallocBugVisitor>(Sym); +} + ProgramStateRef markReleased(ProgramStateRef State, SymbolRef Sym, const Expr *Origin) { AllocationFamily Family = AF_InnerBuffer; Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -18,6 +18,7 @@ CastSizeChecker.cpp CastToStructChecker.cpp CastValueChecker.cpp + cert/StrChecker.cpp CheckObjCDealloc.cpp CheckObjCInstMethSignature.cpp CheckPlacementNew.cpp Index: clang/lib/StaticAnalyzer/Checkers/AllocationState.h =================================================================== --- clang/lib/StaticAnalyzer/Checkers/AllocationState.h +++ clang/lib/StaticAnalyzer/Checkers/AllocationState.h @@ -25,6 +25,8 @@ /// AF_InnerBuffer symbols. std::unique_ptr<BugReporterVisitor> getInnerPointerBRVisitor(SymbolRef Sym); +std::unique_ptr<BugReporterVisitor> getMallocBRVisitor(SymbolRef Sym); + /// 'Sym' represents a pointer to the inner buffer of a container object. /// This function looks up the memory region of that object in /// DanglingInternalBufferChecker's program state map. Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -73,6 +73,7 @@ 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>; @@ -848,6 +849,20 @@ } // end "alpha.cert.pos" +let ParentPackage = STR in { + +def StrCheckerBase : Checker<"StrCheckerBase">, + HelpText<"SEI CERT base checker of rules defined in STR">, + Documentation<NotDocumented>, + Hidden; + +def Str31cChecker : Checker<"31c">, + HelpText<"SEI CERT checker of rules defined in STR31-C">, + Dependencies<[StrCheckerBase]>, + Documentation<HasDocumentation>; + +} // end "alpha.cert.str" + let ParentPackage = SecurityAlpha in { def ArrayBoundChecker : Checker<"ArrayBound">, Index: clang/docs/analyzer/checkers.rst =================================================================== --- clang/docs/analyzer/checkers.rst +++ clang/docs/analyzer/checkers.rst @@ -1960,7 +1960,17 @@ return putenv(env); // putenv function should not be called with auto variables } - + +.. _alpha-security-cert-str-31c: + +alpha.security.cert.str.31c +""""""""""""""""""""""""""" + +Checker for `STR31-C rule<https://wiki.sei.cmu.edu/confluence/x/sNUxBQ>`_. + +It warns on misusing the following functions: +``strcpy()``, ``gets()``, ``fscanf()``, ``sprintf()``. + .. _alpha-security-ArrayBound: alpha.security.ArrayBound (C)
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
