<URL: http://bugs.freeciv.org/Ticket/Display.html?id=39980 >

Having found a small mistake in the previous patch, and had some time to
consider the second (reply) packet, here's something more for 2.1 only.
The other branches will be updated in the larger authentication revision.

Committed S2_1 revision 14187.

Test in more environments, please!  All I know is it works with itself,
and gives a proper failure against 2.2/trunk on a single machine.  Is
there a public server running out there anywhere?

Index: utility/shared.c
===================================================================
--- utility/shared.c    (revision 14186)
+++ utility/shared.c    (working copy)
@@ -47,6 +47,7 @@
 #include "fcintl.h"
 #include "log.h"
 #include "mem.h"
+#include "rand.h"
 #include "support.h"
 
 #include "shared.h"
@@ -74,6 +75,10 @@
 static char *grouping = NULL;
 static char *grouping_sep = NULL;
 
+static const char base64url[] =
+  "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_";
+
+
 /***************************************************************
   Take a string containing multiple lines and create a copy where
   each line is padded to the length of the longest line and centered.
@@ -385,22 +390,15 @@
 ****************************************************************************/
 bool is_safe_filename(const char *name)
 {
-  int i;
+  int i = 0;
 
   /* must not be NULL or empty */
   if (!name || *name == '\0') {
     return FALSE; 
   }
 
-  /* Accept only alphanumerics and '-', '_', '.' The exception is if
-   * part of PARENT_DIR_OPERATOR is one of these, which is prohibited */  
-  for (i = 0; name[i]; i++) {
-    if (!((name[i] <= 'z' && name[i] >= 'a')
-          || (name[i] <= 'Z' && name[i] >= 'A')
-          || (name[i] <= '9' && name[i] >= '0')
-          || name[i] == '-'
-          || name[i] == '_'
-          || name[i] == '.')) {
+  for (; '\0' != name[i]; i++) {
+    if ('.' != name[i] && NULL == strchr(base64url, name[i])) {
       return FALSE;
     }
   }
@@ -452,6 +450,45 @@
   return TRUE;
 }
 
+/*************************************************************************
+  Check for valid base64url.
+*************************************************************************/
+bool is_base64url(const char *s)
+{
+  size_t i = 0;
+
+  /* must not be NULL or empty */
+  if (NULL == s || '\0' == *s) {
+    return FALSE; 
+  }
+
+  for (; '\0' != s[i]; i++) {
+    if (NULL == strchr(base64url, s[i])) {
+      return FALSE;
+    }
+  }
+  return TRUE;
+}
+
+/*************************************************************************
+  generate a random string meeting criteria such as is_ascii_name(),
+  is_base64url(), and is_safe_filename().
+*************************************************************************/
+void randomize_base64url_string(char *s, size_t n)
+{
+  size_t i = 0;
+
+  /* must not be NULL or too short */
+  if (NULL == s || 1 > n) {
+    return; 
+  }
+
+  for (; i < (n - 1); i++) {
+    s[i] = base64url[myrand(sizeof(base64url) - 1)];
+  }
+  s[i] = '\0';
+}
+
 /***************************************************************
   Produce a statically allocated textual representation of the given
   year.
Index: utility/shared.h
===================================================================
--- utility/shared.h    (revision 14186)
+++ utility/shared.h    (working copy)
@@ -25,8 +25,9 @@
 #endif
 #endif
 
+/* Changing these will break network compatability! */
 #define MAX_LEN_ADDR     256   /* see also MAXHOSTNAMELEN and RFC 1123 2.1 */
-#define MAX_LEN_PATH 4095
+#define MAX_LEN_PATH    4095
 
 /* Use FC_INFINITY to denote that a certain event will never occur or
    another unreachable condition. */
@@ -144,8 +145,11 @@
 const char *int_to_text(unsigned int number);
 
 bool is_ascii_name(const char *name);
+bool is_base64url(const char *s);
 bool is_safe_filename(const char *name);
+void randomize_base64url_string(char *s, size_t n);
 const char *textyear(int year);
+
 int compare_strings(const void *first, const void *second);
 int compare_strings_ptrs(const void *first, const void *second);
 
Index: server/srv_main.c
===================================================================
--- server/srv_main.c   (revision 14186)
+++ server/srv_main.c   (working copy)
@@ -1046,13 +1046,16 @@
   if (!packet)
     return TRUE;
 
+  /* These packets may appear before the connection is "established" */
+  switch (type) {
+  case PACKET_PROCESSING_STARTED:
   /* 
    * Old pre-delta clients (before 2003-11-28) send a
    * PACKET_LOGIN_REQUEST (type 0) to the server. We catch this and
    * reply with an old reject packet. Since there is no struct for
    * this old packet anymore we build it by hand.
    */
-  if (type == 0) {
+  {
     unsigned char buffer[4096];
     struct data_out dout;
 
@@ -1063,12 +1066,10 @@
     dio_put_uint16(&dout, 0);
 
     /* 1 == PACKET_LOGIN_REPLY in the old client */
-    dio_put_uint8(&dout, 1);
+    dio_put_uint8(&dout, PACKET_PROCESSING_FINISHED);
 
     dio_put_bool32(&dout, FALSE);
-    dio_put_string(&dout, _("Your client is too old. To use this server, "
-                           "please upgrade your client to a "
-                           "Freeciv 2.1 or later."));
+    dio_put_string(&dout, "Freeciv " VERSION_STRING);
     dio_put_string(&dout, "");
 
     {
@@ -1086,23 +1087,22 @@
     return FALSE;
   }
 
-  if (type == PACKET_SERVER_JOIN_REQ) {
-    return handle_login_request(pconn,
-                               (struct packet_server_join_req *) packet);
-  }
+  case PACKET_SERVER_JOIN_REQ:
+    return server_join_request(pconn, packet);
 
-  /* May be received on a non-established connection. */
-  if (type == PACKET_AUTHENTICATION_REPLY) {
+  case PACKET_AUTHENTICATION_REPLY:
     return handle_authentication_reply(pconn,
                                ((struct packet_authentication_reply *)
                                 packet)->password);
-  }
 
-  if (type == PACKET_CONN_PONG) {
+  case PACKET_CONN_PONG:
     handle_conn_pong(pconn);
     return TRUE;
-  }
 
+  default:
+    break;
+  };
+
   if (!pconn->established) {
     freelog(LOG_ERROR, "Received game packet from unaccepted connection %s",
            conn_description(pconn));
@@ -1110,15 +1110,18 @@
   }
   
   /* valid packets from established connections but non-players */
-  if (type == PACKET_CHAT_MSG_REQ
-      || type == PACKET_SINGLE_WANT_HACK_REQ
-      || type == PACKET_REPORT_REQ) {
+  switch (type) {
+  case PACKET_CHAT_MSG_REQ:
+  case PACKET_SINGLE_WANT_HACK_REQ:
+  case PACKET_REPORT_REQ:
     if (!server_handle_packet(type, packet, NULL, pconn)) {
       freelog(LOG_ERROR, "Received unknown packet %d from %s",
              type, conn_description(pconn));
     }
     return TRUE;
-  }
+  default:
+    break;
+  };
 
   pplayer = pconn->player;
 
@@ -1129,22 +1132,31 @@
     return TRUE;
   }
 
-  if (S_S_RUNNING != server_state()
-      && type != PACKET_NATION_SELECT_REQ
-      && type != PACKET_PLAYER_READY
-      && type != PACKET_CONN_PONG
-      && type != PACKET_REPORT_REQ) {
-    if (S_S_OVER == server_state()) {
+  switch (type) {
+  case PACKET_NATION_SELECT_REQ:
+  case PACKET_PLAYER_READY:
+  case PACKET_CONN_PONG:
+  case PACKET_REPORT_REQ:
+    /* FIXME: appear anytime? */
+    break;
+  default:
+    switch (server_state()) {
+    case S_S_RUNNING:
+      /* OK */
+      break;
+    case S_S_OVER:
       /* This can happen by accident, so we don't want to print
         out lots of error messages. Ie, we use LOG_DEBUG. */
       freelog(LOG_DEBUG, "got a packet of type %d "
                          "in S_S_OVER", type);
-    } else {
+      return TRUE;
+    default:
       freelog(LOG_ERROR, "got a packet of type %d "
                         "outside S_S_RUNNING", type);
-    }
-    return TRUE;
-  }
+      return TRUE;
+    };
+    break;
+  };
 
   pplayer->nturns_idle=0;
 
@@ -2038,7 +2050,7 @@
       }
     } players_iterate_end;
 
-  /* Set up alliances based on team selections */
+    /* Set up alliances based on team selections */
     players_iterate(pplayer) {
       players_iterate(pdest) {
         if (players_on_same_team(pplayer, pdest)
Index: server/connecthand.c
===================================================================
--- server/connecthand.c        (revision 14186)
+++ server/connecthand.c        (working copy)
@@ -201,70 +201,95 @@
 /**************************************************************************
   send the rejection packet to the client.
 **************************************************************************/
-void reject_new_connection(const char *msg, struct connection *pconn)
+void reject_new_connection(struct connection *pconn, const char *message)
 {
   struct packet_server_join_reply packet;
 
   /* zero out the password */
   memset(pconn->server.password, 0, sizeof(pconn->server.password));
 
+  /* now send join_reply packet */
+  memset(&packet, 0, sizeof(packet));
   packet.you_can_join = FALSE;
+  sz_strlcpy(packet.message, message);
   sz_strlcpy(packet.capability, our_capability);
-  sz_strlcpy(packet.message, msg);
   packet.challenge_file[0] = '\0';
   packet.conn_id = -1;
+
   send_packet_server_join_reply(pconn, &packet);
   freelog(LOG_NORMAL, _("Client rejected: %s."), conn_description(pconn));
   flush_connection_send_buffer_all(pconn);
 }
 
 /**************************************************************************
- Returns FALSE if the clients gets rejected and the connection should be
- closed. Returns TRUE if the client get accepted.
+ Returns FALSE when the clients are rejected and the connection should be
+ closed. Returns TRUE otherwise.
 **************************************************************************/
-bool handle_login_request(struct connection *pconn, 
-                          struct packet_server_join_req *req)
+bool server_join_request(struct connection *pconn, void *packet)
 {
   char msg[MAX_LEN_MSG];
+  char username[MAX_LEN_NAME];
+#define req ((struct packet_server_join_req *)packet)
   
+  /* Never assume network data is valid and printable! */
+  sz_strlcpy(username,
+             is_valid_username(req->username)
+             ? req->username
+             : "?");
   freelog(LOG_NORMAL, _("Connection request from %s from %s"),
-          req->username, pconn->addr);
-  
-  /* print server and client capabilities to console */
-  freelog(LOG_NORMAL, _("%s has client version %d.%d.%d%s"),
-          pconn->username, req->major_version, req->minor_version,
-          req->patch_version, req->version_label);
-  freelog(LOG_VERBOSE, "Client caps: %s", req->capability);
-  freelog(LOG_VERBOSE, "Server caps: %s", our_capability);
-  sz_strlcpy(pconn->capability, req->capability);
-  
+          username,
+          pconn->addr);
+
+  /* log client and server capabilities */
+  freelog(LOG_VERBOSE, "%s: client version %d.%d.%d%s",
+          pconn->addr,
+          req->major_version,
+          req->minor_version,
+          req->patch_version,
+          ('\0' == req->version_label[0] || is_ascii_name(req->version_label))
+          ? req->version_label
+          : "?");
+
+  sz_strlcpy(pconn->capability,
+             ('\0' == req->capability[0] || is_ascii_name(req->capability))
+             ? req->capability
+             : "?");
+  freelog(LOG_VERBOSE, "Client: %s", pconn->capability);
+  freelog(LOG_VERBOSE, "Server: %s", our_capability);
+
   /* Make sure the server has every capability the client needs */
-  if (!has_capabilities(our_capability, req->capability)) {
+  if (!has_capabilities(our_capability, pconn->capability)) {
     my_snprintf(msg, sizeof(msg),
                 _("The client is missing a capability that this server 
needs.\n"
                    "Server version: %d.%d.%d%s Client version: %d.%d.%d%s."
                    "  Upgrading may help!"),
                 MAJOR_VERSION, MINOR_VERSION, PATCH_VERSION, VERSION_LABEL,
                 req->major_version, req->minor_version,
-                req->patch_version, req->version_label);
-    reject_new_connection(msg, pconn);
+                req->patch_version,
+                ('\0' == req->version_label[0] || 
is_ascii_name(req->version_label))
+                ? req->version_label
+                : "?");
+    reject_new_connection(pconn, msg);
     freelog(LOG_NORMAL, _("%s was rejected: Mismatched capabilities."),
-            req->username);
+            username);
     return FALSE;
   }
 
   /* Make sure the client has every capability the server needs */
-  if (!has_capabilities(req->capability, our_capability)) {
+  if (!has_capabilities(pconn->capability, our_capability)) {
     my_snprintf(msg, sizeof(msg),
                 _("The server is missing a capability that the client needs.\n"
                    "Server version: %d.%d.%d%s Client version: %d.%d.%d%s."
                    "  Upgrading may help!"),
                 MAJOR_VERSION, MINOR_VERSION, PATCH_VERSION, VERSION_LABEL,
                 req->major_version, req->minor_version,
-                req->patch_version, req->version_label);
-    reject_new_connection(msg, pconn);
+                req->patch_version,
+                ('\0' == req->version_label[0] || 
is_ascii_name(req->version_label))
+                ? req->version_label
+                : "?");
+    reject_new_connection(pconn, msg);
     freelog(LOG_NORMAL, _("%s was rejected: Mismatched capabilities."),
-            req->username);
+            username);
     return FALSE;
   }
 
@@ -272,10 +297,10 @@
 
   /* Name-sanity check: could add more checks? */
   if (!is_valid_username(req->username)) {
-    my_snprintf(msg, sizeof(msg), _("Invalid username '%s'"), req->username);
-    reject_new_connection(msg, pconn);
+    my_snprintf(msg, sizeof(msg), _("Invalid username '%s'"), username);
+    reject_new_connection(pconn, msg);
     freelog(LOG_NORMAL, _("%s was rejected: Invalid name [%s]."),
-            req->username, pconn->addr);
+            username, pconn->addr);
     return FALSE;
   } 
 
@@ -284,9 +309,9 @@
     if (mystrcasecmp(req->username, aconn->username) == 0) { 
       my_snprintf(msg, sizeof(msg), _("'%s' already connected."), 
                   req->username);
-      reject_new_connection(msg, pconn);
+      reject_new_connection(pconn, msg);
       freelog(LOG_NORMAL, _("%s was rejected: Duplicate login name [%s]."),
-              req->username, pconn->addr);
+              username, pconn->addr);
       return FALSE;
     }
   } conn_list_iterate_end;
Index: server/connecthand.h
===================================================================
--- server/connecthand.h        (revision 14186)
+++ server/connecthand.h        (working copy)
@@ -19,15 +19,11 @@
 
 struct connection;
 struct conn_list;
-struct packet_authentication_reply;
-struct packet_login_request;
-struct packet_server_join_req;
 
 void establish_new_connection(struct connection *pconn);
-void reject_new_connection(const char *msg, struct connection *pconn);
+void reject_new_connection(struct connection *pconn, const char *message);
 
-bool handle_login_request(struct connection *pconn,
-                          struct packet_server_join_req *req);
+bool server_join_request(struct connection *pconn, void *packet);
 
 void lost_connection_to_client(struct connection *pconn);
 
Index: server/auth.c
===================================================================
--- server/auth.c       (revision 14186)
+++ server/auth.c       (working copy)
@@ -352,7 +352,7 @@
 }
 
 /**************************************************************************
-  handle authentication of a user; called by handle_login_request()
+  handle authentication of a user; called only by server_join_request()
   if authentication is enabled.
 
   if the connection is rejected right away, return FALSE, otherwise return TRUE
@@ -376,8 +376,9 @@
       sz_strlcpy(pconn->username, username);
       establish_new_connection(pconn);
     } else {
-      reject_new_connection(_("Guests are not allowed on this server. "
-                              "Sorry."), pconn);
+      reject_new_connection(pconn,
+                            N_("Guests are not allowed on this server."
+                              " Sorry."));
       freelog(LOG_NORMAL, _("%s was rejected: Guests not allowed."), username);
       return FALSE;
     }
@@ -402,9 +403,9 @@
                     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);
+        reject_new_connection(pconn,
+                              N_("There was an error reading the user database"
+                                 " and guest logins are not allowed. Sorry"));
         freelog(LOG_NORMAL, 
                 _("%s was rejected: Database error and guests not allowed."),
                 pconn->username);
@@ -427,8 +428,9 @@
         pconn->server.auth_settime = time(NULL);
         pconn->server.status = AS_REQUESTING_NEW_PASS;
       } else {
-        reject_new_connection(_("This server allows only preregistered "
-                                "users. Sorry."), pconn);
+        reject_new_connection(pconn,
+                              N_("This server allows only preregistered users."
+                                 " Sorry."));
         freelog(LOG_NORMAL,
                 _("%s was rejected: Only preregister users allowed."),
                 pconn->username);
@@ -458,7 +460,8 @@
     /* check if the new password is acceptable */
     if (!is_good_password(password, msg)) {
       if (pconn->server.auth_tries++ >= MAX_AUTH_TRIES) {
-        reject_new_connection(_("Sorry, too many wrong tries..."), pconn);
+        reject_new_connection(pconn,
+                              N_("Sorry, too many wrong tries..."));
         freelog(LOG_NORMAL, _("%s was rejected: Too many wrong password "
                 "verifies for new user."), pconn->username);
 
@@ -516,7 +519,8 @@
 
       if (pconn->server.auth_tries >= MAX_AUTH_TRIES) {
         pconn->server.status = AS_NOT_ESTABLISHED;
-        reject_new_connection(_("Sorry, too many wrong tries..."), pconn);
+        reject_new_connection(pconn,
+                              N_("Sorry, too many wrong tries..."));
         freelog(LOG_NORMAL,
                 _("%s was rejected: Too many wrong password tries."),
                 pconn->username);
@@ -537,7 +541,8 @@
     /* waiting on the client to send us a password... don't wait too long */
     if (time(NULL) >= pconn->server.auth_settime + MAX_WAIT_TIME) {
       pconn->server.status = AS_NOT_ESTABLISHED;
-      reject_new_connection(_("Sorry, your connection timed out..."), pconn);
+      reject_new_connection(pconn,
+                            N_("Sorry, your connection timed out..."));
       freelog(LOG_NORMAL,
               _("%s was rejected: Connection timeout waiting for password."),
               pconn->username);
Index: common/player.c
===================================================================
--- common/player.c     (revision 14186)
+++ common/player.c     (working copy)
@@ -861,9 +861,8 @@
 ****************************************************************************/
 bool is_valid_username(const char *name)
 {
-  return (strlen(name) > 0
+  return (is_ascii_name(name)
          && !my_isdigit(name[0])
-         && is_ascii_name(name)
          && mystrcasecmp(name, ANON_USER_NAME) != 0);
 }
 
Index: client/packhand.c
===================================================================
--- client/packhand.c   (revision 14186)
+++ client/packhand.c   (working copy)
@@ -173,13 +173,19 @@
                               int conn_id)
 {
   char msg[MAX_LEN_MSG];
-  char *s_capability = aconnection.capability;
 
-  sz_strlcpy(aconnection.capability, capability);
+  /* Never assume network data is valid and printable!
+   * Is there something that could be checked in message?
+   * In any case, check capability.
+   */
+  sz_strlcpy(aconnection.capability,
+             ('\0' == capability[0] || is_ascii_name(capability))
+             ? capability
+             : "?");
   close_connection_dialog();
 
   if (you_can_join) {
-    freelog(LOG_VERBOSE, "join game accept:%s", message);
+    freelog(LOG_VERBOSE, "join game accept: %s", message);
     aconnection.established = TRUE;
     aconnection.id = conn_id;
     agents_game_joined();
@@ -187,17 +193,22 @@
 
     set_server_busy(FALSE);
     
-    if (get_client_page() == PAGE_MAIN
-       || get_client_page() == PAGE_NETWORK
-       || get_client_page() == PAGE_GGZ) {
+    switch (get_client_page()) {
+    case PAGE_MAIN:
+    case PAGE_NETWORK:
+    case PAGE_GGZ:
       set_client_page(PAGE_START);
-    }
+      break;
+    default:
+      /* no change */
+      break;
+    };
 
     /* we could always use hack, verify we're local */ 
     send_client_wants_hack(challenge_file);
   } else {
     my_snprintf(msg, sizeof(msg),
-               _("You were rejected from the game: %s"), message);
+               _("You were rejected from the game: %s"), Q_(message));
     append_output_window(msg);
     aconnection.id = 0;
     if (auto_connect) {
@@ -208,14 +219,14 @@
       set_client_page(in_ggz ? PAGE_MAIN : PAGE_GGZ);
     }
   }
-  if (strcmp(s_capability, our_capability) == 0) {
+  if (0 == strcmp(aconnection.capability, our_capability)) {
     return;
   }
   my_snprintf(msg, sizeof(msg),
              _("Client capability string: %s"), our_capability);
   append_output_window(msg);
   my_snprintf(msg, sizeof(msg),
-             _("Server capability string: %s"), s_capability);
+             _("Server capability string: %s"), aconnection.capability);
   append_output_window(msg);
 }
 
Index: client/connectdlg_common.c
===================================================================
--- client/connectdlg_common.c  (revision 14186)
+++ client/connectdlg_common.c  (working copy)
@@ -42,9 +42,10 @@
 #include "log.h"
 #include "mem.h"
 #include "netintf.h"
-#include "rand.h"
 #include "registry.h"
+#include "shared.h"
 #include "support.h"
+
 #include "civclient.h"
 #include "climisc.h"
 #include "clinet.h"
@@ -392,21 +393,6 @@
 }
 
 /*************************************************************************
-  generate a random string.
-*************************************************************************/
-static void randomize_string(char *str, size_t n)
-{
-  const char chars[] =
-    "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
-  int i;
-
-  for (i = 0; i < n - 1; i++) {
-    str[i] = chars[myrand(sizeof(chars) - 1)];
-  }
-  str[i] = '\0';
-}
-
-/*************************************************************************
   returns TRUE if a filename is safe (i.e. doesn't have path components).
 *************************************************************************/
 static bool is_filename_safe(const char *filename)
@@ -448,7 +434,7 @@
     sz_strlcat(challenge_fullname, filename);
 
     /* generate an authentication token */ 
-    randomize_string(req.token, sizeof(req.token));
+    randomize_base64url_string(req.token, sizeof(req.token));
 
     section_file_init(&file);
     secfile_insert_str(&file, req.token, "challenge.token");
@@ -464,7 +450,7 @@
 }
 
 /**************************************************************** 
-handle response (by the server) if the client has got hack or not.
+  client has hack (or not).
 *****************************************************************/ 
 void handle_single_want_hack_reply(bool you_have_hack)
 {
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to