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 <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
