Charusso created this revision. Charusso added a reviewer: NoQ. Charusso added a project: clang. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun, mgorny. Charusso added a parent revision: D69746: [analyzer] FixItHint: Apply and test hints with the Clang Tidy's script.
This checker warn on `gets()` based on the following rules: https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator It also tries to rewrite the bad code with the help of `FixItHints`. Repository: rC Clang https://reviews.llvm.org/D69813 Files: clang/include/clang/Lex/Preprocessor.h clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp clang/test/Analysis/Inputs/system-header-simulator.h clang/test/Analysis/analyzer-config.c clang/test/Analysis/cert/str31-alloc.cpp clang/test/Analysis/cert/str31-safe.cpp clang/test/Analysis/cert/str31-unsafe.cpp
Index: clang/test/Analysis/cert/str31-unsafe.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str31-unsafe.cpp @@ -0,0 +1,34 @@ +// RUN: %check_analyzer_fixit %s %t \ +// RUN: -analyzer-checker=core,unix,security.cert.Str \ +// RUN: -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \ +// RUN: -I %S + +// See the examples on the page of STR31: +// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator + +#include "../Inputs/system-header-simulator.h" + +// The following is not defined therefore the safe functions are unavailable. +// #define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +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'}} + // CHECK-FIXES: if (fgets(buf, sizeof(buf), stdin)) {} +} +} // 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 Index: clang/test/Analysis/cert/str31-safe.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str31-safe.cpp @@ -0,0 +1,33 @@ +// RUN: %check_analyzer_fixit %s %t \ +// RUN: -analyzer-checker=core,unix,security.cert.Str \ +// RUN: -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \ +// RUN: -I %S + +// See the examples on the page of STR31: +// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator + +#include "../Inputs/system-header-simulator.h" + +#define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +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'}} + // CHECK-FIXES: if (gets_s(buf, sizeof(buf))) {} +} +} // namespace test_gets_bad + +namespace test_gets_good { +enum { BUFFERSIZE = 32 }; + +void func(void) { + char buff[BUFFERSIZE]; + + if (gets_s(buff, sizeof(buff))) {} +} +} // namespace test_gets_good Index: clang/test/Analysis/cert/str31-alloc.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str31-alloc.cpp @@ -0,0 +1,72 @@ +// RUN: %check_analyzer_fixit %s %t \ +// RUN: -analyzer-checker=core,unix,security.cert.Str \ +// RUN: -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \ +// RUN: -I %S + +// See the examples on the page of STR31: +// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator + +#include "../Inputs/system-header-simulator.h" + +void *malloc(size_t size); +void free(void *memblock); + +#define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +void test_ambiguous_parameter(char *buf) { + if (gets(buf)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf'}} + // CHECK-FIXES: if (gets(buf)) +} + +void test_malloc(size_t size) { + char *buf1 = (char *)malloc(size); + if (gets(buf1)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf1'}} + // CHECK-FIXES: if (gets_s(buf1, size)) {} + free(buf1); +} + +void test_variable_array_ty(size_t size) { + char buf2[size]; + if (gets(buf2)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf2'}} + // CHECK-FIXES: if (gets_s(buf2, sizeof(buf2))) {} +} + +void test_constant_array_ty_offset(size_t size) { + char buf3[13]; + if (gets(buf3 + 1)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf3'}} + // CHECK-FIXES: if (gets_s(buf3 + 1, sizeof(buf3))) {} +} + +template <typename T, size_t size> +struct MyArray { + T data[size]; + + MyArray() { test_dependent_sized_array_ty(); } + + void test_dependent_sized_array_ty() { + if (gets(data)) {} + // expected-warning@-1 {{'gets' could write outside of 'data'}} + // CHECK-FIXES: if (gets_s(data, sizeof(data))) {} + } +}; + +void test_member_ty_offset() { + MyArray<char, 13> buf4; + if (gets(buf4.data + 1)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf4.data'}} + // CHECK-FIXES: if (gets_s(buf4.data + 1, sizeof(buf4.data))) {} +} + +void test_new(size_t size) { + char *buf5; + buf5 = new char[size]; + if (gets(buf5)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf5'}} + // CHECK-FIXES: if (gets_s(buf5, size)) {} + delete[] buf5; +} Index: clang/test/Analysis/analyzer-config.c =================================================================== --- clang/test/Analysis/analyzer-config.c +++ clang/test/Analysis/analyzer-config.c @@ -86,6 +86,7 @@ // CHECK-NEXT: prune-paths = true // CHECK-NEXT: region-store-small-struct-limit = 2 // CHECK-NEXT: report-in-main-source-file = false +// CHECK-NEXT: security.cert.Str:WantToUseSafeFunctions = true // CHECK-NEXT: serialize-stats = false // CHECK-NEXT: silence-checkers = "" // CHECK-NEXT: stable-report-filename = false @@ -98,4 +99,4 @@ // CHECK-NEXT: unroll-loops = false // CHECK-NEXT: widen-loops = false // CHECK-NEXT: [stats] -// CHECK-NEXT: num-entries = 95 +// CHECK-NEXT: num-entries = 96 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 @@ -18,11 +18,16 @@ extern FILE *__stdoutp; extern FILE *__stderrp; +typedef __SIZE_TYPE__ size_t; + int scanf(const char *restrict format, ...); int fscanf(FILE *restrict, const char *restrict, ...); int printf(const char *restrict format, ...); int fprintf(FILE *restrict, const char *restrict, ...); int getchar(void); +char *gets(char *buffer); +char *gets_s(char *buffer, size_t size); +char *fgets(char *str, int n, FILE *stream); // Note, on some platforms errno macro gets replaced with a function call. extern int errno; @@ -112,4 +117,4 @@ #define NULL __DARWIN_NULL #endif -#define offsetof(t, d) __builtin_offsetof(t, d) \ No newline at end of file +#define offsetof(t, d) __builtin_offsetof(t, d) Index: clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp +++ clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp @@ -9,13 +9,19 @@ #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h" // Common strings used for the "category" of many static analyzer issues. -namespace clang { namespace ento { namespace categories { +namespace clang { +namespace ento { +namespace categories { -const char * const CoreFoundationObjectiveC = "Core Foundation/Objective-C"; -const char * const LogicError = "Logic error"; -const char * const MemoryRefCount = - "Memory (Core Foundation/Objective-C/OSObject)"; -const char * const MemoryError = "Memory error"; -const char * const UnixAPI = "Unix API"; -const char * const CXXObjectLifecycle = "C++ object lifecycle"; -}}} +const char *const CoreFoundationObjectiveC = "Core Foundation/Objective-C"; +const char *const LogicError = "Logic error"; +const char *const MemoryRefCount = + "Memory (Core Foundation/Objective-C/OSObject)"; +const char *const MemoryError = "Memory error"; +const char *const UnixAPI = "Unix API"; +const char *const CXXObjectLifecycle = "C++ object lifecycle"; +const char *const SecurityError = "SecurityError"; + +} // namespace categories +} // namespace ento +} // namespace clang Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp @@ -0,0 +1,247 @@ +//===- CERTStrChecker - Checker for CERT STR rules --------------*- 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 CERTStrChecker which tries to find and resolve the CERT +// string handler function issues with fix-it hints. +// +// The rules can be found in section 'Rule 07. Characters and Strings (STR)': +// https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152038 +// +//===----------------------------------------------------------------------===// + +#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 "llvm/ADT/Optional.h" +#include <utility> + +using namespace clang; +using namespace ento; +using namespace ast_matchers; + +namespace { + +struct CallContext { + CallContext(Optional<unsigned> DestinationPos) + : DestinationPos(DestinationPos) {} + + Optional<unsigned> DestinationPos; +}; + +class CERTStrChecker : public Checker<eval::Call, check::BeginFunction> { + using StrCheck = std::function<void(const CERTStrChecker *, const CallEvent &, + const CallContext &, CheckerContext &)>; + +public: + bool evalCall(const CallEvent &Call, CheckerContext &C) const; + void checkBeginFunction(CheckerContext &C) const; + + std::unique_ptr<BugType> BT; + mutable bool UseSafeFunctions; + + const CallDescriptionMap<std::pair<StrCheck, CallContext>> CDM = { + // The following checks STR31-C rules. + // char *gets(char *dest); + {{"gets", 1}, {&CERTStrChecker::evalGets, {0}}}}; + + void evalGets(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; +}; +} // namespace + +//===----------------------------------------------------------------------===// +// Helper functions. +//===----------------------------------------------------------------------===// + +// Returns the proper token based end location of \p E. +static SourceLocation exprLocEnd(const Expr *E, CheckerContext &C) { + assert(E); + return Lexer::getLocForEndOfToken(E->getEndLoc(), /*Offset=*/0, + C.getSourceManager(), C.getLangOpts()); +} + +// Returns a string representation of \p E. +static std::string exprToStr(const Expr *E, CheckerContext &C) { + assert(E); + return Lexer::getSourceText( + CharSourceRange::getTokenRange(E->getSourceRange()), C.getSourceManager(), + C.getLangOpts()); +} + +// Returns the array part of \p E (if it is not ambiguous). +static const Expr *getArrayExpr(const Expr *E, CheckerContext &C) { + assert(E); + static constexpr llvm::StringLiteral ArrayName = "array"; + + auto DRE = declRefExpr().bind(ArrayName); + auto ME = memberExpr().bind(ArrayName); + + auto MatchArrays = + match(expr(anyOf(ME, DRE, hasDescendant(ME), hasDescendant(DRE))), *E, + C.getASTContext()); + + if (MatchArrays.size() == 1) + return MatchArrays[0].getNodeAs<Expr>(ArrayName); + + return nullptr; +} + +static Optional<std::string> getSizeExprAsString(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) { + SVal DestV = Call.getArgSVal(*CallC.DestinationPos); + const MemRegion *DestMR = DestV.getAsRegion(); + + // Arrays. + if (const auto *ER = dyn_cast<ElementRegion>(DestMR)) { + // Dependent-sized array type or a member array. + if (const auto *FR = dyn_cast<FieldRegion>(ER->getSuperRegion())) + if (FR->getDecl()->getType()->isArrayType()) + if (const Expr *ArrayExpr = + getArrayExpr(Call.getArgExpr(*CallC.DestinationPos), C)) + return "sizeof(" + exprToStr(ArrayExpr, C) + ")"; + + // Constant or variable array type. + if (const auto *VR = dyn_cast<VarRegion>(ER->getSuperRegion())) + if (const ValueDecl *VD = VR->getDecl()) + if (VD->getType()->isArrayType()) + return "sizeof(" + VD->getNameAsString() + ")"; + } + + // 'malloc()' family functions and new[]. + if (const SymbolicRegion *SR = DestMR->getSymbolicBase()) + if (const Expr *SizeExpr = getDynamicSizeExpr(C.getState(), SR)) + return exprToStr(SizeExpr, C); + + return None; +} + +std::unique_ptr<PathSensitiveBugReport> getReport(BugType &BT, + const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) { + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + Out << '\'' << Call.getCalleeIdentifier()->getName() + << "' could write outside of "; + + if (const Expr *DestArray = + getArrayExpr(Call.getArgExpr(*CallC.DestinationPos), C)) + Out << '\'' << exprToStr(DestArray, C) << '\''; + else + Out << "the array"; + + return std::make_unique<PathSensitiveBugReport>( + BT, Out.str(), C.generateNonFatalErrorNode()); +} + +//===----------------------------------------------------------------------===// +// Code injection functions. +//===----------------------------------------------------------------------===// + +static void renameFunctionFix(StringRef NewName, const CallEvent &Call, + PathSensitiveBugReport &Report) { + unsigned CallNameLength = Call.getCalleeIdentifier()->getLength(); + SourceLocation CallBegin = Call.getSourceRange().getBegin(); + SourceRange CallNameRange(CallBegin, + CallBegin.getLocWithOffset(CallNameLength - 1)); + + const auto FuncNameFix = FixItHint::CreateReplacement(CallNameRange, NewName); + Report.addFixItHint(FuncNameFix); +} + +//===----------------------------------------------------------------------===// +// Evaluating problematic function calls. +//===----------------------------------------------------------------------===// + +void CERTStrChecker::evalGets(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const { + unsigned DestPos = *CallC.DestinationPos; + const Expr *DestArg = Call.getArgExpr(DestPos); + const Expr *DestArray = getArrayExpr(DestArg, C); + + auto Report = getReport(*BT, Call, CallC, C); + + // Do not try to fix in-parameter buffers. + bool IsFix = true; + if (DestArray) { + if (const auto *DRE = dyn_cast<DeclRefExpr>(DestArray->IgnoreImpCasts())) + IsFix = !isa<ParmVarDecl>(DRE->getDecl()); + } + + if (IsFix) { + if (Optional<std::string> SizeStr = getSizeExprAsString(Call, CallC, C)) { + renameFunctionFix(UseSafeFunctions ? "gets_s" : "fgets", Call, *Report); + + std::string ArgumentFix = ", " + *SizeStr; + if (!UseSafeFunctions) + ArgumentFix += ", stdin"; + + auto CallFix = + FixItHint::CreateInsertion(exprLocEnd(DestArg, C), ArgumentFix); + Report->addFixItHint(CallFix); + } + } + + C.emitReport(std::move(Report)); +} + +//===----------------------------------------------------------------------===// +// Main logic to evaluate a call. +//===----------------------------------------------------------------------===// + +bool CERTStrChecker::evalCall(const CallEvent &Call, CheckerContext &C) const { + if (const auto *Lookup = CDM.lookup(Call)) { + const StrCheck &Check = Lookup->first; + Check(this, Call, Lookup->second, C); + } + + return false; +} + +void CERTStrChecker::checkBeginFunction(CheckerContext &C) const { + if (!C.inTopFrame()) + return; + + if (!UseSafeFunctions) + return; + + Preprocessor &PP = C.getPreprocessor(); + if (PP.isMacroDefined("__STDC_LIB_EXT1__")) { + MacroDefinition MD = PP.getMacroDefinition("__STDC_WANT_LIB_EXT1__"); + if (const MacroInfo *MI = MD.getMacroInfo()) { + const Token &T = MI->tokens().back(); + StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength()); + llvm::APInt IntValue; + ValueStr.getAsInteger(10, IntValue); + UseSafeFunctions = IntValue.getZExtValue(); + return; + } + } + + UseSafeFunctions = false; +} + +void ento::registerCERTStrChecker(CheckerManager &Mgr) { + auto *Checker = Mgr.registerChecker<CERTStrChecker>(); + + Checker->UseSafeFunctions = Mgr.getAnalyzerOptions().getCheckerBooleanOption( + Checker, "WantToUseSafeFunctions"); + + Checker->BT = std::make_unique<BugType>( + Mgr.getCurrentCheckerName(), "Insecure string handler function call", + categories::SecurityError); +} + +bool ento::shouldRegisterCERTStrChecker(const LangOptions &) { return true; } Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -17,6 +17,7 @@ CastSizeChecker.cpp CastToStructChecker.cpp CastValueChecker.cpp + cert/StrChecker.cpp CheckObjCDealloc.cpp CheckObjCInstMethSignature.cpp CheckSecuritySyntaxOnly.cpp Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h +++ clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h @@ -11,16 +11,17 @@ // Common strings used for the "category" of many static analyzer issues. namespace clang { - namespace ento { - namespace categories { - extern const char * const CoreFoundationObjectiveC; - extern const char * const LogicError; - extern const char * const MemoryRefCount; - extern const char * const MemoryError; - extern const char * const UnixAPI; - extern const char * const CXXObjectLifecycle; - } - } -} -#endif +namespace ento { +namespace categories { +extern const char *const CoreFoundationObjectiveC; +extern const char *const LogicError; +extern const char *const MemoryRefCount; +extern const char *const MemoryError; +extern const char *const UnixAPI; +extern const char *const CXXObjectLifecycle; +extern const char *const SecurityError; +} // namespace categories +} // namespace ento +} // namespace clang +#endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_COMMONBUGCATEGORIES_H Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -67,6 +67,7 @@ def Performance : Package<"performance">, ParentPackage<OptIn>; def Security : Package <"security">; +def CERT : Package<"cert">, ParentPackage<Security>; def InsecureAPI : Package<"insecureAPI">, ParentPackage<Security>; def SecurityAlpha : Package<"security">, ParentPackage<Alpha>; def Taint : Package<"taint">, ParentPackage<SecurityAlpha>; @@ -785,6 +786,24 @@ } // end "security" +let ParentPackage = CERT in { + +def CERTStrChecker : Checker<"Str">, + HelpText<"CERT checker of string related rules.">, + CheckerOptions<[ + CmdLineOption<Boolean, + "WantToUseSafeFunctions", + "An integer non-zero value specifying if the target " + "environment is considered to implement '_s' suffixed memory " + "and string handler functions which are safer than older " + "versions (e.g. 'memcpy_s()')", + "true", + Released>, + ]>, + Documentation<NotDocumented>; + +} // end "CERT" + let ParentPackage = SecurityAlpha in { def ArrayBoundChecker : Checker<"ArrayBound">, Index: clang/include/clang/Lex/Preprocessor.h =================================================================== --- clang/include/clang/Lex/Preprocessor.h +++ clang/include/clang/Lex/Preprocessor.h @@ -1032,6 +1032,9 @@ return MD && MD->isDefined(); } + MacroDefinition getMacroDefinition(StringRef Id) { + return getMacroDefinition(&Identifiers.get(Id)); + } MacroDefinition getMacroDefinition(const IdentifierInfo *II) { if (!II->hasMacroDefinition()) return {};
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
