Adding Flawfinder to Patchwatcher

[Kai Blin]

On Sunday 31 August 2008 15:03:38 Rob Shearman wrote:
> 2008/8/28 Austin English <austinenglish at gmail.com>:
> > I had a discussion with Dan about adding Flawfinder to the
> > patchwatcher. Currently, it's got some pretty generic errors, but it
> > seems able to test only patches, so we wouldn't be flooded with old
> > nonbugs (or we could set up a blacklist of safe errors). For
> > reference, I've run it on today's git. I'm attaching the full log, as
> > well as a condensed version of the most common errors (1 per error
> > type). Looks like a lot of chances for buffer overflows..
> >
> > Thoughts?
>
> Too many false positives to make it worth using. Just because you use
> strcpy, for example, it doesn't mean your program has a chance for a
> buffer overflow; it's using strcpy with a destination buffer that
> might not be large enough that causes buffer overflows.

Ack. Just checking for the part of Wine that I know best:

wine-git/dlls/secur32/dispatcher.c:104:  [4] (shell) execvp: This causes a new 
program to execute and is difficult to use safely. try using a library call 
that implements the same functionality if available.

Arguably correct, but no way to fix it. This is expected noise.

wine-git/dlls/secur32/dispatcher.c:119:  [4] (crypto) crypt: Function crypt is 
a poor one-way hashing algorithm; since it only accepts passwords of 8 
characters or less, and only a two-byte salt, it is excessively vulnerable to 
dictionary attacks given today's faster computing equipment. Use a different 
algorithm, such as SHA-1, with a larger non-repeating salt.

This one is just rubbish. For your convenience, line 119 of dispatcher.c looks 
like this:
        helper->crypt.ntlm.a4i = NULL;

Now, given that there's a struct in the helper struct called "crypt" and 
flawfinder triggers on that, there's a ton of repeated useless warnings, as 
flawfinder doesn't even notice this isn't a function call. What does that 
tool do? grep over the sources for a blacklist of strings?

wine-git/dlls/secur32/wrapper.c:568:  [4] (access) ImpersonateSecurityContext: 
If this call fails, the program could fail to drop heightened privileges. 
Make sure the return value is checked, and do not continue if a failure is 
reported.

Er, duh? It's nice to see the blacklist includes win32 function calls as well. 
Just a bit pointless for the implementation of this function. We'll probably 
see this for other "potentially dangerous" functions as well. This would make 
sense if and only if this warning only would trigger if the return value 
wasn't checked, not on a plain string match.

So while I agree that intelligent checking of patches is a nice thing to have, 
I'm not convinced flawfinder is intelligent enough.

Cheers,
Kai

-- 
Kai Blin
WorldForge developer  http://www.worldforge.org/
Wine developer        http://wiki.winehq.org/KaiBlin
Samba team member     http://www.samba.org/samba/team/
--
Will code for cotton.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
Url : http://www.winehq.org/pipermail/wine-devel/attachments/20080904/a9881ae5/attachment.pgp