[cfe-commits] r128679 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp test/Analysis/security-synta

[Lenny Maiorani]

On Apr 1, 2011, at 3:16 AM, pageexec at freemail.hu wrote:

> On 31 Mar 2011 at 22:09, Lenny Maiorani wrote:
> 
>> //===----------------------------------------------------------------------===//
>> +// Check: Any use of 'strcpy' is insecure.
>> +//
>> +// CWE-119: Improper Restriction of Operations within 
>> +// the Bounds of a Memory Buffer 
>> +//===----------------------------------------------------------------------===//
> 
> i don't think this strategy is correct.
> 
> first, there are many valid uses of strcpy and similar functions without an explicit
> bounds check so the 'any use' is wrong. as a consequence, this code, as it is, is not
> much different from a 'grep strcpy -rn .', i.e., not very useful for clang.
> 
> second, as the CWE notes it itself, replacing strcpy with bounds checking versions is
> not without its problems, and i don't see you checking those issues, i.e., this code
> is quite incomplete.
> 
> IMHO, a useful implementation of this kind of checking would be what the CWE description
> suggests as well: data flow analysis so that valid uses of strcpy et al. are not reported
> as false positives.
> 
> cheers,
> 
> PaX Team
> 

Pax Team,

In the case of strcpy() and strcat() (probably sprintf() too)... This check is not meant to be the be all, end all. This is part of the Security checker, specifically looking for security items which could be improved upon. The actual validation of the inputs to this function are checked in the CString checker. I am assuming that if the user has enabled the Security checker then the user is specifically desiring this kind of output.

On that note, adding it for strcmp() and strcasecmp() is not correct as pointed out by Joerg because it can be perfectly safe provided the inputs are NULL terminated. The CWE does state that simply replacing strcpy() with a bounds checking version is not a perfect solution. That replacement still needs to be used correctly. 

Effectively, this is more of an auditor than a path sensitive checker. This sticks with the intended behavior (my interpretation of it) of the rest of the Security checker.

-Lenny