[clang] [analyzer] Adding optin.taint.TaintedDiv checker (PR #106389)
Daniel Krupp via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 26 09:42:37 PDT 2024
https://github.com/dkrupp updated https://github.com/llvm/llvm-project/pull/106389
>From beb6f6787f4a92e8892ba8f19d0af924edd56e3b Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Wed, 28 Aug 2024 15:32:35 +0200
Subject: [PATCH 1/3] Adding optin.taint.TaintedDiv checker
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 | 28 +++++++++++
.../clang/StaticAnalyzer/Checkers/Checkers.td | 6 +++
.../StaticAnalyzer/Core/CheckerManager.h | 5 ++
.../Checkers/DivZeroChecker.cpp | 46 +++++++++++++++++--
.../test/Analysis/taint-diagnostic-visitor.c | 2 +-
clang/test/Analysis/taint-generic.c | 3 ++
6 files changed, 84 insertions(+), 6 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 47c6fc680deb1b..60760b8ca5555a 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/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 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 7da0d0a87e8c0c..d6a38d57846df5 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/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 "
+ "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 24c5b66fd58220..8e0af1b6db9fa2 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -221,6 +221,11 @@ 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..89aac24d7576e5 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,12 @@ 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 +52,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 +68,15 @@ 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 +130,28 @@ 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 526c04c3607775..134e19f111ea34 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=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,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 a5cfdd9db11579..80c2c8cb74e79f 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=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,6 +12,7 @@
// 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 \
@@ -20,6 +22,7 @@
// RUN: not %clang_analyze_cc1 -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=debug.ExprInspection \
// RUN: -analyzer-config \
// RUN: optin.taint.TaintPropagation:Config=justguessit \
>From 0b9e85d786224f587566d6cc4a97a5667f19c4af Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Wed, 28 Aug 2024 17:49:50 +0200
Subject: [PATCH 2/3] formatting fix
---
clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
index 89aac24d7576e5..da160f13301413 100644
--- a/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
@@ -25,7 +25,7 @@ using namespace ento;
using namespace taint;
namespace {
-class DivZeroChecker : public Checker< check::PreStmt<BinaryOperator> > {
+class DivZeroChecker : public Checker<check::PreStmt<BinaryOperator>> {
void reportBug(StringRef Msg, ProgramStateRef StateZero,
CheckerContext &C) const;
void reportTaintBug(StringRef Msg, ProgramStateRef StateZero,
>From 91ac5ed10a154410c246d985752c1bbfcf23b105 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Thu, 26 Sep 2024 15:41:48 +0200
Subject: [PATCH 3/3] Fixing review comments
-Making tainted division the report non-fatal so the analysis can continue
-Adding some test cases
-Fixing minor documentation issues
---
clang/docs/analyzer/checkers.rst | 2 +-
.../StaticAnalyzer/Core/CheckerManager.h | 5 ++---
.../Checkers/DivZeroChecker.cpp | 21 +++++++++----------
clang/test/Analysis/taint-generic.c | 18 ++++++++++++++++
4 files changed, 31 insertions(+), 15 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 60760b8ca5555a..353d02fcb5d303 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1308,7 +1308,7 @@ which warns only when it can prove that the denominator is 0.
return n/size; // warn: Division by a tainted value, possibly zero
}
- int not_vulnerable(void) {
+ int not_vulnerable(int n) {
size_t size = 0;
scanf("%zu", &size);
if (!size)
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index 8e0af1b6db9fa2..e8e4f1b777bc44 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -221,9 +221,8 @@ class CheckerManager {
return static_cast<CHECKER *>(CheckerTags[tag]);
}
- template <typename CHECKER> bool isRegisteredChecker() {
- CheckerTag tag = getTag<CHECKER>();
- return (CheckerTags.count(tag) != 0);
+ template <typename CHECKER> bool isRegisteredChecker() {
+ return CheckerTags.contains(getTag<CHECKER>());
}
//===----------------------------------------------------------------------===//
diff --git a/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
index da160f13301413..c44726df98afbf 100644
--- a/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
@@ -34,7 +34,7 @@ class DivZeroChecker : public Checker<check::PreStmt<BinaryOperator>> {
public:
/// This checker class implements multiple user facing checker
- enum CheckKind { CK_DivZeroChecker, CK_TaintedDivChecker, CK_NumCheckKinds };
+ 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];
@@ -52,14 +52,14 @@ static const Expr *getDenomExpr(const ExplodedNode *N) {
void DivZeroChecker::reportBug(StringRef Msg, ProgramStateRef StateZero,
CheckerContext &C) const {
- if (!ChecksEnabled[CK_DivZeroChecker])
+ if (!ChecksEnabled[CK_DivideZero])
return;
- if (!BugTypes[CK_DivZeroChecker])
- BugTypes[CK_DivZeroChecker].reset(
- new BugType(CheckNames[CK_DivZeroChecker], "Division by zero"));
+ 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>(
- *BugTypes[CK_DivZeroChecker], Msg, N);
+ *BugTypes[CK_DivideZero], Msg, N);
bugreporter::trackExpressionValue(N, getDenomExpr(N), *R);
C.emitReport(std::move(R));
}
@@ -74,7 +74,7 @@ void DivZeroChecker::reportTaintBug(
BugTypes[CK_TaintedDivChecker].reset(
new BugType(CheckNames[CK_TaintedDivChecker], "Division by zero",
categories::TaintedData));
- if (ExplodedNode *N = C.generateErrorNode(StateZero)) {
+ if (ExplodedNode *N = C.generateNonFatalErrorNode(StateZero)) {
auto R = std::make_unique<PathSensitiveBugReport>(
*BugTypes[CK_TaintedDivChecker], Msg, N);
bugreporter::trackExpressionValue(N, getDenomExpr(N), *R);
@@ -118,7 +118,7 @@ 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,
+ reportTaintBug("Division by a tainted value, possibly zero", stateNotZero, C,
taintedSyms);
return;
}
@@ -131,9 +131,8 @@ void DivZeroChecker::checkPreStmt(const BinaryOperator *B,
void ento::registerDivZeroChecker(CheckerManager &mgr) {
DivZeroChecker *checker = mgr.registerChecker<DivZeroChecker>();
- ;
- checker->ChecksEnabled[DivZeroChecker::CK_DivZeroChecker] = true;
- checker->CheckNames[DivZeroChecker::CK_DivZeroChecker] =
+ checker->ChecksEnabled[DivZeroChecker::CK_DivideZero] = true;
+ checker->CheckNames[DivZeroChecker::CK_DivideZero] =
mgr.getCurrentCheckerName();
}
diff --git a/clang/test/Analysis/taint-generic.c b/clang/test/Analysis/taint-generic.c
index 80c2c8cb74e79f..972b633902cfd6 100644
--- a/clang/test/Analysis/taint-generic.c
+++ b/clang/test/Analysis/taint-generic.c
@@ -411,6 +411,24 @@ 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
+}
+
+
+//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; // expected-warning{{Division by zero}}
+ return 8;
+}
+
// Zero-sized VLAs.
void testTaintedVLASize(void) {
int x;
More information about the cfe-commits
mailing list