From 4ca8ca9694782ae4c0b4288f14eb6e106502324d Mon Sep 17 00:00:00 2001 From: JP Cimalando Date: Mon, 28 Jan 2019 07:21:12 +0100 Subject: [PATCH 1/8] Fix buffer overflow on tempo change event --- src/smf_tempo.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/smf_tempo.c b/src/smf_tempo.c index a5592f1..d9a4df9 100644 --- a/src/smf_tempo.c +++ b/src/smf_tempo.c @@ -133,6 +133,11 @@ maybe_add_to_tempo_map(smf_event_t *event) /* Tempo Change? */ if (event->midi_buffer[1] == 0x51) { + if (event->midi_buffer_length < 6) { + g_critical("Tempo Change event seems truncated."); + return; + } + int new_tempo = (event->midi_buffer[3] << 16) + (event->midi_buffer[4] << 8) + event->midi_buffer[5]; if (new_tempo <= 0) { g_critical("Ignoring invalid tempo change."); From d6ccfb8f25d7a273592d7126f4a6629ff21b6c7c Mon Sep 17 00:00:00 2001 From: JP Cimalando Date: Mon, 28 Jan 2019 07:48:37 +0100 Subject: [PATCH 2/8] Fix validity checks of escaped data --- src/smf_load.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/smf_load.c b/src/smf_load.c index 7389654..eb558dc 100644 --- a/src/smf_load.c +++ b/src/smf_load.c @@ -283,7 +283,7 @@ expected_sysex_length(const unsigned char status, const unsigned char *second_by { int sysex_length, len; - assert(status == 0xF0); + assert(status == 0xF0 || status == 0xF7); if (buffer_length < 3) { g_critical("SMF error: end of buffer in expected_sysex_length()."); @@ -441,7 +441,7 @@ extract_escaped_event(const unsigned char *buf, const int buffer_length, smf_eve message_length = expected_escaped_length(status, c, buffer_length - 1, &vlq_length); - if (message_length < 0) + if (message_length <= 0) return (-3); c += vlq_length; From 64b31a2c9cfd07715c94c1ca23b074825f76d52a Mon Sep 17 00:00:00 2001 From: JP Cimalando Date: Mon, 28 Jan 2019 08:27:45 +0100 Subject: [PATCH 3/8] Fix buffer overflow by reducing length of truncated tracks --- src/smf_load.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/smf_load.c b/src/smf_load.c index eb558dc..6c0909f 100644 --- a/src/smf_load.c +++ b/src/smf_load.c @@ -768,11 +768,17 @@ smf_event_is_valid(const smf_event_t *event) static int parse_mtrk_chunk(smf_track_t *track) { + smf_t *smf = track->smf; smf_event_t *event; if (parse_mtrk_header(track)) return (-1); + if (track->file_buffer + track->file_buffer_length > smf->file_buffer + smf->file_buffer_length) { + /* Truncated track? */ + track->file_buffer_length = smf->file_buffer_length - (track->file_buffer - smf->file_buffer); + } + for (;;) { event = parse_next_event(track); From 81c101b88788859cfbe71bcc47496c778bde6439 Mon Sep 17 00:00:00 2001 From: JP Cimalando Date: Mon, 28 Jan 2019 08:28:26 +0100 Subject: [PATCH 4/8] Stop parsing tracks after the first failure. Fixes heap use after free. --- src/smf_load.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/smf_load.c b/src/smf_load.c index 6c0909f..c1be16d 100644 --- a/src/smf_load.c +++ b/src/smf_load.c @@ -889,6 +889,7 @@ smf_load_from_memory(const void *buffer, const int buffer_length) if (parse_mtrk_chunk(track)) { g_warning("SMF warning: Cannot load track."); smf_track_delete(track); + break; } track->file_buffer = NULL; From 644b5e6b2257a918c10f6dceeaaa80d5a9924e5f Mon Sep 17 00:00:00 2001 From: JP Cimalando Date: Mon, 28 Jan 2019 09:02:03 +0100 Subject: [PATCH 5/8] Handle non-EOT-terminated tracks. Fixes raised assertion buffer_length == 0. --- src/smf_load.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/smf_load.c b/src/smf_load.c index c1be16d..e17a65f 100644 --- a/src/smf_load.c +++ b/src/smf_load.c @@ -780,6 +780,11 @@ parse_mtrk_chunk(smf_track_t *track) } for (;;) { + if (track->next_event_offset == track->file_buffer_length) { + g_warning("SMF warning: The track did not finish with the End of Track event."); + break; + } + event = parse_next_event(track); /* Couldn't parse an event? */ From fa0d7f82620519a6e9af20a64e3355d60439aef9 Mon Sep 17 00:00:00 2001 From: JP Cimalando Date: Mon, 28 Jan 2019 10:20:30 +0100 Subject: [PATCH 6/8] Fix the assertion problem `is_sysex_byte(status)` --- src/smf_load.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/smf_load.c b/src/smf_load.c index e17a65f..7027ed7 100644 --- a/src/smf_load.c +++ b/src/smf_load.c @@ -503,11 +503,21 @@ extract_midi_event(const unsigned char *buf, const int buffer_length, smf_event_ return (-1); } - if (is_sysex_byte(status)) + if (is_sysex_byte(status)) { + if (c == buf) { + g_critical("SMF error: running status is not applicable to System Exclusive events."); + return (-2); + } return (extract_sysex_event(buf, buffer_length, event, len, last_status)); + } - if (is_escape_byte(status)) + if (is_escape_byte(status)) { + if (c == buf) { + g_critical("SMF error: running status is not applicable to Escape events."); + return (-2); + } return (extract_escaped_event(buf, buffer_length, event, len, last_status)); + } /* At this point, "c" points to first byte following the status byte. */ message_length = expected_message_length(status, c, buffer_length - (c - buf)); From 854a03a699a2387c31b7d0ce63b56fa60126e17a Mon Sep 17 00:00:00 2001 From: JP Cimalando Date: Mon, 28 Jan 2019 10:41:31 +0100 Subject: [PATCH 7/8] Fix a logic error in Escape event handling --- src/smf_load.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/smf_load.c b/src/smf_load.c index 7027ed7..01ea81f 100644 --- a/src/smf_load.c +++ b/src/smf_load.c @@ -460,12 +460,12 @@ extract_escaped_event(const unsigned char *buf, const int buffer_length, smf_eve memcpy(event->midi_buffer, c, message_length); - if (smf_event_is_valid(event)) { + if (!smf_event_is_valid(event)) { g_critical("Escaped event is invalid."); return (-1); } - if (smf_event_is_system_realtime(event) || smf_event_is_system_common(event)) { + if (!(smf_event_is_system_realtime(event) || smf_event_is_system_common(event))) { g_warning("Escaped event is not System Realtime nor System Common."); } From 433efab4b44df9d40376b886acff7eb5aff55694 Mon Sep 17 00:00:00 2001 From: JP Cimalando Date: Mon, 28 Jan 2019 10:55:27 +0100 Subject: [PATCH 8/8] Fix a memory leak in case of loading failure --- src/smf_load.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/smf_load.c b/src/smf_load.c index 01ea81f..84f7221 100644 --- a/src/smf_load.c +++ b/src/smf_load.c @@ -890,13 +890,17 @@ smf_load_from_memory(const void *buffer, const int buffer_length) smf->file_buffer_length = buffer_length; smf->next_chunk_offset = 0; - if (parse_mthd_chunk(smf)) + if (parse_mthd_chunk(smf)) { + smf_delete(smf); return (NULL); + } for (i = 1; i <= smf->expected_number_of_tracks; i++) { smf_track_t *track = smf_track_new(); - if (track == NULL) + if (track == NULL) { + smf_delete(smf); return (NULL); + } smf_add_track(smf, track);