[analyzer] Fix taint rule of fgets and setproctitle_init

[steakhal]

Summary

There was a typo in the rule.
{{0}, ReturnValueIndex} meant that the discrete index is 0 and the
variadic index is -1.
What we wanted instead is that both 0 and -1 are in the discrete index
list.

Instead of this, we wanted to express that both 0 and the
ReturnValueIndex is in the discrete arg list.

The manual inspection revealed that setproctitle_init also suffered a
probably incomplete propagation rule.

[steakhal]

rebase

[Szelethus]

LGTM! Unrelated to this review, I don't think the term 'sink' is good in a warning message, are users expected to know what that is?

Choked on my coffee reading this:

AUTHORS
Peter Wemm ⟨[e-mail]⟩ stole the idea from the Sendmail 8.7.3 source code by Eric Allman ⟨[e-mail]⟩.

[steakhal]

Committed as 7036413dc21254c8bf2f4ac62a3b087bc4b94ce8.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

RulesConstructionTy GlobalCRules{
       {{"wgetch"}, TR::Source({{}, ReturnValueIndex})},
 
       // Props
       {{"atoi"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
       {{"atol"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
       {{"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}})},
       {{"getc_unlocked"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
       {{"getdelim"}, TR::Prop({{3}}, {{0}})},
       {{"getline"}, TR::Prop({{2}}, {{0}})},
       {{"getw"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
       {{"pread"}, TR::Prop({{0, 1, 2, 3}}, {{1, ReturnValueIndex}})},
RulesConstructionTy GlobalCRules{
                     MsgTaintedBufferSize)},
       {{CDF_MaybeBuiltin, {"bcopy"}},
        TR::SinkProp({{2}}, {{0, 2}}, {{1}}, MsgTaintedBufferSize)}};
 
   // `getenv` returns taint only in untrusted environments.
   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}})});
   }
 
   StaticTaintRules.emplace(std::make_move_iterator(GlobalCRules.begin()),
                            std::make_move_iterator(GlobalCRules.end()));
 
   // User-provided taint configuration.
   CheckerManager *Mgr = C.getAnalysisManager().getCheckerManager();

clang/test/Analysis/taint-checker-callback-order-has-definition.c

void top(const char *fname, char *buf) {
   // CHECK-NEXT: PostCall<fopen(fname, "r")> actually wants to taint arg index: -1
 
   if (!fp)
     return;
 
   (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
 }

clang/test/Analysis/taint-checker-callback-order-without-definition.c

void top(const char *fname, char *buf) {
   // CHECK:      PreCall<fopen(fname, "r")> prepares tainting arg index: -1
   // CHECK-NEXT: PostCall<fopen(fname, "r")> actually wants to taint arg index: -1
 
   if (!fp)
     return;
 
   (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
 }

clang/test/Analysis/taint-generic.c

 #ifdef FILE_IS_STRUCT
 extern struct _FILE *stdin;
 #else
 extern FILE *stdin;
 #endif
 
 #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
 // the processing in the taint checker.
 #define strcpy(dest, src) \
   ((__builtin_object_size(dest, 0) != -1ULL) \
    ? __builtin___strcpy_chk (dest, src, __builtin_object_size(dest, 1)) \
    : __inline_strcpy_chk(dest, src))
void testConfigurationSinks(void) {
   mySink(1, x, 2); // no-warning
   mySink(1, 2, x);
   // expected-warning@-1 {{Untrusted data is passed to a user-defined sink}}
 }
 
 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}}
+}