[analyzer] sprintf is a taint propagator not a source

[steakhal]

Summary

Due to a typo, sprintf() was recognized as a taint source instead of a
taint propagator. It was because an empty taint source list - which is
the first parameter of the TaintPropagationRule - encoded the
unconditional taint sources.
This typo effectively turned the sprintf() into an unconditional taint
source.

This patch fixes that typo and demonstrated the correct behavior with
tests.

[balazske]

Probably it is a bit more correct but it can be better to remove the sprintf like functions from taint propagation. These functions return the number of bytes written to buf. This is in some extent dependent on the input parameters but maybe more no than yes. If there are fixed characters in the format string the return value is never zero only if error occurs. (But a more taint-likely case is if the %s format is used.)

[martong]

In D112558#3089443, @balazske wrote:

Probably it is a bit more correct but it can be better to remove the sprintf like functions from taint propagation. These functions return the number of bytes written to buf. This is in some extent dependent on the input parameters but maybe more no than yes. If there are fixed characters in the format string the return value is never zero only if error occurs. (But a more taint-likely case is if the %s format is used.)

Yeah, it is worth thinking about it, but there are no decisive arguments beside each option. So, we could just keep as is.

clang/test/Analysis/taint-generic.c

345

C'mon, no covid please.
I know it's everywhere, but people die in this sh*t and it might offend any reader of this code who has been affected in anyway.

[steakhal]

In D112558#3089861, @martong wrote:

In D112558#3089443, @balazske wrote:

Probably it is a bit more correct but it can be better to remove the sprintf like functions from taint propagation. These functions return the number of bytes written to buf. This is in some extent dependent on the input parameters but maybe more no than yes. If there are fixed characters in the format string the return value is never zero only if error occurs. (But a more taint-likely case is if the %s format is used.)

Yeah, it is worth thinking about it, but there are no decisive arguments beside each option. So, we could just keep as is.

Okay, I see now. I think the "%d" format string is probably more reasonable. In that case, not sprintf() can actually return 0.

clang/test/Analysis/taint-generic.c

345

Okay.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

const auto OneOf = [FDecl](const auto &... Name) {
       bool ret = false;
       static_cast<void>(unused{
           0, (ret |= CheckerContext::isCLibraryFunction(FDecl, Name), 0)...});
       return ret;
     };
     if (OneOf("snprintf"))
       return {{1}, {0, ReturnValueIndex}, VariadicType::Src, 3};
     if (OneOf("sprintf"))
-      return {{}, {0, ReturnValueIndex}, VariadicType::Src, 2};
+      return {{1}, {0, ReturnValueIndex}, VariadicType::Src, 2};
     if (OneOf("strcpy", "stpcpy", "strcat"))
       return {{1}, {0, ReturnValueIndex}};
     if (OneOf("bcopy"))
       return {{0, 2}, {1}};
     if (OneOf("strdup", "strdupa", "wcsdup"))
       return {{0}, {ReturnValueIndex}};
   }
 

clang/test/Analysis/taint-generic.c

void constraintManagerShouldTreatAsOpaque(int rhs) {
   // This comparison used to hit an assertion in the constraint manager,
   // which didn't handle NonLoc sym-sym comparisons.
   if (i < rhs)
     return;
   if (i < rhs)
     *(volatile int *) 0; // no-warning
 }
 
+int sprintf_is_not_a_source(char *buf, char *msg) {
+  int x = sprintf(buf, "%s", msg); // no-warning
+  return 1 / x; // no-warning: 'sprintf' is not a taint source
+}
+
+int sprintf_propagates_taint(char *buf, char *msg) {
+  scanf("%s", msg);
+  int x = sprintf(buf, "%s", msg); // propagate taint!
+  return 1 / x; // expected-warning {{Division by a tainted value, possibly zero}}
+}
 
 // Test configuration
 int mySource1();
 void mySource2(int*);
 void myScanf(const char*, ...);
 int myPropagator(int, int*);
 int mySnprintf(char*, size_t, const char*, ...);
 bool isOutOfRange(const int*);