bk commit into 5.0 tree (cmiller:1.2138) BUG#17667

[Chad MILLER]

Below is the list of changes that have just been committed into a local
5.0 repository of cmiller. When cmiller does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet
  1.2138 06/05/01 22:10:50 cmiller@zippy.(none) +9 -0
  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.

  mysys/my_memmem.c
    1.1 06/05/01 22:10:44 cmiller@zippy.(none) +65 -0
    New BitKeeper file ``mysys/my_memmem.c''
    
    Implement memmem, a black-box work-alike of the GNU memmem(), which functions
    like strstr() but for arbitrary blocks of memory.

  mysql-test/t/mysql_client_test.opt
    1.1 06/05/01 22:10:44 cmiller@zippy.(none) +1 -0
    New BitKeeper file ``mysql-test/t/mysql_client_test.opt''
    
    Add '--log' server parameter.

  tests/mysql_client_test.c
    1.182 06/05/01 22:10:44 cmiller@zippy.(none) +66 -0
    Add a "%.1234b" and "%.*b" percent-code.  It takes a width, just like "%s", 
    but unlike the string-indicator, it requires the width and doesn't stop printing
    at NUL characters.

  tests/Makefile.am
    1.24 06/05/01 22:10:44 cmiller@zippy.(none) +1 -1
    We may need some of our local functions.

  strings/my_vsnprintf.c
    1.34 06/05/01 22:10:44 cmiller@zippy.(none) +8 -1
    Add a "%.1234b" and "%.*b" percent-code.  It takes a width, just like "%s", 
    but unlike the string-indicator, it requires the width and doesn't stop printing
    at NUL characters.

  sql/sql_parse.cc
    1.537 06/05/01 22:10:44 cmiller@zippy.(none) +1 -1
    The query is not a C-string, but is a sized buffer, containing any character 
    at all, which may include NUL characters.

  mysys/my_memmem.c
    1.0 06/05/01 22:10:44 cmiller@zippy.(none) +0 -0
    BitKeeper file /home/cmiller/work/mysql/mysql-5.0__bug17667/mysys/my_memmem.c

  mysys/mf_iocache2.c
    1.27 06/05/01 22:10:44 cmiller@zippy.(none) +72 -20
    Add a "%.1234b" and "%.*b" percent-code.  It takes a width, just like "%s", 
    but unlike the string-indicator, it requires the width and doesn't stop printing
    at NUL characters.
    
    Also, simplify the code a bit.
    
    TODO:  This code should be unified with the strings/my_vnsprintf.c code in 
    the future.

  mysys/Makefile.am
    1.68 06/05/01 22:10:44 cmiller@zippy.(none) +1 -0
    Add reference to new file, my_memmem.c

  mysql-test/t/mysql_client_test.opt
    1.0 06/05/01 22:10:44 cmiller@zippy.(none) +0 -0
    BitKeeper file
/home/cmiller/work/mysql/mysql-5.0__bug17667/mysql-test/t/mysql_client_test.opt

  include/my_sys.h
    1.179 06/05/01 22:10:44 cmiller@zippy.(none) +5 -0
    Add prototype for my_memmem() .

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	cmiller
# Host:	zippy.(none)
# Root:	/home/cmiller/work/mysql/mysql-5.0__bug17667

--- 1.178/include/my_sys.h	2005-11-21 19:02:32 -05:00
+++ 1.179/include/my_sys.h	2006-05-01 22:10:44 -04:00
@@ -599,6 +599,11 @@
 				    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);

--- 1.67/mysys/Makefile.am	2005-10-13 19:13:55 -04:00
+++ 1.68/mysys/Makefile.am	2006-05-01 22:10:44 -04:00
@@ -55,6 +55,7 @@
 			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

--- 1.33/strings/my_vsnprintf.c	2005-05-23 14:20:21 -04:00
+++ 1.34/strings/my_vsnprintf.c	2006-05-01 22:10:44 -04:00
@@ -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 @@
 
   for (; *fmt ; fmt++)
   {
-    if (fmt[0] != '%')
+    if (*fmt != '%')
     {
       if (to == end)			/* End of buffer */
 	break;
@@ -93,6 +94,12 @@
       if (left_len <= plen)
 	plen = left_len - 1;
       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 */

--- 1.536/sql/sql_parse.cc	2006-03-15 12:15:42 -05:00
+++ 1.537/sql/sql_parse.cc	2006-05-01 22:10:44 -04:00
@@ -1711,7 +1711,7 @@
     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))

--- 1.23/tests/Makefile.am	2005-08-23 14:25:19 -04:00
+++ 1.24/tests/Makefile.am	2006-05-01 22:10:44 -04:00
@@ -42,7 +42,7 @@
 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)
--- New file ---
+++ mysql-test/t/mysql_client_test.opt	06/05/01 22:10:44
--log

--- New file ---
+++ mysys/my_memmem.c	06/05/01 22:10:44
#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


--- 1.26/mysys/mf_iocache2.c	2006-01-03 11:54:34 -05:00
+++ 1.27/mysys/mf_iocache2.c	2006-05-01 22:10:44 -04:00
@@ -252,37 +252,89 @@
 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++)
+  /*
+    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++)
   {
-    if (*fmt++ != '%')
+    /* Copy everything until '%' or end of string */
+    const char *start=fmt;
+    uint length;
+    
+    for (; (*fmt != '\0') && (*fmt != '%'); fmt++) ;
+
+    length= (uint) (fmt - start);
+    out_length+=length;
+    if (my_b_write(info, start, length))
+      goto err;
+
+    if (*fmt == '\0')				/* End of format */
     {
-      /* Copy everything until '%' or end of string */
-      const char *start=fmt-1;
-      uint length;
-      for (; *fmt && *fmt != '%' ; fmt++ ) ;
-      length= (uint) (fmt - start);
-      out_length+=length;
-      if (my_b_write(info, start, length))
-	goto err;
-      if (!*fmt)				/* End of format */
-      {
-	return out_length;
-      }
-      fmt++;
-      /* Found one '%' */
+      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++;
+
+    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 @@
     else
     {
       /* %% or unknown code */
-      if (my_b_write(info, "%", 1))
-	goto err;
-      out_length++;
+      if (my_b_write(info, backtrack, fmt-backtrack))
+        goto err;
+      out_length+= fmt-backtrack;
     }
   }
   return out_length;

--- 1.181/tests/mysql_client_test.c	2006-03-30 13:35:46 -05:00
+++ 1.182/tests/mysql_client_test.c	2006-05-01 22:10:44 -04:00
@@ -14823,6 +14823,71 @@
 }
 
 /*
+  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
 */
 static void test_bug14169()
@@ -15121,6 +15186,7 @@
   { "test_bug16143", test_bug16143 },
   { "test_bug15613", test_bug15613 },
   { "test_bug14169", test_bug14169 },
+  { "test_bug17667", test_bug17667 },
   { 0, 0 }
 };