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

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

steakhal abandoned this revision.
steakhal added a comment.

Obsoleted by D69726 .

This effort continues as the NFC D99959  patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99659/new/

https://reviews.llvm.org/D99659

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This will be obsoleted by D69726  because they 
are going to be the same symbol to begin with.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99659/new/

https://reviews.llvm.org/D99659

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

I like it, looks good to me!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99659/new/

https://reviews.llvm.org/D99659

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

steakhal updated this revision to Diff 334474.
steakhal added a comment.

Add a FIXME about placing a NoteTag describing why the extent was getting 
tainted.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99659/new/

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@-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@-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@-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,12 @@
 svalBuilder.evalEQ(State, DynSize, *DefinedSize);
 
 State = State->assume(DynSizeMatchesSize, true);
+
+// If the size of the allocation is tainted, the associated extent should 
be
+// also tainted.
+// FIXME: Add a NoteTag to describe why the extent become tainted.
+if (taint::isTainted(State, Size))
+  State = taint::addTaint(State, DynSize);
 assert(State);
   }
 


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-warnin

[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@-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@-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@-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);
   }
 


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);