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)) {
> 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
