diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 597ffcc4a10a2..43137f4b020f9 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -2520,13 +2520,34 @@ pointer. These functions include: getenv, localeconv, asctime, setlocale, strerr char *p, *pp; p = getenv("VAR"); - pp = getenv("VAR2"); - // subsequent call to 'getenv' invalidated previous one + setenv("SOMEVAR", "VALUE", /*overwrite = */1); + // call to 'setenv' may invalidate p *p; // dereferencing invalid pointer } + +The ``InvalidatingGetEnv`` option is available for treating getenv calls as +invalidating. When enabled, the checker issues a warning if getenv is called +multiple times and their results are used without first creating a copy. +This level of strictness might be considered overly pedantic for the commonly +used getenv implementations. + +To enable this option, use: +``-analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true``. + +By default, this option is set to *false*. + +When this option is enabled, warnings will be generated for scenarios like the +following: + +.. code-block:: c + + char* p = getenv("VAR"); + char* pp = getenv("VAR2"); // assumes this call can invalidate `env` + strlen(p); // warns about accessing invalid ptr + alpha.security.taint ^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index be813bde8be41..b6e9f0fae1c7f 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1002,6 +1002,15 @@ let ParentPackage = ENV in { 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 index aae1a17bc0ae5..e5dd907c660d8 100644 --- a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp @@ -30,7 +30,10 @@ namespace { class InvalidPtrChecker : public Checker { private: - BugType BT{this, "Use of invalidated pointer", categories::MemoryError}; + // For accurate emission of NoteTags, the BugType of this checker should have + // a unique address. + BugType InvalidPtrBugType{this, "Use of invalidated pointer", + categories::MemoryError}; void EnvpInvalidatingCall(const CallEvent &Call, CheckerContext &C) const; @@ -38,6 +41,15 @@ class InvalidPtrChecker 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 +63,6 @@ class InvalidPtrChecker // SEI CERT ENV34-C const CallDescriptionMap PreviousCallInvalidatingFunctions = { - {{{"getenv"}, 1}, &InvalidPtrChecker::postPreviousReturnInvalidatingCall}, {{{"setlocale"}, 2}, &InvalidPtrChecker::postPreviousReturnInvalidatingCall}, {{{"strerror"}, 1}, @@ -62,6 +73,10 @@ class InvalidPtrChecker &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; @@ -76,6 +91,11 @@ class InvalidPtrChecker // Check if invalidated region is being dereferenced. void checkLocation(SVal l, bool isLoad, const Stmt *S, CheckerContext &C) const; + +private: + const NoteTag *createEnvInvalidationNote(CheckerContext &C, + ProgramStateRef State, + StringRef FunctionName) const; }; } // namespace @@ -84,33 +104,74 @@ class InvalidPtrChecker 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 REGISTER_MAP_WITH_PROGRAMSTATE(PreviousCallResultMap, const FunctionDecl *, const MemRegion *) +const NoteTag *InvalidPtrChecker::createEnvInvalidationNote( + CheckerContext &C, ProgramStateRef State, StringRef FunctionName) const { + + const MemRegion *MainRegion = State->get(); + const auto GetenvRegions = State->get(); + + return C.getNoteTag([this, MainRegion, GetenvRegions, + FunctionName = std::string{FunctionName}]( + PathSensitiveBugReport &BR, llvm::raw_ostream &Out) { + // Only handle the BugType of this checker. + if (&BR.getBugType() != &InvalidPtrBugType) + return; + + // Mark all regions that were interesting before as NOT interesting now + // to avoid extra notes coming from invalidation points higher up the + // bugpath. This ensures that only the last invalidation point is marked + // with a note tag. + llvm::SmallVector InvalidLocationNames; + if (BR.isInteresting(MainRegion)) { + BR.markNotInteresting(MainRegion); + InvalidLocationNames.push_back("the environment parameter of 'main'"); + } + bool InterestingGetenvFound = false; + for (const MemRegion *MR : GetenvRegions) { + if (BR.isInteresting(MR)) { + BR.markNotInteresting(MR); + if (!InterestingGetenvFound) { + InterestingGetenvFound = true; + InvalidLocationNames.push_back( + "the environment returned by 'getenv'"); + } + } + } + + // Emit note tag message. + if (InvalidLocationNames.size() >= 1) + Out << '\'' << FunctionName << "' call may invalidate " + << InvalidLocationNames[0]; + if (InvalidLocationNames.size() == 2) + Out << ", and " << InvalidLocationNames[1]; + }); +} + void InvalidPtrChecker::EnvpInvalidatingCall(const CallEvent &Call, CheckerContext &C) const { - StringRef FunctionName = Call.getCalleeIdentifier()->getName(); + // This callevent invalidates all previously generated pointers to the + // environment. ProgramStateRef State = C.getState(); - const MemRegion *SymbolicEnvPtrRegion = State->get(); - if (!SymbolicEnvPtrRegion) - return; - - State = State->add(SymbolicEnvPtrRegion); + if (const MemRegion *MainEnvPtr = State->get()) + State = State->add(MainEnvPtr); + for (const MemRegion *EnvPtr : State->get()) + State = State->add(EnvPtr); - 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'"; - }); + StringRef FunctionName = Call.getCalleeIdentifier()->getName(); + const NoteTag *InvalidationNote = + createEnvInvalidationNote(C, State, FunctionName); - C.addTransition(State, Note); + C.addTransition(State, InvalidationNote); } void InvalidPtrChecker::postPreviousReturnInvalidatingCall( @@ -124,9 +185,9 @@ void InvalidPtrChecker::postPreviousReturnInvalidatingCall( if (const MemRegion *const *Reg = State->get(FD)) { const MemRegion *PrevReg = *Reg; State = State->add(PrevReg); - Note = C.getNoteTag([PrevReg, FD](PathSensitiveBugReport &BR, - llvm::raw_ostream &Out) { - if (!BR.isInteresting(PrevReg)) + Note = C.getNoteTag([this, PrevReg, FD](PathSensitiveBugReport &BR, + llvm::raw_ostream &Out) { + if (!BR.isInteresting(PrevReg) || &BR.getBugType() != &InvalidPtrBugType) return; Out << '\''; FD->getNameForDiagnostic(Out, FD->getASTContext().getLangOpts(), true); @@ -146,16 +207,15 @@ void InvalidPtrChecker::postPreviousReturnInvalidatingCall( // 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); - const NoteTag *PreviousCallNote = - C.getNoteTag([MR](PathSensitiveBugReport &BR, llvm::raw_ostream &Out) { - if (!BR.isInteresting(MR)) + const NoteTag *PreviousCallNote = C.getNoteTag( + [this, MR](PathSensitiveBugReport &BR, llvm::raw_ostream &Out) { + if (!BR.isInteresting(MR) || &BR.getBugType() != &InvalidPtrBugType) return; - Out << '\'' << "'previous function call was here" << '\''; + Out << "previous function call was here"; }); C.addTransition(State, Node, PreviousCallNote); @@ -185,6 +245,18 @@ static const MemRegion *findInvalidatedSymbolicBase(ProgramStateRef State, // 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 +265,16 @@ void InvalidPtrChecker::checkPostCall(const CallEvent &Call, 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( @@ -218,8 +292,8 @@ void InvalidPtrChecker::checkPostCall(const CallEvent &Call, C.getASTContext().getPrintingPolicy()); Out << "' in a function call"; - auto Report = - std::make_unique(BT, Out.str(), ErrorNode); + auto Report = std::make_unique( + InvalidPtrBugType, Out.str(), ErrorNode); Report->markInteresting(InvalidatedSymbolicBase); Report->addRange(Call.getArgSourceRange(I)); C.emitReport(std::move(Report)); @@ -243,7 +317,7 @@ void InvalidPtrChecker::checkBeginFunction(CheckerContext &C) const { // 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. @@ -262,13 +336,16 @@ void InvalidPtrChecker::checkLocation(SVal Loc, bool isLoad, const Stmt *S, return; auto Report = std::make_unique( - BT, "dereferencing an invalid pointer", ErrorNode); + InvalidPtrBugType, "dereferencing an invalid pointer", ErrorNode); Report->markInteresting(InvalidatedSymbolicBase); C.emitReport(std::move(Report)); } 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 index 794ef8b9cc086..8abfbf84983d8 100644 --- 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: apply-fixits = false diff --git a/clang/test/Analysis/cert/env34-c-cert-examples.c b/clang/test/Analysis/cert/env34-c-cert-examples.c index 19ca73f34c7ee..6d6659e55d86b 100644 --- a/clang/test/Analysis/cert/env34-c-cert-examples.c +++ b/clang/test/Analysis/cert/env34-c-cert-examples.c @@ -1,15 +1,48 @@ +// 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=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*); 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}} + } +} + +void incorrect_usage_double_getenv_invalidation(void) { char *tmpvar; char *tempvar; @@ -18,13 +51,13 @@ void incorrect_usage(void) { 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 index dc7b0340c311e..94bc2cf95ccc9 100644 --- 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 index 0000000000000..e8ffee7fb479d --- /dev/null +++ b/clang/test/Analysis/invalid-ptr-checker.c @@ -0,0 +1,63 @@ +// 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}} +} + +void multiple_invalidation_no_duplicate_notes(void) { + char *v1 = getenv("VAR1"); + + setenv("VAR2", "...", 1); // no note here + + setenv("VAR3", "...", 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}} +}