Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#10 Possible buffer overflows and security issues

release 7.6
closed-accepted
Sean Morrison
Analysis (3)
8
2007-10-14
2006-03-29
Pedro F. Giffuni
No

Hi;

Considering the great job finding bugs done by Coverity
on other open source projects I went looking for
similar tools and I found FlawFinder, a very easy to
use security checker.

I ran flawfinder on BRLCAD 7.6.6 and since the US army
should be expected to keep up the finest standards in
security, I thought you might be interested in the
report it produced.

Discussion

  • Flawfinder report (gzipped)

     
    Attachments
  • Sean Morrison
    Sean Morrison
    2006-03-30

    Logged In: YES
    user_id=785737

    That is excellent, Pedro.. thanks for running the tool and providing the
    report. It reads about as verbosely as the output after using our configure's
    --enable-warnings flag...

    That's a pretty hefty laundry list, but definitely one worth looking into and
    fixing. I've also had it planned to run valgrind on all the tools at some point
    as well. I'm sure it will uncover more than a few issues. Since the list you
    provided has a considerable number of high-risk warnings related to security,
    this will definitely be getting some attention. Thanks again.

     
  • Sean Morrison
    Sean Morrison
    2006-03-30

    • priority: 5 --> 8
    • assigned_to: nobody --> brlcad
     
  • Logged In: YES
    user_id=678384

    Actually that file is not as nice as the html report ;-).

    Many of these can be false positives, the tool is somewhat
    dumb in that it just looks for functions that are known to
    have security limitations. Valgrind is excellent for
    memory leaks and double free's but coverity can find yet
    more types of problems. Depending on developer's time we
    could ask brlcad added to scan.coverity.com.

    enjoy!

     
  • Sean Morrison
    Sean Morrison
    2006-06-21

    Logged In: YES
    user_id=785737

    Having BRL-CAD added to scan.coverity.com sounds like an excellent idea...
    I'm looking into it. Thanks for the link!

     
  • Sean Morrison
    Sean Morrison
    2007-10-12

    Logged In: YES
    user_id=785737
    Originator: NO

    FYI, as a follow up -- BRL-CAD was approved by and and added to the Coverity Scan project as of several months ago. We're already a "Rung 1" project and have a mailing list up for discussing the report issue, but we don't yet have a complete scan. I've been working with David Maxwell (of Coverity) to get our scan updated/fixed (which is why an announcement hasn't yet been made). More details to follow, but if folks are interested in working on resolving the scan issues, I'd be happy to set folks up with an account to our project (Coverity results are through a web-based per-project interface). Contact me via e-mail if interested.

     
  • Sean Morrison
    Sean Morrison
    2007-10-14

    Logged In: YES
    user_id=785737
    Originator: NO

    Flawfinder is now integrated into BRL-CAD's regression testing system so that issues are auto-detected and considered testing failures depending on the level of the issue detected. All of the level 5 issues are now taken care of as well as most of the existing level 4 issues it detected. The remaining few 4's should be taken care of shortly but it's a pretty good effort that should be worthy of closing out this suport request item. Thanks again for seeding the idea and getting things started!

    Cheers!
    Sean

     
  • Sean Morrison
    Sean Morrison
    2007-10-14

    • status: open --> closed-accepted
     
  • Logged In: YES
    user_id=678384
    Originator: YES

    Hi again;

    While I am absolutely delighted that things went so well with the flawfinder report and the new coverity scan, I think adding flawfinder to the regressions went too far: it does produce too many false positives (even at level 5). I think running it in once every 20 years is just about enough ;) ... besides you have coverity scans now.

     
  • Sean Morrison
    Sean Morrison
    2007-10-26

    Logged In: YES
    user_id=785737
    Originator: NO

    Fortunately, the regression tests can certainly be changed very easily if they actually do become problematic, but it's already done some good. It detected when another developer inadvertently re-added a few calls to functions that shouldn't have been used shortly after being enabled that wouldn't have readily been caught otherwise. Since we've taken care of all the issues at a given level, there's limited concern of false positives as the only issues that will come up will be new code added. Either way, as I said, the test can be turned off on a moment's notice if it ever does become a problem. Having it run merely makes detection easier and will allow issues to be reviewered/resolved more quickly.

     
  • Logged In: YES
    user_id=678384
    Originator: YES

    OK.. I'll be watching eagerly for the next version then ;).