Bug#12563279: REGRESSION IN HANDLING PRE-4.1 AUTHENTICATION PACKET

The problem is that clients implementing the 4.0 version of the
protocol (that is, mysql-4.0) do not null terminate a string
at the end of the authentication packet. These clients denote
the end of the string with the end of the packet.

Although this goes against the documented (see MySQL Internals
ClientServer Protocol wiki) description of the protocol, these
old clients still need to be supported.

The solution is to support the documented and actual behavior
of the clients. If a client is using the pre-4.1 version of
the protocol, the end of a string in the authentication packet
can either be denoted with a null character or by the end of
the packet. This restores backwards compatibility with old
clients implementing either the documented or actual behavior.

sql/password.c

@@ -204,21 +204,16 @@ void scramble_323(char *to, const char *message, const char *password)
 }
 
 
-/*
-    Check scrambled message
-    Used in pre 4.1 password handling
-  SYNOPSIS
-    check_scramble_323()
-    scrambled  scrambled message to check.
-    message    original random message which was used for scrambling; must
-               be exactly SCRAMBLED_LENGTH_323 bytes long and
-               NULL-terminated.
-    hash_pass  password which should be used for scrambling
-    All params are IN.
+/**
+  Check scrambled message. Used in pre 4.1 password handling.
 
-  RETURN VALUE
-    0 - password correct
-   !0 - password invalid
+  @param scrambled  Scrambled message to check.
+  @param message    Original random message which was used for scrambling.
+  @param hash_pass  Password which should be used for scrambling.
+
+  @remark scrambled and message must be SCRAMBLED_LENGTH_323 bytes long.
+
+  @return FALSE if password is correct, TRUE otherwise.
 */
 
 my_bool
@@ -227,9 +222,16 @@ check_scramble_323(const char *scrambled, const char *message,
 {
   struct rand_struct rand_st;
   ulong hash_message[2];
-  char buff[16],*to,extra;                      /* Big enough for check */
+  /* Big enough for checks. */
+  char buff[16], scrambled_buff[SCRAMBLE_LENGTH_323 + 1];
+  char *to, extra;
   const char *pos;
 
+  /* Ensure that the scrambled message is null-terminated. */
+  memcpy(scrambled_buff, scrambled, SCRAMBLE_LENGTH_323);
+  scrambled_buff[SCRAMBLE_LENGTH_323]= '\0';
+  scrambled= scrambled_buff;
+
   hash_password(hash_message, message, SCRAMBLE_LENGTH_323);
   randominit(&rand_st,hash_pass[0] ^ hash_message[0],
              hash_pass[1] ^ hash_message[1]);

sql/sql_connect.cc

@@ -653,14 +653,21 @@ bool init_new_connection_handler_thread()
 }
 
 #ifndef EMBEDDED_LIBRARY
+
+/** Get a string according to the protocol of the underlying buffer. */
+typedef char * (*get_proto_string_func_t) (char **, size_t *, size_t *);
+
 /**
-  Get a null character terminated string from a user-supplied buffer.
+  Get a string formatted according to the 4.1 version of the MySQL protocol.
 
-  @param buffer[in, out]    Pointer to the buffer to be scanned.
+  @param buffer[in, out]    Pointer to the user-supplied buffer to be scanned.
   @param max_bytes_available[in, out]  Limit the bytes to scan.
   @param string_length[out] The number of characters scanned not including
                             the null character.
 
+  @remark Strings are always null character terminated in this version of the
+          protocol.
+
   @remark The string_length does not include the terminating null character.
           However, after the call, the buffer is increased by string_length+1
           bytes, beyond the null character if there still available bytes to
@@ -671,9 +678,9 @@ bool init_new_connection_handler_thread()
 */
 
 static
-char *get_null_terminated_string(char **buffer,
-                                 size_t *max_bytes_available,
-                                 size_t *string_length)
+char *get_41_protocol_string(char **buffer,
+                             size_t *max_bytes_available,
+                             size_t *string_length)
 {
   char *str= (char *)memchr(*buffer, '\0', *max_bytes_available);
 
@@ -683,7 +690,60 @@ char *get_null_terminated_string(char **buffer,
   *string_length= (size_t)(str - *buffer);
   *max_bytes_available-= *string_length + 1;
   str= *buffer;
-  *buffer += *string_length + 1;  
+  *buffer += *string_length + 1;
+
+  return str;
+}
+
+
+/**
+  Get a string formatted according to the 4.0 version of the MySQL protocol.
+
+  @param buffer[in, out]    Pointer to the user-supplied buffer to be scanned.
+  @param max_bytes_available[in, out]  Limit the bytes to scan.
+  @param string_length[out] The number of characters scanned not including
+                            the null character.
+
+  @remark If there are not enough bytes left after the current position of
+          the buffer to satisfy the current string, the string is considered
+          to be empty and a pointer to empty_c_string is returned.
+
+  @remark A string at the end of the packet is not null terminated.
+
+  @return Pointer to beginning of the string scanned, or a pointer to a empty
+          string.
+*/
+static
+char *get_40_protocol_string(char **buffer,
+                             size_t *max_bytes_available,
+                             size_t *string_length)
+{
+  char *str;
+  size_t len;
+
+  /* No bytes to scan left, treat string as empty. */
+  if ((*max_bytes_available) == 0)
+  {
+    *string_length= 0;
+    return empty_c_string;
+  }
+
+  str= (char *) memchr(*buffer, '\0', *max_bytes_available);
+
+  /*
+    If the string was not null terminated by the client,
+    the remainder of the packet is the string. Otherwise,
+    advance the buffer past the end of the null terminated
+    string.
+  */
+  if (str == NULL)
+    len= *string_length= *max_bytes_available;
+  else
+    len= (*string_length= (size_t)(str - *buffer)) + 1;
+
+  str= *buffer;
+  *buffer+= len;
+  *max_bytes_available-= len;
 
   return str;
 }
@@ -695,7 +755,7 @@ char *get_null_terminated_string(char **buffer,
   @param buffer[in, out] The buffer to scan; updates position after scan.
   @param max_bytes_available[in, out] Limit the number of bytes to scan
   @param string_length[out] Number of characters scanned
-  
+
   @remark In case the length is zero, then the total size of the string is
     considered to be 1 byte; the size byte.
 
@@ -854,7 +914,7 @@ static int check_connection(THD *thd)
       part at the end of packet.
     */
     end= strmake(end, thd->scramble, SCRAMBLE_LENGTH_323) + 1;
-   
+
     int2store(end, server_capabilites);
     /* write server characteristics: up to 16 bytes allowed */
     end[2]=(char) default_charset_info->number;
@@ -952,7 +1012,20 @@ static int check_connection(THD *thd)
   if ((thd->client_capabilities & CLIENT_TRANSACTIONS) &&
       opt_using_transactions)
     net->return_status= &thd->server_status;
- 
+
+  /*
+    The 4.0 and 4.1 versions of the protocol differ on how strings
+    are terminated. In the 4.0 version, if a string is at the end
+    of the packet, the string is not null terminated. Do not assume
+    that the returned string is always null terminated.
+  */
+  get_proto_string_func_t get_string;
+
+  if (thd->client_capabilities & CLIENT_PROTOCOL_41)
+    get_string= get_41_protocol_string;
+  else
+    get_string= get_40_protocol_string;
+
   /*
     In order to safely scan a head for '\0' string terminators
     we must keep track of how many bytes remain in the allocated
@@ -961,8 +1034,7 @@ static int check_connection(THD *thd)
   size_t bytes_remaining_in_packet= pkt_len - (end - (char *)net->read_pos);
 
   size_t user_len;
-  char *user= get_null_terminated_string(&end, &bytes_remaining_in_packet,
-                                         &user_len);
+  char *user= get_string(&end, &bytes_remaining_in_packet, &user_len);
   if (user == NULL)
   {
     inc_host_errors(&thd->remote.sin_addr);
@@ -991,8 +1063,7 @@ static int check_connection(THD *thd)
     /*
       Old passwords are zero terminated strings.
     */
-    passwd= get_null_terminated_string(&end, &bytes_remaining_in_packet,
-                                       &passwd_len);
+    passwd= get_string(&end, &bytes_remaining_in_packet, &passwd_len);
   }
 
   if (passwd == NULL)
@@ -1007,8 +1078,7 @@ static int check_connection(THD *thd)
 
   if (thd->client_capabilities & CLIENT_CONNECT_WITH_DB)
   {
-    db= get_null_terminated_string(&end, &bytes_remaining_in_packet,
-                                   &db_len);
+    db= get_string(&end, &bytes_remaining_in_packet, &db_len);
     if (db == NULL)
     {
       inc_host_errors(&thd->remote.sin_addr);
@@ -1021,19 +1091,24 @@ static int check_connection(THD *thd)
   char user_buff[USERNAME_LENGTH + 1];	// buffer to store user in utf8
   uint dummy_errors;
 
-  /* Since 4.1 all database names are stored in utf8 */
+  /*
+    Copy and convert the user and database names to the character set used
+    by the server. Since 4.1 all database names are stored in UTF-8. Also,
+    ensure that the names are properly null-terminated as this is relied
+    upon later.
+  */
   if (db)
   {
-    db_buff[copy_and_convert(db_buff, sizeof(db_buff)-1,
-                             system_charset_info,
-                             db, db_len,
-                             thd->charset(), &dummy_errors)]= 0;
+    db_len= copy_and_convert(db_buff, sizeof(db_buff)-1, system_charset_info,
+                             db, db_len, thd->charset(), &dummy_errors);
+    db_buff[db_len]= '\0';
     db= db_buff;
   }
 
-  user_buff[user_len= copy_and_convert(user_buff, sizeof(user_buff)-1,
-                                       system_charset_info, user, user_len,
-                                       thd->charset(), &dummy_errors)]= '\0';
+  user_len= copy_and_convert(user_buff, sizeof(user_buff)-1,
+                             system_charset_info, user, user_len,
+                             thd->charset(), &dummy_errors);
+  user_buff[user_len]= '\0';
   user= user_buff;
 
   /* If username starts and ends in "'", chop them off */