[analyzer] Use the custom propagation rules and sinks in GenericTaintChecker

[boga95]

Summary

The TaintPropagationRule deduction uses the custom rules from the config.
Check custom sinks when looking for errors. Give an error when at least one of the specified arguments is tainted.
Emit error message and omit a parameter if it's out of bound.

[boga95]

Rebase after https://reviews.llvm.org/D59861.

[Szelethus]

In general, don't emit to stderr unless we either emit a warning/error about the incorrect configuration. As an experiment, what happens when you try to emit an error in the middle of the symbolic execution? You can retrieve a DiagnosticsEngine from any decl: D->getASTContext().getDiagnostics() (it's funny how you can retrieve almost all major manager objects if you try hard enough).

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

143

How about llvm::StringLiteral?

820

Hmmm, how do we do with qualified names (MyClass::generateTaint(), std::cin >>)?

[boga95]

I think it shouldn't give compile error in case of incorrect configuration now (maybe warning) because:

• Without qualified names, I can create a code which cannot be configured properly.

• It can throw an error without configuration, for example:

void read(int*); // There is an existing propagation rule for it

I suggest to let it unchanged now, and I will change it when the checker can handle qualified names.
On the other hand, I think we should make this type of error configurable (from the command line). So the user can select between warnings and errors.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

820

These patches focus on C style functions. I have implemented the uses of qualified names, but I intended to make a separate patch for that.

[boga95]

Should I do anything or it is ready?

[NoQ]

I have a few immediate style issues but otherwise it looks good :)

lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

237 ↗

(On Diff #215268)

StringLiterals need to always be declared as constexpr.

251 ↗

(On Diff #215268)

It should be "user-defined" because it's a single adjective.

I recommend against using the word "sink" in user-facing messages because it's too jargony. Do we have a better word for this?

252 ↗

(On Diff #215268)

This semicolon looks unnecessary.

595–596 ↗

(On Diff #215268)

These debug prints should not land.

840 ↗

(On Diff #215268)

The GenericTaintChecker:: qualifier is unnecessary.

[Szelethus]

Some nits inline, otherwise LGTM. @steakhal, do you have anything to add to this?

lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

191 ↗

(On Diff #218395)

How about only passing CustomPropagations?

605 ↗

(On Diff #218395)

I get that there isn't much substance to this assert, but why remove it? We might as well populate the lines in between that and the branch.

844 ↗

(On Diff #218395)

Wasn't this commited before?

[Szelethus]

Also, please mark inlines done as you fix them :)

[steakhal]

lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

191 ↗

(On Diff #218395)

I would even consider to move this function out of the whole class. (Not only this function, but the others as well. Like isStdin, etc.)
I think pure, free-functions (in an anonymous namespace) are easier to reason about.

844 ↗

(On Diff #218395)

Yes it was (https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp#L814).
I would kindly request a rebase.

test/Analysis/taint-generic.c

377 ↗

(On Diff #218395)

We could use this syntacs to achieve shorter lines. Note that @-1. Same for all the other lines.

mySink(x, 1, 2);
// expected-warning@-1 {{Untrusted data is passed to a user-defined sink}}

[Szelethus]

I'm fine with moving in-class function into anonymous namespace later. LGTM, feel free to commit!

[steakhal]

lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

118 ↗

(On Diff #218395)

Shouldn't we still need an out-of-class initializer part for each static constexpr class member variable?
These would provide the memory locations for the declarations.

constexpr llvm::StringLiteral GenericTaintChecker::MsgUncontrolledFormatString;
constexpr llvm::StringLiteral GenericTaintChecker::MsgSanitizeSystemArgs;
constexpr llvm::StringLiteral GenericTaintChecker::MsgTaintedBufferSize;
constexpr llvm::StringLiteral GenericTaintChecker::MsgCustomSink;

[boga95]

Closed by 080ecafdd8b3e990e5ad19202d089c91c9c9b164.

lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

118 ↗

(On Diff #218395)

Constexpr values cannot be initialized out of the class, that's why I moved them to here.

191 ↗

(On Diff #218395)

I could do that in a separate patch if necessary.

605 ↗

(On Diff #218395)

I think there is no need for this. Maybe I can make the ArgNum const.

251 ↗

(On Diff #215268)

Currently, I don't have any better idea. I will think about it.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

private:
   /// Check if the region the expression evaluates to is the standard input,
   /// and thus, is tainted.
   static bool isStdin(const Expr *E, CheckerContext &C);
 
   /// Given a pointer argument, return the value it points to.
   static Optional<SVal> getPointedToSVal(CheckerContext &C, const Expr *Arg);
 
   /// Check for CWE-134: Uncontrolled Format String.
-  static const char MsgUncontrolledFormatString[];
+  static constexpr llvm::StringLiteral MsgUncontrolledFormatString =
+      "Untrusted data is used as a format string "
+      "(CWE-134: Uncontrolled Format String)";
   bool checkUncontrolledFormatString(const CallExpr *CE,
                                      CheckerContext &C) const;
 
   /// Check for:
   /// CERT/STR02-C. "Sanitize data passed to complex subsystems"
   /// CWE-78, "Failure to Sanitize Data into an OS Command"
-  static const char MsgSanitizeSystemArgs[];
+  static constexpr llvm::StringLiteral MsgSanitizeSystemArgs =
+      "Untrusted data is passed to a system call "
+      "(CERT/STR02-C. Sanitize data passed to complex subsystems)";
   bool checkSystemCall(const CallExpr *CE, StringRef Name,
                        CheckerContext &C) const;
 
   /// Check if tainted data is used as a buffer size ins strn.. functions,
   /// and allocators.
-  static const char MsgTaintedBufferSize[];
+  static constexpr llvm::StringLiteral MsgTaintedBufferSize =
+      "Untrusted data is used to specify the buffer size "
+      "(CERT/STR31-C. Guarantee that storage for strings has sufficient space "
+      "for character data and the null terminator)";
   bool checkTaintedBufferSize(const CallExpr *CE, const FunctionDecl *FDecl,
                               CheckerContext &C) const;
 
+  /// Check if tainted data is used as a custom sink's parameter.
+  static constexpr llvm::StringLiteral MsgCustomSink =
+      "Untrusted data is passed to a user-defined sink";
+  bool checkCustomSinks(const CallExpr *CE, StringRef Name,
+                        CheckerContext &C) const;
+
   /// Generate a report if the expression is tainted or points to tainted data.
-  bool generateReportIfTainted(const Expr *E, const char Msg[],
+  bool generateReportIfTainted(const Expr *E, StringRef Msg,
                                CheckerContext &C) const;
 
+  struct TaintPropagationRule;
+  using NameRuleMap = llvm::StringMap<TaintPropagationRule>;
+  using NameArgMap = llvm::StringMap<ArgVector>;
+
   /// 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
   /// InvalidArgIndex in the dst arguments to signify that all the non-const
   /// pointer and reference arguments might be tainted on return. If
   /// ReturnValueIndex is added to the dst list, the return value will be
TaintPropagationRule(ArgVector &&Src, ArgVector &&Dst,
                          VariadicType Var = VariadicType::None,
                          unsigned VarIndex = InvalidArgIndex,
                          PropagationFuncType Func = nullptr)
         : SrcArgs(std::move(Src)), DstArgs(std::move(Dst)),
           VariadicIndex(VarIndex), VarType(Var), PropagationFunc(Func) {}
 
     /// Get the propagation rule for a given function.
     static TaintPropagationRule
-    getTaintPropagationRule(const FunctionDecl *FDecl, StringRef Name,
+    getTaintPropagationRule(const NameRuleMap &CustomPropagations,
+                            const FunctionDecl *FDecl, StringRef Name,
                             CheckerContext &C);
 
     void addSrcArg(unsigned A) { SrcArgs.push_back(A); }
     void addDstArg(unsigned A) { DstArgs.push_back(A); }
 
     bool isNull() const {
       return SrcArgs.empty() && DstArgs.empty() &&
              VariadicType::None == VarType;
struct TaintPropagationRule {
     /// taint rule.
     ProgramStateRef process(const CallExpr *CE, CheckerContext &C) const;
 
     // Functions for custom taintedness propagation.
     static bool postSocket(bool IsTainted, const CallExpr *CE,
                            CheckerContext &C);
   };
 
-  using NameRuleMap = llvm::StringMap<TaintPropagationRule>;
-  using NameArgMap = llvm::StringMap<ArgVector>;
-
   /// Defines a map between the propagation function's name and
   /// TaintPropagationRule.
   NameRuleMap CustomPropagations;
 
   /// Defines a map between the filter function's name and filtering args.
   NameArgMap CustomFilters;
 
   /// Defines a map between the sink function's name and sinking args.
   NameArgMap CustomSinks;
 };
 
 const unsigned GenericTaintChecker::ReturnValueIndex;
 const unsigned GenericTaintChecker::InvalidArgIndex;
-
-const char GenericTaintChecker::MsgUncontrolledFormatString[] =
-    "Untrusted data is used as a format string "
-    "(CWE-134: Uncontrolled Format String)";
-
-const char GenericTaintChecker::MsgSanitizeSystemArgs[] =
-    "Untrusted data is passed to a system call "
-    "(CERT/STR02-C. Sanitize data passed to complex subsystems)";
-
-const char GenericTaintChecker::MsgTaintedBufferSize[] =
-    "Untrusted data is used to specify the buffer size "
-    "(CERT/STR31-C. Guarantee that storage for strings has sufficient space "
-    "for character data and the null terminator)";
 } // end of anonymous namespace
 
 using TaintConfig = GenericTaintChecker::TaintConfiguration;
 
 LLVM_YAML_IS_SEQUENCE_VECTOR(TaintConfig::Propagation)
 LLVM_YAML_IS_SEQUENCE_VECTOR(TaintConfig::NameArgsPair)
 
 namespace llvm {
void GenericTaintChecker::parseConfiguration(CheckerManager &Mgr,
 
   for (auto &S : Config.Sinks) {
     GenericTaintChecker::CustomSinks.try_emplace(S.first, std::move(S.second));
   }
 }
 
 GenericTaintChecker::TaintPropagationRule
 GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule(
-    const FunctionDecl *FDecl, StringRef Name, CheckerContext &C) {
+    const NameRuleMap &CustomPropagations, 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.
 
   // Check for exact name match for functions without builtin substitutes.
   TaintPropagationRule Rule =
       llvm::StringSwitch<TaintPropagationRule>(Name)
           // Source functions
           // TODO: Add support for vfscanf & family.
if (Rule.isNull()) {
     else if (C.isCLibraryFunction(FDecl, "wcsdup"))
       return TaintPropagationRule({0}, {ReturnValueIndex});
   }
 
   // Skipping the following functions, since they might be used for cleansing
   // or smart memory copy:
   // - memccpy - copying until hitting a special character.
 
+  auto It = CustomPropagations.find(Name);
+  if (It != CustomPropagations.end())
+    return It->getValue();
+
   return TaintPropagationRule();
 }
 
 void GenericTaintChecker::checkPreStmt(const CallExpr *CE,
                                        CheckerContext &C) const {
   // Check for taintedness related errors first: system call, uncontrolled
   // format string, tainted buffer size.
   if (checkPre(CE, C))
void GenericTaintChecker::addSourcesPre(const CallExpr *CE,
   if (!FDecl || FDecl->getKind() != Decl::Function)
     return;
 
   StringRef Name = C.getCalleeName(FDecl);
   if (Name.empty())
     return;
 
   // First, try generating a propagation rule for this function.
-  TaintPropagationRule Rule =
-      TaintPropagationRule::getTaintPropagationRule(FDecl, Name, C);
+  TaintPropagationRule Rule = TaintPropagationRule::getTaintPropagationRule(
+      this->CustomPropagations, FDecl, Name, C);
   if (!Rule.isNull()) {
     State = Rule.process(CE, C);
     if (!State)
       return;
     C.addTransition(State);
     return;
   }
 
if (Name.empty())
     return false;
 
   if (checkSystemCall(CE, Name, C))
     return true;
 
   if (checkTaintedBufferSize(CE, FDecl, C))
     return true;
 
+  if (checkCustomSinks(CE, Name, C))
+    return true;
+
   return false;
 }
 
 Optional<SVal> GenericTaintChecker::getPointedToSVal(CheckerContext &C,
                                                      const Expr *Arg) {
   ProgramStateRef State = C.getState();
   SVal AddrVal = C.getSVal(Arg->IgnoreParens());
   if (AddrVal.isUnknownOrUndef())

 GenericTaintChecker::TaintPropagationRule::process(const CallExpr *CE,
                                                    CheckerContext &C) const {
   ProgramStateRef State = C.getState();
 
   // Check for taint in arguments.
   bool IsTainted = true;
   for (unsigned ArgNum : SrcArgs) {
     if (ArgNum >= CE->getNumArgs())
-      return State;
+      continue;
+
     if ((IsTainted = isTaintedOrPointsToTainted(CE->getArg(ArgNum), State, C)))
       break;
   }
 
   // Check for taint in variadic arguments.
   if (!IsTainted && VariadicType::Src == VarType) {
     // Check if any of the arguments is tainted
     for (unsigned i = VariadicIndex; i < CE->getNumArgs(); ++i) {
GenericTaintChecker::TaintPropagationRule::process(const CallExpr *CE,
   // Mark the arguments which should be tainted after the function returns.
   for (unsigned ArgNum : DstArgs) {
     // Should mark the return value?
     if (ArgNum == ReturnValueIndex) {
       State = State->add<TaintArgsOnPostVisit>(ReturnValueIndex);
       continue;
     }
 
+    if (ArgNum >= CE->getNumArgs())
+      continue;
+
     // Mark the given argument.
-    assert(ArgNum < CE->getNumArgs());
     State = State->add<TaintArgsOnPostVisit>(ArgNum);
   }
 
   // Mark all variadic arguments tainted if present.
   if (VariadicType::Dst == VarType) {
     // For all pointer and references that were passed in:
     //   If they are not pointing to const data, mark data as tainted.
     //   TODO: So far we are just going one level down; ideally we'd need to
static bool getPrintfFormatArgumentNum(const CallExpr *CE,
   if (C.getCalleeName(CE).find("setproctitle") != StringRef::npos) {
     ArgNum = 0;
     return true;
   }
 
   return false;
 }
 
-bool GenericTaintChecker::generateReportIfTainted(const Expr *E,
-                                                  const char Msg[],
+bool GenericTaintChecker::generateReportIfTainted(const Expr *E, StringRef Msg,
                                                   CheckerContext &C) const {
   assert(E);
 
   // Check for taint.
   ProgramStateRef State = C.getState();
   Optional<SVal> PointedToSVal = getPointedToSVal(C, E);
   SVal TaintedSVal;
   if (PointedToSVal && isTainted(State, *PointedToSVal))
unsigned ArgNum = llvm::StringSwitch<unsigned>(Name)
                         .Case("execl", 0)
                         .Case("execle", 0)
                         .Case("execlp", 0)
                         .Case("execv", 0)
                         .Case("execvp", 0)
                         .Case("execvP", 0)
                         .Case("execve", 0)
                         .Case("dlopen", 0)
-                        .Default(UINT_MAX);
+                        .Default(InvalidArgIndex);
 
-  if (ArgNum == UINT_MAX || CE->getNumArgs() < (ArgNum + 1))
+  if (ArgNum == InvalidArgIndex || CE->getNumArgs() < (ArgNum + 1))
     return false;
 
   return generateReportIfTainted(CE->getArg(ArgNum), MsgSanitizeSystemArgs, C);
 }
 
 // TODO: Should this check be a part of the CString checker?
 // If yes, should taint be a global setting?
 bool GenericTaintChecker::checkTaintedBufferSize(const CallExpr *CE,
if (ArgNum == InvalidArgIndex) {
     else if (C.isCLibraryFunction(FDecl, "bcopy"))
       ArgNum = 2;
   }
 
   return ArgNum != InvalidArgIndex && CE->getNumArgs() > ArgNum &&
          generateReportIfTainted(CE->getArg(ArgNum), MsgTaintedBufferSize, C);
 }
 
+bool GenericTaintChecker::checkCustomSinks(const CallExpr *CE, StringRef Name,
+                                           CheckerContext &C) const {
+  auto It = CustomSinks.find(Name);
+  if (It == CustomSinks.end())
+    return false;
+
+  const GenericTaintChecker::ArgVector &Args = It->getValue();
+  for (unsigned ArgNum : Args) {
+    if (ArgNum >= CE->getNumArgs())
+      continue;
+
+    if (generateReportIfTainted(CE->getArg(ArgNum), MsgCustomSink, C))
+      return true;
+  }
+
+  return false;
+}
+
 void ento::registerGenericTaintChecker(CheckerManager &Mgr) {
   auto *Checker = Mgr.registerChecker<GenericTaintChecker>();
   std::string Option{"Config"};
   StringRef ConfigFile =
       Mgr.getAnalyzerOptions().getCheckerStringOption(Checker, Option);
   llvm::Optional<TaintConfig> Config =
       getConfiguration<TaintConfig>(Mgr, Checker, Option, ConfigFile);
   if (Config)
     Checker->parseConfiguration(Mgr, Option, std::move(Config.getValue()));
 }
 
 bool ento::shouldRegisterGenericTaintChecker(const LangOptions &LO) {
   return true;
 }

clang/test/Analysis/taint-generic.c

void constraintManagerShouldTreatAsOpaque(int rhs) {
   scanf("%d", &i);
   // This comparison used to hit an assertion in the constraint manager,
   // which didn't handle NonLoc sym-sym comparisons.
   if (i < rhs)
     return;
   if (i < rhs)
     *(volatile int *) 0; // no-warning
 }
+
+
+// Test configuration
+int mySource1();
+void mySource2(int*);
+void myScanf(const char*, ...);
+int myPropagator(int, int*);
+int mySnprintf(char*, size_t, const char*, ...);
+void mySink(int, int, int);
+
+void testConfigurationSources1() {
+  int x = mySource1();
+  Buffer[x] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void testConfigurationSources2() {
+  int x;
+  mySource2(&x);
+  Buffer[x] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void testConfigurationSources3() {
+  int x, y;
+  myScanf("%d %d", &x, &y);
+  Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void testConfigurationPropagation() {
+  int x = mySource1();
+  int y;
+  myPropagator(x, &y);
+  Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void testConfigurationSinks() {
+  int x = mySource1();
+  mySink(x, 1, 2);
+  // expected-warning@-1 {{Untrusted data is passed to a user-defined sink}}
+  mySink(1, x, 2); // no-warning
+  mySink(1, 2, x);
+  // expected-warning@-1 {{Untrusted data is passed to a user-defined sink}}
+}