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

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

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.

> +  char password[MAX_LEN_NAME];

IMHO should a connection have a pointer to a struct user. This field
should then go.

> +    /* 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.

> +    /* used to follow where the connection is in the authentication process 
> */
> +    enum conn_status status;

IMHO should the field have an "auth" in its name -> "auth_status".

> +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?

> +struct packet_authentication_request {

> +  int type;

You should use the enum from above here.

> +  char message[MAX_LEN_MSG]; /* explain to the client if there's a problem */
> +};

> 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.

> +#define GUEST_NAME "guest"

> +#define GUEST_NAME_LEN 5

What is wrong with strlen("guest")?

> +#define MIN_PASSWORD_LEN  6  /* minimum length of password */
> +#define MIN_PASSWORD_CAPS 0  /* minimum number of capital letters required */
> +#define MIN_PASSWORD_NUMS 0  /* minimum number of numbers required */

>  /**************************************************************************
> -Returns 0 if the clients gets rejected and the connection should be
> -closed. Returns 1 if the client get accepted.
> + Returns 0 if the clients gets rejected and the connection should be
> + closed. Returns 1 if the client get accepted.

s/0/FALSE/
s/1/TRUE/

> +    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".

> +/**************************************************************************
> +  Receives a password from a client and verifies it.
> +**************************************************************************/
> +bool handle_authentication_reply(struct connection *pconn,
> +                                 struct packet_authentication_reply *packet)
> +{
> +#ifdef AUTHENTICATION_ENABLED 
> +  char msg[MAX_LEN_MSG];
> +
> +  if (pconn->server.status == CS_REQUESTING_NEW_PASS) {
> +    struct user user;
> +
> +    /* check if the new password is acceptable */
> +    if (!is_good_password(packet->password, msg)) {
> +      if (pconn->server.authentication_tries >= MAX_AUTHENTICATION_TRIES) {
> +        reject_new_connection(_("Sorry, too many wrong tries..."), pconn);
> +      } else {
> +        struct packet_authentication_request request;
> +
> +        request.type = AUTH_BAD;
> +        sz_strlcpy(request.message, msg);
> +        send_packet_authentication_request(pconn, &request);
> +        pconn->server.authentication_tries++; /* increase tries */
> +      }
> +      return TRUE;
> +    }
> +
> +    /* the new password is good, create a database entry for
> +     * this user; we establish the connection in handle_db_lookup */
> +    sz_strlcpy(user.name, pconn->username);
> +    sz_strlcpy(user.password, packet->password);
> +
> +    switch(user_db_save(&user)) {
> +    case USER_DB_SUCCESS:
> +      break;
> +    case USER_DB_ERROR:
> +      notify_conn(&pconn->self, _("Warning: There was an error in saving "
> +                                  "to the database. Continuing, but your "
> +                                  "stats will not be saved."));
> +      freelog(LOG_ERROR, "Error writing to database for %s", 
> pconn->username);
> +      break;
> +    default:
> +      assert(0);
> +    }
> +
> +    establish_new_connection(pconn);
> +  } else if (pconn->server.status == CS_REQUESTING_OLD_PASS) { 
> +    if (strncmp(pconn->password, packet->password, MAX_LEN_NAME) == 0) {
> +      establish_new_connection(pconn);
> +    } else {
> +      if (pconn->server.authentication_tries >= MAX_AUTHENTICATION_TRIES) {
> +        reject_new_connection(_("Sorry, too many wrong tries..."), pconn);
> +      } else {
> +        struct packet_authentication_request request;
> +
> +        request.type = AUTH_WRONG;
> +        sz_strlcpy(request.message,
> +                   _("Your password is incorrect. Try again."));
> +        send_packet_authentication_request(pconn, &request);
> +        pconn->server.authentication_tries++; /* increase tries */
> +      }
> +    }
> +  } else {
> +    freelog(LOG_ERROR, "%s's sending bad auth packets", pconn->username);
> +  }
> +
> +  return TRUE;
> +#else
> +  return FALSE;
> +#endif /* AUTHENTICATION_ENABLED */

Since the check for
  if (pconn->server.authentication_tries >= MAX_AUTHENTICATION_TRIES) {
is quite universal I would move it at the top.


> +/**************************************************************************
> + Verifies that a password is valid. Does some [very] rudimentary safety 
> + checks. TODO: do we want to frown on non-printing characters?
> + Fill the msg (length MAX_LEN_MSG) with any worthwhile information that 
> + the client ought to know. 
> +**************************************************************************/
> +static bool is_good_password(const char *password, char *msg)
> +{
> +  int i, num_caps = 0, num_nums = 0;
> +   
> +  /* check password length */
> +  if (strlen(password) < MIN_PASSWORD_LEN) {
> +    my_snprintf(msg, MAX_LEN_MSG,
> +                _("Your password is too short, the minimum length is %d. "
> +                  "Try again."), MIN_PASSWORD_LEN);
> +    return FALSE;
> +  }
> + 
> +  my_snprintf(msg, MAX_LEN_MSG,
> +              _("The password must have at least %d capital letters, %d "
> +                "numbers, and be at minimum %d [printable] characters long. "
> +                "Try again"), 
> +              MIN_PASSWORD_CAPS, MIN_PASSWORD_NUMS, MIN_PASSWORD_LEN);
> +
> +  for (i = 0; i < strlen(password); i++) {
> +    if (my_isupper(password[i])) {
> +      num_caps++;
> +    }
> +    if (my_isdigit(password[i])) {
> +      num_nums++;
> +    }
> +  }
> +
> +  /* check number of capital letters */
> +  if (num_caps < MIN_PASSWORD_CAPS) {
> +    return FALSE;
> +  }
> +
> +  /* check number of numbers */
> +  if (num_nums < MIN_PASSWORD_NUMS) {
> +    return FALSE;
> +  }

> +  if (!is_sane_name(password)) {
> +    return FALSE;
> +  }

Since this is the most basic check it should be done first.

> --- snap/server/userdb/lockfile.c     1969-12-31 18:00:00.000000000 -0600
> +++ snap-a6/server/userdb/lockfile.c  2003-05-17 22:38:29.000000000 -0500

> +#define HAS_FILE_LOCKING /* FIXME: use configure here! */
> +#define HAS_FLOCK        /* FIXME: use configure here! */
> +#ifdef HAS_FLOCK /* FIXME: need a check for sys/file.h */

No comment ;)

> +static int fd;

This variable is set but never used.

> +/**************************************************************************
> + attempt to create a lock, so that other servers aren't accessing the 
> + database when we are. return TRUE for success, FALSE for failure
> + N.B. This is a simple locking mechanism: it for example does not check for
> + stale lock files. This means that if the application crashes in between 
> + lock creation and removal, you'll get a dangling lock file (this could 
> + easily happen if you were to corrupt the database. The registry functions
> + will bomb and quit the server). Perhaps this warrants a FIXME.
> +**************************************************************************/
> +bool create_lock(void)
> +{
> +#ifdef HAS_FILE_LOCKING
> +  int i;
> +#ifndef HAS_FLOCK
> +  struct flock fl = { F_WRLCK, SEEK_SET, 0, 0, 0 };
> +
> +  fl.l_pid = getpid();
> +#endif
> +
> +  /* 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.

> +/**************************************************************************
> + loads a user from the database
> +**************************************************************************/
> +enum userdb_status user_db_load(struct user *puser)
> +{
> +  struct section_file sf;
> +  int i, num_users = 0;
> +
> +  if (!create_lock()) {
> +    return USER_DB_ERROR;
> +  }
> +
> +  /* load the file */
> +  if (!section_file_load(&sf, FC_USER_DATABASE)) {
> +    remove_lock();
> +    return USER_DB_NOT_FOUND;
> +  } else {
> +    num_users = secfile_lookup_int(&sf, "db.user_num");
> +  }
> +
> +  /* find the user in the database */
> +  for (i = 0; i < num_users; i++) {
> +    char *name = secfile_lookup_str(&sf, "db.users%d.name", i);
> +    if (strncmp(name, puser->name, MAX_LEN_NAME) == 0) {
> +      break;
> +    }
> +  }
> +
> +  /* if we couldn't find the user, return, else, fill the user struct */
> +  if (i == num_users) {
> +    section_file_free(&sf);
> +    remove_lock();
> +    return USER_DB_NOT_FOUND;
> +  } else {
> +    fill_user(&sf, puser, i);
> +
> +    section_file_free(&sf);
> +    remove_lock();
> +    return USER_DB_SUCCESS;
> +  }
> +}

This can be better writted using goto:

enum userdb_status user_db_load(struct user *puser)
{
 struct section_file sf;
 int i, num_users = 0;
 enum userdb_status result;

 if (!create_lock()) {
   result = USER_DB_ERROR;
   goto out;
 }

 /* load the file */
 if (!section_file_load(&sf, FC_USER_DATABASE)) {
   result = USER_DB_NOT_FOUND;
   goto out2;
 }

 num_users = secfile_lookup_int(&sf, "db.user_num");

 /* find the user in the database */
 for (i = 0; i < num_users; i++) {
   char *name = secfile_lookup_str(&sf, "db.users%d.name", i);
   if (strncmp(name, puser->name, MAX_LEN_NAME) == 0) {
     break;
   }
 }

 /* if we couldn't find the user, return, else, fill the user struct */
 if (i == num_users) {
  result = USER_DB_NOT_FOUND;
 } else {
  fill_user(&sf, puser, i);
  result = USER_DB_SUCCESS;
 }
 section_file_free(&sf);

out2:
  remove_lock();
out:
 return result;
}


> +/**************************************************************************
> + saves a user to the database. if the user already exists, replace the data.
> +**************************************************************************/
> +enum userdb_status user_db_save(struct user *puser)

Same here.

> +/**************************************************
> + The user db statuses are:
> +
> + 1: an error occurred, possibly we couldn't access the database file.
> + 2: we were able to successfully insert an entry. or we found the entry 
> +    we were searching for
> + 4: the user we were searching for was not found.

These numbers look odd.

> +enum userdb_status {
> +  USER_DB_ERROR = 1,
> +  USER_DB_SUCCESS,
> +  USER_DB_NOT_FOUND
> +};

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 Make a software that is foolproof, and only fools will want to use it.