[Freeciv-Dev] (PR#10628) fatal flaw in packet_conn_ping_info

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

If there are more than 32 connections packet_conn_ping_info will fail 
badly.  This is a security flaw in the server (I think).

This patch fixes it, using optional capabilities.  Again we have the 
choice of manditory capabilities (but with big drawbacks).

jason

? newtiles
? data/isotrident/selection.png
? data/isotrident/selection.spec
Index: client/packhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
retrieving revision 1.410
diff -u -r1.410 packhand.c
--- client/packhand.c   19 Oct 2004 18:11:21 -0000      1.410
+++ client/packhand.c   19 Oct 2004 20:08:27 -0000
@@ -1664,21 +1664,23 @@
 /*************************************************************************
 ...
 **************************************************************************/
-void handle_conn_ping_info(int connections, int *conn_id, float *ping_time)
+void handle_conn_ping_info(struct packet_conn_ping_info *packet)
 {
   int i;
 
-  for (i = 0; i < connections; i++) {
-    struct connection *pconn = find_conn_by_id(conn_id[i]);
+  for (i = 0; i < packet->connections; i++) {
+    struct connection *pconn = find_conn_by_id(packet->conn_id[i]);
 
     if (!pconn) {
       continue;
     }
 
-    pconn->ping_time = ping_time[i];
+    pconn->ping_time = packet->ping_time[i];
     freelog(LOG_DEBUG, "conn-id=%d, ping=%fs", pconn->id,
            pconn->ping_time);
   }
+  /* The old_ping_time data is ignored. */
+
   update_players_dialog();
 }
 
Index: common/capstr.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/capstr.c,v
retrieving revision 1.191
diff -u -r1.191 capstr.c
--- common/capstr.c     4 Oct 2004 19:35:14 -0000       1.191
+++ common/capstr.c     19 Oct 2004 20:08:27 -0000
@@ -75,6 +75,9 @@
 
 /* +2.0 is the capability string for the 2.0.x release(s).
  *
+ * "conn_ping_info" means the packet_conn_ping_info uses MAX_NUM_CONNECTIONS
+ * not MAX_NUM_PLAYERS.
+ *
  *   - No new manditory capabilities can be added to the release branch; doing
  *     so would break network capability of supposedly "compatible" releases.
  *
@@ -82,7 +85,7 @@
  *     as long as possible.  We want to maintain network compatibility with
  *     the stable branch for as long as possible.
  */
-#define CAPABILITY "+2.0"
+#define CAPABILITY "+2.0 conn_ping_info"
 
 void init_our_capability(void)
 {
Index: common/packets.def
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/packets.def,v
retrieving revision 1.55
diff -u -r1.55 packets.def
--- common/packets.def  25 Sep 2004 22:18:41 -0000      1.55
+++ common/packets.def  19 Oct 2004 20:08:27 -0000
@@ -849,9 +849,14 @@
 
 # Information about the ping times of the connections.
 PACKET_CONN_PING_INFO=87; sc,lsend
-  UINT8 connections;
-  CONNECTION conn_id[MAX_NUM_PLAYERS:connections];
-  float1000000(float) ping_time[MAX_NUM_PLAYERS:connections];
+  # "Old" values
+  UINT8 old_connections; remove-cap(conn_ping_info)
+  CONNECTION old_conn_id[MAX_NUM_PLAYERS:connections]; 
remove-cap(conn_ping_info)
+  float1000000(float) old_ping_time[MAX_NUM_PLAYERS:connections]; 
remove-cap(conn_ping_info)
+
+  UINT8 connections; add-cap(conn_ping_info)
+  CONNECTION conn_id[MAX_NUM_CONNECTIONS:connections]; add-cap(conn_ping_info)
+  float1000000(float) ping_time[MAX_NUM_CONNECTIONS:connections]; 
add-cap(conn_ping_info)
 end
 
 PACKET_CONN_PING=88;sc
Index: server/sernet.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/sernet.c,v
retrieving revision 1.123
diff -u -r1.123 sernet.c
--- server/sernet.c     4 Oct 2004 04:37:33 -0000       1.123
+++ server/sernet.c     19 Oct 2004 20:08:28 -0000
@@ -960,14 +960,20 @@
   } conn_list_iterate_end;
 
   packet.connections = i;
+  packet.old_connections = MIN(i, MAX_NUM_PLAYERS);
 
   i = 0;
   conn_list_iterate(game.game_connections, pconn) {
     if (!pconn->used) {
       continue;
     }
+    assert(i < ARRAY_SIZE(packet.conn_id));
     packet.conn_id[i] = pconn->id;
     packet.ping_time[i] = pconn->ping_time;
+    if (i < packet.old_connections) {
+      packet.old_conn_id[i] = pconn->id;
+      packet.old_ping_time[i] = pconn->ping_time;
+    }
     i++;
   } conn_list_iterate_end;
   lsend_packet_conn_ping_info(&game.est_connections, &packet);
Index: server/sernet.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/sernet.h,v
retrieving revision 1.10
diff -u -r1.10 sernet.h
--- server/sernet.h     22 Nov 2003 23:34:55 -0000      1.10
+++ server/sernet.h     19 Oct 2004 20:08:28 -0000
@@ -15,7 +15,6 @@
 
 struct connection;
 
-#define MAX_NUM_CONNECTIONS (2 * (MAX_NUM_PLAYERS + MAX_NUM_BARBARIANS))
 #define DEFAULT_SOCK_PORT 5555
 #define BUF_SIZE 512
 
Index: utility/shared.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/utility/shared.h,v
retrieving revision 1.135
diff -u -r1.135 shared.h
--- utility/shared.h    18 Oct 2004 23:49:27 -0000      1.135
+++ utility/shared.h    19 Oct 2004 20:08:28 -0000
@@ -64,6 +64,7 @@
 /* MAX_NUM_PLAYERS + MAX_NUM_BARBARIANS <= 32 !!!! */
 #define MAX_NUM_PLAYERS  30
 #define MAX_NUM_BARBARIANS   2
+#define MAX_NUM_CONNECTIONS (2 * (MAX_NUM_PLAYERS + MAX_NUM_BARBARIANS))
 #define MAX_NUM_ITEMS   200    /* eg, unit_types */
 #define MAX_NUM_TECH_LIST 10
 #define MAX_NUM_BUILDING_LIST 10