Page MenuHomePhabricator

[analyzer] vfork checker: allow execve after vfork
ClosedPublic

Authored by janvcelak on Jan 29 2020, 7:30 AM.

Details

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.)

Diff Detail

Event Timeline

janvcelak created this revision.Jan 29 2020, 7:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2020, 7:30 AM

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 added a comment.Jan 29 2020, 11:43 AM

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.

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

xazax.hun added inline comments.Jan 29 2020, 11:56 AM
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.

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 added inline comments.Jan 30 2020, 3:01 AM
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.

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 added a comment.Feb 11 2020, 12:36 PM

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.

LGTM granted the test is supplied, nice catch!

I'm attaching updated version with tests.

janvcelak marked 2 inline comments as done.Feb 17 2020, 12:17 PM
NoQ accepted this revision.Feb 17 2020, 9:39 PM

Thank you!! I'll commit.

This revision is now accepted and ready to land.Feb 17 2020, 9:39 PM
This revision was automatically updated to reflect the committed changes.