[analyzer] Add optin.taint.TaintedDiv checker (#106389)

Tainted division operation is separated out from the core.DivideZero
checker into the optional optin.taint.TaintedDiv checker. The checker
warns when the denominator in a division operation is an attacker
controlled value.

clang/docs/analyzer/checkers.rst

@@ -1288,6 +1288,34 @@ by explicitly marking the ``size`` parameter as sanitized. See the
     delete[] ptr;
   }
 
+.. _optin-taint-TaintedDiv:
+
+optin.taint.TaintedDiv (C, C++, ObjC)
+"""""""""""""""""""""""""""""""""""""
+This checker warns when the denominator in a division
+operation is a tainted (potentially attacker controlled) value.
+If the attacker can set the denominator to 0, a runtime error can
+be triggered. The checker warns when the denominator is a tainted
+value and the analyzer cannot prove that it is not 0. This warning
+is more pessimistic than the :ref:`core-DivideZero` checker
+which warns only when it can prove that the denominator is 0.
+
+.. code-block:: c
+
+  int vulnerable(int n) {
+    size_t size = 0;
+    scanf("%zu", &size);
+    return n / size; // warn: Division by a tainted value, possibly zero
+  }
+
+  int not_vulnerable(int n) {
+    size_t size = 0;
+    scanf("%zu", &size);
+    if (!size)
+      return 0;
+    return n / size; // no warning
+  }
+
 .. _security-checkers:
 
 security

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

@@ -1703,6 +1703,12 @@ def TaintedAllocChecker: Checker<"TaintedAlloc">,
   Dependencies<[DynamicMemoryModeling, TaintPropagationChecker]>,
   Documentation<HasDocumentation>;
 
+def TaintedDivChecker: Checker<"TaintedDiv">,
+  HelpText<"Check for divisions where the denominator is tainted "
+           "(attacker controlled) and might be 0.">,
+  Dependencies<[TaintPropagationChecker]>,
+  Documentation<HasDocumentation>;
+
 } // end "optin.taint"
 
 //===----------------------------------------------------------------------===//

clang/include/clang/StaticAnalyzer/Core/CheckerManager.h

@@ -221,6 +221,10 @@ public:
     return static_cast<CHECKER *>(CheckerTags[tag]);
   }
 
+  template <typename CHECKER> bool isRegisteredChecker() {
+    return CheckerTags.contains(getTag<CHECKER>());
+  }
+
 //===----------------------------------------------------------------------===//
 // Functions for running checkers for AST traversing.
 //===----------------------------------------------------------------------===//

clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp

@@ -25,9 +25,7 @@ using namespace ento;
 using namespace taint;
 
 namespace {
-class DivZeroChecker : public Checker< check::PreStmt<BinaryOperator> > {
-  const BugType BT{this, "Division by zero"};
-  const BugType TaintBT{this, "Division by zero", categories::TaintedData};
+class DivZeroChecker : public Checker<check::PreStmt<BinaryOperator>> {
   void reportBug(StringRef Msg, ProgramStateRef StateZero,
                  CheckerContext &C) const;
   void reportTaintBug(StringRef Msg, ProgramStateRef StateZero,
@@ -35,6 +33,12 @@ class DivZeroChecker : public Checker< check::PreStmt<BinaryOperator> > {
                       llvm::ArrayRef<SymbolRef> TaintedSyms) const;
 
 public:
+  /// This checker class implements several user facing checkers
+  enum CheckKind { CK_DivideZero, CK_TaintedDivChecker, CK_NumCheckKinds };
+  bool ChecksEnabled[CK_NumCheckKinds] = {false};
+  CheckerNameRef CheckNames[CK_NumCheckKinds];
+  mutable std::unique_ptr<BugType> BugTypes[CK_NumCheckKinds];
+
   void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;
 };
 } // end anonymous namespace
@@ -48,8 +52,14 @@ static const Expr *getDenomExpr(const ExplodedNode *N) {
 
 void DivZeroChecker::reportBug(StringRef Msg, ProgramStateRef StateZero,
                                CheckerContext &C) const {
+  if (!ChecksEnabled[CK_DivideZero])
+    return;
+  if (!BugTypes[CK_DivideZero])
+    BugTypes[CK_DivideZero].reset(
+        new BugType(CheckNames[CK_DivideZero], "Division by zero"));
   if (ExplodedNode *N = C.generateErrorNode(StateZero)) {
-    auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
+    auto R = std::make_unique<PathSensitiveBugReport>(*BugTypes[CK_DivideZero],
+                                                      Msg, N);
     bugreporter::trackExpressionValue(N, getDenomExpr(N), *R);
     C.emitReport(std::move(R));
   }
@@ -58,8 +68,15 @@ void DivZeroChecker::reportBug(StringRef Msg, ProgramStateRef StateZero,
 void DivZeroChecker::reportTaintBug(
     StringRef Msg, ProgramStateRef StateZero, CheckerContext &C,
     llvm::ArrayRef<SymbolRef> TaintedSyms) const {
-  if (ExplodedNode *N = C.generateErrorNode(StateZero)) {
-    auto R = std::make_unique<PathSensitiveBugReport>(TaintBT, Msg, N);
+  if (!ChecksEnabled[CK_TaintedDivChecker])
+    return;
+  if (!BugTypes[CK_TaintedDivChecker])
+    BugTypes[CK_TaintedDivChecker].reset(
+        new BugType(CheckNames[CK_TaintedDivChecker], "Division by zero",
+                    categories::TaintedData));
+  if (ExplodedNode *N = C.generateNonFatalErrorNode(StateZero)) {
+    auto R = std::make_unique<PathSensitiveBugReport>(
+        *BugTypes[CK_TaintedDivChecker], Msg, N);
     bugreporter::trackExpressionValue(N, getDenomExpr(N), *R);
     for (auto Sym : TaintedSyms)
       R->markInteresting(Sym);
@@ -101,8 +118,8 @@ void DivZeroChecker::checkPreStmt(const BinaryOperator *B,
   if ((stateNotZero && stateZero)) {
     std::vector<SymbolRef> taintedSyms = getTaintedSymbols(C.getState(), *DV);
     if (!taintedSyms.empty()) {
-      reportTaintBug("Division by a tainted value, possibly zero", stateZero, C,
-                     taintedSyms);
+      reportTaintBug("Division by a tainted value, possibly zero", stateNotZero,
+                     C, taintedSyms);
       return;
     }
   }
@@ -113,9 +130,27 @@ void DivZeroChecker::checkPreStmt(const BinaryOperator *B,
 }
 
 void ento::registerDivZeroChecker(CheckerManager &mgr) {
-  mgr.registerChecker<DivZeroChecker>();
+  DivZeroChecker *checker = mgr.registerChecker<DivZeroChecker>();
+  checker->ChecksEnabled[DivZeroChecker::CK_DivideZero] = true;
+  checker->CheckNames[DivZeroChecker::CK_DivideZero] =
+      mgr.getCurrentCheckerName();
 }
 
 bool ento::shouldRegisterDivZeroChecker(const CheckerManager &mgr) {
   return true;
 }
+
+void ento::registerTaintedDivChecker(CheckerManager &mgr) {
+  DivZeroChecker *checker;
+  if (!mgr.isRegisteredChecker<DivZeroChecker>())
+    checker = mgr.registerChecker<DivZeroChecker>();
+  else
+    checker = mgr.getChecker<DivZeroChecker>();
+  checker->ChecksEnabled[DivZeroChecker::CK_TaintedDivChecker] = true;
+  checker->CheckNames[DivZeroChecker::CK_TaintedDivChecker] =
+      mgr.getCurrentCheckerName();
+}
+
+bool ento::shouldRegisterTaintedDivChecker(const CheckerManager &mgr) {
+  return true;
+}

clang/test/Analysis/divzero-tainted-div-difference.c

@@ -0,0 +1,34 @@
+// RUN: %clang_analyze_cc1 -Wno-format-security -Wno-pointer-to-int-cast \
+// RUN:   -Wno-incompatible-library-redeclaration -verify=normaldiv %s \
+// RUN:   -analyzer-checker=optin.taint.GenericTaint \
+// RUN:   -analyzer-checker=core
+
+// RUN: %clang_analyze_cc1 -Wno-format-security -Wno-pointer-to-int-cast \
+// RUN:   -Wno-incompatible-library-redeclaration -verify=tainteddiv %s \
+// RUN:   -analyzer-checker=optin.taint.GenericTaint \
+// RUN:   -analyzer-checker=optin.taint.TaintedDiv
+
+int getchar(void);
+
+
+//If we are sure that we divide by zero
+//we emit a divide by zero warning
+int testDivZero(void) {
+  int x = getchar(); // taint source
+  if (!x)
+    return 5 / x; // normaldiv-warning{{Division by zero}}
+  return 8;
+}
+
+// The attacker provided value might be 0
+int testDivZero2(void) {
+  int x = getchar(); // taint source
+  return 5 / x; // tainteddiv-warning{{Division by a tainted value}}
+}
+
+int testDivZero3(void) {
+  int x = getchar(); // taint source
+  if (!x)
+    return 0;
+  return 5 / x; // no warning
+}

clang/test/Analysis/taint-diagnostic-visitor.c

@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=optin.taint,core,alpha.security.ArrayBoundV2,optin.taint.TaintedAlloc -analyzer-output=text -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=optin.taint,core,alpha.security.ArrayBoundV2 -analyzer-output=text -verify %s
 
 // This file is for testing enhanced diagnostics produced by the GenericTaintChecker
 

clang/test/Analysis/taint-generic.c

@@ -1,6 +1,7 @@
 // RUN: %clang_analyze_cc1 -Wno-format-security -Wno-pointer-to-int-cast \
 // RUN:   -Wno-incompatible-library-redeclaration -verify %s \
 // RUN:   -analyzer-checker=optin.taint.GenericTaint \
+// RUN:   -analyzer-checker=optin.taint.TaintedDiv \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=alpha.security.ArrayBoundV2 \
 // RUN:   -analyzer-checker=debug.ExprInspection \
@@ -11,16 +12,15 @@
 // RUN:   -Wno-incompatible-library-redeclaration -verify %s \
 // RUN:   -DFILE_IS_STRUCT \
 // RUN:   -analyzer-checker=optin.taint.GenericTaint \
+// RUN:   -analyzer-checker=optin.taint.TaintedDiv \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=alpha.security.ArrayBoundV2 \
 // RUN:   -analyzer-checker=debug.ExprInspection \
 // RUN:   -analyzer-config \
 // RUN:     optin.taint.TaintPropagation:Config=%S/Inputs/taint-generic-config.yaml
 
-// RUN: not %clang_analyze_cc1 -Wno-pointer-to-int-cast \
-// RUN:   -Wno-incompatible-library-redeclaration -verify %s \
+// RUN: not %clang_analyze_cc1 -verify %s \
 // RUN:   -analyzer-checker=optin.taint.GenericTaint  \
-// RUN:   -analyzer-checker=debug.ExprInspection \
 // RUN:   -analyzer-config \
 // RUN:     optin.taint.TaintPropagation:Config=justguessit \
 // RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-FILE
@@ -30,10 +30,8 @@
 // CHECK-INVALID-FILE-SAME:        that expects a valid filename instead of
 // CHECK-INVALID-FILE-SAME:        'justguessit'
 
-// RUN: not %clang_analyze_cc1 -Wno-incompatible-library-redeclaration \
-// RUN:   -verify %s \
+// RUN: not %clang_analyze_cc1 -verify %s \
 // RUN:   -analyzer-checker=optin.taint.GenericTaint  \
-// RUN:   -analyzer-checker=debug.ExprInspection \
 // RUN:   -analyzer-config \
 // RUN:     optin.taint.TaintPropagation:Config=%S/Inputs/taint-generic-config-ill-formed.yaml \
 // RUN:   2>&1 | FileCheck -DMSG=%errc_EINVAL %s -check-prefix=CHECK-ILL-FORMED
@@ -42,10 +40,8 @@
 // CHECK-ILL-FORMED-SAME:        'optin.taint.TaintPropagation:Config',
 // CHECK-ILL-FORMED-SAME:        that expects a valid yaml file: [[MSG]]
 
-// RUN: not %clang_analyze_cc1 -Wno-incompatible-library-redeclaration \
-// RUN:   -verify %s \
+// RUN: not %clang_analyze_cc1 -verify %s \
 // RUN:   -analyzer-checker=optin.taint.GenericTaint \
-// RUN:   -analyzer-checker=debug.ExprInspection \
 // RUN:   -analyzer-config \
 // RUN:     optin.taint.TaintPropagation:Config=%S/Inputs/taint-generic-config-invalid-arg.yaml \
 // RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-ARG
@@ -408,6 +404,14 @@ int testDivByZero(void) {
   return 5/x; // expected-warning {{Division by a tainted value, possibly zero}}
 }
 
+int testTaintedDivFP(void) {
+  int x;
+  scanf("%d", &x);
+  if (!x)
+    return 0;
+  return 5/x; // x cannot be 0, so no tainted warning either
+}
+
 // Zero-sized VLAs.
 void testTaintedVLASize(void) {
   int x;