Re: [patch] sed: missing bound check resulting stack overflow

[Jonathan Gray]

On Wed, Dec 10, 2014 at 11:46:57AM +0100, Sébastien Marie wrote:
> On Wed, Dec 10, 2014 at 11:16:21AM +0100, Sébastien Marie wrote:
> > Hi,
> > 
> > In compile_flags, the variable holding the filename ('w' flag of 's'
> > command) is an array with PATH_MAX length.
> > 
> > We should check the size of wanted filename, before copying it in wfile.
> > 
> > $ echo | sed -e s/a//w`perl -e "print '_' x 10000"` 
> > Bus error (core dumped)
> > 
> > Found also with afl-fuzz.
> > 
> 
> Here a new patch that check the file len while copying.

Something like this came up with the recent deroff change.
Perhaps make the comparison with two pointers like the
fix millert@ committed for that?

Index: compile.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/compile.c,v
retrieving revision 1.36
diff -u -p -r1.36 compile.c
--- compile.c   8 Oct 2014 04:19:08 -0000       1.36
+++ compile.c   10 Dec 2014 11:18:14 -0000
@@ -538,7 +538,7 @@ compile_flags(char *p, struct s_subst *s
 {
        int gn;                 /* True if we have seen g or n */
        long l;
-       char wfile[PATH_MAX], *q;
+       char wfile[PATH_MAX], *q, *eq;
 
        s->n = 1;                               /* Default */
        s->p = 0;
@@ -584,9 +584,12 @@ compile_flags(char *p, struct s_subst *s
 #endif
                        EATSPACE();
                        q = wfile;
+                       eq = wfile + sizeof(wfile) - 1;
                        while (*p) {
                                if (*p == '\n')
                                        break;
+                               if (q + 1 >= eq)
+                                       err(COMPILE, "wfile too long");
                                *q++ = *p++;
                        }
                        *q = '\0';