Bug 131088

Summary: mc shows ftp password in plain text
Product: [Fedora] Fedora Reporter: George Toft <george>
Component: mcAssignee: Jindrich Novy <jnovy>
Status: CLOSED RAWHIDE QA Contact:
Severity: medium Docs Contact:
Priority: medium    
Version: 3CC: leonard-rh-bugzilla, pknirsch, samoilov
Target Milestone: ---Keywords: Security
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 4.6.1-0.9 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2004-11-08 12:57:51 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
This patch prevents mc displaying user's ftp password when uploading.
none
This patch prevents mc displaying user's ftp password when uploading.
none
The patch tells mc not to display password in path in copy dialog, delete, error dialogs, etc.
none
the improved version of strippwd patch
none
add_new_entry_cmd() without memory leaks.
none
the strippwd patch with the fix of possible memleak none

Description George Toft 2004-08-27 13:45:01 UTC
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040625

Description of problem:
While performing FTP upload to remote server, the remote credentials
are displayed in clear text.

Version-Release number of selected component (if applicable):


How reproducible:
Always

Steps to Reproduce:
1.  Launch mc
2.  [F9] - select "FTP link..."
3.  Enter username:password@remotehost information
4.  Select file to upload
5.  View remote host credentials
    

Actual Results:  This was taken during an actual upload.  The filename
and credentials have been changed and are not valid.

################################### Copy
###################################
#                                                                    
     #
#   Source   msjavx.html                                             
     #                               
#   Target  
/#ftp:joeuser:mypassword@ftp.georgetoft.com/FTP/msjavx.html   #
#                                                                    
     #
#   File     [                                        ]  62% ETA
0:00.45   #
#   Bytes    [                                        ]  81% 43.83
KB/s    #
#   Count    [                                        ]  67%         
     #
#                                                                    
     #
#                  [ Skip ]                     [ Abort ]            
     #
#                                                                    
     #
############################################################################


Expected Results:  The remote host password (and preferably the
username) should NOT be displayed.

Additional info:

Comment 1 Jindrich Novy 2004-09-08 13:03:21 UTC
Hi George, the following patch should help you to get rid of this issue.

Jindrich

Comment 2 Jindrich Novy 2004-09-08 13:05:51 UTC
Created attachment 103581 [details]
This patch prevents mc displaying user's ftp password when uploading.

This patch prevents mc displaying user's ftp password when uploading.

Comment 3 Jindrich Novy 2004-09-08 13:06:00 UTC
Created attachment 103582 [details]
This patch prevents mc displaying user's ftp password when uploading.

This patch prevents mc displaying user's ftp password when uploading.

Comment 4 Jindrich Novy 2004-09-14 14:10:34 UTC
Created attachment 103826 [details]
The patch tells mc not to display password in path in copy dialog, delete, error dialogs, etc.

The patch is tested, please apply.

Comment 5 Leonard den Ottolander 2004-10-06 00:54:11 UTC
Could you also get a patch like this for the directory hotlist?


Comment 6 Jindrich Novy 2004-10-06 10:53:12 UTC
I improved the patch a little bit and consulted it with Jakub.
Ok, let me check it.

Comment 7 Leonard den Ottolander 2004-10-06 17:15:02 UTC
Jindrich, could you propose these patches on the mc-devel at gnome dot
org list? That's probably easier. I've already started a thread on the
issue.


Comment 8 Leonard den Ottolander 2004-10-06 17:32:57 UTC
Please mark patches as obsolete when appropriate.


Comment 9 Leonard den Ottolander 2004-10-06 17:40:50 UTC
Please compare
http://mail.gnome.org/archives/mc-devel/2004-August/msg00167.html .

Comment 10 Andrew V. Samoilov 2004-10-06 23:46:44 UTC
strip_password_only() can write to const *char path in this
implementation.  It can be fixed:

new_path = g_strdup_printf ("%.*s%s", at - col, path, at);

Also, upstream supports username with @ in it, so this function has to
be reworked to check against last @ before /.

Comment 11 Jindrich Novy 2004-10-07 08:59:43 UTC
Andrew, thanks for the workaround.

Leonard, that's a good idea, I'll play with the patches a bit more and
do the proposal.


Comment 12 Jindrich Novy 2004-10-08 13:03:45 UTC
The improved version of the .strippwd patch fixes showing passwords in
hotlist and error messages in case chdir fails in subshell, etc.

With Jakub's help I modified strip_password in the way that
strip_password_only () is no more needed.

Leonard, the version of strip_passwd () is different from fix
mentioned in mc-devel. With Jakub we concluded that our implementation
that doesn't modify the input path at all is a bit better.



Comment 13 Jindrich Novy 2004-10-08 13:06:18 UTC
Created attachment 104935 [details]
the improved version of strippwd patch

Note that it uses mbstrlen () instead of strlen ().

Comment 14 Andrew V. Samoilov 2004-10-12 15:31:22 UTC
Memory leaks, look for label_string.
Also it seems there is one more typo in util.c: s/Cavevat/Caveat/
BTW, can you consult us about use of functions from GPLed src/util.c
in the LGPLed vfs/*.c ? Is it legal?

Comment 15 Jindrich Novy 2004-10-15 12:38:16 UTC
Hi Andrew,

I see no memory leak in function add2hotlist_cmd () regarding
label_string. name_trunc () prints to static string so there's no need
to g_free(). There's also no problem with input_dialog ().
label_string is not g_free()d because it's done so in remove_group ()
called by done_hotlist (). If I missed something, can you please
specify where exactly is the leak? 

Well, the vfs/*.c is licensed under LGPL because it used gnome-vfs
some time ago but as I had a discussion on the topic with Jakub,
there's no legal problem to use LGPLed source from GPLed programs. The
license may change back to GPL if all the authors agree, what is
rather rare case...

Comment 16 Andrew V. Samoilov 2004-10-15 14:29:21 UTC
Hi, Jindrich

label_string is leaked if input_dialog() returns NULL or empty string.
In last case this memory (g_strdup("")) is also leaked.

About our legal issue: we have reverse case.  Now LGPLed VFS sources use
GPLed functions from src/util.c and Roland Illig points this as a problem.


Comment 17 Andrew V. Samoilov 2004-10-15 16:44:59 UTC
Created attachment 105280 [details]
add_new_entry_cmd() without memory leaks.

quick_dialog() and so add_new_entry_cmd() returns malloc()ed title and url.
BTW platform should be changed to All.

Comment 18 Leonard den Ottolander 2004-10-27 13:06:55 UTC
Jindrich, can we fix this quickly so it can be committed before the
release of 4.6.1? TIA.


Comment 19 Leonard den Ottolander 2004-10-27 13:13:12 UTC
Please see http://www.pavelsh.pp.ru/wiki/doku.php?id=mc-bugs , "hide
FTP password in various places" for references to existing patches.


Comment 20 Jindrich Novy 2004-10-27 14:05:57 UTC
Leonard, sure, I'll have look at it.

Comment 21 Jindrich Novy 2004-10-27 15:51:21 UTC
Created attachment 105847 [details]
the strippwd patch with the fix of possible memleak

So finally I had a time to fix the possible memory leak Andrew pointed out.

Andrew, have you posted your add_new_entry_cmd() patch also to mc-devel?

Comment 22 Jindrich Novy 2004-11-08 12:57:51 UTC
Hi,

The significant part of the patch was applied upstream and
mc-4.6.1-0.9 shouldn't have problems with showing ftp passwords in
URLs any more.