Charusso updated this revision to Diff 265963. Charusso retitled this revision from "[analyzer] CERT: STR30-C" to "[analyzer] CERT STR rule checkers: STR30-C". Charusso added a comment. Herald added subscribers: ASDenysPetrov, martong, steakhal.
- Refactor. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71155/new/ https://reviews.llvm.org/D71155 Files: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp clang/test/Analysis/cert/str30-c-notes.cpp clang/test/Analysis/cert/str30-c.cpp
Index: clang/test/Analysis/cert/str30-c.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str30-c.cpp @@ -0,0 +1,58 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.30c \ +// RUN: -verify %s + +// See the examples on the page of STR30-C: +// https://wiki.sei.cmu.edu/confluence/x/VtYxBQ + +#include "../Inputs/system-header-simulator.h" + +typedef __SIZE_TYPE__ size_t; +typedef __typeof__((char *)0 - (char *)0) ptrdiff_t; + +void free(void *memblock); +void *malloc(size_t size); + +char *strrchr(const char *str, int c); +int puts(const char *str); + +namespace test_strrchr_bad { +const char *get_dirname(const char *pathname) { + char *slash; + slash = strrchr(pathname, '/'); + if (slash) { + *slash = '\0'; /* Undefined behavior */ + // expected-warning@-1 {{'*slash' is pointing to a constant string}} + } + return pathname; +} + +int main(void) { + puts(get_dirname(__FILE__)); + return 0; +} +} // namespace test_strrchr_bad + +namespace test_strrchr_good { +char *get_dirname(const char *pathname, char *dirname, size_t size) { + const char *slash; + slash = strrchr(pathname, '/'); + if (slash) { + ptrdiff_t slash_idx = slash - pathname; + if ((size_t)slash_idx < size) { + memcpy(dirname, pathname, slash_idx); + dirname[slash_idx] = '\0'; + return dirname; + } + } + return 0; +} + +int main(void) { + char dirname[260]; + if (get_dirname(__FILE__, dirname, sizeof(dirname))) { + puts(dirname); + } + return 0; +} +} // namespace test_strrchr_good Index: clang/test/Analysis/cert/str30-c-notes.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str30-c-notes.cpp @@ -0,0 +1,43 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.30c \ +// RUN: -analyzer-output=text -verify %s + +// See the examples on the page of STR30-C: +// https://wiki.sei.cmu.edu/confluence/x/VtYxBQ + +#include "../Inputs/system-header-simulator.h" + +typedef __SIZE_TYPE__ size_t; + +void free(void *memblock); +void *malloc(size_t size); + +char *strrchr(const char *str, int c); +int puts(const char *str); + +namespace test_strrchr_bad { +const char *get_dirname(const char *pathname) { + char *slash; + slash = strrchr(pathname, '/'); + // expected-note@-1 {{'strrchr' returns a pointer to the constant 'pathname'}} + + slash = strrchr(slash, '/'); + // expected-note@-1 {{'strrchr' returns a pointer to the constant 'slash'}} + + if (slash) { + // expected-note@-1 {{'slash' is non-null}} + // expected-note@-2 {{Taking true branch}} + + *slash = '\0'; /* Undefined behavior */ + // expected-note@-1 {{'*slash' is pointing to a constant string}} + // expected-warning@-2 {{'*slash' is pointing to a constant string}} + } + return pathname; +} + +int main(void) { + puts(get_dirname(__FILE__)); + // expected-note@-1 {{Calling 'get_dirname'}} + return 0; +} +} // namespace test_strrchr_bad Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp @@ -12,6 +12,9 @@ // - https://wiki.sei.cmu.edu/confluence/x/ptUxBQ // // This checker is a base checker which consist of the following checkers: +// - STR30-C: Do not attempt to modify string literals: +// - https://wiki.sei.cmu.edu/confluence/x/VtYxBQ +// // - 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 @@ -83,6 +86,17 @@ /// \{ /// 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, + CheckerContext &C) const; + void checkStrstr(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; + void checkStrpbrk(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; /// STR31-C. void checkGets(const CallEvent &Call, const CallContext &CallC, CheckerContext &C) const; @@ -105,6 +119,7 @@ void createCallOverflowReport(const CallEvent &Call, const CallContext &CallC, CheckerContext &C) const; + bool EnableStr30cChecker = false; bool EnableStr31cChecker = false; bool EnableStr32cChecker = false; @@ -113,6 +128,18 @@ 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); + {{"memchr", 3}, {&StrCheckerBase::checkMemchr, {None, 0}}}, + // char *strchr(const char *str, int c); + {{"strchr", 2}, {&StrCheckerBase::checkStrchr, {None, 0}}}, + // char *strrchr(const char *str, int c); + {{"strrchr", 2}, {&StrCheckerBase::checkStrrchr, {None, 0}}}, + // char *strstr(const char *str, const char *strSearch); + {{"strstr", 2}, {&StrCheckerBase::checkStrstr, {None, 0}}}, + // char *strpbrk(const char *str, const char *strCharSet); + {{"strpbrk", 2}, {&StrCheckerBase::checkStrpbrk, {None, 0}}}, + // The following checks STR31-C rules. // char *gets(char *dest); {{"gets", 1}, {&StrCheckerBase::checkGets, {0}}}, @@ -138,6 +165,9 @@ /// It stores the allocations which we emit notes on. REGISTER_LIST_WITH_PROGRAMSTATE(AllocationList, const MemRegion *) +/// It stores the constant strings which we have pointers to them. +REGISTER_LIST_WITH_PROGRAMSTATE(PointerList, const MemRegion *) + //===----------------------------------------------------------------------===// // Helper functions. //===----------------------------------------------------------------------===// @@ -356,6 +386,131 @@ return true; } +//===----------------------------------------------------------------------===// +// The following checks STR30-C rules. +//===----------------------------------------------------------------------===// + +/// Check whether a constant string is being modified. If so, emit a report. +static bool isConstantStringModify(SVal L, SVal V, const Stmt *S, + CheckerContext &C, const BugType &BT) { + // FIXME: Handle more complex modifications. + const auto *BO = dyn_cast<BinaryOperator>(S); + if (!BO) + return false; + + const MemRegion *MR = L.getAsRegion(); + if (!MR) + return false; + + if (!C.getState()->contains<PointerList>(MR)) + return false; + + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + + Out << '\'' << printPretty(BO->getLHS(), C) + << "' is pointing to a constant string"; + + auto Report = std::make_unique<PathSensitiveBugReport>(BT, Out.str(), + C.generateErrorNode()); + Report->addRange(BO->getLHS()->getSourceRange()); + Report->markInteresting(MR); + + // Track the allocation. + bugreporter::trackExpressionValue(Report->getErrorNode(), BO->getLHS(), + *Report); + + C.emitReport(std::move(Report)); + return true; +} + +/// Check whether a constant string is being stored. If so, emit a report. +static void checkCharPtr(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) { + const MemRegion *MR = Call.getReturnValue().getAsRegion(); + if (!MR) + return; + + ProgramStateRef State = C.getState(); + const Expr *SrcArg = Call.getArgExpr(*CallC.SourcePos); + const VarRegion *SrcVR = nullptr; + if (const auto *DRE = dyn_cast<DeclRefExpr>(SrcArg->IgnoreImpCasts())) + if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) + SrcVR = State->getRegion(VD, C.getLocationContext()); + + if (!SrcVR) + return; + + const MemRegion *PtrMR = MR->getBaseRegion(); + if (!PtrMR) + return; + + const auto *PtrTR = PtrMR->getAs<TypedRegion>(); + if (!PtrTR) + return; + + QualType Ty = PtrTR->getLocationType(); + if (Ty->isPointerType()) + Ty = Ty->getPointeeType(); + + if (!Ty.isConstQualified()) + return; + + std::string CallName = Call.getCalleeIdentifier()->getName().str(); + std::string ArgName = SrcVR->getDecl()->getNameAsString(); + + const NoteTag *Tag = C.getNoteTag( + [=](PathSensitiveBugReport &BR) -> std::string { + if (!BR.isInteresting(MR)) + return ""; + + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + Out << '\'' << CallName << "' returns a pointer to the constant '" + << ArgName << '\''; + + return Out.str().str(); + }, + /*IsPrunable=*/false); + + State = State->add<PointerList>(MR); + C.addTransition(State, Tag); +} + +void StrCheckerBase::checkMemchr(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr30cChecker) + checkCharPtr(Call, CallC, C); +} + +void StrCheckerBase::checkStrchr(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr30cChecker) + checkCharPtr(Call, CallC, C); +} +void StrCheckerBase::checkStrrchr(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr30cChecker) + checkCharPtr(Call, CallC, C); +} + +void StrCheckerBase::checkStrstr(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr30cChecker) + checkCharPtr(Call, CallC, C); +} + +void StrCheckerBase::checkStrpbrk(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr30cChecker) + checkCharPtr(Call, CallC, C); +} + //===----------------------------------------------------------------------===// // The following checks STR31-C rules. //===----------------------------------------------------------------------===// @@ -413,6 +568,10 @@ void StrCheckerBase::checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const { + if (EnableStr30cChecker) + if (isConstantStringModify(L, V, S, C, BT)) + return; + if (!EnableStr32cChecker) return; @@ -521,5 +680,6 @@ \ bool ento::shouldRegister##Name(const CheckerManager &) { return true; } +REGISTER_CHECKER(Str30cChecker) REGISTER_CHECKER(Str31cChecker) REGISTER_CHECKER(Str32cChecker) Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -122,6 +122,7 @@ {{CDF_MaybeBuiltin, "mempcpy", 3}, &CStringChecker::evalMempcpy}, {{CDF_MaybeBuiltin, "memcmp", 3}, &CStringChecker::evalMemcmp}, {{CDF_MaybeBuiltin, "memmove", 3}, &CStringChecker::evalMemmove}, + {{CDF_MaybeBuiltin, "memchr", 3}, &CStringChecker::evalMemchr}, {{CDF_MaybeBuiltin, "memset", 3}, &CStringChecker::evalMemset}, {{CDF_MaybeBuiltin, "explicit_memset", 3}, &CStringChecker::evalMemset}, {{CDF_MaybeBuiltin, "strcpy", 2}, &CStringChecker::evalStrcpy}, @@ -138,6 +139,10 @@ {{CDF_MaybeBuiltin, "strcasecmp", 2}, &CStringChecker::evalStrcasecmp}, {{CDF_MaybeBuiltin, "strncasecmp", 3}, &CStringChecker::evalStrncasecmp}, {{CDF_MaybeBuiltin, "strsep", 2}, &CStringChecker::evalStrsep}, + {{CDF_MaybeBuiltin, "strpbrk", 2}, &CStringChecker::evalStrpbrk}, + {{CDF_MaybeBuiltin, "strchr", 2}, &CStringChecker::evalStrchr}, + {{CDF_MaybeBuiltin, "strrchr", 2}, &CStringChecker::evalStrrchr}, + {{CDF_MaybeBuiltin, "strstr", 2}, &CStringChecker::evalStrstr}, {{CDF_MaybeBuiltin, "bcopy", 3}, &CStringChecker::evalBcopy}, {{CDF_MaybeBuiltin, "bcmp", 3}, &CStringChecker::evalMemcmp}, {{CDF_MaybeBuiltin, "bzero", 2}, &CStringChecker::evalBzero}, @@ -189,6 +194,14 @@ void evalStrsep(CheckerContext &C, const CallExpr *CE) const; + // Evaluate function calls which returns a pointer to the string argument. + void evalCharPtrCommon(CheckerContext &C, const CallExpr *CE) const; + void evalMemchr(CheckerContext &C, const CallExpr *CE) const; + void evalStrchr(CheckerContext &C, const CallExpr *CE) const; + void evalStrrchr(CheckerContext &C, const CallExpr *CE) const; + void evalStrstr(CheckerContext &C, const CallExpr *CE) const; + void evalStrpbrk(CheckerContext &C, const CallExpr *CE) const; + void evalStdCopy(CheckerContext &C, const CallExpr *CE) const; void evalStdCopyBackward(CheckerContext &C, const CallExpr *CE) const; void evalStdCopyCommon(CheckerContext &C, const CallExpr *CE) const; @@ -2120,6 +2133,58 @@ C.addTransition(State); } +void CStringChecker::evalCharPtrCommon(CheckerContext &C, + const CallExpr *CE) const { + CurrentFunctionDescription = "char pointer returning function"; + + SValBuilder &SVB = C.getSValBuilder(); + ProgramStateRef State = C.getState(); + const LocationContext *LCtx = C.getLocationContext(); + + const Expr *SrcExpr = CE->getArg(0); + + // Add a conjured index to the source string. + SVal SrcV = C.getSVal(SrcExpr); + DefinedOrUnknownSVal Index = SVB.conjureSymbolVal( + SrcExpr, LCtx, SVB.getArrayIndexType(), C.blockCount()); + + SVal Offset = SVB.evalBinOp(State, BO_Add, SrcV, Index, SrcExpr->getType()); + + State = State->BindExpr(CE, LCtx, Offset); + C.addTransition(State); +} + +void CStringChecker::evalMemchr(CheckerContext &C, const CallExpr *CE) const { + // void *memchr(const void *buffer, int c, size_t count); + const Expr *BufferExpr = CE->getArg(0); + QualType Ty = BufferExpr->getType().getUnqualifiedType(); + if (Ty->isPointerType()) + Ty = Ty->getPointeeType(); + + if (Ty->isAnyCharacterType()) + evalCharPtrCommon(C, CE); +} + +void CStringChecker::evalStrchr(CheckerContext &C, const CallExpr *CE) const { + // char *strchr(const char *str, int c); + evalCharPtrCommon(C, CE); +} + +void CStringChecker::evalStrrchr(CheckerContext &C, const CallExpr *CE) const { + // char *strrchr(const char *str, int c); + evalCharPtrCommon(C, CE); +} + +void CStringChecker::evalStrstr(CheckerContext &C, const CallExpr *CE) const { + // char *strstr(const char *str, const char *strSearch); + evalCharPtrCommon(C, CE); +} + +void CStringChecker::evalStrpbrk(CheckerContext &C, const CallExpr *CE) const { + // char *strpbrk(const char *str, const char *strCharSet); + evalCharPtrCommon(C, CE); +} + // These should probably be moved into a C++ standard library checker. void CStringChecker::evalStdCopy(CheckerContext &C, const CallExpr *CE) const { evalStdCopyCommon(C, CE); Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -856,6 +856,11 @@ Documentation<NotDocumented>, Hidden; +def Str30cChecker : Checker<"30c">, + HelpText<"SEI CERT checker of rules defined in STR30-C">, + Dependencies<[StrCheckerBase]>, + Documentation<HasDocumentation>; + def Str31cChecker : Checker<"31c">, HelpText<"SEI CERT checker of rules defined in STR31-C">, Dependencies<[StrCheckerBase]>, Index: clang/docs/analyzer/checkers.rst =================================================================== --- clang/docs/analyzer/checkers.rst +++ clang/docs/analyzer/checkers.rst @@ -1961,6 +1961,16 @@ return putenv(env); // putenv function should not be called with auto variables } +.. _alpha-security-cert-str-30c: + +alpha.security.cert.str.30c +""""""""""""""""""""""""""" + +Checker for `STR30-C rule<https://wiki.sei.cmu.edu/confluence/x/VtYxBQ>`_. + +It warns on misusing the following functions: +``strpbrk()``, ``strchr()``, ``strrchr()``, ``strstr()``, ``memchr()``. + .. _alpha-security-cert-str-31c: alpha.security.cert.str.31c
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
