Checker should warn against any use of vfork()

[llvmbot]

Bugzilla Link	11053
Resolution	FIXED
Resolved on	Oct 10, 2011 23:36
Version	trunk
OS	All
Attachments	Patch adds use of vfork() as a security issue.
Reporter	LLVM Bugzilla Contributor
CC	@AnnaZaks,@tkremenek

Extended Description

According to SEI CERT guideline POS33-C[*], vfork(2) should not be used due to potential denial of service issues and undefined behaviour across different implementations. The attached patch adds a check to experimental.security.SecuritySyntactic to detect and report an issue on use of vfork().

[*] https://www.securecoding.cert.org/confluence/display/seccode/POS33-C.+Do+not+use+vfork%28%29

[llvmbot]

assigned to @AnnaZaks

[AnnaZaks]

Hi Graham,

Could you add a test case to clang/test/Analysis/security-syntax-checks.m as well?

Thanks!
Anna.

[llvmbot]

Patch warns on use of vfork() and includes test case
New version of the patch including a test case. I have tried to follow the style used throughout the test files, and it Works On My Machine™.

[tkremenek]

If all uses of vfork() are considered a security risk, should we not just make this an opt-in compiler warning?

[AnnaZaks]

Hi Graham,

As per our conversation with Ted, we can proceed with the patch to the analyzer. If/When we migrate these into the compiler warnings we'll work out a mechanism to make sure both warnings (one coming from the compiler and another from the analyzer) are not displayed at the same time.

However, I am not convinced that we should propose fork() as a substitute for vfork(). It seems that fork() is an acceptable solution on systems with memory overcommit (Linux), while posix_spawn() is the solution to use on systems without overcommit to avoid out of memory errors when forking a process. Overall, posix_spawn() seems to be a better generic alternative.

What do you think?

[llvmbot]

Hi Anna,

sorry to take a while to get back. posix_spawn() is a great replacement for the vfork()/execve() dance, I chose to suggest fork() because it's API compatible with vfork() even if the behaviour is different - said difference in behaviour shouldn't matter in many cases. Overall posix_spawn() is probably a closer match to what many people try to do with any *fork() action, so it does seem a good recommendation.

Thanks,
Graham.

[AnnaZaks]

Hi Graham,

How about I change the warning to say the following and commit?
"Replace calls to vfork with calls to the safer 'posix_spawn' function."
(I also removed the POS33-C reference since, unfortunately, it's inconsistent with our recommendation.)

Also, you can just send your patches directly to clang mailing lists. That way they will get more exposure/comments.
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

Thanks for the patch!
Anna.

[AnnaZaks]

Applied in r141643!