From: Sergey Vojtovich Date: March 4 2011 8:09am Subject: Re: bzr commit into mysql-trunk branch (nirbhay.choubey:3717) Bug#11756377 List-Archive: http://lists.mysql.com/commits/132428 Message-Id: <20110304080946.GA17109@june> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Hi Nirbhay, assuming we all read relevant manual sections: http://dev.mysql.com/doc/refman/5.5/en/mysql-history-file.html http://dev.mysql.com/doc/refman/5.5/en/password-security-user.html ...is there still any reason to do this change? If there is, I assume we all understand that: - security insensible data may get filtered out (if it contains listed substrings); - security sensible data may not be filtered out (if it doesn't contain listed substrings); It makes me feel that suggested behaviour is rather complex and misleading. Please see a few technical comments inline. On Wed, Mar 02, 2011 at 02:49:55PM +0000, Nirbhay Choubey wrote: > #At file:///home/nirbhay/Project/mysql/repo/bugs/source/mysql-trunk.48287/ based on revid:vinay.fisrekar@stripped > > 3717 Nirbhay Choubey 2011-03-02 > BUG#11756377 - 48287: STOP LOGGING PASSWORDS IN HISTORY > > In interactive mode, MySQL client maintains a history file > (.mysql_history) to log all the commands executed through > it. Hence, eventually, it also logs the 'password' carrying > statements. > > Currently there is no way to disallow the logging of such > statements. > > This patch introduces a new MySQL client option '--log-password', > which by default is not set, and hence, in default mode all the > statements containing 'password' will not be logged into the > history file. > @ client/client_priv.h > BUG#11756377 - 48287: STOP LOGGING PASSWORDS IN HISTORY > > Added a client option to control logging of password > carrying statements in the history file. > @ client/mysql.cc > BUG#11756377 - 48287: STOP LOGGING PASSWORDS IN HISTORY > > Added function check_password() to check for the presence > of some specific string patterns that is usually found in > the statements that carry passwords along. > In this case, the entire statement is not logged in order > to improve the efficiency. > > modified: > client/client_priv.h > client/mysql.cc > === modified file 'client/client_priv.h' > --- a/client/client_priv.h 2011-02-21 11:33:20 +0000 > +++ b/client/client_priv.h 2011-03-02 14:49:53 +0000 > @@ -86,7 +86,8 @@ enum options_client > OPT_DEFAULT_PLUGIN, > OPT_RAW_OUTPUT, OPT_WAIT_SERVER_ID, OPT_STOP_NEVER, > OPT_BINLOG_ROWS_EVENT_MAX_SIZE, > - OPT_MAX_CLIENT_OPTION > + OPT_MAX_CLIENT_OPTION, > + OPT_LOG_PASSWORD > }; OPT_MAX_CLIENT_OPTION is supposed to be last in options_client enum. > > /** > > === modified file 'client/mysql.cc' > --- a/client/mysql.cc 2011-02-23 04:53:07 +0000 > +++ b/client/mysql.cc 2011-03-02 14:49:53 +0000 > @@ -136,7 +136,7 @@ static my_bool ignore_errors=0,wait_flag > vertical=0, line_numbers=1, column_names=1,opt_html=0, > opt_xml=0,opt_nopager=1, opt_outfile=0, named_cmds= 0, > tty_password= 0, opt_nobeep=0, opt_reconnect=1, > - opt_secure_auth= 0, > + opt_secure_auth= 0, opt_log_password= 0, tabs > default_pager_set= 0, opt_sigint_ignore= 0, > auto_vertical_output= 0, > show_warnings= 0, executing_query= 0, interrupted_query= 0, > @@ -276,6 +276,9 @@ static void init_username(); > static void add_int_to_prompt(int toadd); > static int get_result_width(MYSQL_RES *res); > static int get_field_disp_length(MYSQL_FIELD * field); > +static my_bool is_likely_to_contain_passwords(const char *string, int len); > +static void add_filtered_history(const char *string); > + > > /* A structure which contains information on the commands this program > can understand. */ > @@ -1616,6 +1619,10 @@ static struct my_option my_long_options[ > "Default authentication client-side plugin to use.", > (uchar**) &opt_default_auth, (uchar**) &opt_default_auth, 0, > GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0}, > + {"log-password", OPT_LOG_PASSWORD, > + "Log password containing statements in the history file.", Does default value appear somewhere in --help output? Probably it is a good idea to mention it. > + &opt_log_password, &opt_log_password, 0, GET_BOOL, NO_ARG, > + 0, 0, 0, 0, 0, 0}, > { 0, 0, 0, 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0} > }; > > @@ -1966,7 +1973,7 @@ static int read_and_execute(bool interac > in_string=0; > #ifdef HAVE_READLINE > if (interactive && status.add_to_history && not_in_history(line)) > - add_history(line); > + add_filtered_history(line); > #endif > continue; > } > @@ -2068,7 +2075,7 @@ static bool add_line(String &buffer,char > DBUG_RETURN(0); > #ifdef HAVE_READLINE > if (status.add_to_history && line[0] && not_in_history(line)) > - add_history(line); > + add_filtered_history(line); > #endif > char *end_of_line=line+(uint) strlen(line); > > @@ -2406,7 +2413,7 @@ static void fix_history(String *final_co > ptr++; > } > if (total_lines > 1) > - add_history(fixed_buffer.ptr()); > + add_filtered_history(fixed_buffer.ptr()); > } > > /* > @@ -5074,3 +5081,29 @@ static int com_prompt(String *buffer __a > tee_fprintf(stdout, "PROMPT set to '%s'\n", current_prompt); > return 0; > } > + > + > +static void add_filtered_history(const char *string) > +{ > + if (opt_log_password || !is_likely_to_contain_passwords(string, > + strlen(string))) > + add_history(string); > +} > + > + > +/* Check for presence of password in the statement. */ > +static my_bool is_likely_to_contain_passwords(const char *string, int len) > +{ > + char str[len]; > + char *temp; > + > + strncpy(str, string, (size_t) len); Here you copy string first time... > + temp= str; > + for ( ; *temp; *temp ++) > + *temp= my_toupper(charset_info, *temp); here you copy string second time... > + if (strstr(str, "IDENTIFIED BY") || strstr(str, "PASSWORD")) > + return 1; > + else > + return 0; > +} ...just to perform case insensitive substring lookup. Why not something like: charset_info->coll->instr(charset_info, string, len, STRING_WITH_LEN("PASSWORD"), match, 1) Regards, Sergey -- Sergey Vojtovich MySQL AB, Software Engineer Izhevsk, Russia, www.mysql.com