[analyzer] Add more sources to Taint analysis

[gamesh411]

Summary

Add more functions as taint sources to GenericTaintChecker.

[steakhal]

Sorry for the wall-of-nits. Overall, looks great.

clang/docs/analyzer/checkers.rst

2358

typo/dup?
I cannot recognize the getcw() call. Could you please refer to the specification or an instance where it was defined?

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

552–573

If we handle gets, scanf, we should also model the *_s versions as well.

char *gets_s(char *s, rsize_t n);
int scanf_s(const char *restrict format, ...);
int fscanf_s(FILE *restrict stream, const char *restrict format, ...);
int sscanf_s(const char *restrict s, const char *restrict format, ...);
int vscanf_s(const char *restrict format, va_list arg);
int vfscanf_s(FILE *restrict stream, const char *restrict format, va_list arg);
int vsscanf_s(const char *restrict s, const char *restrict format, va_list arg);

557–559

IMO these functions are highly domain-specific.
On errors, they return specific/well-defined values e.g. -1, '?' or ':'.
That being said, the analyzer does not have this knowledge, thus it will model these as conjured symbols.
If these values were modeled as tainted, we would likely introduce the number of false-positives regarding our limited capabilities of modeling the function accurately.

tldr; I'm against these three rules; or alternatively prove that my concerns are not issues on real code bases.

563

We should check readlinkat as well.

566

In what cases can this function introduce taint?

568

On success, getgroups() returns the number of supplementary group IDs. On error, -1 is returned, and errno is set appropriately.

According to this, the return value index should be also tainted.

clang/test/Analysis/taint-generic.c

496

Well, this can never return zero.
What we should do is to do a state-split for the failure case; since the application should definitely handle a failure in this part; thus the split would be justified.

503

Sometimes we taint the fd and propagate based on that, and othertimes, we simply just return taint.
However, I think it still looks like a better tradeoff this way.
I just wanted to highlight this. Maybe a comment on the CallDescription would be beneficial in describing this discrepancy.

524

Well, it can never be zero AFAIK. It might be -1 on error, or the number of bytes stored into buf.
So, I would rather modify the example to 1 / (s + 1).

528

Please avoid spelling the given function in the name of the test directly in a verbatim manner.
If we would use the CDF_MaybeBuiltin matching mode, the get_current_dir_name_is_source would match for the CallDescription {"get_current_dir_name"} due to the way we fuzzy match for builtins.
Prefixing with Test like testGet_current_dir_name would be fine though, only the underscores are handled differently.
This applies to the rest of the test cases as well.

[steakhal]

Also great to see that the docs are in-sync to the code, which is a great plus!

[steakhal]

The pre-merge checks failed due to an unrelated test case: Driver/hip-link-bundle-archive.hip (on Windows); so I think we are good to go on that part.

[gamesh411]

• s/getcw/getwd
• add gets_s
• remove getopt variants
• add realinkat
• discuss getnameinfo?
• rename tests
• update getnameinfo
• comment on source/propagator discrepancy
• update tests where 1 / tainted, and tainted cannot be 0
• renamed tests

clang/docs/analyzer/checkers.rst

2358

getwd is the right one instead of getcw

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

552–573

I have added gets_s, and will add the _s variants for the others in the other patch that deals with the propagatorsj.

557–559

I agree that the handling of these should be in another checker, I remember some false positives ( mainly uninteresting, typical "just won't fix" errors ) relating to getopt, and a domain-specific checker could be more appropriate here.
Removed them.

563

Added

566

The getnameinfo converts from

struct sockaddr_in {
    sa_family_t    sin_family; /* address family: AF_INET */
    in_port_t      sin_port;   /* port in network byte order */
    struct in_addr sin_addr;   /* internet address */
};

/* Internet address */
struct in_addr {
    uint32_t       s_addr;     /* address in network byte order */
};

to hostname and servername strings.
One could argue that by crafting a specific IP address, that is known to resolve to a specific hostname in the running environment could lead an attacker injecting a chosen (in some circumstances arbitrary) string into the code at the point of this function.

I know this is a bit contrived, and more on the cybersecurity side of things, so I am not sure whether to add this here, or add this in a specific checker, or just leave altogether. Please share your opinion about this.

568

Added

clang/test/Analysis/taint-generic.c

496

Removed

503

Added a comment

528

fixed the test names

[steakhal]

Fewer nits this time.
We are converging!

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

566

int readlinkat(int dirfd, const char *pathname, char *buf, size_t bufsiz);

566

Let it be, I don't mind. We will remove it if we find some FPs for this.

clang/test/Analysis/taint-generic.c

374

Please, also rename this test case.

[gamesh411]

add readlinkat
rename _IO_getc testcase

[gamesh411]

readlinkat fix incoming

[gamesh411]

fix readlinkat arg index
extend testcase for readlinkat

[steakhal]

Please check the docs if they are still in sync with the content.
I think it looks great overall.

clang/docs/analyzer/checkers.rst

void test() {
    int *p = (int *)malloc(ts * sizeof(int));
      // warn: untrusted data as buffer size
  }
 
 There are built-in sources, propagations and sinks defined in code inside ``GenericTaintChecker``.
 These operations are handled even if no external taint configuration is provided.
 
 Default sources defined by ``GenericTaintChecker``:
-``fdopen``, ``fopen``, ``freopen``, ``getch``, ``getchar``, ``getchar_unlocked``, ``gets``, ``scanf``, ``socket``, ``wgetch``
+ ``_IO_getc``, ``fdopen``, ``fopen``, ``freopen``, ``get_current_dir_name``, ``getch``, ``getchar``, ``getchar_unlocked``, ``getwd``, ``getcwd``, ``getgroups``, ``gethostname``, ``getlogin``, ``getlogin_r``, ``getnameinfo``, ``gets``, ``gets_s``, ``getseuserbyname``, ``readlink``, ``readlinkat``, ``scanf``, ``scanf_s``, ``socket``, ``wgetch``
 
 Default propagations defined by ``GenericTaintChecker``:
 ``atoi``, ``atol``, ``atoll``, ``fgetc``, ``fgetln``, ``fgets``, ``fscanf``, ``sscanf``, ``getc``, ``getc_unlocked``, ``getdelim``, ``getline``, ``getw``, ``pread``, ``read``, ``strchr``, ``strrchr``, ``tolower``, ``toupper``
 
 Default sinks defined in ``GenericTaintChecker``:
 ``printf``, ``setproctitle``, ``system``, ``popen``, ``execl``, ``execle``, ``execlp``, ``execv``, ``execvp``, ``execvP``, ``execve``, ``dlopen``, ``memcpy``, ``memmove``, ``strncpy``, ``strndup``, ``malloc``, ``calloc``, ``alloca``, ``memccpy``, ``realloc``, ``bcopy``
 
 The user can configure taint sources, sinks, and propagation rules by providing a configuration file via checker option ``alpha.security.taint.TaintPropagation:Config``.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

void GenericTaintChecker::initTaintRules(CheckerContext &C) const {
   RulesConstructionTy GlobalCRules{
       // Sources
       {{"fdopen"}, TR::Source({{ReturnValueIndex}})},
       {{"fopen"}, TR::Source({{ReturnValueIndex}})},
       {{"freopen"}, TR::Source({{ReturnValueIndex}})},
       {{"getch"}, TR::Source({{ReturnValueIndex}})},
       {{"getchar"}, TR::Source({{ReturnValueIndex}})},
       {{"getchar_unlocked"}, TR::Source({{ReturnValueIndex}})},
       {{"gets"}, TR::Source({{0}, ReturnValueIndex})},
+      {{"gets_s"}, TR::Source({{0}, ReturnValueIndex})},
       {{"scanf"}, TR::Source({{}, 1})},
+      {{"scanf_s"}, TR::Source({{}, {1}})},
       {{"wgetch"}, TR::Source({{}, ReturnValueIndex})},
+      // Sometimes the line between taint sources and propagators is blurry.
+      // _IO_getc is choosen to be a source, but could also be a propagator.
+      // This way it is simpler, as modeling it as a propagator would require
+      // to model the possible sources of _IO_FILE * values, which the _IO_getc
+      // function takes as parameters.
+      {{"_IO_getc"}, TR::Source({{ReturnValueIndex}})},
+      {{"getcwd"}, TR::Source({{0, ReturnValueIndex}})},
+      {{"getwd"}, TR::Source({{0, ReturnValueIndex}})},
+      {{"readlink"}, TR::Source({{1, ReturnValueIndex}})},
+      {{"readlinkat"}, TR::Source({{2, ReturnValueIndex}})},
+      {{"get_current_dir_name"}, TR::Source({{ReturnValueIndex}})},
+      {{"gethostname"}, TR::Source({{0}})},
+      {{"getnameinfo"}, TR::Source({{2, 4}})},
+      {{"getseuserbyname"}, TR::Source({{1, 2}})},
+      {{"getgroups"}, TR::Source({{1, ReturnValueIndex}})},
+      {{"getlogin"}, TR::Source({{ReturnValueIndex}})},
+      {{"getlogin_r"}, TR::Source({{0}})},
 
       // 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}})},

clang/test/Analysis/taint-generic.c

 // RUN:     alpha.security.taint.TaintPropagation:Config=%S/Inputs/taint-generic-config-invalid-arg.yaml \
 // RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-ARG
 
 // CHECK-INVALID-ARG: (frontend): invalid input for checker option
 // CHECK-INVALID-ARG-SAME:        'alpha.security.taint.TaintPropagation:Config',
 // CHECK-INVALID-ARG-SAME:        that expects an argument number for propagation
 // CHECK-INVALID-ARG-SAME:        rules greater or equal to -1
 
+typedef long long rsize_t;
+
 int scanf(const char *restrict format, ...);
 char *gets(char *str);
+char *gets_s(char *str, rsize_t n);
 int getchar(void);
 
 typedef struct _FILE FILE;
 #ifdef FILE_IS_STRUCT
 extern struct _FILE *stdin;
 #else
 extern FILE *stdin;
 #endif

 }
 
 void testGets(void) {
   char str[50];
   gets(str);
   system(str); // expected-warning {{Untrusted data is passed to a system call}}
 }
 
+void testGets_s(void) {
+  char str[50];
+  gets_s(str, 49);
+  system(str); // expected-warning {{Untrusted data is passed to a system call}}
+}
+
 void testTaintedBufferSize(void) {
   size_t ts;
   scanf("%zd", &ts);
 
   int *buf1 = (int*)malloc(ts*sizeof(int)); // expected-warning {{Untrusted data is used to specify the buffer size}}
   char *dst = (char*)calloc(ts, sizeof(char)); //expected-warning {{Untrusted data is used to specify the buffer size}}
   bcopy(buf1, dst, ts); // expected-warning {{Untrusted data is used to specify the buffer size}}
   __builtin_memcpy(dst, buf1, (ts + 4)*sizeof(char)); // expected-warning {{Untrusted data is used to specify the buffer size}}
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 testSprintf_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
+  return 1 / x;                    // no-warning: 'sprintf' is not a taint source
 }
 
-int sprintf_propagates_taint(char *buf, char *msg) {
+int testSprintf_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}}
+  return 1 / x;                    // expected-warning {{Division by a tainted value, possibly zero}}
 }
 
+int scanf_s(const char *format, ...);
+int testScanf_s_(int *out) {
+  scanf_s("%d", out);
+  return 1 / *out; // expected-warning {{Division by a tainted value, possibly zero}}
+}
+
+#define _IO_FILE FILE
+int _IO_getc(_IO_FILE *__fp);
+int testUnderscoreIO_getc(_IO_FILE *fp) {
+  char c = _IO_getc(fp);
+  return 1 / c; // expected-warning {{Division by a tainted value, possibly zero}}
+}
+
+char *getcwd(char *buf, size_t size);
+int testGetcwd(char *buf, size_t size) {
+  char *c = getcwd(buf, size);
+  return system(c); // expected-warning {{Untrusted data is passed to a system call}}
+}
+
+char *getwd(char *buf);
+int testGetwd(char *buf) {
+  char *c = getwd(buf);
+  return system(c); // expected-warning {{Untrusted data is passed to a system call}}
+}
+
+typedef signed long long ssize_t;
+ssize_t readlink(const char *path, char *buf, size_t bufsiz);
+int testReadlink(char *path, char *buf, size_t bufsiz) {
+  ssize_t s = readlink(path, buf, bufsiz);
+  system(buf); // expected-warning {{Untrusted data is passed to a system call}}
+  // readlink never returns 0
+  return 1 / (s + 1); // expected-warning {{Division by a tainted value, possibly zero}}
+}
+
+ssize_t readlinkat(int dirfd, const char *pathname, char *buf, size_t bufsiz);
+int testReadlinkat(int dirfd, char *path, char *buf, size_t bufsiz) {
+  ssize_t s = readlinkat(dirfd, path, buf, bufsiz);
+  system(buf);        // expected-warning {{Untrusted data is passed to a system call}}
+  (void)(1 / dirfd);  // arg 0 is not tainted
+  system(path);       // arg 1 is not tainted
+  (void)(1 / bufsiz); // arg 3 is not tainted
+  // readlinkat never returns 0
+  return 1 / (s + 1); // expected-warning {{Division by a tainted value, possibly zero}}
+}
+
+char *get_current_dir_name(void);
+int testGet_current_dir_name() {
+  char *d = get_current_dir_name();
+  return system(d); // expected-warning {{Untrusted data is passed to a system call}}
+}
+
+int gethostname(char *name, size_t len);
+int testGethostname(char *name, size_t len) {
+  gethostname(name, len);
+  return system(name); // expected-warning {{Untrusted data is passed to a system call}}
+}
+
+struct sockaddr;
+typedef size_t socklen_t;
+int getnameinfo(const struct sockaddr *restrict addr, socklen_t addrlen,
+                char *restrict host, socklen_t hostlen,
+                char *restrict serv, socklen_t servlen, int flags);
+int testGetnameinfo(const struct sockaddr *restrict addr, socklen_t addrlen,
+                    char *restrict host, socklen_t hostlen,
+                    char *restrict serv, socklen_t servlen, int flags) {
+  getnameinfo(addr, addrlen, host, hostlen, serv, servlen, flags);
+
+  system(host);        // expected-warning {{Untrusted data is passed to a system call}}
+  return system(serv); // expected-warning {{Untrusted data is passed to a system call}}
+}
+
+int getseuserbyname(const char *linuxuser, char **selinuxuser, char **level);
+int testGetseuserbyname(const char *linuxuser, char **selinuxuser, char **level) {
+  getseuserbyname(linuxuser, selinuxuser, level);
+  system(selinuxuser[0]);  // expected-warning {{Untrusted data is passed to a system call}}
+  return system(level[0]); // expected-warning {{Untrusted data is passed to a system call}}
+}
+
+typedef int gid_t;
+int getgroups(int size, gid_t list[]);
+int testGetgroups(int size, gid_t list[], bool flag) {
+  int result = getgroups(size, list);
+  if (flag)
+    return 1 / list[0]; // expected-warning {{Division by a tainted value, possibly zero}}
+
+  return 1 / (result + 1); // expected-warning {{Division by a tainted value, possibly zero}}
+}
+
+char *getlogin(void);
+int testGetlogin() {
+  char *n = getlogin();
+  return system(n); // expected-warning {{Untrusted data is passed to a system call}}
+}
+
+int getlogin_r(char *buf, size_t bufsize);
+int testGetlogin_r(char *buf, size_t bufsize) {
+  getlogin_r(buf, bufsize);
+  return system(buf); // expected-warning {{Untrusted data is passed to a system call}}
+}
+
 // 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*);
 void mySink(int, int, int);
void testConfigurationSources3(void) {
   myScanf("%d %d", &x, &y);
   Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
 }
 
 void testConfigurationPropagation(void) {
   int x = mySource1();
   int y;
   myPropagator(x, &y);
   Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
 }
 
 void testConfigurationFilter(void) {
   int x = mySource1();
   if (isOutOfRange(&x)) // the filter function
     return;
   Buffer[x] = 1; // 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}}
 }
 
 void testUnknownFunction(void (*foo)(void)) {
   foo(); // no-crash
 }
 
 void testProctitleFalseNegative(void) {
   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}}
 }