List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:March 5 2011 8:00am
Subject:Re: bzr commit into mysql-trunk branch (nirbhay.choubey:3717)
Bug#11756377
View as plain text  
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.: "IDENTIFIED<SP><SP>BY".
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 <svoj@stripped>
> 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=1
> 

-- 
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