[analyzer] Fix taint propagation by remembering to the location context

[steakhal]

Summary

Fixes the issue D118987 by mapping the propagation to the callsite's
LocationContext.
This way we can keep track of the in-flight propagations.

Note that empty propagation sets won't be inserted.

[Szelethus]

Sounds about right! Just a nit, otherwise LGTM.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

441

Are there any comfortable ways of making this non-global?
https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors

Also, I wonder how efficient immutable data structures are against something like std::vector, when the element type is small and the number of elements are expected to be very low. Just a thought, I don't have particularly strong views on this.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

441

Thanks for catching it.
I grepped for a similar code and it seems like there is the REGISTER_SET_FACTORY_WITH_PROGRAMSTATE for exactly this.
Now I know about this :)

[steakhal]

using REGISTER_SET_FACTORY_WITH_PROGRAMSTATE

[steakhal]

upload the right diff

[NoQ]

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

441

ImmutableList is another good candidate. I prefer these data structures to be immutable because that's a much stronger guarantee that they won't actually ever be updated while stored in the state.

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

3

This test will be annoying to update when more debug prints are added. It might be better to test this through debug.TaintTest or a new ExprInspection function.

[NoQ]

Looks correct, thanks for fixing this!

[Szelethus]

LGTM!

[Szelethus]

Can we reopen this if the code is not upstream at this time?

[steakhal]

I'll re-land the whole stack tomorrow, by adding the asserts requirement for the tests to make the tests pass on all bots.

[steakhal]

Committed as a848a5cf2f2f2f8a621bdc5a12e0fe49dc743176.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

 } // namespace llvm
 
 /// A set which is used to pass information from call pre-visit instruction
 /// to the call post-visit. The values are signed integers, which are either
 /// ReturnValueIndex, or indexes of the pointer/reference argument, which
 /// points to data, which should be tainted on return.
 REGISTER_MAP_WITH_PROGRAMSTATE(TaintArgsOnPostVisit, const LocationContext *,
                                ImmutableSet<ArgIdxTy>)
-ImmutableSet<ArgIdxTy>::Factory TaintArgsOnPostVisitFactory;
+REGISTER_SET_FACTORY_WITH_PROGRAMSTATE(ArgIdxFactory, ArgIdxTy)
 
 void GenericTaintRuleParser::validateArgVector(const std::string &Option,
                                                const ArgVecTy &Args) const {
   for (ArgIdxTy Arg : Args) {
     if (Arg < ReturnValueIndex) {
       Mgr.reportInvalidCheckerOptionValue(
           Mgr.getChecker<GenericTaintChecker>(), Option,
           "an argument number for propagation rules greater or equal to -1");
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();
const auto WouldEscape = [](SVal V, QualType Ty) -> bool {
     const bool IsNonConstRef = Ty->isReferenceType() && !Ty.isConstQualified();
     const bool IsNonConstPtr =
         Ty->isPointerType() && !Ty->getPointeeType().isConstQualified();
 
     return IsNonConstRef || IsNonConstPtr;
   };
 
   /// Propagate taint where it is necessary.
-  ImmutableSet<ArgIdxTy> Result = TaintArgsOnPostVisitFactory.getEmptySet();
+  auto &F = State->getStateManager().get_context<ArgIdxFactory>();
+  ImmutableSet<ArgIdxTy> Result = F.getEmptySet();
+
   ForEachCallArg(
-      [this, WouldEscape, &Call, &Result](ArgIdxTy I, const Expr *E, SVal V) {
+      [this, WouldEscape, &Call, &Result, &F](ArgIdxTy I, const Expr *E,
+                                              SVal V) {
         if (PropDstArgs.contains(I)) {
           LLVM_DEBUG(llvm::dbgs() << "PreCall<"; Call.dump(llvm::dbgs());
                      llvm::dbgs()
                      << "> prepares tainting arg index: " << I << '\n';);
-          Result = TaintArgsOnPostVisitFactory.add(Result, I);
+          Result = F.add(Result, I);
         }
 
         // TODO: We should traverse all reachable memory regions via the
         // escaping parameter. Instead of doing that we simply mark only the
         // referred memory region as tainted.
         if (WouldEscape(V, E->getType())) {
           LLVM_DEBUG(if (!Result.contains(I)) {
             llvm::dbgs() << "PreCall<";
             Call.dump(llvm::dbgs());
             llvm::dbgs() << "> prepares tainting arg index: " << I << '\n';
           });
-          Result = TaintArgsOnPostVisitFactory.add(Result, I);
+          Result = F.add(Result, I);
         }
       });
 
   if (!Result.isEmpty())
     State = State->set<TaintArgsOnPostVisit>(C.getStackFrame(), Result);
   C.addTransition(State);
 }
 
void GenericTaintChecker::taintUnsafeSocketProtocol(const CallEvent &Call,
   // Allow internal communication protocols.
   bool SafeProtocol = DomName.equals("AF_SYSTEM") ||
                       DomName.equals("AF_LOCAL") || DomName.equals("AF_UNIX") ||
                       DomName.equals("AF_RESERVED_36");
   if (SafeProtocol)
     return;
 
   ProgramStateRef State = C.getState();
-  ImmutableSet<ArgIdxTy> Result = TaintArgsOnPostVisitFactory.add(
-      TaintArgsOnPostVisitFactory.getEmptySet(), ReturnValueIndex);
+  auto &F = State->getStateManager().get_context<ArgIdxFactory>();
+  ImmutableSet<ArgIdxTy> Result = F.add(F.getEmptySet(), ReturnValueIndex);
   State = State->set<TaintArgsOnPostVisit>(C.getStackFrame(), Result);
   C.addTransition(State);
 }
 
 /// Checker registration
 
 void ento::registerGenericTaintChecker(CheckerManager &Mgr) {
   Mgr.registerChecker<GenericTaintChecker>();
 }
 
 bool ento::shouldRegisterGenericTaintChecker(const CheckerManager &mgr) {
   return true;
 }

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

 // RUN: %clang_analyze_cc1 %s \
 // RUN:   -analyzer-checker=core,alpha.security.taint \
 // RUN:   -mllvm -debug-only=taint-checker \
 // RUN:   2>&1 | FileCheck %s
 
 struct _IO_FILE;
 typedef struct _IO_FILE FILE;
 FILE *fopen(const char *fname, const char *mode);
 
 void nested_call(void) {}
 
 char *fgets(char *s, int n, FILE *fp) {
   nested_call();   // no-crash: we should not try adding taint to a non-existent argument.
   return (char *)0;
 }
 
 void top(const char *fname, char *buf) {
   FILE *fp = fopen(fname, "r");
   // 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.
   // 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() {
   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}}
+}