[Freeciv-Dev] Security bugs in Freeciv

[Luigi Auriemma <aluigi@xxxxxxxxxxxxx>]

Hey,

I want to report a couple of bugs I have found in Freeciv 2.1.0-beta1.
The following is an explanation of the problems

--------------------------------------------------------
A] memcpy crash in generic_handle_player_attribute_chunk
--------------------------------------------------------

handle_player_attribute_chunk (which points to
generic_handle_player_attribute_chunk) is a function used by both
client and server when a PACKET_PLAYER_ATTRIBUTE_CHUNK packet is
received.
The function acts like a reassembler of data for an allocated buffer
which can have a size of max 262144 bytes.
Exist two problems in this function:
- the length of the current chunk received (chunk_length) is not
  verified so using a negative value an attacker can bypass the initial
  check and can copy a huge amount of data ((unsigned)chunk_length) in
  the data buffer with the conseguent crash
- on 32 bit systems the check
  "chunk->offset + chunk->chunk_length > chunk->total_length" can be
  bypassed using a very big positive offset like 0x7fffffff which will
  allow the copying of data from our packet to the memory located at
  the allocated buffer plus the malformed offset.
  Doesn't seem possible to execute malicious code with this bug since
  the destination memory is usually invalid

From common/packets.c:

void generic_handle_player_attribute_chunk(struct player *pplayer,
                       const struct
                       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_buffer.length)) { /* wrong attribute data */
    if (pplayer->attribute_block_buffer.data) {
      free(pplayer->attribute_block_buffer.data);
      pplayer->attribute_block_buffer.data = NULL;
    }
    pplayer->attribute_block_buffer.length = 0;
    freelog(LOG_ERROR, "Received wrong attribute chunk");
    return;
  }
  /* first one in a row */
  if (chunk->offset == 0) {
    if (pplayer->attribute_block_buffer.data) {
      free(pplayer->attribute_block_buffer.data);
      pplayer->attribute_block_buffer.data = NULL;
    }
    pplayer->attribute_block_buffer.data = fc_malloc
(chunk->total_length); pplayer->attribute_block_buffer.length =
chunk->total_length; }
  memcpy((char *) (pplayer->attribute_block_buffer.data) + chunk->offset,
     chunk->data, chunk->chunk_length);
  ...


----------------------------------------------
B] invalid memory access in handle_unit_orders
----------------------------------------------

The server's function handle_unit_orders doesn't check the maximum
length of the packet->length value which should not be bigger than
2000 (MAX_LEN_ROUTE) while is possible to use any positive number.
The crash could require different tries (usually 3) before happening.

From server/unithand.c:

void handle_unit_orders(struct player *pplayer,
                        struct packet_unit_orders *packet)
{
  struct unit *punit = player_find_unit_by_id(pplayer, packet->unit_id);
  struct tile *src_tile = map_pos_to_tile(packet->src_x, packet->src_y);
  int i;

  if (!punit || packet->length < 0 || punit->activity != ACTIVITY_IDLE) {
    return;
  }

  if (src_tile != punit->tile) {
    /* Failed sanity check.  Usually this happens if the orders were sent
     * in the previous turn, and the client thought the unit was in a
     * different position than it's actually in.  The easy solution is to
     * discard the packet.  We don't send an error message to the client
     * here (though maybe we should?). */
    return;
  }

  for (i = 0; i < packet->length; i++) {
  ...

Actually there is no proof-of-concept available, I have tested the bugs
here in practice and they work perfectly.

I wait your reply.

And sorry for having sent the mail to both freeciv-dev and contact but I
don't know what is better for security bugs.


BYEZ


--- 
Luigi Auriemma
http://aluigi.org
http://mirror.aluigi.org