[Analyzer] Add bug visitor for taint checker

[vlad.tsyrklevich]

Summary

Add a bug visitor to the taint checker to make it easy to distinguish where the tainted value originated. This is especially useful when the original taint source is obscured by complex data flow.

[NoQ]

Yay, this is awesome!

It's actually possible to test visitors with the -analyzer-output=text option. This option converts path notes to note: diagnostics, which you can catch with expected-note{{}}, see test/Analysis/inlining/path-notes.c for an example (well, it's also possible to test this with other -analyzer-output variants, but that'd be an overkill).

lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

109

Could you call this V? Because S is often a statement pointer (so often that you actually shadow this member with a local statement later).

681

C.getSVal(E)

Promoting the new fancy helper function^^

[vlad.tsyrklevich]

Fixes and a test for Artem's suggestions.

[zaks.anna]

Looks great!

lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

private:
   static const char MsgTaintedBufferSize[];
   bool checkTaintedBufferSize(const CallExpr *CE, const FunctionDecl *FDecl,
                               CheckerContext &C) const;
 
   /// Generate a report if the expression is tainted or points to tainted data.
   bool generateReportIfTainted(const Expr *E, const char Msg[],
                                CheckerContext &C) const;
 
+  /// The bug visitor prints a diagnostic message at the location where a given
+  /// variable was tainted.
+  class TaintBugVisitor
+      : public BugReporterVisitorImpl<TaintBugVisitor> {
+  private:
+    const SVal V;
+
+  public:
+    TaintBugVisitor(const SVal V) : V(V) {}
+    void Profile(llvm::FoldingSetNodeID &ID) const override { ID.Add(V); }
+
+    std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+                                                   const ExplodedNode *PrevN,
+                                                   BugReporterContext &BRC,
+                                                   BugReport &BR) override;
+  };
 
   typedef SmallVector<unsigned, 2> ArgVector;
 
   /// \brief A struct used to specify taint propagation rules for a function.
   ///
   /// If any of the possible taint source arguments is tainted, all of the
   /// destination arguments should also be tainted. Use InvalidArgIndex in the
   /// src list to specify that all of the arguments can introduce taint. Use

 } // end of anonymous namespace
 
 /// A set which is used to pass information from call pre-visit instruction
 /// to the call post-visit. The values are unsigned integers, which are either
 /// ReturnValueIndex, or indexes of the pointer/reference argument, which
 /// points to data, which should be tainted on return.
 REGISTER_SET_WITH_PROGRAMSTATE(TaintArgsOnPostVisit, unsigned)
 
+std::shared_ptr<PathDiagnosticPiece>
+GenericTaintChecker::TaintBugVisitor::VisitNode(const ExplodedNode *N,
+    const ExplodedNode *PrevN, BugReporterContext &BRC, BugReport &BR) {
+
+  // Find the ExplodedNode where the taint was first introduced
+  if (!N->getState()->isTainted(V) || PrevN->getState()->isTainted(V))
+    return nullptr;
+
+  const Stmt *S = PathDiagnosticLocation::getStmt(N);
+  if (!S)
+    return nullptr;
+
+  const LocationContext *NCtx = N->getLocationContext();
+  PathDiagnosticLocation L =
+      PathDiagnosticLocation::createBegin(S, BRC.getSourceManager(), NCtx);
+  if (!L.isValid() || !L.asLocation().isValid())
+    return nullptr;
+
+  return std::make_shared<PathDiagnosticEventPiece>(
+      L, "Taint originated here");
+}
+
 GenericTaintChecker::TaintPropagationRule
 GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule(
                                                      const FunctionDecl *FDecl,
                                                      StringRef Name,
                                                      CheckerContext &C) {
   // TODO: Currently, we might lose precision here: we always mark a return
   // value as tainted even if it's just a pointer, pointing to tainted data.
 

 
 bool GenericTaintChecker::generateReportIfTainted(const Expr *E,
                                                   const char Msg[],
                                                   CheckerContext &C) const {
   assert(E);
 
   // Check for taint.
   ProgramStateRef State = C.getState();
-  if (!State->isTainted(getPointedToSymbol(C, E)) &&
-      !State->isTainted(E, C.getLocationContext()))
+  const SymbolRef PointedToSym = getPointedToSymbol(C, E);
+  SVal TaintedSVal;
+  if (State->isTainted(PointedToSym))
+    TaintedSVal = nonloc::SymbolVal(PointedToSym);
+  else if (State->isTainted(E, C.getLocationContext()))
+    TaintedSVal = C.getSVal(E);
+  else
     return false;
 
   // Generate diagnostic.
   if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
     initBugType();
     auto report = llvm::make_unique<BugReport>(*BT, Msg, N);
     report->addRange(E->getSourceRange());
+    report->addVisitor(llvm::make_unique<TaintBugVisitor>(TaintedSVal));
     C.emitReport(std::move(report));
     return true;
   }
   return false;
 }
 
 bool GenericTaintChecker::checkUncontrolledFormatString(const CallExpr *CE,
                                                         CheckerContext &C) const{

test/Analysis/taint-diagnostic-visitor.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core -analyzer-output=text -verify %s
+
+// This file is for testing enhanced diagnostics produced by the GenericTaintChecker
+
+int scanf(const char *restrict format, ...);
+int system(const char *command);
+
+void taintDiagnostic()
+{
+  char buf[128];
+  scanf("%s", buf); // expected-note {{Taint originated here}}
+  system(buf); // expected-warning {{Untrusted data is passed to a system call}} // expected-note {{Untrusted data is passed to a system call (CERT/STR02-C. Sanitize data passed to complex subsystems)}}
+}