From: Sergey Vojtovich Date: March 5 2011 8:00am Subject: Re: bzr commit into mysql-trunk branch (nirbhay.choubey:3717) Bug#11756377 List-Archive: http://lists.mysql.com/commits/132513 Message-Id: <20110305080049.GA19474@june> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Hi! As promised, I'm sending out my view on this bug. The problem: security sensible data may go to the history file. svoj: A valid point. It is suggested that we filter out security sensible data by default. svoj: Well Ok, but it looks paranoic assuming it is documented, there is a workaround and we do something to secure this data. It is suggested that we consider data to be security sensible if it contains "IDENTIFIED BY" or "PASSWORD". svoj: Ok. It is suggested that we perform rough filtering. As I mentioned before: security sensible data may not be filtered out and security insensible data may be filetered out. E.g.: "IDENTIFIEDBY". svoj: Probably Ok, assuming it makes 90% users happier and 10% unhappier. It is suggested that users have no control of what security sensible data is. svoj: It doesn't sound Ok. Probably at least it is worth to study what more senior CLI colleagues do? E.g.: man bash (HISTCONTROL, HISTIGNORE). Regards, Sergey On Fri, Mar 04, 2011 at 11:09:46AM +0300, Sergey Vojtovich wrote: > 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 > > -- > MySQL Code Commits Mailing List > For list archives: http://lists.mysql.com/commits > To unsubscribe: http://lists.mysql.com/commits?unsub=svoj@stripped > -- Sergey Vojtovich MySQL AB, Software Engineer Izhevsk, Russia, www.mysql.com