impossibly long duration with crafted midi file

[SegfaultMasters]

An inline function wmidi_write() called by write_wav_output() from send_output() at line 2063 in wildmidi.c is recursively being called inside a while loop as no break statement is issued, results in generating a WAV file of unrestricted size, causing the system to hang up.

Affected version:

0.4.3 (Master)

Command:

wildmidi -o /home/aceteam/wildmidi/some.WAV $POC

Debugging

(gdb) 
2063                if (send_output(output_buffer, res) < 0) { [ 0%] -  
(gdb) s
write_wav_output (output_data=0x629000005200 "", output_size=16384) at /home/woot/Desktop/wildmidi-master/src/wildmidi.c:495
495	    if (wmidi_write(audio_fd, output_data, output_size) < 0) {
(gdb) s
wmidi_write (fd=3, buf=0x629000005200, size=16384) at /home/woot/Desktop/wildmidi-master/src/wildmidi.c:380
380	    return write(fd, buf, size);

(gdb) n 100
Initializing Sound System
Initializing libWildMidi 0.4.3

 +  Volume up        e  Better resampling    n  Next Midi
 -  Volume down      l  Log volume           q  Quit
 ,  1sec Seek Back   r  Reverb               .  1sec Seek Forward
 m  save as midi     p  Pause On/Off

Playing $POC 
[Approx 1092m 48s Total]
2063                if (send_output(output_buffer, res) < 0) { [ 0%] /  
(gdb) n 100
2063                if (send_output(output_buffer, res) < 0) { [ 0%] /  
(gdb) n 1000
2063                if (send_output(output_buffer, res) < 0) { [ 0%] /  
(gdb) n 5000
2063                if (send_output(output_buffer, res) < 0) { [ 0%] /  
(gdb) n 50000
     =             [    ] [100] [ 3m  9s Processed] [ 0%] -  %] \   [ 0%] -  WAV file size keeps increasing
	 
2054                perc_play = (wm_info->current_sample * 100)[ 0%] \  

Backtrace:

(gdb) bt
#0  wmidi_write (fd=3, buf=0x629000005200, size=16384) at /home/woot/Desktop/wildmidi-master/src/wildmidi.c:380
#1  0x000000000040253a in write_wav_output (output_data=0x629000005200 "", output_size=16384) at /home/woot/Desktop/wildmidi-master/src/wildmidi.c:495
#2  0x00000000004054f3 in main (argc=4, argv=0x7fffffffde38) at /home/woot/Desktop/wildmidi-master/src/wildmidi.c:2063

(gdb) i r
rax            0x4000	16384
rbx            0x7fffffffdd30	140737488346416
rcx            0x7ffff69322c0	140737330225856
rdx            0x0	0
rsi            0x0	0
rdi            0xc527fff8a40	13548474305088
rbp            0x7fffffffd990	0x7fffffffd990
rsp            0x7fffffffd980	0x7fffffffd980
r8             0x10000000000	1099511627776
r9             0xc527fff9240	13548474307136
r10            0xffffffff	4294967295
r11            0x246	582
r12            0xffffffffb4a	17592186043210
r13            0x7fffffffda50	140737488345680
r14            0x7fffffffda50	140737488345680
r15            0x0	0
rip            0x4025ea	0x4025ea <write_wav_output+217>
eflags         0x206	[ PF IF ]
cs             0x33	51
ss             0x2b	43
ds             0x0	0
es             0x0	0
fs             0x0	0
gs             0x0	0

Please do confirm if you are able to reproduce the issue with this Reproducer

[sezero]

An inline function wmidi_write() called by write_wav_output()
from send_output() ....

send_output() is a function pointer, not a function of its own:
it points to write_wav_output() if the -o cmdline switch is used.

is recursively being called inside a while loop as no break
statement is issued,

This is incorrect: if send_output() returns negative there is the
goto end2 statement and the end2 label is out of any loop.

results in generating a WAV file of unrestricted size, causing
the system to hang up.

[...]

Please do confirm if you are able to reproduce the issue with
this Reproducer

I cannot reproduce this with your attached hang_main_00 as input:
It generated a 1129292 bytes wav file and exited cleanly. I am on
i686 CentOS 6.10.

[sezero]

Could not reproduce on x86_64 Fedora 27, either.

[SegfaultMasters]

Apologises, the uploaded POC was incorrect, we have removed it.
Please try reproducing with the below linked POC, it will work.

Corrected - https://github.com/SegfaultMasters/covering360/blob/master/wildmidi/1_hang_main_00

Using Ubuntu 16.04 LTS 64bit

Caution - This issue may consume too much memory of your disk

[sezero]

Reproduced. Although the issue is not related to any of the output
functions: something is borked in midi parsing I guess, as evidenced
by the impossible duration reported: Approx 1092m 48s Total
Changing the title accordingly.

@chrisisonwildcode , this is for you I think.

[SegfaultMasters]

This seems like a security issue when wildmidi is used with other applications, we can say it as excessive memory consumption. And I do agree, the issue is with midi parsing

[sezero]

Further info:

• wildmidi-0.3.14: does not seem affected: Approx 0m 12s Total

• libtimidity: is affected: Approx 1013m 35s Total

[SegfaultMasters]

Thanks for the further clarification.
For which package should we raise CVE for?

[sezero]

Last release from the 0.4 series is 0.4.2, so that should be it I guess: @psi29a?

[sezero]

For libtimidity, there is a signed integer overflow, found thanks
to gcc ubsan, and a lazy solution can be something like this:

diff --git a/src/readmidi.c b/src/readmidi.c
index d8367a4..4a375f0 100644
--- a/src/readmidi.c
+++ b/src/readmidi.c
@@ -502,6 +502,14 @@ static MidEvent *groom_list(MidSong *song, sint32 divisions,sint32 *eventsp,
       /* Recompute time in samples*/
       if ((dt=meep->event.time - at) && !counting_time)
 	{
+	  if (song->sample_increment > 2147483647/dt ||
+	      song->sample_correction > 2147483647/dt)
+	    {
+	      DEBUG_MSG("INTEGER OVERFLOW\n");
+	      free_midi_list(song);
+	      timi_free(groomed_list);
+	      return NULL;
+	    }
 	  samples_to_do = song->sample_increment * dt;
 	  sample_cum += song->sample_correction * dt;
 	  if (sample_cum & 0xFFFF0000)

In wildmidi, ubsan didn't help because things are unsigned.

[chrisisonwildcode]

Three things I want to hash out so everybody knows where I am at in the dev cycle.

Firstly, yes I guess it is theoretically possible to unsigned integer overflow wildmidi by using a crafted midi file. The way to prevent this would be to hard code an upper limit on delta times. what is the longest piece of music in existence to date? Also there shouldn't(quotations) be excessive memory consumption unless the lib is being abused by the calling application, for example our example player only gets the lib to process chunks of audio at a time. The only way to prevent abuse of the lib would be to limit the length of audio data it will process at a time forcing the calling app to ask for chunks instead of the whole thing at once.

Secondly, I am slowly working on a side project that will help to reverse engineer file formats and debug issues with those formats including the incredibly long reported file times. While planned for a while it is still in its infancy but moving into alpha status within the next month or 2 as key parts are written and tested. Again the side project will (underline/bold) greatly help move wildmidi forward with debugging current issues with currently supported formats and help with development for future supported formats. It will not be a part of wildmidi as its usability goes way beyond what we do with wildmidi.

Thirdly, the delta times that create the incredibly long file times occur after the last event that influences any note. If I remember there is an option to "trim the fat" and when used with those files they report a more respectable time. There are conflicting opinions about the cause of the long delta times hence the side project in my second point.

[sezero]

yes I guess it is theoretically possible to unsigned integer
overflow wildmidi by using a crafted midi file. The way to
prevent this would be to hard code an upper limit on delta
times. what is the longest piece of music in existence to date?

I agree: there is not a foolproof way of truly 'fixing' this.

For the record, I pushed the following patch to fix the signed int
overflow in libtimidity:
https://sourceforge.net/p/libtimidity/libtimidity/ci/11be98a89eac229111420e6a3d521edbfddb0dbc/
sezero/libtimidity@11be98a

[SegfaultMasters]

Is there any way for us to test and confirm the patch solves the issue? I see it is libtimidity but not wildmidi though

[sezero]

Is there any way for us to test and confirm the patch solves the issue?

No: because there is no patch for wildmidi yet.

I see it is libtimidity but not wildmidi though

Yes, I specifically said that it is so.

[psi29a]

There was a midi file that just looped itself for 300+ minutes... perfectly valid.

Should we put in an arbitrary limit on wave-out with an additional flag that more or less says: I know what I'm doing, render this super long file that will eat my 10TiB HDD?

This means that playback is not effected, only wave-out/write to disk.

[sezero]

Don't know, but I have a feeling that that might make things unnecessarily complex.

What I wonder is how 0.3.14 is not affected by this.

[psi29a]

Hmmm...

[psi29a]

I'm torn on this one because there is nothing wrong with a long midi file.

[sezero]

Should we close as WONTFIX ?

[SegfaultMasters]

Totally agree that it's an expected behavior, but I strongly believe the user should be well aware of the size of the output file to be generated.

Secondly, even SIGINT doesn't stop the program, we need to kill the process in order to stop the file generation.

Should we put in an arbitrary limit on wave-out with an additional flag that more or less says: I know what I'm doing, render this super long file that will eat my 10TiB HDD?

This fix sounds good.

[sezero]

Secondly, even SIGINT doesn't stop the program, we need to
kill the process in order to stop the file generation.

If this is running the wildmidi player binary, hitting 'q' stops it just fine.

[SegfaultMasters]

If this is running the wildmidi player binary, hitting 'q' stops it just fine.

True, only if the user is using wildmidi binary running on CLI. But if wildmidi is used in any web applications (in nodejs) cannot kill the process, instead, it consumes a lot of memory.

Would you agree if I say that we can leverage wildimidi usage to convert files via online just like other online converters

[sezero]

I'll defer to the wisdom of project owners for this one.

[psi29a]

I'm comfortable with a default limit of 1GiB, high enough to contain most midi files. Anything larger than this requires an extra flag (via player and lib) to by-pass limit.

I'm still curious as to why this isn't a problem for 3.x, do we not loop there?

[sezero]

I'm still curious as to why this isn't a problem for 3.x

As am I.

Another question is whether this is a dup of #176 somehow (how?)??..

[psi29a]

What does 0.3.x do when trying to play back this midi file? (I haven't yet tested.) Does it write anything to disk? Does it error out?

[sezero]

What does 0.3.x do when trying to play back this midi file?
(I haven't yet tested.) Does it write anything to disk? Does
it error out?

It just plays it it and exits cleanly for me. (Try yourself.)

[psi29a]

Yes, but doesn't write to disk? That's the problem right... writing out to disk?

[sezero]

Why do you think that it doesn't write to disk? Running wildmidi with the -o switch writes a small wav file and exits cleanly. (Try yourself.)

[sezero]

For me, wildmidi-0.3.14 produces a 1572264 bytes wav file with default rate and channels using an old version of eaw patches.

[psi29a]

I misunderstood. Waiting for additional information from @chrisisonwildcode since he seems to be on track there as well.

[SegfaultMasters]

Hey there, is there any update on fixing this issue. Just curious to know

[sezero]

We're waiting @chrisisonwildcode to look at things, so I gather it'll take some time :(

[sezero]

Created PR #220 for this.

[sezero]

Fixed by the following commits, closing:

wildmidi-0.3 branch:
775894c

master branch:
ddd719d