[Freeciv-Dev] (PR#13802) player attribute chunk security problem

["Mateusz Stefek" <mstefek@xxxxxxxxx>]

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

> [jdorje - Mon Aug 29 05:54:59 2005]:
> 
> Mateusz Stefek wrote:
> > <URL: http://bugs.freeciv.org/Ticket/Display.html?id=13802 >
> > 
> > packets.c:
> > 
> > void post_receive_packet_player_attribute_chunk(struct connection *pc,
> >                                             struct 
> > packet_player_attribute_chunk
> >                                             *packet)
> > {
> >   /*
> >    * Because of the changes in enum packet_type during the 1.12.1
> >    * timeframe an old server will trigger the following condition.
> >    */
> >   if (packet->total_length <= 0
> >       || packet->total_length >= MAX_ATTRIBUTE_BLOCK) {
> >     freelog(LOG_FATAL, _("The server you tried to connect is too old "
> >                      "(1.12.0 or earlier). Please choose another "
> >                      "server next time. Good bye."));
> >     exit(EXIT_FAILURE);
> >   }
> > 
> > This mean that a client can crash the server. I thought it was already
> > fixed - this code comes from pre-Delta times.
> > Why do we still need it?
> > Can't we just ignore wrong packets?
> 
> If we get a bad attribute chunk we need to drop all attributes for that 
> player I think.  
> 
> -jason
> 
This patch does that.
--
mateusz

Index: common/packets.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/packets.c,v
retrieving revision 1.275
diff -u -r1.275 packets.c
--- common/packets.c    28 Apr 2005 17:19:52 -0000      1.275
+++ common/packets.c    9 Sep 2005 15:28:48 -0000
@@ -571,6 +571,24 @@
                                           packet_player_attribute_chunk
                                           *chunk)
 {
+  freelog(LOG_DEBUG, "received attribute chunk %d/%d %d", chunk->offset,
+         chunk->total_length, chunk->chunk_length);
+
+  if (chunk->total_length < 0
+      || chunk->total_length >= MAX_ATTRIBUTE_BLOCK
+      || chunk->offset < 0
+      || chunk->offset + chunk->chunk_length > chunk->total_length
+      || (chunk->offset != 0
+          && chunk->total_length != pplayer->attribute_block.length)) {
+    /* wrong attribute data */
+    if (pplayer->attribute_block.data) {
+      free(pplayer->attribute_block.data);
+      pplayer->attribute_block.data = NULL;
+    }
+    pplayer->attribute_block.length = 0;
+    freelog(LOG_ERROR, "Received wrong attribute chunk");
+    return;
+  }
   /* first one in a row */
   if (chunk->offset == 0) {
     if (pplayer->attribute_block.data) {
@@ -680,33 +698,6 @@
 
 }
 
-void post_receive_packet_player_attribute_chunk(struct connection *pc,
-                                               struct 
packet_player_attribute_chunk
-                                               *packet)
-{
-  /*
-   * Because of the changes in enum packet_type during the 1.12.1
-   * timeframe an old server will trigger the following condition.
-   */
-  if (packet->total_length <= 0
-      || packet->total_length >= MAX_ATTRIBUTE_BLOCK) {
-    freelog(LOG_FATAL, _("The server you tried to connect is too old "
-                        "(1.12.0 or earlier). Please choose another "
-                        "server next time. Good bye."));
-    exit(EXIT_FAILURE);
-  }
-  assert(packet->total_length > 0
-        && packet->total_length < MAX_ATTRIBUTE_BLOCK);
-  /* 500 bytes header, just to be sure */
-  assert(packet->chunk_length > 0
-        && packet->chunk_length < MAX_LEN_PACKET - 500);
-  assert(packet->chunk_length <= packet->total_length);
-  assert(packet->offset >= 0 && packet->offset < packet->total_length);
-
-  freelog(LOG_DEBUG, "received attribute chunk %d/%d %d", packet->offset,
-         packet->total_length, packet->chunk_length);
-}
-
 void post_receive_packet_game_state(struct connection *pc,
                                    struct packet_game_state *packet)
 {
Index: common/packets.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/packets.h,v
retrieving revision 1.166
diff -u -r1.166 packets.h
--- common/packets.h    8 Aug 2005 16:30:23 -0000       1.166
+++ common/packets.h    9 Sep 2005 15:29:00 -0000
@@ -84,9 +84,6 @@
 void pre_send_packet_player_attribute_chunk(struct connection *pc,
                                            struct packet_player_attribute_chunk
                                            *packet);
-void post_receive_packet_player_attribute_chunk(struct connection *pc, struct 
packet_player_attribute_chunk
-                                               *packet);
-
 void post_receive_packet_game_state(struct connection *pc,
                                    struct packet_game_state *packet);
 void post_send_packet_game_state(struct connection *pc,
Index: common/packets.def
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/packets.def,v
retrieving revision 1.155
diff -u -r1.155 packets.def
--- common/packets.def  4 Sep 2005 04:31:17 -0000       1.155
+++ common/packets.def  9 Sep 2005 15:29:00 -0000
@@ -697,7 +697,7 @@
 PACKET_PLAYER_ATTRIBUTE_BLOCK=46;cs
 end
 
-PACKET_PLAYER_ATTRIBUTE_CHUNK=47; pre-send,post-recv,sc,cs,handle-via-packet
+PACKET_PLAYER_ATTRIBUTE_CHUNK=47; pre-send, sc,cs,handle-via-packet
   UINT32 offset, total_length, chunk_length;
   /* to keep memory management simple don't allocate dynamic memory */
   MEMORY data[ATTRIBUTE_CHUNK_SIZE:chunk_length];