[Groff] Re: Bug#107459: pic can be forced to run commands in safe mode

[Colin Watson]

[My earlier message was held for moderator approval and doesn't seem to
have been approved yet, so I'm quoting the whole thing while I post with
an address that's actually subscribed.]

On Thu, Aug 02, 2001 at 12:38:57PM +0100, Colin Watson wrote:
> On Thu, Aug 02, 2001 at 11:36:37AM +0200, Arnaud Giersch wrote:
> > Package: groff
> > Version: 1.15.2-1
> > 
> > pic can be forced to execute commands (sh X..X) when running in safe
> > mode (-S). It can be exploited trough lpd when groff/pic is run in
> > print filters, and arbitrary commands with id of lpd can be run.
> > 
> > pic command 'plot -1.99854281554743185012 "%n"' will overwrite memory
> > where safe mode variable is stored and then it alows to use "sh"
> > command.
> > 
> > How to reproduce:
> > 
> > pic -S > /dev/null << EOT
> > .PS
> > plot -1.99854281554743185012 "%n"
> > sh Xid >&2X
> > .PE
> > EOT
> > 
> > Actual Results: uid=1000(giersch) gid=300(parallel) ...
> > 
> > Expected Results: pic:<standard input>:3: unsafe to run command `id >&2'
> > 
> > Bug has been discovered by Zenith Parsec <address@hidden>.  Exploit
> > with patch has been posted to bugtraq:
> >         http://www.securityfocus.com/bid/3103
> > 
> > I've made an patched version for the Potato (groff_1.15.2-1.ag)
> > available at:
> >         http://arnaud.giersch.free.fr/debian/
> > 
> > As far as I can see in the sources, the other versions (Woody and Sid)
> > are vulnerable too.
> 
> Hi,
> 
> Are you aware of this problem? I haven't seen any traffic about it here.
> Although I haven't yet managed to overwrite the correct bit of memory to
> make the exploit work, I've got pic 1.17.2 to segfault by varying plot's
> first argument, which is a good indication that something's wrong. The
> relevant code in 1.15.2 and 1.17.2 seems largely identical.
> 
> At the very least, this should reliably segfault:
> 
>   $ pic -S >/dev/null
>   .PS
>   plot 0 "%n"

Here is the patch I used for the Debian groff package. It may not be the
best possible, but it seems to have done the job for now (at least I can
no longer get pic to segfault, and doc/pic.ms formats correctly as far
as I can tell).

        Fix format string vulnerability. Patch adapted from one by
        Zenith Parsec <address@hidden>.

        * src/preproc/pic/pic.y (format_number): Call do_sprintf()
        rather than using sprintf() directly.
        (do_sprintf): Use snprintf() rather than sprintf().

--- groff-1.17.2.orig/src/preproc/pic/pic.y
+++ groff-1.17.2/src/preproc/pic/pic.y
@@ -1769,8 +1769,7 @@
       return strsave(form);
     }
   }
-  sprintf(sprintf_buf, form, n);
-  return strsave(sprintf_buf);
+  return do_sprintf(form, &n, 1);
 }
 
 char *do_sprintf(const char *form, const double *v, int nv)
@@ -1792,18 +1791,19 @@
       if (*form == '%') {
        one_format += *form++;
        one_format += '\0';
-       sprintf(sprintf_buf, one_format.contents());
+       snprintf(sprintf_buf, sizeof(sprintf_buf), "%s", one_format.contents());
       }
       else {
        if (i >= nv) {
-         lex_error("too few arguments to sprintf");
+         lex_error("too few arguments to snprintf");
          result += one_format;
          result += form;
          break;
        }
        one_format += *form++;
        one_format += '\0';
-       sprintf(sprintf_buf, one_format.contents(), v[i++]);
+       snprintf(sprintf_buf, sizeof(sprintf_buf),
+                one_format.contents(), v[i++]);
       }
       one_format.clear();
       result += sprintf_buf;

Thanks,

-- 
Colin Watson                                  address@hidden