[analyzer] vfork checker: allow execve after vfork

[janvcelak]

Summary

execve is missing in the list of functions that are allowed after vfork(). As a result, clang analyzer reports the following false positive:

#include <unistd.h>

int main(int argc, char *argv[])
{
	char *a[] = {"true", NULL};
	char *e[] = {NULL};
	if (vfork() == 0) {
		execve("/bin/true", a, e);
		_exit(1);
	}
	return 0;
}

$ scan-build clang -Wall -c repro.c      
scan-build: Using '/usr/bin/clang-9' for static analysis
repro.c:7:6: warning: Call to function 'vfork' is insecure as it can lead to denial of service situations in the parent process. Replace calls to vfork with calls to the safer 'posix_spawn' function
        if (vfork() == 0) {
            ^~~~~
repro.c:8:3: warning: This function call is prohibited after a successful vfork
                execve("/bin/true", a, e);
                ^~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
scan-build: 2 bugs found.
scan-build: Run 'scan-view /tmp/scan-build-2020-01-29-162705-3770808-1' to examine bug reports.

The list of exec functions in the code is take from the exec(3) man page which are just a fronted for execve(2). Quoting the manual page:

The exec() family of functions replaces the current process image with a new process image. The functions escribed in this manual page are front-ends for execve(2). (See the manual page for execve(2) for further details about the replacement of the current process image.)

[Charusso]

Hey, thanks! The patch looks great, but please note that we do the reviews with context using git diff -U999999 or uploading with arc (https://secure.phabricator.com/book/phabricator/article/arcanist/).

[NoQ]

Yup, thanks, nice catch!

We should add a test for this (cf. test/Analysis/vfork.c).

Also do you have any immediate opinions on https://bugs.llvm.org/show_bug.cgi?id=43871? 'Cause i'm super confused. Like, it's trivial to fix but i'm not sure what the correct behavior is.

[janvcelak]

Sorry. I missed that step in contribution guidelines. I'm attaching updated version with larger context.

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp

110

Well, this is not the case now, but I wonder if it would also make sense to sort this list alphabetically.

[janvcelak]

In D73629#1847750, @NoQ wrote:

We should add a test for this (cf. test/Analysis/vfork.c).

There is already a test to check if the whitelist works by making sure that call to execl doesn't generate the warning. I think there is no point adding execve into the tests unless we want to add a test for each of the functions in the list. However, let me know if you want it added.

[janvcelak]

clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp

110

I had this idea as well. The list is currently not sorted but seems to match the order of the functions as they are listed in the exec(3) manual page so I kept it.

Let me know what is preferred and I can update the diff.

[janvcelak]

Please, can the reviewers respond to my comments? I would like to know if this needs additional revision or if it can be accepted as is.

[NoQ]

Let's still add the test. It'll make sure that you understand exactly what you're doing and that there aren't more aspects to this problem. More tests never hurt and it's a local culture to have at least some tests for every commit that changes the behavior. Our tests are also very cheap to add.

[Szelethus]

LGTM granted the test is supplied, nice catch!

[janvcelak]

I'm attaching updated version with tests.

[NoQ]

Thank you!! I'll commit.

clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp

 }
 
 // Returns true iff ok to call function after successful vfork.
 bool VforkChecker::isCallWhitelisted(const IdentifierInfo *II,
                                  CheckerContext &C) const {
   if (VforkWhitelist.empty()) {
     // According to manpage.
     const char *ids[] = {
-      "_exit",
       "_Exit",
+      "_exit",
       "execl",
-      "execlp",
       "execle",
+      "execlp",
       "execv",
+      "execve",
       "execvp",
       "execvpe",
       nullptr
     };
 
     ASTContext &AC = C.getASTContext();
     for (const char **id = ids; *id; ++id)
       VforkWhitelist.insert(&AC.Idents.get(*id));
   }
 
   return VforkWhitelist.count(II);

clang/test/Analysis/Inputs/system-header-simulator.h

typedef struct __SomeStruct {
   char * p;
 } SomeStruct;
 void fakeSystemHeaderCall(SomeStruct *);
 
 typedef int pid_t;
 pid_t fork(void);
 pid_t vfork(void);
 int execl(const char *path, const char *arg, ...);
+int execle(const char *path, const char *arg, ...);
+int execlp(const char *file, const char *arg, ...);
+int execv(const char *path, char *const argv[]);
+int execve(const char *path, char *const argv[], char *const envp[]);
+int execvp(const char *file, char *const argv[]);
+int execvpe(const char *file, char *const argv[], char *const envp[]);
 
 void exit(int status) __attribute__ ((__noreturn__));
 void _exit(int status) __attribute__ ((__noreturn__));
 void _Exit(int status) __attribute__ ((__noreturn__));
 
 #define UINT32_MAX        4294967295U
 #define INT64_MIN        (-INT64_MAX-1)
 #define __DBL_MAX__ 1.7976931348623157e+308
 #define DBL_MAX __DBL_MAX__
 #ifndef NULL
 #define __DARWIN_NULL 0
 #define NULL __DARWIN_NULL
 #endif
 
 #define offsetof(t, d) __builtin_offsetof(t, d)
 No newline at end of file

clang/test/Analysis/vfork.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,security.insecureAPI.vfork,unix.Vfork -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,security.insecureAPI.vfork,unix.Vfork -verify -x c++ %s
 
 #include "Inputs/system-header-simulator.h"
 
 void foo();
 
 // Ensure that child process is properly checked.
-int f1(int x) {
+int f1(int x, int y) {
   pid_t pid = vfork(); // expected-warning{{Call to function 'vfork' is insecure}}
   if (pid != 0)
     return 0;
 
   switch (x) {
   case 0:
     // Ensure that modifying pid is ok.
     pid = 1; // no-warning
     // Ensure that calling whitelisted routines is ok.
+    switch (y) {
+    case 0:
       execl("", "", 0); // no-warning
+      break;
+    case 1:
+      execle("", "", 0); // no-warning
+      break;
+    case 2:
+      execlp("", "", 0); // no-warning
+      break;
+    case 3:
+      execv("", NULL); // no-warning
+      break;
+    case 4:
+      execve("", NULL, NULL); // no-warning
+      break;
+    case 5:
+      execvp("", NULL); // no-warning
+      break;
+    case 6:
+      execvpe("", NULL, NULL); // no-warning
+      break;
+    }
     _exit(1); // no-warning
     break;
   case 1:
     // Ensure that writing variables is prohibited.
     x = 0; // expected-warning{{This assignment is prohibited after a successful vfork}}
     break;
   case 2:
     // Ensure that calling functions is prohibited.