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


** Changed in: openbabel (Ubuntu)
     Assignee: Ubuntu Security Team (ubuntu-security) => Jonathan Riddell (jr)

-- 
main inclusion review for openbabel
https://bugs.launchpad.net/bugs/236051
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Reply via email to