[Commits] a60f851: MDEV-8825 mysql_upgrade leaks the admin password when it spawns a shell process to execute mysqlcheck

serg at mariadb.org serg at mariadb.org
Tue Dec 8 00:47:37 EET 2015


revision-id: a60f8511df7b9931eadb95960b40080ee6c0ef6f (mariadb-5.5.46-25-ga60f851)
parent(s): f18599a129cb64a65187a6c12a9b2505f39f85f5
committer: Sergei Golubchik
timestamp: 2015-12-07 15:20:24 +0100
message:

MDEV-8825 mysql_upgrade leaks the admin password when it spawns a shell process to execute mysqlcheck

don't put common arguments on the command-line,
use a config file instead

---
 client/mysql_upgrade.c            | 87 +++++++++++++++++++++++----------------
 mysql-test/r/mysql_upgrade.result |  2 +-
 mysql-test/t/mysql_upgrade.test   |  2 +-
 3 files changed, 53 insertions(+), 38 deletions(-)

diff --git a/client/mysql_upgrade.c b/client/mysql_upgrade.c
index ee6845d..0eac9cb 100644
--- a/client/mysql_upgrade.c
+++ b/client/mysql_upgrade.c
@@ -53,6 +53,8 @@ static DYNAMIC_STRING conn_args;
 static char *opt_password= 0;
 static char *opt_plugin_dir= 0, *opt_default_auth= 0;
 
+static char *cnf_file_path= 0, defaults_file[FN_REFLEN + 32];
+
 static my_bool tty_password= 0;
 
 static char opt_tmpdir[FN_REFLEN] = "";
@@ -109,6 +111,7 @@ static struct my_option my_long_options[]=
    &opt_force, &opt_force, 0, GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0},
   {"host", 'h', "Connect to host.", 0,
    0, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
+#define PASSWORD_OPT 12
   {"password", 'p',
    "Password to use when connecting to server. If password is not given,"
    " it's solicited on the tty.", &opt_password,&opt_password,
@@ -146,6 +149,7 @@ static struct my_option my_long_options[]=
    "do not try to upgrade the data.",
    &opt_systables_only, &opt_systables_only, 0,
    GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0},
+#define USER_OPT (array_elements(my_long_options) - 6)
   {"user", 'u', "User for login if not current user.", &opt_user,
    &opt_user, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
   {"verbose", 'v', "Display more output about the process.",
@@ -182,6 +186,8 @@ static void free_used_memory(void)
 
   dynstr_free(&ds_args);
   dynstr_free(&conn_args);
+  if (cnf_file_path)
+    my_delete(cnf_file_path, MYF(MY_WME));
 }
 
 
@@ -233,31 +239,32 @@ static void verbose(const char *fmt, ...)
   this way we pass the same arguments on to mysql and mysql_check
 */
 
-static void add_one_option(DYNAMIC_STRING* ds,
-                           const struct my_option *opt,
-                           const char* argument)
-
+static void add_one_option_cmd_line(DYNAMIC_STRING *ds,
+                                    const struct my_option *opt,
+                                    const char* arg)
 {
-  const char* eq= NullS;
-  const char* arg= NullS;
-  if (opt->arg_type != NO_ARG)
+  dynstr_append(ds, "--");
+  dynstr_append(ds, opt->name);
+  if (arg)
   {
-    eq= "=";
-    switch (opt->var_type & GET_TYPE_MASK) {
-    case GET_STR:
-      arg= argument;
-      break;
-    case GET_BOOL:
-      arg= (*(my_bool *)opt->value) ? "1" : "0";
-      break;
-    default:
-      die("internal error at %s: %d",__FILE__, __LINE__);
-    }
+    dynstr_append(ds, "=");
+    dynstr_append_os_quoted(ds, arg, NullS);
   }
-  dynstr_append_os_quoted(ds, "--", opt->name, eq, arg, NullS);
   dynstr_append(ds, " ");
 }
 
+static void add_one_option_cnf_file(DYNAMIC_STRING *ds,
+                                    const struct my_option *opt,
+                                    const char* arg)
+{
+  dynstr_append(ds, opt->name);
+  if (arg)
+  {
+    dynstr_append(ds, "=");
+    dynstr_append_os_quoted(ds, arg, NullS);
+  }
+  dynstr_append(ds, "\n");
+}
 
 static my_bool
 get_one_option(int optid, const struct my_option *opt,
@@ -288,16 +295,17 @@ get_one_option(int optid, const struct my_option *opt,
   case 'p':
     if (argument == disabled_my_option)
       argument= (char*) "";			/* Don't require password */
-    tty_password= 1;
     add_option= FALSE;
     if (argument)
     {
       /* Add password to ds_args before overwriting the arg with x's */
-      add_one_option(&ds_args, opt, argument);
+      add_one_option_cnf_file(&ds_args, opt, argument);
       while (*argument)
         *argument++= 'x';                       /* Destroy argument */
       tty_password= 0;
     }
+    else
+      tty_password= 1;
     break;
 
   case 't':
@@ -344,18 +352,18 @@ get_one_option(int optid, const struct my_option *opt,
   case OPT_SHARED_MEMORY_BASE_NAME: /* --shared-memory-base-name */
   case OPT_PLUGIN_DIR:                          /* --plugin-dir */
   case OPT_DEFAULT_AUTH:                        /* --default-auth */
-    add_one_option(&conn_args, opt, argument);
+    add_one_option_cmd_line(&conn_args, opt, argument);
     break;
   }
 
   if (add_option)
   {
     /*
-      This is an option that is accpted by mysql_upgrade just so
+      This is an option that is accepted by mysql_upgrade just so
       it can be passed on to "mysql" and "mysqlcheck"
       Save it in the ds_args string
     */
-    add_one_option(&ds_args, opt, argument);
+    add_one_option_cnf_file(&ds_args, opt, argument);
   }
   return 0;
 }
@@ -561,8 +569,7 @@ static int run_query(const char *query, DYNAMIC_STRING *ds_res,
 
   ret= run_tool(mysql_path,
                 ds_res,
-                "--no-defaults",
-                ds_args.str,
+                defaults_file,
                 "--database=mysql",
                 "--batch", /* Turns off pager etc. */
                 force ? "--force": "--skip-force",
@@ -751,8 +758,7 @@ static int run_mysqlcheck_upgrade(void)
   print_conn_args("mysqlcheck");
   retch= run_tool(mysqlcheck_path,
                   NULL, /* Send output from mysqlcheck directly to screen */
-                  "--no-defaults",
-                  ds_args.str,
+                  defaults_file,
                   "--check-upgrade",
                   "--all-databases",
                   "--auto-repair",
@@ -805,8 +811,7 @@ static int run_mysqlcheck_views(void)
   print_conn_args("mysqlcheck");
   return run_tool(mysqlcheck_path,
                   NULL, /* Send output from mysqlcheck directly to screen */
-                  "--no-defaults",
-                  ds_args.str,
+                  defaults_file,
                   "--all-databases", "--repair",
                   upgrade_views,
                   "--skip-process-tables",
@@ -830,8 +835,7 @@ static int run_mysqlcheck_fixnames(void)
   print_conn_args("mysqlcheck");
   return run_tool(mysqlcheck_path,
                   NULL, /* Send output from mysqlcheck directly to screen */
-                  "--no-defaults",
-                  ds_args.str,
+                  defaults_file,
                   "--all-databases",
                   "--fix-db-names",
                   "--fix-table-names",
@@ -1031,12 +1035,23 @@ int main(int argc, char **argv)
   {
     opt_password= get_tty_password(NullS);
     /* add password to defaults file */
-    dynstr_append_os_quoted(&ds_args, "--password=", opt_password, NullS);
-    dynstr_append(&ds_args, " ");
+    add_one_option_cnf_file(&ds_args, &my_long_options[PASSWORD_OPT], opt_password);
+    DBUG_ASSERT(strcmp(my_long_options[PASSWORD_OPT].name, "password") == 0);
   }
   /* add user to defaults file */
-  dynstr_append_os_quoted(&ds_args, "--user=", opt_user, NullS);
-  dynstr_append(&ds_args, " ");
+  add_one_option_cnf_file(&ds_args, &my_long_options[USER_OPT], opt_user);
+  DBUG_ASSERT(strcmp(my_long_options[USER_OPT].name, "user") == 0);
+
+  cnf_file_path= strmov(defaults_file, "--defaults-file=");
+  {
+    int fd= create_temp_file(cnf_file_path,
+                                  opt_tmpdir[0] ? opt_tmpdir : NULL,
+                                  "mysql_upgrade-", O_CREAT | O_WRONLY,
+                                  MYF(MY_FAE));
+    my_write(fd, USTRING_WITH_LEN( "[client]\n"), MYF(MY_FAE));
+    my_write(fd, (uchar*)ds_args.str, ds_args.length, MYF(MY_FAE));
+    my_close(fd, MYF(0));
+  }
 
   /* Find mysql */
   find_tool(mysql_path, IF_WIN("mysql.exe", "mysql"), self_name);
diff --git a/mysql-test/r/mysql_upgrade.result b/mysql-test/r/mysql_upgrade.result
index 8b8fea2..805ee82 100644
--- a/mysql-test/r/mysql_upgrade.result
+++ b/mysql-test/r/mysql_upgrade.result
@@ -36,7 +36,7 @@ Phase 4/4: Running 'mysql_fix_privilege_tables'
 OK
 Run it again - should say already completed
 This installation of MySQL is already upgraded to VERSION, use --force if you still need to run mysql_upgrade
-Force should run it regardless of wether it's been run before
+Force should run it regardless of whether it has been run before
 Phase 1/4: Fixing views
 Phase 2/4: Fixing table and database names
 Phase 3/4: Checking and upgrading tables
diff --git a/mysql-test/t/mysql_upgrade.test b/mysql-test/t/mysql_upgrade.test
index 13be03a..0b9e143 100644
--- a/mysql-test/t/mysql_upgrade.test
+++ b/mysql-test/t/mysql_upgrade.test
@@ -19,7 +19,7 @@ file_exists $MYSQLD_DATADIR/mysql_upgrade_info;
 # It should have created a file in the MySQL Servers datadir
 file_exists $MYSQLD_DATADIR/mysql_upgrade_info;
 
---echo Force should run it regardless of wether it's been run before
+--echo Force should run it regardless of whether it has been run before
 --exec $MYSQL_UPGRADE --force 2>&1
 
 # It should have created a file in the MySQL Servers datadir


More information about the commits mailing list