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

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

On Mon, May 05, 2003 at 07:16:45PM -0700, Mike Kaufman wrote:
> another patch.
> 
> o added a define so that you can compile the server such that guests
>   are not allowed to log in.
> o added a define so you can compile so that unregistered usernames 
>   are not allowed
> o added get_sane_name() at the end of is_good_password()
> o connect->Connect

Just some points. I didn't checked the GUI code.

> +#define MAX_AUTHENTICATION_TRIES 3

When is this reseted? When the server restarts? IMHO this allows a
nice DOS attack for the given user. Better would be to allow unlimited
tries but slow them down. 3s for a failed login for example.

>    struct conn_list self;     /* list with this connection as single element 
> */
>    char username[MAX_LEN_NAME];

> +  char password[MAX_LEN_NAME];

Do we really need to save this here? This just increases the chance
that it leaks in some way.

> +    /* used to prevent client from faking that it is a new user */
> +    bool is_old_user;

>  /*********************************************************
> + the server requests a password from the client
> +*********************************************************/
> +struct packet_authentication_request {
> +  int type;
> +  char message[MAX_LEN_MSG]; /* explain to the client if there's a problem */
> +};
> +
> +/*********************************************************
> + ... and the client replies.
> +*********************************************************/
> +struct packet_authentication_reply {
> +  int type;
> +  char password[MAX_LEN_NAME];
> +};

Why is the type needed for the reply? IMHO it isn't. Also "bool
is_old_user" should be changed to "enum auth_state auth_state" where
enum auth_state looks like

enum auth_state {
  NONE,
  JOINED,
  GUEST,
  REQUEST_SENT,
  AUTHED
};

and maybe some others. So you go from NONE to JOINED. And from JOINED
to GUEST or REQUEST_SENT. From REQUEST_SENT to AUTHED or you keep in
REQUEST_SENT if the wrong password was sent.

> +#undef NEW_USERS_DISALLOWED  /* if defined, new users may not log in. */
> +#undef GUESTS_DISALLOWED     /* if defined, users cannot log in as guests */

Please don't use double negation.

> @@ -189,7 +233,7 @@
>    } conn_list_iterate_end;
>  
>    freelog(LOG_NORMAL, _("Connection request from %s from %s"),
> -            req->username, pconn->addr);
> +          req->username, pconn->addr);

Looks like noise.

>    /* print server and client capabilities to console */
>    freelog(LOG_NORMAL, _("%s has client version %d.%d.%d%s"),
> @@ -229,6 +273,36 @@
>      return FALSE;
>    }
>  
> +#ifdef AUTHENTICATION_ENABLED 
> +  /* if authentication is enabled, we need an extra check as to whether
> +   * a connection can be established: the client must authenticate itself */
> +
> +  if (has_capability("auth", req->capability) 
> +      && !is_guest_name(req->username)) {
> +    struct user *puser = fc_malloc(sizeof(struct user)); /* must allocate */
> +
> +    sz_strlcpy(puser->name, req->username);
> +    sz_strlcpy(pconn->username, req->username);
> +    user_db_load(puser);
> +    return TRUE;

?? Who frees puser? You should check the result of user_db_load. What
is the purpose of this code anyway?

>  /**************************************************************************
> +  Receives an answer from the user database.
> +**************************************************************************/
> +void handle_db_lookup(struct user *puser, enum userdb_status status)

?? server code calls user_db_load in the user db which calls
handle_db_lookup in the server code?? What is kind of idea is this?

> +/**************************************************************************
> +  return a guest name
> +**************************************************************************/

> +static char *get_guest_name(void)

Add a const.

> +/**************************************************************************
> +  return a unique guest name

> +  WARNING: do not pass pconn->username to this function: it won't return!

Add "name is MAX_LEN_NAME bytes big".

It would even better if this function doesn't replace the name in place.

> +/**************************************************************************
> + 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 with any worthwhile information that the client ought to know. 

"msg is MAX_LEN_MSG long"

> +**************************************************************************/
> +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 "
> +                  "letters. Try again."), MIN_PASSWORD_LEN);
> +    return FALSE;
> +  }
> + 
> +  for (i = 0; i < strlen(password); i++) {

> +    if (password[i] == ' ') {

Please take a look at common/support.h:is*

> +      my_snprintf(msg, MAX_LEN_MSG,
> +                  _("Your password may not contain spaces. Try again."));
> +      return FALSE;
> +    }
> +
> +    if (password[i] >= 'A' && password[i] <= 'Z') {
> +      num_caps++;
> +    }
> +    if (password[i] >= '0' && password[i] <= '9') {
> +      num_nums++;
> +    }

You may also want to count the chars which aren't in a..z.

> -#define MAX_NUM_CONNECTIONS (MAX_NUM_PLAYERS+MAX_NUM_BARBARIANS)
> +#define MAX_NUM_CONNECTIONS (2 * (MAX_NUM_PLAYERS + MAX_NUM_BARBARIANS))

Reason?

> diff -Nur -Xsnap/diff_ignore snap/server/userdb/user.h 
> snap-a6/server/userdb/user.h
> --- snap/server/userdb/user.h 1969-12-31 18:00:00.000000000 -0600
> +++ snap-a6/server/userdb/user.h      2003-05-04 22:30:05.000000000 -0500
> @@ -0,0 +1,25 @@
> +/**********************************************************************
> + Freeciv - Copyright (C) 2003 - M.C. Kaufman
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 2, or (at your option)
> +   any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +***********************************************************************/
> +#ifndef FC__USER_H
> +#define FC__USER_H
> +
> +#include "shared.h"
> +
> +struct user {
> +  char name[MAX_LEN_NAME];
> +  char password[MAX_LEN_NAME];
> +
> +  /* add more fields here as warranted */
> +};
> +
> +#endif /* FC__USER_H */

IMHO this file belongs to (core) freeciv. It should IMHO in server/.

> +/**************************************************************************
> + top-level function to load a user from the database, hands off to
> + handle_db_lookup()
> +**************************************************************************/
> +void user_db_load(struct user *user)
> +{
> +  enum userdb_status status = db_load(user);
> +
> +  try_remove_lock();
> +  handle_db_lookup(user, status);
> +}
> +
> +/**************************************************************************
> + top-level function to save a user to the database, hands off to
> + handle_db_lookup()
> +**************************************************************************/
> +void user_db_save(struct user *user)
> +{
> +  enum userdb_status status = db_save(user);
> +
> +  try_remove_lock();
> +  handle_db_lookup(user, status);
> +}

The usage of handle_db_lookup here is the wrong concept.

> +/**************************************************************************
> + create a lock, so that other servers aren't accessing the database when 
> + we are. return 1 for success, 0 for failure
> +**************************************************************************/

> +static int check_lock(void)

bool

> +{
> +  FILE *lock;
> +  static int check;
> +

> +  lock = fopen(FC_LOCK_FILE, "r");
> +
> +  /* if we aren't locked, create one */
> +  if (!lock) {
> +    lock = fopen(FC_LOCK_FILE, "w");

This isn't atomic. In the time window between these two opens another
may got the lock.

> +/**************************************************
> + The user db statuses are:
> +
> + 1: an error occurred, possibly we couldn't access the database file.
> + 2: the password we checked for was ok, or we were able to successfully
> +    insert an entry.
> + 3: the user's password was wrong.
> + 4: the user's we were searching for was found.
> + 5: the user's we were searching for was not found.
> +***************************************************/
> +enum userdb_status {
> +  USER_DB_ERROR = 1,

> +  USER_DB_LOAD_SUCCESS,
> +  USER_DB_SAVE_SUCCESS,

Are the reason for these two two values handle_db_lookup? Please
remove this ugly callback function.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Premature optimization is the root of all evil."
    -- D. E. Knuth in "Structured Programming with go to Statements"