X-Debbugs-CC: mh...@suse.de, n...@debian.org
Package: xine-lib
Version: 1.1.14-3
Severity: serious
Tag: security
Hi,
so CVE-2008-5236 is
Multiple heap-based buffer overflows in xine-lib 1.1.12, and other
1.1.15 and earlier versions, allow remote attackers to execute
arbitrary code via vectors related to (1) a crafted EBML element
length processed by the parse_block_group function in
demux_matroska.c; (2) a certain combination of sps, w, and h values
processed by the real_parse_audio_specific_data and
demux_real_send_chunk functions in demux_real.c; and (3) an
unspecified combination of three values processed by the open_ra_file
function in demux_realaudio.c. NOTE: vector 2 reportedly exists
because of an incomplete fix in 1.1.15.
Attached are a patch by Matthias Hopf of SuSE (please credit him in the
ChangeLog) and a "backport" to the Debian package including the required
1.1.14->1.1.15 chagnes as well. I should recommend to use xine-lib
1.1.15 alltogether (at least for the three files) to have a better basis
for future security updates.
So let's look at the three patched files:
demux_matroska.c:
The fix seems to be sane and protect against int overflow in
malloc(sizeof(xine_bmiheader) + track->codec_private_len);
followed by memcpy'ing track->codec_private_len bytes.
Also, various size variables in xine (e.g. buf_element_s.size) that
codec_private_len is assigned to in the code are still signed,
so bounding the range seems like a good thing.
I should note that there are still potential problems that could lead to
(at least) a crash, e.g. the pointer returned by above malloc is not
checked against NULL.
demux_real.c:
As the w and h parameters are later used in for loops, overflow
of the product might lead to out of bounds memory writes as described
in the advisory description.
It would seem that setting the stream buffer to NULL results in later
checks to prevent bad things from happening.
I would like to point out that a lot of the variables used to
calculate (positive) offsets are signed and wonder whether they also
need to be checked for < 0 in order to forbid manipulation via that
route. They are later multiplied, so that two negative signs could
also lead to a positive offset (but negative ones might be bad, too).
Also, I wonder whether producs other than w*h need to be taken
into account.
demux_realaudio.c:
This seems to require a signedness change from 1.1.15 to work
properly. Protecting w,h against overflow seems essential for much of
the same reasons as above (also, like in the snipped below, they are
still assigned to signed variables).
Similar comments as for demux_real.c apply here, too, w.r.t.
signedness of pos below and the possibilities overflowing other
products.
In particular, I wonder if the follwing (in function
demux_ra_send_chunk) is a recipe for trouble. (But could be just me.)
int x, y;
int pos;
for (y = 0; y < this->h; y++)
for (x = 0; x < this->h / 2; x++) {
pos = x * 2 * this->w + y * this->cfs;
if(this->input->read(this->input, this->frame_buffer + pos,
this->cfs) < this->cfs) {
xprintf(this->stream->xine, XINE_VERBOSITY_DEBUG,
"demux_realaudio: failed to read audio chunk\n");
Kind regards
T.
--
Thomas Viehmann, http://thomas.viehmann.net/
Patches by Matthias Hopf of SuSE backproted to Debian xine-lib 1.1.14-3.
diff -u xine-lib-1.1.14/src/demuxers/demux_realaudio.c xine-lib-1.1.14/src/demuxers/demux_realaudio.c
--- xine-lib-1.1.14/src/demuxers/demux_realaudio.c
+++ xine-lib-1.1.14/src/demuxers/demux_realaudio.c
@@ -64,7 +64,9 @@
off_t data_start;
off_t data_size;
- int sps, cfs, w, h;
+ uint32_t cfs;
+ uint16_t w, h;
+ int frame_len;
size_t frame_size;
uint8_t *frame_buffer;
@@ -197,12 +199,18 @@
this->w = _X_BE_16 (this->header+42);
this->h = _X_BE_16 (this->header+40);
this->cfs = _X_BE_32 (this->header+24);
- if (this->sps) {
- this->frame_size = this->sps * this->h * this->sps;
- this->frame_buffer = xine_xmalloc (this->frame_size);
- } else {
- this->frame_size = this->w * this->h;
- this->frame_buffer = xine_xmalloc (this->frame_size);
+ if (this->w < 0x8000 && this->h < 0x8000) {
+ uint64_t fs;
+ this->frame_len = this->w * this->h;
+ fs = (uint64_t) this->frame_len * sps;
+ if (fs < 0x80000000) {
+ this->frame_size = fs;
+ this->frame_buffer = calloc(this->frame_size, 1);
+ }
+ }
+ if (! this->frame_buffer) {
+ xprintf(this->stream->xine, XINE_VERBOSITY_DEBUG, "demux_realaudio: malloc failed\n");
+ return 0;
}
if (this->audio_type == BUF_AUDIO_28_8 || this->audio_type == BUF_AUDIO_SIPRO)
diff -u xine-lib-1.1.14/src/demuxers/demux_real.c xine-lib-1.1.14/src/demuxers/demux_real.c
--- xine-lib-1.1.14/src/demuxers/demux_real.c
+++ xine-lib-1.1.14/src/demuxers/demux_real.c
@@ -365,15 +365,20 @@
"demux_real: error, i don't handle buf type 0x%08x\n", stream->buf_type);
}
- if (stream->sps) {
- stream->frame_size = stream->w / stream->sps * stream->h * stream->sps;
- stream->frame_buffer = xine_xmalloc (stream->frame_size);
- stream->frame_num_bytes = 0;
- } else {
+ /*
+ * when stream->sps is set it used to do this:
+ * stream->frame_size = stream->w / stream->sps * stream->h * stream->sps;
+ * but it looks pointless? the compiler will probably optimise it away, I suppose?
+ */
+ if (stream->w < 32768 && stream->h < 32768) {
stream->frame_size = stream->w * stream->h;
- stream->frame_buffer = xine_xmalloc (stream->frame_size);
- stream->frame_num_bytes = 0;
+ stream->frame_buffer = calloc(stream->frame_size, 1);
+ } else {
+ stream->frame_size = 0;
+ stream->frame_buffer = NULL;
}
+
+ stream->frame_num_bytes = 0;
stream->sub_packet_cnt = 0;
if (!stream->frame_buffer)
diff -u xine-lib-1.1.14/src/demuxers/demux_matroska.c xine-lib-1.1.14/src/demuxers/demux_matroska.c
--- xine-lib-1.1.14/src/demuxers/demux_matroska.c
+++ xine-lib-1.1.14/src/demuxers/demux_matroska.c
@@ -1179,7 +1179,12 @@
break;
case MATROSKA_ID_TR_CODECPRIVATE: {
- char *codec_private = malloc (elem.len);
+ char *codec_private;
+ if (elem.len >= 0x80000000)
+ return 0;
+ codec_private = malloc (elem.len);
+ if (! codec_private)
+ return 0;
lprintf("CodecPrivate\n");
if (!ebml_read_binary(ebml, &elem, codec_private)) {
free(codec_private);
# CVE 2008-5236
Multiple heap-based buffer overflows in xine-lib 1.1.12, and other
1.1.15 and earlier versions, allow remote attackers to execute
arbitrary code via vectors related to (1) a crafted EBML element
length processed by the parse_block_group function in
demux_matroska.c; (2) a certain combination of sps, w, and h values
processed by the real_parse_audio_specific_data and
demux_real_send_chunk functions in demux_real.c; and (3) an
unspecified combination of three values processed by the open_ra_file
function in demux_realaudio.c. NOTE: vector 2 reportedly exists
because of an incomplete fix in 1.1.15.
diff -r f98dfc2d6b7a src/demuxers/demux_matroska.c
--- a/src/demuxers/demux_matroska.c Thu Oct 02 17:31:31 2008 +0200
+++ b/src/demuxers/demux_matroska.c Wed Oct 08 14:51:04 2008 +0200
@@ -1179,7 +1183,12 @@
break;
case MATROSKA_ID_TR_CODECPRIVATE: {
- char *codec_private = malloc (elem.len);
+ char *codec_private;
+ if (elem.len >= 0x80000000)
+ return 0;
+ codec_private = malloc (elem.len);
+ if (! codec_private)
+ return 0;
lprintf("CodecPrivate\n");
if (!ebml_read_binary(ebml, &elem, codec_private)) {
free(codec_private);
diff -r f98dfc2d6b7a src/demuxers/demux_real.c
--- a/src/demuxers/demux_real.c Thu Oct 02 17:31:31 2008 +0200
+++ b/src/demuxers/demux_real.c Wed Oct 08 14:51:04 2008 +0200
@@ -359,9 +362,14 @@
* stream->frame_size = stream->w / stream->sps * stream->h * stream->sps;
* but it looks pointless? the compiler will probably optimise it away, I suppose?
*/
- stream->frame_size = stream->w * stream->h;
+ if (stream->w < 32768 && stream->h < 32768) {
+ stream->frame_size = stream->w * stream->h;
+ stream->frame_buffer = calloc(stream->frame_size, 1);
+ } else {
+ stream->frame_size = 0;
+ stream->frame_buffer = NULL;
+ }
- stream->frame_buffer = calloc(stream->frame_size, 1);
stream->frame_num_bytes = 0;
stream->sub_packet_cnt = 0;
diff -r f98dfc2d6b7a src/demuxers/demux_realaudio.c
--- a/src/demuxers/demux_realaudio.c Thu Oct 02 17:31:31 2008 +0200
+++ b/src/demuxers/demux_realaudio.c Wed Oct 08 14:51:04 2008 +0200
@@ -202,11 +202,19 @@
this->h = _X_BE_16 (this->header+40);
this->cfs = _X_BE_32 (this->header+24);
- this->frame_len = this->w * this->h;
- this->frame_size = this->frame_len * sps;
-
- this->frame_buffer = calloc(this->frame_size, 1);
- _x_assert(this->frame_buffer != NULL);
+ if (this->w < 0x8000 && this->h < 0x8000) {
+ uint64_t fs;
+ this->frame_len = this->w * this->h;
+ fs = (uint64_t) this->frame_len * sps;
+ if (fs < 0x80000000) {
+ this->frame_size = fs;
+ this->frame_buffer = calloc(this->frame_size, 1);
+ }
+ }
+ if (! this->frame_buffer) {
+ xprintf(this->stream->xine, XINE_VERBOSITY_DEBUG, "demux_realaudio: malloc failed\n");
+ return 0;
+ }
if (this->audio_type == BUF_AUDIO_28_8 || this->audio_type == BUF_AUDIO_SIPRO)
this->block_align = this->cfs;
