diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index a22bda189dd295..81264428c72ed1 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 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 diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 6bc389f9da265f..349040c15eeb83 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; +def TaintedDivChecker: Checker<"TaintedDiv">, + HelpText<"Check for divisions where the denominator is tainted " + "(attacker controlled) and might be 0.">, + Dependencies<[TaintPropagationChecker]>, + Documentation; + } // end "optin.taint" //===----------------------------------------------------------------------===// diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h index 24c5b66fd58220..de40b96614dbc9 100644 --- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h +++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h @@ -221,6 +221,10 @@ class CheckerManager { return static_cast(CheckerTags[tag]); } + template bool isRegisteredChecker() { + return CheckerTags.contains(getTag()); + } + //===----------------------------------------------------------------------===// // 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..7c8b44eb05942a 100644 --- a/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp @@ -25,9 +25,7 @@ using namespace ento; using namespace taint; namespace { -class DivZeroChecker : public Checker< check::PreStmt > { - const BugType BT{this, "Division by zero"}; - const BugType TaintBT{this, "Division by zero", categories::TaintedData}; +class DivZeroChecker : public Checker> { 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 > { llvm::ArrayRef 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 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(BT, Msg, N); + auto R = std::make_unique(*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 TaintedSyms) const { - if (ExplodedNode *N = C.generateErrorNode(StateZero)) { - auto R = std::make_unique(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( + *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 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 *checker = mgr.registerChecker(); + 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()) + checker = mgr.registerChecker(); + else + checker = mgr.getChecker(); + 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/divzero-tainted-div-difference.c b/clang/test/Analysis/divzero-tainted-div-difference.c new file mode 100644 index 00000000000000..28486ccdf7e4fe --- /dev/null +++ b/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 +} diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c b/clang/test/Analysis/taint-diagnostic-visitor.c index 526c04c3607775..223df9951fd6b9 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 -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..ad5a99fe8b3a35 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,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;