Re: midiplay: Fix out-of-bounds memory access
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 :
> > > 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 - 1.17
> > > +++ midiplay.c 30 Apr 2016 04:35:31 -
> > > @@ -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 > 100) { /* 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.c8 Feb 2015 23:40:34 - 1.17
> +++ midiplay.c4 May 2016 08:02:38 -
> @@ -312,7 +312,7 @@ playdata(u_char *buf, u_int tot, char *n
> goto ret;
> }
> len = GET32(p + MARK_LEN);
> - if (len > 100) { /* a safe guard */
> + if (len + MARK_LEN + SIZE_LEN > end - p) {
> warnx("Crazy track length");
> goto ret;
> }
>
Re: midiplay: Fix out-of-bounds memory access
On Sun, May 01, 2016 at 12:53:17PM +0300, Vadim Zhukov wrote:
> 2016-04-30 7:38 GMT+03:00 Jonathan Gray :
> > 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 - 1.17
> > +++ midiplay.c 30 Apr 2016 04:35:31 -
> > @@ -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 > 100) { /* 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 - 1.17
+++ midiplay.c 4 May 2016 08:02:38 -
@@ -312,7 +312,7 @@ playdata(u_char *buf, u_int tot, char *n
goto ret;
}
len = GET32(p + MARK_LEN);
- if (len > 100) { /* a safe guard */
+ if (len + MARK_LEN + SIZE_LEN > end - p) {
warnx("Crazy track length");
goto ret;
}
Re: midiplay: Fix out-of-bounds memory access
Vadim Zhukov wrote:
> 2016-04-30 7:38 GMT+03:00 Jonathan Gray :
> > 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 - 1.17
> > +++ midiplay.c 30 Apr 2016 04:35:31 -
> > @@ -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 > 100) { /* a safe guard */
> > + if (p + MARK_LEN + SIZE_LEN + len > end) {
>
> It's better to avoid "+" in checks to avoid overflow, no?
Yeah, I'm pretty sure the above overflow check is undefined.
Re: midiplay: Fix out-of-bounds memory access
2016-04-30 7:38 GMT+03:00 Jonathan Gray :
> 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 - 1.17
> +++ midiplay.c 30 Apr 2016 04:35:31 -
> @@ -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 > 100) { /* 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)) {
> warnx("Crazy track length");
> goto ret;
> }
> if (memcmp(p, MARK_TRACK, MARK_LEN) == 0) {
> tracks[t].start = p + MARK_LEN + SIZE_LEN;
> tracks[t].end = tracks[t].start + len;
> tracks[t].curtime = getvar(&tracks[t]);
> t++;
> }
>
--
WBR,
Vadim Zhukov
Re: midiplay: Fix out-of-bounds memory access
> Any reason to not replace the somewhat arbitary earlier test > for this? I chose to keep the condition simpler and the existing constraints intact for minimal impact, but I would agree it's better to consolidate with the existing check. Your diff looks good to me, +1.
Re: midiplay: Fix out-of-bounds memory access
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 - 1.17
+++ midiplay.c 30 Apr 2016 04:35:31 -
@@ -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 > 100) { /* a safe guard */
+ if (p + MARK_LEN + SIZE_LEN + len > end) {
warnx("Crazy track length");
goto ret;
}
if (memcmp(p, MARK_TRACK, MARK_LEN) == 0) {
tracks[t].start = p + MARK_LEN + SIZE_LEN;
tracks[t].end = tracks[t].start + len;
tracks[t].curtime = getvar(&tracks[t]);
t++;
}
midiplay: Fix out-of-bounds memory access
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.
Index: midiplay.c
===
RCS file: /cvs/src/usr.bin/midiplay/midiplay.c,v
retrieving revision 1.17
diff -u -p -u -r1.17 midiplay.c
--- midiplay.c 8 Feb 2015 23:40:34 - 1.17
+++ midiplay.c 27 Apr 2016 21:45:13 -
@@ -319,6 +319,10 @@ playdata(u_char *buf, u_int tot, char *n
if (memcmp(p, MARK_TRACK, MARK_LEN) == 0) {
tracks[t].start = p + MARK_LEN + SIZE_LEN;
tracks[t].end = tracks[t].start + len;
+ if (tracks[t].end > end) {
+ warnx("Track length exceeds remaining size");
+ goto ret;
+ }
tracks[t].curtime = getvar(&tracks[t]);
t++;
}
