Comment 2 for bug 236051

Revision history for this message
Kees Cook (kees) wrote :

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