main inclusion review for openbabel

Package looks ok in general, but it is massively reading/parsing a lot of different file formats. Jamie, Kees, can you please give this an inspection for general code quality and common vulnerabilities? Thanks!

I see a few cases of being able to run off the stack during sprintf. I'd prefer all the sprintfs were checked and replaced with snprintf, but it looks to be a large task:

  $ grep -R sprintf . | wc -l
  472

As long as this compiles without warnings from -Wformat-security and compiles with the now-default -D_FORTIFY_SOURCE=2 at -O2, we should get the libc protections for most abuse cases of *printf. (i.e. it will need a no-change rebuild bump to get recompiled with the hardened toolchain). (Also -- it does compile fine, I just ran a rebuild.)

scanf seems to be accidentally safe -- old BUFF_SIZE was 1024, which was raised. Some of the sscanf's where %1024s, which would have been a problem. The rest are okay.

One area that bothers me is the unbounded mallocs using multiplication and user-controlled input. For example in src/formats/xtcformat.cpp:
      xdr_int(&xd, &natoms);
...
        floatCoord = (float *)malloc(natoms * 3 * sizeof(float));
...
      // Read the positions
      if (xdr3dfcoord(&xd, (float *)floatCoord, &natoms, &prec) == 0) {

There really needs to be bounds checking for this stuff to keep allocations from wrapping. Prior to that happening I would need to recommend against it going into main.

  $ grep -R 'alloc(.*\*' . | wc -l
  157

I'm one of the upstream maintainers. We just released 2.2.0-final, which addresses all these and some other internally-discovered issues, including some minor denial-of-service issues with malformed data. (For example, the code could consume large amounts of memory.)

We do compile with -Wformat-security and other warnings enabled. Also, I'd like to correct the inclusion report. There is a test suite. Run "make check" -- ideally with Perl's "prove" binary installed.

While we don't consider this a particularly security-sensitive program, we do take it seriously because it serves as a base library for other programs. Any security issue found would be fixed ASAP.

Geoff, thanks for the follow-up! Seems that we should get 2.2.0-final into Intrepid (we have beta5 at the moment). Jonathan, any chance you could test the 2.2.0-1 from Debian experimental? If it works, we should sync it over.

I do consider it security sensitive in a way that such file formats are often taken from unknown third-party sources. Thus this provides the classical vector of putting a crafted .cml (or other format) somewhere and luring people to open it, which would then run arbitrary code on their systems. Of course our current measures of SSP, ASLR, and FORTIFY_SOURCE make this hard to do, but a general source code review still can't hurt.

Thanks, Martin

2.2 synced, re-opening so this can be reconsidered.

back to Jamie, Kees for review.

Nuff blocking. Moved to main, set as beta milestone.

Looks good to me, thanks for getting everything addressed.