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.
This checker warns on most of the following rules: https://wiki.sei.cmu.edu/confluence/display/c/STR32-C.+Do+not+pass+a+non-null-terminated+character+sequence+to+a+library+function+that+expects+a+string Repository: rC Clang https://reviews.llvm.org/D71033 Files: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp clang/lib/StaticAnalyzer/Core/DynamicSize.cpp clang/test/Analysis/Inputs/system-header-simulator.h clang/test/Analysis/cert/str32-c-notes.cpp clang/test/Analysis/cert/str32-c.cpp clang/test/Analysis/malloc.c
Index: clang/test/Analysis/malloc.c =================================================================== --- clang/test/Analysis/malloc.c +++ clang/test/Analysis/malloc.c @@ -9,21 +9,6 @@ void clang_analyzer_eval(int); -// Without -fms-compatibility, wchar_t isn't a builtin type. MSVC defines -// _WCHAR_T_DEFINED if wchar_t is available. Microsoft recommends that you use -// the builtin type: "Using the typedef version can cause portability -// problems", but we're ok here because we're not actually running anything. -// Also of note is this cryptic warning: "The wchar_t type is not supported -// when you compile C code". -// -// See the docs for more: -// https://msdn.microsoft.com/en-us/library/dh8che7s.aspx -#if !defined(_WCHAR_T_DEFINED) -// "Microsoft implements wchar_t as a two-byte unsigned value" -typedef unsigned short wchar_t; -#define _WCHAR_T_DEFINED -#endif // !defined(_WCHAR_T_DEFINED) - typedef __typeof(sizeof(int)) size_t; void *malloc(size_t); void *alloca(size_t); Index: clang/test/Analysis/cert/str32-c.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str32-c.cpp @@ -0,0 +1,88 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,unix,security.cert.str.32.c \ +// RUN: -verify %s + +// See the examples on the page of STR32-C: +// https://wiki.sei.cmu.edu/confluence/display/c/STR32-C.+Do+not+pass+a+non-null-terminated+character+sequence+to+a+library+function+that+expects+a+string + +#include "../Inputs/system-header-simulator.h" + +void *realloc(void *memblock, size_t size); + +namespace test_strncpy_bad { +enum { STR_SIZE = 32 }; + +size_t func(const char *source) { + char c_str[STR_SIZE]; + size_t ret = 0; + + if (source) { + c_str[sizeof(c_str) - 1] = '\0'; + strncpy(c_str, source, sizeof(c_str)); + ret = strlen(c_str); + // expected-warning@-1 {{'c_str' is not null-terminated}} + } + return ret; +} +} // namespace test_strncpy_bad + +namespace test_strncpy_good { +enum { STR_SIZE = 32 }; + +size_t func(const char *src) { + char c_str[STR_SIZE]; + size_t ret = 0; + + if (src) { + strncpy(c_str, src, sizeof(c_str) - 1); + c_str[sizeof(c_str) - 1] = '\0'; + ret = strlen(c_str); + } + return ret; +} +} // namespace test_strncpy_good + +namespace test_realloc_bad { +wchar_t *cur_msg = NULL; +size_t cur_msg_size = 1024; +size_t cur_msg_len = 0; + +void lessen_memory_usage(void) { + if (cur_msg == NULL) + return; + + size_t temp_size = cur_msg_size / 2 + 1; + wchar_t *temp = (wchar_t *)realloc(cur_msg, temp_size * sizeof(wchar_t)); + /* temp and cur_msg may no longer be null-terminated */ + if (temp == NULL) + return; + + cur_msg = temp; + cur_msg_size = temp_size; + cur_msg_len = wcslen(cur_msg); + // expected-warning@-1 {{'cur_msg' is not null-terminated}} +} +} // namespace test_realloc_bad + +namespace test_realloc_good { +wchar_t *cur_msg = NULL; +size_t cur_msg_size = 1024; +size_t cur_msg_len = 0; + +void lessen_memory_usage(void) { + if (cur_msg == NULL) + return; + + size_t temp_size = cur_msg_size / 2 + 1; + wchar_t *temp = (wchar_t *)realloc(cur_msg, temp_size * sizeof(wchar_t)); + /* temp and cur_msg may no longer be null-terminated */ + if (temp == NULL) + return; + + cur_msg = temp; + /* Properly null-terminate cur_msg */ + cur_msg[temp_size - 1] = L'\0'; + cur_msg_size = temp_size; + cur_msg_len = wcslen(cur_msg); +} +} // namespace test_realloc_good Index: clang/test/Analysis/cert/str32-c-notes.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str32-c-notes.cpp @@ -0,0 +1,66 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,unix,security.cert.str.32.c \ +// RUN: -analyzer-output=text -verify %s + +// See the examples on the page of STR32-C: +// https://wiki.sei.cmu.edu/confluence/display/c/STR32-C.+Do+not+pass+a+non-null-terminated+character+sequence+to+a+library+function+that+expects+a+string + +#include "../Inputs/system-header-simulator.h" + +void *realloc(void *memblock, size_t size); + +namespace test_strncpy_bad { +enum { STR_SIZE = 32 }; + +size_t func(const char *source) { + char c_str[STR_SIZE]; + // expected-note@-1 {{'c_str' initialized here}} + size_t ret = 0; + + if (source) { + // expected-note@-1 {{Assuming 'source' is non-null}} + // expected-note@-2 {{Taking true branch}} + + c_str[sizeof(c_str) - 1] = '\0'; + strncpy(c_str, source, sizeof(c_str)); + // expected-note@-1 {{'strncpy' could write outside of 'c_str'}} + + ret = strlen(c_str); + // expected-note@-1 {{'c_str' is not null-terminated}} + // expected-warning@-2 {{'c_str' is not null-terminated}} + } + return ret; +} +} // namespace test_strncpy_bad + +namespace test_realloc_bad { +wchar_t *cur_msg = NULL; +size_t cur_msg_size = 1024; +size_t cur_msg_len = 0; + +void lessen_memory_usage(void) { + if (cur_msg == NULL) { + // expected-note@-1 {{Assuming 'cur_msg' is not equal to NULL}} + // expected-note@-2 {{Taking false branch}} + return; + } + + size_t temp_size = cur_msg_size / 2 + 1; + wchar_t *temp = (wchar_t *)realloc(cur_msg, temp_size * sizeof(wchar_t)); + // expected-note@-1 {{Memory is allocated}} + // expected-note@-2 {{'temp' is allocated without considering the length of the string it will store, therefore it could overflow}} + + /* temp and cur_msg may no longer be null-terminated */ + if (temp == NULL) { + // expected-note@-1 {{Assuming 'temp' is not equal to NULL}} + // expected-note@-2 {{Taking false branch}} + return; + } + + cur_msg = temp; // no-warning: rebinding + cur_msg_size = temp_size; + cur_msg_len = wcslen(cur_msg); + // expected-note@-1 {{'cur_msg' is not null-terminated}} + // expected-warning@-2 {{'cur_msg' is not null-terminated}} +} +} // namespace test_realloc_bad 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 @@ -38,12 +38,29 @@ extern int errno; typedef int errno_t; -size_t strlen(const char *); +void *memcpy(void *dest, const void *src, size_t count); + +size_t strlen(const char *str); char *strcpy(char *dest, const char *src); errno_t strcpy_s(char *dest, size_t destSize, const char *src); char *strncpy(char *dest, const char *src, size_t count); -void *memcpy(void *dest, const void *src, size_t count); +// Without -fms-compatibility, wchar_t isn't a builtin type. MSVC defines +// _WCHAR_T_DEFINED if wchar_t is available. Microsoft recommends that you use +// the builtin type: "Using the typedef version can cause portability +// problems", but we're ok here because we're not actually running anything. +// Also of note is this cryptic warning: "The wchar_t type is not supported +// when you compile C code". +// +// See the docs for more: +// https://msdn.microsoft.com/en-us/library/dh8che7s.aspx +#if !defined(_WCHAR_T_DEFINED) && !__cplusplus +// "Microsoft implements wchar_t as a two-byte unsigned value" +typedef unsigned short wchar_t; +#define _WCHAR_T_DEFINED +#endif + +size_t wcslen(const wchar_t *str); typedef unsigned long __darwin_pthread_key_t; typedef __darwin_pthread_key_t pthread_key_t; Index: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/DynamicSize.cpp +++ clang/lib/StaticAnalyzer/Core/DynamicSize.cpp @@ -40,6 +40,19 @@ return MR->getMemRegionManager().getStaticSize(MR, SVB); } +Optional<DynamicSizeInfo> getDynamicSizeInfo(ProgramStateRef State, + const MemRegion *MR, + SValBuilder &SVB) { + if (const DynamicSizeInfo *SizeInfo = State->get<DynamicSizeMap>(MR)) + return *SizeInfo; + + if (const auto *TVR = MR->getAs<TypedValueRegion>()) + if (TVR->getValueType()->getAsArrayTypeUnsafe()) + return DynamicSizeInfo(MR->getMemRegionManager().getStaticSize(MR, SVB)); + + return None; +} + const Expr *getDynamicSizeExpr(ProgramStateRef State, const MemRegion *MR) { if (const DynamicSizeInfo *SizeInfo = State->get<DynamicSizeMap>(MR)) return SizeInfo->getSizeExpr(); Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp @@ -15,6 +15,9 @@ // - '31.c' // 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 // +// - '32.c' +// https://wiki.sei.cmu.edu/confluence/display/c/STR32-C.+Do+not+pass+a+non-null-terminated+character+sequence+to+a+library+function+that+expects+a+string +// //===----------------------------------------------------------------------===// #include "AllocationState.h" @@ -39,11 +42,14 @@ struct CallContext { CallContext(Optional<unsigned> DestinationPos, - Optional<unsigned> SourcePos = None) - : DestinationPos(DestinationPos), SourcePos(SourcePos) {} + Optional<unsigned> SourcePos = None, + Optional<unsigned> LengthPos = None) + : DestinationPos(DestinationPos), SourcePos(SourcePos), + LengthPos(LengthPos) {} Optional<unsigned> DestinationPos; Optional<unsigned> SourcePos; + Optional<unsigned> LengthPos; }; class StrCheckerBase : public Checker<check::PostCall, check::Bind> { @@ -72,9 +78,12 @@ CheckerContext &C) const; void checkStrcpy(const CallEvent &Call, const CallContext &CallC, CheckerContext &C) const; + void checkStrncpy(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; std::unique_ptr<BugType> BT; - bool EnableStr31cChecker = false; + + bool EnableStr31cChecker = false, EnableStr32cChecker = false; private: const CallDescriptionMap<std::pair<StrCheck, CallContext>> CDM = { @@ -86,13 +95,20 @@ // 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}}}}; + {{"strcpy", 2}, {&StrCheckerBase::checkStrcpy, {0, 1}}}, + + // The following checks STR32-C rules. + // char *strncpy(char *dest, const char *src, size_t count) + {{"strncpy", 3}, {&StrCheckerBase::checkStrncpy, {0, 1, 2}}}}; }; } // namespace // It stores whether the string is null-terminated. REGISTER_MAP_WITH_PROGRAMSTATE(NullTerminationMap, const MemRegion *, bool) +// It stores the allocations which we emit notes on. +REGISTER_LIST_WITH_PROGRAMSTATE(AllocationList, const MemRegion *) + //===----------------------------------------------------------------------===// // Helper functions. //===----------------------------------------------------------------------===// @@ -146,14 +162,15 @@ ProgramStateRef State = C.getState(); std::string CallName = Call.getCalleeIdentifier()->getName(); - Optional<std::string> ArrayName = getName(Call.getArgExpr(DestPos), C); + std::string ArrayName = "the array"; + if (Optional<std::string> TmpArrayName = getName(Call.getArgExpr(DestPos), C)) + ArrayName = *TmpArrayName; const NoteTag *Tag = C.getNoteTag( [=]() -> std::string { SmallString<128> Msg; llvm::raw_svector_ostream Out(Msg); - Out << '\'' << CallName << "' could write outside of " - << (ArrayName ? *ArrayName : "the array"); + Out << '\'' << CallName << "' could write outside of " << ArrayName; return Out.str(); }, @@ -164,6 +181,46 @@ C.addTransition(State, C.getPredecessor(), Tag); } +static bool isWrongAllocation(SVal DestV, SVal SrcV, CheckerContext &C) { + ProgramStateRef State = C.getState(); + SValBuilder &SVB = C.getSValBuilder(); + + // Check the size of the allocation to prevent false alarms. + const MemRegion *SrcMR = getRegion(SrcV); + const MemRegion *DestMR = getRegion(DestV); + if (!SrcMR || !DestMR) + return false; + + DefinedOrUnknownSVal SrcSize = getDynamicSize(State, SrcMR, SVB); + DefinedOrUnknownSVal DestSize = getDynamicSize(State, DestMR, SVB); + + // 'strlen(src) + integer' is most likely fine. + // FIXME: We cannot catch every '+ integer' part at the moment so we do not + // check that property for now. + SValHasDescendant HasDescendantSrc(SrcMR); + if (HasDescendantSrc.Visit(DestSize)) + return false; + + if (const MemRegion *DestSizeR = DestSize.getAsRegion()) + if (DestSizeR == SrcMR) + return false; + + // 'StringRegion' returns the size with the null-terminator. + if (const llvm::APSInt *SrcSizeInt = SVB.getKnownValue(State, SrcSize)) + if (const llvm::APSInt *DestSizeInt = SVB.getKnownValue(State, DestSize)) + if (SrcSizeInt->getZExtValue() <= DestSizeInt->getZExtValue()) + return false; + + return true; +} + +static bool isWrongAllocation(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) { + SVal DestV = Call.getArgSVal(*CallC.DestinationPos); + SVal SrcV = Call.getArgSVal(*CallC.SourcePos); + return isWrongAllocation(DestV, SrcV, C); +} + //===----------------------------------------------------------------------===// // Evaluating problematic function calls. //===----------------------------------------------------------------------===// @@ -202,37 +259,15 @@ 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 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); - - // 'strlen(src) + integer' is most likely fine. - // FIXME: We cannot catch every '+ integer' part at the moment so we do not - // check that property for now. - SValHasDescendant HasDescendantSrc(SrcMR); - if (HasDescendantSrc.Visit(DestSize)) - return; - - // 'StringRegion' returns the size with the null-terminator. - if (const llvm::APSInt *SrcSizeInt = SVB.getKnownValue(State, SrcSize)) - if (const llvm::APSInt *DestSizeInt = SVB.getKnownValue(State, DestSize)) - if (SrcSizeInt->getZExtValue() <= DestSizeInt->getZExtValue()) - return; + if (EnableStr31cChecker && isWrongAllocation(Call, CallC, C)) + createOverflowNote(Call, CallC, C); +} - createOverflowNote(Call, CallC, C); +void StrCheckerBase::checkStrncpy(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr32cChecker) + createOverflowNote(Call, CallC, C); } //===----------------------------------------------------------------------===// @@ -286,15 +321,79 @@ } } -void StrCheckerBase::checkBind(SVal L, SVal V, const Stmt *S, - CheckerContext &C) const { +static bool checkAllocation(SVal L, SVal V, const Stmt *S, CheckerContext &C, + BugType &BT) { + const MemRegion *LocMR = getRegion(L); + const MemRegion *NonLocMR = getRegion(V); + if (!LocMR || !NonLocMR) + return false; + + // A C-string is null-terminated. + if (isa<StringRegion>(NonLocMR)) + return false; + + // Only check for character allocations. + QualType Ty; + if (const auto *LocTVR = LocMR->getAs<TypedValueRegion>()) { + Ty = LocTVR->getValueType(); + if (!Ty.isNull() && Ty->isPointerType()) + Ty = Ty->getPointeeType(); + } + + if (Ty.isNull() || !Ty->isAnyCharacterType()) + return false; + + // See whether we have made a report so it prevent reports on rebindings. ProgramStateRef State = C.getState(); + if (State->get<AllocationList>().contains(NonLocMR)) + return false; + + // Check for allocations we model. + Optional<DynamicSizeInfo> SizeInfo = + getDynamicSizeInfo(State, NonLocMR, C.getSValBuilder()); + if (!SizeInfo) + return false; + + // 'strlen(something) + something' is most likely fine. + // FIXME: Use the 'SValVisitor' to catch every such constructs of the symbol. + if (const SymExpr *SizeSym = SizeInfo->getSize().getAsSymExpr()) + for (const SymExpr *Sym : SizeSym->symbols()) + if (const auto *SIE = dyn_cast<SymIntExpr>(Sym)) + if (SIE->getOpcode() == BO_Add) + if (const auto *SM = dyn_cast<SymbolMetadata>(SIE->getLHS())) + return true; + + std::string ArrayName = "The array"; + if (const auto *VR = LocMR->getAs<VarRegion>()) + if (const VarDecl *VD = VR->getDecl()) + ArrayName = '\'' + VD->getNameAsString() + '\''; - // Check for a hand-written null-termination, e.g: 'c_string[length] = '\0';'. - const MemRegion *MR = getRegion(L); - if (!MR) - return; + const NoteTag *Tag = C.getNoteTag( + [=]() -> std::string { + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + Out << ArrayName + << " is allocated without considering the length of the string it " + "will store, therefore it could overflow"; + return Out.str(); + }, + /*IsPrunable=*/false); + + // Mark the string could be not null-terminated. + State = State->set<NullTerminationMap>(NonLocMR, false); + State = State->add<AllocationList>(NonLocMR); + C.addTransition(State, C.getPredecessor(), Tag); + return true; +} + +void StrCheckerBase::checkBind(SVal L, SVal V, const Stmt *S, + CheckerContext &C) const { + if (EnableStr32cChecker) + if (checkAllocation(L, V, S, C, *BT)) + return; + + ProgramStateRef State = C.getState(); bool IsNullTermination = false; if (const llvm::APInt *Int = C.getSValBuilder().getKnownValue(State, V)) IsNullTermination = Int->isNullValue(); @@ -303,8 +402,10 @@ return; // Mark the string null-terminated. - State = State->set<NullTerminationMap>(MR, true); - C.addTransition(State, C.getPredecessor()); + if (const auto *MR = getRegion(L)) { + State = State->set<NullTerminationMap>(MR, true); + C.addTransition(State, C.getPredecessor()); + } } void ento::registerStrCheckerBase(CheckerManager &Mgr) { @@ -326,3 +427,4 @@ bool ento::shouldRegister##Name(const LangOptions &LO) { return true; } REGISTER_CHECKER(Str31cChecker) +REGISTER_CHECKER(Str32cChecker) Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h +++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h @@ -13,11 +13,13 @@ #ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_DYNAMICSIZE_H #define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_DYNAMICSIZE_H +#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSizeInfo.h" #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h" +#include "llvm/ADT/Optional.h" namespace clang { namespace ento { @@ -26,6 +28,12 @@ DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR, SValBuilder &SVB); +/// \returns The stored dynamic size information or the array's static size +/// information for the region \p MR. +Optional<DynamicSizeInfo> getDynamicSizeInfo(ProgramStateRef State, + const MemRegion *MR, + SValBuilder &SVB); + /// \returns The stored dynamic size expression for the region \p MR. const Expr *getDynamicSizeExpr(ProgramStateRef State, const MemRegion *MR); Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -800,6 +800,11 @@ Dependencies<[StrCheckerBase]>, Documentation<HasDocumentation>; +def Str32cChecker : Checker<"32.c">, + HelpText<"SEI CERT checker of rules defined in STR32-C">, + Dependencies<[StrCheckerBase]>, + Documentation<HasDocumentation>; + } // end "cert.str" let ParentPackage = SecurityAlpha in {
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
