Re: [Freeciv-Dev] (PR#39437) attribute.c serialization error
http://bugs.freeciv.org/Ticket/Display.html?id=39437 > trunk revision 13068. S2_1 revision 13069. S2_0 revision 13070. ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
Re: [Freeciv-Dev] (PR#39437) attribute.c serialization error
http://bugs.freeciv.org/Ticket/Display.html?id=39437 >
While I have not delved deeply enough to discern the underlying cause of
attributes with bad lengths (probably a code path with an uninitialized
variable), here is a patch to allow the game to proceed without crashing.
Basically, whenever you have data coming from an external source, you
always should verify the form! Even on today's machines, there will always
be bit flips and corruption.
This checks the data at the savefile and network levels.
Of course, there's also the problem that each client can send up to
256KB of data! That's not a good design It really doesn't scale, as
the game gets longer, it will get slower and slower! (Another bug for
another day.)
Index: server/savegame.c
===
--- server/savegame.c (revision 13067)
+++ server/savegame.c (working copy)
@@ -2540,24 +2540,39 @@
load_player_units(plr, plrno, file);
- if (section_file_lookup(file, "player%d.attribute_v2_block_length", plrno)) {
-int raw_length1, raw_length2, part_nr, parts;
+ /* Toss any existing attribute_block (should not exist) */
+ if (plr->attribute_block.data) {
+free(plr->attribute_block.data);
+plr->attribute_block.data = NULL;
+ }
+
+ /* This is a big heap of opaque data for the client, check everything! */
+ plr->attribute_block.length = secfile_lookup_int_default(
+ file, 0, "player%d.attribute_v2_block_length", plrno);
+
+ if (0 > plr->attribute_block.length) {
+freelog(LOG_ERROR, "player%d.attribute_v2_block_length=%d too small",
+plrno,
+plr->attribute_block.length);
+plr->attribute_block.length = 0;
+ } else if (MAX_ATTRIBUTE_BLOCK < plr->attribute_block.length) {
+freelog(LOG_ERROR, "player%d.attribute_v2_block_length=%d too big (max
%d)",
+plrno,
+plr->attribute_block.length,
+MAX_ATTRIBUTE_BLOCK);
+plr->attribute_block.length = 0;
+ } else if (0 < plr->attribute_block.length) {
+int part_nr, parts;
+size_t actual_length;
size_t quoted_length;
char *quoted;
-raw_length1 =
- secfile_lookup_int(file, "player%d.attribute_v2_block_length", plrno);
-if (plr->attribute_block.data) {
- free(plr->attribute_block.data);
- plr->attribute_block.data = NULL;
-}
-plr->attribute_block.data = fc_malloc(raw_length1);
-plr->attribute_block.length = raw_length1;
+plr->attribute_block.data = fc_malloc(plr->attribute_block.length);
quoted_length = secfile_lookup_int
(file, "player%d.attribute_v2_block_length_quoted", plrno);
quoted = fc_malloc(quoted_length + 1);
-quoted[0] = 0;
+quoted[0] = '\0';
parts =
secfile_lookup_int(file, "player%d.attribute_v2_block_parts", plrno);
@@ -2566,9 +2581,13 @@
char *current = secfile_lookup_str(file,
"player%d.attribute_v2_block_data.part%d",
plrno, part_nr);
- if (!current)
+ if (!current) {
+freelog(LOG_ERROR, "attribute_v2_block_parts=%d actual=%d",
+parts,
+part_nr);
break;
- freelog(LOG_DEBUG, "quoted_length=%lu quoted=%lu current=%lu",
+ }
+ freelog(LOG_DEBUG, "attribute_v2_block_length_quoted=%lu have=%lu
part=%lu",
(unsigned long) quoted_length,
(unsigned long) strlen(quoted),
(unsigned long) strlen(current));
@@ -2576,17 +2595,17 @@
strcat(quoted, current);
}
if (quoted_length != strlen(quoted)) {
- freelog(LOG_DEBUG, "quoted_length=%lu quoted=%lu",
+ freelog(LOG_FATAL, "attribute_v2_block_length_quoted=%lu actual=%lu",
(unsigned long) quoted_length,
(unsigned long) strlen(quoted));
assert(0);
}
-raw_length2 =
+actual_length =
unquote_block(quoted,
plr->attribute_block.data,
plr->attribute_block.length);
-assert(raw_length1 == raw_length2);
+assert(actual_length == plr->attribute_block.length);
free(quoted);
}
}
@@ -3378,41 +3397,77 @@
secfile_insert_int(file, i, "player%d.total_ncities", plrno);
}
-#define PART_SIZE (2*1024)
+ /* This is a big heap of opaque data from the client. Although the binary
+ * format is not user editable, keep the lines short enough for debugging,
+ * and hope that data compression will keep the file a reasonable size.
+ * Note that the "quoted" format is a multiple of 3.
+ */
+#define PART_SIZE (3*256)
if (plr->attribute_block.data) {
char *quoted = quote_block(plr->attribute_block.data,
plr->attribute_block.length);
+char *quoted_at = strchr(quoted, ':');
+size_t bytes_left = strlen(quoted);
+size_t bytes_at_colon = 1 + (quoted_at - quoted);
+size_t bytes_adjust = bytes_at_colon % 3;
+int curre
Re: [Freeciv-Dev] (PR#39437) attribute.c serialization error
http://bugs.freeciv.org/Ticket/Display.html?id=39437 >
A similar crashing bug was reported to the development list over a year
ago, but not logged in the bug database?
Extract of Message
Subject: [Freeciv-Dev] Security bugs in Freeciv
Date: Sat, 8 Jul 2006 14:31:24 +0200
From: Luigi Auriemma <[EMAIL PROTECTED]>
...
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 0x7fff 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.
...
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
Re: [Freeciv-Dev] (PR#39437) attribute.c serialization error
http://bugs.freeciv.org/Ticket/Display.html?id=39437 > Ties-Jan wrote: > Attached I have a savegame with the error. > > Load the game and start as Ivan the Terrible (Russian). The game will crash. > Thanks for the report. You'll have to go back to your old savegame! === *** malloc: vm_allocate(size=553717760) failed (error code=3) *** malloc[22530]: error: Can't allocate region 0: Detected fatal error in mem.c line 41: 0: Out of memory trying to malloc 553713668 bytes at line 224 of attribute.c. shared.c:716: failed assertion `FALSE' Abort trap === pvalue = fc_malloc(value_length + 4); Sadly, the code says this is a known bug: /* There are some untraceable attribute bugs caused by the CMA that * can cause this to happen. I think the only safe thing to do is * to delete all attributes. Another symptom of the bug is the * value_length (above) is set to a random value, which can also * cause a bug. */ === / This method isn't endian safe and there will also be problems if sizeof(int) at serialization time is different from sizeof(int) at deserialization time. */ (heavy sigh) This is just so wrong ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
