266835 - Sends wrong FTP password for anonymous:userpasswd@server

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Mike Hommey [:glandium]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20041028 Firefox/1.0RC1 (Debian package 0.99.0+1.0RC1-1)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20041028 Firefox/1.0RC1 (Debian package 0.99.0+1.0RC1-1)

[Bug first reported here: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=226784 ]

Copy-Paste from the original report:
I recently discovered that this program has a bug with sending the
correct password specified by user using the server:passwd syntax if the
username is "anonymous". I know that having a password for anonymous FTP
user is a bit unusual, though the program should work correctly in this
case and not blindly ignore what the user has entered and send some
other password string instead.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Frank Wein [:mcsmurf]]

Attachment: Patch

Note: I don't know if this patch is correct or perfect or if it complies with
the RfCs, but it's a patch that should fix this bug ;-)!

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Note: I don't know if this patch is correct

It's not.  It leaks anonPassword when mPassword is set.  Switch to nsXPIDLCString?

Other than that, looks ok...

Comment 3

[Frank Wein [:mcsmurf]]

Attachment: Patch 2

Comment 4

[Frank Wein [:mcsmurf]]

Attachment: Patch 3

Comment 5

[Mike Hommey [:glandium]]

The last patch doesn't work on firefox 1.0rc2:
nsFtpConnectionThread.cpp: In member function `nsresult nsFtpState::S_pass()':
nsFtpConnectionThread.cpp:1067: error: `AppendLiteral' undeclared (first use
   this function)
nsFtpConnectionThread.cpp:1067: error: (Each undeclared identifier is reported
   only once for each function it appears in.)

I guess changing AppendLiteral by Append does the trick.

Comment 6

[Frank Wein [:mcsmurf]]

(In reply to comment #5)
> The last patch doesn't work on firefox 1.0rc2:
[...]
Yes, this patch was never intended to work with Firefox. But yes, if this ever
should go into Aviary branch (for FF 1.0.1 then?) AppendLiteral can be changed
to  Append.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Frank, anonPassword is not a literal, so you can't call AppendLiteral on it.  On
modern enough compilers that simply won't build.

You want to call AppendASCII if you're sure that the string is ASCII.  In this
case, I doubt it has to be ascii.  In fact, it's worth checking how it's stored;
you may need to get it as a WString and do a UTF16-to-UTF8 conversion or
something...

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

>You want to call AppendASCII if you're sure that the string is ASCII.  In this
>case, I doubt it has to be ascii. 

it comes from prefs. GetCharPref uses ASCII. (it may work with UTF-8, but its
interface only guarantees ASCII)

I doubt it's worth getting a wstring out of prefs here... behaviour of nonascii
ftp passwords is not really defined anyway, afaik...

Comment 9

[Frank Wein [:mcsmurf]]

Attachment: Patch 4 (diff -uw)

Comment 10

[Frank Wein [:mcsmurf]]

Attachment: Patch 4 (diff -u)

Comment 11

[Frank Wein [:mcsmurf]]

Comment on attachment 164794 [details] [diff] [review]
Patch 4 (diff -uw)

r=biesi per irc

Comment 12

[Frank Wein [:mcsmurf]]

Patch 4 (diff -u) has been checked in:
2004-11-06 08:57	bmlk%gmx.de 	mozilla/ netwerk/ protocol/ ftp/ src/
nsFtpConnectionThread.cpp 	1.290 	19/16  	send user/pref specified password if
specified instead of default password for anonymous ftp, bug 266835 p= Frank
Wein, r=biesi, sr=darin"

Comment 13

[Mike Hommey [:glandium]]

Could you provide a patch for firefox ?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

this patch also fixes firefox.

Comment 15

[Mike Hommey [:glandium]]

No, it doesn't, because there's no AppendLiteral nor AppendASCII in firefox.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

there is AppendASCII and AppendLiteral in firefox, since firefox uses the same
gecko as mozilla.


now should you mean the aviary 1.0 branch, indeed, there is neither of those
there. you can use Append. to my knowledge, on that branch only crash and
security fixes are accepted at this point, though. (according to
http://blog.ebrahim.org/archives/2004/11/06/firefox_10_on_track_relnote_nomination.php)

please stop reopening bugs just for branch landings.

Comment 17

[[e-mail]]

Comment on attachment 164794 [details] [diff] [review]
Patch 4 (diff -uw)

>+                passwordStr.AppendASCII(anonPassword);
Why are we using AppendASCII on nsCSubtrings?

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #17)
> (From update of attachment 164794 [details] [diff] [review])
> >+                passwordStr.AppendASCII(anonPassword);
> Why are we using AppendASCII on nsCSubtrings?

why not? :-)  although now that you mention it, why do we have it on non-wide
strings?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So that wide strings and 8-bit strings have the same API

Comment 20

[Josh Birnbaum]

*** Bug 292600 has been marked as a duplicate of this bug. ***