[clang] [analyzer] New optin.taint.TaintedAlloc checker for catching unbounded memory allocation calls (PR #92420)
Daniel Krupp via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 5 04:22:08 PDT 2024
https://github.com/dkrupp updated https://github.com/llvm/llvm-project/pull/92420
>From f6fdd544a90b865e5e0e530930db87cad405216e Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Tue, 30 Apr 2024 15:20:52 +0200
Subject: [PATCH 1/8] [analyzer] Adding taint analysis capability to
unix.Malloc checker
unix.Malloc checker will warn if a memory allocation function
(malloc, calloc, realloc, alloca) is called with a tainted
(attacker controlled) size parameter.
A large, maliciously set size value can trigger memory exhaustion.
To get this warning, the alpha.security.taint.TaintPropagation checker
also needs to be switched on.
The warning will only be emitted, if the analyzer cannot prove
that the size is below reasonable bounds (<SIZE_MAX/4).
---
clang/docs/analyzer/checkers.rst | 35 +++++++
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 96 ++++++++++++++++---
clang/test/Analysis/malloc.c | 42 +++++++-
3 files changed, 159 insertions(+), 14 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 3a31708a1e9de..c1874aada6218 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1314,6 +1314,41 @@ Check for memory leaks, double free, and use-after-free problems. Traces memory
.. literalinclude:: checkers/unix_malloc_example.c
:language: c
+If the ``alpha.security.taint.TaintPropagation`` checker is enabled, the checker
+warns for cases when the ``size`` parameter of the ``malloc`` , ``calloc``,
+``realloc``, ``alloca`` is tainted (potentially attacker controlled). If an
+attacker can inject a large value as the size parameter, memory exhaustion
+denial of service attack can be carried out.
+
+The analyzer emits warning only if it cannot prove that the size parameter is
+within reasonable bounds (``<= SIZE_MAX/4``). This functionality partially
+covers the SEI Cert coding standard rule `INT04-C
+<https://wiki.sei.cmu.edu/confluence/display/c/INT04-C.+Enforce+limits+on+integer+values+originating+from+tainted+sources>`_.
+
+You can silence this warning either by bound checking the ``size`` parameter, or
+by explicitly marking the ``size`` parameter as sanitized. See the
+:ref:`alpha-security-taint-TaintPropagation` checker for more details.
+
+.. code-block:: c
+
+ void t1(void) {
+ size_t size;
+ scanf("%zu", &size);
+ int *p = malloc(size); // warn: malloc is called with a tainted (potentially attacker controlled) value
+ free(p);
+ }
+
+ void t3(void) {
+ size_t size;
+ scanf("%zu", &size);
+ if (1024<size)
+ return;
+ int *p = malloc(size); // No warning expected as the the user input is bound
+ free(p);
+ }
+
+.. _unix-MismatchedDeallocator:
+
.. _unix-MallocSizeof:
unix.MallocSizeof (C)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 34af7fb131f5a..e5e4db082baab 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -60,6 +60,7 @@
#include "clang/Basic/TargetInfo.h"
#include "clang/Lex/Lexer.h"
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Checkers/Taint.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
@@ -365,6 +366,7 @@ class MallocChecker
mutable std::unique_ptr<BugType> BT_MismatchedDealloc;
mutable std::unique_ptr<BugType> BT_OffsetFree[CK_NumCheckKinds];
mutable std::unique_ptr<BugType> BT_UseZerroAllocated[CK_NumCheckKinds];
+ mutable std::unique_ptr<BugType> BT_TaintedAlloc[CK_NumCheckKinds];
#define CHECK_FN(NAME) \
void NAME(const CallEvent &Call, CheckerContext &C) const;
@@ -462,6 +464,13 @@ class MallocChecker
};
bool isMemCall(const CallEvent &Call) const;
+ void reportTaintBug(StringRef Msg, ProgramStateRef State, CheckerContext &C,
+ llvm::ArrayRef<SymbolRef> TaintedSyms,
+ AllocationFamily Family, const Expr *SizeEx) const;
+
+ void CheckTaintedness(CheckerContext &C, const CallEvent &Call,
+ const SVal SizeSVal, ProgramStateRef State,
+ AllocationFamily Family) const;
// TODO: Remove mutable by moving the initializtaion to the registry function.
mutable std::optional<uint64_t> KernelZeroFlagVal;
@@ -521,9 +530,9 @@ class MallocChecker
/// malloc leaves it undefined.
/// \param [in] State The \c ProgramState right before allocation.
/// \returns The ProgramState right after allocation.
- [[nodiscard]] static ProgramStateRef
+ [[nodiscard]] ProgramStateRef
MallocMemAux(CheckerContext &C, const CallEvent &Call, const Expr *SizeEx,
- SVal Init, ProgramStateRef State, AllocationFamily Family);
+ SVal Init, ProgramStateRef State, AllocationFamily Family) const;
/// Models memory allocation.
///
@@ -534,9 +543,10 @@ class MallocChecker
/// malloc leaves it undefined.
/// \param [in] State The \c ProgramState right before allocation.
/// \returns The ProgramState right after allocation.
- [[nodiscard]] static ProgramStateRef
- MallocMemAux(CheckerContext &C, const CallEvent &Call, SVal Size, SVal Init,
- ProgramStateRef State, AllocationFamily Family);
+ [[nodiscard]] ProgramStateRef MallocMemAux(CheckerContext &C,
+ const CallEvent &Call, SVal Size,
+ SVal Init, ProgramStateRef State,
+ AllocationFamily Family) const;
// Check if this malloc() for special flags. At present that means M_ZERO or
// __GFP_ZERO (in which case, treat it like calloc).
@@ -649,8 +659,9 @@ class MallocChecker
/// \param [in] Call The expression that reallocated memory
/// \param [in] State The \c ProgramState right before reallocation.
/// \returns The ProgramState right after allocation.
- [[nodiscard]] static ProgramStateRef
- CallocMem(CheckerContext &C, const CallEvent &Call, ProgramStateRef State);
+ [[nodiscard]] ProgramStateRef CallocMem(CheckerContext &C,
+ const CallEvent &Call,
+ ProgramStateRef State) const;
/// See if deallocation happens in a suspicious context. If so, escape the
/// pointers that otherwise would have been deallocated and return true.
@@ -1779,7 +1790,7 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
const CallEvent &Call,
const Expr *SizeEx, SVal Init,
ProgramStateRef State,
- AllocationFamily Family) {
+ AllocationFamily Family) const {
if (!State)
return nullptr;
@@ -1787,10 +1798,71 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
return MallocMemAux(C, Call, C.getSVal(SizeEx), Init, State, Family);
}
+void MallocChecker::reportTaintBug(StringRef Msg, ProgramStateRef State,
+ CheckerContext &C,
+ llvm::ArrayRef<SymbolRef> TaintedSyms,
+ AllocationFamily Family,
+ const Expr *SizeEx) const {
+ if (ExplodedNode *N = C.generateErrorNode(State)) {
+
+ std::optional<MallocChecker::CheckKind> CheckKind =
+ getCheckIfTracked(Family);
+ if (!CheckKind)
+ return;
+ if (!BT_TaintedAlloc[*CheckKind])
+ BT_TaintedAlloc[*CheckKind].reset(new BugType(CheckNames[*CheckKind],
+ "Tainted Memory Allocation",
+ categories::MemoryError));
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ *BT_TaintedAlloc[*CheckKind], Msg, N);
+
+ bugreporter::trackExpressionValue(N, SizeEx, *R);
+ for (auto Sym : TaintedSyms)
+ R->markInteresting(Sym);
+ C.emitReport(std::move(R));
+ }
+}
+
+void MallocChecker::CheckTaintedness(CheckerContext &C, const CallEvent &Call,
+ const SVal SizeSVal, ProgramStateRef State,
+ AllocationFamily Family) const {
+ std::vector<SymbolRef> TaintedSyms =
+ clang::ento::taint::getTaintedSymbols(State, SizeSVal);
+ if (!TaintedSyms.empty()) {
+ SValBuilder &SVB = C.getSValBuilder();
+ QualType SizeTy = SVB.getContext().getSizeType();
+ QualType CmpTy = SVB.getConditionType();
+ // In case the symbol is tainted, we give a warning if the
+ // size is larger than SIZE_MAX/4
+ BasicValueFactory &BVF = SVB.getBasicValueFactory();
+ const llvm::APSInt MaxValInt = BVF.getMaxValue(SizeTy);
+ NonLoc MaxLength =
+ SVB.makeIntVal(MaxValInt / APSIntType(MaxValInt).getValue(4));
+ std::optional<NonLoc> SizeNL = SizeSVal.getAs<NonLoc>();
+ auto Cmp = SVB.evalBinOpNN(State, BO_GE, *SizeNL, MaxLength, CmpTy)
+ .getAs<DefinedOrUnknownSVal>();
+ if (!Cmp)
+ return;
+ auto [StateTooLarge, StateNotTooLarge] = State->assume(*Cmp);
+ if (!StateTooLarge && StateNotTooLarge) {
+ // we can prove that size is not too large so ok.
+ return;
+ }
+
+ std::string Callee = "Memory allocation function";
+ if (Call.getCalleeIdentifier())
+ Callee = Call.getCalleeIdentifier()->getName().str();
+ reportTaintBug(
+ Callee + " is called with a tainted (potentially attacker controlled) "
+ "value. Make sure the value is bound checked.",
+ State, C, TaintedSyms, Family, Call.getArgExpr(0));
+ }
+}
+
ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
const CallEvent &Call, SVal Size,
SVal Init, ProgramStateRef State,
- AllocationFamily Family) {
+ AllocationFamily Family) const {
if (!State)
return nullptr;
@@ -1819,9 +1891,7 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
if (Size.isUndef())
Size = UnknownVal();
- // TODO: If Size is tainted and we cannot prove that it is within
- // reasonable bounds, emit a warning that an attacker may
- // provoke a memory exhaustion error.
+ CheckTaintedness(C, Call, Size, State, AF_Malloc);
// Set the region's extent.
State = setDynamicExtent(State, RetVal.getAsRegion(),
@@ -2761,7 +2831,7 @@ MallocChecker::ReallocMemAux(CheckerContext &C, const CallEvent &Call,
ProgramStateRef MallocChecker::CallocMem(CheckerContext &C,
const CallEvent &Call,
- ProgramStateRef State) {
+ ProgramStateRef State) const {
if (!State)
return nullptr;
diff --git a/clang/test/Analysis/malloc.c b/clang/test/Analysis/malloc.c
index e5cb45ba73352..6dba76a57d83f 100644
--- a/clang/test/Analysis/malloc.c
+++ b/clang/test/Analysis/malloc.c
@@ -3,7 +3,8 @@
// RUN: -analyzer-checker=alpha.deadcode.UnreachableCode \
// RUN: -analyzer-checker=alpha.core.CastSize \
// RUN: -analyzer-checker=unix \
-// RUN: -analyzer-checker=debug.ExprInspection
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation
#include "Inputs/system-header-simulator.h"
@@ -48,6 +49,45 @@ void myfoo(int *p);
void myfooint(int p);
char *fooRetPtr(void);
+void t1(void) {
+ size_t size;
+ scanf("%zu", &size);
+ int *p = malloc(size); // expected-warning{{malloc is called with a tainted (potentially attacker controlled) value}}
+ free(p);
+}
+
+void t2(void) {
+ size_t size;
+ scanf("%zu", &size);
+ int *p = calloc(size,2); // expected-warning{{calloc is called with a tainted (potentially attacker controlled) value}}
+ free(p);
+}
+
+void t3(void) {
+ size_t size;
+ scanf("%zu", &size);
+ if (1024<size)
+ return;
+ int *p = malloc(size); // No warning expected as the the user input is bound
+ free(p);
+}
+
+void t4(void) {
+ size_t size;
+ int *p = malloc(sizeof(int));
+ scanf("%zu", &size);
+ p = (int*) realloc((void*) p, size); // // expected-warning{{realloc is called with a tainted (potentially attacker controlled) value}}
+ free(p);
+}
+
+void t5(void) {
+ size_t size;
+ int *p = alloca(sizeof(int));
+ scanf("%zu", &size);
+ p = (int*) alloca(size); // // expected-warning{{alloca is called with a tainted (potentially attacker controlled) value}}
+}
+
+
void f1(void) {
int *p = malloc(12);
return; // expected-warning{{Potential leak of memory pointed to by 'p'}}
>From 17960b623df51f2f274831d160ed44016b81daf2 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Thu, 23 May 2024 10:12:48 +0200
Subject: [PATCH 2/8] Addressing review commnents
- Create a new optional checker optin.taint.TaintMalloc
- Add test case for testing taint diagnostic notes
---
clang/docs/analyzer/checkers.rst | 77 ++++++++--------
.../clang/StaticAnalyzer/Checkers/Checkers.td | 13 +++
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 87 ++++++++++---------
clang/test/Analysis/malloc.c | 27 +++---
.../test/Analysis/taint-diagnostic-visitor.c | 12 ++-
5 files changed, 124 insertions(+), 92 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index c1874aada6218..4e17839edaab7 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -599,7 +599,47 @@ Warns when a nullable pointer is returned from a function that has _Nonnull retu
optin
^^^^^
-Checkers for portability, performance or coding style specific rules.
+Checkers for portability, performance, optional security and coding style specific rules.
+
+.. _optin-taint-TaintMalloc:
+
+optin.taint.TaintMalloc
+"""""""""""""""""""""""
+
+This checker warns for cases when the ``size`` parameter of the ``malloc`` ,
+``calloc``, ``realloc``, ``alloca`` is tainted (potentially attacker controlled).
+If an attacker can inject a large value as the size parameter, memory exhaustion
+denial of service attack can be carried out.
+
+The ``alpha.security.taint.TaintPropagation`` checker also needs to be enabled for
+this checker to give warnings.
+
+The analyzer emits warning only if it cannot prove that the size parameter is
+within reasonable bounds (``<= SIZE_MAX/4``). This functionality partially
+covers the SEI Cert coding standard rule `INT04-C
+<https://wiki.sei.cmu.edu/confluence/display/c/INT04-C.+Enforce+limits+on+integer+values+originating+from+tainted+sources>`_.
+
+You can silence this warning either by bound checking the ``size`` parameter, or
+by explicitly marking the ``size`` parameter as sanitized. See the
+:ref:`alpha-security-taint-TaintPropagation` checker for more details.
+
+.. code-block:: c
+
+ void t1(void) {
+ size_t size;
+ scanf("%zu", &size);
+ int *p = malloc(size); // warn: malloc is called with a tainted (potentially attacker controlled) value
+ free(p);
+ }
+
+ void t3(void) {
+ size_t size = 0;
+ scanf("%zu", &size);
+ if (1024 < size)
+ return;
+ int *p = malloc(size); // No warning expected as the the user input is bound
+ free(p);
+ }
.. _optin-core-EnumCastOutOfRange:
@@ -1314,41 +1354,6 @@ Check for memory leaks, double free, and use-after-free problems. Traces memory
.. literalinclude:: checkers/unix_malloc_example.c
:language: c
-If the ``alpha.security.taint.TaintPropagation`` checker is enabled, the checker
-warns for cases when the ``size`` parameter of the ``malloc`` , ``calloc``,
-``realloc``, ``alloca`` is tainted (potentially attacker controlled). If an
-attacker can inject a large value as the size parameter, memory exhaustion
-denial of service attack can be carried out.
-
-The analyzer emits warning only if it cannot prove that the size parameter is
-within reasonable bounds (``<= SIZE_MAX/4``). This functionality partially
-covers the SEI Cert coding standard rule `INT04-C
-<https://wiki.sei.cmu.edu/confluence/display/c/INT04-C.+Enforce+limits+on+integer+values+originating+from+tainted+sources>`_.
-
-You can silence this warning either by bound checking the ``size`` parameter, or
-by explicitly marking the ``size`` parameter as sanitized. See the
-:ref:`alpha-security-taint-TaintPropagation` checker for more details.
-
-.. code-block:: c
-
- void t1(void) {
- size_t size;
- scanf("%zu", &size);
- int *p = malloc(size); // warn: malloc is called with a tainted (potentially attacker controlled) value
- free(p);
- }
-
- void t3(void) {
- size_t size;
- scanf("%zu", &size);
- if (1024<size)
- return;
- int *p = malloc(size); // No warning expected as the the user input is bound
- free(p);
- }
-
-.. _unix-MismatchedDeallocator:
-
.. _unix-MallocSizeof:
unix.MallocSizeof (C)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 40f443047bd4b..d6fb5049e98cb 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -36,6 +36,8 @@ def CoreAlpha : Package<"core">, ParentPackage<Alpha>;
// Note: OptIn is *not* intended for checkers that are too noisy to be on by
// default. Such checkers belong in the alpha package.
def OptIn : Package<"optin">;
+def TaintOptIn : Package<"taint">, ParentPackage<OptIn>;
+
def CoreOptIn : Package<"core">, ParentPackage<OptIn>;
// In the Portability package reside checkers for finding code that relies on
@@ -452,6 +454,7 @@ def EnumCastOutOfRangeChecker : Checker<"EnumCastOutOfRange">,
} // end "optin.core"
+
//===----------------------------------------------------------------------===//
// Unix API checkers.
//===----------------------------------------------------------------------===//
@@ -625,6 +628,16 @@ def BlockInCriticalSectionChecker : Checker<"BlockInCriticalSection">,
} // end "alpha.unix"
+let ParentPackage = TaintOptIn in {
+
+def TaintMallocChecker: Checker<"TaintMalloc">,
+ HelpText<"Check for memory allocations, where the size parameter "
+ "might be a tainted (attacker controlled) value.">,
+ Dependencies<[DynamicMemoryModeling]>,
+ Documentation<HasDocumentation>;
+
+} // end "optin.taint"
+
//===----------------------------------------------------------------------===//
// C++ checkers.
//===----------------------------------------------------------------------===//
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index e5e4db082baab..f163e68096e44 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -323,6 +323,7 @@ class MallocChecker
CK_NewDeleteLeaksChecker,
CK_MismatchedDeallocatorChecker,
CK_InnerPointerChecker,
+ CK_TaintMallocChecker,
CK_NumCheckKinds
};
@@ -366,7 +367,7 @@ class MallocChecker
mutable std::unique_ptr<BugType> BT_MismatchedDealloc;
mutable std::unique_ptr<BugType> BT_OffsetFree[CK_NumCheckKinds];
mutable std::unique_ptr<BugType> BT_UseZerroAllocated[CK_NumCheckKinds];
- mutable std::unique_ptr<BugType> BT_TaintedAlloc[CK_NumCheckKinds];
+ mutable std::unique_ptr<BugType> BT_TaintedAlloc;
#define CHECK_FN(NAME) \
void NAME(const CallEvent &Call, CheckerContext &C) const;
@@ -1803,22 +1804,22 @@ void MallocChecker::reportTaintBug(StringRef Msg, ProgramStateRef State,
llvm::ArrayRef<SymbolRef> TaintedSyms,
AllocationFamily Family,
const Expr *SizeEx) const {
- if (ExplodedNode *N = C.generateErrorNode(State)) {
- std::optional<MallocChecker::CheckKind> CheckKind =
- getCheckIfTracked(Family);
- if (!CheckKind)
- return;
- if (!BT_TaintedAlloc[*CheckKind])
- BT_TaintedAlloc[*CheckKind].reset(new BugType(CheckNames[*CheckKind],
- "Tainted Memory Allocation",
- categories::MemoryError));
- auto R = std::make_unique<PathSensitiveBugReport>(
- *BT_TaintedAlloc[*CheckKind], Msg, N);
+ if (!ChecksEnabled[CK_TaintMallocChecker])
+ return;
+
+ if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
+ if (!BT_TaintedAlloc)
+ BT_TaintedAlloc.reset(new BugType(CheckNames[CK_TaintMallocChecker],
+ "Tainted Memory Allocation",
+ categories::TaintedData));
+ auto R = std::make_unique<PathSensitiveBugReport>(*BT_TaintedAlloc, Msg, N);
+ R->addRange(SizeEx->getSourceRange());
bugreporter::trackExpressionValue(N, SizeEx, *R);
- for (auto Sym : TaintedSyms)
- R->markInteresting(Sym);
+ for (auto TaintedSym : TaintedSyms) {
+ R->markInteresting(TaintedSym);
+ }
C.emitReport(std::move(R));
}
}
@@ -1827,36 +1828,37 @@ void MallocChecker::CheckTaintedness(CheckerContext &C, const CallEvent &Call,
const SVal SizeSVal, ProgramStateRef State,
AllocationFamily Family) const {
std::vector<SymbolRef> TaintedSyms =
- clang::ento::taint::getTaintedSymbols(State, SizeSVal);
- if (!TaintedSyms.empty()) {
- SValBuilder &SVB = C.getSValBuilder();
- QualType SizeTy = SVB.getContext().getSizeType();
- QualType CmpTy = SVB.getConditionType();
- // In case the symbol is tainted, we give a warning if the
- // size is larger than SIZE_MAX/4
- BasicValueFactory &BVF = SVB.getBasicValueFactory();
- const llvm::APSInt MaxValInt = BVF.getMaxValue(SizeTy);
- NonLoc MaxLength =
- SVB.makeIntVal(MaxValInt / APSIntType(MaxValInt).getValue(4));
- std::optional<NonLoc> SizeNL = SizeSVal.getAs<NonLoc>();
- auto Cmp = SVB.evalBinOpNN(State, BO_GE, *SizeNL, MaxLength, CmpTy)
- .getAs<DefinedOrUnknownSVal>();
- if (!Cmp)
- return;
- auto [StateTooLarge, StateNotTooLarge] = State->assume(*Cmp);
- if (!StateTooLarge && StateNotTooLarge) {
- // we can prove that size is not too large so ok.
- return;
- }
+ taint::getTaintedSymbols(State, SizeSVal);
+ if (TaintedSyms.empty())
+ return;
- std::string Callee = "Memory allocation function";
- if (Call.getCalleeIdentifier())
- Callee = Call.getCalleeIdentifier()->getName().str();
- reportTaintBug(
- Callee + " is called with a tainted (potentially attacker controlled) "
- "value. Make sure the value is bound checked.",
- State, C, TaintedSyms, Family, Call.getArgExpr(0));
+ SValBuilder &SVB = C.getSValBuilder();
+ QualType SizeTy = SVB.getContext().getSizeType();
+ QualType CmpTy = SVB.getConditionType();
+ // In case the symbol is tainted, we give a warning if the
+ // size is larger than SIZE_MAX/4
+ BasicValueFactory &BVF = SVB.getBasicValueFactory();
+ const llvm::APSInt MaxValInt = BVF.getMaxValue(SizeTy);
+ NonLoc MaxLength =
+ SVB.makeIntVal(MaxValInt / APSIntType(MaxValInt).getValue(4));
+ std::optional<NonLoc> SizeNL = SizeSVal.getAs<NonLoc>();
+ auto Cmp = SVB.evalBinOpNN(State, BO_GE, *SizeNL, MaxLength, CmpTy)
+ .getAs<DefinedOrUnknownSVal>();
+ if (!Cmp)
+ return;
+ auto [StateTooLarge, StateNotTooLarge] = State->assume(*Cmp);
+ if (!StateTooLarge && StateNotTooLarge) {
+ // we can prove that size is not too large so ok.
+ return;
}
+
+ std::string Callee = "Memory allocation function";
+ if (Call.getCalleeIdentifier())
+ Callee = Call.getCalleeIdentifier()->getName().str();
+ reportTaintBug(
+ Callee + " is called with a tainted (potentially attacker controlled) "
+ "value. Make sure the value is bound checked.",
+ State, C, TaintedSyms, Family, Call.getArgExpr(0));
}
ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
@@ -3804,3 +3806,4 @@ REGISTER_CHECKER(MallocChecker)
REGISTER_CHECKER(NewDeleteChecker)
REGISTER_CHECKER(NewDeleteLeaksChecker)
REGISTER_CHECKER(MismatchedDeallocatorChecker)
+REGISTER_CHECKER(TaintMallocChecker)
diff --git a/clang/test/Analysis/malloc.c b/clang/test/Analysis/malloc.c
index 6dba76a57d83f..70b7f89a8d6d3 100644
--- a/clang/test/Analysis/malloc.c
+++ b/clang/test/Analysis/malloc.c
@@ -4,7 +4,8 @@
// RUN: -analyzer-checker=alpha.core.CastSize \
// RUN: -analyzer-checker=unix \
// RUN: -analyzer-checker=debug.ExprInspection \
-// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation
+// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation \
+// RUN: -analyzer-checker=optin.taint.TaintMalloc
#include "Inputs/system-header-simulator.h"
@@ -50,41 +51,41 @@ void myfooint(int p);
char *fooRetPtr(void);
void t1(void) {
- size_t size;
+ size_t size = 0;
scanf("%zu", &size);
int *p = malloc(size); // expected-warning{{malloc is called with a tainted (potentially attacker controlled) value}}
free(p);
}
void t2(void) {
- size_t size;
+ size_t size = 0;
scanf("%zu", &size);
int *p = calloc(size,2); // expected-warning{{calloc is called with a tainted (potentially attacker controlled) value}}
free(p);
}
void t3(void) {
- size_t size;
+ size_t size = 0;
scanf("%zu", &size);
- if (1024<size)
+ if (1024 < size)
return;
int *p = malloc(size); // No warning expected as the the user input is bound
free(p);
}
void t4(void) {
- size_t size;
- int *p = malloc(sizeof(int));
- scanf("%zu", &size);
- p = (int*) realloc((void*) p, size); // // expected-warning{{realloc is called with a tainted (potentially attacker controlled) value}}
+ size_t size = 0;
+ int *p = malloc(sizeof(int));
+ scanf("%zu", &size);
+ p = (int*) realloc((void*) p, size); // expected-warning{{realloc is called with a tainted (potentially attacker controlled) value}}
free(p);
}
void t5(void) {
- size_t size;
- int *p = alloca(sizeof(int));
- scanf("%zu", &size);
- p = (int*) alloca(size); // // expected-warning{{alloca is called with a tainted (potentially attacker controlled) value}}
+ size_t size = 0;
+ int *p = alloca(sizeof(int));
+ scanf("%zu", &size);
+ p = (int*) alloca(size); // expected-warning{{alloca is called with a tainted (potentially attacker controlled) value}}
}
diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c b/clang/test/Analysis/taint-diagnostic-visitor.c
index b8b3710a7013e..dcec6c5236a5c 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 -analyzer-output=text -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2,optin.taint.TaintMalloc -analyzer-output=text -verify %s
// This file is for testing enhanced diagnostics produced by the GenericTaintChecker
@@ -115,3 +115,13 @@ void multipleTaintedArgs(void) {
system(buf); // expected-warning {{Untrusted data is passed to a system call}}
// expected-note at -1{{Untrusted data is passed to a system call}}
}
+
+void testTaintedMalloc(){
+ size_t size = 0;
+ scanf("%zu", &size); // expected-note {{Value assigned to 'size'}}
+ // expected-note at -1 {{Taint originated here}}
+ // expected-note at -2 {{Taint propagated to the 2nd argument}}
+ int *p = malloc(size);// expected-warning{{malloc is called with a tainted (potentially attacker controlled) value}}
+ // expected-note at -1{{malloc is called with a tainted (potentially attacker controlled) value}}
+ free(p);
+}
>From 6d404bac76b24d6e4203bec29259dd1b283f9579 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Mon, 27 May 2024 16:54:29 +0200
Subject: [PATCH 3/8] minor review fixes
---
clang/docs/analyzer/checkers.rst | 79 +++++++++----------
.../clang/StaticAnalyzer/Checkers/Checkers.td | 30 ++++---
2 files changed, 57 insertions(+), 52 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 4e17839edaab7..613a6962fbac4 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -601,46 +601,6 @@ optin
Checkers for portability, performance, optional security and coding style specific rules.
-.. _optin-taint-TaintMalloc:
-
-optin.taint.TaintMalloc
-"""""""""""""""""""""""
-
-This checker warns for cases when the ``size`` parameter of the ``malloc`` ,
-``calloc``, ``realloc``, ``alloca`` is tainted (potentially attacker controlled).
-If an attacker can inject a large value as the size parameter, memory exhaustion
-denial of service attack can be carried out.
-
-The ``alpha.security.taint.TaintPropagation`` checker also needs to be enabled for
-this checker to give warnings.
-
-The analyzer emits warning only if it cannot prove that the size parameter is
-within reasonable bounds (``<= SIZE_MAX/4``). This functionality partially
-covers the SEI Cert coding standard rule `INT04-C
-<https://wiki.sei.cmu.edu/confluence/display/c/INT04-C.+Enforce+limits+on+integer+values+originating+from+tainted+sources>`_.
-
-You can silence this warning either by bound checking the ``size`` parameter, or
-by explicitly marking the ``size`` parameter as sanitized. See the
-:ref:`alpha-security-taint-TaintPropagation` checker for more details.
-
-.. code-block:: c
-
- void t1(void) {
- size_t size;
- scanf("%zu", &size);
- int *p = malloc(size); // warn: malloc is called with a tainted (potentially attacker controlled) value
- free(p);
- }
-
- void t3(void) {
- size_t size = 0;
- scanf("%zu", &size);
- if (1024 < size)
- return;
- int *p = malloc(size); // No warning expected as the the user input is bound
- free(p);
- }
-
.. _optin-core-EnumCastOutOfRange:
optin.core.EnumCastOutOfRange (C, C++)
@@ -978,6 +938,45 @@ optin.portability.UnixAPI
"""""""""""""""""""""""""
Finds implementation-defined behavior in UNIX/Posix functions.
+.. _optin-taint-TaintMalloc:
+
+optin.taint.TaintMalloc
+"""""""""""""""""""""""
+
+This checker warns for cases when the ``size`` parameter of the ``malloc`` ,
+``calloc``, ``realloc``, ``alloca`` is tainted (potentially attacker controlled).
+If an attacker can inject a large value as the size parameter, memory exhaustion
+denial of service attack can be carried out.
+
+The ``alpha.security.taint.TaintPropagation`` checker also needs to be enabled for
+this checker to give warnings.
+
+The analyzer emits warning only if it cannot prove that the size parameter is
+within reasonable bounds (``<= SIZE_MAX/4``). This functionality partially
+covers the SEI Cert coding standard rule `INT04-C
+<https://wiki.sei.cmu.edu/confluence/display/c/INT04-C.+Enforce+limits+on+integer+values+originating+from+tainted+sources>`_.
+
+You can silence this warning either by bound checking the ``size`` parameter, or
+by explicitly marking the ``size`` parameter as sanitized. See the
+:ref:`alpha-security-taint-TaintPropagation` checker for more details.
+
+.. code-block:: c
+
+ void t1(void) {
+ size_t size;
+ scanf("%zu", &size);
+ int *p = malloc(size); // warn: malloc is called with a tainted (potentially attacker controlled) value
+ free(p);
+ }
+
+ void t3(void) {
+ size_t size = 0;
+ scanf("%zu", &size);
+ if (1024 < size)
+ return;
+ int *p = malloc(size); // No warning expected as the the user input is bound
+ free(p);
+ }
.. _security-checkers:
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index d6fb5049e98cb..fb8e593534911 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -36,7 +36,6 @@ def CoreAlpha : Package<"core">, ParentPackage<Alpha>;
// Note: OptIn is *not* intended for checkers that are too noisy to be on by
// default. Such checkers belong in the alpha package.
def OptIn : Package<"optin">;
-def TaintOptIn : Package<"taint">, ParentPackage<OptIn>;
def CoreOptIn : Package<"core">, ParentPackage<OptIn>;
@@ -45,6 +44,9 @@ def CoreOptIn : Package<"core">, ParentPackage<OptIn>;
// development, but unwanted for developers who target only a single platform.
def PortabilityOptIn : Package<"portability">, ParentPackage<OptIn>;
+// Optional checkers related to taint security analysis.
+def TaintOptIn : Package<"taint">, ParentPackage<OptIn>;
+
def Nullability : Package<"nullability">,
PackageOptions<[
CmdLineOption<Boolean,
@@ -454,7 +456,6 @@ def EnumCastOutOfRangeChecker : Checker<"EnumCastOutOfRange">,
} // end "optin.core"
-
//===----------------------------------------------------------------------===//
// Unix API checkers.
//===----------------------------------------------------------------------===//
@@ -628,16 +629,6 @@ def BlockInCriticalSectionChecker : Checker<"BlockInCriticalSection">,
} // end "alpha.unix"
-let ParentPackage = TaintOptIn in {
-
-def TaintMallocChecker: Checker<"TaintMalloc">,
- HelpText<"Check for memory allocations, where the size parameter "
- "might be a tainted (attacker controlled) value.">,
- Dependencies<[DynamicMemoryModeling]>,
- Documentation<HasDocumentation>;
-
-} // end "optin.taint"
-
//===----------------------------------------------------------------------===//
// C++ checkers.
//===----------------------------------------------------------------------===//
@@ -1731,6 +1722,21 @@ def UnixAPIPortabilityChecker : Checker<"UnixAPI">,
} // end optin.portability
+
+//===----------------------------------------------------------------------===//
+// Taint checkers.
+//===----------------------------------------------------------------------===//
+
+let ParentPackage = TaintOptIn in {
+
+def TaintMallocChecker: Checker<"TaintMalloc">,
+ HelpText<"Check for memory allocations, where the size parameter "
+ "might be a tainted (attacker controlled) value.">,
+ Dependencies<[DynamicMemoryModeling]>,
+ Documentation<HasDocumentation>;
+
+} // end "optin.taint"
+
//===----------------------------------------------------------------------===//
// NonDeterminism checkers.
//===----------------------------------------------------------------------===//
>From 4456ae942b701481e2c7fed01c7012c4a0baccb8 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Tue, 28 May 2024 14:57:41 +0200
Subject: [PATCH 4/8] Fixing review comments
- Handling of C++ operator new[] allocation was added to the checker with test cases
- The checker is renamed to optin.taint.TaintAlloc, as besides malloc it handles the c++ new array allocations too
- Test cases and documentation was updated
---
clang/docs/analyzer/checkers.rst | 16 +++++++++---
.../clang/StaticAnalyzer/Checkers/Checkers.td | 2 +-
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 26 ++++++++++---------
clang/test/Analysis/malloc.c | 2 +-
clang/test/Analysis/malloc.cpp | 24 ++++++++++++++---
.../test/Analysis/taint-diagnostic-visitor.c | 7 +++--
6 files changed, 51 insertions(+), 26 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 613a6962fbac4..1ff1f1fe328c5 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -938,13 +938,14 @@ optin.portability.UnixAPI
"""""""""""""""""""""""""
Finds implementation-defined behavior in UNIX/Posix functions.
-.. _optin-taint-TaintMalloc:
+.. _optin-taint-TaintAlloc:
-optin.taint.TaintMalloc
-"""""""""""""""""""""""
+optin.taint.TaintAlloc (C, C++)
+"""""""""""""""""""""""""""""""
This checker warns for cases when the ``size`` parameter of the ``malloc`` ,
-``calloc``, ``realloc``, ``alloca`` is tainted (potentially attacker controlled).
+``calloc``, ``realloc``, ``alloca`` or the size parameter of the
+array new C++ operator is tainted (potentially attacker controlled).
If an attacker can inject a large value as the size parameter, memory exhaustion
denial of service attack can be carried out.
@@ -978,6 +979,13 @@ by explicitly marking the ``size`` parameter as sanitized. See the
free(p);
}
+ void tcpp(void) {
+ size_t size = 0;
+ scanf("%zu", &size);
+ int *ptr = new int[size];// warn: Memory allocation function is called with a tainted (potentially attacker controlled) value
+ delete[] ptr;
+ }
+
.. _security-checkers:
security
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index fb8e593534911..4e5c9aeab4329 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1729,7 +1729,7 @@ def UnixAPIPortabilityChecker : Checker<"UnixAPI">,
let ParentPackage = TaintOptIn in {
-def TaintMallocChecker: Checker<"TaintMalloc">,
+def TaintAllocChecker: Checker<"TaintAlloc">,
HelpText<"Check for memory allocations, where the size parameter "
"might be a tainted (attacker controlled) value.">,
Dependencies<[DynamicMemoryModeling]>,
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index f163e68096e44..c31a443066194 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -323,7 +323,7 @@ class MallocChecker
CK_NewDeleteLeaksChecker,
CK_MismatchedDeallocatorChecker,
CK_InnerPointerChecker,
- CK_TaintMallocChecker,
+ CK_TaintAllocChecker,
CK_NumCheckKinds
};
@@ -467,7 +467,7 @@ class MallocChecker
bool isMemCall(const CallEvent &Call) const;
void reportTaintBug(StringRef Msg, ProgramStateRef State, CheckerContext &C,
llvm::ArrayRef<SymbolRef> TaintedSyms,
- AllocationFamily Family, const Expr *SizeEx) const;
+ AllocationFamily Family) const;
void CheckTaintedness(CheckerContext &C, const CallEvent &Call,
const SVal SizeSVal, ProgramStateRef State,
@@ -1707,6 +1707,12 @@ MallocChecker::processNewAllocation(const CXXAllocatorCall &Call,
// MallocUpdateRefState() instead of MallocMemAux() which breaks the
// existing binding.
SVal Target = Call.getObjectUnderConstruction();
+ if (Call.getOriginExpr()->isArray()) {
+ std::optional<const Expr *> SizeEx = NE->getArraySize();
+ if (SizeEx)
+ CheckTaintedness(C, Call, C.getSVal(*SizeEx), State, AF_CXXNewArray);
+ }
+
State = MallocUpdateRefState(C, NE, State, Family, Target);
State = ProcessZeroAllocCheck(Call, 0, State, Target);
return State;
@@ -1802,21 +1808,17 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
void MallocChecker::reportTaintBug(StringRef Msg, ProgramStateRef State,
CheckerContext &C,
llvm::ArrayRef<SymbolRef> TaintedSyms,
- AllocationFamily Family,
- const Expr *SizeEx) const {
+ AllocationFamily Family) const {
- if (!ChecksEnabled[CK_TaintMallocChecker])
+ if (!ChecksEnabled[CK_TaintAllocChecker])
return;
- if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
+ if (ExplodedNode *N = C.generateNonFatalErrorNode(State, this)) {
if (!BT_TaintedAlloc)
- BT_TaintedAlloc.reset(new BugType(CheckNames[CK_TaintMallocChecker],
+ BT_TaintedAlloc.reset(new BugType(CheckNames[CK_TaintAllocChecker],
"Tainted Memory Allocation",
categories::TaintedData));
auto R = std::make_unique<PathSensitiveBugReport>(*BT_TaintedAlloc, Msg, N);
-
- R->addRange(SizeEx->getSourceRange());
- bugreporter::trackExpressionValue(N, SizeEx, *R);
for (auto TaintedSym : TaintedSyms) {
R->markInteresting(TaintedSym);
}
@@ -1858,7 +1860,7 @@ void MallocChecker::CheckTaintedness(CheckerContext &C, const CallEvent &Call,
reportTaintBug(
Callee + " is called with a tainted (potentially attacker controlled) "
"value. Make sure the value is bound checked.",
- State, C, TaintedSyms, Family, Call.getArgExpr(0));
+ State, C, TaintedSyms, Family);
}
ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
@@ -3806,4 +3808,4 @@ REGISTER_CHECKER(MallocChecker)
REGISTER_CHECKER(NewDeleteChecker)
REGISTER_CHECKER(NewDeleteLeaksChecker)
REGISTER_CHECKER(MismatchedDeallocatorChecker)
-REGISTER_CHECKER(TaintMallocChecker)
+REGISTER_CHECKER(TaintAllocChecker)
diff --git a/clang/test/Analysis/malloc.c b/clang/test/Analysis/malloc.c
index 70b7f89a8d6d3..421ee72ff42b1 100644
--- a/clang/test/Analysis/malloc.c
+++ b/clang/test/Analysis/malloc.c
@@ -5,7 +5,7 @@
// RUN: -analyzer-checker=unix \
// RUN: -analyzer-checker=debug.ExprInspection \
// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation \
-// RUN: -analyzer-checker=optin.taint.TaintMalloc
+// RUN: -analyzer-checker=optin.taint.TaintAlloc
#include "Inputs/system-header-simulator.h"
diff --git a/clang/test/Analysis/malloc.cpp b/clang/test/Analysis/malloc.cpp
index 300b344ab25d6..6ec51e92af4ee 100644
--- a/clang/test/Analysis/malloc.cpp
+++ b/clang/test/Analysis/malloc.cpp
@@ -3,7 +3,9 @@
// RUN: -analyzer-checker=alpha.deadcode.UnreachableCode \
// RUN: -analyzer-checker=alpha.core.CastSize \
// RUN: -analyzer-checker=unix.Malloc \
-// RUN: -analyzer-checker=cplusplus.NewDelete
+// RUN: -analyzer-checker=cplusplus.NewDelete \
+// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation \
+// RUN: -analyzer-checker=optin.taint.TaintAlloc
// RUN: %clang_analyze_cc1 -w -verify %s \
// RUN: -triple i386-unknown-linux-gnu \
@@ -11,14 +13,18 @@
// RUN: -analyzer-checker=alpha.deadcode.UnreachableCode \
// RUN: -analyzer-checker=alpha.core.CastSize \
// RUN: -analyzer-checker=unix.Malloc \
-// RUN: -analyzer-checker=cplusplus.NewDelete
+// RUN: -analyzer-checker=cplusplus.NewDelete \
+// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation \
+// RUN: -analyzer-checker=optin.taint.TaintAlloc
// RUN: %clang_analyze_cc1 -w -verify %s -DTEST_INLINABLE_ALLOCATORS \
// RUN: -analyzer-checker=core \
// RUN: -analyzer-checker=alpha.deadcode.UnreachableCode \
// RUN: -analyzer-checker=alpha.core.CastSize \
// RUN: -analyzer-checker=unix.Malloc \
-// RUN: -analyzer-checker=cplusplus.NewDelete
+// RUN: -analyzer-checker=cplusplus.NewDelete \
+// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation \
+// RUN: -analyzer-checker=optin.taint.TaintAlloc
// RUN: %clang_analyze_cc1 -w -verify %s -DTEST_INLINABLE_ALLOCATORS \
// RUN: -triple i386-unknown-linux-gnu \
@@ -26,7 +32,9 @@
// RUN: -analyzer-checker=alpha.deadcode.UnreachableCode \
// RUN: -analyzer-checker=alpha.core.CastSize \
// RUN: -analyzer-checker=unix.Malloc \
-// RUN: -analyzer-checker=cplusplus.NewDelete
+// RUN: -analyzer-checker=cplusplus.NewDelete \
+// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation \
+// RUN: -analyzer-checker=optin.taint.TaintAlloc
#include "Inputs/system-header-simulator-cxx.h"
@@ -36,6 +44,14 @@ void free(void *);
void *realloc(void *ptr, size_t size);
void *calloc(size_t nmemb, size_t size);
char *strdup(const char *s);
+int scanf( const char* format, ... );
+
+void taintAlloc() {
+ size_t size = 0;
+ scanf("%zu", &size);
+ int *ptr = new int[size];// expected-warning{{Memory allocation function is called with a tainted (potentially attacker controlled) value}}
+ delete[] ptr;
+}
void checkThatMallocCheckerIsRunning() {
malloc(4);
diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c b/clang/test/Analysis/taint-diagnostic-visitor.c
index dcec6c5236a5c..5e5effd861c19 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.TaintMalloc -analyzer-output=text -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2,optin.taint.TaintAlloc -analyzer-output=text -verify %s
// This file is for testing enhanced diagnostics produced by the GenericTaintChecker
@@ -118,9 +118,8 @@ void multipleTaintedArgs(void) {
void testTaintedMalloc(){
size_t size = 0;
- scanf("%zu", &size); // expected-note {{Value assigned to 'size'}}
- // expected-note at -1 {{Taint originated here}}
- // expected-note at -2 {{Taint propagated to the 2nd argument}}
+ scanf("%zu", &size); // expected-note {{Taint originated here}}
+ // expected-note at -1 {{Taint propagated to the 2nd argument}}
int *p = malloc(size);// expected-warning{{malloc is called with a tainted (potentially attacker controlled) value}}
// expected-note at -1{{malloc is called with a tainted (potentially attacker controlled) value}}
free(p);
>From ebdc4a712c50ad6c12365bac420db8dc1d98a505 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Wed, 29 May 2024 18:20:22 +0200
Subject: [PATCH 5/8] formatting related review fixes
---
clang/docs/analyzer/checkers.rst | 6 +++---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 16 +++++++---------
2 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 1ff1f1fe328c5..fcfc64fdeadca 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -963,14 +963,14 @@ by explicitly marking the ``size`` parameter as sanitized. See the
.. code-block:: c
- void t1(void) {
+ void vulnerable(void) {
size_t size;
scanf("%zu", &size);
int *p = malloc(size); // warn: malloc is called with a tainted (potentially attacker controlled) value
free(p);
}
- void t3(void) {
+ void not_vulnerable(void) {
size_t size = 0;
scanf("%zu", &size);
if (1024 < size)
@@ -979,7 +979,7 @@ by explicitly marking the ``size`` parameter as sanitized. See the
free(p);
}
- void tcpp(void) {
+ void vulnerable_cpp(void) {
size_t size = 0;
scanf("%zu", &size);
int *ptr = new int[size];// warn: Memory allocation function is called with a tainted (potentially attacker controlled) value
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index c31a443066194..62b052f9b6641 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -469,7 +469,7 @@ class MallocChecker
llvm::ArrayRef<SymbolRef> TaintedSyms,
AllocationFamily Family) const;
- void CheckTaintedness(CheckerContext &C, const CallEvent &Call,
+ void checkTaintedness(CheckerContext &C, const CallEvent &Call,
const SVal SizeSVal, ProgramStateRef State,
AllocationFamily Family) const;
@@ -1710,7 +1710,7 @@ MallocChecker::processNewAllocation(const CXXAllocatorCall &Call,
if (Call.getOriginExpr()->isArray()) {
std::optional<const Expr *> SizeEx = NE->getArraySize();
if (SizeEx)
- CheckTaintedness(C, Call, C.getSVal(*SizeEx), State, AF_CXXNewArray);
+ checkTaintedness(C, Call, C.getSVal(*SizeEx), State, AF_CXXNewArray);
}
State = MallocUpdateRefState(C, NE, State, Family, Target);
@@ -1809,10 +1809,6 @@ void MallocChecker::reportTaintBug(StringRef Msg, ProgramStateRef State,
CheckerContext &C,
llvm::ArrayRef<SymbolRef> TaintedSyms,
AllocationFamily Family) const {
-
- if (!ChecksEnabled[CK_TaintAllocChecker])
- return;
-
if (ExplodedNode *N = C.generateNonFatalErrorNode(State, this)) {
if (!BT_TaintedAlloc)
BT_TaintedAlloc.reset(new BugType(CheckNames[CK_TaintAllocChecker],
@@ -1826,9 +1822,11 @@ void MallocChecker::reportTaintBug(StringRef Msg, ProgramStateRef State,
}
}
-void MallocChecker::CheckTaintedness(CheckerContext &C, const CallEvent &Call,
+void MallocChecker::checkTaintedness(CheckerContext &C, const CallEvent &Call,
const SVal SizeSVal, ProgramStateRef State,
AllocationFamily Family) const {
+ if (!ChecksEnabled[CK_TaintAllocChecker])
+ return;
std::vector<SymbolRef> TaintedSyms =
taint::getTaintedSymbols(State, SizeSVal);
if (TaintedSyms.empty())
@@ -1850,7 +1848,7 @@ void MallocChecker::CheckTaintedness(CheckerContext &C, const CallEvent &Call,
return;
auto [StateTooLarge, StateNotTooLarge] = State->assume(*Cmp);
if (!StateTooLarge && StateNotTooLarge) {
- // we can prove that size is not too large so ok.
+ // We can prove that size is not too large so there is no issue.
return;
}
@@ -1895,7 +1893,7 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
if (Size.isUndef())
Size = UnknownVal();
- CheckTaintedness(C, Call, Size, State, AF_Malloc);
+ checkTaintedness(C, Call, Size, State, AF_Malloc);
// Set the region's extent.
State = setDynamicExtent(State, RetVal.getAsRegion(),
>From 977fcc4b7a9144e81df383b70bb9c9e2d2babd4b Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Mon, 3 Jun 2024 15:00:54 +0200
Subject: [PATCH 6/8] Adding a fixme for the checker dependency, minor review
fixes
---
clang/docs/analyzer/checkers.rst | 2 +-
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td | 2 ++
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 3 +--
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index fcfc64fdeadca..32a6e4443a72a 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -964,7 +964,7 @@ by explicitly marking the ``size`` parameter as sanitized. See the
.. code-block:: c
void vulnerable(void) {
- size_t size;
+ size_t size = 0;
scanf("%zu", &size);
int *p = malloc(size); // warn: malloc is called with a tainted (potentially attacker controlled) value
free(p);
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 4e5c9aeab4329..392af5f8c5a05 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1733,6 +1733,8 @@ def TaintAllocChecker: Checker<"TaintAlloc">,
HelpText<"Check for memory allocations, where the size parameter "
"might be a tainted (attacker controlled) value.">,
Dependencies<[DynamicMemoryModeling]>,
+ //FIXME: GenericTaintChecker should be a dependency, but only after it
+ //is transformed into a modeling checker
Documentation<HasDocumentation>;
} // end "optin.taint"
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 62b052f9b6641..7d6f90be928da 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1708,8 +1708,7 @@ MallocChecker::processNewAllocation(const CXXAllocatorCall &Call,
// existing binding.
SVal Target = Call.getObjectUnderConstruction();
if (Call.getOriginExpr()->isArray()) {
- std::optional<const Expr *> SizeEx = NE->getArraySize();
- if (SizeEx)
+ if (auto SizeEx = NE->getArraySize())
checkTaintedness(C, Call, C.getSVal(*SizeEx), State, AF_CXXNewArray);
}
>From 7c3ff97fa7b55ce528f974e4fe08b9dd1beca3e9 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Wed, 5 Jun 2024 12:01:21 +0200
Subject: [PATCH 7/8] Renaming the checker to optin.taint.TaintedAlloc
---
clang/docs/analyzer/checkers.rst | 4 ++--
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td | 2 +-
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 8 ++++----
clang/test/Analysis/malloc.c | 2 +-
clang/test/Analysis/malloc.cpp | 8 ++++----
clang/test/Analysis/taint-diagnostic-visitor.c | 2 +-
6 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 32a6e4443a72a..1c88660a3ac0d 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -938,9 +938,9 @@ optin.portability.UnixAPI
"""""""""""""""""""""""""
Finds implementation-defined behavior in UNIX/Posix functions.
-.. _optin-taint-TaintAlloc:
+.. _optin-taint-TaintedAlloc:
-optin.taint.TaintAlloc (C, C++)
+optin.taint.TaintedAlloc (C, C++)
"""""""""""""""""""""""""""""""
This checker warns for cases when the ``size`` parameter of the ``malloc`` ,
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 392af5f8c5a05..3bb824fe80f94 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1729,7 +1729,7 @@ def UnixAPIPortabilityChecker : Checker<"UnixAPI">,
let ParentPackage = TaintOptIn in {
-def TaintAllocChecker: Checker<"TaintAlloc">,
+def TaintedAllocChecker: Checker<"TaintedAlloc">,
HelpText<"Check for memory allocations, where the size parameter "
"might be a tainted (attacker controlled) value.">,
Dependencies<[DynamicMemoryModeling]>,
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 7d6f90be928da..467cf8889b6d3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -323,7 +323,7 @@ class MallocChecker
CK_NewDeleteLeaksChecker,
CK_MismatchedDeallocatorChecker,
CK_InnerPointerChecker,
- CK_TaintAllocChecker,
+ CK_TaintedAllocChecker,
CK_NumCheckKinds
};
@@ -1810,7 +1810,7 @@ void MallocChecker::reportTaintBug(StringRef Msg, ProgramStateRef State,
AllocationFamily Family) const {
if (ExplodedNode *N = C.generateNonFatalErrorNode(State, this)) {
if (!BT_TaintedAlloc)
- BT_TaintedAlloc.reset(new BugType(CheckNames[CK_TaintAllocChecker],
+ BT_TaintedAlloc.reset(new BugType(CheckNames[CK_TaintedAllocChecker],
"Tainted Memory Allocation",
categories::TaintedData));
auto R = std::make_unique<PathSensitiveBugReport>(*BT_TaintedAlloc, Msg, N);
@@ -1824,7 +1824,7 @@ void MallocChecker::reportTaintBug(StringRef Msg, ProgramStateRef State,
void MallocChecker::checkTaintedness(CheckerContext &C, const CallEvent &Call,
const SVal SizeSVal, ProgramStateRef State,
AllocationFamily Family) const {
- if (!ChecksEnabled[CK_TaintAllocChecker])
+ if (!ChecksEnabled[CK_TaintedAllocChecker])
return;
std::vector<SymbolRef> TaintedSyms =
taint::getTaintedSymbols(State, SizeSVal);
@@ -3805,4 +3805,4 @@ REGISTER_CHECKER(MallocChecker)
REGISTER_CHECKER(NewDeleteChecker)
REGISTER_CHECKER(NewDeleteLeaksChecker)
REGISTER_CHECKER(MismatchedDeallocatorChecker)
-REGISTER_CHECKER(TaintAllocChecker)
+REGISTER_CHECKER(TaintedAllocChecker)
diff --git a/clang/test/Analysis/malloc.c b/clang/test/Analysis/malloc.c
index 421ee72ff42b1..8ee29aeb324f7 100644
--- a/clang/test/Analysis/malloc.c
+++ b/clang/test/Analysis/malloc.c
@@ -5,7 +5,7 @@
// RUN: -analyzer-checker=unix \
// RUN: -analyzer-checker=debug.ExprInspection \
// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation \
-// RUN: -analyzer-checker=optin.taint.TaintAlloc
+// RUN: -analyzer-checker=optin.taint.TaintedAlloc
#include "Inputs/system-header-simulator.h"
diff --git a/clang/test/Analysis/malloc.cpp b/clang/test/Analysis/malloc.cpp
index 6ec51e92af4ee..7af1b59e04a5a 100644
--- a/clang/test/Analysis/malloc.cpp
+++ b/clang/test/Analysis/malloc.cpp
@@ -5,7 +5,7 @@
// RUN: -analyzer-checker=unix.Malloc \
// RUN: -analyzer-checker=cplusplus.NewDelete \
// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation \
-// RUN: -analyzer-checker=optin.taint.TaintAlloc
+// RUN: -analyzer-checker=optin.taint.TaintedAlloc
// RUN: %clang_analyze_cc1 -w -verify %s \
// RUN: -triple i386-unknown-linux-gnu \
@@ -15,7 +15,7 @@
// RUN: -analyzer-checker=unix.Malloc \
// RUN: -analyzer-checker=cplusplus.NewDelete \
// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation \
-// RUN: -analyzer-checker=optin.taint.TaintAlloc
+// RUN: -analyzer-checker=optin.taint.TaintedAlloc
// RUN: %clang_analyze_cc1 -w -verify %s -DTEST_INLINABLE_ALLOCATORS \
// RUN: -analyzer-checker=core \
@@ -24,7 +24,7 @@
// RUN: -analyzer-checker=unix.Malloc \
// RUN: -analyzer-checker=cplusplus.NewDelete \
// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation \
-// RUN: -analyzer-checker=optin.taint.TaintAlloc
+// RUN: -analyzer-checker=optin.taint.TaintedAlloc
// RUN: %clang_analyze_cc1 -w -verify %s -DTEST_INLINABLE_ALLOCATORS \
// RUN: -triple i386-unknown-linux-gnu \
@@ -34,7 +34,7 @@
// RUN: -analyzer-checker=unix.Malloc \
// RUN: -analyzer-checker=cplusplus.NewDelete \
// RUN: -analyzer-checker=alpha.security.taint.TaintPropagation \
-// RUN: -analyzer-checker=optin.taint.TaintAlloc
+// RUN: -analyzer-checker=optin.taint.TaintedAlloc
#include "Inputs/system-header-simulator-cxx.h"
diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c b/clang/test/Analysis/taint-diagnostic-visitor.c
index 5e5effd861c19..f51423646e8ae 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.TaintAlloc -analyzer-output=text -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2,optin.taint.TaintedAlloc -analyzer-output=text -verify %s
// This file is for testing enhanced diagnostics produced by the GenericTaintChecker
>From 1fc69668460032823f5b949730a25b4da9fd3636 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Wed, 5 Jun 2024 13:21:50 +0200
Subject: [PATCH 8/8] minor doc fix
---
clang/docs/analyzer/checkers.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 1c88660a3ac0d..0bf28a3d29a60 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -941,7 +941,7 @@ Finds implementation-defined behavior in UNIX/Posix functions.
.. _optin-taint-TaintedAlloc:
optin.taint.TaintedAlloc (C, C++)
-"""""""""""""""""""""""""""""""
+"""""""""""""""""""""""""""""""""
This checker warns for cases when the ``size`` parameter of the ``malloc`` ,
``calloc``, ``realloc``, ``alloca`` or the size parameter of the
More information about the cfe-commits
mailing list