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

Here's a quick patch against 2.1, really only fixing the first packet.
There's much more to do later....  But the first exchange is most crucial
for protecting against evildoers!

Because this is a moment just before release, and we are taking care about
changing translation strings, I've left them in place.  In a future patch,
we need to remove the client version %d.%d.%d%s (the client knows its own)
and server version %d.%d.%d%s (already learned in the server list).  These
variable parts prevent the client from doing its translation.

Index: utility/shared.c
===================================================================
--- utility/shared.c    (revision 14182)
+++ 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 14182)
+++ 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 14182)
+++ 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 14182)
+++ 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,
+          is_ascii_name(req->version_label)
+          ? req->version_label
+          : "?");
+
+  sz_strlcpy(pconn->capability,
+             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,
+                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,
+                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 14182)
+++ 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 14182)
+++ 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 14182)
+++ 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 14182)
+++ client/packhand.c   (working copy)
@@ -197,7 +197,7 @@
     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) {
Index: client/connectdlg_common.c
===================================================================
--- client/connectdlg_common.c  (revision 14182)
+++ 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