Adding Flawfinder to Patchwatcher
kai.blin at gmail.com
Thu Sep 4 03:22:43 CDT 2008
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:  (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:  (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
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:  (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
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.
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
Size: 189 bytes
Desc: This is a digitally signed message part.
Url : http://www.winehq.org/pipermail/wine-devel/attachments/20080904/a9881ae5/attachment.pgp
More information about the wine-devel