spudec_process_control can read beyond the end of a packet, now with fix!

[wififreedom]

    for (type = this->packet[off++]; type != 0xff; type = this->packet[off++]) {

The above loop does not check for end of packet.

I have just seen a problem where a packet is 2020 bytes long.

At byte 2019 there is a control command 0x02 (stop display). There is no 0xff after the 0x02, because that's beyond the end of the fragment.

But spudec_process_control continues on reading control commands, and encounters invalid control types.

Last part of the output with MPLAYER_VERBOSE=2:

SPUgot: off=2019  size=2020
date=0
cmd=1  Start display!
...
date=...
cmd=2  Stop display!
cmd=0  Menu ID
...
cmd=214  spudec: Error determining control type 0xd6.  Skipping -52 bytes.
SPUasm: packet too short

[wififreedom]

spudec_process_control should not read beyond the available data.

However, there is a second problem that is the actual cause in spudec_assemble(). This function may conclude that the packet has been assembled completely before it actually is:

• It looks for a next control pointer that points to itself.

• If that's present, it concludes the packet is complete. But not all data of the control sequence may have been assembled, some of it may still be in the next fragment that's not been assembled yet.

For the case above I have analyzed the data with hexdump. There is another fragment with a lot of 0xff beyond offset 2020. I'm not sure if that ever reaches spudec_assemble.

[wififreedom]

OK. I've got a fix.

First of all, in spudec_assemble, there is this code:

  if (len < 2) {
      mp_msg(MSGT_SPUDEC,MSGL_WARN,"SPUasm: packet too short\n");
      return;
  }
  spu->packet_pts = pts100;
  if (spu->packet_offset == 0) {
    unsigned int len2 = get_be16(packet);

The check for len < 2 is only required for the first fragment, where len2 (packet size) needs to be read (2 bytes), so it should be within the if instead. For the next fragments, there is no packet size to read, there is only data. So this becomes:

  spu->packet_pts = pts100;
  if (spu->packet_offset == 0) {
    unsigned int len2 = 0;
    if (len < 2) {
        mp_msg(MSGT_SPUDEC,MSGL_WARN,"SPUasm: packet too short\n");
        return;
    }
    len2 = get_be16(packet);

[wififreedom]

Fix, part 2:

Then, also in spudec_assemble, there is a kludge between #if 1 and #else that was necessary because of the invalid check for len < 2 above:

#if 1
  if ((spu->packet_offset == spu->packet_size) ||
      ((spu->packet_offset + 1) == spu->packet_size)){
  ...
  }
#else
  if (spu->packet_offset == spu->packet_size) {
    spudec_decode(spu, pts100);
    spu->packet_offset = 0;
  }
#endif

With the fix for len < 2 above, the code within #if 1 and #else can be removed, and the original correct code can be reinstated:

  if (spu->packet_offset == spu->packet_size) {
    spudec_decode(spu, pts100);
    spu->packet_offset = 0;
  }

[wififreedom]

The above fixes in spudec_assemble should be enough to cause spudec_process_control to no longer read beyond the packet size, as long as the packet is well-formed.

I also recommend that spudec_process_control is modified to not read beyond the packet size, in case the packet is not well-formed.

[beastd]

Thanks for the report!

Seems there is more problem than one like you mentioned.

Will look into what I can do and would be grateful for testing.

[beastd]

Could you attach e.g. hex dump of the case that failed for you?

Think it will make it easier to understand what exactly was going on.

I guess I finally understand what was going wrong now.

[wififreedom]

Here's the relevant part, as I decoded it manually.

Relevant part of hexdump, decoded below:

0000  00 00 01 ba 45 .. .. ..  .. .. .. .. .. .. 00 00
0010  01 bd 07 ec 81 80 05 ..  .. .. .. .. 20 07 e4 07
0020  c6 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
...
07e0  .. .. .. 00 00 07 de ..  .. .. .. .. .. .. .. ..
07f0  .. .. .. .. .. .. .. ..  .. .. .. 00 80 07 de 02

0800  00 00 01 ba 45 .. .. ..  .. .. .. .. .. .. 00 00
0810  01 bd 00 05 81 00 00 20  ff .. .. .. .. .. .. ff
0820  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
*
1000

==== First fragment of length 2019 ====
data:
0000  00 00 01 ba 45 .. .. ..  .. .. .. .. .. .. 00 00
0010  01 bd 07 ec 81 80 05 ..  .. .. .. .. 20 07 e4 07
0020  c6 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00

data decoded (function mpeg_run):
0000  00 00 01 = "00 00 01"
0000           ba = packet start code
0000              45 = mpeg version 4
0000                 .. .. ..  .. .. .. .. .. .. 00 00
0010  01 = "00 00 01"
0010     bd = packet
0010        07 ec = len = 0x07ec = 2028 bytes
  idx = 0x0014
0010              81 = .VOB stream
0010                 80 = pts_flags
0010                    05 = hdrlen
  dataidx = 0x0017 + 0x05 = 0x001c
0010                       ..  .. .. .. ..
0010                                       20 = aid
  pos = 0x001d
  fragment_size = len - (pos - idx) = 0x07ec - (0x001d - 0x0014) = 0x07ec - 0x0009 = 0x07e3 = 2019 bytes
0010                                          07 e4 = 0x7e4 = SPDSZ = packet size = 2020 bytes

data decoded (functions spudec_assemble, spudec_process_control):
0010                                                07
0020 c6 = SP_DCSQTA = 0x07c6 -> SP_DCSQT is at 0x001d + 0x07c6 = 0x07e3

data (SP_DCSQT at 0x07e3):
07e0  .. .. .. 00 00 07 de ..  .. .. .. .. .. .. .. ..
07f0  .. .. .. .. .. .. .. ..  .. .. .. 00 80 07 de 02

data decoded (functions spudec_assemble, spudec_process_control):
07e0  .. .. .. 00 00 = SP_DCSQ_STM
07e0                 07 de = SP_NXT_DSCQ_SA = 0x07de -> next SP_DCSQ is at 0x001d + 0x07de = 0x07fb
07f0
07e0                                    00 80 = SP_DCSQ_STM
07f0                                          07 de = SP_NXT_DSCQ_SA, points to itself
  this is where spudec_assemble ays: ok, I have all data, but it doesn't have the 0xff in the next fragment
  and calls spudec_process_control
07f0                                                02 = STP_DSP = Stop Display
  this is where spudec_process_control continues to read past the first fragment,
  without spudec_assemble having read the next fragment,
  so spudec_process_control encounters undefined data

==== Next and last fragment of length 1, which spudec_assemble never reads ====
data:
0800  00 00 01 ba 45 .. .. ..  .. .. .. .. .. .. 00 00
0810  01 bd 00 05 81 00 00 20  ff .. .. .. .. .. .. ff
0820  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff

data decoded (function mpeg_run):
0800
0810  .. bd = packet
0810        00 05 = len = 0x0005 = 5 bytes
  idx = 0x0814
0810              81 = .VOB stream
0810                 00 = pts_flags
0810                    00 = hdrlen
  dataidx = 0x0817 + 0x00 = 0x0817
0810                       20 = aid
  pos = 0x0818
  fragment_size = len - (pos - idx) = 0x0005 - (0x0818 - 0x0814) = 0x0005 - 0x0004 = 0x0001 = 1
data decoded (function_spudec_process_control, but it never gets this data):
0810                           ff = CMD_END

[wififreedom]

What I think happened:

In spudec_assemble, the invalid len < 2 check for all fragments, instead of for only the first fragment, caused rejection of valid fragments with length 1 byte, logging "SPUasm: packet too short\n".

This made it seem that some DVDs rounded the packet size up with 1 byte, which they don't. Then to solve that rounding problem that doesn't really exist, the kludge between #if 1 and #else was added, which is not needed. Instead, the len < 2 check should be moved to within the code for the first fragment only.

[wififreedom]

... and, of course, spudec_process_control should be made safer to never read beyond the available assembled data.

[beastd]

Replying to wififreedom:

... and, of course, spudec_process_control should be made safer to never read beyond the available assembled data.

Started to work on this first.

I intend to post patches here tonight (European time) or tomorrow.
Would be great if you could give them some testing with this specific samples where you experienced the troubles.

[wififreedom]

I still have the data, so testing patches shouldn't be a problem.

[beastd]

I have attached a patch ( attachment:v1-0001-WIP-XXX-spudec_process_control-Don-t-read-beyond-.patch​ ) that attempts to stop the over reading in function spudec_process_control

It's a bit rushed and I only tested that it compiles.

Would be great if you could test it (without your proposed 2-part-fix).
The idea being that spudec_process_control now never reads beyond the boundaries of the packet buffer.

The display may end up incomplete.

Given the first test works as expected.
Now applying your 2-part-fix on top should then still work with the bounds checks never causing early returns from spudec_process_control.

Hope this makes sense and thanks in advance!

[wififreedom]

I've seen this, will check tomorrow at the earliest.

[wififreedom]

I've done a quick review:

I'm looking at the code and find it hard to read:

if (off >= this->packet_size - 3) {
  return;
}
...
off+=4;

Easier to read and verify:

if ((off + 4) > this->packet_size) {
  return;
}
...
off+=4;

I'd personally prefer adding functions like checked_get_be8, checked_get_be16 and checked_get_be24 that perform the checks, and never accessing the packet data directly.

There's no offset check for the get_be16 in this code yet:

  next_control:
    if (!display)
      continue;
    if (end_pts == UINT_MAX && start_off != next_off) {           
      end_pts = get_be16(this->packet + next_off) * 1024;         
      end_pts = 1 - pts100 >= end_pts ? 0 : pts100 + end_pts - 1; 
    } 

By just returning, the code fails silently, without any indication that something's wrong with the input (or with the code). Maybe instead of "return;", use "goto input_short;" and add at the end of the function:

  return;

input_short:
  mp_msg(... some error message ...);
  return;

and/or return a value and let the caller decide to log a message or not.

And, I just realized, related but off-topic for this bug report: there is also the image data, handled in different spudec_* functions. If spudec_process_control did not implemented bounds checks...

Also related, but off-topic: an additional control code 0x07 appears to exist that you may want to handle or ignore and skip.

I'll wait with testing in case you want to modify the patch. Either way, let me know.

[beastd]

In general I'm not really satisfied with the whole spudec.c implementation.

I kind of like the idea of printing a message in case we run into any of these checks as it gives hints in case we are decoding incomplete data.

Regarding

if (off >= this->packet_size - 3) {
  return;
}
...
off+=4;

I did this on purpose because (off + 4) > this->packet_size can overflow.
Though for the case of - 5 I should have split it because of possible underflow.

Another probably better way to deal with this would be to pad the buffer at the end (with 0xFF bytes) and get rid of most checks. But right now at least I don't want to invest to much into changing this code.

I guess I will have to improve this at least a little, but it might take me a while for me to update the patch.

Feel free to try or improve on this if you want to.

[wififreedom]

I see, it's safer to write it that way.

It would be good to have some corrupted frames to test this.

Based on a real frame, it would be possible to write some test code that crafts bad frames by just manipulating the packet size and frame size fields. Possibly starting from 0 and incrementing by 1 until the actual packet and frame size is exceeded, writing out all the frames to .idx and .sub files, then reading them with the vobsub and spudec modules with something like valgrind checking for memory issues.

[beastd]

Replying to wififreedom:

Based on a real frame, it would be possible to write some test code that crafts bad frames by just manipulating the packet size and frame size fields. Possibly starting from 0 and incrementing by 1 until the actual packet and frame size is exceeded, writing out all the frames to .idx and .sub files, then reading them with the vobsub and spudec modules with something like valgrind checking for memory issues.

That sounds like a good method for some more intensive testing.
Could also even go more crazy and fuzz/mutate/randomly generate other parts of the data.

It generates more test cases than needed, but the results could guide the way to select the interesting input to keep around for testing.

You might be able to detect my off >= this->packet_size - 5 mistake this way.

Please go ahead if you would like to do this kind of exhaustive testing.

Also if you want to improve mplayer your contributions would be welcome.

[wififreedom]

With a small generated and modified frame the runtime is not prohibitive. I'll try to write this test.

[wififreedom]

I've:

• created a .srt file by hand: one subtitle with just a "." that starts at time 0.
• exported that exported to a .idx/.sub file
• hexdumped it and imported it into a .c file
• packet size is 92 bytes in 1 fragment
• wrote a small program around it that varies packet size and SPDSZ and creates .sub/.idx files 0...92
• create a very small mp4 video file with black screen, 2x2 pixels.

Example output, with standard ubuntu mplayer, frame size 91 (1 byte short):

+ valgrind mplayer -really-quiet -vobsub sub-91 -vobsubid 0 subtest.mp4
...
do_connect: could not connect to socket
connect: No such file or directory
==144587== Invalid read of size 1
==144587==    at 0x41B7E88: ??? (in /usr/bin/mplayer)
==144587==    by 0x40F6650: ??? (in /usr/bin/mplayer)
==144587==    by 0x409092A: ??? (in /usr/bin/mplayer)
==144587==    by 0x4084211: ??? (in /usr/bin/mplayer)
==144587==    by 0x93DD600: (below main) (libc_start_call_main.h:59)
==144587==  Address 0x16a2e852 is 0 bytes after a block of size 82 alloc'd
==144587==    at 0x4C26858: malloc (vg_replace_malloc.c:447)
==144587==    by 0x41B7C14: ??? (in /usr/bin/mplayer)
==144587==    by 0x40F6650: ??? (in /usr/bin/mplayer)
==144587==    by 0x409092A: ??? (in /usr/bin/mplayer)
==144587==    by 0x4084211: ??? (in /usr/bin/mplayer)
==144587==    by 0x93DD600: (below main) (libc_start_call_main.h:59)
==144587== 
==144587== Invalid read of size 4
==144587==    at 0x41C68D4: ??? (in /usr/bin/mplayer)
==144587==    by 0x40F65A5: ??? (in /usr/bin/mplayer)
==144587==    by 0x409092A: ??? (in /usr/bin/mplayer)
==144587==    by 0x4084211: ??? (in /usr/bin/mplayer)
==144587==    by 0x93DD600: (below main) (libc_start_call_main.h:59)
==144587==  Address 0x15603980 is 0 bytes after a block of size 32 alloc'd
==144587==    at 0x4C26858: malloc (vg_replace_malloc.c:447)
==144587==    by 0x41C6B49: ??? (in /usr/bin/mplayer)
==144587==    by 0x41C778F: ??? (in /usr/bin/mplayer)
==144587==    by 0x408EA28: ??? (in /usr/bin/mplayer)
==144587==    by 0x4081E2C: ??? (in /usr/bin/mplayer)
==144587==    by 0x93DD600: (below main) (libc_start_call_main.h:59)
==144587== 
...

[beastd]

Replying to wififreedom:

[...]

Example output, with standard ubuntu mplayer, frame size 91 (1 byte short):

+ valgrind mplayer -really-quiet -vobsub sub-91 -vobsubid 0 subtest.mp4
...
do_connect: could not connect to socket
connect: No such file or directory
==144587== Invalid read of size 1
==144587==    at 0x41B7E88: ??? (in /usr/bin/mplayer)
==144587==    by 0x40F6650: ??? (in /usr/bin/mplayer)
==144587==    by 0x409092A: ??? (in /usr/bin/mplayer)
==144587==    by 0x4084211: ??? (in /usr/bin/mplayer)
==144587==    by 0x93DD600: (below main) (libc_start_call_main.h:59)
==144587==  Address 0x16a2e852 is 0 bytes after a block of size 82 alloc'd
==144587==    at 0x4C26858: malloc (vg_replace_malloc.c:447)
==144587==    by 0x41B7C14: ??? (in /usr/bin/mplayer)
==144587==    by 0x40F6650: ??? (in /usr/bin/mplayer)
==144587==    by 0x409092A: ??? (in /usr/bin/mplayer)
==144587==    by 0x4084211: ??? (in /usr/bin/mplayer)
==144587==    by 0x93DD600: (below main) (libc_start_call_main.h:59)
==144587== 
==144587== Invalid read of size 4
==144587==    at 0x41C68D4: ??? (in /usr/bin/mplayer)
==144587==    by 0x40F65A5: ??? (in /usr/bin/mplayer)
==144587==    by 0x409092A: ??? (in /usr/bin/mplayer)
==144587==    by 0x4084211: ??? (in /usr/bin/mplayer)
==144587==    by 0x93DD600: (below main) (libc_start_call_main.h:59)
==144587==  Address 0x15603980 is 0 bytes after a block of size 32 alloc'd
==144587==    at 0x4C26858: malloc (vg_replace_malloc.c:447)
==144587==    by 0x41C6B49: ??? (in /usr/bin/mplayer)
==144587==    by 0x41C778F: ??? (in /usr/bin/mplayer)
==144587==    by 0x408EA28: ??? (in /usr/bin/mplayer)
==144587==    by 0x4081E2C: ??? (in /usr/bin/mplayer)
==144587==    by 0x93DD600: (below main) (libc_start_call_main.h:59)
==144587== 
...

I guess you're binary is missing debug data.
Have a look at ./configure --enable-debug[=<number>]

[wififreedom]

I'm having trouble running valgrind properly. It doesn't like axv512 instructions, but when I disable them with "./configure --extra-cflags=-mno-avx512f", I get:

+ valgrind /home/user/repos/svn/mplayer/mplayer -really-quiet -vobsub sub-91 -vobsubid 0 subtest.mp4
...
==202457==
Failed to idle DMA.
Failed to idle DMA.
Failed to idle DMA.
Failed to idle DMA.
Failed to idle DMA.
Failed to idle DMA.
Failed to idle DMA.
X connection to :0 broken (explicit kill or server shutdown).
==202457== 
==202457== HEAP SUMMARY:
==202457==     in use at exit: 8,340,929 bytes in 14,604 blocks
...

and my window manager and browser windows crash. Not good, I can't keep testing like that.

Anyway, this is the relevant output:

==202457== Invalid read of size 1
==202457==    at 0x42803C1: spudec_process_control (spudec.c:474)
==202457==    by 0x42803C1: spudec_decode (spudec.c:595)
==202457==    by 0x42803C1: spudec_assemble (spudec.c:663)
==202457==    by 0x41C51E5: update_subtitles (mpcommon.c:254)
==202457==    by 0x41774C2: generate_video_frame (mplayer.c:1805)
==202457==    by 0x41774C2: update_video (mplayer.c:2525)
==202457==    by 0x416BA63: main (mplayer.c:3820)
==202457==  Address 0x931be52 is 0 bytes after a block of size 82 alloc'd
==202457==    at 0x703C858: malloc (vg_replace_malloc.c:447)
==202457==    by 0x4280146: spudec_assemble (spudec.c:626)
==202457==    by 0x41C51E5: update_subtitles (mpcommon.c:254)
==202457==    by 0x41774C2: generate_video_frame (mplayer.c:1805)
==202457==    by 0x41774C2: update_video (mplayer.c:2525)
==202457==    by 0x416BA63: main (mplayer.c:3820)
==202457== 
==202457== Invalid read of size 4
==202457==    at 0x428DD14: vobsub_queue_reseek (vobsub.c:1092)
==202457==    by 0x428FF01: vobsub_get_packet (vobsub.c:1124)
==202457==    by 0x41C5141: update_subtitles (mpcommon.c:222)
==202457==    by 0x41774C2: generate_video_frame (mplayer.c:1805)
==202457==    by 0x41774C2: update_video (mplayer.c:2525)
==202457==    by 0x416BA63: main (mplayer.c:3820)
==202457==  Address 0x832b830 is 0 bytes after a block of size 32 alloc'd
==202457==    at 0x703C858: malloc (vg_replace_malloc.c:447)
==202457==    by 0x428DF89: packet_queue_ensure (vobsub.c:545)
==202457==    by 0x428EB48: packet_queue_grow (vobsub.c:559)
==202457==    by 0x428EB48: vobsub_add_timestamp (vobsub.c:662)
==202457==    by 0x428EB48: vobsub_parse_timestamp (vobsub.c:705)
==202457==    by 0x428EB48: vobsub_parse_one_line (vobsub.c:810)
==202457==    by 0x428EB48: vobsub_open (vobsub.c:928)
==202457==    by 0x4175AF6: add_vob_subtitle (mplayer.c:1116)
==202457==    by 0x4169702: main (mplayer.c:3228)
==202457== 

[wififreedom]

I've got a workaround for the "Failed to idle DMA" issue: "-vo gl". So I can test again.

[wififreedom]

Off topic, but you should know: invalid write with "-vo jpeg":

+ valgrind /home/user/repos/svn/mplayer/mplayer -vo jpeg -vobsub sub-84 -vobsubid 0 subtest.mp4
==209318== Invalid write of size 8
==209318==    at 0x4CFC9D7: ??? (in /home/user/repos/svn/mplayer/mplayer)
==209318==    by 0x1F: ???
==209318==    by 0x5: ???
==209318==  Address 0x96517c0 is 8 bytes after a block of size 24 alloc'd
==209318==    at 0x7044D09: posix_memalign (vg_replace_malloc.c:2232)
==209318==    by 0x4D2A3FE: av_malloc (mem.c:107)
==209318==    by 0x41E3E62: mp_image_alloc_planes (mp_image.c:59)
==209318==    by 0x41E8DF7: vf_get_image (vf.c:419)
==209318==    by 0x420C793: put_image (vf_scale.c:473)
==209318==    by 0x41E2DCF: filter_video (dec_video.c:500)
==209318==    by 0x41774FD: generate_video_frame (mplayer.c:1809)
==209318==    by 0x41774FD: update_video (mplayer.c:2525)
==209318==    by 0x416BA63: main (mplayer.c:3820)
==209318==

[wififreedom]

About the vobsub_queue_reseek issue discovered with valgrind:

• first call to vobsub_queue_reseek leaves queue->current_index = 1, queue->packets_size = 1, queue->packets_reserve = 1.
• second call to vobsub_queue_reseek accesses packet at queue->current_index = 1 (past the end) without first checking if queue->current_index < queue->packets_size.

Would you like me to create separate tickets for the additional memory issues?

[wififreedom]

small c program to generate input to test vobsub and spudec modules with valgrind

[wififreedom]

script to run mplayer with valgrind, using input generated by subtest.c

[wififreedom]

I've attached the small program and script I used to produce the above valgrind results. Feel free to modify it as you like to test mplayer and your modified patch.

[beastd]

Replying to wififreedom:
[...]

Would you like me to create separate tickets for the additional memory issues?

Yes, please open new tickets. Otherwise this ticket will get to convoluted and hard to follow.

We can collect the spudec problems here as long as they are loosly related.

All in all I think this is going somewhere, I will attach reworked patches later this weekend.

With the new patch set we can hopefully close this ticket soon.

[wififreedom]

Created ​https://trac.mplayerhq.hu/ticket/2433 for above out of bounds write with "-vo jpeg".

Created ​https://trac.mplayerhq.hu/ticket/2434 for out of bounds read in vobsub_queue_reseek.