[clang][analyzer] Move checker 'cert.pos.34c' (in alpha.security) into 'PutenvStackArray'

clang/docs/analyzer/checkers.rst

@@ -2792,6 +2792,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)
@@ -2818,55 +2843,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
 ^^^^^^^^^^^^^^^^^^^^
 

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

@@ -1030,15 +1030,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">,
@@ -1049,10 +1040,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>;
@@ -1073,6 +1060,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"
 
 //===----------------------------------------------------------------------===//

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

clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp → clang/lib/StaticAnalyzer/Checkers/PutenvStackArrayChecker.cpp

@@ -1,18 +1,18 @@
-//== 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.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
-// 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;
+}

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");
+}