[clang] [analyzer] Fix core.VLASize checker false positive taint reports (PR #68140)
Daniel Krupp via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 13 08:46:02 PST 2024
https://github.com/dkrupp updated https://github.com/llvm/llvm-project/pull/68140
>From 4b310278d2923ff718d074a7f7c8806ad03c6401 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Tue, 3 Oct 2023 19:58:28 +0200
Subject: [PATCH 1/3] [analyzer] Fix core.VLASize checker false positive taint
reports
The checker reported a false positive on this code
void testTaintedSanitizedVLASize(void) {
int x;
scanf("%d", &x);
if (x<1)
return;
int vla[x]; // no-warning
}
After the fix, the checker only emits tainted warning if the vla size is
coming from a tainted source and it cannot prove that it is positive.
---
.../StaticAnalyzer/Checkers/VLASizeChecker.cpp | 16 ++++++++--------
clang/test/Analysis/taint-diagnostic-visitor.c | 4 ++--
clang/test/Analysis/taint-generic.c | 11 ++++++++++-
3 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
index b195d912cadfe9..46b5f5e10f0e65 100644
--- a/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
@@ -162,12 +162,6 @@ ProgramStateRef VLASizeChecker::checkVLAIndexSize(CheckerContext &C,
if (SizeV.isUnknown())
return nullptr;
- // Check if the size is tainted.
- if (isTainted(State, SizeV)) {
- reportTaintBug(SizeE, State, C, SizeV);
- return nullptr;
- }
-
// Check if the size is zero.
DefinedSVal SizeD = SizeV.castAs<DefinedSVal>();
@@ -189,10 +183,10 @@ ProgramStateRef VLASizeChecker::checkVLAIndexSize(CheckerContext &C,
DefinedOrUnknownSVal Zero = SVB.makeZeroVal(SizeTy);
SVal LessThanZeroVal = SVB.evalBinOp(State, BO_LT, SizeD, Zero, SizeTy);
+ ProgramStateRef StatePos, StateNeg;
if (std::optional<DefinedSVal> LessThanZeroDVal =
LessThanZeroVal.getAs<DefinedSVal>()) {
ConstraintManager &CM = C.getConstraintManager();
- ProgramStateRef StatePos, StateNeg;
std::tie(StateNeg, StatePos) = CM.assumeDual(State, *LessThanZeroDVal);
if (StateNeg && !StatePos) {
@@ -202,6 +196,12 @@ ProgramStateRef VLASizeChecker::checkVLAIndexSize(CheckerContext &C,
State = StatePos;
}
+ // Check if the size is tainted.
+ if ((StateNeg || StateZero) && isTainted(State, SizeV)) {
+ reportTaintBug(SizeE, State, C, SizeV);
+ return nullptr;
+ }
+
return State;
}
@@ -220,7 +220,7 @@ void VLASizeChecker::reportTaintBug(const Expr *SizeE, ProgramStateRef State,
SmallString<256> buf;
llvm::raw_svector_ostream os(buf);
os << "Declared variable-length array (VLA) ";
- os << "has tainted size";
+ os << "has a tainted (attacker controlled) size, that can be 0 or negative";
auto report = std::make_unique<PathSensitiveBugReport>(*TaintBT, os.str(), N);
report->addRange(SizeE->getSourceRange());
diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c b/clang/test/Analysis/taint-diagnostic-visitor.c
index 8a7510177f3e44..45369785ed6924 100644
--- a/clang/test/Analysis/taint-diagnostic-visitor.c
+++ b/clang/test/Analysis/taint-diagnostic-visitor.c
@@ -46,8 +46,8 @@ void taintDiagnosticVLA(void) {
scanf("%d", &x); // expected-note {{Value assigned to 'x'}}
// expected-note at -1 {{Taint originated here}}
// expected-note at -2 {{Taint propagated to the 2nd argument}}
- int vla[x]; // expected-warning {{Declared variable-length array (VLA) has tainted size}}
- // expected-note at -1 {{Declared variable-length array (VLA) has tainted size}}
+ int vla[x]; // expected-warning {{Declared variable-length array (VLA) has a tainted}}
+ // expected-note at -1 {{Declared variable-length array (VLA) has a tainted}}
}
diff --git a/clang/test/Analysis/taint-generic.c b/clang/test/Analysis/taint-generic.c
index c6a01594f15abb..ae2ae5b23aab3c 100644
--- a/clang/test/Analysis/taint-generic.c
+++ b/clang/test/Analysis/taint-generic.c
@@ -405,7 +405,16 @@ int testDivByZero(void) {
void testTaintedVLASize(void) {
int x;
scanf("%d", &x);
- int vla[x]; // expected-warning{{Declared variable-length array (VLA) has tainted size}}
+ int vla[x]; // expected-warning{{Declared variable-length array (VLA) has a tainted (attacker controlled) size, that can be 0 or negative}}
+}
+
+// Tainted-sanitized VLAs.
+void testTaintedSanitizedVLASize(void) {
+ int x;
+ scanf("%d", &x);
+ if (x<1)
+ return;
+ int vla[x]; // no-warning
}
int testTaintedAllocaMem() {
>From 94fa4af57d28854df1c6ab3e3be2a7a902b620f1 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Tue, 10 Oct 2023 14:35:52 +0200
Subject: [PATCH 2/3] fixup!
---
clang/docs/analyzer/checkers.rst | 27 ++++++++++++++++---
.../Checkers/VLASizeChecker.cpp | 2 +-
clang/test/Analysis/taint-generic.c | 2 +-
3 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index dbd6d778782353..a48ba784ee9433 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -213,9 +213,8 @@ Check for undefined results of binary operators.
core.VLASize (C)
""""""""""""""""
-Check for declarations of Variable Length Arrays of undefined or zero size.
-
- Check for declarations of VLA of undefined or zero size.
+Check for declarations of Variable Length Arrays (VLA) of undefined, zero or negative
+size.
.. code-block:: c
@@ -229,6 +228,28 @@ Check for declarations of Variable Length Arrays of undefined or zero size.
int vla2[x]; // warn: zero size
}
+
+The checker also gives warning if the `TaintPropagation` checker is switched on
+and an unbound, attacker controlled (tainted) value is used to define
+the size of the VLA.
+
+.. code-block:: c
+
+void taintedVLA(void) {
+ int x;
+ scanf("%d", &x);
+ int vla[x]; // Declared variable-length array (VLA) has a tainted (attacker controlled) size, that can be 0 or negative
+ }
+
+ void taintedVerfieidVLA(void) {
+ int x;
+ scanf("%d", &x);
+ if (x<1)
+ return;
+ int vla[x]; // no-warning. The analyzer can prove that the x can only be positive.
+ }
+
+
.. _core-uninitialized-ArraySubscript:
core.uninitialized.ArraySubscript (C)
diff --git a/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
index 46b5f5e10f0e65..4a583d7e6e1cd6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
@@ -220,7 +220,7 @@ void VLASizeChecker::reportTaintBug(const Expr *SizeE, ProgramStateRef State,
SmallString<256> buf;
llvm::raw_svector_ostream os(buf);
os << "Declared variable-length array (VLA) ";
- os << "has a tainted (attacker controlled) size, that can be 0 or negative";
+ os << "has a tainted (attacker controlled) size that can be 0 or negative";
auto report = std::make_unique<PathSensitiveBugReport>(*TaintBT, os.str(), N);
report->addRange(SizeE->getSourceRange());
diff --git a/clang/test/Analysis/taint-generic.c b/clang/test/Analysis/taint-generic.c
index ae2ae5b23aab3c..9774dc577a95c6 100644
--- a/clang/test/Analysis/taint-generic.c
+++ b/clang/test/Analysis/taint-generic.c
@@ -405,7 +405,7 @@ int testDivByZero(void) {
void testTaintedVLASize(void) {
int x;
scanf("%d", &x);
- int vla[x]; // expected-warning{{Declared variable-length array (VLA) has a tainted (attacker controlled) size, that can be 0 or negative}}
+ int vla[x]; // expected-warning{{Declared variable-length array (VLA) has a tainted (attacker controlled) size that can be 0 or negative}}
}
// Tainted-sanitized VLAs.
>From bbf44265be2b3f23cb0ebd9404839761084bad78 Mon Sep 17 00:00:00 2001
From: Daniel Krupp <daniel.krupp at ericsson.com>
Date: Tue, 13 Feb 2024 17:45:23 +0100
Subject: [PATCH 3/3] Satisfy git-clang-format
---
clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
index 7db61da62211e8..ed8c22d8626c83 100644
--- a/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
@@ -184,7 +184,8 @@ ProgramStateRef VLASizeChecker::checkVLAIndexSize(CheckerContext &C,
QualType SizeTy = SizeE->getType();
DefinedOrUnknownSVal Zero = SVB.makeZeroVal(SizeTy);
- SVal LessThanZeroVal = SVB.evalBinOp(State, BO_LT, SizeD, Zero, SVB.getConditionType());
+ SVal LessThanZeroVal =
+ SVB.evalBinOp(State, BO_LT, SizeD, Zero, SVB.getConditionType());
ProgramStateRef StatePos, StateNeg;
if (std::optional<DefinedSVal> LessThanZeroDVal =
LessThanZeroVal.getAs<DefinedSVal>()) {
More information about the cfe-commits
mailing list