Author: Balázs Kéri Date: 2024-05-23T12:56:16+02:00 New Revision: 3c23047413957024294872e38c27707c71d05805
URL: https://github.com/llvm/llvm-project/commit/3c23047413957024294872e38c27707c71d05805 DIFF: https://github.com/llvm/llvm-project/commit/3c23047413957024294872e38c27707c71d05805.diff LOG: [clang][analyzer] Move checker 'cert.pos.34c' (in alpha.security) into 'PutenvStackArray' (#92424) The "cert" package looks not useful and the checker has not a meaningful name with the old naming scheme. Additionally tests and documentation is updated. Added: clang/lib/StaticAnalyzer/Checkers/PutenvStackArrayChecker.cpp clang/test/Analysis/putenv-stack-array.c Modified: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt Removed: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp clang/test/Analysis/cert/pos34-c-fp-suppression.cpp clang/test/Analysis/cert/pos34-c.cpp ################################################################################ diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index b4bd9dac1cbc2..ac9f0b06f63ba 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -2833,6 +2833,31 @@ Warn on mmap() calls that are both writable and executable. // code } +.. _alpha-security-putenv-stack-array: + +alpha.security.PutenvStackArray (C) +""""""""""""""""""""""""""""""""""" +Finds calls to the ``putenv`` function which pass a pointer to a stack-allocated +(automatic) array as the argument. Function ``putenv`` does not copy the passed +string, only a pointer to the data is stored and this data can be read even by +other threads. Content of a stack-allocated array is likely to be overwritten +after returning from the parent function. + +The problem can be solved by using a static array variable or dynamically +allocated memory. Even better is to avoid using ``putenv`` (it has other +problems related to memory leaks) and use ``setenv`` instead. + +The check corresponds to CERT rule +`POS34-C. Do not call putenv() with a pointer to an automatic variable as the argument +<https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument>`_. + +.. code-block:: c + + int f() { + char env[] = "NAME=value"; + return putenv(env); // putenv function should not be called with stack-allocated string + } + .. _alpha-security-ReturnPtrRange: alpha.security.ReturnPtrRange (C) @@ -2859,55 +2884,6 @@ alpha.security.cert SEI CERT checkers which tries to find errors based on their `C coding rules <https://wiki.sei.cmu.edu/confluence/display/c/2+Rules>`_. -.. _alpha-security-cert-pos-checkers: - -alpha.security.cert.pos -^^^^^^^^^^^^^^^^^^^^^^^ - -SEI CERT checkers of `POSIX C coding rules <https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152405>`_. - -.. _alpha-security-cert-pos-34c: - -alpha.security.cert.pos.34c -""""""""""""""""""""""""""" -Finds calls to the ``putenv`` function which pass a pointer to an automatic variable as the argument. - -.. code-block:: c - - int func(const char *var) { - char env[1024]; - int retval = snprintf(env, sizeof(env),"TEST=%s", var); - if (retval < 0 || (size_t)retval >= sizeof(env)) { - /* Handle error */ - } - - return putenv(env); // putenv function should not be called with auto variables - } - -Limitations: - - - Technically, one can pass automatic variables to ``putenv``, - but one needs to ensure that the given environment key stays - alive until it's removed or overwritten. - Since the analyzer cannot keep track of which envvars get overwritten - and when, it needs to be slightly more aggressive and warn for such - cases too, leading in some cases to false-positive reports like this: - - .. code-block:: c - - void baz() { - char env[] = "NAME=value"; - putenv(env); // false-positive warning: putenv function should not be called... - // More code... - putenv((char *)"NAME=anothervalue"); - // This putenv call overwrites the previous entry, thus that can no longer dangle. - } // 'env' array becomes dead only here. - -alpha.security.cert.env -^^^^^^^^^^^^^^^^^^^^^^^ - -SEI CERT checkers of `Environment C coding rules <https://wiki.sei.cmu.edu/confluence/x/JdcxBQ>`_. - alpha.security.taint ^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index d0ba1ce548403..40f443047bd4b 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1035,15 +1035,6 @@ let ParentPackage = ENV in { } // end "security.cert.env" -let ParentPackage = POSAlpha in { - - def PutenvWithAuto : Checker<"34c">, - HelpText<"Finds calls to the 'putenv' function which pass a pointer to " - "an automatic variable as the argument.">, - Documentation<HasDocumentation>; - -} // end "alpha.cert.pos" - let ParentPackage = SecurityAlpha in { def ArrayBoundChecker : Checker<"ArrayBound">, @@ -1054,10 +1045,6 @@ def ArrayBoundCheckerV2 : Checker<"ArrayBoundV2">, HelpText<"Warn about buffer overflows (newer checker)">, Documentation<HasDocumentation>; -def ReturnPointerRangeChecker : Checker<"ReturnPtrRange">, - HelpText<"Check for an out-of-bound pointer being returned to callers">, - Documentation<HasDocumentation>; - def MallocOverflowSecurityChecker : Checker<"MallocOverflow">, HelpText<"Check for overflows in the arguments to malloc()">, Documentation<HasDocumentation>; @@ -1078,6 +1065,15 @@ def MmapWriteExecChecker : Checker<"MmapWriteExec">, ]>, Documentation<HasDocumentation>; +def PutenvStackArray : Checker<"PutenvStackArray">, + HelpText<"Finds calls to the function 'putenv' which pass a pointer to " + "an automatic (stack-allocated) array as the argument.">, + Documentation<HasDocumentation>; + +def ReturnPointerRangeChecker : Checker<"ReturnPtrRange">, + HelpText<"Check for an out-of-bound pointer being returned to callers">, + Documentation<HasDocumentation>; + } // end "alpha.security" //===----------------------------------------------------------------------===// diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 45d3788f105dc..cd5a3bdd02e4a 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -96,7 +96,7 @@ add_clang_library(clangStaticAnalyzerCheckers PointerSortingChecker.cpp PointerSubChecker.cpp PthreadLockChecker.cpp - cert/PutenvWithAutoChecker.cpp + PutenvStackArrayChecker.cpp RetainCountChecker/RetainCountChecker.cpp RetainCountChecker/RetainCountDiagnostics.cpp ReturnPointerRangeChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PutenvStackArrayChecker.cpp similarity index 70% rename from clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp rename to clang/lib/StaticAnalyzer/Checkers/PutenvStackArrayChecker.cpp index a82f7caf16b29..d59cebf0aa5cb 100644 --- a/clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/PutenvStackArrayChecker.cpp @@ -1,4 +1,4 @@ -//== PutenvWithAutoChecker.cpp --------------------------------- -*- C++ -*--=// +//== PutenvStackArrayChecker.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. @@ -6,13 +6,13 @@ // //===----------------------------------------------------------------------===// // -// This file defines PutenvWithAutoChecker which finds calls of ``putenv`` -// function with automatic variable as the argument. +// This file defines PutenvStackArrayChecker which finds calls of ``putenv`` +// function with automatic array variable as the argument. // https://wiki.sei.cmu.edu/confluence/x/6NYxBQ // //===----------------------------------------------------------------------===// -#include "../AllocationState.h" +#include "AllocationState.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" @@ -26,9 +26,9 @@ using namespace clang; using namespace ento; namespace { -class PutenvWithAutoChecker : public Checker<check::PostCall> { +class PutenvStackArrayChecker : public Checker<check::PostCall> { private: - BugType BT{this, "'putenv' function should not be called with auto variables", + BugType BT{this, "'putenv' called with stack-allocated string", categories::SecurityError}; const CallDescription Putenv{CDM::CLibrary, {"putenv"}, 1}; @@ -37,8 +37,8 @@ class PutenvWithAutoChecker : public Checker<check::PostCall> { }; } // namespace -void PutenvWithAutoChecker::checkPostCall(const CallEvent &Call, - CheckerContext &C) const { +void PutenvStackArrayChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { if (!Putenv.matches(Call)) return; @@ -50,7 +50,7 @@ void PutenvWithAutoChecker::checkPostCall(const CallEvent &Call, return; StringRef ErrorMsg = "The 'putenv' function should not be called with " - "arguments that have automatic storage"; + "arrays that have automatic storage"; ExplodedNode *N = C.generateErrorNode(); auto Report = std::make_unique<PathSensitiveBugReport>(BT, ErrorMsg, N); @@ -60,8 +60,10 @@ void PutenvWithAutoChecker::checkPostCall(const CallEvent &Call, C.emitReport(std::move(Report)); } -void ento::registerPutenvWithAuto(CheckerManager &Mgr) { - Mgr.registerChecker<PutenvWithAutoChecker>(); +void ento::registerPutenvStackArray(CheckerManager &Mgr) { + Mgr.registerChecker<PutenvStackArrayChecker>(); } -bool ento::shouldRegisterPutenvWithAuto(const CheckerManager &) { return true; } +bool ento::shouldRegisterPutenvStackArray(const CheckerManager &) { + return true; +} diff --git a/clang/test/Analysis/cert/pos34-c-fp-suppression.cpp b/clang/test/Analysis/cert/pos34-c-fp-suppression.cpp deleted file mode 100644 index d982fcb8a1baf..0000000000000 --- a/clang/test/Analysis/cert/pos34-c-fp-suppression.cpp +++ /dev/null @@ -1,51 +0,0 @@ -// RUN: %clang_analyze_cc1 \ -// RUN: -analyzer-checker=alpha.security.cert.pos.34c\ -// RUN: -verify %s - -#include "../Inputs/system-header-simulator.h" -void free(void *memblock); -void *malloc(size_t size); -int putenv(char *); -int rand(); - -namespace test_auto_var_used_good { - -extern char *ex; -int test_extern() { - return putenv(ex); // no-warning: extern storage class. -} - -void foo(void) { - char *buffer = (char *)"huttah!"; - if (rand() % 2 == 0) { - buffer = (char *)malloc(5); - strcpy(buffer, "woot"); - } - putenv(buffer); -} - -void bar(void) { - char *buffer = (char *)malloc(5); - strcpy(buffer, "woot"); - - if (rand() % 2 == 0) { - free(buffer); - buffer = (char *)"blah blah blah"; - } - putenv(buffer); -} - -void baz() { - char env[] = "NAME=value"; - // TODO: False Positive - putenv(env); - // expected-warning@-1 {{The 'putenv' function should not be called with arguments that have automatic storage}} - - /* - DO SOMETHING - */ - - putenv((char *)"NAME=anothervalue"); -} - -} // namespace test_auto_var_used_good diff --git a/clang/test/Analysis/cert/pos34-c.cpp b/clang/test/Analysis/cert/pos34-c.cpp deleted file mode 100644 index f2bd7b393d889..0000000000000 --- a/clang/test/Analysis/cert/pos34-c.cpp +++ /dev/null @@ -1,61 +0,0 @@ -// RUN: %clang_analyze_cc1 \ -// RUN: -analyzer-checker=alpha.security.cert.pos.34c\ -// RUN: -verify %s - -// Examples from the CERT rule's page. -// https://wiki.sei.cmu.edu/confluence/x/6NYxBQ - -#include "../Inputs/system-header-simulator.h" -void free(void *memblock); -void *malloc(size_t size); -int putenv(char *); -int snprintf(char *str, size_t size, const char *format, ...); - -namespace test_auto_var_used_bad { - -int volatile_memory1(const char *var) { - char env[1024]; - int retval = snprintf(env, sizeof(env), "TEST=%s", var); - if (retval < 0 || (size_t)retval >= sizeof(env)) { - /* Handle error */ - } - - return putenv(env); - // expected-warning@-1 {{The 'putenv' function should not be called with arguments that have automatic storage}} -} - -} // namespace test_auto_var_used_bad - -namespace test_auto_var_used_good { - -int test_static(const char *var) { - static char env[1024]; - - int retval = snprintf(env, sizeof(env), "TEST=%s", var); - if (retval < 0 || (size_t)retval >= sizeof(env)) { - /* Handle error */ - } - - return putenv(env); -} - -int test_heap_memory(const char *var) { - static char *oldenv; - const char *env_format = "TEST=%s"; - const size_t len = strlen(var) + strlen(env_format); - char *env = (char *)malloc(len); - if (env == NULL) { - return -1; - } - if (putenv(env) != 0) { // no-warning: env was dynamically allocated. - free(env); - return -1; - } - if (oldenv != NULL) { - free(oldenv); /* avoid memory leak */ - } - oldenv = env; - return 0; -} - -} // namespace test_auto_var_used_good diff --git a/clang/test/Analysis/putenv-stack-array.c b/clang/test/Analysis/putenv-stack-array.c new file mode 100644 index 0000000000000..fbbf93259ab85 --- /dev/null +++ b/clang/test/Analysis/putenv-stack-array.c @@ -0,0 +1,70 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=alpha.security.PutenvStackArray \ +// RUN: -verify %s + +#include "Inputs/system-header-simulator.h" +void free(void *); +void *malloc(size_t); +int putenv(char *); +int snprintf(char *, size_t, const char *, ...); + +int test_auto_var(const char *var) { + char env[1024]; + (void)snprintf(env, sizeof(env), "TEST=%s", var); + return putenv(env); // expected-warning{{The 'putenv' function should not be called with arrays that have automatic storage}} +} + +int test_static_var(const char *var) { + static char env[1024]; + (void)snprintf(env, sizeof(env), "TEST=%s", var); + return putenv(env); // no-warning: static array is used +} + +void test_heap_memory(const char *var) { + const char *env_format = "TEST=%s"; + const size_t len = strlen(var) + strlen(env_format); + char *env = (char *)malloc(len); + if (env == NULL) + return; + if (putenv(env) != 0) // no-warning: env was dynamically allocated. + free(env); +} + +typedef struct { + int A; + char Env[1024]; +} Mem; + +int test_auto_var_struct() { + Mem mem; + return putenv(mem.Env); // expected-warning{{The 'putenv' function should not be called with}} +} + +int test_auto_var_subarray() { + char env[1024]; + return putenv(env + 100); // expected-warning{{The 'putenv' function should not be called with}} +} + +int test_constant() { + char *env = "TEST"; + return putenv(env); // no-warning: data is not on the stack +} + +extern char *ext_env; +int test_extern() { + return putenv(ext_env); // no-warning: extern storage class. +} + +void test_auto_var_reset() { + char env[] = "NAME=value"; + putenv(env); // expected-warning{{The 'putenv' function should not be called with}} + // ... (do something) + // Even cases like this are likely a bug: + // It looks like that if one string was passed to putenv, + // it should not be deallocated at all, because when reading the + // environment variable a pointer into this string is returned. + // In this case, if another (or the same) thread reads variable "NAME" + // at this point and does not copy the returned string, the data may + // become invalid. + putenv((char *)"NAME=anothervalue"); +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
