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 +""""""""""""""""""""""""""" + +SEI CERT checker of `STR31-C rule`_. + +It warns on misusing the following functions: +``strcpy()``, ``gets()``, ``fscanf()``, ``sprintf()``. + .. _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 @@ -73,6 +73,7 @@ def CERT : Package<"cert">, ParentPackage; def POS : Package<"pos">, ParentPackage; +def STR : Package<"str">, ParentPackage; def Unix : Package<"unix">; def UnixAlpha : Package<"unix">, ParentPackage; @@ -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, + Hidden; + +def Str31cChecker : Checker<"31c">, + HelpText<"SEI CERT checker of rules defined in STR31-C">, + Dependencies<[StrCheckerBase]>, + Documentation; + +} // end "alpha.cert.str" + let ParentPackage = SecurityAlpha in { def ArrayBoundChecker : Checker<"ArrayBound">, 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 getInnerPointerBRVisitor(SymbolRef Sym); +std::unique_ptr 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/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 CheckPlacementNew.cpp 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 getMallocBRVisitor(SymbolRef Sym) { + return std::make_unique(Sym); +} + ProgramStateRef markReleased(ProgramStateRef State, SymbolRef Sym, const Expr *Origin) { AllocationFamily Family = AF_InnerBuffer; Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp @@ -0,0 +1,272 @@ +//===- 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/pages/viewpage.action?pageId=87152038 +// +// This checker is a base checker which consist of the following checkers: +// - '31c' +// 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 "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" +#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 + +using namespace clang; +using namespace ento; +using namespace ast_matchers; + +namespace { + +struct CallContext { + CallContext(Optional DestinationPos, + Optional SourcePos = None) + : DestinationPos(DestinationPos), SourcePos(SourcePos) {} + + Optional DestinationPos; + Optional SourcePos; +}; + +class StrCheckerBase : public Checker { + using StrCheck = std::function; + +public: + // We report a note when any of the calls in 'CDM' is being used because + // they can cause a not null-terminated string. + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + + 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; + + void createOverflowReport(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; + + bool EnableStr31cChecker = false; + +private: + const CallDescriptionMap> 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 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()) + .str(); +} + +// Returns the appropriate region of \p V. +static const MemRegion *getRegion(SVal V) { + const MemRegion *MR = V.getAsRegion(); + if (!MR) + return nullptr; + + return MR->getBaseRegion(); +} + +// Tries to obtain a pretty-printable name of \p E. +Optional getName(const Expr *E, CheckerContext &C) { + if (!E) + return None; + + static constexpr llvm::StringLiteral Name = "name"; + + auto DRE = declRefExpr().bind(Name); + auto ME = memberExpr().bind(Name); + + auto Matches = + match(expr(anyOf(ME, DRE, hasDescendant(ME), hasDescendant(DRE))), *E, + C.getASTContext()); + + if (Matches.size() == 1) + return '\'' + exprToStr(Matches[0].getNodeAs(Name), C) + '\''; + + return None; +} + +void StrCheckerBase::createOverflowReport(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(); + Optional ArrayName = getName(Call.getArgExpr(DestPos), C); + + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + Out << '\'' << CallName << "' could write outside of " + << (ArrayName ? *ArrayName : "the array"); + std::string ReportMsg = Out.str().str(); + + const Expr *Arg = Call.getArgExpr(DestPos); + + auto Report = std::make_unique(BT, ReportMsg, + C.generateErrorNode()); + Report->addRange(Arg->getSourceRange()); + + // Track the allocation. + const auto *VR = MR->getAs(); + if (VR && VR->getValueType()->getAsArrayTypeUnsafe()) { + bugreporter::trackExpressionValue(Report->getErrorNode(), Arg, *Report); + } else if (const SymbolRef Sym = DestV.getAsSymbol()) { + Report->addVisitor(allocation_state::getMallocBRVisitor(Sym)); + } + + C.emitReport(std::move(Report)); +} + +//===----------------------------------------------------------------------===// +// Evaluating problematic function calls. +//===----------------------------------------------------------------------===// + +void StrCheckerBase::checkGets(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr31cChecker) + createOverflowReport(Call, CallC, C); +} + +void StrCheckerBase::checkSprintf(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr31cChecker) + createOverflowReport(Call, CallC, C); +} + +void StrCheckerBase::checkFscanf(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (!EnableStr31cChecker) + return; + + const auto *FormatExpr = + dyn_cast(Call.getArgExpr(1)->IgnoreImpCasts()); + if (!FormatExpr) + return; + + // FIXME: Handle multiple buffers. + if (FormatExpr->getString() != "%s") + return; + + createOverflowReport(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 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: Use the 'SValVisitor' to catch every such constructs of the symbol. + // FIXME: We cannot catch every '+ integer' part at the moment so we do not + // check that property for now. + if (const SymExpr *SE = DestSize.getAsSymExpr()) + if (const auto *SIE = dyn_cast(SE)) + if (SIE->getOpcode() == BO_Add) + if (const auto *SM = dyn_cast(SIE->getLHS())) + if (SM->getRegion() == SrcMR) + 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; + + createOverflowReport(Call, CallC, C); +} + +//===----------------------------------------------------------------------===// +// Main logic to check a call. +//===----------------------------------------------------------------------===// + +void StrCheckerBase::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + const auto *Lookup = CDM.lookup(Call); + if (!Lookup) + return; + + const StrCheck &Check = Lookup->first; + Check(this, Call, Lookup->second, C); + return; +} + +void ento::registerStrCheckerBase(CheckerManager &Mgr) { + Mgr.registerChecker(); +} + +bool ento::shouldRegisterStrCheckerBase(const CheckerManager &) { return true; } + +#define REGISTER_CHECKER(Name) \ + void ento::register##Name(CheckerManager &Mgr) { \ + auto *Checker = Mgr.getChecker(); \ + Checker->Enable##Name = true; \ + } \ + \ + bool ento::shouldRegister##Name(const CheckerManager &) { return true; } + +REGISTER_CHECKER(Str31cChecker) 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,22 +18,32 @@ 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); +int sprintf(char *buffer, const char *format, ...); +int snprintf(char *buffer, size_t count, const char *format, ...); +int _snprintf_s(char *buffer, size_t sizeOfBuffer, size_t count, const char *format, ...); +int fscanf(FILE *stream, const char *format, ...); +int fscanf_s(FILE *stream, const char *format, ...); +char *getenv(const char *varname); // Note, on some platforms errno macro gets replaced with a function call. extern int errno; - -typedef __typeof(sizeof(int)) size_t; +typedef int errno_t; size_t strlen(const char *); +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); -char *strcpy(char *restrict, const char *restrict); -char *strncpy(char *dst, const char *src, size_t n); -void *memcpy(void *dst, const void *src, size_t n); +void *memcpy(void *dest, const void *src, size_t count); typedef unsigned long __darwin_pthread_key_t; typedef __darwin_pthread_key_t pthread_key_t; @@ -118,4 +128,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/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/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 EOF -1 + +void free(void *memblock); +void *malloc(size_t size); + +// FIXME: The following tests could fail if the destination array has an offset. + +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' is missing so there is not enough space for the null terminator. + dest = (char *)malloc(size * 2); + + strcpy(dest, &src[13]); + // expected-warning@-1 {{'strcpy' could write outside of 'dest'}} + free(dest); +} + +void test_char_by_char(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/cert/str31-c-notes.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str31-c-notes.cpp @@ -0,0 +1,64 @@ +// 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/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); + +void do_something(char *buffer); + +void test_simple_size(unsigned size) { + size = 13; + + char *buf = (char *)malloc(size); + // expected-note@-1 {{Memory is allocated}} + + 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_size_redefinition() { + unsigned size = 13; + + char *buf = (char *)malloc(size + 1); + // expected-note@-1 {{Memory is allocated}} + + size = 42; + + if (gets(buf)) { + // expected-note@-1 {{'gets' could write outside of 'buf'}} + // expected-warning@-2 {{'gets' could write outside of 'buf'}} + } + + char *p = buf; + free(p); +} + +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}} + 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.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str31-c.cpp @@ -0,0 +1,185 @@ +// 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/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 EOF -1 +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