diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -132,6 +132,21 @@ 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 + } + .. _core-UndefinedBinaryOperatorResult: @@ -2308,6 +2323,33 @@ // 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. These functions +include getenv(), setlocale(), localeconv(), asctime(), and +strerror(). In such cases, the function call results must be +treated as being const-qualified. + +Checker models return values of these functions as const +qualified. Their modification is checked in StoreToImmutable +core checker. + +.. 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 ^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -220,6 +220,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, @@ -972,6 +976,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 { diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h @@ -764,8 +764,11 @@ assert(s->getType()->isAnyPointerType() || s->getType()->isReferenceType() || s->getType()->isBlockPointerType()); - assert(isa(sreg) || isa(sreg) || - isa(sreg)); + assert(isa(sreg) || + isa(sreg) || + isa(sreg) || + isa(sreg)); + } public: diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/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 diff --git a/clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp new file mode 100644 --- /dev/null +++ b/clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp @@ -0,0 +1,62 @@ +//== 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 { + +class StoreToImmutableChecker : public Checker { +private: + BugType ImmutableMemoryBind{this, + "Immutable memory should not be overwritten", + categories::MemoryError}; + +public: + void checkBind(SVal loc, SVal val, const Stmt *S, CheckerContext &C) const; +}; +} // namespace + +// 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); + C.emitReport(std::move(Report)); +} + +void ento::registerStoreToImmutableChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); +} + +bool ento::shouldRegisterStoreToImmutableChecker(const CheckerManager &) { + return true; +} \ No newline at end of file diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp new file mode 100644 --- /dev/null +++ b/clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp @@ -0,0 +1,92 @@ +//== 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 +// +// CERT SEI Rule ENV30-C +// For more information see: https://wiki.sei.cmu.edu/confluence/x/79UxBQ +//===----------------------------------------------------------------------===// + +#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: + void evalConstQualifiedReturnCall(const CallEvent &Call, + CheckerContext &C) const; + + // SEI CERT ENV30-C + const CallDescriptionSet ConstQualifiedReturnFunctions = { + {"getenv", 1}, + {"setlocale", 2}, + {"strerror", 1}, + {"localeconv", 0}, + {"asctime", 1}, + }; + +public: + bool evalCall(const CallEvent &Call, CheckerContext &C) const; +}; +} // namespace + +void ModelConstQualifiedReturnChecker::evalConstQualifiedReturnCall( + 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 FunName = Call.getCalleeIdentifier()->getName(); + const NoteTag *Note = C.getNoteTag( + [SymReg, FunName](PathSensitiveBugReport &BR, llvm::raw_ostream &Out) { + if (BR.getBugType().getCheckerName() != "core.StoreToImmutable" || + !BR.isInteresting(SymReg)) + return; + Out << "return value of '" << FunName + << "' should be treated as const-qualified"; + }); + + C.addTransition(State, Note); +} + +bool ModelConstQualifiedReturnChecker::evalCall(const CallEvent &Call, CheckerContext &C) const { + if (!ConstQualifiedReturnFunctions.contains(Call)) + return false; + + evalConstQualifiedReturnCall(Call, C); + return true; +} + +void ento::registerModelConstQualifiedReturnChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); +} + +bool ento::shouldRegisterModelConstQualifiedReturnChecker(const CheckerManager &) { + return true; +} diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c --- a/clang/test/Analysis/analyzer-enabled-checkers.c +++ b/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 diff --git a/clang/test/Analysis/cert/env30-c.cpp b/clang/test/Analysis/cert/env30-c.cpp new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/cert/env30-c.cpp @@ -0,0 +1,216 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core.StoreToImmutable,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 non_const_parameter_function(char *e); +void const_parameter_function(const char *); + + + +void getenv_test1() { + char *p = getenv("VAR"); + // expected-note@-1{{return value of 'getenv' should be treated as const-qualified}} + *p = 'A'; + // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}} + // expected-note@-2{{modifying immutable memory}} +} + +void getenv_test2() { + char *p; + + p = getenv("VAR"); + non_const_parameter_function(p); // FIXME: Maybe we should warn for these. +} + +void getenv_test3() { + 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_test4() { + char *p = getenv("VAR"); + // expected-note@-1{{return value of 'getenv' should be treated as const-qualified}} + char *pp = p; + modify(pp); // Writing to immutable region within the call. + // expected-note@-1{{Calling 'modify'}} +} + +void does_not_modify(char *p) { + *p; +} + +void getenv_test5() { + char *p = getenv("VAR"); + does_not_modify(p); // no-warning +} + +void getenv_test6() { + 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}} + } + + *array[0] = 'A'; + // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}} + // expected-note@-2{{modifying immutable memory}} +} + +void getenv_test7() { + 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}} + *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}} + *p = 'A'; + // expected-warning@-1{{modifying immutable memory [core.StoreToImmutable]}} + // expected-note@-2{{modifying immutable memory}} +} + +void asctime_test() { + const tm *t; + char* p = asctime(t); + // expected-note@-1{{return value of 'asctime' should be treated as const-qualified}} + + *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}} + 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'}} +} + +} // 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 \ No newline at end of file diff --git a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c --- a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c +++ b/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