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 @@ -984,6 +984,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 @@ -51,7 +51,6 @@ // SEI CERT ENV34-C const CallDescriptionMap PreviousCallInvalidatingFunctions = { - {{{"getenv"}, 1}, &InvalidPtrChecker::postPreviousReturnInvalidatingCall}, {{{"setlocale"}, 2}, &InvalidPtrChecker::postPreviousReturnInvalidatingCall}, {{{"strerror"}, 1}, @@ -62,6 +61,15 @@ &InvalidPtrChecker::postPreviousReturnInvalidatingCall}, }; + // If set to true, consider getenv calls as invalidating operations on the + // environment variable buffer. This is implied in the standard, but can lead + // to practical false positives. + bool InvalidatingGetEnv = false; + + // 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 +92,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 +106,33 @@ 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); + + const NoteTag *Note = + C.getNoteTag([Region, FunctionName, 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 = nullptr; + + 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( @@ -185,6 +207,17 @@ // function call as an argument. void InvalidPtrChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const { + + ProgramStateRef State = C.getState(); + + // Model 'getenv' calls + CallDescription GetEnvCall{{"getenv"}, 1}; + if (GetEnvCall.matches(Call)) { + State = + State->add(Call.getReturnValue().getAsRegion()); + C.addTransition(State); + } + // Check if function invalidates 'envp' argument of 'main' if (const auto *Handler = EnvpInvalidatingFunctions.lookup(Call)) (this->**Handler)(Call, C); @@ -193,14 +226,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 +278,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 +303,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,6 +1,11 @@ // 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 +// +// TODO: write test cases that follow the pattern: +// "getenv -> store pointer -> setenv -> use stored pointer" +// and not rely solely on getenv as an invalidating function #include "../Inputs/system-header-simulator.h" char *getenv(const char *name);