Bug #52748 Semi-Sync ACK packet isn't check for length

Description:
Sending a crafted ACK packet in a semi-sync replication triggers a segfault.

int ReplSemiSyncMaster::readSlaveReply(NET *net, uint32 server_id,
                                       const char *event_buf)
{
  ...
  char     log_file_name[FN_REFLEN];
  ...

  strcpy(log_file_name, (const char*)packet + REPLY_BINLOG_NAME_OFFSET);

How to repeat:
Send a binlog filename in the ACK packet that is > FN_LENGTH:

  * 4 byte mysql-packet-header
  * 0xef as semi-sync magic
  * 8 byte log-pos
  * rest is log-filename

like:

  [0000]  09 04 00 00 ef 47 03 00 00 00 00 00 00 61 61 61  .....G.......aaa
  [0010]  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
  [0020]  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
  ...
  [0400]  61 61 61 61 61 61 61 61 61 61 61 61 61           aaaaaaaaaaaaa

leads to as stack-overrun:

Thread 20 (process 57663):
#0  0x00007fff85274886 in __kill ()
#1  0x00007fff85314e4f in __abort ()
#2  0x00007fff852f952c in __stack_chk_fail ()
#3  0x0000000100a7da49 in ReplSemiSyncMaster::readSlaveReply ()
#4  0x6161616161616161 in ?? ()

The length of the log_file_name is not checked:

int ReplSemiSyncMaster::readSlaveReply(NET *net, uint32 server_id,
                                       const char *event_buf)
{
  ...
  char     log_file_name[FN_REFLEN];
  ...

  strcpy(log_file_name, (const char*)packet + REPLY_BINLOG_NAME_OFFSET);

Suggested fix:
Check the length of the binlog filename that it is less than FN_LENGTH.

Thank you for the report.

> Send a binlog filename in the ACK packet that is > FN_LENGTH:

How can I do this? MySQL server refuses to start with such file name for binary log.

I used the modified version of MySQL Proxy's replicant plugin to trigger it, but you can also just change plugins/semisync/semisync_slave.cc:

int ReplSemiSyncSlave::slaveReply(...) {
  ...
  uchar reply_buffer[REPLY_MAGIC_NUM_LEN
                     + REPLY_BINLOG_POS_LEN
                     + REPLY_BINLOG_NAME_LEN + 255]; /* <--- to-crash: get some more room */
  int  reply_res, name_len = strlen(binlog_filename);

  function_enter(kWho);

  /* Prepare the buffer of the reply. */
  reply_buffer[REPLY_MAGIC_NUM_OFFSET] = kPacketMagicNum;
  int8store(reply_buffer + REPLY_BINLOG_POS_OFFSET, binlog_filepos);
  memcpy(reply_buffer + REPLY_BINLOG_NAME_OFFSET,
         binlog_filename,
         name_len); /** to-crash: no trailing 0 */

  /* to-crash: and now append a 255 char long string to make it over run the static buffer  on the master */
  memset(reply_buffer + EPLY_BINLOG_NAME_OFFSET + name_len, 'A', 255);

Thank you for the feedback.

I still can not reproduce it in mysql-next-mr tree (5.6.99). Could you please try this tree in your environment and inform us if problem still exists?

will check the length and use strncpy to make the code safer.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/108969

3057 He Zhenxing	2010-05-24
      BUG#52748 Semi-Sync ACK packet isn't check for length
      
      Check the length and use strncpy to make the code safer.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/108970

3057 He Zhenxing	2010-05-24
      BUG#52748 Semi-Sync ACK packet isn't check for length
      
      Check the length and use strncpy to make the code safer.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/109588

3057 He Zhenxing	2010-05-31
      BUG#52748 Semi-Sync ACK packet isn't check for length
      
      Check the length and use strncpy to make the code safer.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/109659

3132 He Zhenxing	2010-06-01
      BUG#52748 Semi-Sync ACK packet isn't check for length
      
      Check the length and use strncpy to make the code safer.
     @ plugin/semisync/semisync_master.cc
        replace strcpy with strncpy to make the code safer

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/109661

3077 He Zhenxing	2010-06-01 [merge]
      BUG#52748 Semi-Sync ACK packet isn't check for length
      
      Check the length and use strncpy to make the code safer.

pushed to 5.1-rep-semisync and trunk-bugfixing

pushed to next-mr-bugfixing

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/109943

3133 He Zhenxing	2010-06-02
      Post fix for bug#52748

Pushed into 5.5.5-m3 (revid:alik@sun.com-20100615080459-smuswd9ooeywcxuc) (version source revid:marko.makela@oracle.com-20100601134335-ccthwwru23kn09qw) (merge vers: 5.1.48) (pib:16)

Pushed into mysql-next-mr (revid:alik@sun.com-20100615080558-cw01bzdqr1bdmmec) (version source revid:marko.makela@oracle.com-20100601134335-ccthwwru23kn09qw) (pib:16)

Noted in 5.5.5 changelog.

ACK packets in semisynchronous replication were not checked
for length and malformed packets could cause a server crash.