On Wed, May 04, 2016 at 02:01:57PM +0200, Alexandre Ratchov wrote:
> On Sun, May 01, 2016 at 12:53:17PM +0300, Vadim Zhukov wrote:
> > 2016-04-30 7:38 GMT+03:00 Jonathan Gray <[email protected]>:
> > > On Wed, Apr 27, 2016 at 07:49:50PM -0700, Geoff Hill wrote:
> > >> Fix possible reads past the end of the buffer.
> > >>
> > >> Found by random fuzz testing (zzuf). Without the fix the fuzzer crashes
> > >> in several seconds; with the patch, the fuzzer runs clean for hours.
> > >
> > > Any reason to not replace the somewhat arbitary earlier test
> > > for this?
> > >
> > > Index: midiplay.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.bin/midiplay/midiplay.c,v
> > > retrieving revision 1.17
> > > diff -u -p -U9 -r1.17 midiplay.c
> > > --- midiplay.c 8 Feb 2015 23:40:34 -0000 1.17
> > > +++ midiplay.c 30 Apr 2016 04:35:31 -0000
> > > @@ -306,19 +306,19 @@ playdata(u_char *buf, u_int tot, char *n
> > > tracks = calloc(ntrks, sizeof(struct track));
> > > if (tracks == NULL)
> > > err(1, "malloc() tracks failed");
> > > for (t = 0; t < ntrks; ) {
> > > if (p >= end - MARK_LEN - SIZE_LEN) {
> > > warnx("Cannot find track %d", t);
> > > goto ret;
> > > }
> > > len = GET32(p + MARK_LEN);
> > > - if (len > 1000000) { /* a safe guard */
> > > + if (p + MARK_LEN + SIZE_LEN + len > end) {
> >
> > It's better to avoid "+" in checks to avoid overflow, no? Something like:
> >
> > if (len >= end - (p + MARK_LEN + SIZE_LEN)) {
> >
>
> imho moving the constants on lhs is more natural: we compare the
> actual length (lhs) to the max length (rhs). As comparison is
> unsigned (len is unsigned so pointer difference is promoted to
> unsigned), there's no need for addtional overflow checks.
>
> As we're at it, note that most of the calls of GET8, GET16, GET24,
> GET32 have no checks and similar buffer overflow can occur at many
> many many places. The whole approach is wrong.
>
> Anyway...
>
> OK?
Moving the vars around just means there could be an underflow instead
of an overflow. Some variant of this should be committed without
this thread dragging on even more though.
>
> Index: midiplay.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/midiplay/midiplay.c,v
> retrieving revision 1.17
> diff -u -p -u -p -r1.17 midiplay.c
> --- midiplay.c 8 Feb 2015 23:40:34 -0000 1.17
> +++ midiplay.c 4 May 2016 08:02:38 -0000
> @@ -312,7 +312,7 @@ playdata(u_char *buf, u_int tot, char *n
> goto ret;
> }
> len = GET32(p + MARK_LEN);
> - if (len > 1000000) { /* a safe guard */
> + if (len + MARK_LEN + SIZE_LEN > end - p) {
> warnx("Crazy track length");
> goto ret;
> }
>
