[PATCH] D114706: [analyzer] Fix sensitive argument logic in GenericTaintChecker

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 abandoned this revision.
gamesh411 added a comment.

This is superseded by D116025 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114706

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

[PATCH] D114706: [analyzer] Fix sensitive argument logic in GenericTaintChecker

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> The semantics of taint sinks is that if ANY of the arguments is tainted, a
> warning should be emmitted. Before this change, if there were multiple
> arguments that are sensitive, and if the first arg is not tainted, but any of
> the noninitial are tainted, a warning is not emitted. After this change the
> correct semantics is reflected in code.

It worked well with custom sinks, didn't it? So, as I see, this patch is about 
system calls and subscript indices. Could you please update the description to 
have this information?




Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:951-957
+  // Find the first argument that receives a tainted value.
+  // The report is emitted as a side-effect.
+  return llvm::find_if(SensitiveArgs, [this, &C, &Call](unsigned Idx) {
+   return Idx < Call.getNumArgs() &&
+  generateReportIfTainted(Call.getArgExpr(Idx), MsgCustomSink,
+  C);
+ }) != SensitiveArgs.end();

steakhal wrote:
> These algorithms are not supposed to have side-effects. Even though we 
> technically could have side-effects, I would still apply it separately.
> That being said, this seems to be a copy-paste version of the previous hunk, 
> like 3rd time.

> That being said, this seems to be a copy-paste version of the previous hunk, 
> like 3rd time.
+1, to put it into a named function



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114706

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

[PATCH] D114706: [analyzer] Fix sensitive argument logic in GenericTaintChecker

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

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

E.g. `execl()` and `execlp` functions are actually variadic. You should also 
account for them.
I would rather map directly to a full-fledged Propagation rule instead of to a 
sensitive arg index list. That way you could express this property.
Demonstrate this in a test.

Actually, the `CallDescriptionFlags::CDF_MaybeBuiltin` CallDescriptions will 
use the `CCtx::isCLibraryFunction()` for matching the call.
By using that the code would get significantly shorter and more readable as 
well.




Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:951-957
+  // Find the first argument that receives a tainted value.
+  // The report is emitted as a side-effect.
+  return llvm::find_if(SensitiveArgs, [this, &C, &Call](unsigned Idx) {
+   return Idx < Call.getNumArgs() &&
+  generateReportIfTainted(Call.getArgExpr(Idx), MsgCustomSink,
+  C);
+ }) != SensitiveArgs.end();

These algorithms are not supposed to have side-effects. Even though we 
technically could have side-effects, I would still apply it separately.
That being said, this seems to be a copy-paste version of the previous hunk, 
like 3rd time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114706

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

[PATCH] D114706: [analyzer] Fix sensitive argument logic in GenericTaintChecker

[Endre Fülöp via Phabricator via cfe-commits]

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

The semantics of taint sinks is that if ANY of the arguments is tainted, a
warning should be emmitted. Before this change, if there were multiple
arguments that are sensitive, and if the first arg is not tainted, but any of
the noninitial are tainted, a warning is not emitted. After this change the
correct semantics is reflected in code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114706

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

Index: clang/test/Analysis/taint-generic.c
===
--- clang/test/Analysis/taint-generic.c
+++ clang/test/Analysis/taint-generic.c
@@ -209,6 +209,15 @@
   strncat(dst2, dst, ts); // no-warning
 }
 
+void testTaintedBufferSizeNoninitialTaintedArg() {
+  size_t ts;
+  scanf("%zd", &ts);
+
+  const int nontainted_num = 1;
+
+  (void)calloc(nontainted_num, ts); //expected-warning {{Untrusted data is used to specify the buffer size}}
+}
+
 #define AF_UNIX   1   /* local to host (pipes) */
 #define AF_INET   2   /* internetwork: UDP, TCP, etc. */
 #define AF_LOCAL  AF_UNIX   /* backward compatibility */
Index: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -869,24 +869,26 @@
   // TODO: It might make sense to run this check on demand. In some cases,
   // we should check if the environment has been cleansed here. We also might
   // need to know if the user was reset before these calls(seteuid).
-  unsigned ArgNum = llvm::StringSwitch(Name)
-.Case("system", 0)
-.Case("popen", 0)
-.Case("execl", 0)
-.Case("execle", 0)
-.Case("execlp", 0)
-.Case("execv", 0)
-.Case("execvp", 0)
-.Case("execvP", 0)
-.Case("execve", 0)
-.Case("dlopen", 0)
-.Default(InvalidArgIndex);
-
-  if (ArgNum == InvalidArgIndex || Call.getNumArgs() < (ArgNum + 1))
-return false;
-
-  return generateReportIfTainted(Call.getArgExpr(ArgNum), MsgSanitizeSystemArgs,
- C);
+  const ArgVector SensitiveArgs = llvm::StringSwitch(Name)
+  .Case("system", {0})
+  .Case("popen", {0})
+  .Case("execl", {0, 1})
+  .Case("execle", {0, 1})
+  .Case("execlp", {0, 1})
+  .Case("execv", {0, 1})
+  .Case("execvp", {0, 1})
+  .Case("execvP", {0})
+  .Case("execve", {0, 1, 2})
+  .Case("dlopen", {0})
+  .Default({});
+
+  // Find the first argument that receives a tainted value.
+  // The report is emitted as a side-effect.
+  return llvm::find_if(SensitiveArgs, [this, &C, &Call](unsigned Idx) {
+   return Idx < Call.getNumArgs() &&
+  generateReportIfTainted(Call.getArgExpr(Idx),
+  MsgSanitizeSystemArgs, C);
+ }) != SensitiveArgs.end();
 }
 
 // TODO: Should this check be a part of the CString checker?
@@ -895,40 +897,45 @@
  CheckerContext &C) const {
   const auto *FDecl = Call.getDecl()->getAsFunction();
   // If the function has a buffer size argument, set ArgNum.
-  unsigned ArgNum = InvalidArgIndex;
+  ArgVector SensitiveArgs = {};
   unsigned BId = 0;
   if ((BId = FDecl->getMemoryFunctionKind())) {
 switch (BId) {
 case Builtin::BImemcpy:
 case Builtin::BImemmove:
 case Builtin::BIstrncpy:
-  ArgNum = 2;
+  SensitiveArgs = {2};
   break;
 case Builtin::BIstrndup:
-  ArgNum = 1;
+  SensitiveArgs = {1};
   break;
 default:
   break;
 }
   }
 
-  if (ArgNum == InvalidArgIndex) {
+  if (SensitiveArgs.empty()) {
 using CCtx = CheckerContext;
 if (CCtx::isCLibraryFunction(FDecl, "malloc") ||
-CCtx::isCLibraryFunction(FDecl, "calloc") ||
 CCtx::isCLibraryFunction(FDecl, "alloca"))
-  ArgNum = 0;
+  SensitiveArgs = {0};
+else if (CCtx::isCLibraryFunction(FDecl,