Base 64 decoder: fix invalid memory access

Fixes #1222.
VamuveTV · Jun 1, 2015 · 5df8f07 · 5df8f07
1 parent 9be0e47
commit 5df8f07
Show file tree

Hide file tree

Showing 7 changed files with 28 additions and 27 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -1,3 +1,8 @@
+2015-06-01  Moritz Bunkus  <moritz@bunkus.org>
+
+        * mkvmerge, mkvpropedit: bug fix: fixed an invalid memory access
+        leading to a crash in the Base 64 decoder. Fixes #1222.
+
 2015-05-31  Moritz Bunkus  <moritz@bunkus.org>
 
         * MKVToolNix GUI: bug fix: fixed progress parsing for interface

diff --git a/src/common/base64.cpp b/src/common/base64.cpp
@@ -96,12 +96,12 @@ base64_encode(const unsigned char *src,
   return out;
 }
 
-int
-base64_decode(const std::string &src,
-              unsigned char *dst) {
-  unsigned int pos     = 0;
-  unsigned int dst_pos = 0;
-  unsigned int pad     = 0;
+std::string
+base64_decode(std::string const &src) {
+  auto pos = 0u;
+  auto pad = 0u;
+  auto dst = std::string{};
+
   while (pos < src.size()) {
     unsigned char in[4];
     unsigned int in_pos = 0;
@@ -118,7 +118,7 @@ base64_decode(const std::string &src,
         pad++;
 
       else if (!isblanktab(c) && !iscr(c))
-        return -1;
+        throw mtx::base64::invalid_data_x{};
     }
 
     unsigned int values_idx;
@@ -134,7 +134,7 @@ base64_decode(const std::string &src,
         :                                                        255;
 
       if (255 == values[values_idx])
-        throw mtx::base64::invalid_data_x();
+        throw mtx::base64::invalid_data_x{};
     }
 
     unsigned char mid[6];
@@ -145,20 +145,16 @@ base64_decode(const std::string &src,
     mid[4] = values[2] << 6;
     mid[5] = values[3];
 
-    dst[dst_pos] = mid[0] | mid[1];
-    dst_pos++;
+    dst += mid[0] | mid[1];
     if (1 >= pad) {
-      dst[dst_pos] = mid[2] | mid[3];
-      dst_pos++;
-      if (0 == pad) {
-        dst[dst_pos] = mid[4] | mid[5];
-        dst_pos++;
-      }
+      dst += mid[2] | mid[3];
+      if (0 == pad)
+        dst += mid[4] | mid[5];
     }
 
     if (0 != pad)
-      return dst_pos;
+      break;
   }
 
-  return dst_pos;
+  return dst;
 }
diff --git a/src/common/base64.h b/src/common/base64.h
@@ -34,6 +34,6 @@ namespace mtx {
 }
 
 std::string base64_encode(const unsigned char *src, int src_len, bool line_breaks = false, int max_line_len = 72);
-int base64_decode(const std::string &src, unsigned char *dst);
+std::string base64_decode(std::string const &src);
 
 #endif // MTX_COMMON_BASE64_H
diff --git a/src/common/hacks.cpp b/src/common/hacks.cpp
@@ -72,9 +72,7 @@ engage_hacks(const std::string &hacks) {
         "IE9oIGhvbmV5LCB0aGF0J3Mgc28gc3dlZXQhCiAgIC8tLS0tLS0tXC8gICBPZiB"
         "jb3Vyc2UgSSdsbCBtYXJyeSB5b3UhCiAgLyB8ICAgICB8fAogKiAgfHwtLS0tfH"
         "wKICAgIF5eICAgIF5eCg==";
-      char correction[200];
-      memset(correction, 0, 200);
-      base64_decode(initial, (unsigned char *)correction);
+      auto correction = base64_decode(initial);
       mxinfo(correction);
       mxexit();
     }

diff --git a/src/common/xml/ebml_converter.cpp b/src/common/xml/ebml_converter.cpp
@@ -275,11 +275,7 @@ ebml_converter_c::parse_binary(parser_context_t &ctx) {
 
   } else if (format == "base64") {
     try {
-      std::string destination(' ', content.size());
-      auto dest_len = base64_decode(content, reinterpret_cast<unsigned char *>(&destination[0]));
-      if (-1 == dest_len)
-        throw malformed_data_x{ ctx.name, ctx.node.offset_debug(), Y("Invalid data for Base64 encoding found.") };
-      content = destination.substr(0, dest_len);
+      content = base64_decode(content);
 
     } catch (mtx::base64::exception &) {
       throw malformed_data_x{ ctx.name, ctx.node.offset_debug(), Y("Invalid data for Base64 encoding found.") };

diff --git a/tests/results.txt b/tests/results.txt
@@ -342,3 +342,4 @@ T_493truehd_ac3_setting_track_properties_mpeg_ts:8256eca144895021b5cf265efaf7bf2
 T_494dont_abort_with_aac_error_proection_specific_config:f18d6c1a0cd91fcb216fac7ee68bd5f1:passed:20150416-092327:0.59008572
 T_495default_durataion_and_sync:b360d52e56d8d291a7dc24ecd2b63c18-0a38dc656269f588fc4228d50e930d12-8f3d5eeacee5a77075eed0e7ae0d882a:passed:20150417-213623:0.894261521
 T_496segment_size_0:9fd6d5e57b47693a483f9aad112a6a15-a78f5ddc1a9fb900b3797ced8752cb92:passed:20150530-180226:0.051304978
+T_497crash_in_base64_decoder:f6526cfaaef01627c52ee2ba25f03255:passed:20150601-192215:0.02848152
diff --git a/tests/test-497crash_in_base64_decoder.rb b/tests/test-497crash_in_base64_decoder.rb
@@ -0,0 +1,5 @@
+#!/usr/bin/ruby -w
+
+# T_497crash_in_base64_decoder
+describe "mkvmerge / crash in the Base 64 decoder"
+test_merge "--tags 0:data/text/tags-crash-invalid-memory-access.xml data/subtitles/srt/ven.srt"