llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.02k stars 11.96k forks source link

Clang buffer overflow checks fail to detect simple case. #11486

Open llvmbot opened 13 years ago

llvmbot commented 13 years ago
Bugzilla Link 11114
Version trunk
OS Linux
Reporter LLVM Bugzilla Contributor
CC @AnnaZaks,@tkremenek

Extended Description

Tested with version of clang/scan-build in trunk. The static analyzer fails to detect a simple buffer overflow in program found here. I guess more of an FYI than anything else..

http://www.debian-administration.org/articles/408

clang -v clang version 3.0 (trunk 141707) Target: x86_64-unknown-linux-gnu Thread model: posix

scan-build gcc -o buggy buggy.c scan-build: 'clang' executable not found in '/opt/clang/scan-build/bin'. scan-build: Using 'clang' from path: /opt/clang/bin/clang scan-build: Removing directory '/tmp/scan-build-2011-10-11-1' because it contains no reports.

clang --analyze -Xclang -analyzer-checker -Xclang security.experimental buggy.c clang --analyze -Xclang -analyzer-checker -Xclang security.experimental.ArrayBound buggy.c clang --analyze -Xclang -analyzer-checker -Xclang security.experimental.ArrayBound2 buggy.c

llvmbot commented 12 years ago

I noticed that somewhere between the version I originally tested and the latest (r145757), changes have been made to 'ArrayBoundV2' that now correctly throw a warning for the previously attached example.

However, the analyzer again fails to throw a warning if the attachment code is modified so that the declaration:

char foo[1];

is replaced with:

char foo = (char) malloc(sizeof(char));

which, for these purposes, should exhibit the same behavior.

tkremenek commented 12 years ago

How mature are the security checks at this stage? I know that they're still marked 'experimental', and so it might be that the necessary functionality simply hasn't been written into them yet.

Generally speaking, 'experimental' checkers have enough functionality in place for those actively working on improving the analyzer to use them. They may be raft with false positives, or may have many missing features.

tkremenek commented 12 years ago

clone to rdar://problem/10488474

tkremenek commented 12 years ago

How mature are the security checks at this stage? I know that they're still marked 'experimental', and so it might be that the necessary functionality simply hasn't been written into them yet. Is this a case of features that haven't been fully implemented, or is this a bug in mostly finished code?

They aren't mature at all, and are far from complete. They are, however, being actively worked on, so bug reports like these, with extensive analyses, are extremely useful.

llvmbot commented 12 years ago

I successfully reproduced this behavior myself on a recent version of the trunk (r145146). Since the example code you linked to is very blatantly vulnerable to a buffer overflow, I too would expect the analyzer to throw up a warning. Sure enough, it fails to do so for any of the invocations you gave.

Just to be safe, I double-checked the list of registered checkers with:

clang -cc1 -analyzer-checker-help

to see if there might be some other relevant checker that ought to be applied, but none stand out.

One guess is that, although this is an issue with array bounds, the checker fails this case because there's nothing in the buggy program to indicate what the bound on argv[1] might be. However, this doesn't appear to be the case either, as it seems that the static analyzer fails to catch other bad uses of strcpy() where information on the all the string lengths is explicit in the source text. It would be tempting to blame strcpy() itself, and to hypothesize that the checks fail because the overflowing operations occur in an externally linked chunk of code that is not being checked, i.e. . However, the checks also fail for a naive strpcy() implemented by hand. Attached is a very small test case showing a very blatant string overflow that is not caught by the analyzer.

It appears that the analyzer simply fails to catch the bad string copy, even when the indices are made explicit and nothing is hidden behind function parameters.

How mature are the security checks at this stage? I know that they're still marked 'experimental', and so it might be that the necessary functionality simply hasn't been written into them yet. Is this a case of features that haven't been fully implemented, or is this a bug in mostly finished code?

llvmbot commented 12 years ago

Analyzer fails to warn for this even more obvious string overflow

llvmbot commented 13 years ago

assigned to @tkremenek