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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

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.  There's no way to do sanity checking on attributes and 
a "bad" set of attributes will crash all clients that try to connect to 
the game on reload (it might be possible to do checks on attributes at 
the client end to avoid this, but it wouldn't be easy).  The only fix I 
can think of is to get rid of attributes (which would IMO be a very good 
move).

-jason