SECURITY FIX

Bug#17667: An attacker has the opportunity to bypass query logging.

This adds a new, local-only printf format specifier to our *printf functions
that allows us to print known-size buffers that must not be interpreted as 
NUL-terminated "strings."

It uses this format-specifier to print to the log, thus fixing this 
problem.

include/my_sys.h

@@ -599,6 +599,11 @@ extern char *_my_strdup_with_length(const byte *from, uint length,
 				    const char *sFile, uint uLine,
 				    myf MyFlag);
 
+/* implemented in my_memmem.c */
+extern void *my_memmem(const void *haystack, size_t haystacklen,
+    const void *needle, size_t needlelen);
+
+
 #ifdef __WIN__
 extern int my_access(const char *path, int amode);
 extern File my_sopen(const char *path, int oflag, int shflag, int pmode);

mysql-test/t/mysql_client_test.opt

+--log

mysys/Makefile.am

@@ -55,6 +55,7 @@ libmysys_a_SOURCES =    my_init.c my_getwd.c mf_getdate.c my_mmap.c \
 			charset.c charset-def.c my_bitmap.c my_bit.c md5.c \
 			my_gethostbyname.c rijndael.c my_aes.c sha1.c \
 			my_handler.c my_netware.c my_largepage.c \
+			my_memmem.c \
 			my_windac.c my_access.c base64.c
 EXTRA_DIST =		thr_alarm.c thr_lock.c my_pthread.c my_thr_init.c \
 			thr_mutex.c thr_rwlock.c

mysys/mf_iocache2.c

@@ -252,37 +252,89 @@ uint my_b_printf(IO_CACHE *info, const char* fmt, ...)
 uint my_b_vprintf(IO_CACHE *info, const char* fmt, va_list args)
 {
   uint out_length=0;
+  uint minimum_width; /* as yet unimplemented */
+  uint minimum_width_sign;
+  uint precision; /* as yet unimplemented for anything but %b */
 
-  for (; *fmt ; fmt++)
-  {
-    if (*fmt++ != '%')
+  /*
+    Store the location of the beginning of a format directive, for the
+    case where we learn we shouldn't have been parsing a format string
+    at all, and we don't want to lose the flag/precision/width/size
+    information.
+   */
+  const char* backtrack;
+
+  for (; *fmt != '\0'; fmt++)
   {
     /* Copy everything until '%' or end of string */
-      const char *start=fmt-1;
+    const char *start=fmt;
     uint length;
-      for (; *fmt && *fmt != '%' ; fmt++ ) ;
+    
+    for (; (*fmt != '\0') && (*fmt != '%'); fmt++) ;
+
     length= (uint) (fmt - start);
     out_length+=length;
     if (my_b_write(info, start, length))
       goto err;
-      if (!*fmt)				/* End of format */
+
+    if (*fmt == '\0')				/* End of format */
     {
       return out_length;
     }
+
+    /* 
+      By this point, *fmt must be a percent;  Keep track of this location and
+      skip over the percent character. 
+    */
+    DBUG_ASSERT(*fmt == '%');
+    backtrack= fmt;
     fmt++;
-      /* Found one '%' */
-    }
+
+    minimum_width= 0;
+    precision= 0;
+    minimum_width_sign= 1;
     /* Skip if max size is used (to be compatible with printf) */
-    while (my_isdigit(&my_charset_latin1, *fmt) || *fmt == '.' || *fmt == '-')
+    while (*fmt == '-') { fmt++; minimum_width_sign= -1; }
+    if (*fmt == '*') {
+      precision= (int) va_arg(args, int);
+      fmt++;
+    } else {
+      while (my_isdigit(&my_charset_latin1, *fmt)) {
+        minimum_width=(minimum_width * 10) + (*fmt - '0');
         fmt++;
+      }
+    }
+    minimum_width*= minimum_width_sign;
+
+    if (*fmt == '.') {
+      fmt++;
+      if (*fmt == '*') {
+        precision= (int) va_arg(args, int);
+        fmt++;
+      } else {
+        while (my_isdigit(&my_charset_latin1, *fmt)) {
+          precision=(precision * 10) + (*fmt - '0');
+          fmt++;
+        }
+      }
+    }
+
     if (*fmt == 's')				/* String parameter */
     {
       reg2 char *par = va_arg(args, char *);
       uint length = (uint) strlen(par);
+      /* TODO: implement minimum width and precision */
       out_length+=length;
       if (my_b_write(info, par, length))
 	goto err;
     }
+    else if (*fmt == 'b')                       /* Sized buffer parameter, only precision makes sense */
+    {
+      char *par = va_arg(args, char *);
+      out_length+= precision;
+      if (my_b_write(info, par, precision))
+        goto err;
+    }
     else if (*fmt == 'd' || *fmt == 'u')	/* Integer parameter */
     {
       register int iarg;
@@ -317,9 +369,9 @@ uint my_b_vprintf(IO_CACHE *info, const char* fmt, va_list args)
     else
     {
       /* %% or unknown code */
-      if (my_b_write(info, "%", 1))
+      if (my_b_write(info, backtrack, fmt-backtrack))
         goto err;
-      out_length++;
+      out_length+= fmt-backtrack;
     }
   }
   return out_length;

mysys/my_memmem.c

+#include "my_base.h"
+
+/*
+  my_memmem, port of a GNU extension.
+
+  Returns a pointer to the beginning of the substring, needle, or NULL if the
+  substring is not found in haystack.
+*/
+void *my_memmem(const void *haystack, size_t haystacklen,
+    const void *needle, size_t needlelen)
+{
+  const void *cursor;
+  const void *last_possible_needle_location = haystack + haystacklen - needlelen;
+
+  /* Easy answers */
+  if (needlelen > haystacklen) return(NULL);
+  if (needle == NULL) return(NULL);
+  if (haystack == NULL) return(NULL);
+  if (needlelen == 0) return(NULL);
+  if (haystacklen == 0) return(NULL);
+
+  for (cursor = haystack; cursor <= last_possible_needle_location; cursor++) {
+    if (memcmp(needle, cursor, needlelen) == 0) {
+      return((void *) cursor);
+    }
+  }
+  return(NULL);
+}
+
+  
+
+#ifdef MAIN
+#include <assert.h>
+
+int main(int argc, char *argv[]) {
+  char haystack[10], needle[3];
+
+  memmove(haystack, "0123456789", 10);
+
+  memmove(needle, "no", 2);
+  assert(my_memmem(haystack, 10, needle, 2) == NULL);
+
+  memmove(needle, "345", 3);
+  assert(my_memmem(haystack, 10, needle, 3) != NULL);
+
+  memmove(needle, "789", 3);
+  assert(my_memmem(haystack, 10, needle, 3) != NULL);
+  assert(my_memmem(haystack, 9, needle, 3) == NULL);
+
+  memmove(needle, "012", 3);
+  assert(my_memmem(haystack, 10, needle, 3) != NULL);
+  assert(my_memmem(NULL, 10, needle, 3) == NULL);
+
+  assert(my_memmem(NULL, 10, needle, 3) == NULL);
+  assert(my_memmem(haystack, 0, needle, 3) == NULL);
+  assert(my_memmem(haystack, 10, NULL, 3) == NULL);
+  assert(my_memmem(haystack, 10, needle, 0) == NULL);
+
+  assert(my_memmem(haystack, 1, needle, 3) == NULL);
+
+  printf("success\n");
+  return(0);
+}
+
+#endif

sql/sql_parse.cc

@@ -1711,7 +1711,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
     if (alloc_query(thd, packet, packet_length))
       break;					// fatal error is set
     char *packet_end= thd->query + thd->query_length;
-    mysql_log.write(thd,command,"%s",thd->query);
+    mysql_log.write(thd,command, "%.*b", thd->query_length, thd->query);
     DBUG_PRINT("query",("%-.4096s",thd->query));
 
     if (!(specialflag & SPECIAL_NO_PRIOR))

strings/my_vsnprintf.c

@@ -27,6 +27,7 @@
     %#[l]d
     %#[l]u
     %#[l]x
+    %#.#b 	Local format; note first # is ignored and second is REQUIRED
     %#.#s	Note first # is ignored
     
   RETURN
@@ -40,7 +41,7 @@ int my_vsnprintf(char *to, size_t n, const char* fmt, va_list ap)
 
   for (; *fmt ; fmt++)
   {
-    if (fmt[0] != '%')
+    if (*fmt != '%')
     {
       if (to == end)			/* End of buffer */
 	break;
@@ -95,6 +96,12 @@ int my_vsnprintf(char *to, size_t n, const char* fmt, va_list ap)
       to=strnmov(to,par,plen);
       continue;
     }
+    else if (*fmt == 'b')				/* Buffer parameter */
+    {
+      char *par = va_arg(ap, char *);
+      to=memmove(to, par, abs(width));
+      continue;
+    }
     else if (*fmt == 'd' || *fmt == 'u'|| *fmt== 'x')	/* Integer parameter */
     {
       register long larg;

tests/Makefile.am

@@ -42,7 +42,7 @@ INCLUDES =		-I$(top_builddir)/include -I$(top_srcdir)/include \
 LIBS =			@CLIENT_LIBS@
 LDADD =			@CLIENT_EXTRA_LDFLAGS@ \
                         $(top_builddir)/libmysql/libmysqlclient.la
-mysql_client_test_LDADD= $(LDADD) $(CXXLDFLAGS)
+mysql_client_test_LDADD= $(LDADD) $(CXXLDFLAGS) -lmysys -L../mysys
 mysql_client_test_SOURCES= mysql_client_test.c $(yassl_dummy_link_fix)
 insert_test_SOURCES=       insert_test.c $(yassl_dummy_link_fix)
 select_test_SOURCES=       select_test.c $(yassl_dummy_link_fix)

tests/mysql_client_test.c

@@ -14822,6 +14822,71 @@ static void test_bug15613()
   mysql_stmt_close(stmt);
 }
 
+/*
+  Bug#17667: An attacker has the opportunity to bypass query logging.
+*/
+static void test_bug17667()
+{
+  int rc;
+  myheader("test_bug17667");
+  struct buffer_and_length {
+    const char *buffer;
+    const uint length;
+  } statements[]= {
+    { "drop table if exists bug17667", 29 },
+    { "create table bug17667 (c varchar(20))", 37 },
+    { "insert into bug17667 (c) values ('regular') /* NUL=\0 with comment */", 68 },
+    { "insert into bug17667 (c) values ('NUL=\0 in value')", 50 },
+    { "insert into bug17667 (c) values ('5 NULs=\0\0\0\0\0')", 48 },
+    { "/* NUL=\0 with comment */ insert into bug17667 (c) values ('encore')", 67 },
+    { "drop table bug17667", 19 },
+    { NULL, 0 } };  
+
+  struct buffer_and_length *statement_cursor;
+  FILE *log_file;
+
+  for (statement_cursor= statements; statement_cursor->buffer != NULL;
+      statement_cursor++) {
+    rc= mysql_real_query(mysql, statement_cursor->buffer,
+        statement_cursor->length);
+    myquery(rc);
+  }
+
+  sleep(1); /* The server may need time to flush the data to the log. */
+  log_file= fopen("var/log/master.log", "r");
+  if (log_file != NULL) {
+
+    for (statement_cursor= statements; statement_cursor->buffer != NULL;
+        statement_cursor++) {
+     char line_buffer[MAX_TEST_QUERY_LENGTH*2]; 
+     /* more than enough room for the query and some marginalia. */
+
+      do {
+        memset(line_buffer, '/', MAX_TEST_QUERY_LENGTH*2);
+
+        DIE_UNLESS(fgets(line_buffer, MAX_TEST_QUERY_LENGTH*2, log_file) !=
+            NULL);
+        /* If we reach EOF before finishing the statement list, then we failed. */
+
+      } while (my_memmem(line_buffer, MAX_TEST_QUERY_LENGTH*2,
+            statement_cursor->buffer, statement_cursor->length) == NULL);
+    }
+
+    printf("success.  All queries found intact in the log.\n");
+
+  } else {
+    fprintf(stderr, "Could not find the log file, var/log/master.log, so "
+        "test_bug17667 is \ninconclusive.  Run test from the "
+        "mysql-test/mysql-test-run* program \nto set up the correct "
+        "environment for this test.\n\n");
+  }
+
+  if (log_file != NULL)
+    fclose(log_file);
+
+}
+
+
 /*
   Bug#14169: type of group_concat() result changed to blob if tmp_table was used
 */
@@ -15121,6 +15186,7 @@ static struct my_tests_st my_tests[]= {
   { "test_bug16143", test_bug16143 },
   { "test_bug15613", test_bug15613 },
   { "test_bug14169", test_bug14169 },
+  { "test_bug17667", test_bug17667 },
   { 0, 0 }
 };