[clang][analyzer]Fix non-effective taint sanitation

[dkrupp]

Summary

There was a bug in alpha.security.taint.TaintPropagation checker in Clang Static Analyzer.
Taint filtering could only sanitize const arguments.
After this patch, taint filtering is effective also on non-const parameters.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

970

Is it only a formatting hunk?
I cannot really tell from my phone xd

996

"a a" -> "a"

1001

Why do we need this extra condition?

clang/test/Analysis/taint-generic.c

1014

Can you add a case where the parameter points to const?

1057

Wouldn't it make more sense if depending on the return value of this filter we early return like in the previous examples?

[dkrupp]

-typo fixed in comment

[dkrupp]

@steakhal thanks for the review. I addressed/answered your comments.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

970

yeap. formatting only.

970

Yes. Only formatting.

1001

That's the heart of the fix.

In case we are processing a filter rule, tthe PropDstArgs is empty. Then this part this part without the extra condition put the taintedness back to every pointed to non-const parameter. This is clearly an unwanted behaviour. See also the extra comment.

clang/test/Analysis/taint-generic.c

1014

The previous isOutOfRange(const int*) case is testing the sanitizing when we have a pointer to const.

1057

This test case simulates the case when the buffer is passed by reference and the sanitize command actually changes the contents of the buffer. For example removes shell escapes etc.

[steakhal]

Approved, modulo formatting hunks.
Unless you have really good reasons for having them as well.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

970

I would suggest not mixing formatting changes with semantic changes in a single revision. Consider not doing the formatting, or at least format in a different revision.

Adding and updating comments are fine.

1001

Makes sense. It actually closely follows how we deal with sinks.

clang/test/Analysis/taint-generic.c

1014

Thanks. Good.

1057

Then have a void return type for it please.

[dkrupp]

-formatting issues fixed
-sanitizecCmd changed to void

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

970

Yes. I realized. I reverted the formatting change.

[steakhal]

Thanks, all good!

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

ForEachCallArg([this, &State](ArgIdxTy I, const Expr *E, SVal S) {
     if (FilterArgs.contains(I)) {
       State = removeTaint(State, S);
       if (auto P = getPointeeOf(State, S))
         State = removeTaint(State, *P);
     }
   });
 
   /// Check for taint propagation sources.
-  /// A rule is relevant if PropSrcArgs is empty, or if any of its signified
+  /// A rule will make the destination variables tainted if PropSrcArgs
+  /// is empty (taints the destination
+  /// arguments unconditionally), or if any of its signified
   /// args are tainted in context of the current CallEvent.
   bool IsMatching = PropSrcArgs.isEmpty();
   std::vector<SymbolRef> TaintedSymbols;
   std::vector<ArgIdxTy> TaintedIndexes;
   ForEachCallArg([this, &C, &IsMatching, &State, &TaintedSymbols,
                   &TaintedIndexes](ArgIdxTy I, const Expr *E, SVal) {
     std::optional<SVal> TaintedSVal =
         getTaintedPointeeOrPointer(State, C.getSVal(E));
     IsMatching =
         IsMatching || (PropSrcArgs.contains(I) && TaintedSVal.has_value());
 
     // We track back tainted arguments except for stdin
     if (TaintedSVal && !isStdin(*TaintedSVal, C.getASTContext())) {
       std::vector<SymbolRef> TaintedArgSyms =
           getTaintedSymbols(State, *TaintedSVal);
       if (!TaintedArgSyms.empty()) {
         llvm::append_range(TaintedSymbols, TaintedArgSyms);
         TaintedIndexes.push_back(I);
       }
     }
   });
 
+  // Early return for propagation rules which dont match.
+  // Matching propagations, Sinks and Filters will pass this point.
   if (!IsMatching)
     return;
 
   const auto WouldEscape = [](SVal V, QualType Ty) -> bool {
     if (!isa<Loc>(V))
       return false;
 
     const bool IsNonConstRef = Ty->isReferenceType() && !Ty.isConstQualified();
     const bool IsNonConstPtr =
         Ty->isPointerType() && !Ty->getPointeeType().isConstQualified();
 
     return IsNonConstRef || IsNonConstPtr;
   };
 
   /// Propagate taint where it is necessary.
   auto &F = State->getStateManager().get_context<ArgIdxFactory>();
   ImmutableSet<ArgIdxTy> Result = F.getEmptySet();
   ForEachCallArg(
       [&](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 = F.add(Result, I);
         }
 
+        // Taint property gets lost if the variable is passed as a
+        // non-const pointer or reference to a function which is
+        // not inlined. For matching rules we want to preserve the taintedness.
         // 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())) {
+        if (WouldEscape(V, E->getType()) && getTaintedPointeeOrPointer(State, V)) {
           LLVM_DEBUG(if (!Result.contains(I)) {
             llvm::dbgs() << "PreCall<";
             Call.dump(llvm::dbgs());
             llvm::dbgs() << "> prepares tainting arg index: " << I << '\n';
           });
           Result = F.add(Result, I);
         }
       });
 
   if (!Result.isEmpty())
     State = State->set<TaintArgsOnPostVisit>(C.getStackFrame(), Result);
   const NoteTag *InjectionTag = taintOriginTrackerTag(
       C, std::move(TaintedSymbols), std::move(TaintedIndexes),
       Call.getCalleeStackFrame(0));
   C.addTransition(State, InjectionTag);
 }
 
 bool GenericTaintRule::UntrustedEnv(CheckerContext &C) {
   return !C.getAnalysisManager()
               .getAnalyzerOptions()
               .ShouldAssumeControlledEnvironment;

clang/test/Analysis/Inputs/taint-generic-config.yaml

- Name:  isOutOfRange2
     Args:  [0]
 
   # int x; // x is tainted
   # myAnotherNamespace::isOutOfRange(&x); // x is not tainted anymore
   - Name:  isOutOfRange2
     Scope: "myAnotherNamespace::"
     Args:  [0]
 
+  # char *str; // str is tainted
+  # sanitizeCmd(str) // str is not tainted anymore
+  - Name: sanitizeCmd
+    Args: [0]
+
 # A list of sink functions
 Sinks:
   # int x, y; // x and y are tainted
   # mySink(x, 0, 1); // It will warn
   # mySink(0, 1, y); // It will warn
   # mySink(0, x, 1); // It won't warn
   - Name: mySink
     Args: [0, 2]

clang/test/Analysis/taint-generic.c

 }
 
 // Test configuration
 int mySource1(void);
 void mySource2(int*);
 void myScanf(const char*, ...);
 int myPropagator(int, int*);
 int mySnprintf(char*, size_t, const char*, ...);
-bool isOutOfRange(const int*);
+bool isOutOfRange(const int*); // const filter function
+void sanitizeCmd(char*); // non-const filter function
 void mySink(int, int, int);
 
 void testConfigurationSources1(void) {
   int x = mySource1();
   Buffer[x] = 1; // expected-warning {{Out of bound memory access }}
 }
 
 void testConfigurationSources2(void) {

 
 void testConfigurationFilter(void) {
   int x = mySource1();
   if (isOutOfRange(&x)) // the filter function
     return;
   Buffer[x] = 1; // no-warning
 }
 
+void testConfigurationFilterNonConst(void) {
+  char buffer[1000];
+  myScanf("%s", buffer); // makes buffer tainted
+  system(buffer); // expected-warning {{Untrusted data is passed to a system call}}
+}
+
+void testConfigurationFilterNonConst2(void) {
+  char buffer[1000];
+  myScanf("%s", buffer); // makes buffer tainted
+  sanitizeCmd(buffer); // removes taintedness
+  system(buffer); // no-warning
+}
+
 void testConfigurationSinks(void) {
   int x = mySource1();
   mySink(x, 1, 2);
   // expected-warning@-1 {{Untrusted data is passed to a user-defined sink}}
   mySink(1, x, 2); // no-warning
   mySink(1, 2, x);
   // expected-warning@-1 {{Untrusted data is passed to a user-defined sink}}
 }