Suspected heap overflow in snd_midi_event_encode_byte()

Suspected heap overflow in snd_midi_event_encode_byte()

[Prashant Malani]

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?

Best regards,

Prashant

Re: 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);

Re: Suspected heap overflow in snd_midi_event_encode_byte()

[Prashant Malani]

Thanks Takashi,

I can confirm that this patch fixes the heap overflow issue.
Could you kindly submit this patch into the alsa-lib tree?

Best regards.

On Wed, Aug 22, 2018 at 12:52 AM, Takashi Iwai <tiwai@suse.de> wrote:

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

Re: Suspected heap overflow in snd_midi_event_encode_byte()

[Takashi Iwai]

On Wed, 22 Aug 2018 23:32:35 +0200,
Prashant Malani wrote:
> 
> Thanks Takashi,
> 
> I can confirm that this patch fixes the heap overflow issue.
> Could you kindly submit this patch into the alsa-lib tree?

Now applied the following patch to git tree.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] seq: Fix signedness in MIDI encoder/decoder

The qlen field of struct snd_midi_event was declared as size_t while
status_events[] assigns the qlen to -1 indicating to skip.  This leads
to the misinterpretation since size_t is unsigned, hence it passes the
check "dev.qlen > 0" incorrectly in snd_midi_event_encode_byte(),
which eventually results in a memory corruption.

Also, snd_midi_event_decode() doesn't consider about a negative qlen
value and tries to copy the size as is.

This patch fixes these issues: the first one is addressed by simply
replacing size_t with ssize_t in snd_midi_event struct.  For the
latter, a check "qlen <= 0" is added to bail out; this is also good as
a slight optimization.

Reported-by: Prashant Malani <pmalani@chromium.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 src/seq/seq_midi_event.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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);
-- 
2.18.0

Re: Suspected heap overflow in snd_midi_event_encode_byte()

[Prashant Malani]

Thanks.

Curious to know: Whats the URL for alsa-lib git tree? I'd love to be able
to check it out.

On Wed, Aug 22, 2018 at 11:46 PM, Takashi Iwai <tiwai@suse.de> wrote:

> On Wed, 22 Aug 2018 23:32:35 +0200,
> Prashant Malani wrote:
> >
> > Thanks Takashi,
> >
> > I can confirm that this patch fixes the heap overflow issue.
> > Could you kindly submit this patch into the alsa-lib tree?
>
> Now applied the following patch to git tree.
>
>
> thanks,
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] seq: Fix signedness in MIDI encoder/decoder
>
> The qlen field of struct snd_midi_event was declared as size_t while
> status_events[] assigns the qlen to -1 indicating to skip.  This leads
> to the misinterpretation since size_t is unsigned, hence it passes the
> check "dev.qlen > 0" incorrectly in snd_midi_event_encode_byte(),
> which eventually results in a memory corruption.
>
> Also, snd_midi_event_decode() doesn't consider about a negative qlen
> value and tries to copy the size as is.
>
> This patch fixes these issues: the first one is addressed by simply
> replacing size_t with ssize_t in snd_midi_event struct.  For the
> latter, a check "qlen <= 0" is added to bail out; this is also good as
> a slight optimization.
>
> Reported-by: Prashant Malani <pmalani@chromium.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  src/seq/seq_midi_event.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> 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);
> --
> 2.18.0
>
>

Re: Suspected heap overflow in snd_midi_event_encode_byte()

[Takashi Iwai]

On Thu, 23 Aug 2018 22:41:15 +0200,
Prashant Malani wrote:
> 
> Thanks.
> 
> Curious to know: Whats the URL for alsa-lib git tree? I'd love to be able
> to check it out.

You can get it from git.alsa-project.org


Takashi

> 
> On Wed, Aug 22, 2018 at 11:46 PM, Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Wed, 22 Aug 2018 23:32:35 +0200,
> > Prashant Malani wrote:
> > >
> > > Thanks Takashi,
> > >
> > > I can confirm that this patch fixes the heap overflow issue.
> > > Could you kindly submit this patch into the alsa-lib tree?
> >
> > Now applied the following patch to git tree.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > -- 8< --
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] seq: Fix signedness in MIDI encoder/decoder
> >
> > The qlen field of struct snd_midi_event was declared as size_t while
> > status_events[] assigns the qlen to -1 indicating to skip.  This leads
> > to the misinterpretation since size_t is unsigned, hence it passes the
> > check "dev.qlen > 0" incorrectly in snd_midi_event_encode_byte(),
> > which eventually results in a memory corruption.
> >
> > Also, snd_midi_event_decode() doesn't consider about a negative qlen
> > value and tries to copy the size as is.
> >
> > This patch fixes these issues: the first one is addressed by simply
> > replacing size_t with ssize_t in snd_midi_event struct.  For the
> > latter, a check "qlen <= 0" is added to bail out; this is also good as
> > a slight optimization.
> >
> > Reported-by: Prashant Malani <pmalani@chromium.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  src/seq/seq_midi_event.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > 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);
> > --
> > 2.18.0
> >
> >
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: Suspected heap overflow in snd_midi_event_encode_byte()

[Mike Frysinger]

the preferred flow looks something like:
- we put together the fix
- send fix to upstream (alsa) project
- upstream reviews/merges
- if we get a new release, update to that
- if new release is going to be a while, backport the patch to latest
ebuild and send a PR to Gentoo's GH (github.com/gentoo/gentoo)
- once Gentoo merges the PR, roll the fix into portage-stable

otherwise if things are up for debate along the way, we have
chromiumos-overlay to hold our own forked packages.
-mike

On Mon, Aug 27, 2018 at 8:33 AM Takashi Iwai <tiwai@suse.de> wrote:

> On Thu, 23 Aug 2018 22:41:15 +0200,
> Prashant Malani wrote:
> >
> > Thanks.
> >
> > Curious to know: Whats the URL for alsa-lib git tree? I'd love to be able
> > to check it out.
>
> You can get it from git.alsa-project.org
>
>
> Takashi
>
> >
> > On Wed, Aug 22, 2018 at 11:46 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >
> > > On Wed, 22 Aug 2018 23:32:35 +0200,
> > > Prashant Malani wrote:
> > > >
> > > > Thanks Takashi,
> > > >
> > > > I can confirm that this patch fixes the heap overflow issue.
> > > > Could you kindly submit this patch into the alsa-lib tree?
> > >
> > > Now applied the following patch to git tree.
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> > >
> > > -- 8< --
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Subject: [PATCH] seq: Fix signedness in MIDI encoder/decoder
> > >
> > > The qlen field of struct snd_midi_event was declared as size_t while
> > > status_events[] assigns the qlen to -1 indicating to skip.  This leads
> > > to the misinterpretation since size_t is unsigned, hence it passes the
> > > check "dev.qlen > 0" incorrectly in snd_midi_event_encode_byte(),
> > > which eventually results in a memory corruption.
> > >
> > > Also, snd_midi_event_decode() doesn't consider about a negative qlen
> > > value and tries to copy the size as is.
> > >
> > > This patch fixes these issues: the first one is addressed by simply
> > > replacing size_t with ssize_t in snd_midi_event struct.  For the
> > > latter, a check "qlen <= 0" is added to bail out; this is also good as
> > > a slight optimization.
> > >
> > > Reported-by: Prashant Malani <pmalani@chromium.org>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >  src/seq/seq_midi_event.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > 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);
> > > --
> > > 2.18.0
> > >
> > >
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >
>

Re: Suspected heap overflow in snd_midi_event_encode_byte()

[Prashant Malani]

Got it. Thanks!

On Mon, Aug 27, 2018 at 5:33 AM, Takashi Iwai <tiwai@suse.de> wrote:

> On Thu, 23 Aug 2018 22:41:15 +0200,
> Prashant Malani wrote:
> >
> > Thanks.
> >
> > Curious to know: Whats the URL for alsa-lib git tree? I'd love to be able
> > to check it out.
>
> You can get it from git.alsa-project.org
>
>
> Takashi
>
> >
> > On Wed, Aug 22, 2018 at 11:46 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >
> > > On Wed, 22 Aug 2018 23:32:35 +0200,
> > > Prashant Malani wrote:
> > > >
> > > > Thanks Takashi,
> > > >
> > > > I can confirm that this patch fixes the heap overflow issue.
> > > > Could you kindly submit this patch into the alsa-lib tree?
> > >
> > > Now applied the following patch to git tree.
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> > >
> > > -- 8< --
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Subject: [PATCH] seq: Fix signedness in MIDI encoder/decoder
> > >
> > > The qlen field of struct snd_midi_event was declared as size_t while
> > > status_events[] assigns the qlen to -1 indicating to skip.  This leads
> > > to the misinterpretation since size_t is unsigned, hence it passes the
> > > check "dev.qlen > 0" incorrectly in snd_midi_event_encode_byte(),
> > > which eventually results in a memory corruption.
> > >
> > > Also, snd_midi_event_decode() doesn't consider about a negative qlen
> > > value and tries to copy the size as is.
> > >
> > > This patch fixes these issues: the first one is addressed by simply
> > > replacing size_t with ssize_t in snd_midi_event struct.  For the
> > > latter, a check "qlen <= 0" is added to bail out; this is also good as
> > > a slight optimization.
> > >
> > > Reported-by: Prashant Malani <pmalani@chromium.org>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >  src/seq/seq_midi_event.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > 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);
> > > --
> > > 2.18.0
> > >
> > >
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >
>