[PATCH] D119129: [analyzer] Fix taint rule of fgets and setproctitle_init

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbf5963bf1967: [analyzer] Fix taint rule of fgets and setproctitle_init (authored by steakhal).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119129

Files:
  clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  clang/test/Analysis/taint-checker-callback-order-has-definition.c
  clang/test/Analysis/taint-checker-callback-order-without-definition.c
  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
@@ -58,9 +58,11 @@
 
 #define bool _Bool
 
+char *getenv(const char *name);
 int fscanf(FILE *restrict stream, const char *restrict format, ...);
 int sprintf(char *str, const char *format, ...);
 void setproctitle(const char *fmt, ...);
+void setproctitle_init(int argc, char *argv[], char *envp[]);
 typedef __typeof(sizeof(int)) size_t;
 
 // Define string functions. Use builtin for some of them. They all default to
@@ -404,3 +406,20 @@
 void testUnknownFunction(void (*foo)(void)) {
   foo(); // no-crash
 }
+
+void testProctitleFalseNegative() {
+  char flag[80];
+  fscanf(stdin, "%79s", flag);
+  char *argv[] = {"myapp", flag};
+  // FIXME: We should have a warning below: Untrusted data passed to sink.
+  setproctitle_init(1, argv, 0);
+}
+
+void testProctitle2(char *real_argv[]) {
+  char *app = getenv("APP_NAME");
+  if (!app)
+    return;
+  char *argv[] = {app, "--foobar"};
+  setproctitle_init(1, argv, 0);         // expected-warning {{Untrusted data is passed to a user-defined sink}}
+  setproctitle_init(1, real_argv, argv); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
Index: clang/test/Analysis/taint-checker-callback-order-without-definition.c
===================================================================
--- clang/test/Analysis/taint-checker-callback-order-without-definition.c
+++ clang/test/Analysis/taint-checker-callback-order-without-definition.c
@@ -19,16 +19,11 @@
 
   (void)fgets(buf, 42, fp); // Trigger taint propagation.
 
-  // FIXME: Why is the arg index 1 prepared for taint?
-  // Before the call it wasn't tainted, and it also shouldn't be tainted after the call.
-
   // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: -1
   // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 0
-  // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 1
   // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 2
   //
   // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: -1
   // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 0
-  // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 1
   // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 2
 }
Index: clang/test/Analysis/taint-checker-callback-order-has-definition.c
===================================================================
--- clang/test/Analysis/taint-checker-callback-order-has-definition.c
+++ clang/test/Analysis/taint-checker-callback-order-has-definition.c
@@ -25,11 +25,9 @@
   (void)fgets(buf, 42, fp); // Trigger taint propagation.
   // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: -1
   // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 0
-  // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 1
   // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 2
-
+  //
   // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: -1
   // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 0
-  // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 1
   // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 2
 }
Index: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -559,7 +559,7 @@
       {{"atoll"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
       {{"fgetc"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
       {{"fgetln"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
-      {{"fgets"}, TR::Prop({{2}}, {{0}, ReturnValueIndex})},
+      {{"fgets"}, TR::Prop({{2}}, {{0, ReturnValueIndex}})},
       {{"fscanf"}, TR::Prop({{0}}, {{}, 2})},
       {{"sscanf"}, TR::Prop({{0}}, {{}, 2})},
       {{"getc"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
@@ -632,7 +632,7 @@
   if (TR::UntrustedEnv(C)) {
     // void setproctitle_init(int argc, char *argv[], char *envp[])
     GlobalCRules.push_back(
-        {{{"setproctitle_init"}}, TR::Sink({{2}}, MsgCustomSink)});
+        {{{"setproctitle_init"}}, TR::Sink({{1, 2}}, MsgCustomSink)});
     GlobalCRules.push_back({{"getenv"}, TR::Source({{ReturnValueIndex}})});
   }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D119129.408425.patch
Type: text/x-patch
Size: 4614 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220214/a9b09532/attachment-0001.bin>