llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) <details> <summary>Changes</summary> The `security.ArrayBound` checker is to report potential out-of-bounds access when it detects that the accessed offset is tainted (potentially attacker-controlled). However, until now this only reported cases where _overflow_ was possible with the tainted offset. (This is probably an accidental oversight -- in the old implementation it was easy to forget adding a second check that would report the "potential underflow with tainted offset" case.) This commit corrects this oversight and ensures that potential underflow with a tainted offset is also correctly reported. --- I will evaluate this change on our usual set of open source projects. --- Full diff: https://github.com/llvm/llvm-project/pull/216077.diff 2 Files Affected: - (modified) clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp (+9-11) - (modified) clang/test/Analysis/ArrayBound/verbose-tests.c (+17) ``````````diff diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 9e4e8e83749ad..256b17154cd44 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -334,13 +334,15 @@ static BugDescription describeInvalidAccess(bounds::CheckResult Res, std::string(Buf)}; } -static BugDescription describeTaintBug(StringRef RegName, StringRef OffsetName, - bool AlsoMentionUnderflow) { +static BugDescription describeTaintBug(bounds::CheckResult Res, + StringRef RegName, + StringRef OffsetName) { return {formatv("Potential out of bound access to {0} with tainted {1}", RegName, OffsetName), - formatv("Access of {0} with a tainted {1} that may be {2}too large", - RegName, OffsetName, - AlsoMentionUnderflow ? "negative or " : "")}; + formatv("Access of {0} with a tainted {1} that may be{2}{3}{4}.", + RegName, OffsetName, Res.mayUnderflow() ? " negative" : "", + (Res.mayUnderflow() && Res.mayOverflow()) ? " or" : "", + Res.mayOverflow() ? " too large" : "")}; } /// When the access was ambiguous (that is, mayBeInBounds() && mayBeInvalid()), @@ -480,10 +482,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, return; } - // FIXME: Remove `Res.mayOverflow()` and provide diagnostics for the case - // when the tainted access operation cannot overflow but can underflow. - // (This is an NFC commit, so I cannot include this improvement.) - if (Res.mayOverflow() && isTainted(State, ByteOffset)) { + if (isTainted(State, ByteOffset)) { // Diagnostic detail: saying "tainted offset" is always correct, but // the common case is that 'idx' is tainted in 'arr[idx]' and then it's // nicer to say "tainted index". @@ -492,8 +491,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E, if (isTainted(State, ASE->getIdx(), C.getStackFrame())) OffsetName = "index"; - BugDescription Desc = - describeTaintBug(RegName, OffsetName, Res.mayUnderflow()); + BugDescription Desc = describeTaintBug(Res, RegName, OffsetName); reportOOB(C, State, Desc, ByteOffset, Extent, /*IsTaintBug=*/true); return; } diff --git a/clang/test/Analysis/ArrayBound/verbose-tests.c b/clang/test/Analysis/ArrayBound/verbose-tests.c index c0b1f2a8ae6be..81b5001c60f89 100644 --- a/clang/test/Analysis/ArrayBound/verbose-tests.c +++ b/clang/test/Analysis/ArrayBound/verbose-tests.c @@ -100,6 +100,23 @@ void taintedIndexNonneg(void) { // expected-note@-2 {{Access of 'TenElements' with a tainted index that may be too large}} } +void taintedIndexNonlarge(void) { + int index; + scanf("%d", &index); + // expected-note@-1 {{Taint originated here}} + // expected-note@-2 {{Taint propagated to the 2nd argument}} + + // expected-note@+2 {{Assuming 'index' is < 10}} + // expected-note@+1 {{Taking false branch}} + if (index >= 10) + return; + + TenElements[index] = 5; + // expected-warning@-1 {{Potential out of bound access to 'TenElements' with tainted index}} + // expected-note@-2 {{Access of 'TenElements' with a tainted index that may be negative}} +} + + void taintedIndexUnsigned(void) { unsigned index; scanf("%u", &index); `````````` </details> https://github.com/llvm/llvm-project/pull/216077 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
