[BoundV2] ArrayBoundV2 checks if the extent is tainted

[gamesh411]

Summary

Use upperbound instead of offset to check if the extent of memory region
being accessed is tainted or not.

Original author: steakhal

[gamesh411]

add analyzer tag

[martong]

clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp

208

Could you please explain why we change rawOffset to *upperBoundToCheck? And perhaps the same explanation could infiltrate into the checker's code itself as a comment to upperbound.

clang/test/Analysis/taint-diagnostic-visitor.c

46–48

Could we get rid of the seemingly unrelated malloc taint report by using an array on the stack?

[steakhal]

clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp

208

In the test attached you can see that the extent is tainted, not the offset.
Thus checking the offset for taint won't suffice.
The bug condition should depend on the calculation itself, which is basically what is done here.

clang/test/Analysis/taint-diagnostic-visitor.c

46–48

No, we need the extent to be tainted.

[martong]

clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp

208

Okay makes sense, but then please update the comment

// If we are under constrained and the index variables are tainted, report.

to mention the extent as well.

clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp

if (!upperboundToCheck)
       break;
 
     ProgramStateRef state_exceedsUpperBound, state_withinUpperBound;
     std::tie(state_exceedsUpperBound, state_withinUpperBound) =
       state->assume(*upperboundToCheck);
 
     // If we are under constrained and the index variables are tainted, report.
     if (state_exceedsUpperBound && state_withinUpperBound) {
-      SVal ByteOffset = rawOffset.getByteOffset();
-      if (isTainted(state, ByteOffset)) {
+      if (isTainted(state, *upperboundToCheck)) {
         reportOOB(checkerContext, state_exceedsUpperBound, OOB_Tainted,
-                  std::make_unique<TaintBugVisitor>(ByteOffset));
+                  std::make_unique<TaintBugVisitor>(*upperboundToCheck));
         return;
       }
     } else if (state_exceedsUpperBound) {
       // If we are constrained enough to definitely exceed the upper bound,
       // report.
       assert(!state_withinUpperBound);
       reportOOB(checkerContext, state_exceedsUpperBound, OOB_Excedes);
       return;

clang/test/Analysis/taint-diagnostic-visitor.c

-// 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,unix.Malloc,alpha.security.ArrayBoundV2 -analyzer-output=text -verify %s
 
 // This file is for testing enhanced diagnostics produced by the GenericTaintChecker
 
 int scanf(const char *restrict format, ...);
 int system(const char *command);
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t size);
+void free(void *ptr);
 
 void taintDiagnostic(void)
 {
   char buf[128];
   scanf("%s", buf); // expected-note {{Taint originated here}}
   system(buf); // expected-warning {{Untrusted data is passed to a system call}} // expected-note {{Untrusted data is passed to a system call (CERT/STR02-C. Sanitize data passed to complex subsystems)}}
 }
 

 
 void taintDiagnosticVLA(void) {
   int x;
   scanf("%d", &x); // expected-note {{Value assigned to 'x'}}
                    // expected-note@-1 {{Taint originated here}}
   int vla[x]; // expected-warning {{Declared variable-length array (VLA) has tainted size}}
               // expected-note@-1 {{Declared variable-length array (VLA) has tainted size}}
 }
+
+void taintDiagnosticMalloc(int conj) {
+  int x;
+  scanf("%d", &x);
+  // expected-note@-1 2 {{Taint originated here}} Once for malloc(tainted), once for BoundsV2.
+
+  int *p = (int *)malloc(x + conj); // Generic taint checker forbids tainted allocation.
+  // expected-warning@-1 {{Untrusted data is used to specify the buffer size}}
+  // expected-note@-2    {{Untrusted data is used to specify the buffer size}}
+
+  p[1] = 1; // BoundsV2 checker can not prove that the access is safe.
+  // expected-warning@-1 {{Out of bound memory access (index is tainted)}}
+  // expected-note@-2    {{Out of bound memory access (index is tainted)}}
+  free(p);
+}