[PATCH] D99659: [analyzer][taint] Extent of heap regions should get taint sometimes

[Balázs Benics via Phabricator via cfe-commits]

steakhal created this revision.
steakhal added reviewers: NoQ, vsavchenko, martong, balazske, xazax.hun.
Herald added subscribers: ASDenysPetrov, Charusso, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity.
Herald added a reviewer: Szelethus.
steakhal requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

If we allocate a tainted amount of memory, the extent of that region will clearly depend on a tainted value.

We could later make use of this, for example when we try to prove that the array access is valid (`0 <= idx < extent`).
If the inequality would depend on the tainted value, we should still emit a warning (in ArrayBoundV2), as we currently do if the `idx` is tainted.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D99659

Files:
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Analysis/malloc.c


Index: clang/test/Analysis/malloc.c
===================================================================
--- clang/test/Analysis/malloc.c
+++ clang/test/Analysis/malloc.c
@@ -3,11 +3,14 @@
 // RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
 // RUN:   -analyzer-checker=alpha.core.CastSize \
 // RUN:   -analyzer-checker=unix \
+// RUN:   -analyzer-checker=alpha.security.taint \
 // RUN:   -analyzer-checker=debug.ExprInspection
 
 #include "Inputs/system-header-simulator.h"
 
 void clang_analyzer_eval(int);
+size_t clang_analyzer_getExtent(void *);
+void clang_analyzer_isTainted(int);
 
 // Without -fms-compatibility, wchar_t isn't a builtin type. MSVC defines
 // _WCHAR_T_DEFINED if wchar_t is available. Microsoft recommends that you use
@@ -36,6 +39,7 @@
 wchar_t *wcsdup(const wchar_t *s);
 char *strndup(const char *s, size_t n);
 int memcmp(const void *s1, const void *s2, size_t n);
+int getchar(void);
 
 // Windows variants
 char *_strdup(const char *strSource);
@@ -1883,3 +1887,36 @@
   s->memP = malloc(sizeof(int));
   free(s);
 } // FIXME: should warn here
+
+void extentGetsTainted(int conj) {
+  int tainted = getchar();
+  {
+    int *p = (int *)malloc(conj);
+    clang_analyzer_isTainted(clang_analyzer_getExtent(p)); // expected-warning {{NO}}
+    free(p);
+  }
+  {
+    int *p = (int *)malloc(tainted);
+    // expected-warning at -1 {{Untrusted data is used to specify the buffer size \
+(CERT/STR31-C. Guarantee that storage for strings has sufficient space for \
+character data and the null terminator)}}
+    clang_analyzer_isTainted(clang_analyzer_getExtent(p)); // expected-warning {{YES}}
+    free(p);
+  }
+  {
+    int *p = (int *)malloc(5 + tainted);
+    // expected-warning at -1 {{Untrusted data is used to specify the buffer size \
+(CERT/STR31-C. Guarantee that storage for strings has sufficient space for \
+character data and the null terminator)}}
+    clang_analyzer_isTainted(clang_analyzer_getExtent(p)); // expected-warning {{YES}}
+    free(p);
+  }
+  {
+    int *p = (int *)malloc(conj + tainted);
+    // expected-warning at -1 {{Untrusted data is used to specify the buffer size \
+(CERT/STR31-C. Guarantee that storage for strings has sufficient space for \
+character data and the null terminator)}}
+    clang_analyzer_isTainted(clang_analyzer_getExtent(p)); // expected-warning {{YES}}
+    free(p);
+  }
+}
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -46,6 +46,7 @@
 
 #include "AllocationState.h"
 #include "InterCheckerAPI.h"
+#include "Taint.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
@@ -1601,6 +1602,11 @@
         svalBuilder.evalEQ(State, DynSize, *DefinedSize);
 
     State = State->assume(DynSizeMatchesSize, true);
+
+    // If the size of the allocation is tainted, the associated dynamic extent
+    // should be tainted as well.
+    if (taint::isTainted(State, Size))
+      State = taint::addTaint(State, DynSize);
     assert(State);
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D99659.334425.patch
Type: text/x-patch
Size: 3171 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210331/3ff7197d/attachment-0001.bin>