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

dkrupp · 2024-08-28T13:44:04Z

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.

llvmbot · 2024-08-28T13:44:34Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Daniel Krupp (dkrupp)

Changes

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.

Full diff: https://github.com/llvm/llvm-project/pull/106389.diff

6 Files Affected:

(modified) clang/docs/analyzer/checkers.rst (+28)
(modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+6)
(modified) clang/include/clang/StaticAnalyzer/Core/CheckerManager.h (+7)
(modified) clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp (+45-5)
(modified) clang/test/Analysis/taint-diagnostic-visitor.c (+1-1)
(modified) clang/test/Analysis/taint-generic.c (+3)

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 89a1018e14c0e6..64620ea065c02f 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1053,6 +1053,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 if the analyzer cannot prove
+that the denominator is not 0 and it is a tainted value.
+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(void) {
+    size_t size = 0;
+    scanf("%zu", &size);
+    if (!size)
+      return 0;
+    return n/size; // no warning
+  }
+
 .. _security-checkers:
 
 security
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index fb4114619ac3d3..5a9afa0f15f5a0 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1710,6 +1710,12 @@ def TaintedAllocChecker: Checker<"TaintedAlloc">,
   Dependencies<[DynamicMemoryModeling, TaintPropagationChecker]>,
   Documentation<HasDocumentation>;
 
+def TaintedDivChecker: Checker<"TaintedDiv">,
+  HelpText<"Check for divisions, where the denominator "
+           "might be 0 as it is a tainted (attacker controlled) value.">,
+  Dependencies<[TaintPropagationChecker]>,
+  Documentation<HasDocumentation>;
+
 } // end "optin.taint"
 
 //===----------------------------------------------------------------------===//
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index ad25d18f280700..e900ef0c178a2f 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -223,6 +223,13 @@ class CheckerManager {
     return static_cast<CHECKER *>(CheckerTags[tag]);
   }
 
+
+  template <typename CHECKER>
+  bool isRegisteredChecker() {
+    CheckerTag tag = getTag<CHECKER>();
+    return (CheckerTags.count(tag) != 0);
+  }
+
 //===----------------------------------------------------------------------===//
 // Functions for running checkers for AST traversing.
 //===----------------------------------------------------------------------===//
diff --git a/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
index 5496f087447fbe..60e06ad699c92e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
@@ -26,8 +26,6 @@ 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};
   void reportBug(StringRef Msg, ProgramStateRef StateZero,
                  CheckerContext &C) const;
   void reportTaintBug(StringRef Msg, ProgramStateRef StateZero,
@@ -35,6 +33,16 @@ class DivZeroChecker : public Checker< check::PreStmt<BinaryOperator> > {
                       llvm::ArrayRef<SymbolRef> TaintedSyms) const;
 
 public:
+  /// This checker class implements multiple user facing checker
+  enum CheckKind {
+    CK_DivZeroChecker,
+    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 +56,14 @@ static const Expr *getDenomExpr(const ExplodedNode *N) {
 
 void DivZeroChecker::reportBug(StringRef Msg, ProgramStateRef StateZero,
                                CheckerContext &C) const {
+  if (!ChecksEnabled[CK_DivZeroChecker])
+    return;
+  if (!BugTypes[CK_DivZeroChecker])
+      BugTypes[CK_DivZeroChecker].reset(new BugType(
+                                           CheckNames[CK_DivZeroChecker],
+                                           "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_DivZeroChecker], Msg, N);
     bugreporter::trackExpressionValue(N, getDenomExpr(N), *R);
     C.emitReport(std::move(R));
   }
@@ -58,8 +72,16 @@ void DivZeroChecker::reportBug(StringRef Msg, ProgramStateRef StateZero,
 void DivZeroChecker::reportTaintBug(
     StringRef Msg, ProgramStateRef StateZero, CheckerContext &C,
     llvm::ArrayRef<SymbolRef> TaintedSyms) const {
+  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.generateErrorNode(StateZero)) {
-    auto R = std::make_unique<PathSensitiveBugReport>(TaintBT, Msg, N);
+    auto R = std::make_unique<PathSensitiveBugReport>(
+        *BugTypes[CK_TaintedDivChecker], Msg, N);
     bugreporter::trackExpressionValue(N, getDenomExpr(N), *R);
     for (auto Sym : TaintedSyms)
       R->markInteresting(Sym);
@@ -113,9 +135,27 @@ void DivZeroChecker::checkPreStmt(const BinaryOperator *B,
 }
 
 void ento::registerDivZeroChecker(CheckerManager &mgr) {
-  mgr.registerChecker<DivZeroChecker>();
+  DivZeroChecker *checker = mgr.registerChecker<DivZeroChecker>();;
+  checker->ChecksEnabled[DivZeroChecker::CK_DivZeroChecker] = true;
+  checker->CheckNames[DivZeroChecker::CK_DivZeroChecker] =
+    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;
+}
diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c b/clang/test/Analysis/taint-diagnostic-visitor.c
index f51423646e8aec..11820003e8c4d8 100644
--- a/clang/test/Analysis/taint-diagnostic-visitor.c
+++ b/clang/test/Analysis/taint-diagnostic-visitor.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2,optin.taint.TaintedAlloc -analyzer-output=text -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2,optin.taint.TaintedAlloc,optin.taint.TaintedDiv -analyzer-output=text -verify %s
 
 // This file is for testing enhanced diagnostics produced by the GenericTaintChecker
 
diff --git a/clang/test/Analysis/taint-generic.c b/clang/test/Analysis/taint-generic.c
index 1c139312734bca..e74e1a48ee40bb 100644
--- a/clang/test/Analysis/taint-generic.c
+++ b/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=alpha.security.taint \
+// RUN:   -analyzer-checker=optin.taint.TaintedDiv \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=alpha.security.ArrayBoundV2 \
 // RUN:   -analyzer-checker=debug.ExprInspection \
@@ -11,6 +12,7 @@
 // RUN:   -Wno-incompatible-library-redeclaration -verify %s \
 // RUN:   -DFILE_IS_STRUCT \
 // RUN:   -analyzer-checker=alpha.security.taint \
+// RUN:   -analyzer-checker=optin.taint.TaintedDiv \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=alpha.security.ArrayBoundV2 \
 // RUN:   -analyzer-checker=debug.ExprInspection \
@@ -20,6 +22,7 @@
 // RUN: not %clang_analyze_cc1 -Wno-pointer-to-int-cast \
 // RUN:   -Wno-incompatible-library-redeclaration -verify %s \
 // RUN:   -analyzer-checker=alpha.security.taint \
+// RUN:   -analyzer-checker=optin.taint.TaintedDiv \
 // RUN:   -analyzer-checker=debug.ExprInspection \
 // RUN:   -analyzer-config \
 // RUN:     alpha.security.taint.TaintPropagation:Config=justguessit \

github-actions · 2024-08-28T13:47:59Z

✅ With the latest revision this PR passed the C/C++ code formatter.

NagyDonat

LGTM overall, I added some minor inline remarks.

Also consider adding a few simple testcases to distinguish the effects of DivideZero and TaintedDiv. It would also be interesting to highlight what happens in situations like

int test(void) {
  int x = getchar(); // or any other taint source
  if (!x)
    return 5 / x;
  return 8;
}

(I presume that in this case core.DivideZero will create a bug report, but the new TaintedDiv checker won't.)

NagyDonat · 2024-08-29T09:53:45Z

clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp

+  if (!BugTypes[CK_DivZeroChecker])
+    BugTypes[CK_DivZeroChecker].reset(
+        new BugType(CheckNames[CK_DivZeroChecker], "Division by zero"));


It's a pity that we need this lazy dynamic initialization 🫤. We should really introduce a clear framework to avoid this.

clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp

steakhal · 2024-08-30T09:37:02Z

I had a look, and I don't think I have more to add to the existing review.

steakhal

Ah it didn't publish my inline comments. Here they are.

clang/docs/analyzer/checkers.rst

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

dkrupp · 2024-09-26T13:22:17Z

LGTM overall, I added some minor inline remarks.

Also consider adding a few simple testcases to distinguish the effects of DivideZero and TaintedDiv. It would also be interesting to highlight what happens in situations like
int test(void) {
  int x = getchar(); // or any other taint source
  if (!x)
    return 5 / x;
  return 8;
}
(I presume that in this case core.DivideZero will create a bug report, but the new TaintedDiv checker won't.)

Test added.

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.

-Making tainted division the report non-fatal so the analysis can continue -Adding some test cases -Fixing minor documentation issues

NagyDonat

Looks good overall, I added several suggestions but they mostly tweak the documentation and the comments.

Moreover, I don't see any tests where the separation between optin.taint.TaintedDiv and core.DivideZero is tested by enabling one of them and disabling the other. You should probably add a new short test file for this where:

taint modeling is active;
there are two RUN lines (with different -verify=... tags): one that enables optin.taint.TaintedDiv and one that enables core.DivideZero;
there are testcases that perform division by:
- a value that comes from a tainted source and is known to be zero at the point of division (I know that e.g. in an if (x == 0) block getSVal() will return a ConcreteInt as the value of x instead of the (potentially tainted) symbol that was used to represent it earlier -- but it would be still nice to see this situation.)
- a value that comes from a tainted source and may be either zero or nonzero;
- a value that comes from a tainted source and is known to be nonzero.

clang/docs/analyzer/checkers.rst

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

clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp

clang/test/Analysis/taint-generic.c

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

-separating test cases for the tainteddiv and the dividezero checkers -simplifying taint test execution lit test invocations

dkrupp · 2024-10-01T07:55:58Z

Looks good overall, I added several suggestions but they mostly tweak the documentation and the comments.

Moreover, I don't see any tests where the separation between optin.taint.TaintedDiv and core.DivideZero is tested by enabling one of them and disabling the other. You should probably add a new short test file for this where:
* taint modeling is active;

* there are two RUN lines (with different `-verify=...` tags): one that enables `optin.taint.TaintedDiv` and one that enables `core.DivideZero`;

* there are testcases that perform division by:
  
  * a value that comes from a tainted source and is known to be zero at the point of division (I know that e.g. in an `if (x == 0)` block `getSVal()` will return a `ConcreteInt` as the value of `x` instead of the (potentially tainted) symbol that was used to represent it earlier -- but it would be still nice to see this situation.)
  * a value that comes from a tainted source and may be either zero or nonzero;
  * a value that comes from a tainted source and is known to be nonzero.

Thanks. I added divzero-tainted-div-difference.c test file for this.

NagyDonat

Looks good to me – after some very minor cleanup ;)

clang/docs/analyzer/checkers.rst

clang/test/Analysis/taint-generic.c

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.

dkrupp requested review from NagyDonat and steakhal and removed request for NagyDonat August 28, 2024 13:44

llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Aug 28, 2024

dkrupp force-pushed the taint_divzero_sep branch 2 times, most recently from e979542 to ccc5da0 Compare August 28, 2024 14:05

NagyDonat reviewed Aug 29, 2024

View reviewed changes

steakhal reviewed Aug 30, 2024

View reviewed changes

clang/docs/analyzer/checkers.rst Outdated Show resolved Hide resolved

clang/include/clang/StaticAnalyzer/Core/CheckerManager.h Outdated Show resolved Hide resolved

dkrupp force-pushed the taint_divzero_sep branch from faf7434 to b37f804 Compare September 26, 2024 13:47

whisperity changed the title ~~Adding optin.taint.TaintedDiv checker~~ [analyzer] Adding optin.taint.TaintedDiv checker Sep 26, 2024

dkrupp added 3 commits September 26, 2024 15:52

Adding optin.taint.TaintedDiv checker

beb6f67

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.

formatting fix

0b9e85d

Fixing review comments

91ac5ed

-Making tainted division the report non-fatal so the analysis can continue -Adding some test cases -Fixing minor documentation issues

dkrupp force-pushed the taint_divzero_sep branch from b37f804 to 91ac5ed Compare September 26, 2024 16:42

formatting fixes

c1f550a

dkrupp requested a review from NagyDonat September 30, 2024 11:17

NagyDonat changed the title ~~[analyzer] Adding optin.taint.TaintedDiv checker~~ [analyzer] Add optin.taint.TaintedDiv checker Sep 30, 2024

steakhal approved these changes Sep 30, 2024

View reviewed changes

NagyDonat reviewed Sep 30, 2024

View reviewed changes

Fixing review comments

bad88fd

-separating test cases for the tainteddiv and the dividezero checkers -simplifying taint test execution lit test invocations

dkrupp force-pushed the taint_divzero_sep branch from 7c163af to bad88fd Compare October 1, 2024 07:55

dkrupp requested a review from NagyDonat October 1, 2024 07:56

NagyDonat approved these changes Oct 1, 2024

View reviewed changes

clang/docs/analyzer/checkers.rst Outdated Show resolved Hide resolved

clang/test/Analysis/taint-generic.c Outdated Show resolved Hide resolved

clang/test/Analysis/taint-generic.c Outdated Show resolved Hide resolved

clang/test/Analysis/taint-generic.c Outdated Show resolved Hide resolved

Minor cleanup of documentation and RUN command line breaks

44805d2

dkrupp merged commit 09b8dbf into llvm:main Oct 1, 2024
9 checks passed

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

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

Uh oh!

Conversation

dkrupp commented Aug 28, 2024

Uh oh!

llvmbot commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NagyDonat left a comment

Choose a reason for hiding this comment

Uh oh!

NagyDonat Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steakhal commented Aug 30, 2024

Uh oh!

steakhal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dkrupp commented Sep 26, 2024

Uh oh!

NagyDonat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dkrupp commented Oct 1, 2024

Uh oh!

NagyDonat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Aug 28, 2024 •

edited

Loading

github-actions bot commented Aug 28, 2024 •

edited

Loading