Index: clang/docs/analyzer/checkers.rst =================================================================== --- clang/docs/analyzer/checkers.rst +++ clang/docs/analyzer/checkers.rst @@ -145,6 +145,39 @@ x = &y; // warn } +.. _core-StoreToImmutable: + +core.StoreToImmutable (C, C++) +""""""""""""""""""""""""""""""""""""""" +Check for writes to immutable memory. + +.. code-block:: cpp + + void write_to_envvar() { + char *p = getenv("VAR"); + if (!p) + return; + p[0] = '\0'; // warn + } + +Note that this example depends on :ref:`_alpha-security-cert-env-ModelConstQualifiedReturn` +checker that models return value of getenv as const-qualified, however there are other +scenarios where such immutable memory regions arise. + +To fix the error, user can add `const` as preventive measure and make a copy of the +immutable memory region to a mutable location and do the mutation there instead. + +.. code-block:: cpp + + void write_to_envvar() { + const char *p = getenv("VAR"); + if (!p) + return; + + copy_of_p = (char *)malloc(strlen(p) + 1); + strcpy(copy_of_p, p); + copy_of_p[0] = '\0'; + } .. _core-UndefinedBinaryOperatorResult: @@ -2340,6 +2373,32 @@ // dereferencing invalid pointer } +.. _alpha-security-cert-env-ModelConstQualifiedReturn: + +alpha.security.cert.env.ModelConstQualifiedReturn +""""""""""""""""""""""""""""""""""""""""""""""""" + +Corresponds to SEI CERT Rules ENV30-C. + +Some functions return a pointer to an object that cannot be +modified without causing undefined behavior. In such cases, +the function call results must be treated as being const-qualified. + +These functions include ``getenv(), setlocale(), localeconv(), asctime(), strerror()``. + +Checker models return values of these functions as const +qualified. Their modification is checked in :ref:`_core-StoreToImmutable`. + +.. code-block:: c + + void writing_to_envvar() { + char *p = getenv("VAR"); // note: getenv return value + // should be treated as const + if (!p) + return; + p[0] = '\0'; // warn + } + alpha.security.taint ^^^^^^^^^^^^^^^^^^^^ Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -227,6 +227,10 @@ Dependencies<[StackAddrEscapeBase]>, Documentation; +def StoreToImmutableChecker : Checker<"StoreToImmutable">, + HelpText<"Check for writes to immutable memory.">, + Documentation; + def DynamicTypePropagation : Checker<"DynamicTypePropagation">, HelpText<"Generate dynamic type information">, Documentation, @@ -979,6 +983,12 @@ HelpText<"Finds usages of possibly invalidated pointers">, Documentation; + def ModelConstQualifiedReturnChecker : Checker<"ModelConstQualifiedReturn">, + HelpText<"Model return values of some functions as const-qualified. " + "StoreToImmutable checker would warn on their modification.">, + Documentation; + + } // end "alpha.cert.env" let ParentPackage = SecurityAlpha in { Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h +++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h @@ -764,8 +764,8 @@ assert(s->getType()->isAnyPointerType() || s->getType()->isReferenceType() || s->getType()->isBlockPointerType()); - assert(isa(sreg) || isa(sreg) || - isa(sreg)); + assert((isa(sreg))); } public: Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -66,6 +66,7 @@ MismatchedIteratorChecker.cpp MmapWriteExecChecker.cpp MIGChecker.cpp + cert/ModelConstQualifiedReturnChecker.cpp MoveChecker.cpp MPI-Checker/MPIBugReporter.cpp MPI-Checker/MPIChecker.cpp @@ -106,6 +107,7 @@ StackAddrEscapeChecker.cpp StdLibraryFunctionsChecker.cpp STLAlgorithmModeling.cpp + StoreToImmutableChecker.cpp StreamChecker.cpp StringChecker.cpp Taint.cpp Index: clang/lib/StaticAnalyzer/Checkers/StoreToImmutable.h =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/StoreToImmutable.h @@ -0,0 +1,29 @@ +//=== StoreToImmutable.h - Expose ImmutableMemoryBind BugType ------*- 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 +// +//===----------------------------------------------------------------------===// +// +// Defines getter function for ImmutableMemoryBind BugType +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_STORETOIMMUTABLE_H +#define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_STORETOIMMUTABLE_H + + +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" + +namespace clang { +namespace ento { +namespace immutablestore { + +const BugType *getImmutableMemoryBindBugType(); + +} // namespace immutablestore +} // namespace ento +} // namespace clang + +#endif // LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_STORETOIMMUTABLE_H Index: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp @@ -0,0 +1,84 @@ +//== StoreToImmutableChecker.cpp ------------------------------- -*- 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 StoreToImmutableChecker which warns binds on immutable +// memory. +//===----------------------------------------------------------------------===// + +#include "clang/AST/Type.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/CheckerContext.h" + +using namespace clang; +using namespace ento; + +namespace { + +static const BugType *ImmutableMemoryBindPtr; + +class StoreToImmutableChecker : public Checker { +public: + BugType ImmutableMemoryBind{this, + "Immutable memory should not be overwritten", + categories::MemoryError}; + void checkBind(SVal loc, SVal val, const Stmt *S, CheckerContext &C) const; +}; +} // namespace + + +namespace clang { +namespace ento { +namespace immutablestore { + +const BugType *getImmutableMemoryBindBugType() { return ImmutableMemoryBindPtr; } + +} // namespace immutablestore +} // namespace ento +} // namespace clang + + + + +// Check if region that should be immutable is being modified +void StoreToImmutableChecker::checkBind(SVal Loc, SVal Val, const Stmt *S, + CheckerContext &C) const { + const MemRegion *R = Loc.getAsRegion(); + if (!R) + return; + + if (!isa(R->getMemorySpace())) + return; + + ExplodedNode *ErrorNode = C.generateErrorNode(); + if (!ErrorNode) + return; + + auto Report = std::make_unique( + ImmutableMemoryBind, "Modifying immutable memory", ErrorNode); + Report->markInteresting(R); + + if (const auto *BP = dyn_cast(S)) + if (const auto *UP = dyn_cast(BP->getLHS())) { + bugreporter::trackExpressionValue( + Report->getErrorNode(), UP->getSubExpr()->IgnoreImpCasts(), *Report); + } + + C.emitReport(std::move(Report)); +} + +void ento::registerStoreToImmutableChecker(CheckerManager &Mgr) { + StoreToImmutableChecker *Checker = Mgr.registerChecker(); + ImmutableMemoryBindPtr = &Checker->ImmutableMemoryBind; +} + +bool ento::shouldRegisterStoreToImmutableChecker(const CheckerManager &) { + return true; +} Index: clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp @@ -0,0 +1,102 @@ +//== ModelConstQualifiedReturnChecker.cpp ----------------------- -*- 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 ModelConstQualifiedReturnChecker. +// Return values of certain functions should be treated as const-qualified. +// +// Checker is based on CERT SEI Rule ENV30-C. +// For more information see: https://wiki.sei.cmu.edu/confluence/x/79UxBQ +//===----------------------------------------------------------------------===// + +#include "../StoreToImmutable.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/CallDescription.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" + +using namespace clang; +using namespace ento; + +namespace { + +class ModelConstQualifiedReturnChecker : public Checker { +private: + using HandlerFn = void (ModelConstQualifiedReturnChecker::*)( + const CallEvent &Call, CheckerContext &C) const; + + void postConstQualifiedReturnCall(const CallEvent &Call, + CheckerContext &C) const; + + // SEI CERT ENV30-C + const CallDescriptionMap ConstQualifiedReturnFunctions = { + {{"getenv", 1}, + &ModelConstQualifiedReturnChecker::postConstQualifiedReturnCall}, + {{"setlocale", 2}, + &ModelConstQualifiedReturnChecker::postConstQualifiedReturnCall}, + {{"strerror", 1}, + &ModelConstQualifiedReturnChecker::postConstQualifiedReturnCall}, + {{"localeconv", 0}, + &ModelConstQualifiedReturnChecker::postConstQualifiedReturnCall}, + {{"asctime", 1}, + &ModelConstQualifiedReturnChecker::postConstQualifiedReturnCall}, + }; + +public: + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; +}; +} // namespace + +void ModelConstQualifiedReturnChecker::postConstQualifiedReturnCall( + const CallEvent &Call, CheckerContext &C) const { + const LocationContext *LCtx = C.getLocationContext(); + SValBuilder &SVB = C.getSValBuilder(); + MemRegionManager &RegMgr = SVB.getRegionManager(); + + // Function call will return a pointer to the new symbolic region. + SymbolRef FreshSym = SVB.getSymbolManager().conjureSymbol( + Call.getOriginExpr(), LCtx, C.blockCount()); + const SymbolicRegion *SymReg = RegMgr.getSymbolicRegion( + FreshSym, + RegMgr.getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind)); + + ProgramStateRef State = C.getState(); + State = + State->BindExpr(Call.getOriginExpr(), LCtx, loc::MemRegionVal{SymReg}); + + StringRef FunctionName = Call.getCalleeIdentifier()->getName(); + + const NoteTag *Note = C.getNoteTag( + [SymReg, FunctionName](PathSensitiveBugReport &BR, llvm::raw_ostream &Out) { + if (&BR.getBugType() != immutablestore::getImmutableMemoryBindBugType() || + !BR.isInteresting(SymReg)) + return; + Out << "Return value of '" << FunctionName + << "' should be treated as const-qualified"; + }); + C.addTransition(State, Note); +} + +void ModelConstQualifiedReturnChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const { + // Check if function return value should be const qualified + const auto *Handler = ConstQualifiedReturnFunctions.lookup(Call); + if (!Handler) + return; + + (this->**Handler)(Call, C); +} + +void ento::registerModelConstQualifiedReturnChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); +} + +bool ento::shouldRegisterModelConstQualifiedReturnChecker(const CheckerManager &) { + return true; +} Index: clang/test/Analysis/analyzer-enabled-checkers.c =================================================================== --- clang/test/Analysis/analyzer-enabled-checkers.c +++ clang/test/Analysis/analyzer-enabled-checkers.c @@ -21,6 +21,7 @@ // CHECK-NEXT: core.NullDereference // CHECK-NEXT: core.StackAddrEscapeBase // CHECK-NEXT: core.StackAddressEscape +// CHECK-NEXT: core.StoreToImmutable // CHECK-NEXT: core.UndefinedBinaryOperatorResult // CHECK-NEXT: core.VLASize // CHECK-NEXT: core.builtin.BuiltinFunctions Index: clang/test/Analysis/cert/env30-c.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/env30-c.cpp @@ -0,0 +1,222 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,alpha.security.cert.env.ModelConstQualifiedReturn \ +// RUN: -analyzer-output=text -verify -Wno-unused %s + +#include "../Inputs/system-header-simulator.h" +char *getenv(const char *name); +char *setlocale(int category, const char *locale); +char *strerror(int errnum); +int setenv(const char *name, const char *value, int overwrite); +void free(void *memblock); +void *malloc(size_t size); +char *strdup(const char *s); + +typedef struct { +} tm; +char *asctime(const tm *timeptr); + +int strcmp(const char *, const char *); +void const_parameter_function(const char *); + + + +void getenv_test1() { + char *a, *b, *c; + a = getenv("VAR"); + // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}} + // expected-note@-2{{Value assigned to 'a'}} + b = a; + // expected-note@-1{{The value of 'a' is assigned to 'b'}} + c = b; + // expected-note@-1{{The value of 'b' is assigned to 'c'}} + *c = 'A'; + // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}} + // expected-note@-2{{Modifying immutable memory}} +} + +void getenv_test2() { + char *p; + p = getenv("VAR"); + const_parameter_function(p); // no-warning +} + +void modify(char *p) { + *p = 'A'; + // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}} + // expected-note@-2{{Modifying immutable memory}} +} + +void getenv_test3() { + char *p = getenv("VAR"); + // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}} + // expected-note@-2{{'p' initialized here}} + char *pp = p; + // expected-note@-1{{'pp' initialized to the value of 'p'}} + modify(pp); // Writing to immutable region within the call. + // expected-note@-1{{Calling 'modify'}} + // expected-note@-2{{Passing value via 1st parameter 'p'}} +} + +void does_not_modify(char *p) { + *p; +} + +void getenv_test4() { + char *p = getenv("VAR"); + does_not_modify(p); // no-warning +} + +void getenv_test5() { + static char *array[] = {0}; + + if (!array[0]) { + // expected-note@-1{{Taking true branch}} + array[0] = getenv("TEMPDIR"); + // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}} + // expected-note@-2{{Assigning value}} + } + + *array[0] = 'A'; + // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}} + // expected-note@-2{{Modifying immutable memory}} +} + +void getenv_test6() { + char *p = getenv("VAR"); + // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}} + if (!p) + // expected-note@-1{{Assuming 'p' is non-null}} + // expected-note@-2{{Taking false branch}} + return; + p[0] = '\0'; + // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}} + // expected-note@-2{{Modifying immutable memory}} +} + +void setlocale_test() { + char *p = setlocale(0, "VAR"); + // expected-note@-1{{Return value of 'setlocale' should be treated as const-qualified}} + // expected-note@-2{{'p' initialized here}} + *p = 'A'; + // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}} + // expected-note@-2{{Modifying immutable memory}} +} + +void strerror_test() { + char *p = strerror(0); + // expected-note@-1{{Return value of 'strerror' should be treated as const-qualified}} + // expected-note@-2{{'p' initialized here}} + *p = 'A'; + // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}} + // expected-note@-2{{Modifying immutable memory}} +} + +void asctime_test() { + char* p = asctime(nullptr); + // expected-note@-1{{Return value of 'asctime' should be treated as const-qualified}} + // expected-note@-2{{'p' initialized here}} + + *p = 'A'; + // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}} + // expected-note@-2{{Modifying immutable memory}} +} + + +// Test cases from CERT Rule Page + +namespace noncompliant { +void trstr(char *c_str, char orig, char rep) { + while (*c_str != '\0') { + // expected-note@-1{{Assuming the condition is true}} + // expected-note@-2{{Loop condition is true. Entering loop body}} + if (*c_str == orig) { + // expected-note@-1{{Assuming the condition is true}} + // expected-note@-2{{Taking true branch}} + *c_str = rep; + // expected-warning@-1{{Modifying immutable memory [core.StoreToImmutable]}} + // expected-note@-2{{Modifying immutable memory}} + } + ++c_str; + } +} + +void func(void) { + char *env = getenv("TEST_ENV"); + // expected-note@-1{{Return value of 'getenv' should be treated as const-qualified}} + // expected-note@-2{{'env' initialized here}} + if (env == NULL) { + // expected-note@-1{{Assuming 'env' is not equal to NULL}} + // expected-note@-2{{Taking false branch}} + return; + } + trstr(env, '"', '_'); // Writing to immutable region within the call. + // expected-note@-1{{Calling 'trstr'}} + // expected-note@-2{{Passing value via 1st parameter 'c_str'}} +} + +} // namespace noncompliant + +namespace compliant1 { +void trstr(char *c_str, char orig, char rep) { + while (*c_str != '\0') { + if (*c_str == orig) { + *c_str = rep; // no-warning + } + ++c_str; + } +} + +void func(void) { + const char *env; + char *copy_of_env; + + env = getenv("TEST_ENV"); + if (env == NULL) { + return; + } + + copy_of_env = (char *)malloc(strlen(env) + 1); + if (copy_of_env == NULL) { + return; + } + + strcpy(copy_of_env, env); + trstr(copy_of_env, '"', '_'); + /* ... */ + free(copy_of_env); +} +} // namespace compliant1 + +namespace compliant2 { +void trstr(char *c_str, char orig, char rep) { + while (*c_str != '\0') { + if (*c_str == orig) { + *c_str = rep; // no-warning + } + ++c_str; + } +} + +void func(void) { + const char *env; + char *copy_of_env; + + env = getenv("TEST_ENV"); + if (env == NULL) { + return; + } + + copy_of_env = strdup(env); + if (copy_of_env == NULL) { + return; + } + + trstr(copy_of_env, '"', '_'); + + if (setenv("TEST_ENV", copy_of_env, 1) != 0) { + return; + } + /* ... */ + free(copy_of_env); +} +} // namespace compliant2 Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c =================================================================== --- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c +++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c @@ -32,6 +32,7 @@ // CHECK-NEXT: core.NullDereference // CHECK-NEXT: core.StackAddrEscapeBase // CHECK-NEXT: core.StackAddressEscape +// CHECK-NEXT: core.StoreToImmutable // CHECK-NEXT: core.UndefinedBinaryOperatorResult // CHECK-NEXT: core.VLASize // CHECK-NEXT: core.builtin.BuiltinFunctions