List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:March 4 2011 8:09am
Subject:Re: bzr commit into mysql-trunk branch (nirbhay.choubey:3717)
Bug#11756377
View as plain text  
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
Thread
bzr commit into mysql-trunk branch (nirbhay.choubey:3717) Bug#11756377Nirbhay Choubey2 Mar
  • Re: bzr commit into mysql-trunk branch (nirbhay.choubey:3717)Bug#11756377Sergey Vojtovich4 Mar
    • Re: bzr commit into mysql-trunk branch (nirbhay.choubey:3717)Bug#11756377Sergey Vojtovich5 Mar