Fix OOB reads upon malformed Format Description and v2 Rows Events

mysql-test/suite/binlog/r/binlog_old_versions.result

@@ -13,6 +13,7 @@ SELECT COUNT(*) FROM t3;
 COUNT(*)
 17920
 DROP TABLE t1, t2, t3;
+FOUND 3 /[Uu]nknown [Ee]vent/ in invalid_row_v2_tag.sql
 ==== Read modern binlog (version 5.1.23) ====
 SELECT * FROM t1 ORDER BY a;
 a	b

mysql-test/suite/binlog/r/fdle_overflow.result

@@ -0,0 +1,11 @@
+SET @saved_dbug= @@GLOBAL.debug_dbug;
+SET @@GLOBAL.debug_dbug= '+d,truncate_fde_at_post_header_len';
+FLUSH BINARY LOGS;
+SHOW BINLOG EVENTS IN 'master-bin.000002';
+ERROR HY000: Error when executing command SHOW BINLOG EVENTS: Wrong offset or I/O error
+SET @@GLOBAL.debug_dbug= '+d,truncate_fde_common_header_len';
+FLUSH BINARY LOGS;
+SHOW BINLOG EVENTS IN 'master-bin.000003';
+ERROR HY000: Error when executing command SHOW BINLOG EVENTS: Wrong offset or I/O error
+SET @@GLOBAL.debug_dbug= @saved_dbug;
+RESET MASTER;

mysql-test/suite/binlog/t/binlog_old_versions.test

@@ -35,6 +35,18 @@ SELECT COUNT(*) FROM t3;
 # Reset.
 DROP TABLE t1, t2, t3;
 
+# MDEV-39485 Heap-buffer-overflow upon read in `Rows_log_event` constructor
+#
+# This binlog file contains a normal Format Description
+# Event followed by 3 malformed v2 Write Rows Events:
+# 1. A tag followed by no data
+# 2. An undersized tagged data followed by an unrecognized tag
+# 3. A tag followed by a length longer than the entire event
+--let SEARCH_PATTERN= [Uu]nknown [Ee]vent
+--let SEARCH_FILE= $MYSQLTEST_VARDIR/tmp/invalid_row_v2_tag.sql
+--exec $MYSQL_BINLOG --force-read --verbose suite/binlog/std_data/invalid_row_v2_tag.001 > $SEARCH_FILE
+--source include/search_pattern_in_file.inc
+
 
 --echo ==== Read modern binlog (version 5.1.23) ====
 

mysql-test/suite/binlog/t/fdle_overflow.test

@@ -0,0 +1,33 @@
+--source include/have_debug.inc
+--source include/have_binlog_format_mixed.inc # format-agnostic
+
+# OOB read on malformed `Format_description_log_event`
+#
+# Verify that Format Description Events with truncated contents fails gracefully
+# rather than underflow the post-header count and lead to buffer over-read.
+
+SET @saved_dbug= @@GLOBAL.debug_dbug;
+
+
+# MDEV-40366: `used_checksum_alg`
+
+SET @@GLOBAL.debug_dbug= '+d,truncate_fde_at_post_header_len';
+FLUSH BINARY LOGS;
+
+--let $binlog_file= query_get_value(SHOW BINLOG STATUS, File, 1)
+--error ER_ERROR_WHEN_EXECUTING_COMMAND
+--eval SHOW BINLOG EVENTS IN '$binlog_file'
+
+
+# MDEV-40365: `common_header_len` & `post_header_len`
+
+SET @@GLOBAL.debug_dbug= '+d,truncate_fde_common_header_len';
+FLUSH BINARY LOGS;
+
+--let $binlog_file= query_get_value(SHOW BINLOG STATUS, File, 1)
+--error ER_ERROR_WHEN_EXECUTING_COMMAND
+--eval SHOW BINLOG EVENTS IN '$binlog_file'
+
+# Clean-up
+SET @@GLOBAL.debug_dbug= @saved_dbug;
+RESET MASTER;

sql/log_event.cc

@@ -1022,7 +1022,7 @@ Log_event* Log_event::read_log_event(const uchar *buf, uint event_len,
                                      my_bool crc_check,
                                      my_bool print_errors)
 {
-  Log_event* ev;
+  Log_event* ev= nullptr;
   enum enum_binlog_checksum_alg alg;
   DBUG_ENTER("Log_event::read_log_event(char*,...)");
   DBUG_ASSERT(fdle != 0);
@@ -1041,14 +1041,21 @@ Log_event* Log_event::read_log_event(const uchar *buf, uint event_len,
   }
 
   uint event_type= buf[EVENT_TYPE_OFFSET];
+  switch (event_type) {
+  case FORMAT_DESCRIPTION_EVENT:
+    // If event is FD the descriptor is in it.
+    if (unlikely(get_checksum_alg(buf, event_len, &alg)))
+      goto exit;
+    break;
+  case START_EVENT_V3:
   // all following START events in the current file are without checksum
-  if (event_type == START_EVENT_V3)
     (const_cast< Format_description_log_event *>(fdle))->checksum_alg= BINLOG_CHECKSUM_ALG_OFF;
+    // fall-through
+  default:
   /*
     CRC verification by SQL and Show-Binlog-Events master side.
     The caller has to provide @fdle->checksum_alg to
     be the last seen FD's (A) descriptor.
-    If event is FD the descriptor is in it.
     Notice, FD of the binlog can be only in one instance and therefore
     Show-Binlog-Events executing master side thread needs just to know
     the only FD's (A) value -  whereas RL can contain more.
@@ -1063,11 +1070,9 @@ Log_event* Log_event::read_log_event(const uchar *buf, uint event_len,
 
     Notice, a pre-checksum FD version forces alg := BINLOG_CHECKSUM_ALG_UNDEF.
   */
-  alg= (event_type != FORMAT_DESCRIPTION_EVENT) ?
-    fdle->checksum_alg : get_checksum_alg(buf, event_len);
+    alg= fdle->checksum_alg;
   // Emulate the corruption during reading an event
   DBUG_EXECUTE_IF("corrupt_read_log_event_char",
-    if (event_type != FORMAT_DESCRIPTION_EVENT)
     {
       uchar *debug_event_buf_c= const_cast<uchar*>(buf);
       int debug_cor_pos= rand() % (event_len - BINLOG_CHECKSUM_LEN);
@@ -1076,6 +1081,7 @@ Log_event* Log_event::read_log_event(const uchar *buf, uint event_len,
       DBUG_SET("-d,corrupt_read_log_event_char");
     }
   );                                                 
+  }
   if (crc_check && event_checksum_test(const_cast<uchar*>(buf), event_len, alg))
   {
 #ifdef MYSQL_CLIENT
@@ -2269,33 +2275,39 @@ Format_description_log_event(const uchar *buf, uint event_len,
    common_header_len(0), post_header_len(NULL), event_type_permutation(0)
 {
   DBUG_ENTER("Format_description_log_event::Format_description_log_event(char*,...)");
-  if (!Start_log_event_v3::is_valid())
+  if (unlikely(
+      event_len < LOG_EVENT_MINIMAL_HEADER_LEN + ST_POST_HEADER_LEN_OFFSET ||
+      !Start_log_event_v3::is_valid()))
     DBUG_VOID_RETURN; /* sanity check */
   buf+= LOG_EVENT_MINIMAL_HEADER_LEN;
-  if ((common_header_len=buf[ST_COMMON_HEADER_LEN_OFFSET]) < OLD_HEADER_LEN)
+  if (unlikely(
+      (common_header_len=buf[ST_COMMON_HEADER_LEN_OFFSET]) < OLD_HEADER_LEN))
     DBUG_VOID_RETURN; /* sanity check */
   number_of_event_types=
-    event_len - (LOG_EVENT_MINIMAL_HEADER_LEN + ST_COMMON_HEADER_LEN_OFFSET + 1);
+    event_len - (LOG_EVENT_MINIMAL_HEADER_LEN + ST_POST_HEADER_LEN_OFFSET);
   DBUG_PRINT("info", ("common_header_len=%d number_of_event_types=%d",
                       common_header_len, number_of_event_types));
   /* If alloc fails, we'll detect it in is_valid() */
 
-  post_header_len= (uint8*) my_memdup(PSI_INSTRUMENT_ME,
-                                      buf+ST_COMMON_HEADER_LEN_OFFSET+1,
-                                      number_of_event_types*
-                                      sizeof(*post_header_len),
-                                      MYF(0));
   calc_server_version_split();
+  buf+= ST_POST_HEADER_LEN_OFFSET;
   if (!is_version_before_checksum(&server_version_split))
   {
-    /* the last bytes are the checksum alg desc and value (or value's room) */
+    /* the last bytes are the checksum alg desc */
+    if (unlikely(number_of_event_types < BINLOG_CHECKSUM_ALG_DESC_LEN))
+      DBUG_VOID_RETURN; /* sanity check: But there are no last bytes. */
     number_of_event_types -= BINLOG_CHECKSUM_ALG_DESC_LEN;
-    checksum_alg= (enum_binlog_checksum_alg)post_header_len[number_of_event_types];
+    checksum_alg= (enum_binlog_checksum_alg)buf[number_of_event_types];
   }
   else
   {
     checksum_alg= BINLOG_CHECKSUM_ALG_UNDEF;
   }
+  post_header_len= (uint8*) my_memdup(PSI_INSTRUMENT_ME,
+                                      buf,
+                                      number_of_event_types*
+                                      sizeof(*post_header_len),
+                                      MYF(0));
   deduct_options_written_to_bin_log();
   reset_crypto();
 
@@ -2408,35 +2420,50 @@ Format_description_log_event::is_version_before_checksum(const master_version_sp
 }
 
 /**
-   @param buf buffer holding serialized FD event
-   @param len netto (possible checksum is stripped off) length of the event buf
-
-   @return  the version-safe checksum alg descriptor where zero
+   @param buf buffer holding serialized FD event including the 4-byte checksum
+   @param len length of the event buf
+   @param alg output the version-safe checksum alg descriptor where zero
             designates no checksum, 255 - the orginator is
             checksum-unaware (effectively no checksum) and the actuall
             [1-254] range alg descriptor.
+   @return whether this is an invalid FD event
 */
-enum enum_binlog_checksum_alg get_checksum_alg(const uchar *buf, ulong len)
+bool get_checksum_alg(const uchar *buf, ulong len,
+                      enum enum_binlog_checksum_alg *alg)
 {
-  enum enum_binlog_checksum_alg ret;
+  constexpr ptrdiff_t POST_HEADER_LEN_OFFSET=
+    LOG_EVENT_MINIMAL_HEADER_LEN + ST_POST_HEADER_LEN_OFFSET;
   char version[ST_SERVER_VER_LEN];
 
   DBUG_ENTER("get_checksum_alg");
   DBUG_ASSERT(buf[EVENT_TYPE_OFFSET] == FORMAT_DESCRIPTION_EVENT);
 
+  if (unlikely(len < POST_HEADER_LEN_OFFSET))
+    DBUG_RETURN(true);
   memcpy(version,
          buf + LOG_EVENT_MINIMAL_HEADER_LEN + ST_SERVER_VER_OFFSET,
          ST_SERVER_VER_LEN);
   version[ST_SERVER_VER_LEN - 1]= 0;
 
   Format_description_log_event::master_version_split version_split(version);
-  ret= Format_description_log_event::is_version_before_checksum(&version_split)
-    ? BINLOG_CHECKSUM_ALG_UNDEF
-    : (enum_binlog_checksum_alg)buf[len - BINLOG_CHECKSUM_LEN - BINLOG_CHECKSUM_ALG_DESC_LEN];
-  DBUG_ASSERT(ret == BINLOG_CHECKSUM_ALG_OFF ||
-              ret == BINLOG_CHECKSUM_ALG_UNDEF ||
-              ret == BINLOG_CHECKSUM_ALG_CRC32);
-  DBUG_RETURN(ret);
+  if (Format_description_log_event::is_version_before_checksum(&version_split))
+    *alg= BINLOG_CHECKSUM_ALG_UNDEF;
+  else
+  {
+    /*
+      len >= POST_HEADER_LEN_OFFSET >
+      BINLOG_CHECKSUM_LEN + BINLOG_CHECKSUM_ALG_DESC_LEN
+    */
+    ulong checksum_alg_offset=
+      len - BINLOG_CHECKSUM_LEN - BINLOG_CHECKSUM_ALG_DESC_LEN;
+    if (unlikely(checksum_alg_offset < POST_HEADER_LEN_OFFSET))
+      DBUG_RETURN(true);
+    *alg= static_cast<enum_binlog_checksum_alg>(buf[checksum_alg_offset]);
+  }
+  DBUG_ASSERT(*alg == BINLOG_CHECKSUM_ALG_OFF ||
+              *alg == BINLOG_CHECKSUM_ALG_UNDEF ||
+              *alg == BINLOG_CHECKSUM_ALG_CRC32);
+  DBUG_RETURN(false);
 }
 
 Start_encryption_log_event::
@@ -3396,9 +3423,17 @@ Rows_log_event::Rows_log_event(const uchar *buf, uint event_len,
       case RW_V_EXTRAINFO_TAG:
       {
         /* Have an 'extra info' section, read it in */
-        assert((end - pos) >= EXTRA_ROW_INFO_HDR_BYTES);
+        if (unlikely((end - pos) <= EXTRA_ROW_INFO_LEN_OFFSET))
+        {
+          m_cols.bitmap= 0;
+          DBUG_VOID_RETURN;
+        }
         uint8 infoLen= pos[EXTRA_ROW_INFO_LEN_OFFSET];
-        assert((end - pos) >= infoLen);
+        if (unlikely(infoLen < EXTRA_ROW_INFO_HDR_BYTES || (end-pos) < infoLen))
+        {
+          m_cols.bitmap= 0;
+          DBUG_VOID_RETURN;
+        }
         /* Just store/use the first tag of this type, skip others */
         if (likely(!m_extra_row_data))
         {

sql/log_event.h

@@ -204,7 +204,7 @@ class String;
 #define STOP_HEADER_LEN      0
 #define LOAD_HEADER_LEN      (4 + 4 + 4 + 1 +1 + 4)
 #define SLAVE_HEADER_LEN     0
-#define START_V3_HEADER_LEN     (2 + ST_SERVER_VER_LEN + 4)
+#define START_V3_HEADER_LEN  ST_COMMON_HEADER_LEN_OFFSET
 #define ROTATE_HEADER_LEN    8 // this is FROZEN (the Rotate post-header is frozen)
 #define INTVAR_HEADER_LEN      0
 #define CREATE_FILE_HEADER_LEN 4
@@ -280,6 +280,7 @@ class String;
 #define ST_SERVER_VER_OFFSET  2
 #define ST_CREATED_OFFSET     (ST_SERVER_VER_OFFSET + ST_SERVER_VER_LEN)
 #define ST_COMMON_HEADER_LEN_OFFSET (ST_CREATED_OFFSET + 4)
+#define ST_POST_HEADER_LEN_OFFSET (ST_COMMON_HEADER_LEN_OFFSET + 1)
 
 /* slave event post-header (this event is never written) */
 
@@ -5868,7 +5869,8 @@ bool slave_execute_deferred_events(THD *thd);
 bool event_that_should_be_ignored(const uchar *buf);
 bool event_checksum_test(uchar *buf, ulong event_len,
                          enum_binlog_checksum_alg alg);
-enum enum_binlog_checksum_alg get_checksum_alg(const uchar *buf, ulong len);
+bool get_checksum_alg(const uchar *buf, ulong len,
+                      enum enum_binlog_checksum_alg *alg);
 extern TYPELIB binlog_checksum_typelib;
 #ifdef WITH_WSREP
 enum Log_event_type wsrep_peak_event(rpl_group_info *rgi, ulonglong* event_size);

sql/log_event_server.cc

@@ -2372,9 +2372,12 @@ bool Format_description_log_event::write()
     We don't call Start_log_event_v3::write() because this would make 2
     my_b_safe_write().
   */
-  uchar buff[START_V3_HEADER_LEN+1];
-  size_t rec_size= sizeof(buff) + BINLOG_CHECKSUM_ALG_DESC_LEN +
-                   number_of_event_types;
+  uchar buff[ST_POST_HEADER_LEN_OFFSET];
+  const size_t buff_size= DBUG_EVALUATE_IF("truncate_fde_common_header_len",
+    ST_COMMON_HEADER_LEN_OFFSET, sizeof(buff));
+  size_t rec_size= buff_size +
+    DBUG_EVALUATE_IF("truncate_fde_at_post_header_len", 0,
+      BINLOG_CHECKSUM_ALG_DESC_LEN + number_of_event_types);
   int2store(buff + ST_BINLOG_VER_OFFSET,binlog_version);
   memcpy((char*) buff + ST_SERVER_VER_OFFSET,server_version,ST_SERVER_VER_LEN);
   if (!dont_set_created)
@@ -2413,10 +2416,11 @@ bool Format_description_log_event::write()
     checksum_alg= BINLOG_CHECKSUM_ALG_CRC32;  // Forcing (V) room to fill anyway
   }
   ret= write_header(rec_size) ||
-       write_data(buff, sizeof(buff)) ||
+    write_data(buff, buff_size) ||
+    DBUG_EVALUATE_IF("truncate_fde_at_post_header_len", false,
        write_data(post_header_len, number_of_event_types) ||
        write_data(&checksum_byte, sizeof(checksum_byte)) ||
-       write_footer();
+       write_footer());
   if (no_checksum)
     checksum_alg= BINLOG_CHECKSUM_ALG_OFF;
   return ret;

sql/slave.cc

@@ -6520,7 +6520,12 @@ static int queue_event(Master_info* mi, const uchar *buf, ulong event_len)
   */
   if (buf[EVENT_TYPE_OFFSET] == FORMAT_DESCRIPTION_EVENT)
   {
-    checksum_alg= get_checksum_alg(buf, event_len);
+    if (unlikely(get_checksum_alg(buf, event_len, &checksum_alg)))
+    {
+      error= ER_SLAVE_RELAY_LOG_WRITE_FAILURE;
+      unlock_data_lock= FALSE;
+      goto err;
+    }
   }
   else if (buf[EVENT_TYPE_OFFSET] == START_EVENT_V3)
   {

sql/sql_repl.cc

@@ -1511,10 +1511,10 @@ gtid_state_from_pos(const char *name, uint32 offset,
         goto end;
       }
 
-      current_checksum_alg= get_checksum_alg((uchar*) packet.ptr(),
-                                             packet.length());
       found_format_description_event= true;
-      if (unlikely(!(tmp= new Format_description_log_event((uchar*) packet.ptr(),
+      if (unlikely(get_checksum_alg((uchar*) packet.ptr(), packet.length(),
+                                    &current_checksum_alg) ||
+                   !(tmp= new Format_description_log_event((uchar*)packet.ptr(),
                                                            packet.length(),
                                                            fdev))))
       {
@@ -2248,6 +2248,8 @@ static int init_binlog_sender(binlog_send_info *info,
 static int send_format_descriptor_event(binlog_send_info *info, IO_CACHE *log,
                                         LOG_INFO *linfo, my_off_t start_pos)
 {
+  static constexpr const char* CORRUPT_FDE=
+    "Corrupt Format_description event found or out-of-memory";
   int error;
   ulong ev_offset;
   THD *thd= info->thd;
@@ -2314,9 +2316,14 @@ static int send_format_descriptor_event(binlog_send_info *info, IO_CACHE *log,
     DBUG_RETURN(1);
   }
 
-  info->current_checksum_alg= get_checksum_alg((uchar*) packet->ptr() +
-                                               ev_offset,
-                                               packet->length() - ev_offset);
+  if (unlikely(get_checksum_alg((uchar*) packet->ptr() + ev_offset,
+                                packet->length() - ev_offset,
+                                &(info->current_checksum_alg))))
+  {
+    info->error= ER_MASTER_FATAL_ERROR_READING_BINLOG;
+    info->errmsg= CORRUPT_FDE;
+    DBUG_RETURN(1);
+  }
 
   DBUG_ASSERT(info->current_checksum_alg == BINLOG_CHECKSUM_ALG_OFF ||
               info->current_checksum_alg == BINLOG_CHECKSUM_ALG_UNDEF ||
@@ -2344,8 +2351,7 @@ static int send_format_descriptor_event(binlog_send_info *info, IO_CACHE *log,
                                               ev_len, info->fdev)))
   {
     info->error= ER_MASTER_FATAL_ERROR_READING_BINLOG;
-    info->errmsg= "Corrupt Format_description event found "
-        "or out-of-memory";
+    info->errmsg= CORRUPT_FDE;
     DBUG_RETURN(1);
   }
   delete info->fdev;