[analyzer] CERT: POS34-C

[zukatsinadze]

Summary

This patch introduces a new checker:
alpha.security.cert.pos.34c

This checker is implemented based on the following rule:
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
The check warns if putenv function is
called with automatic storage variable as an argument.

[Charusso]

clang/docs/analyzer/checkers.rst

1954

I would remove that line.

1966

I think it is clear it is a CERT-checker.

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

839

I would write `putenv` -> 'putenv' and the CERT rule-number should be clear from that point so you could emit it.

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

53

I think you could shrink that into:

!IsPossiblyAutoVar && !isa<StackSapceRegion>(MSR)

What you have specified is a negation of

!isa<StackSapceRegion>(MSR) && !isa<CodeSpaceRegion>(MSR) && !isa<GlobalInternalSpaceRegion>(MSR)

~ according to: https://clang.llvm.org/doxygen/classclang_1_1ento_1_1MemSpaceRegion.html

where the CodeSpaceRegion and GlobalInternalSpaceRegion is not that interesting for the checker.

59

@NoQ, it is from your documentation. Would we prefer that or the registerChecker(CheckerManager &) is the new way to construct the BugType? I believe the latter is more appropriate.

61

We would like to point out the allocation's site, where it comes from.
We have two facilities to do so: MallocBugVisitor to track dynamic allocation and trackExpressionValue() to track any kind of an expression.

You could find examples from my CERT-checker: D70411. The rough idea is that:

// Track the argument.
if (isa<StackSpaceRegion>(MSR)) {
  bugreporter::trackExpressionValue(C.getPredecessor(), ArgExpr, *Report);
} else if (const SymbolRef Sym = ArgV.getAsSymbol()) { // It is a `HeapSpaceRegion`
  Report->addVisitor(allocation_state::getMallocBRVisitor(Sym));
}

~ here you need to copy-paste the getMallocBRVisitor() from my review. Sorry!

clang/test/Analysis/cert/pos34-c.cpp

5

Could you inject the rule's page please?

14

I would name the rule properly, as a sentence like: Example from the CERT rule's page.

My idea was that to have a /cert/pos34-c.cpp which only consists of the rule's page stuff mentioning the rule's page on the top. And having a separate file which only consists of arbitrary tests called like /cert/pos34-c-fp-suppression.cpp as it shows some false-positive suppression what you have found on tests or something. Like @aaron.ballman wrote two tests in D70823 and I would copy-paste them from his comment.

[NoQ]

Thanks! This looks like a simple and efficient check. I have one overall suggestion.

Currently the check may warn on non-bugs of the following kind:

void foo() {
  char env[] = "NAME=value";
  putenv(env);
  doStuff();
  putenv("NAME=anothervalue");
}

I.e., it's obviously harmless if the local variable pointer is removed from the environment before it goes out of scope. Can we instead warn when the *last* putenv() on the execution path through the current stack frame is a local variable (that goes out of scope at the end of the stack frame)?

That'd allow the checker to be enabled by default, which will give a lot more users access to it. Otherwise we'll have to treat it as an opt-in check and users will only enable it when they know about it, which is much less usefulness.

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

28

The modern idiom is BugType BT{this, "Bug kind", "Bug category"}; - and drop the lazy initialization as well.

36–38

The modern idiom here is CallDescription.

43–44

You can call getMemorySpace() directly on any region.

46–53

Simply check whether the memory space is a stack memory space. This should be a one-liner.

59

Replied above^^

61

I'm not sure it's valid to use C.getPredecessor() for tracking; it might get you into trouble for the same reason that using C.getPredecessor() as error node will get you into trouble. Please use the error node itself instead.

[zukatsinadze]

Addressed most of the inline comments.

[zukatsinadze]

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

839

Oops. Forgot this one. Will fix it later.

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

46–53

I could not get rid of isPossiblyAutoVar and GlobalInternalSpaceRegion.

clang/test/Analysis/cert/pos34-c-fp-suppression.cpp

16

I need isPossiblyAutoVar for this type.

21

And GlobalInternalSpaceRegion for this.

[zukatsinadze]

In D71433#1784238, @NoQ wrote:

Thanks! This looks like a simple and efficient check. I have one overall suggestion.

Currently the check may warn on non-bugs of the following kind:

void foo() {
  char env[] = "NAME=value";
  putenv(env);
  doStuff();
  putenv("NAME=anothervalue");
}

I.e., it's obviously harmless if the local variable pointer is removed from the environment before it goes out of scope. Can we instead warn when the *last* putenv() on the execution path through the current stack frame is a local variable (that goes out of scope at the end of the stack frame)?

That'd allow the checker to be enabled by default, which will give a lot more users access to it. Otherwise we'll have to treat it as an opt-in check and users will only enable it when they know about it, which is much less usefulness.

@NoQ I like the idea, but I am not really sure how to do that. I started working on Static Analyzer just lask week.

[NoQ]

In D71433#1784920, @zukatsinadze wrote:

@NoQ I like the idea, but I am not really sure how to do that. I started working on Static Analyzer just lask week.

Let's get the initial attempt right first, and delay this for the next patch. You could accomplish this by keeping track of the last putenv() in a program state trait and moving the warning in checkEndFunction().

clang/test/Analysis/cert/pos34-c-fp-suppression.cpp

16

This test is pretty questionable. There is no indication in the code that a points to an automatic variable.

21

This test is wrong. "hello" is not an automatic variable.

[zukatsinadze]

• Removed extra test
• Used CallDescription for checking call.

[Charusso]

I have created a notes.cpp test-file to test the notes in my checkers, but I think this checker is fine without that test file. @NoQ, what do you think?

[Charusso]

In D71433#1784238, @NoQ wrote:

Currently the check may warn on non-bugs of the following kind:

void foo() {
  char env[] = "NAME=value";
  putenv(env);
  doStuff();
  putenv("NAME=anothervalue");
}

That could be the next round as a follow-up patch as the next semester starts in February after the 10.0.0 release-tag has been set, so both of the patches could land in LLVM 11. We have not seen any putenv() in the wild yet.

[NoQ]

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

58–59

This is impossible because StackSpaceRegion and HeapSpaceRegion do not overlap and above you checked that it's the former.

clang/test/Analysis/cert/pos34-c.cpp

7

Btw - CERT has minified links!

[zukatsinadze]

Addressed new inline comments.

[Charusso]

In D71433#1808316, @Charusso wrote:

In D71433#1784238, @NoQ wrote:

Currently the check may warn on non-bugs of the following kind:

void foo() {
  char env[] = "NAME=value";
  putenv(env);
  doStuff();
  putenv("NAME=anothervalue");
}

That could be the next round as a follow-up patch as the next semester starts in February [...]

Well, the next semester is about to start. Could you implement that request please?

[zukatsinadze]

In D71433#1863638, @Charusso wrote:

In D71433#1808316, @Charusso wrote:

In D71433#1784238, @NoQ wrote:

Currently the check may warn on non-bugs of the following kind:

void foo() {
  char env[] = "NAME=value";
  putenv(env);
  doStuff();
  putenv("NAME=anothervalue");
}

That could be the next round as a follow-up patch as the next semester starts in February [...]

Well, the next semester is about to start. Could you implement that request please?

Sure. I plan to start working on it next week.

[Szelethus]

This is a very neat checker, the source code reads so easily, we might as well put it as the official CERT rule description.

I think adding the non-compliant and compliant code examples would be nice. I also wrote some inline comments, but I'm fine with leaving them for a later patch. LGTM, granted that @NoQ and @Charusso are happy.

clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h

16–22

We could turns these into llvm::StringLiterals one day.

clang/lib/StaticAnalyzer/Checkers/AllocationState.h

28–30 ↗

(On Diff #243127)

Is this deadcode now?

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

3383–3386 ↗

(On Diff #243127)

And this?

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

51

How about The 'putenv' function should not be called with arguments that have automatic storage. But I guess we could leave wordsmithing for later.

56

This should always be true, since we bail out a couple lines up if it isn't, right?

[zukatsinadze]

• Removed dead code.
• Removed unnecessary if condition.
• Changed error phrasing.

[zukatsinadze]

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

3383–3386 ↗

(On Diff #243127)

Forgot to delete. Thanks

[Szelethus]

I think for an alpha checker this is ready to land if you're ready -- do you have commit access or need assistance?

[zukatsinadze]

In D71433#1880436, @Szelethus wrote:

I think for an alpha checker this is ready to land if you're ready -- do you have commit access or need assistance?

Thank you. @Charusso will help.

clang/docs/analyzer/checkers.rst

if (reminderCount == 1) {
    reminderText =
      [NSString stringWithFormat:
        NSLocalizedString(@"%@ Reminders", @"Indicates multiple reminders"),
          reminderCount];
  }
 
 alpha.security
 ^^^^^^^^^^^^^^
+
+
+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
+  }
+  
 .. _alpha-security-ArrayBound:
 
 alpha.security.ArrayBound (C)
 """""""""""""""""""""""""""""
 Warn about buffer overflows (older checker).
 
 .. code-block:: c
 
  void test() {
    char *s = "";
    char c = s[1]; // warn

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

 
 def Performance : Package<"performance">, ParentPackage<OptIn>;
 
 def Security : Package <"security">;
 def InsecureAPI : Package<"insecureAPI">, ParentPackage<Security>;
 def SecurityAlpha : Package<"security">, ParentPackage<Alpha>;
 def Taint : Package<"taint">, ParentPackage<SecurityAlpha>;
 
+def CERT : Package<"cert">, ParentPackage<SecurityAlpha>;
+def POS : Package<"pos">, ParentPackage<CERT>;
+
 def Unix : Package<"unix">;
 def UnixAlpha : Package<"unix">, ParentPackage<Alpha>;
 def CString : Package<"cstring">, ParentPackage<Unix>;
 def CStringAlpha : Package<"cstring">, ParentPackage<UnixAlpha>;
 
 def OSX : Package<"osx">;
 def OSXAlpha : Package<"osx">, ParentPackage<Alpha>;
 def OSXOptIn : Package<"osx">, ParentPackage<OptIn>;

 def FloatLoopCounter : Checker<"FloatLoopCounter">,
   HelpText<"Warn on using a floating point value as a loop counter (CERT: "
            "FLP30-C, FLP30-CPP)">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation<HasDocumentation>;
 
 } // end "security"
 
+let ParentPackage = POS 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">,
   HelpText<"Warn about buffer overflows (older checker)">,
   Documentation<HasAlphaDocumentation>;
 
 def ArrayBoundCheckerV2 : Checker<"ArrayBoundV2">,
   HelpText<"Warn about buffer overflows (newer checker)">,

clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h

 //=--- CommonBugCategories.h - Provides common issue categories -*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_COMMONBUGCATEGORIES_H
 #define LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_COMMONBUGCATEGORIES_H
 
 // Common strings used for the "category" of many static analyzer issues.
 namespace clang {
-  namespace ento {
-    namespace categories {
-      extern const char * const CoreFoundationObjectiveC;
-      extern const char * const LogicError;
-      extern const char * const MemoryRefCount;
-      extern const char * const MemoryError;
-      extern const char * const UnixAPI;
-      extern const char * const CXXObjectLifecycle;
-    }
-  }
-}
+extern const char *const CoreFoundationObjectiveC;
+extern const char *const LogicError;
+extern const char *const MemoryRefCount;
+extern const char *const MemoryError;
+extern const char *const UnixAPI;
+extern const char *const CXXObjectLifecycle;
+extern const char *const SecurityError;
+} // namespace categories
+} // namespace ento
+} // namespace clang
 #endif
-

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   ObjCUnusedIVarsChecker.cpp
   OSObjectCStyleCast.cpp
   PaddingChecker.cpp
   PointerArithChecker.cpp
   PointerIterationChecker.cpp
   PointerSortingChecker.cpp
   PointerSubChecker.cpp
   PthreadLockChecker.cpp
+  cert/PutenvWithAutoChecker.cpp
   RetainCountChecker/RetainCountChecker.cpp
   RetainCountChecker/RetainCountDiagnostics.cpp
   ReturnPointerRangeChecker.cpp
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrModeling.cpp

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

+//== PutenvWithAutoChecker.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.
+// https://wiki.sei.cmu.edu/confluence/x/6NYxBQ
+//
+//===----------------------------------------------------------------------===//
+
+#include "AllocationState.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
+
+using namespace clang;
+using namespace ento;
+
+class PutenvWithAutoChecker : public Checker<check::PostCall> {
+private:
+  BugType BT{this, "'putenv' function should not be called with auto variables",
+             categories::SecurityError};
+  const CallDescription Putenv{"putenv", 1};
+
+public:
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+};
+
+void PutenvWithAutoChecker::checkPostCall(const CallEvent &Call,
+                                          CheckerContext &C) const {
+  if (!Call.isCalled(Putenv))
+    return;
+
+  SVal ArgV = Call.getArgSVal(0);
+  const Expr *ArgExpr = Call.getArgExpr(0);
+  const MemSpaceRegion *MSR = ArgV.getAsRegion()->getMemorySpace();
+
+  if (!isa<StackSpaceRegion>(MSR))
+    return;
+
+  StringRef ErrorMsg = "The 'putenv' function should not be called with "
+                       "arguments that have automatic storage";
+  ExplodedNode *N = C.generateErrorNode();
+  auto Report = std::make_unique<PathSensitiveBugReport>(BT, ErrorMsg, N);
+
+  // Track the argument.
+  bugreporter::trackExpressionValue(Report->getErrorNode(), ArgExpr, *Report);
+
+  C.emitReport(std::move(Report));
+}
+
+void ento::registerPutenvWithAuto(CheckerManager &Mgr) {
+  Mgr.registerChecker<PutenvWithAutoChecker>();
+}
+
+bool ento::shouldRegisterPutenvWithAuto(const LangOptions &) { return true; }

clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp

 //=--- CommonBugCategories.cpp - Provides common issue categories -*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 
 // Common strings used for the "category" of many static analyzer issues.
-namespace clang { namespace ento { namespace categories {
+namespace clang {
+namespace ento {
+namespace categories {
 
-const char * const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
-const char * const LogicError = "Logic error";
-const char * const MemoryRefCount =
+const char *const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
+const char *const LogicError = "Logic error";
+const char *const MemoryRefCount =
     "Memory (Core Foundation/Objective-C/OSObject)";
-const char * const MemoryError = "Memory error";
-const char * const UnixAPI = "Unix API";
-const char * const CXXObjectLifecycle = "C++ object lifecycle";
-}}}
+const char *const MemoryError = "Memory error";
+const char *const UnixAPI = "Unix API";
+const char *const CXXObjectLifecycle = "C++ object lifecycle";
+const char *const SecurityError = "Security error";
+} // namespace categories
+} // namespace ento
+} // namespace clang

clang/test/Analysis/cert/pos34-c-fp-suppression.cpp

+// 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

clang/test/Analysis/cert/pos34-c.cpp

+// 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