Bug #87200 Suspicious UAF at sql/sql_partition.cc
Submitted: 26 Jul 2017 8:35 Modified: 27 Jul 2017 14:03
Reporter: alex chen Email Updates:
Status: Verified Impact on me:
None 
Category:MySQL Server: InnoDB storage engine Severity:S3 (Non-critical)
Version:5.7 OS:Any
Assigned to: CPU Architecture:Any

[26 Jul 2017 8:35] alex chen
Description:
Hi all,

Our code scanner has reported a use after free at the end of function `generate_partition_syntax`, see steps I commented bellow, would any one have a look and see if there is a real issue?

// ===============================
  if (unlikely(mysql_file_read(fptr, (uchar*)buf, *buf_length, MYF(MY_FNABP))))
  {
    if (!use_sql_alloc)
      my_free(buf);      // <== Step 1: this line frees buf, but haven't assign a NULL to it
    else
      buf= NULL;
  }
  else
    buf[*buf_length]= 0;

close_file:
  if (buf == NULL)           // Step 2: so this line won't know there was an error occurred.
  {
    my_error(ER_INTERNAL_ERROR, MYF(0), "Failed to generate partition syntax");
  }
  mysql_file_close(fptr, MYF(0));
  DBUG_RETURN(buf);         // Step 3: a freed buf is returned to caller
}
// ===============================

Regards,

SourceBrella Inc. 
Alex

How to repeat:
only if `mysql_file_read` in the description block returns error

Suggested fix:
assign NULL to buf variable after some error occurred.
[26 Jul 2017 18:24] Sinisa Milivojevic
Hi!

This is not a bug, because as you can see in the code of my_free(), pointer is set to NULL upon being freed, there is a check before it, including the magic number, so the code is quite safe.
[27 Jul 2017 2:46] alex chen
Hi Sinisa,

to be sure, you are referring the implementation of `my_free` bellow here? I could not find a NULL assignment in this function, and even if there is one, it can't affect the value of `ptr` copied from the caller (they hold a same value, but they are not a same variable) , might still need some help from you.

the `my_free` implementation that I was looking at,
https://github.com/mysql/mysql-server/blob/71f48ab393bce80a59e5a2e498cd1f46f6b43f9a/mysys/...

// =========================================
void my_free(void *ptr)
{
  my_memory_header *mh;

  if (ptr == NULL)
    return;

  mh= USER_TO_HEADER(ptr);
  DBUG_ASSERT(mh->m_magic == MAGIC);
  PSI_MEMORY_CALL(memory_free)(mh->m_key, mh->m_size, mh->m_owner);
  /* Catch double free */
  mh->m_magic= 0xDEAD;
  MEM_FREELIKE_BLOCK(ptr, 0);
  my_raw_free(mh);
}
// =========================================
[27 Jul 2017 2:51] Tom Shi
This seems a true problem. At least, I found two implementations of the function "my_free()", one of which calls "my_raw_free" directly. "my_raw_free" calls "free()" directly. No "null assignment" exists.

The my_free() implementation is here: https://github.com/mysql/mysql-server/blob/5.7/mysys/my_malloc.c#L157
[27 Jul 2017 13:29] Sinisa Milivojevic
Upon further analysis, this turns out to be a bug, as pointer is not assigned to NULL in one place.

Verified as reported.
[27 Jul 2017 13:33] Sinisa Milivojevic
Thank you for your contribution.
[27 Jul 2017 13:47] alex chen
Hi @ Sinisa

Sure, thank you for your concern
[27 Jul 2017 14:03] alex chen
by the way, is a bug like this could be exploitable ?
[27 Jul 2017 14:10] Sinisa Milivojevic
No, it is not exploitable as the path is quite improbable to force ...