[Freeciv-Dev] Re: civclient loops infinitely with corrupted network data (PR#1247)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Jan 29, 2002 at 07:32:04PM -0800, Vasco Alexandre Da Silva Costa wrote:
> On Tue, 29 Jan 2002, Raimar Falke wrote:
> 
> > On Tue, Jan 29, 2002 at 07:03:04AM -0800, Christian Knoke wrote:
> > > Recent CVS + CMA 3.4
> > > 
> > > Civclient loops, spoiling
> > > 
> > > 1: Received unknown packet (type 0) from server!
> > > 1: received short packet (type 0, len 0) from server
> > > 
> > > again and again, seemingly because it receives data
> > > it doesn't expect, at a time when the other side
> > > already has aborted. I report it, though it won't
> > > happen with an intact civserver:
> > 
> > It is known that the network part isn't robust. It is low priority
> > since nobody complained about it and nobody is interrested in it.
> 
> Erm, i don't consider this low priority... Unless CMA is merely optional
> at runtime. (i believe it is?)

Christian has a program which acts changes/corrupts the data stream
between client and server. Lets see what can happen with the current
(non-CMA) code: 

 - lets suppose Christian's program change the size field of a
 packet_generic_integer from 7 to 8. First we get a "short packet"
 message. And then we have a problem since the packet layer has read 8
 bytes and now the stream is out of sync. No way to recover from this.

 - second example: suppose we have only fixed length packets and no
 more size fields. Now Christian's program change the type field from
 a packet_generic_integer (payload size of 4) to a
 packet_generic_values (payload size of 10). Same problem as above:
 de-sync which can't recovered.

One solution to this is to introduce checksums for every packet. But
this is clearly overkill since normally the OS (using TCP/IP) ensures
that the data isn't corrupted.

So fixing corruption at this low level (malformed packets) is quite
useless. But there is another level above: what happens if a packet's
syntax is correct (it is well-formed) but the values it does contain
are bogus (move the unit of an enemy)? The server should
discard/reject the packet. Although we have code which does this at
some points I have no idea how save the code is against such
attacks. This would be an area where some work would yield a benefit.

So this has nothing to do with the CMA (the CMA also does no network
related changes).

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Premature optimization is the root of all evil."
    -- D. E. Knuth in "Structured Programming with go to Statements"