[clang] edde4ef - [analyzer] Introduce the assume-controlled-environment config option

[Balazs Benics via cfe-commits]

Author: Balazs Benics
Date: 2021-10-13T10:50:26+02:00
New Revision: edde4efc66df2257f0b2351d5f98b4fbb2ced620

URL: https://github.com/llvm/llvm-project/commit/edde4efc66df2257f0b2351d5f98b4fbb2ced620
DIFF: https://github.com/llvm/llvm-project/commit/edde4efc66df2257f0b2351d5f98b4fbb2ced620.diff

LOG: [analyzer] Introduce the assume-controlled-environment config option

If the `assume-controlled-environment` is `true`, we should expect `getenv()`
to succeed, and the result should not be considered tainted.
By default, the option will be `false`.

Reviewed By: NoQ, martong

Differential Revision: https://reviews.llvm.org/D111296

Added: 
    clang/test/Analysis/assume-controlled-environment.c

Modified: 
    clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
    clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
    clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
    clang/test/Analysis/analyzer-config.c

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
index 16ca7a8456e39..aab8e1284bf64 100644
--- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
+++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
@@ -328,6 +328,14 @@ ANALYZER_OPTION(
     "holds a single element.",
     false)
 
+ANALYZER_OPTION(
+    bool, ShouldAssumeControlledEnvironment, "assume-controlled-environment",
+    "Whether the analyzed application runs in a controlled environment. "
+    "We will assume that environment variables exist in queries and they hold "
+    "no malicious data. For instance, if this option is enabled, 'getenv()' "
+    "might be modeled by the analyzer to never return NULL.",
+    false)
+
 //===----------------------------------------------------------------------===//
 // Unsigned analyzer options.
 //===----------------------------------------------------------------------===//

diff  --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
index 42c777eb2c521..3fc046015f151 100644
--- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -435,7 +435,6 @@ GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule(
           .Case("getch", {{}, {ReturnValueIndex}})
           .Case("getchar", {{}, {ReturnValueIndex}})
           .Case("getchar_unlocked", {{}, {ReturnValueIndex}})
-          .Case("getenv", {{}, {ReturnValueIndex}})
           .Case("gets", {{}, {0, ReturnValueIndex}})
           .Case("scanf", {{}, {}, VariadicType::Dst, 1})
           .Case("socket", {{},
@@ -468,6 +467,16 @@ GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule(
 
   if (!Rule.isNull())
     return Rule;
+
+  // `getenv` returns taint only in untrusted environments.
+  if (FData.FullName == "getenv") {
+    if (C.getAnalysisManager()
+            .getAnalyzerOptions()
+            .ShouldAssumeControlledEnvironment)
+      return {};
+    return {{}, {ReturnValueIndex}};
+  }
+
   assert(FData.FDecl);
 
   // Check if it's one of the memory setting/copying functions.

diff  --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 74adc5882bfbf..e8b963a535d8b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -568,6 +568,7 @@ class StdLibraryFunctionsChecker
 
   bool DisplayLoadedSummaries = false;
   bool ModelPOSIX = false;
+  bool ShouldAssumeControlledEnvironment = false;
 
 private:
   Optional<Summary> findFunctionSummary(const FunctionDecl *FD,
@@ -1433,13 +1434,19 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
                 RetType{Ssize_tTy}),
       GetLineSummary);
 
-  // char *getenv(const char *name);
-  addToFunctionSummaryMap(
-      "getenv", Signature(ArgTypes{ConstCharPtrTy}, RetType{CharPtrTy}),
-      Summary(NoEvalCall)
-          .Case({NotNull(Ret)})
-          .Case({NotNull(Ret)->negate()})
-          .ArgConstraint(NotNull(ArgNo(0))));
+  {
+    Summary GetenvSummary = Summary(NoEvalCall)
+                                .ArgConstraint(NotNull(ArgNo(0)))
+                                .Case({NotNull(Ret)});
+    // In untrusted environments the envvar might not exist.
+    if (!ShouldAssumeControlledEnvironment)
+      GetenvSummary.Case({NotNull(Ret)->negate()});
+
+    // char *getenv(const char *name);
+    addToFunctionSummaryMap(
+        "getenv", Signature(ArgTypes{ConstCharPtrTy}, RetType{CharPtrTy}),
+        std::move(GetenvSummary));
+  }
 
   if (ModelPOSIX) {
 
@@ -2653,11 +2660,12 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
 
 void ento::registerStdCLibraryFunctionsChecker(CheckerManager &mgr) {
   auto *Checker = mgr.registerChecker<StdLibraryFunctionsChecker>();
+  const AnalyzerOptions &Opts = mgr.getAnalyzerOptions();
   Checker->DisplayLoadedSummaries =
-      mgr.getAnalyzerOptions().getCheckerBooleanOption(
-          Checker, "DisplayLoadedSummaries");
-  Checker->ModelPOSIX =
-      mgr.getAnalyzerOptions().getCheckerBooleanOption(Checker, "ModelPOSIX");
+      Opts.getCheckerBooleanOption(Checker, "DisplayLoadedSummaries");
+  Checker->ModelPOSIX = Opts.getCheckerBooleanOption(Checker, "ModelPOSIX");
+  Checker->ShouldAssumeControlledEnvironment =
+      Opts.ShouldAssumeControlledEnvironment;
 }
 
 bool ento::shouldRegisterStdCLibraryFunctionsChecker(

diff  --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c
index 4cca1cb553b62..429229f6b6ca4 100644
--- a/clang/test/Analysis/analyzer-config.c
+++ b/clang/test/Analysis/analyzer-config.c
@@ -15,6 +15,7 @@
 // CHECK-NEXT: apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries = false
 // CHECK-NEXT: apiModeling.StdCLibraryFunctions:ModelPOSIX = false
 // CHECK-NEXT: apply-fixits = false
+// CHECK-NEXT: assume-controlled-environment = false
 // CHECK-NEXT: avoid-suppressing-null-argument-paths = false
 // CHECK-NEXT: c++-allocator-inlining = true
 // CHECK-NEXT: c++-container-inlining = false

diff  --git a/clang/test/Analysis/assume-controlled-environment.c b/clang/test/Analysis/assume-controlled-environment.c
new file mode 100644
index 0000000000000..749b8198f6fb0
--- /dev/null
+++ b/clang/test/Analysis/assume-controlled-environment.c
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -verify=untrusted-env %s \
+// RUN:   -analyzer-checker=core                    \
+// RUN:   -analyzer-checker=alpha.security.taint    \
+// RUN:   -analyzer-checker=debug.TaintTest
+
+// RUN: %clang_analyze_cc1 -verify %s -DEXPECT_NO_WARNINGS    \
+// RUN:   -analyzer-config assume-controlled-environment=true \
+// RUN:   -analyzer-checker=core                              \
+// RUN:   -analyzer-checker=alpha.security.taint              \
+// RUN:   -analyzer-checker=debug.TaintTest
+
+
+#ifdef EXPECT_NO_WARNINGS
+// expected-no-diagnostics
+#endif
+
+char *getenv(const char *name);
+
+void foo() {
+  char *p = getenv("FOO"); // untrusted-env-warning {{tainted}}
+  (void)p;                 // untrusted-env-warning {{tainted}}
+}