[Freeciv-Dev] Re: client/server authentication (PR#1767)

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

On Thu, May 22, 2003 at 11:59:07AM -0700, Mike Kaufman wrote:
> On Thu, May 22, 2003 at 05:22:49AM -0700, Raimar Falke wrote:
> > On Sat, May 17, 2003 at 08:51:27PM -0700, Mike Kaufman wrote:
> > > my bad, shoulda known that a capability was going to be added in the mean
> > > time, here's an updated patch. sorry for the bandwidth.
> > 
> > This is only a patch review. I made no actual testing.
> > 
> > > +static enum {
> > > +  LOGIN_TYPE,
> > > +  NEW_PASSWORD_TYPE,
> > > +  VERIFY_PASSWORD_TYPE,
> > > +  ENTER_PASSWORD_TYPE
> > > +} dialog_config;
> > 
> > These are duplicated in the GUI code.
> 
> and...? we don't have a connectdlg_common.c yet, and 

> I'm not going to make one for this triviality

Why not?

>. Maybe when the new connectdlg shows up.

> > > +  char password[MAX_LEN_NAME];
> > 
> > IMHO should a connection have a pointer to a struct user. This field
> > should then go.
> 
> no. I thought about this. This would require that user.h be in common and
> that is not right as the client has absolutely nothing to do with that
> struct. The client should not have access to it.

Not at all. You don't have to expose the content of struct user if you
use a pointer.

> > > +    /* Holds number of tries for authentication from client. */
> > > +    int authentication_tries;
> > 
> > If you limit this you also have to think about a real rate
> > limiting. So what stops an attacker issuing another connection after
> > his 3 tries are over? IMHO we should rate limit the connection per
> > time from one IP. To one connection per 2 sec for example. This
> > prevents bture force attacks against the servers.
> 
> No. This problem is orthogonal to this patch. You have the same problem
> in the current code (with probably even more overhead).

Look: why do you have added is_good_password? Because this makes
guessing the password harder. Or in other terms: it increases the
search space.

The time to break a password is:

            number of possible passwords
 time =    ----------------------------------------------------------
            guessing speed (how many passwords can be tried per sec)

You increased the numerator with is_good_password. But this doesn't
help if we also don't limit the denominator to a low value.

> > > +enum authentication_type {
> > > +  AUTH_NORMAL,   /* request a password for a returning user */
> > > +  AUTH_NEW,      /* request a password for a new user */
> > 
> > > +  AUTH_BAD,     /* inform the client that the new password was bad */
> > > +  AUTH_WRONG,   /* inform the client that the entered password was wrong 
> > > */
> > 
> > Why can't we use normal messages for these?
> 
> because the client is expected to take some kind of configuration action in
> response to this which can't be handled my a text message.

I see.

> > > diff -Nur -Xsnap/diff_ignore snap/server/connecthand.c 
> > > snap-a6/server/connecthand.c
> > > --- snap/server/connecthand.c     2003-05-03 22:20:18.000000000 -0500
> > > +++ snap-a6/server/connecthand.c  2003-05-17 22:38:29.000000000 -0500
> > 
> > > +#define NEW_USERS_ALLOWED  /* if defined, new users may log in. */
> > > +#define GUESTS_ALLOWED     /* if defined, users can log in as guests */
> > 
> > These settings are hidden quite nice in this file. I think that these
> > two settings and the settings below should be server variables.
> 
> sure, but there is a perpetual argument over new server options are you are
> advocating five (or at least 3) new ones in a single stroke. Until a better
> system in stdinhand is worked out, the answer will be: not in this patch.

I agree that we need some improvement. But look at this problem from
another angle: if we add now the auth options and some other options
in the future we may increase the pressure to a range where people
really start working on this area. So far everybody agreed that it
needs fixing but is low priority. At least this is my impression.

> > > +    switch(user_db_load(&user)) {
> > > +    case USER_DB_ERROR:
> > > +#ifdef GUESTS_ALLOWED
> > > +      sz_strlcpy(tmpname, pconn->username);
> > > +      get_unique_guest_name(tmpname); /* do not pass pconn->username 
> > > here! */
> > > +      sz_strlcpy(pconn->username, tmpname);
> > > +
> > > +      freelog(LOG_ERROR, "Error reading database; connection -> guest");
> > > +      notify_conn(&pconn->self, _("There was an error reading the user "
> > > +                                  "database, logging in as guest 
> > > connection "
> > > +                                  "'%s'."), pconn->username);
> > > +      establish_new_connection(pconn);
> > > +#else
> > > +      reject_new_connection(_("There was an error reading the user 
> > > database "
> > > +                            "and guest logins are not allowed. Sorry"), 
> > > pconn);
> > > +#endif /* GUESTS_ALLOWED */
> > > +      break;
> > 
> > I would just let the server answer "error, please try guest".
> 
> uh, this won't work if guest logins aren't allowed...

And the server will answer "you lost ;)" if if guest logins aren't
allowed.

> > > +/**************************************************************************
> > > +  Receives a password from a client and verifies it.
> > > +**************************************************************************/
> > > +bool handle_authentication_reply(struct connection *pconn,
> > 
> > Since the check for
> >   if (pconn->server.authentication_tries >= MAX_AUTHENTICATION_TRIES) {
> > is quite universal I would move it at the top.
> 
> Are you actually reading the patch Raimar? There are five or six paths
> through this function are only two of them require that check...

I know. But does it harm if we add this check to all code paths? IMHO
not.

> > > +static int fd;
> > 
> > This variable is set but never used.
> 
> are you kidding?! Oh, you're not kidding. 

> Look at the definitions for LOCK_CMD and UNLOCK_CMD.

Uhhh I overlooked these.

> There is a bug though and that is that it should be instead:
> 
> #ifdef HAS_FILE_LOCKING
> static int fd;
> #endif

Yes.

> > > +  /* open the file */
> > 
> > > +  if ((fd = open(FC_LOCK_FILE, O_RDWR|O_CREAT, S_IRUSR|S_IWUSR)) == -1) {
> > 
> > If you don't use O_EXCL here you can use fopen IMHO.
> 
> you know how flock and/or fnctl works right? They kinda require a file
> descriptor...

fileno() will yield this. I'm not sure if this is more portable than
S_IRUSR. Just choose the one which has less configure problems.

> > This can be better writted using goto:
> 
> barely.

Why not?

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  "brand memory are for windows users that think their stability
   problems come from the memory"
    -- bomek in #freeciv