Bugzilla Bug 141563

#######################################################################

                             Luigi Auriemma

Application:  Freeciv
              http://www.freeciv.org
Versions:     <= 2.1.0-beta1 and SVN <= 15 Jul 2006
Platforms:    Windows, *nix, *BSD, MacOS and more
Bugs:         A] memcpy crash in generic_handle_player_attribute_chunk
              B] invalid memory access in handle_unit_orders
Exploitation: remote, versus server
Date:         23 Jul 2006
Author:       Luigi Auriemma
              e-mail: aluigi@autistici.org
              web:    aluigi.org


#######################################################################


1) Introduction
2) Bugs
3) The Code
4) Fix


#######################################################################

===============
1) Introduction
===============


Freeciv is an open source clone of the well known Civilization game.
The game supports also online gaming through its own metaserver (which
can be seen also on the web) and GGZ (http://www.ggzgamingzone.org).


#######################################################################

=======
2) Bugs
=======

--------------------------------------------------------
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 subsequent crash
- 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
size of the packet->length value which should not be bigger than 2000
(MAX_LEN_ROUTE) while is possible for an attacker 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++) {
  ...

please provide fixed ebuilds, thx

According to MDKSA-2006:135, remote attackers could also possibly execute
arbitrary code.

package masked for now.

Time to decide wether we need a masking GLSA.

Any good reason not to?

B3 is normally a vote.

Let me rephrase: Any good reason not to vote "yes"? :D

eh .. well ... I guess no :P

well ... mmm.... i'd vote a weak yes... since everybody can remotely crash the
server. There's no authentification, is there ? If there is, then consider my
vote as a "no".

I'd say no for DoS to a game.

Created an attachment (id=93307) [edit]
Patch for Part A of Advisory

This patch is a proposed solution for part A of the advisory.

Created an attachment (id=93308) [edit]
Patch for Part B of Advisory

This patch should solve Part B of Advisory.

Created an attachment (id=93309) [edit]
Patch for 2.0.8 to pull in security fix patches

I have attached the 2 patches I have created to correct these security holes
and
a patch for the ebuild.

I have checked that FreeCiv builds correctly with these patches and starts.

I have NOT checked whether the holes have been closed as I don't have
exploit code for this. However, the fixes are reasonable simple, so they
can be shown correct by inspection.

I hope these get closed by 2.0.9.

David -- thanks for the patches. Patch for Part B looks fine, however upstream
added an extra check for Part A:

http://svn.gna.org/viewcvs/freeciv/trunk/common/packets.c?rev=12146&r1=11710&r2=12146&makepatch=1&diff_format=h

Their fix for Part B is the same:

http://svn.gna.org/viewcvs/freeciv/trunk/server/unithand.c?dir_pagestart=50&rev=12106&r1=12065&r2=12106&makepatch=1&diff_format=h

Games team, please apply these two patches; thanks!

Applied both upstream patches and rev bumped to force out.

Comitted directly to stable and removed old versions.

This one is ready for GLSA decision.

I would vote no for a DoS of a game

but since some sources mention the possibility of remote code execution and
since we had a GLSA for a DoS in freeciv before (bug #125304 - GLSA 200603-11)

if there is a way to execute code via this flaw then: Yes
otherwise: 0.5 yes

guess I vote yes at the moment

I don't think I voted on this one the last time, so I take the freedom to vote
NO.

Because, honestly, it's a pity of your game server is DoSed, but it's not
breaking security, and nobody sane is going to run that on a box that offers
any important services and requires (high) availability.

Voting NO and closing.

Feel free to reopen if you disagree.