code suggestion

Question

Hello all,

I find this code in the FtpConnection.Login() method:

public virtual void Login(string user, string password)
{
CheckConnection(true);

FTPReply reply = control.SendCommand("USER " + user);

// we allow for a site with no password - 230 response
string[] validCodes = new string[]{"230", "331"};
lastValidReply = control.ValidateReply(reply, validCodes);
if (lastValidReply.ReplyCode.Equals("230"))
return ;
else
{
Password(password);
}
}

Strictly, this code is incorrect. If there is no password set on the server, but a (obviously incorrect) password is provided to this method, it will login anyway. Better code would in my opinion be:

if (lastValidReply.ReplyCode.Equals("230") && (password == null || password.Length < 1))

Keep up the good work.

Kind regards,

Davidvl

Answer 1

This is a bit of a grey area. You might be right, but if we made the change you suggest then it could well break existing user's code, so we're probably going to have to leave it.

- Hans (EDT)

Answer 2

This is a bit of a grey area. You might be right, but if we made the change you suggest then it could well break existing user's code, so we're probably going to have to leave it.

- Hans (EDT)

Hans, I disagree. Although it might not be a real security bug, it's a fact that an end user can get access to a system without passing the correct credentials. That should not be the case.
Strictly, this code suggestion would not break 'users code' since only the end user would know whether the result (logged in or not) is really(!) correct. If it breaks end user experience (what you are targeting at I think) then this user is passing incorrect credentials which should result in -not- being logged in.

David