Clang buffer overflow checks fail to detect simple case.

[Quuxplusone]

Bugzilla Link	PR11114
Status	NEW
Importance	P normal
Reported by	william.metcalf@gmail.com
Reported on	2011-10-11 17:10:01 -0700
Last modified on	2011-12-02 22:48:10 -0800
Version	trunk
Hardware	PC Linux
CC	bjs428@mail.missouri.edu, ganna@apple.com, kremenek@apple.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments	testcase_11114.c (775 bytes, text/plain)
Blocks
Blocked by
See also

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

[Quuxplusone]

Attached testcase_11114.c (775 bytes, text/plain): Analyzer fails to warn for this even more obvious string overflow

[Quuxplusone]

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. <string.h>.  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?

[Quuxplusone]

(In reply to comment #2)
>
> 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.

[Quuxplusone]

clone to rdar://problem/10488474

[Quuxplusone]

(In reply to comment #2)

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

[Quuxplusone]

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.