[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

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

steakhal added a comment.

In D71524#2304742 , @boga95 wrote:

> As far as I remember I tried to make `std::cin` tainted, but it was 
> complicated.

What sort of issues did you find by implementing that approach? Could you 
elaborate?

> I run the checker against many projects and there wasn't any false positive 
> related to this heuristic.

It seems a bit hand-wavy to me.

> We can restrict the `operator>>`  to `std::basic_stream` and cover only the 
> standard library.

If we choose this approach, then we have to do something about this, yes. This 
could be a reasonable choice.

We are arguing about whether the `operator>>` is a //propagation// or a 
//source// function.
IMO this operator should //propagate// taint, from the //stream// to an object.

I've opened the cppreference  
to find an example where your logic would introduce serious false positives.
I've spent like 2 minutes to come up with this, there might be many more:

  std::istringstream is("hello, world");
  char arr[10];
  is >> std::setw(6) >> arr;
  std::cout << "Input from \"" << is.str() << "\" with setw(6) gave \""
<< arr << "\"\n";

godbolt 

You would deliberately mark `arr` as a tainted.
Fortunately/unfortunately it's quite hard to //remove// taint - so any 
unnecessarily tainted value is a serious problem, thus requires careful design 
decisions.

This example might seem to be a made-up one, however, we should consider 
developers who want to extract data from streams - which aren't tainted.
Marking the stream itself tainted, and propagating taint over the //extraction 
operator// would not have such false positives.


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

https://reviews.llvm.org/D71524

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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Borsik Gábor via Phabricator via cfe-commits]

boga95 marked an inline comment as done.
boga95 added a comment.

In D71524#2291925 , @steakhal wrote:

> In D71524#2284386 , @Szelethus wrote:
>
>> I figured you're still working on this, sorry! I'd really like to chat about 
>> my earlier comment D71524#1917251 , 
>> as it kind of challenges the high level idea.
>
> What about marking the `std::cin` object itself as tainted and any object 
> created by `ifstream::ifstream(const char*)` or similar functions.
> Then propagate taint via the extraction operator (`operator>>`) only if the 
> stream was tainted.
> This way we could reduce the false-positives of this crude heuristic. What do 
> you think?

As far as I remember I tried to make `std::cin` tainted, but it was 
complicated. I run the checker against many projects and there wasn't any false 
positive related to this heuristic.
We can restrict the `operator>>`  to `std::basic_stream` and cover only the 
standard library. I think most of the programmers will use this in a 
conventional way, therefore it should work for their implementation too.


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

https://reviews.llvm.org/D71524

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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

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

steakhal added a comment.

In D71524#2284386 , @Szelethus wrote:

> I figured you're still working on this, sorry! I'd really like to chat about 
> my earlier comment D71524#1917251 , 
> as it kind of challenges the high level idea.

What about marking the `std::cin` object itself as tainted and any object 
created by `ifstream::ifstream(const char*)` or similar functions.
Then propagate taint via the extraction operator (`operator>>`) only if the 
stream was tainted.
This way we could reduce the false-positives of this crude heuristic. What do 
you think?


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

https://reviews.llvm.org/D71524

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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I figured you're still working on this, sorry! I'd really like to chat about my 
earlier comment D71524#1917251 , as it 
kind of challenges the high level idea.


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

https://reviews.llvm.org/D71524

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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Borsik Gábor via Phabricator via cfe-commits]

boga95 marked 9 inline comments as done.
boga95 added a comment.

Ping




Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:289-293
+  {"c_str", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"data", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"size", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"length", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"getline", {"std::", {{0}, {1, ReturnValueIndex};

NoQ wrote:
> Szelethus wrote:
> > Hmm, is this the appropriate place to put these? It seems like this job is 
> > handled in `getTaintPropagationRule`. I thought `CustomPropagations` are 
> > reserved for the config file.
> So `0` stands for `this`? Can we have a named constant please? ^.^
We are planning to move all of the propagation rules into a configuration file.


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

https://reviews.llvm.org/D71524

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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Borsik Gábor via Phabricator via cfe-commits]

boga95 updated this revision to Diff 256839.

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

https://reviews.llvm.org/D71524

Files:
  clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Taint.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/diagnostics/explicit-suppression.cpp
  clang/test/Analysis/taint-generic.cpp

Index: clang/test/Analysis/taint-generic.cpp
===
--- clang/test/Analysis/taint-generic.cpp
+++ clang/test/Analysis/taint-generic.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_analyze_cc1  -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -analyzer-config alpha.security.taint.TaintPropagation:Config=%S/Inputs/taint-generic-config.yaml -Wno-format-security -verify -std=c++11 %s
 
+#include "Inputs/system-header-simulator-cxx.h"
+
 #define BUFSIZE 10
 int Buffer[BUFSIZE];
 
@@ -124,3 +126,64 @@
   foo.myMemberScanf("%d", );
   Buffer[x] = 1; // expected-warning {{Out of bound memory access }}
 }
+
+// Test I/O
+void testCin() {
+  int x, y;
+  std::cin >> x >> y;
+  Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void testWcin() {
+  int x, y;
+  std::wcin >> x >> y;
+  Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void mySink(const std::string &, int, const char *);
+
+void testFstream() {
+  std::string str;
+  std::ifstream file("example.txt");
+  file >> str;
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testIfstream() {
+  std::string str;
+  std::fstream file("example.txt");
+  file >> str;
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testStdstring() {
+  std::string str1;
+  std::cin >> str1;
+
+  std::string  = str1;
+  mySink(str2, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+
+  const std::string  = str1;
+  mySink(str3, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testTaintedThis() {
+  std::string str;
+  mySink(std::string(), 0, str.c_str()); // no-warning
+
+  std::cin >> str;
+  mySink(std::string(), 0, str.c_str()); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testOverloadedAssignmentOp() {
+  std::string str1, str2;
+  std::cin >> str1;
+  str2 = str1;
+  mySink(str2, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testGetline() {
+  std::string str;
+  std::getline(std::cin, str);
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
Index: clang/test/Analysis/diagnostics/explicit-suppression.cpp
===
--- clang/test/Analysis/diagnostics/explicit-suppression.cpp
+++ clang/test/Analysis/diagnostics/explicit-suppression.cpp
@@ -19,6 +19,6 @@
 void testCopyNull(C *I, C *E) {
   std::copy(I, E, (C *)0);
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:699 {{Called C++ object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:742 {{Called C++ object pointer is null}}
 #endif
 }
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -579,6 +579,9 @@
 void resize(size_type count);
 void shrink_to_fit();
 void swap(basic_string );
+  private:
+unsigned size;
+char *ptr;
   };
 
   typedef basic_string string;
@@ -588,6 +591,46 @@
   typedef basic_string u32string;
 #endif
 
+  template 
+  class char_traits {};
+
+  class ios_base {};
+
+  template  >
+  class basic_ios : public ios_base {};
+
+  template  >
+  class basic_istream : virtual public basic_ios {};
+  template  >
+  class basic_ostream : virtual public basic_ios {};
+
+  using istream = basic_istream;
+  using ostream = basic_ostream;
+  using wistream = basic_istream;
+
+  istream >>(istream , int );
+  wistream >>(wistream , int );
+
+  extern istream cin;
+  extern istream wcin;
+
+  template  >
+  class basic_fstream : public basic_istream, public basic_ostream {
+  public:
+basic_fstream(const char *) {}
+  };
+  template  >
+  class basic_ifstream : public basic_istream {
+  public:
+basic_ifstream(const char *) {}
+  };
+
+  using ifstream = basic_ifstream;
+  using fstream = basic_ifstream;
+
+  istream >>(istream , string );
+  istream (istream , string );
+
   class exception {
   public:
 exception() throw();
Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ 

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.
Herald added a subscriber: ASDenysPetrov.

In D71524#1924508 , @steakhal wrote:

> I think `CallDescription` can only identify objects/functions which has 
> `IdefntifyerInfo` in them. AFAIK operators don't have such. Though somehow 
> AST matchers of Clang Tidy were triggered with this: 
> `functionDecl(hasName("operator>>"))`


Yup, we could make improvements on `CallDescription` as well :)

> I'm afraid it needs to be a different patch to replace with 
> `CallDescriptionMap`, even though I agree with you.

I don't mean to deploy the map in this patch, but if this lands as-is, the 
proposed logic will have to be revisited again.


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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

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

steakhal added a comment.

In D71524#1917251 , @Szelethus wrote:

> Are we sure this is what we want? If this is a heuristic, we should document 
> it well, and even then I'm not sure whether we want it. I'm also pretty sure 
> this would make the eventual change to `CallDescriptionMap` more difficult, 
> because the way taintedness is propagated around `std::basic_istream` not 
> really the property of the global `>>` operator and what its parameters are, 
> but rather the property of `std::basic_istream::operator>>`, 
> right? What do you think?


I think `CallDescription` can only identify objects/functions which has 
`IdefntifyerInfo` in them. AFAIK operators don't have such. Though somehow AST 
matchers of Clang Tidy were triggered with this: 
`functionDecl(hasName("operator>>"))`
I'm afraid it needs to be a different patch to replace with 
`CallDescriptionMap`, even though I agree with you.


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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:139
 
+  /// Add taint sources for extraction operator on pre-visit.
+  bool addOverloadedOpPre(const CallExpr *CE, CheckerContext ) const;

steakhal wrote:
> boga95 wrote:
> > Szelethus wrote:
> > > Extraction operator? Is that a thing?
> > I can call it `operator>>` if you think that is better.
> I think `extraction operator` is the right term for this.
> It is used in the standard: 
> http://eel.is/c++draft/input.streams#istream.extractors
> 
> 
Well, I consider myself proven wrong :)


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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:289
+  NameRuleMap CustomPropagations{
+  {"c_str", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"data", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,

Szelethus wrote:
> Hmm, is this the appropriate place to put these? It seems like this job is 
> handled in `getTaintPropagationRule`. I thought `CustomPropagations` are 
> reserved for the config file.
So `0` stands for `this`? Can we have a named constant please? ^.^



Comment at: clang/lib/StaticAnalyzer/Checkers/Taint.cpp:155-162
+  if (auto LCV = V.getAs()) {
+if (Optional binding =
+State->getStateManager().getStoreManager().getDefaultBinding(
+*LCV)) {
+  if (SymbolRef Sym = binding->getAsSymbol())
+return isTainted(State, Sym, Kind);
+}

Aha, ok, so this is your plan: only ever deal with fully conjured objects.

I suspect that it'll fail when the function that produces the object is fully 
inlined. It's probably unlikely to happen with things like `std::cin` but it's 
pretty likely to happen with custom taint sources defined by the user. Once you 
want to fix this, you'll probably have to iterate through all direct bindings 
in the structure and mark them tainted as well.



Comment at: clang/test/Analysis/taint-generic.cpp:129
+// Test I/O
+namespace std {
+template 

Let's add a header simulator for this if we didn't already.


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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

So, as far as I understand, your thinking here is that `CXXOperatorCallExpr`s 
(which are global, non-member operators) when they are `>>`, they will 
propagate taintedness from the 0th argument to the 1st and the return value, 
correct? So, if I do this:

  struct Panda {};
  
  // Turn integers into pandas!
  bool operator>>(int i, Panda p) {
return printf("Panda integer party!");
  }
  
  // Turn characters into pandas!
  bool operator>>(char i, Panda p) {
return printf("Panda character party!");
  }
  
  int main() {
Panda p1, p2;
bool success1 = getTaintedInt() >> p1;
bool success2 = getTaintedChar() >> p2;
// success1, success2, p1, p2 are all tainted
  }

Are we sure this is what we want? If this is a heuristic, we should document it 
well, and even then I'm not sure whether we want it. I'm also pretty sure this 
would make the eventual change to `CallDescriptionMap` more difficult, because 
the way taintedness is propagated around `std::basic_istream` not really the 
property of the global `>>` operator and what its parameters are, but rather 
the property of `std::basic_istream::operator>>`, right? What do 
you think?




Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:289-293
+  {"c_str", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"data", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"size", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"length", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"getline", {"std::", {{0}, {1, ReturnValueIndex};

Hmm, is this the appropriate place to put these? It seems like this job is 
handled in `getTaintPropagationRule`. I thought `CustomPropagations` are 
reserved for the config file.



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:579
+ CheckerContext ) {
+  const auto *OCE = dyn_cast(Call.getOriginExpr());
+  if (OCE) {

As far as I know, `Call.getOriginExpr()` may be null, we should probably use 
`dyn_cast_or_null`.




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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Borsik Gábor via Phabricator via cfe-commits]

boga95 updated this revision to Diff 249019.
boga95 marked 2 inline comments as done.
boga95 added a comment.

Rebase to master.


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

https://reviews.llvm.org/D71524

Files:
  clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Taint.cpp
  clang/test/Analysis/taint-generic.cpp

Index: clang/test/Analysis/taint-generic.cpp
===
--- clang/test/Analysis/taint-generic.cpp
+++ clang/test/Analysis/taint-generic.cpp
@@ -124,3 +124,127 @@
   foo.myMemberScanf("%d", );
   Buffer[x] = 1; // expected-warning {{Out of bound memory access }}
 }
+
+// Test I/O
+namespace std {
+template 
+class char_traits {};
+
+class ios_base {};
+
+template >
+class basic_ios : public ios_base {};
+
+template >
+class basic_istream : virtual public basic_ios {};
+template >
+class basic_ostream : virtual public basic_ios {};
+
+using istream = basic_istream;
+using ostream = basic_ostream;
+using wistream = basic_istream;
+
+istream >>(istream , int );
+wistream >>(wistream , int );
+
+extern istream cin;
+extern istream wcin;
+
+template >
+class basic_fstream : public basic_istream, public basic_ostream {
+public:
+  basic_fstream(const char *) {}
+};
+template >
+class basic_ifstream : public basic_istream {
+public:
+  basic_ifstream(const char *) {}
+};
+
+using ifstream = basic_ifstream;
+using fstream = basic_ifstream;
+
+template 
+class allocator {};
+
+namespace __cxx11 {
+template <
+class CharT,
+class Traits = std::char_traits,
+class Allocator = std::allocator>
+class basic_string {
+public:
+  const char *c_str();
+  basic_string operator=(const basic_string &);
+
+private:
+  unsigned size;
+  char *ptr;
+};
+} // namespace __cxx11
+
+using string = __cxx11::basic_string;
+
+istream >>(istream , string );
+istream (istream , string );
+} // namespace std
+
+void testCin() {
+  int x, y;
+  std::cin >> x >> y;
+  Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void testWcin() {
+  int x, y;
+  std::wcin >> x >> y;
+  Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void mySink(const std::string &, int, const char *);
+
+void testFstream() {
+  std::string str;
+  std::ifstream file("example.txt");
+  file >> str;
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testIfstream() {
+  std::string str;
+  std::fstream file("example.txt");
+  file >> str;
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testStdstring() {
+  std::string str1;
+  std::cin >> str1;
+
+  std::string  = str1;
+  mySink(str2, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+
+  const std::string  = str1;
+  mySink(str3, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testTaintedThis() {
+  std::string str;
+  mySink(std::string(), 0, str.c_str()); // no-warning
+
+  std::cin >> str;
+  mySink(std::string(), 0, str.c_str()); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testOverloadedAssignmentOp() {
+  std::string str1, str2;
+  std::cin >> str1;
+  str2 = str1;
+  mySink(str2, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testGetline() {
+  std::string str;
+  std::getline(std::cin, str);
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -152,6 +152,14 @@
 return isTainted(State, Sym, Kind);
   if (const MemRegion *Reg = V.getAsRegion())
 return isTainted(State, Reg, Kind);
+  if (auto LCV = V.getAs()) {
+if (Optional binding =
+State->getStateManager().getStoreManager().getDefaultBinding(
+*LCV)) {
+  if (SymbolRef Sym = binding->getAsSymbol())
+return isTainted(State, Sym, Kind);
+}
+  }
   return false;
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -138,6 +138,9 @@
   bool checkPre(const CallEvent , const FunctionData ,
 CheckerContext ) const;
 
+  /// Add taint sources for operator>> on pre-visit.
+  static bool addOverloadedOpPre(const CallEvent , CheckerContext );
+
   /// Add taint sources on a pre-visit. Returns true on matching.
   bool addSourcesPre(const CallEvent , const FunctionData ,
  CheckerContext ) const;
@@ -154,6 +157,9 @@
   /// and thus, is tainted.
   static bool 

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

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

steakhal added a comment.

In D71524#1902654 , @steakhal wrote:

> In D71524#1889566 , @boga95 wrote:
>
> > @steakhal's revision is on the top of this. Changing the order will only 
> > cause unnecessary work on both sides.
>
>
> I would happily rebase this patch if you want.


Here is the rebased version of your patch.
https://github.com/steakhal/llvm-project/compare/use-callevent...steakhal:boga-rebased-tainted-objects-D71524

Minor modifications applied:

- clang-formatted every modified lines
- simplified the `getArgWithImplicitThis` function body
- simplified the `getNumArgsWithImplicitThis` function body
- omitting the spelling of the `TaintPropagationRule` type name where obvious


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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

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

steakhal added a comment.

In D71524#1889566 , @boga95 wrote:

> @steakhal's revision is on the top of this. Changing the order will only 
> cause unnecessary work on both sides.


I would happily rebase this patch if you want.




Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:139
 
+  /// Add taint sources for extraction operator on pre-visit.
+  bool addOverloadedOpPre(const CallExpr *CE, CheckerContext ) const;

boga95 wrote:
> Szelethus wrote:
> > Extraction operator? Is that a thing?
> I can call it `operator>>` if you think that is better.
I think `extraction operator` is the right term for this.
It is used in the standard: 
http://eel.is/c++draft/input.streams#istream.extractors





Comment at: clang/test/Analysis/taint-generic.cpp:189
+  istream& getline(istream& is, string& str);
+}
+

balazske wrote:
> These `std` declarations are at a better place in 
> `system-header-simulator-cxx.h` or a similar file.
In the current form, it seems to be a bit verbose.
Why don't we create a minimal `std::string` which does not inherit from 
anything and implements the features and behavior only what is necessary.

After minimizing this class there would be no benefit moving to the 
`system-header-simulator-cxx.h` header.


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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/test/Analysis/taint-generic.cpp:189
+  istream& getline(istream& is, string& str);
+}
+

These `std` declarations are at a better place in 
`system-header-simulator-cxx.h` or a similar file.


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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D71524#1889566 , @boga95 wrote:

> @steakhal's revision is on the top of this. Changing the order will only 
> cause unnecessary work on both sides.


He recently rebased on top of master. I'm no fan of creating unnecessary work 
for either of you, and it might just be the case that my worry is greater then 
necessary. But for now, landing more patches with this infrastructure and 
having to painstakingly redo them all sounds like more work.

It'd be best to wait for @NoQ's feedback, after all, he did approve earlier 
patches. And again, sorry for not being available earlier.


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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Borsik Gábor via Phabricator via cfe-commits]

boga95 marked 6 inline comments as done.
boga95 added a comment.

@steakhal's revision is on the top of this. Changing the order will only cause 
unnecessary work on both sides.




Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:103-132
   struct FunctionData {
 FunctionData() = delete;
 FunctionData(const FunctionData &) = default;
 FunctionData(FunctionData &&) = default;
 FunctionData =(const FunctionData &) = delete;
 FunctionData =(FunctionData &&) = delete;
 

Szelethus wrote:
> Szelethus wrote:
> > I know this isn't really relevant, but isn't this redundant with 
> > `CallDescription`?
> Ah, okay, so `CallDescription` doesn't really have the `FunctionDecl`, but 
> this still feels like a duplication of functionalities.
We have already thought about that and it is on our TODO list.



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:139
 
+  /// Add taint sources for extraction operator on pre-visit.
+  bool addOverloadedOpPre(const CallExpr *CE, CheckerContext ) const;

Szelethus wrote:
> Extraction operator? Is that a thing?
I can call it `operator>>` if you think that is better.



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:202
   using ConfigDataMap =
   std::unordered_multimap>;
   using NameRuleMap = ConfigDataMap;

Szelethus wrote:
> http://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options
> 
> > We never use hash_set and unordered_set because they are generally very 
> > expensive (each insertion requires a malloc) and very non-portable.
I didn't find any multimap among the alternatives. I think the performance 
won't be an issue here, because the elements are inserted at the beginning of 
the analysis if there is any. Otherwise, we are planning to replace it with 
CallDescriptionMap.



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:268
CheckerContext ) {
-  if (isTainted(State, E, C.getLocationContext()) || isStdin(E, C))
+  if (isTainted(State, E, C.getLocationContext()) || isStdin(E, C) ||
+  isStdstream(E, C))

xazax.hun wrote:
> If we consider `Stdin`  and `Stdstream` to be tainted does it make sense to 
> fold them into `isTainted` so we never miss checking for them?
Then we have to pass the `CheckerContext` to the `isTainted` functions. I think 
this function wraps it well.


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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added a reviewer: steakhal.
Szelethus added a comment.
This revision now requires changes to proceed.

This patch is really cool, but I still feel anxious a bit about duplicating so 
much functionality, especially since we're working very hard to make 
`CallEvent` a widespread thing. @steakhal's patch D72035 
 already made some great progress in this 
direction, and should land before this one. @NoQ, do you agree that we should 
maybe stop for a bit before making the code a bit more consistent with the rest 
of the checkers?

I added @steakhal as reviewer if you don't mind.


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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Borsik Gábor via Phabricator via cfe-commits]

boga95 updated this revision to Diff 246120.
boga95 marked 5 inline comments as done.
Herald added a subscriber: martong.

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

https://reviews.llvm.org/D71524

Files:
  clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Taint.cpp
  clang/test/Analysis/taint-generic.cpp

Index: clang/test/Analysis/taint-generic.cpp
===
--- clang/test/Analysis/taint-generic.cpp
+++ clang/test/Analysis/taint-generic.cpp
@@ -124,3 +124,126 @@
   foo.myMemberScanf("%d", );
   Buffer[x] = 1; // expected-warning {{Out of bound memory access }}
 }
+
+
+// Test I/O
+namespace std {
+  template class char_traits {};
+
+  class ios_base {};
+
+  template>
+  class basic_ios : public ios_base {};
+
+  template>
+  class basic_istream : virtual public basic_ios {};
+  template>
+  class basic_ostream : virtual public basic_ios {};
+
+  using istream = basic_istream;
+  using ostream = basic_ostream;
+  using wistream = basic_istream;
+
+  istream& operator>>(istream& is, int& val);
+  wistream& operator>>(wistream& is, int& val);
+
+  extern istream cin;
+  extern istream wcin;
+
+  template>
+  class basic_fstream :
+public basic_istream, public basic_ostream {
+  public:
+  basic_fstream(const char*) {}
+  };
+  template>
+  class basic_ifstream : public basic_istream {
+  public:
+basic_ifstream(const char*) {}
+  };
+
+  using ifstream = basic_ifstream;
+  using fstream = basic_ifstream;
+
+  template class allocator {};
+
+namespace __cxx11 {
+template<
+  class CharT,
+  class Traits = std::char_traits,
+  class Allocator = std::allocator>
+class basic_string {
+public:
+  const char* c_str();
+  basic_string operator=(const basic_string&);
+private:
+  unsigned size;
+  char* ptr;
+};
+  }
+
+  using string = __cxx11::basic_string;
+
+  istream& operator>>(istream& is, string& val);
+  istream& getline(istream& is, string& str);
+}
+
+void testCin() {
+  int x, y;
+  std::cin >> x >> y;
+  Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void testWcin() {
+  int x, y;
+  std::wcin >> x >> y;
+  Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void mySink(const std::string&, int, const char*);
+
+void testFstream() {
+  std::string str;
+  std::ifstream file("example.txt");
+  file >> str;
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testIfstream() {
+  std::string str;
+  std::fstream file("example.txt");
+  file >> str;
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testStdstring() {
+  std::string str1;
+  std::cin >> str1;
+
+  std::string& str2 = str1;
+  mySink(str2, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+
+  const std::string& str3 = str1;
+  mySink(str3, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testTaintedThis() {
+  std::string str;
+  mySink(std::string(), 0, str.c_str()); // no-warning
+
+  std::cin >> str;
+  mySink(std::string(), 0, str.c_str()); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testOverloadedAssignmentOp() {
+  std::string str1, str2;
+  std::cin >> str1;
+  str2 = str1;
+  mySink(str2, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testGetline() {
+  std::string str;
+  std::getline(std::cin, str);
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -152,6 +152,14 @@
 return isTainted(State, Sym, Kind);
   if (const MemRegion *Reg = V.getAsRegion())
 return isTainted(State, Reg, Kind);
+  if (auto LCV = V.getAs()) {
+if (Optional binding =
+State->getStateManager().getStoreManager().getDefaultBinding(
+*LCV)) {
+  if (SymbolRef Sym = binding->getAsSymbol())
+return isTainted(State, Sym, Kind);
+}
+  }
   return false;
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -135,6 +135,9 @@
   bool checkPre(const CallExpr *CE, const FunctionData ,
 CheckerContext ) const;
 
+  /// Add taint sources for operator>> on pre-visit.
+  bool addOverloadedOpPre(const CallExpr *CE, CheckerContext ) const;
+
   /// Add taint sources on a pre-visit. Returns true on matching.
   bool addSourcesPre(const CallExpr *CE, 

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

A portion of my concerns are answered by this patch: D72035 
.




Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:103-132
   struct FunctionData {
 FunctionData() = delete;
 FunctionData(const FunctionData &) = default;
 FunctionData(FunctionData &&) = default;
 FunctionData =(const FunctionData &) = delete;
 FunctionData =(FunctionData &&) = delete;
 

Szelethus wrote:
> I know this isn't really relevant, but isn't this redundant with 
> `CallDescription`?
Ah, okay, so `CallDescription` doesn't really have the `FunctionDecl`, but this 
still feels like a duplication of functionalities.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:268
CheckerContext ) {
-  if (isTainted(State, E, C.getLocationContext()) || isStdin(E, C))
+  if (isTainted(State, E, C.getLocationContext()) || isStdin(E, C) ||
+  isStdstream(E, C))

If we consider `Stdin`  and `Stdstream` to be tainted does it make sense to 
fold them into `isTainted` so we never miss checking for them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I strongly agree with this comment: D70878#1780513 
, maybe the placement of functions 
like `getArg` and `getNumArgs` would be most appropriate in `CallDescription`. 
How about we try to cut down on duplicating functionalities?

I realize that many, if not all of my comments should've been placed at D70878 
, I was unfortunately not available then, but 
I think it would be a lot better if we settled on this before making the 
eventual changes too hard to switch. Checkers that were written with 
`CallDescriptionMap` (D70818 , D63915 
) or have recently been converted to it 
(D67706 , D62557 
, D68165 ) 
are a joy to look at, and reduce the overall complexity of the codebase 
greatly. Are there strong arguments against it?

The overall direction is great, as demonstrated by the test files. Its very 
exciting to see taintness analysis improving this much lately!




Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:103-132
   struct FunctionData {
 FunctionData() = delete;
 FunctionData(const FunctionData &) = default;
 FunctionData(FunctionData &&) = default;
 FunctionData =(const FunctionData &) = delete;
 FunctionData =(FunctionData &&) = delete;
 

I know this isn't really relevant, but isn't this redundant with 
`CallDescription`?



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:139
 
+  /// Add taint sources for extraction operator on pre-visit.
+  bool addOverloadedOpPre(const CallExpr *CE, CheckerContext ) const;

Extraction operator? Is that a thing?



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:202
   using ConfigDataMap =
   std::unordered_multimap>;
   using NameRuleMap = ConfigDataMap;

http://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options

> We never use hash_set and unordered_set because they are generally very 
> expensive (each insertion requires a malloc) and very non-portable.



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:373
+/// Treat implicit this parameter as the 0. argument.
+const Expr *getArg(const CallExpr *CE, unsigned ArgNum) {
+  if (const auto *MCE = dyn_cast(CE)) {

I would be shocked if this is the first time I've come across very similar 
function -- in any case, could you rename it to something like 
`getArgIgnoringImplicitThis`?



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:816
+bool GenericTaintChecker::isStdstream(const Expr *E, CheckerContext ) {
+  llvm::Regex R{".*std::basic_istream.*"};
+  std::string Error;

In this case, maybe `llvm::Regex` is overkill?  
[[https://llvm.org/doxygen/classllvm_1_1StringRef.html#a82369bea2700347f68e1f43e30d2d47b
 | `llvm::StringRef::find()`]] may be sufficient.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Borsik Gábor via Phabricator via cfe-commits]

boga95 added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71524



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

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Borsik Gábor via Phabricator via cfe-commits]

boga95 created this revision.
boga95 added reviewers: NoQ, Szelethus.
Herald added subscribers: cfe-commits, Charusso, donat.nagy, mikhail.ramalho, 
a.sidorin, szepet, baloghadamsoftware.
Herald added a project: clang.

I extended the supported C++ features:

- The `this` pointer can be tainted (0. argument)
- All `std::basic_istream` objects are tainted unconditionally (`std::cin`, 
`std::ifstream`, etc.)
- `std::getline` and some member function of `std::string` propagates taint
- Extraction operator and copy assignment propagate taint


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71524

Files:
  clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Taint.cpp
  clang/test/Analysis/taint-generic.cpp

Index: clang/test/Analysis/taint-generic.cpp
===
--- clang/test/Analysis/taint-generic.cpp
+++ clang/test/Analysis/taint-generic.cpp
@@ -124,3 +124,126 @@
   foo.myMemberScanf("%d", );
   Buffer[x] = 1; // expected-warning {{Out of bound memory access }}
 }
+
+
+// Test I/O
+namespace std {
+  template class char_traits {};
+
+  class ios_base {};
+
+  template>
+  class basic_ios : public ios_base {};
+
+  template>
+  class basic_istream : virtual public basic_ios {};
+  template>
+  class basic_ostream : virtual public basic_ios {};
+
+  using istream = basic_istream;
+  using ostream = basic_ostream;
+  using wistream = basic_istream;
+
+  istream& operator>>(istream& is, int& val);
+  wistream& operator>>(wistream& is, int& val);
+
+  extern istream cin;
+  extern istream wcin;
+
+  template>
+  class basic_fstream :
+public basic_istream, public basic_ostream {
+  public:
+  basic_fstream(const char*) {}
+  };
+  template>
+  class basic_ifstream : public basic_istream {
+  public:
+basic_ifstream(const char*) {}
+  };
+
+  using ifstream = basic_ifstream;
+  using fstream = basic_ifstream;
+
+  template class allocator {};
+
+namespace __cxx11 {
+template<
+  class CharT,
+  class Traits = std::char_traits,
+  class Allocator = std::allocator>
+class basic_string {
+public:
+  const char* c_str();
+  basic_string operator=(const basic_string&);
+private:
+  unsigned size;
+  char* ptr;
+};
+  }
+
+  using string = __cxx11::basic_string;
+
+  istream& operator>>(istream& is, string& val);
+  istream& getline(istream& is, string& str);
+}
+
+void testCin() {
+  int x, y;
+  std::cin >> x >> y;
+  Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void testWcin() {
+  int x, y;
+  std::wcin >> x >> y;
+  Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void mySink(const std::string&, int, const char*);
+
+void testFstream() {
+  std::string str;
+  std::ifstream file("example.txt");
+  file >> str;
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testIfstream() {
+  std::string str;
+  std::fstream file("example.txt");
+  file >> str;
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testStdstring() {
+  std::string str1;
+  std::cin >> str1;
+
+  std::string& str2 = str1;
+  mySink(str2, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+
+  const std::string& str3 = str1;
+  mySink(str3, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testTaintedThis() {
+  std::string str;
+  mySink(std::string(), 0, str.c_str()); // no-warning
+
+  std::cin >> str;
+  mySink(std::string(), 0, str.c_str()); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testOverloadedAssignmentOp() {
+  std::string str1, str2;
+  std::cin >> str1;
+  str2 = str1;
+  mySink(str2, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testGetline() {
+  std::string str;
+  std::getline(std::cin, str);
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -152,6 +152,14 @@
 return isTainted(State, Sym, Kind);
   if (const MemRegion *Reg = V.getAsRegion())
 return isTainted(State, Reg, Kind);
+  if (auto LCV = V.getAs()) {
+if (Optional binding =
+State->getStateManager().getStoreManager().getDefaultBinding(
+*LCV)) {
+  if (SymbolRef Sym = binding->getAsSymbol())
+return isTainted(State, Sym, Kind);
+}
+  }
   return false;
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++