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?
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;
}
