[Freeciv-Dev] (PR#4594) topology fix for goto route packet

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Because the server doesn't rigorously check the input coordinates for a 
client goto route, the client can send invalid (non-normal) coordinates 
to either trigger an assert or get Bad memory access on the server (this 
could be a security risk).

The attached patch crash-server.diff will demonstrate this.  Just do a 
client goto and bam.

The second attached patch, fix-server.diff, will avoid the crash by 
testing for the normal-ness of the position.  Note that this means the 
client must send NORMAL positions even though it seems the server could 
easily call normalize_map_pos on them.  This patch is targeted toward 
S1_14 and fixes a potential security hole.

The third patch, goto_pack.diff, is the one that lead me to discovering 
all this and is the one intended for application to CVS HEAD.  In 
gen-topologies we can't send (map.xsize,map.ysize) as a special-case 
because it may be a normal position.  This method is also a hack; 
there's nothing special about these values so why use them?  The 
solution elsewhere is to use (-1,-1) instead - or in this case 
(255,255).  Changing this marker requires a new manditory capability 
(otherwise we'll crash the server as in crash-server.diff).

jason

Index: common/packets.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/packets.c,v
retrieving revision 1.249
diff -u -r1.249 packets.c
--- common/packets.c    2003/07/10 03:34:30     1.249
+++ common/packets.c    2003/07/17 18:29:11
@@ -3065,8 +3065,8 @@
     }
     /* if we finished fill the last chunk with NOPs */
     for (; chunk_pos < GOTO_CHUNK; chunk_pos++) {
-      dio_put_uint8(&dout, map.xsize);
-      dio_put_uint8(&dout, map.ysize);
+      dio_put_uint8(&dout, 255);
+      dio_put_uint8(&dout, 255);
     }
 
     dio_put_uint16(&dout, packet->unit_id);

Index: common/packets.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/packets.c,v
retrieving revision 1.219.2.3
diff -u -r1.219.2.3 packets.c
--- common/packets.c    2003/03/17 16:20:54     1.219.2.3
+++ common/packets.c    2003/07/17 18:31:07
@@ -2905,7 +2905,9 @@
   for (i = 0; i < GOTO_CHUNK; i++) {
     dio_get_uint8(&din, &pos[i].x);
     dio_get_uint8(&din, &pos[i].y);
-    if (pos[i].x != map.xsize) num_valid++;
+    if (is_normal_map_pos(pos[i].x, pos[i].y)) {
+      num_valid++;
+    }
   }
   dio_get_uint16(&din, &unit_id);
 

Index: common/capstr.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/capstr.c,v
retrieving revision 1.138
diff -u -r1.138 capstr.c
--- common/capstr.c     2003/07/14 21:01:49     1.138
+++ common/capstr.c     2003/07/17 18:39:19
@@ -79,7 +79,7 @@
                    "+impr_req +waste +fastfocus +continent +small_dipl " \
                    "+no_nation_selected +diplomacy +no_extra_tiles " \
                    "+diplomacy2 +citizens_style +root_tech auth " \
-                   "+nat_ulimit retake"
+                   "+nat_ulimit retake +goto_pack"
 
 /* "+1.14.0" is protocol for 1.14.0 release.
  *
@@ -137,6 +137,9 @@
  * allowing easy adding of arbitrarily many nations.
  *
  * "retake" means that a client can switch players during a running game.
+ *
+ * "goto_pack" changes the goto route packet to send (255,255) instead of
+ * map.xsize/map.ysize to mark the end of the goto chunk.
  */
 
 void init_our_capability(void)
Index: common/packets.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/packets.c,v
retrieving revision 1.249
diff -u -r1.249 packets.c
--- common/packets.c    2003/07/10 03:34:30     1.249
+++ common/packets.c    2003/07/17 18:39:20
@@ -3064,9 +3064,10 @@
       chunk_pos++;
     }
     /* if we finished fill the last chunk with NOPs */
+    assert(!is_normal_map_pos(MAX_UINT8, MAX_UINT8));
     for (; chunk_pos < GOTO_CHUNK; chunk_pos++) {
-      dio_put_uint8(&dout, map.xsize);
-      dio_put_uint8(&dout, map.ysize);
+      dio_put_uint8(&dout, MAX_UINT8);
+      dio_put_uint8(&dout, MAX_UINT8);
     }
 
     dio_put_uint16(&dout, packet->unit_id);
@@ -3106,7 +3107,9 @@
   for (i = 0; i < GOTO_CHUNK; i++) {
     dio_get_uint8(&din, &pos[i].x);
     dio_get_uint8(&din, &pos[i].y);
-    if (pos[i].x != map.xsize) num_valid++;
+    if (is_normal_map_pos(pos[i].x, pos[i].y)) {
+      num_valid++;
+    }
   }
   dio_get_uint16(&din, &unit_id);
 
Index: common/shared.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/shared.h,v
retrieving revision 1.113
diff -u -r1.113 shared.h
--- common/shared.h     2003/05/13 11:16:52     1.113
+++ common/shared.h     2003/07/17 18:39:20
@@ -110,6 +110,8 @@
 
 /* This is duplicated in rand.h to avoid extra includes: */
 #define MAX_UINT32 0xFFFFFFFF
+#define MAX_UINT16 0xFFFF
+#define MAX_UINT8 0xFF
 
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 #define ADD_TO_POINTER(p, n) ((void *)((char *)(p)+(n)))