On Sunday 31 August 2008 15:03:38 Rob Shearman wrote:
2008/8/28 Austin English austinenglish@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