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 @@ -997,6 +997,15 @@ def InvalidPtrChecker : Checker<"InvalidPtr">, HelpText<"Finds usages of possibly invalidated pointers">, + CheckerOptions<[ + CmdLineOption, + ]>, Documentation; } // end "alpha.cert.env" diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp @@ -38,6 +38,15 @@ CheckerContext &C) const; // SEI CERT ENV31-C + + // If set to true, consider getenv calls as invalidating operations on the + // environment variable buffer. This is implied in the standard, but in + // practice does not cause problems (in the commonly used environments). + bool InvalidatingGetEnv = false; + + // GetEnv can be treated invalidating and non-invalidating as well. + const CallDescription GetEnvCall{{"getenv"}, 1}; + const CallDescriptionMap EnvpInvalidatingFunctions = { {{{"setenv"}, 3}, &InvalidPtrChecker::EnvpInvalidatingCall}, {{{"unsetenv"}, 1}, &InvalidPtrChecker::EnvpInvalidatingCall}, @@ -51,7 +60,6 @@ // SEI CERT ENV34-C const CallDescriptionMap PreviousCallInvalidatingFunctions = { - {{{"getenv"}, 1}, &InvalidPtrChecker::postPreviousReturnInvalidatingCall}, {{{"setlocale"}, 2}, &InvalidPtrChecker::postPreviousReturnInvalidatingCall}, {{{"strerror"}, 1}, @@ -62,6 +70,10 @@ &InvalidPtrChecker::postPreviousReturnInvalidatingCall}, }; + // The private members of this checker corresponding to commandline options + // are set in this function. + friend void ento::registerInvalidPtrChecker(CheckerManager &); + public: // Obtain the environment pointer from 'main()' (if present). void checkBeginFunction(CheckerContext &C) const; @@ -84,7 +96,10 @@ REGISTER_SET_WITH_PROGRAMSTATE(InvalidMemoryRegions, const MemRegion *) // Stores the region of the environment pointer of 'main' (if present). -REGISTER_TRAIT_WITH_PROGRAMSTATE(EnvPtrRegion, const MemRegion *) +REGISTER_TRAIT_WITH_PROGRAMSTATE(MainEnvPtrRegion, const MemRegion *) + +// Stores the regions of environments returned by getenv calls. +REGISTER_SET_WITH_PROGRAMSTATE(GetenvEnvPtrRegions, const MemRegion *) // Stores key-value pairs, where key is function declaration and value is // pointer to memory region returned by previous call of this function @@ -95,22 +110,35 @@ CheckerContext &C) const { StringRef FunctionName = Call.getCalleeIdentifier()->getName(); ProgramStateRef State = C.getState(); - const MemRegion *SymbolicEnvPtrRegion = State->get(); - if (!SymbolicEnvPtrRegion) - return; - State = State->add(SymbolicEnvPtrRegion); + auto PlaceInvalidationNote = [&C, FunctionName, + &State](const MemRegion *Region, + StringRef Message, ExplodedNode *Pred) { + State = State->add(Region); + + // Make copy of string data for the time when notes are *actually* created. + const NoteTag *Note = + C.getNoteTag([Region, FunctionName = std::string{FunctionName}, + Message = std::string{Message}]( + PathSensitiveBugReport &BR, llvm::raw_ostream &Out) { + if (!BR.isInteresting(Region)) + return; + Out << '\'' << FunctionName << "' " << Message; + }); + return C.addTransition(State, Pred, Note); + }; - const NoteTag *Note = - C.getNoteTag([SymbolicEnvPtrRegion, FunctionName]( - PathSensitiveBugReport &BR, llvm::raw_ostream &Out) { - if (!BR.isInteresting(SymbolicEnvPtrRegion)) - return; - Out << '\'' << FunctionName - << "' call may invalidate the environment parameter of 'main'"; - }); + ExplodedNode *CurrentChainEnd = C.getPredecessor(); + + if (const MemRegion *MainEnvPtr = State->get()) + CurrentChainEnd = PlaceInvalidationNote( + MainEnvPtr, "call may invalidate the environment parameter of 'main'", + CurrentChainEnd); - C.addTransition(State, Note); + for (const MemRegion *EnvPtr : State->get()) + CurrentChainEnd = PlaceInvalidationNote( + EnvPtr, "call may invalidate the environment returned by getenv", + CurrentChainEnd); } void InvalidPtrChecker::postPreviousReturnInvalidatingCall( @@ -146,8 +174,7 @@ // Remember to this region. const auto *SymRegOfRetVal = cast(RetVal.getAsRegion()); - const MemRegion *MR = - const_cast(SymRegOfRetVal->getBaseRegion()); + const MemRegion *MR = SymRegOfRetVal->getBaseRegion(); State = State->set(FD, MR); ExplodedNode *Node = C.addTransition(State, Note); @@ -185,6 +212,18 @@ // function call as an argument. void InvalidPtrChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const { + + ProgramStateRef State = C.getState(); + + // Model 'getenv' calls + if (GetEnvCall.matches(Call)) { + const MemRegion *Region = Call.getReturnValue().getAsRegion(); + if (Region) { + State = State->add(Region); + C.addTransition(State); + } + } + // Check if function invalidates 'envp' argument of 'main' if (const auto *Handler = EnvpInvalidatingFunctions.lookup(Call)) (this->**Handler)(Call, C); @@ -193,14 +232,16 @@ if (const auto *Handler = PreviousCallInvalidatingFunctions.lookup(Call)) (this->**Handler)(Call, C); + // If pedantic mode is on, regard 'getenv' calls invalidating as well + if (InvalidatingGetEnv && GetEnvCall.matches(Call)) + postPreviousReturnInvalidatingCall(Call, C); + // Check if one of the arguments of the function call is invalidated // If call was inlined, don't report invalidated argument if (C.wasInlined) return; - ProgramStateRef State = C.getState(); - for (unsigned I = 0, NumArgs = Call.getNumArgs(); I < NumArgs; ++I) { if (const auto *SR = dyn_cast_or_null( @@ -243,7 +284,7 @@ // Save the memory region pointed by the environment pointer parameter of // 'main'. - C.addTransition(State->set(EnvpReg)); + C.addTransition(State->set(EnvpReg)); } // Check if invalidated region is being dereferenced. @@ -268,7 +309,10 @@ } void ento::registerInvalidPtrChecker(CheckerManager &Mgr) { - Mgr.registerChecker(); + auto *Checker = Mgr.registerChecker(); + Checker->InvalidatingGetEnv = + Mgr.getAnalyzerOptions().getCheckerBooleanOption(Checker, + "InvalidatingGetEnv"); } bool ento::shouldRegisterInvalidPtrChecker(const CheckerManager &) { diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c --- a/clang/test/Analysis/analyzer-config.c +++ b/clang/test/Analysis/analyzer-config.c @@ -11,6 +11,7 @@ // CHECK-NEXT: alpha.osx.cocoa.DirectIvarAssignment:AnnotatedFunctions = false // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01 +// CHECK-NEXT: alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv = false // CHECK-NEXT: alpha.security.taint.TaintPropagation:Config = "" // CHECK-NEXT: alpha.unix.Errno:AllowErrnoReadOutsideConditionExpressions = true // CHECK-NEXT: alpha.unix.StdCLibraryFunctions:DisplayLoadedSummaries = false diff --git a/clang/test/Analysis/cert/env34-c-cert-examples.c b/clang/test/Analysis/cert/env34-c-cert-examples.c --- a/clang/test/Analysis/cert/env34-c-cert-examples.c +++ b/clang/test/Analysis/cert/env34-c-cert-examples.c @@ -1,15 +1,49 @@ +// Default options. // RUN: %clang_analyze_cc1 \ // RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \ // RUN: -verify -Wno-unused %s +// +// Test the laxer handling of getenv function (this is the default). +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \ +// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=false \ +// RUN: -verify -Wno-unused %s +// +// Test the stricter handling of getenv function. +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \ +// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \ +// RUN: -verify=pedantic -Wno-unused %s #include "../Inputs/system-header-simulator.h" char *getenv(const char *name); +int setenv(const char *name, const char *value, int overwrite); int strcmp(const char*, const char*); char *strdup(const char*); void free(void *memblock); void *malloc(size_t size); -void incorrect_usage(void) { +void incorrect_usage_setenv_getenv_invalidation(void) { + char *tmpvar; + char *tempvar; + + tmpvar = getenv("TMP"); + + if (!tmpvar) + return; + + setenv("TEMP", "", 1); //setenv can invalidate env + + if (!tmpvar) + return; + + if (strcmp(tmpvar, "") == 0) { // body of strcmp is unknown + // expected-warning@-1{{use of invalidated pointer 'tmpvar' in a function call}} + // pedantic-warning@-2{{use of invalidated pointer 'tmpvar' in a function call}} + } +} + +void incorrect_usage_double_getenv_invalidation(void) { char *tmpvar; char *tempvar; @@ -18,13 +52,13 @@ if (!tmpvar) return; - tempvar = getenv("TEMP"); + tempvar = getenv("TEMP"); //getenv should not invalidate env in non-pedantic mode if (!tempvar) return; if (strcmp(tmpvar, tempvar) == 0) { // body of strcmp is unknown - // expected-warning@-1{{use of invalidated pointer 'tmpvar' in a function call}} + // pedantic-warning@-1{{use of invalidated pointer 'tmpvar' in a function call}} } } diff --git a/clang/test/Analysis/cert/env34-c.c b/clang/test/Analysis/cert/env34-c.c --- a/clang/test/Analysis/cert/env34-c.c +++ b/clang/test/Analysis/cert/env34-c.c @@ -1,5 +1,6 @@ // RUN: %clang_analyze_cc1 \ // RUN: -analyzer-checker=alpha.security.cert.env.InvalidPtr\ +// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \ // RUN: -analyzer-output=text -verify -Wno-unused %s #include "../Inputs/system-header-simulator.h" diff --git a/clang/test/Analysis/invalid-ptr-checker.c b/clang/test/Analysis/invalid-ptr-checker.c new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/invalid-ptr-checker.c @@ -0,0 +1,50 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=alpha.security.cert.env.InvalidPtr \ +// RUN: -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=false \ +// RUN: -analyzer-output=text -verify -Wno-unused %s +// +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=alpha.security.cert.env.InvalidPtr \ +// RUN: -analyzer-config \ +// RUN: alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \ +// RUN: -analyzer-output=text -verify=expected,pedantic -Wno-unused %s + +#include "Inputs/system-header-simulator.h" + +char *getenv(const char *name); +int setenv(const char *name, const char *value, int overwrite); +int strcmp(const char *, const char *); + +int custom_env_handler(const char **envp); + +void getenv_after_getenv(void) { + char *v1 = getenv("V1"); + // pedantic-note@-1{{previous function call was here}} + + char *v2 = getenv("V2"); + // pedantic-note@-1{{'getenv' call may invalidate the result of the previous 'getenv'}} + + strcmp(v1, v2); + // pedantic-warning@-1{{use of invalidated pointer 'v1' in a function call}} + // pedantic-note@-2{{use of invalidated pointer 'v1' in a function call}} +} + +void setenv_after_getenv(void) { + char *v1 = getenv("VAR1"); + + setenv("VAR2", "...", 1); + // expected-note@-1{{'setenv' call may invalidate the environment returned by getenv}} + + strcmp(v1, ""); + // expected-warning@-1{{use of invalidated pointer 'v1' in a function call}} + // expected-note@-2{{use of invalidated pointer 'v1' in a function call}} +} + +int main(int argc, const char *argv[], const char *envp[]) { + setenv("VAR", "...", 0); + // expected-note@-1 2 {{'setenv' call may invalidate the environment parameter of 'main'}} + + *envp; + // expected-warning@-1 2 {{dereferencing an invalid pointer}} + // expected-note@-2 2 {{dereferencing an invalid pointer}} +}