diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 4473c54d8d6e3..7b7dd8b846ee6 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -917,6 +917,14 @@ let ParentPackage = Security in { def ArrayBoundChecker : Checker<"ArrayBound">, HelpText<"Warn about out of bounds access to memory">, + CheckerOptions<[ + CmdLineOption + ]>, Documentation; def FloatLoopCounter diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp index 6ad5acd4e76f2..f0392e25e7695 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp @@ -177,6 +177,10 @@ class ArrayBoundChecker : public Checker, static bool isInAddressOf(const Stmt *S, ASTContext &AC); public: + // When this parameter is set to true, the checker treats all + // unknown values as tainted and reports wanings if such values + // are used as offsets to access array elements + bool IsAggressive = false; void checkPostStmt(const ArraySubscriptExpr *E, CheckerContext &C) const { performCheck(E, C); } @@ -696,27 +700,36 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const { reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize); return; } - // ...and it can be valid as well... - if (isTainted(State, ByteOffset)) { - // ...but it's tainted, so report an error. - - // 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". - const char *OffsetName = "offset"; - if (const auto *ASE = dyn_cast(E)) - if (isTainted(State, ASE->getIdx(), C.getLocationContext())) - OffsetName = "index"; + if (!IsAggressive) { + // ...and it can be valid as well... + if (isTainted(State, ByteOffset)) { + // ...but it's tainted, so report an error. + + // 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". + const char *OffsetName = "offset"; + if (const auto *ASE = dyn_cast(E)) + if (isTainted(State, ASE->getIdx(), C.getLocationContext())) + OffsetName = "index"; + + Messages Msgs = + getTaintMsgs(Space, Reg, OffsetName, AlsoMentionUnderflow); + reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize, + /*IsTaintBug=*/true); + return; + } + // ...and it isn't tainted, so the checker will (optimistically) assume + // that the offset is in bounds and mention this in the note tag. + SUR.recordUpperBoundAssumption(*KnownSize); + } else { Messages Msgs = - getTaintMsgs(Space, Reg, OffsetName, AlsoMentionUnderflow); + getTaintMsgs(Space, Reg, "offset", AlsoMentionUnderflow); reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize, /*IsTaintBug=*/true); return; } - // ...and it isn't tainted, so the checker will (optimistically) assume - // that the offset is in bounds and mention this in the note tag. - SUR.recordUpperBoundAssumption(*KnownSize); } // Actually update the state. The "if" only fails in the extremely unlikely @@ -837,7 +850,9 @@ bool ArrayBoundChecker::isIdiomaticPastTheEndPtr(const Expr *E, } void ento::registerArrayBoundChecker(CheckerManager &mgr) { - mgr.registerChecker(); + ArrayBoundChecker *checker = mgr.registerChecker(); + checker->IsAggressive = mgr.getAnalyzerOptions().getCheckerBooleanOption( + checker, "AggressiveReport"); } bool ento::shouldRegisterArrayBoundChecker(const CheckerManager &mgr) { diff --git a/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp b/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp new file mode 100644 index 0000000000000..425ecf38a7ade --- /dev/null +++ b/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp @@ -0,0 +1,51 @@ +// RUN: %clang_analyze_cc1 -std=c++11 -Wno-array-bounds -analyzer-config unroll-loops=true -analyzer-config security.ArrayBound:AggressiveReport=true -analyzer-checker=unix,core,security.ArrayBound -verify %s + +// Test the interactions of `security.ArrayBound` with C++ features. + +void test_tainted_index_local() { + int arr[10]; + unsigned index = 10; + arr[index] = 7; + // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}} +} + +void test_tainted_index_local_range() { + int arr[10]; + for (unsigned index = 0; index < 11; index++) + arr[index] = index; + // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}} +} + +void test_tainted_index1(unsigned index) { + int arr[10]; + if (index < 12) + arr[index] = index; + // expected-warning@-1{{Potential out of bound access to 'arr' with tainted offset}} + if (index == 12) + arr[index] = index; + // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}} +} + +void test_tainted_index2(unsigned index) { + int arr[10]; + if (index < 12) + arr[index] = index; + // expected-warning@-1{{Potential out of bound access to 'arr' with tainted offset}} +} + +unsigned strlen(const char *s); +void strcpy(char *dst, char *src); +void test_ex(int argc, char *argv[]) { + char proc_name[16]; + unsigned offset = strlen(argv[0]); + if (offset == 16) { + strcpy(proc_name, argv[0]); + proc_name[offset] = 'x'; + // expected-warning@-1{{Out of bound access to memory after the end of 'proc_name'}} + } + if (offset <= 16) { + strcpy(proc_name, argv[0]); + proc_name[offset] = argv[0][16]; + // expected-warning@-1{{Potential out of bound access to the region with tainted offset}} + } +} diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c index 7936273415ad4..af88ed5eec16c 100644 --- a/clang/test/Analysis/analyzer-config.c +++ b/clang/test/Analysis/analyzer-config.c @@ -120,6 +120,7 @@ // CHECK-NEXT: region-store-small-array-limit = 5 // CHECK-NEXT: region-store-small-struct-limit = 2 // CHECK-NEXT: report-in-main-source-file = false +// CHECK-NEXT: security.ArrayBound:AggressiveReport = false // CHECK-NEXT: security.cert.env.InvalidPtr:InvalidatingGetEnv = false // CHECK-NEXT: serialize-stats = false // CHECK-NEXT: silence-checkers = ""