From a905ac34b59731bb69a036306297c50742753329 Mon Sep 17 00:00:00 2001 From: unknown Date: Mon, 26 Nov 2007 08:20:40 +0100 Subject: [PATCH 1/4] Bug#31752: check strmake() bounds strmake() calls are easy to get wrong. Add checks in extra debug mode to identify possible exploits. Remove some dead code. Remove some off-by-one errors identified with new checks. sql/log.cc: fix off-by-one buffer-length argument to prevent stack smashing sql/repl_failsafe.cc: fix off-by-one buffer-length argument to prevent stack smashing sql/set_var.cc: fix off-by-one buffer-length argument to prevent stack smashing (already approved, backports #31588) sql/sql_show.cc: misdimensioned buffers: functions further down the callstack expect bufsize of FN_REFLEN sql/unireg.cc: When EXTRA_DEBUG is enabled, strmake() will write funny patterns to buffers it operates on to identify possibly overflows. This leads to badness in mysql_create_frm(), so we explicitly put any unused bytes (back) into a defined state. Not a bug-fix, but part of the strmake() bug detector. strings/strmake.c: strmake() takes maximum string length rather than buffer-length (string length + 1 to accomodate \0 terminator) as argument. Since this is easy to get wrong, add extra debug code to identify off-by-ones so we can prevent stack smashing. Alternative "BAD_STRING_COMPILER" removed after checking with Monty. --- sql/log.cc | 2 +- sql/repl_failsafe.cc | 2 +- sql/set_var.cc | 2 +- sql/sql_show.cc | 4 ++-- sql/unireg.cc | 3 +++ strings/strmake.c | 32 +++++++++++++++++--------------- 6 files changed, 25 insertions(+), 20 deletions(-) diff --git a/sql/log.cc b/sql/log.cc index b91ec2b3dee..5a4f02a827b 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -966,7 +966,7 @@ void MYSQL_LOG::make_log_name(char* buf, const char* log_ident) if (dir_len > FN_REFLEN) dir_len=FN_REFLEN-1; strnmov(buf, log_file_name, dir_len); - strmake(buf+dir_len, log_ident, FN_REFLEN - dir_len); + strmake(buf+dir_len, log_ident, FN_REFLEN - dir_len -1); } diff --git a/sql/repl_failsafe.cc b/sql/repl_failsafe.cc index 4c8703226a6..4ea90346638 100644 --- a/sql/repl_failsafe.cc +++ b/sql/repl_failsafe.cc @@ -926,7 +926,7 @@ int load_master_data(THD* thd) 0, (SLAVE_IO | SLAVE_SQL))) send_error(thd, ER_MASTER_INFO); strmake(active_mi->master_log_name, row[0], - sizeof(active_mi->master_log_name)); + sizeof(active_mi->master_log_name) -1); active_mi->master_log_pos= my_strtoll10(row[1], (char**) 0, &error); /* at least in recent versions, the condition below should be false */ if (active_mi->master_log_pos < BIN_LOG_HEADER_SIZE) diff --git a/sql/set_var.cc b/sql/set_var.cc index 520ee5c9f70..1d18eba30a8 100644 --- a/sql/set_var.cc +++ b/sql/set_var.cc @@ -1573,7 +1573,7 @@ bool sys_var::check_set(THD *thd, set_var *var, TYPELIB *enum_names) ¬_used)); if (error_len) { - strmake(buff, error, min(sizeof(buff), error_len)); + strmake(buff, error, min(sizeof(buff) - 1, error_len)); goto err; } } diff --git a/sql/sql_show.cc b/sql/sql_show.cc index bf0e254d3e4..be658c8fe5d 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -136,7 +136,7 @@ int mysqld_show_tables(THD *thd,const char *db,const char *wild) { Item_string *field=new Item_string("",0,thd->charset()); List field_list; - char path[FN_LEN],*end; + char path[FN_REFLEN],*end; List files; char *file_name; Protocol *protocol= thd->protocol; @@ -457,7 +457,7 @@ int mysqld_extend_show_tables(THD *thd,const char *db,const char *wild) Item *item; List files; List field_list; - char path[FN_LEN]; + char path[FN_REFLEN]; char *file_name; TABLE *table; Protocol *protocol= thd->protocol; diff --git a/sql/unireg.cc b/sql/unireg.cc index e5ee0222f20..795198fc55f 100644 --- a/sql/unireg.cc +++ b/sql/unireg.cc @@ -140,6 +140,9 @@ bool mysql_create_frm(THD *thd, my_string file_name, strmake((char*) forminfo+47,create_info->comment ? create_info->comment : "", 60); forminfo[46]=(uchar) strlen((char*)forminfo+47); // Length of comment +#ifdef EXTRA_DEBUG + memset((char*) forminfo+47 + forminfo[46], 0, 61 - forminfo[46]); +#endif if (my_pwrite(file,(byte*) fileinfo,64,0L,MYF_RW) || my_pwrite(file,(byte*) keybuff,key_info_length, diff --git a/strings/strmake.c b/strings/strmake.c index d2252f648f6..47d8a04e361 100644 --- a/strings/strmake.c +++ b/strings/strmake.c @@ -28,23 +28,25 @@ #include #include "m_string.h" -#ifdef BAD_STRING_COMPILER - -char *strmake(char *dst,const char *src,uint length) -{ - reg1 char *res; - - if ((res=memccpy(dst,src,0,length))) - return res-1; - dst[length]=0; - return dst+length; -} - -#define strmake strmake_overlapp /* Use orginal for overlapping str */ -#endif - char *strmake(register char *dst, register const char *src, uint length) { +#ifdef EXTRA_DEBUG + /* + 'length' is the maximum length of the string; the buffer needs + to be one character larger to accomodate the terminating '\0'. + This is easy to get wrong, so we make sure we write to the + entire length of the buffer to identify incorrect buffer-sizes. + We only initialise the "unused" part of the buffer here, a) for + efficiency, and b) because dst==src is allowed, so initialising + the entire buffer would overwrite the source-string. Also, we + write a character rather than '\0' as this makes spotting these + problems in the results easier. + */ + uint n= strlen(src) + 1; + if (n <= length) + memset(dst + n, (int) 'Z', length - n + 1); +#endif + while (length--) if (! (*dst++ = *src++)) return dst-1; From 1c72446ef69cf6c50cf9b2dae69b2b24a7576103 Mon Sep 17 00:00:00 2001 From: unknown Date: Mon, 26 Nov 2007 09:13:23 +0100 Subject: [PATCH 2/4] Bug#31752: check strmake() bounds strmake() called with wrong parameters: 5.0-specific fixes. client/mysql.cc: In debug-mode, strmake() fills unused part of buffer with a test-pattern. This overwrites our previous extra '\0' (from previous bzero()). sql/sp.cc: off-by-one buffer-size. --- client/mysql.cc | 5 ++++- sql/sp.cc | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/client/mysql.cc b/client/mysql.cc index 8e1b6c2a9b4..ff2c1d228cd 100644 --- a/client/mysql.cc +++ b/client/mysql.cc @@ -2987,7 +2987,10 @@ com_connect(String *buffer, char *line) Two null bytes are needed in the end of buff to allow get_arg to find end of string the second time it's called. */ - strmake(buff, line, sizeof(buff)-2); + tmp= strmake(buff, line, sizeof(buff)-2); +#ifdef EXTRA_DEBUG + tmp[1]= 0; +#endif tmp= get_arg(buff, 0); if (tmp && *tmp) { diff --git a/sql/sp.cc b/sql/sp.cc index 75d6fa4618f..bae5933aec1 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -1902,7 +1902,7 @@ sp_use_new_db(THD *thd, LEX_STRING new_db, LEX_STRING *old_db, if (thd->db) { - old_db->length= (strmake(old_db->str, thd->db, old_db->length) - + old_db->length= (strmake(old_db->str, thd->db, old_db->length - 1) - old_db->str); } else From 5ce7cdbe54667610f3ab1acf849985d637472e1d Mon Sep 17 00:00:00 2001 From: unknown Date: Mon, 26 Nov 2007 09:38:42 +0100 Subject: [PATCH 3/4] Bug#31752: check strmake() bounds strmake() called with wrong parameters: 5.1-specific fixes. sql/sql_db.cc: fix off-by-one buffer-length --- sql/sql_db.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/sql_db.cc b/sql/sql_db.cc index abbf2131957..5fd0f873364 100644 --- a/sql/sql_db.cc +++ b/sql/sql_db.cc @@ -1387,7 +1387,7 @@ static void backup_current_db_name(THD *thd, } else { - strmake(saved_db_name->str, thd->db, saved_db_name->length); + strmake(saved_db_name->str, thd->db, saved_db_name->length - 1); saved_db_name->length= thd->db_length; } } From 0805384869656fc9efaa28de331e825aa8b885d7 Mon Sep 17 00:00:00 2001 From: unknown Date: Thu, 6 Dec 2007 11:48:27 +0100 Subject: [PATCH 4/4] Bug#31752: check strmake() bounds post-fixes: prevent semi-related overflow, additional comments mysys/mf_pack.c: extra comments sql/log.cc: prevent overflow (length parameter of strmake() should never become < 0) sql/sql_show.cc: additional comments sql/unireg.cc: additional comments --- mysys/mf_pack.c | 4 ++-- sql/log.cc | 2 +- sql/sql_show.cc | 5 +++-- sql/unireg.cc | 5 +++++ 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/mysys/mf_pack.c b/mysys/mf_pack.c index 049aa59a578..485b2c5ec3d 100644 --- a/mysys/mf_pack.c +++ b/mysys/mf_pack.c @@ -272,7 +272,7 @@ void symdirget(char *dir) SYNOPSIS unpack_dirname() - to Store result here. May be = from + to result-buffer, FN_REFLEN characters. may be == from from 'Packed' directory name (may contain ~) IMPLEMENTATION @@ -398,7 +398,7 @@ uint unpack_filename(my_string to, const char *from) /* Convert filename (unix standard) to system standard */ /* Used before system command's like open(), create() .. */ - /* Returns length of to */ + /* Returns used length of to; total length should be FN_REFLEN */ uint system_filename(my_string to, const char *from) { diff --git a/sql/log.cc b/sql/log.cc index 5a4f02a827b..4e0964fe9d6 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -963,7 +963,7 @@ err: void MYSQL_LOG::make_log_name(char* buf, const char* log_ident) { uint dir_len = dirname_length(log_file_name); - if (dir_len > FN_REFLEN) + if (dir_len >= FN_REFLEN) dir_len=FN_REFLEN-1; strnmov(buf, log_file_name, dir_len); strmake(buf+dir_len, log_ident, FN_REFLEN - dir_len -1); diff --git a/sql/sql_show.cc b/sql/sql_show.cc index be658c8fe5d..741d30e6a99 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -136,7 +136,8 @@ int mysqld_show_tables(THD *thd,const char *db,const char *wild) { Item_string *field=new Item_string("",0,thd->charset()); List field_list; - char path[FN_REFLEN],*end; + char path[FN_REFLEN],*end; // for unpack_dirname() + List files; char *file_name; Protocol *protocol= thd->protocol; @@ -457,7 +458,7 @@ int mysqld_extend_show_tables(THD *thd,const char *db,const char *wild) Item *item; List files; List field_list; - char path[FN_REFLEN]; + char path[FN_REFLEN]; // for unpack_dirname() char *file_name; TABLE *table; Protocol *protocol= thd->protocol; diff --git a/sql/unireg.cc b/sql/unireg.cc index 795198fc55f..dcb49bc1766 100644 --- a/sql/unireg.cc +++ b/sql/unireg.cc @@ -141,6 +141,11 @@ bool mysql_create_frm(THD *thd, my_string file_name, 60); forminfo[46]=(uchar) strlen((char*)forminfo+47); // Length of comment #ifdef EXTRA_DEBUG + /* + EXTRA_DEBUG causes strmake() to initialize its buffer behind the + payload with a magic value to detect wrong buffer-sizes. We + explicitly zero that segment again. + */ memset((char*) forminfo+47 + forminfo[46], 0, 61 - forminfo[46]); #endif