[Freeciv-Dev] Re: Security bugs in Freeciv 1.10.0 (PR#484)

[David Pfitzner <dwp@xxxxxxxxxxxxxx>]

On Fri, 28 Jul 2000 Taneli Huuskonen <huuskone@xxxxxxxxxxxxxx> wrote:

> I was looking at the source of Freeciv 1.10.0 to see if there was any
> obvious risk in allowing untrusted people to connect to a server running
> on my machine.  Unfortunately, a rather brief examination revealed a
> number of potential security holes.  At least one of them looked rather
> severe, meaning that a malicious player could overwrite any memory
> location with arbitrary data and consequently hijack the whole server
> process.

Unfortunately I'm not too surprised by your findings.  As the saying
goes "that's a known problem" :-/  Both the server and client _should_
do much more sanity-checking of the requests and data they receive
from each other.  (OTOH it once used to be even worse!!)

> The bug lies in the function handle_player_worklist( ).  It does no
> sanity checking whatsoever on its input data, but simply copies the
> supplied worklist into the player's array of worklists at the given
> index.  By sending a malformed packet with a carefully calculated
> index, an attacker can insert its contents anywhere in memory.

Yep, thanks.

> Moreover, since the name of the worklist is copied with strcpy( ),
> the cracker can overwrite even larger portions of memory than a single
> worklist with data containing no NUL bytes.

Actually I don't think this particular case of strcpy() is a problem
(at least for handle_player_worklist()) because the packet handling 
code _does_ take some care to check string lengths earlier in the
code path, when the packet_player_request is filled in.  Nevertheless 
redundant checking is a good idea (eg, I could be wrong), and 
copy_worklist() is called from a number of other places.

> There are other similar failures to check that array indices are within
> bounds, for instance in handle_unit_paradrop_to( ), but the one in the
> worklist handling routine looks like it might be the easiest to exploit
> as the player structures are statically allocated.
> 
> Furthermore, there are several smaller bugs that could, nevertheless, be
> exploited to crash a running server and/or to cheat in the game.  IMHO,
> the whole programme needs a security audit. 

Agreed.

> I am willing to help with
> that, but my time is limited, so I can only audit a smallish portion.

Any help appreciated :-)

Regards,
-- David