[analyzer] Add failing test case demonstrating buggy taint propagation

Recently we uncovered a serious bug in the `GenericTaintChecker`.
It was already flawed before D116025, but that was the patch that turned
this silent bug into a crash.

It happens if the `GenericTaintChecker` has a rule for a function, which
also has a definition.

  char *fgets(char *s, int n, FILE *fp) {
    nested_call();   // no parameters!
    return (char *)0;
  }

  // Within some function:
  fgets(..., tainted_fd);

When the engine inlines the definition and finds a function call within
that, the `PostCall` event for the call will get triggered sooner than the
`PostCall` for the original function.
This mismatch violates the assumption of the `GenericTaintChecker` which
wants to propagate taint information from the `PreCall` event to the
`PostCall` event, where it can actually bind taint to the return value
**of the same call**.

Let's get back to the example and go through step-by-step.
The `GenericTaintChecker` will see the `PreCall<fgets(..., tainted_fd)>`
event, so it would 'remember' that it needs to taint the return value
and the buffer, from the `PostCall` handler, where it has access to the
return value symbol.
However, the engine will inline fgets and the `nested_call()` gets
evaluated subsequently, which produces an unimportant
`PreCall<nested_call()>`, then a `PostCall<nested_call()>` event, which is
observed by the `GenericTaintChecker`, which will unconditionally mark
tainted the 'remembered' arg indexes, trying to access a non-existing
argument, resulting in a crash.
If it doesn't crash, it will behave completely unintuitively, by marking
completely unrelated memory regions tainted, which is even worse.

The resulting assertion is something like this:
  Expr.h: const Expr *CallExpr::getArg(unsigned int) const: Assertion
          `Arg < getNumArgs() && "Arg access out of range!"' failed.

The gist of the backtrace:
  CallExpr::getArg(unsigned int) const
  SimpleFunctionCall::getArgExpr(unsigned int)
  CallEvent::getArgSVal(unsigned int) const
  GenericTaintChecker::checkPostCall(const CallEvent &, CheckerContext&) const

Prior to D116025, there was a check for the argument count before it
applied taint, however, it still suffered from the same underlying
issue/bug regarding propagation.

This path does not intend to fix the bug, rather start a discussion on
how to fix this.

---

Let me elaborate on how I see this problem.

This pre-call, post-call juggling is just a workaround.
The engine should by itself propagate taint where necessary right where
it invalidates regions.
For the tracked values, which potentially escape, we need to erase the
information we know about them; and this is exactly what is done by
invalidation.
However, in the case of taint, we basically want to approximate from the
opposite side of the spectrum.
We want to preserve taint in most cases, rather than cleansing them.

Now, we basically sanitize all escaping tainted regions implicitly,
since invalidation binds a fresh conjured symbol for the given region,
and that has not been associated with taint.

IMO this is a bad default behavior, we should be more aggressive about
preserving taint if not further spreading taint to the reachable
regions.

We have a couple of options for dealing with it (let's call it //tainting
policy//):
  1) Taint only the parameters which were tainted prior to the call.
  2) Taint the return value of the call, since it likely depends on the
     tainted input - if any arguments were tainted.
  3) Taint all escaped regions - (maybe transitively using the cluster
     algorithm) - if any arguments were tainted.
  4) Not taint anything - this is what we do right now :D

The `ExprEngine` should not deal with taint on its own. It should be done
by a checker, such as the `GenericTaintChecker`.
However, the `Pre`-`PostCall` checker callbacks are not designed for this.
`RegionChanges` would be a much better fit for modeling taint propagation.
What we would need in the `RegionChanges` callback is the `State` prior
invalidation, the `State` after the invalidation, and a `CheckerContext` in
which the checker can create transitions, where it would place `NoteTags`
for the modeled taint propagations and report errors if a taint sink
rule gets violated.
In this callback, we could query from the prior State, if the given
value was tainted; then act and taint if necessary according to the
checker's tainting policy.

By using RegionChanges for this, we would 'fix' the mentioned
propagation bug 'by-design'.

Reviewed By: Szelethus

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

Author: Balazs Benics

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

@@ -32,6 +32,8 @@
 #include <memory>
 #include <utility>
 
+#define DEBUG_TYPE "taint-checker"
+
 using namespace clang;
 using namespace ento;
 using namespace taint;
@@ -691,6 +693,13 @@ void GenericTaintChecker::checkPostCall(const CallEvent &Call,
   if (TaintArgs.isEmpty())
     return;
 
+  LLVM_DEBUG(for (ArgIdxTy I
+                  : TaintArgs) {
+    llvm::dbgs() << "PostCall<";
+    Call.dump(llvm::dbgs());
+    llvm::dbgs() << "> actually wants to taint arg index: " << I << '\n';
+  });
+
   for (ArgIdxTy ArgNum : TaintArgs) {
     // Special handling for the tainted return value.
     if (ArgNum == ReturnValueIndex) {
@@ -768,15 +777,25 @@ void GenericTaintRule::process(const GenericTaintChecker &Checker,
 
   /// Propagate taint where it is necessary.
   ForEachCallArg(
-      [this, &State, WouldEscape](ArgIdxTy I, const Expr *E, SVal V) {
-        if (PropDstArgs.contains(I))
+      [this, &State, WouldEscape, &Call](ArgIdxTy I, const Expr *E, SVal V) {
+        if (PropDstArgs.contains(I)) {
+          LLVM_DEBUG(llvm::dbgs() << "PreCall<"; Call.dump(llvm::dbgs());
+                     llvm::dbgs()
+                     << "> prepares tainting arg index: " << I << '\n';);
           State = State->add<TaintArgsOnPostVisit>(I);
+        }
 
         // TODO: We should traverse all reachable memory regions via the
         // escaping parameter. Instead of doing that we simply mark only the
         // referred memory region as tainted.
-        if (WouldEscape(V, E->getType()))
+        if (WouldEscape(V, E->getType())) {
+          LLVM_DEBUG(if (!State->contains<TaintArgsOnPostVisit>(I)) {
+            llvm::dbgs() << "PreCall<";
+            Call.dump(llvm::dbgs());
+            llvm::dbgs() << "> prepares tainting arg index: " << I << '\n';
+          });
           State = State->add<TaintArgsOnPostVisit>(I);
+        }
       });
 
   C.addTransition(State);

clang/test/Analysis/taint-checker-callback-order-has-definition.c

@@ -0,0 +1,42 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,alpha.security.taint \
+// RUN:   -mllvm -debug-only=taint-checker \
+// RUN:   2>&1 | FileCheck %s
+
+// FIXME: We should not crash.
+// XFAIL: *
+
+struct _IO_FILE;
+typedef struct _IO_FILE FILE;
+FILE *fopen(const char *fname, const char *mode);
+
+void nested_call(void) {}
+
+char *fgets(char *s, int n, FILE *fp) {
+  nested_call();   // no-crash: we should not try adding taint to a non-existent argument.
+  return (char *)0;
+}
+
+void top(const char *fname, char *buf) {
+  FILE *fp = fopen(fname, "r");
+  // CHECK:      PreCall<fopen(fname, "r")> prepares tainting arg index: -1
+  // CHECK-NEXT: PostCall<fopen(fname, "r")> actually wants to taint arg index: -1
+
+  if (!fp)
+    return;
+
+  (void)fgets(buf, 42, fp); // Trigger taint propagation.
+  // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: -1
+  // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 0
+  // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 1
+  // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 2
+
+  // FIXME: We should propagate taint from PreCall<fgets> -> PostCall<fgets>.
+  // CHECK-NEXT: PostCall<nested_call()> actually wants to taint arg index: -1
+  // CHECK-NEXT: PostCall<nested_call()> actually wants to taint arg index: 0
+  // CHECK-NEXT: PostCall<nested_call()> actually wants to taint arg index: 1
+  // CHECK-NEXT: PostCall<nested_call()> actually wants to taint arg index: 2
+
+  // FIXME: We should not crash.
+  // CHECK: PLEASE submit a bug report
+}

clang/test/Analysis/taint-checker-callback-order-without-definition.c

@@ -0,0 +1,34 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core,alpha.security.taint \
+// RUN:   -mllvm -debug-only=taint-checker \
+// RUN:   2>&1 | FileCheck %s
+
+struct _IO_FILE;
+typedef struct _IO_FILE FILE;
+FILE *fopen(const char *fname, const char *mode);
+
+char *fgets(char *s, int n, FILE *fp); // no-definition
+
+void top(const char *fname, char *buf) {
+  FILE *fp = fopen(fname, "r"); // Introduce taint.
+  // CHECK:      PreCall<fopen(fname, "r")> prepares tainting arg index: -1
+  // CHECK-NEXT: PostCall<fopen(fname, "r")> actually wants to taint arg index: -1
+
+  if (!fp)
+    return;
+
+  (void)fgets(buf, 42, fp); // Trigger taint propagation.
+
+  // FIXME: Why is the arg index 1 prepared for taint?
+  // Before the call it wasn't tainted, and it also shouldn't be tainted after the call.
+
+  // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: -1
+  // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 0
+  // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 1
+  // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 2
+  //
+  // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: -1
+  // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 0
+  // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 1
+  // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 2
+}