Re: Status of FTP password hiding patches

[Jindrich Novy <jnovy redhat com>]

On Tue, 2004-10-26 at 14:04, Leonard den Ottolander wrote:
> I would like to know what the status of the various FTP password hiding
> patches is. Together with the quoting issues in vfs/extfs I believe this
> to be the only issue that needs to be fixed before the release of 4.6.1.

Hi,

I fixed the possible memory leak regarding label_string found by Andrew
V. Samoilov, see bug 131088 in Red Hat bugzilla. To test this patch,
create an user "test" for example, set his password to "testpass",
change your cwd by "cd /#ftp:test:testpass localhost" and:

- try to upload a file to this directory and note the password is
removed. Copy, error and other dialogs display "/#ftp:test localhost"

- try to add the new ftp directory to directory hotlist, password will
be also removed

- try to find any dialog where the password is displayed

I tested it and was unable to find any dialog that still shows a
password in it. The patch is cleanly applicable to 20th Oct mc CVS
snapshot.

HTH,
Jindrich

-- 
Jindrich Novy <jnovy redhat com>, http://people.redhat.com/jnovy/

--- mc-4.6.1-20041020/src/subshell.c.strippwd	2004-10-27 17:05:35.366429216 +0200
+++ mc-4.6.1-20041020/src/subshell.c	2004-10-27 17:05:35.370428608 +0200
@@ -788,9 +788,12 @@ do_subshell_chdir (const char *directory
     feed_subshell (QUIETLY, FALSE);
 
     if (subshell_alive && strcmp (subshell_cwd, current_panel->cwd)
-	&& strcmp (current_panel->cwd, "."))
+	&& strcmp (current_panel->cwd, ".")) {
+	char *cwd = strip_password (g_strdup (current_panel->cwd), 1);
 	fprintf (stderr, _("Warning: Cannot change to %s.\n"),
-		 current_panel->cwd);
+		 cwd);
+	g_free (cwd);
+    }
 
     if (reset_prompt)
 	prompt_pos = 0;
--- mc-4.6.1-20041020/src/util.c.strippwd	2004-10-27 17:05:35.349431800 +0200
+++ mc-4.6.1-20041020/src/util.c	2004-10-27 17:05:35.372428304 +0200
@@ -423,6 +423,22 @@ name_trunc (const char *txt, int trunc_l
     return x;
 }
 
+/*
+ * path_trunc() is the same as name_trunc() above but
+ * it deletes possible password from path for security
+ * reasons.
+ */
+const char *
+path_trunc (const char *path, int trunc_len) {
+    const char *ret;
+    char *secure_path = strip_password (g_strdup (path), 1);
+    
+    ret = name_trunc (secure_path, trunc_len);
+    g_free (secure_path);
+    
+    return ret;
+}
+
 const char *size_trunc (double size)
 {
     static char x [BUF_TINY];
@@ -604,29 +620,29 @@ strip_password (char *p, int has_prefix)
     
     for (i = 0; i < sizeof (prefixes)/sizeof (prefixes[0]); i++) {
 	char *q;
+	size_t host_len;
 
 	if (has_prefix) {
 	    if((q = strstr (p, prefixes[i].name)) == 0)
 	       continue;
             else
 	        p = q + prefixes[i].len;
-       	};
+       	}
 
         if ((dir = strchr (p, PATH_SEP)) != NULL)
-   	    *dir = '\0';
+	    host_len = dir - p;
+	else
+	    host_len = strlen (p);
+	
         /* search for any possible user */
-        at = strrchr (p, '@');
+	at = memchr (p, '@', host_len);
 
         /* We have a username */
         if (at) {
-            *at = 0;
-            inner_colon = strchr (p, ':');
-  	    *at = '@';
+	    inner_colon = memchr (p, ':', at - p);
             if (inner_colon)
-                strcpy (inner_colon, at);
+		memmove (inner_colon, at, strlen(at) + 1 );
         }
-        if (dir)
-	    *dir = PATH_SEP;
 	break;
     }
     return (result);
--- mc-4.6.1-20041020/src/filegui.c.strippwd	2004-10-27 17:05:35.309437880 +0200
+++ mc-4.6.1-20041020/src/filegui.c	2004-10-27 17:05:35.374428000 +0200
@@ -68,6 +68,7 @@
 #include "fileopctx.h"		/* FILE_CONT */
 #include "filegui.h"
 #include "key.h"		/* get_event */
+#include "util.h"		/* strip_password() */
 #include "tty.h"
 
 /* }}} */
@@ -424,7 +425,8 @@ file_progress_show_bytes (FileOpContext 
 
 /* }}} */
 
-#define truncFileString(ui, s) name_trunc (s, ui->eta_extra + 47)
+#define truncFileString(ui, s)       name_trunc (s, ui->eta_extra + 47)
+#define truncFileStringSecure(ui, s) path_trunc (s, ui->eta_extra + 47)
 
 FileProgressStatus
 file_progress_show_source (FileOpContext *ctx, const char *s)
@@ -473,7 +475,7 @@ file_progress_show_target (FileOpContext
 
     if (s != NULL) {
 	label_set_text (ui->file_label[1], _("Target"));
-	label_set_text (ui->file_string[1], truncFileString (ui, s));
+	label_set_text (ui->file_string[1], truncFileStringSecure (ui, s));
 	return check_progress_buttons (ctx);
     } else {
 	label_set_text (ui->file_label[1], "");
@@ -495,7 +497,7 @@ file_progress_show_deleting (FileOpConte
     ui = ctx->ui;
 
     label_set_text (ui->file_label[0], _("Deleting"));
-    label_set_text (ui->file_label[0], truncFileString (ui, s));
+    label_set_text (ui->file_label[0], truncFileStringSecure (ui, s));
     return check_progress_buttons (ctx);
 }
 
@@ -855,6 +857,7 @@ file_mask_dialog (FileOpContext *ctx, Fi
     int source_easy_patterns = easy_patterns;
     char *source_mask, *orig_mask, *dest_dir, *tmpdest;
     const char *error;
+    char *def_text_secure;
     struct stat buf;
     int val;
     QuickDialog Quick_input;
@@ -881,6 +884,9 @@ file_mask_dialog (FileOpContext *ctx, Fi
     fmd_widgets[FMCB22].result = &ctx->stable_symlinks;
     fmd_widgets[FMCB21].result = &ctx->dive_into_subdirs;
 
+    /* filter out a possible password from def_text */
+    def_text_secure = strip_password (g_strdup (def_text), 1);
+
     /* Create the dialog */
 
     ctx->stable_symlinks = 0;
@@ -894,7 +900,7 @@ file_mask_dialog (FileOpContext *ctx, Fi
     Quick_input.i18n = 1;
     Quick_input.widgets = fmd_widgets;
     fmd_widgets[FMDI0].text = text;
-    fmd_widgets[FMDI2].text = def_text;
+    fmd_widgets[FMDI2].text = def_text_secure;
     fmd_widgets[FMDI2].str_result = &dest_dir;
     fmd_widgets[FMDI1].str_result = &source_mask;
 
@@ -902,9 +908,11 @@ file_mask_dialog (FileOpContext *ctx, Fi
   ask_file_mask:
 
     if ((val = quick_dialog_skip (&Quick_input, SKIP)) == B_CANCEL) {
+        g_free(def_text_secure);
         g_free(def_text);
 	return 0;
     }
+    g_free(def_text_secure);
 
     if (ctx->follow_links)
 	ctx->stat_func = (mc_stat_fn) mc_stat;
--- mc-4.6.1-20041020/src/hotlist.c.strippwd	2004-10-27 17:05:35.278442592 +0200
+++ mc-4.6.1-20041020/src/hotlist.c	2004-10-27 17:43:44.652404848 +0200
@@ -761,7 +761,7 @@ add2hotlist (char *label, char *director
 			   /* should be inserted before first item */
 	new->next = current;
 	current_group->head = new;
-    } else if (pos == 1) { /* befor current */
+    } else if (pos == 1) { /* before current */
 	struct hotlist  *p = current_group->head;
 
 	while (p->next != current)
@@ -905,7 +905,8 @@ static void add_new_entry_cmd (void)
     int ret;
 
     /* Take current directory as default value for input fields */
-    title = url = current_panel->cwd;
+    url   = strip_password (g_strdup (current_panel->cwd), 1);
+    title = g_strdup (url);
 
     ret = add_new_entry_input (_("New hotlist entry"), _("Directory label"),
 			       _("Directory path"), "[Hotlist]", &title, &url);
@@ -919,6 +920,9 @@ static void add_new_entry_cmd (void)
 	add2hotlist (title, url, HL_TYPE_ENTRY, 1);
 
     hotlist_state.modified = 1;
+    
+    g_free (title);
+    g_free (url);
 }
 
 static int add_new_group_input (const char *header, const char *label, char **result)
@@ -1004,14 +1008,19 @@ void add2hotlist_cmd (void)
     char *prompt, *label;
     const char *cp = _("Label for \"%s\":");
     int l = mbstrlen (cp);
+    static char label_string[MC_MAXPATHLEN+1];
 
-    prompt = g_strdup_printf (cp, name_trunc (current_panel->cwd, COLS-2*UX-(l+8)));
-    label = input_dialog (_(" Add to hotlist "), prompt, current_panel->cwd);
+    strncpy (label_string, current_panel->cwd, MC_MAXPATHLEN);
+    label_string[MC_MAXPATHLEN] = '\0';
+    strip_password (label_string, 1);
+    
+    prompt = g_strdup_printf (cp, path_trunc (current_panel->cwd, COLS-2*UX-(l+8)));
+    label = input_dialog (_(" Add to hotlist "), prompt, label_string);
     g_free (prompt);
     if (!label || !*label)
 	return;
 
-    add2hotlist (label,g_strdup (current_panel->cwd), HL_TYPE_ENTRY, 0);
+    add2hotlist (label, g_strdup (label_string), HL_TYPE_ENTRY, 0);
     hotlist_state.modified = 1;
 }
 
--- mc-4.6.1-20041020/src/file.c.strippwd	2004-10-27 17:05:35.306438336 +0200
+++ mc-4.6.1-20041020/src/file.c	2004-10-27 17:05:35.379427240 +0200
@@ -1118,8 +1118,8 @@ move_file_file (FileOpContext *ctx, cons
 		msize = 40;
 	    msize /= 2;
 
-	    strcpy (st, name_trunc (s, msize));
-	    strcpy (dt, name_trunc (d, msize));
+	    strcpy (st, path_trunc (s, msize));
+	    strcpy (dt, path_trunc (d, msize));
 	    message (1, MSG_ERROR,
 			_(" `%s' and `%s' are the same file "), st, dt);
 	    do_refresh ();
@@ -1236,8 +1236,8 @@ move_dir_dir (FileOpContext *ctx, const 
 	    msize = 40;
 	msize /= 2;
 
-	strcpy (st, name_trunc (s, msize));
-	strcpy (dt, name_trunc (d, msize));
+	strcpy (st, path_trunc (s, msize));
+	strcpy (dt, path_trunc (d, msize));
 	message (1, MSG_ERROR,
 		    _(" `%s' and `%s' are the same directory "), st, dt);
 	do_refresh ();
@@ -2164,7 +2164,7 @@ int
 file_error (const char *format, const char *file)
 {
     g_snprintf (cmd_buf, sizeof (cmd_buf), format,
-		name_trunc (file, 30), unix_error_string (errno));
+		path_trunc (file, 30), unix_error_string (errno));
 
     return do_file_error (cmd_buf);
 }
@@ -2176,8 +2176,8 @@ files_error (const char *format, const c
     char nfile1[16];
     char nfile2[16];
 
-    strcpy (nfile1, name_trunc (file1, 15));
-    strcpy (nfile2, name_trunc (file2, 15));
+    strcpy (nfile1, path_trunc (file1, 15));
+    strcpy (nfile2, path_trunc (file2, 15));
 
     g_snprintf (cmd_buf, sizeof (cmd_buf), format, nfile1, nfile2,
 		unix_error_string (errno));
@@ -2198,7 +2198,7 @@ real_query_recursive (FileOpContext *ctx
 	      "   Delete it recursively? ")
 	    : _("\n   Background process: Directory not empty \n"
 		"   Delete it recursively? ");
-	text = g_strconcat (_(" Delete: "), name_trunc (s, 30), " ", (char *) NULL);
+	text = g_strconcat (_(" Delete: "), path_trunc (s, 30), " ", (char *) NULL);
 
 	if (safe_delete)
 	    query_set_sel (1);
--- mc-4.6.1-20041020/src/command.c.strippwd	2004-09-25 16:34:27.000000000 +0200
+++ mc-4.6.1-20041020/src/command.c	2004-10-27 17:05:35.380427088 +0200
@@ -179,8 +179,10 @@ void do_cd_command (char *cmd)
 	}
     } else
 	if (!examine_cd (&cmd [3])) {
+	    char *d = strip_password (g_strdup (&cmd [3]), 1);
 	    message (1, MSG_ERROR, _(" Cannot chdir to \"%s\" \n %s "),
-		     &cmd [3], unix_error_string (errno));
+		     d, unix_error_string (errno));
+	    g_free (d);
 	    return;
 	}
 }
--- mc-4.6.1-20041020/src/screen.c.strippwd	2004-10-27 17:05:35.364429520 +0200
+++ mc-4.6.1-20041020/src/screen.c	2004-10-27 17:05:35.383426632 +0200
@@ -2337,8 +2337,10 @@ panel_callback (WPanel *panel, widget_ms
 	current_panel = panel;
 	panel->active = 1;
 	if (mc_chdir (panel->cwd) != 0) {
+	    char *cwd = strip_password (g_strdup (panel->cwd), 1);
 	    message (1, MSG_ERROR, _(" Cannot chdir to \"%s\" \n %s "),
-		     panel->cwd, unix_error_string (errno));
+		     cwd, unix_error_string (errno));
+	    g_free(cwd);
 	} else
 	    subshell_chdir (panel->cwd);
 
--- mc-4.6.1-20041020/src/util.h.strippwd	2004-10-27 17:05:35.353431192 +0200
+++ mc-4.6.1-20041020/src/util.h	2004-10-27 17:05:35.385426328 +0200
@@ -28,6 +28,11 @@ char *fake_name_quote (const char *c, in
  * Return static buffer, no need to free() it. */
 const char *name_trunc (const char *txt, int trunc_len);
 
+/* path_trunc() is the same as name_trunc() above but
+ * it deletes possible password from path for security
+ * reasons. */
+const char *path_trunc (const char *path, int trunc_len);
+
 /* return a static string representing size, appending "K" or "M" for
  * big sizes.
  * NOTE: uses the same static buffer as size_trunc_sep. */