[Freeciv-Dev] (PR#10727) [PATCH] Hack challenge protocol issues.

["Vasco Alexandre da Silva Costa" <vasc@xxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=10727 >

This one fixes a nasty read on a free'd memory block. Thanks go to
Use_less for spotting it.

Index: client/connectdlg_common.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/connectdlg_common.c,v
retrieving revision 1.26
diff -u -r1.26 connectdlg_common.c
--- client/connectdlg_common.c  24 Oct 2004 23:18:07 -0000      1.26
+++ client/connectdlg_common.c  16 Nov 2004 01:18:35 -0000
@@ -37,7 +37,9 @@
 #include <sys/wait.h>
 #endif
 
+#include "capability.h"
 #include "fcintl.h"
+#include "log.h"
 #include "mem.h"
 #include "netintf.h"
 #include "rand.h"
@@ -67,6 +69,7 @@
 char player_name[MAX_LEN_NAME];
 char *current_filename = NULL;
 
+static char challenge_fullname[MAX_LEN_PATH];
 static bool client_has_hack = FALSE;
 
 int internal_server_port;
@@ -369,33 +372,78 @@
 #endif
 }
 
+/*************************************************************************
+  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)
+{
+  const char *unsafe = "/\\";
+  const char *s;
+
+  for (s = filename; *s != '\0'; s++) {
+    if (strchr(unsafe, *s)) {
+      return FALSE;
+    }
+  }
+  return TRUE;
+}
+
 /**************************************************************** 
 if the client is capable of 'wanting hack', then the server will 
 send the client a filename in the packet_join_game_reply packet.
 
-this function creates the file with a suitably random number in it 
-and then sends the filename and number to the server. If the server 
-can open and read the number, then the client is given hack access.
+this function creates the file with a suitably random string in it 
+and then sends the string to the server. If the server can open
+and read the string, then the client is given hack access.
 *****************************************************************/ 
-void send_client_wants_hack(char * filename)
+void send_client_wants_hack(const char *filename)
 {
-  struct section_file file;
-
-  /* find some entropy */ 
-  int entropy = myrand(MAX_UINT32) - time(NULL);
+  if (has_capability("new_hack", aconnection.capability)) {
+    if (filename[0] != '\0') {
+      struct packet_single_want_hack_req req;
+      struct section_file file;
 
-  /* we don't want zero */ 
-  if (entropy == 0) {
-    entropy++;
-  }
+      if (!is_filename_safe(filename)) {
+       return;
+      }
 
-  section_file_init(&file);
-  secfile_insert_int(&file, entropy, "challenge.entropy");
-  section_file_save(&file, filename, 0);      
-  section_file_free(&file);
+      /* get the full filename path */
+      interpret_tilde(challenge_fullname, sizeof(challenge_fullname),
+         "~/.freeciv/");
+      make_dir(challenge_fullname);
+
+      sz_strlcat(challenge_fullname, filename);
+
+      /* generate an authentication token */ 
+      randomize_string(req.token, sizeof(req.token));
+
+      section_file_init(&file);
+      secfile_insert_str(&file, req.token, "challenge.token");
+      if (!section_file_save(&file, challenge_fullname, 0)) {
+       freelog(LOG_ERROR, "Couldn't write token to temporary file: %s",
+           challenge_fullname);
+      }
+      section_file_free(&file);
 
-  /* tell the server what we put into the file */ 
-  dsend_packet_single_want_hack_req(&aconnection, entropy);
+      /* tell the server what we put into the file */ 
+      send_packet_single_want_hack_req(&aconnection, &req);
+    }
+  }
 }
 
 /**************************************************************** 
@@ -403,6 +451,17 @@
 *****************************************************************/ 
 void handle_single_want_hack_reply(bool you_have_hack)
 {
+  if (has_capability("new_hack", aconnection.capability)) {
+    /* remove challenge file */
+    if (challenge_fullname[0] != '\0') {
+      if (remove(challenge_fullname) == -1) {
+       freelog(LOG_ERROR, "Couldn't remove temporary file: %s",
+           challenge_fullname);
+      }
+      challenge_fullname[0] = '\0';
+    }
+  }
+
   if (you_have_hack) {
     append_output_window(_("We have control of the server "
                          "(command access level hack)"));
Index: client/connectdlg_common.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/connectdlg_common.h,v
retrieving revision 1.6
diff -u -r1.6 connectdlg_common.h
--- client/connectdlg_common.h  21 Aug 2004 16:34:03 -0000      1.6
+++ client/connectdlg_common.h  16 Nov 2004 01:18:35 -0000
@@ -25,7 +25,7 @@
 bool is_server_running(void);
 bool can_client_access_hack(void);
 
-void send_client_wants_hack(char *filename);
+void send_client_wants_hack(const char *filename);
 void send_start_saved_game(void);
 void send_save_game(char *filename);
 
Index: common/capstr.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/capstr.c,v
retrieving revision 1.194
diff -u -r1.194 capstr.c
--- common/capstr.c     4 Nov 2004 03:02:10 -0000       1.194
+++ common/capstr.c     16 Nov 2004 01:18:35 -0000
@@ -90,7 +90,7 @@
  *     as long as possible.  We want to maintain network compatibility with
  *     the stable branch for as long as possible.
  */
-#define CAPABILITY "+2.0 connecting conn_ping_info username_info"
+#define CAPABILITY "+2.0 connecting conn_ping_info username_info new_hack"
 
 void init_our_capability(void)
 {
Index: common/packets.def
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/packets.def,v
retrieving revision 1.58
diff -u -r1.58 packets.def
--- common/packets.def  4 Nov 2004 03:02:10 -0000       1.58
+++ common/packets.def  16 Nov 2004 01:18:36 -0000
@@ -1254,8 +1254,9 @@
 /*********************************************************
  Below are the packets that control single-player mode.  
 *********************************************************/
-PACKET_SINGLE_WANT_HACK_REQ=108;cs,handle-per-conn,no-handle,dsend
- UINT32 challenge;
+PACKET_SINGLE_WANT_HACK_REQ=108;cs,handle-per-conn,no-handle
+ UINT32 old_token; remove-cap(new_hack)
+ STRING token[MAX_LEN_NAME]; add-cap(new_hack)
 end
 
 PACKET_SINGLE_WANT_HACK_REPLY=109;sc,dsend
Index: server/connecthand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/connecthand.c,v
retrieving revision 1.35
diff -u -r1.35 connecthand.c
--- server/connecthand.c        23 Oct 2004 23:12:22 -0000      1.35
+++ server/connecthand.c        16 Nov 2004 01:18:46 -0000
@@ -82,7 +82,7 @@
   sz_strlcpy(packet.capability, our_capability);
   my_snprintf(packet.message, sizeof(packet.message), _("%s Welcome"),
               pconn->username);
-  sz_strlcpy(packet.challenge_file, create_challenge_filename());
+  sz_strlcpy(packet.challenge_file, new_challenge_filename(pconn));
   packet.conn_id = pconn->id;
   send_packet_server_join_reply(pconn, &packet);
 
Index: server/gamehand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/gamehand.c,v
retrieving revision 1.144
diff -u -r1.144 gamehand.c
--- server/gamehand.c   10 Oct 2004 20:02:02 -0000      1.144
+++ server/gamehand.c   16 Nov 2004 01:18:46 -0000
@@ -18,6 +18,7 @@
 #include <assert.h>
 #include <stdio.h> /* for remove() */ 
 
+#include "capability.h"
 #include "events.h"
 #include "fcintl.h"
 #include "improvement.h"
@@ -34,15 +35,10 @@
 #include "unittools.h"
 
 #include "gamehand.h"
+#include "srv_main.h"
 
-#define NUMBER_OF_TRIES 500
-#ifndef __VMS
-#  define CHALLENGE_ROOT ".freeciv-tmp"
-#else /*VMS*/
-#  define CHALLENGE_ROOT "freeciv-tmp"
-#endif
+#define CHALLENGE_ROOT "challenge"
 
-static char challenge_filename[MAX_LEN_PATH];
 
 /****************************************************************************
   Initialize the game.id variable to a random string of characters.
@@ -413,74 +409,78 @@
 }
 
 /************************************************************************** 
-find a suitably random file that we can write too, and return it's name.
+  generate challenge filename for this connection, cannot fail.
 **************************************************************************/
-const char *create_challenge_filename(void)
+static void gen_challenge_filename(struct connection *pc)
 {
-  int i;
-  unsigned int tmp = 0;
+}
 
-  /* find a suitable file to place the challenge in, we'll remove the file
-   * once the challenge is  */
-#ifndef CHALLENGE_PATH
-  sz_strlcpy(challenge_filename, user_home_dir());
-  sz_strlcat(challenge_filename, "/");
-  sz_strlcat(challenge_filename, CHALLENGE_ROOT);
-#else
-  sz_strlcpy(challenge_filename, CHALLENGE_PATH);
-#endif
-
-  for (i = 0; i < NUMBER_OF_TRIES; i++) {
-    char test_str[MAX_LEN_PATH];
-
-    sz_strlcpy(test_str, challenge_filename);
-    tmp = time(NULL);
-    cat_snprintf(test_str, MAX_LEN_PATH, "-%d", tmp);
-
-    /* file doesn't exist but we can create one and write to it */
-    if (!is_reg_file_for_access(test_str, FALSE) &&
-        is_reg_file_for_access(test_str, TRUE)) {
-      cat_snprintf(challenge_filename, MAX_LEN_PATH, "-%d", tmp);
-      break;
-    } else {
-      die("we can't seem to write to the filesystem!");
-    }
+/************************************************************************** 
+  get challenge filename for this connection.
+**************************************************************************/
+static const char *get_challenge_filename(struct connection *pc)
+{
+  static char filename[MAX_LEN_PATH];
+
+  my_snprintf(filename, sizeof(filename), "%s_%d_%d",
+      CHALLENGE_ROOT, srvarg.port, pc->id);
+
+  return filename;
+}
+
+/************************************************************************** 
+  get challenge full filename for this connection.
+**************************************************************************/
+static const char *get_challenge_fullname(struct connection *pc)
+{
+  static char fullname[MAX_LEN_PATH];
+
+  interpret_tilde(fullname, sizeof(fullname), "~/.freeciv/");
+  sz_strlcat(fullname, get_challenge_filename(pc));
+
+  return fullname;
+}
+
+/************************************************************************** 
+  find a file that we can write too, and return it's name.
+**************************************************************************/
+const char *new_challenge_filename(struct connection *pc)
+{
+  if (!has_capability("new_hack", pc->capability)) {
+    return "";
   }
 
-  return challenge_filename;
+  gen_challenge_filename(pc);
+  return get_challenge_filename(pc);
 }
 
+
 /************************************************************************** 
 opens a file specified by the packet and compares the packet values with
 the file values. Sends an answer to the client once it's done.
 **************************************************************************/
-void handle_single_want_hack_req(struct connection *pc, int challenge)
+void handle_single_want_hack_req(struct connection *pc,
+                                const struct packet_single_want_hack_req
+                                *packet)
 {
   struct section_file file;
-  int entropy = 0;
-  bool could_load = TRUE;
+  char *token = NULL;
   bool you_have_hack = FALSE;
 
-  if (is_reg_file_for_access(challenge_filename, FALSE)) {
-    if (section_file_load_nodup(&file, challenge_filename)) {
-      entropy = secfile_lookup_int_default(&file, 0, "challenge.entropy");
-      section_file_free(&file);
-    } else {
-      freelog(LOG_ERROR, "couldn't load temporary file: %s",
-              challenge_filename);
-      could_load = FALSE;
-    }
+  if (!has_capability("new_hack", pc->capability)) {
+    dsend_packet_single_want_hack_reply(pc, FALSE);
+    return ;
+  }
 
-    /* remove temp file */
-    if (remove(challenge_filename) != 0) {
-      freelog(LOG_ERROR, "couldn't remove temporary file: %s",
-              challenge_filename);
-    }
-  } else {
-    could_load = FALSE;
+  if (section_file_load_nodup(&file, get_challenge_fullname(pc))) {
+    token = secfile_lookup_str_default(&file, NULL, "challenge.token");
+    you_have_hack = (token && strcmp(token, packet->token) == 0);
+    section_file_free(&file);
   }
 
-  you_have_hack = (could_load && entropy && entropy == challenge);
+  if (!token) {
+    freelog(LOG_DEBUG, "Failed to read authentication token");
+  }
 
   if (you_have_hack) {
     pc->access_level = ALLOW_HACK;
Index: server/gamehand.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/gamehand.h,v
retrieving revision 1.11
diff -u -r1.11 gamehand.h
--- server/gamehand.h   10 Apr 2004 03:47:49 -0000      1.11
+++ server/gamehand.h   16 Nov 2004 01:18:46 -0000
@@ -25,7 +25,12 @@
 
 int update_timeout(void);
 
-const char *create_challenge_filename(void);
-void handle_single_want_hack_req(struct connection *pc, int challenge);
+const char *new_challenge_filename(struct connection *pc);
+
+struct packet_single_want_hack_req;
+
+void handle_single_want_hack_req(struct connection *pc,
+                                const struct packet_single_want_hack_req
+                                *packet);
 
 #endif  /* FC__GAMEHAND_H */
Index: server/srv_main.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/srv_main.c,v
retrieving revision 1.208
diff -u -r1.208 srv_main.c
--- server/srv_main.c   11 Nov 2004 16:56:39 -0000      1.208
+++ server/srv_main.c   16 Nov 2004 01:18:59 -0000
@@ -952,7 +952,7 @@
 
   if (type == PACKET_SINGLE_WANT_HACK_REQ) {
     handle_single_want_hack_req(pconn,
-               ((struct packet_single_want_hack_req *)packet)->challenge);
+                               (struct packet_single_want_hack_req *) packet);
     return TRUE;
   }