Bug 64775 (CVE-2004-0991)

Created attachment 27590 [details]
patch used by gentoo

* This comment was added by mail.
Date: Wed, 12 Jan 2005 15:29:55 +0100
From: Martin Schulze <joey@infodrom.org>
To: vendor-sec@lst.de
Subject: [vendor-sec] Re: what's the background of CAN-2004-0991 (mpg123)?

Ludwig Nussel wrote:
| Yesterday Gentoo published an advisory about a buffer overflow in
| mpg123 referring to CAN-2004-0991 and an old bugtraq posting which
| is actually CAN-2004-0805. I assumed Gentoo just missed a patch and
| forgot about it. Now heise.de has a headline "Critical hole in
| mpg123" citing Gentoo and Debian. The cited Debian changelog
| suggests that it is a new issue indeed. What's the background of
| this bug?

Below is the original mail with attached patch.

Yuri D'Elia wrote:
| Hi. I'm not sure if I should report here the problem or not, so excuse
| me in advance. I discovered a race condition in mpg123 (checked with
| debian's 0.59r-17 sources) which could cause layer2 decoding routines
| to read outside array's limits. There are essentially two problems.
| 
| In common.c:373 the 'oldhead' parameter is set before validating some
| conversion parameters. In case of problems (es bitrate_index == 0) some
| checks will be ignored (as in common.c:346) and some oldhead values will
| be used in the decoding routines. I've not investigated this further
| though.
| 
| Then, the sampling_frequency value in the same routine is not checked
| for consistency for layer 2 frames. In layer2.c:230 it's clearly limited
| to 3, whether by crafting an mpeg frame we could raise it up to 8. This
| will allow 'table' to set to random values, which is being used
| subsequently. It should be possible at least to cause a read outside
| process's limits, causing a seg-fault. The problem can also be triggered
| multiple times in a single stream.
| 
| I attached a patch which solves both problems. In the first case by
| moving the assignment only at the end of the function. In the second
| one, by verifying sampling_frequency in decode_header. I suspect that
| head_check would be a better place but, for efficiency, decode_header is
| better.
| 
| sample.mp3 exploits both problems in one run.
| Here's the results on a mips platform with the original mpg123:
| 
| 11% ./mpg123-0.59r-old/mpg123 sample.mp3 
| High Performance MPEG 1.0/2.0/2.5 Audio Player for Layer 1, 2 and 3.
| Version 0.59r (1999/Jun/15). Written and copyrights by Michael Hipp.
| Uses code from various people. See 'README' for more!
| THIS SOFTWARE COMES WITH ABSOLUTELY NO WARRANTY! USE AT YOUR OWN RISK!
| 
| Playing MPEG stream from sample.mp3 ...
| Free format not supported: (head fff4041b)
| MPEG 1.0 layer II, 112 kbit/s, 24000 Hz stereo
| zsh: segmentation fault (core dumped)  ./mpg123-0.59r-old/mpg123 sample.mp3
| 
| and then after the patch:
| 
| 12% ./mpg123-0.59r-new/mpg123 sample.mp3
| High Performance MPEG 1.0/2.0/2.5 Audio Player for Layer 1, 2 and 3.
| Version 0.59r (1999/Jun/15). Written and copyrights by Michael Hipp.
| Uses code from various people. See 'README' for more!
| THIS SOFTWARE COMES WITH ABSOLUTELY NO WARRANTY! USE AT YOUR OWN RISK!
| 
| Playing MPEG stream from sample.mp3 ...
| Free format not supported: (head fff4041b)
| MPEG 1.0 layer III, 96 kbit/s, 44100 Hz stereo
| 
| [0:00] Decoding of sample.mp3 finished.
| 
| Thanks
| 

| diff -ud -rud mpg123-0.59r-old/common.c mpg123-0.59r-new/common.c
| --- mpg123-0.59r-old/common.c	Mon Nov 01 19:42:50 CET 2004
| +++ mpg123-0.59r-new/common.c	Mon Nov 01 19:38:15 CET 2004
| @@ -370,8 +370,6 @@
|  
|      fr->stereo    = (fr->mode == MPG_MD_MONO) ? 1 : 2;
|  
| -    oldhead = newhead;
| -
|      if(!fr->bitrate_index) {
|        fprintf(stderr,"Free format not supported: (head %08lx)\n",newhead);
|        return (0);
| @@ -398,6 +396,10 @@
|            return (0);
|          }
|  #endif
| +	if( fr->sampling_frequency > 3) {
| +	  fprintf(stderr, "Bogus sampling frequency for layer-2\n");
| +	  return (0);
| +	}
|          fr->framesize = (long) tabsel_123[fr->lsf][1][fr->bitrate_index] * 144000;
|          fr->framesize /= freqs[fr->sampling_frequency];
|          fr->framesize += fr->padding - 4;
| @@ -422,6 +424,8 @@
|        fprintf(stderr,"Frame size too big: %d\n", fr->framesize+4-fr->padding);
|        return (0);
|      }
| +
| +    oldhead = newhead;
|      return 1;
|  }
|  

Regards,

	Joey

* This comment was added by mail.
Date: Wed, 12 Jan 2005 18:13:36 +0100
From: Daniel Kobras <kobras@debian.org>
To: vendor-sec@lst.de
Subject: [vendor-sec] Re: what's the background of CAN-2004-0991 (mpg123)?
Reply-To: Daniel Kobras <kobras@debian.org>, vendor-sec@lst.de

On Wed, Jan 12, 2005 at 02:51:33PM +0100, Martin Schulze forwarded:
| Ludwig Nussel wrote:
| > Yesterday Gentoo published an advisory about a buffer overflow in
| > mpg123 referring to CAN-2004-0991 and an old bugtraq posting which
| > is actually CAN-2004-0805. I assumed Gentoo just missed a patch and
| > forgot about it. Now heise.de has a headline "Critical hole in
| > mpg123" citing Gentoo and Debian. The cited Debian changelog
| > suggests that it is a new issue indeed. What's the background of
| > this bug?

CAN-2004-0991 is a new issue indeed. Going through a couple of
indirections, it might allow to modify the value of a boundary check in
layer2.c, so that one ends up in a situation that looks similar to the
one in CAN-2004-0805. Maybe that's why Gentoo included the reference in
the advisory, even though CAN-2004-0991 needs to be fixed separately.

As for the technical details, function common.c::decode_header() assumes
that the sampling frequency as declared in each frame header does not
change mid-stream. The internal representation sampling_frequency is
used as an index to certain arrays in layer2.c, and its value also
depends on further flags in the frame header (lsf and mpeg25). Unlike
the sampling frequency, changes in those flags are honoured by mpg123,
but the sampling_frequency variable is not updated accordingly. This
inconsistency can be easily used to crash the player with an
out-of-bounds read access. A more carefully crafted exploit might be
able to alter the value of fr->II_sblimit in layer2.c::do_layer2(),
which in turn would lead to a situation similar to the one described in
CAN-2004-0805. (I consider this bug to be extremely hard to exploit, and
I don't see how it could have become a 'critical hole' in the news.)

The fix I coded is twofold: First, I force an update of variable
sampling_frequency whenever one of the flags lsf or mpeg25 have changed.
Second, layer2.c now checks directly for sane values of the array index,
instead of indirectly checking one of the flags. The latter isn't
strictly required, but should make the code more robust. I've attached
our patch to version 0.59r.

Gentoo maintainer Jeremy Huddleston has checked version pre0.59s, and
from what he told me, sampling_frequency is always properly updated
there, so this version should not be vulnerable. Consequently, the
Gentoo patch only includes the robustness change in layer2.c (and I'm
surprised they released an advisory at all).

This issue was discovered and first analysed by Yuri D'Elia
<wavexx@yuv.info>.

Feel free to get in touch with me if you need additional information.
Also feel free to cite my portion of this mail in public.

Regards,

Daniel.

Created attachment 27617 [details]
patch from Daniel Kobras

We have pre0.59s. The patch from comment #1 should be OK. 

Packages with fix for 49776 and 49775 are submitted. 

swamp id: 209 
 
patchinfo files will be submitted in the next few minutes. 

updates released. 

CVE-2004-0991: CVSS v2 Base Score: 7.5 (AV:N/AC:L/Au:N/C:P/I:P/A:P)