From 43021d16c7bcd3f9f70214827755a5163782b633 Mon Sep 17 00:00:00 2001
From: Moritz Bunkus <moritz@bunkus.org>
Date: Thu, 25 Oct 2018 21:59:24 +0200
Subject: [PATCH] kax_file_c, ElementReader: only delete returned upper-level
 element if found

The `EbmlElement::Read` function returns two values via reference
parameters. They're called `UpperEltFound` (an integer) and
`FoundElt` (a pointer to an EBML element). They're used for passing
back the first element found (if any) that is not a child of the
element currently being read so that the calling code can continue
parsing the file using the upper-level element.

If the calling code doesn't need that element, it has to delete it
itself. However, the code must not simply rely on the `FoundElt`
pointer being not null as the `Read` function assigns temporary
results to that variable. Depending on the file content, that
temporary element may have already been deleted by the `Read`
function. When the calling code then simply deletes `FoundElt` itself,
this leads to a typical case of use-after-free.

Instead the calling code must only work with the returned `FoundElt`
pointer if the other returned value, `UpperEltFound`, trueish in the
C++ sense (if it isn't 0). Then and only then may the calling code
attempt to delete the object `FoundElt` points to.

This vulnerability allows arbitrary code execution via specially
crafted Matroska files. It was reported by Cisco TALOS on 2018-10-25
and is known as TALOS 2018-0694.
---
 NEWS.md                                    | 11 +++++++++++
 src/common/kax_file.cpp                    |  4 ++--
 src/mkvtoolnix-gui/info/element_reader.cpp |  3 ++-
 tests/results.txt                          |  1 +
 tests/test-662cisco_talos_2018_0694.rb     |  9 +++++++++
 5 files changed, 25 insertions(+), 3 deletions(-)
 create mode 100755 tests/test-662cisco_talos_2018_0694.rb

diff --git a/NEWS.md b/NEWS.md
index bb54a41da..05a08048e 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,3 +1,14 @@
+# Version ?
+
+## Bug fixes
+
+* mkvmerge, mkvinfo, mkvextract, mkvpropedit, MKVToolNix GUI's info tool &
+  chapter editor: fixed a case of memory being accessed after it had been
+  freed earlier. This can be triggered by specially crafted Matroska files and
+  lead to arbitrary code execution. The vulnerability was reported as Cisco
+  TALOS 2018-0694 on 2018-10-25.
+
+
 # Version 28.1.0 "Morning Child" 2018-10-23
 
 ## Bug fixes
diff --git a/src/common/kax_file.cpp b/src/common/kax_file.cpp
index 8a6aeb2de..1221b71a1 100644
--- a/src/common/kax_file.cpp
+++ b/src/common/kax_file.cpp
@@ -152,13 +152,13 @@ kax_file_c::read_one_element() {
   auto l2 = static_cast<EbmlElement *>(nullptr);
   try {
     l1->Read(*m_es.get(), EBML_INFO_CONTEXT(*callbacks), upper_lvl_el, l2, true);
-    if (!found_in(*l1, l2))
+    if (upper_lvl_el && !found_in(*l1, l2))
       delete l2;
 
   } catch (std::runtime_error &e) {
     mxdebug_if(m_debug_resync, boost::format("exception reading element data: %1%\n") % e.what());
     m_in.setFilePointer(l1->GetElementPosition() + 1);
-    if (!found_in(*l1, l2))
+    if (upper_lvl_el && !found_in(*l1, l2))
       delete l2;
     return {};
   }
diff --git a/src/mkvtoolnix-gui/info/element_reader.cpp b/src/mkvtoolnix-gui/info/element_reader.cpp
index af198eacf..aa34b5a59 100644
--- a/src/mkvtoolnix-gui/info/element_reader.cpp
+++ b/src/mkvtoolnix-gui/info/element_reader.cpp
@@ -46,7 +46,8 @@ ElementReader::run() {
 
     m_element.Read(es, EBML_INFO_CONTEXT(*callbacks), upper_lvl, upper_lvl_el, true);
 
-    delete upper_lvl_el;
+    if (upper_lvl)
+      delete upper_lvl_el;
 
   } catch (mtx::mm_io::exception &) {
   }
diff --git a/tests/results.txt b/tests/results.txt
index 6b9cf3cb8..f50761a61 100644
--- a/tests/results.txt
+++ b/tests/results.txt
@@ -507,3 +507,4 @@ T_658X_av1:20347e54bac4d1b0fe82d1f2f6c17c4d+8a967d44a1de7c00bf8be24156802f4f:pas
 T_659av1_timing_info_in_bitstream:b0b31fd8f8f569f7dcb7f29a3960050d-15361e7c74dfdbd59ffe96009b44a63f:passed:20181007-235434:0.016544501
 T_660propedit_replace_one_byte_with_ebmlvoid_all_surrounding_elements_coded_size_length_8:a16d7aa5d00d120d20c4c3c718e1b00b:passed:20181020-160502:0.0
 T_661av1_in_matroska_by_mkvmerge_28_0_0:66667a5dd1a2cafb3aa37ce41aedfbf7:passed:20181021-225056:0.008359871
+T_662cisco_talos_2018_0694:2bacc50e6e28fb2a92d2abf2583405f5-3292227553ffd194cd6c24cf07413fe2:passed:20181025-215511:0.024040919
diff --git a/tests/test-662cisco_talos_2018_0694.rb b/tests/test-662cisco_talos_2018_0694.rb
new file mode 100755
index 000000000..2883329d0
--- /dev/null
+++ b/tests/test-662cisco_talos_2018_0694.rb
@@ -0,0 +1,9 @@
+#!/usr/bin/ruby -w
+
+# T_662cisco_talos_2018_0694
+describe "mkvinfo / Cisco TALOS 2018-0694 use after free"
+
+file = "data/segfaults-assertions/cisco-talos-2018-0694.mkv"
+
+test_merge file
+test_info file, :args => "-v -v"