[alsa-devel] Suspected heap overflow in snd_midi_event_encode_byte()

[Takashi Iwai]

On Tue, 21 Aug 2018 23:24:14 +0200,
Prashant Malani wrote:
> 
> Hi,
> 
> The Chromium fuzzers detected a potential heap overflow in
> snd_midi_event_encode_byte() when attempting to encode an invalid data
> sequence. The potential bug was observed in alsa-lib-1.1.5 (the source
> seems similar to alsa-lib-1.1.6 so it is likely present there too).
> 
> Code to reproduce (condensed to fit in an email):
> 
> std::array<int, 4> arr{ {0x0A, 0x0B, 0x0C, 0x0D} };
> snd_midi_event_t* encoder;
> snd_midi_event_new(arr.size(), &encoder);
> for (int i = 0; i < arr.size(); i++) {
>   snd_seq_event_t event;
>   int result = snd_midi_event_encode_byte(encoder, arr[i], &event);
>   if (result < 0) {
>     // Log error and return....
>   }
>   if ( result == 1) {
>     // Send the completed message and return.
>   }
> }
> ....
> 
> The first call to snd_midi_event_encode_byte() using byte 0x0A causes the
> |encoder->qlen| to underflow and become a large unsigned value, and
> |encoder->read| to become 2.  Subsequent bytes processed will get written
> to the |encoder->buf| buffer, with |dev->read| getting incremented after
> every byte. As a result, by the time we get to byte 0x0D, |encoder->read|
> is already 4, and this results in a heap overflow (relevant line in
> alsa-lib is src/seq/seq_midi_event.c:425)
> 
> I'm not sure if the above input array is a valid input to
> snd_midi_event_encode_byte(), but I'm guessing we should be doing some
> error checking to make sure we're not processing
> incorrect/unexpected/invalid bytes.
> 
> Any suggestions about how one can submit a fix for this?

Does the patch below help?  The first hunk should suffice for your
case.  The latter is a similar issue for decoding.


thanks,

Takashi

-- 8< --
diff --git a/src/seq/seq_midi_event.c b/src/seq/seq_midi_event.c
index 2e7d1035442a..5a12a18ce781 100644
--- a/src/seq/seq_midi_event.c
+++ b/src/seq/seq_midi_event.c
@@ -35,7 +35,7 @@
 
 /* midi status */
 struct snd_midi_event {
-	size_t qlen;	/* queue length */
+	ssize_t qlen;	/* queue length */
 	size_t read;	/* chars read */
 	int type;	/* current event type */
 	unsigned char lastcmd;
@@ -606,6 +606,8 @@ long snd_midi_event_decode(snd_midi_event_t *dev, unsigned char *buf, long count
 				status_event[type].decode(ev, xbuf + 0);
 			qlen = status_event[type].qlen;
 		}
+		if (qlen <= 0)
+			return 0;
 		if (count < qlen)
 			return -ENOMEM;
 		memcpy(buf, xbuf, qlen);