[crossfire] large numbers of parties causes weirdness

[Brendan Lally]

Well, there was a mention of a security audit a while back, here's a result 
from it, and even a sample exploit code :)

The party that a person is a member of is stored in a 'partyid' in the player 
struct. This is an sint16. each party gets an sint16 as the party number, and 
The following code, run as a script by a player is enough to cause breakages 
in the party system that crossfire uses.

#!/bin/bash
for i in $(seq 1 1 65536); 
do
echo "issue 1 1 party form $i"
sleep 0.05
done

there are three issues.
1) when it gets above about 1800 it causes clients to crash when doing party 
list. 

2) when it gets above 32768 the parties stop being functional (they can be 
joined, but aren't considered to be joined by any aspect of the game code).

3) when it gets above 65535, the partyid (which is stored as a sint16) will 
wrap around, allowing parties to be joined by the one 65536 places after them 
on the list. This avoids the password check. This would allow, for example 
someone to run that script and then join the party that corresponds to party 
dm on metalforge, without needing the password.

with regards to the third one a possible solution is (this compiles, haven't 
tested yet, but it should work.).


Index: server/c_party.c
===================================================================
RCS file: /cvsroot/crossfire/crossfire/server/c_party.c,v
retrieving revision 1.10
diff -C5 -r1.10 c_party.c
*** server/c_party.c    25 Dec 2004 18:09:29 -0000      1.10
--- server/c_party.c    28 Apr 2005 03:19:30 -0000
***************
*** 452,461 ****
--- 452,466 ----
                "You are already a member of party: %s"
                    ,tmpparty->partyname);
            return 1;
          }
          else {
+         if(find_party_struct(tmpparty->partyid)!=tmpparty) {
+             new_draw_info_format(NDI_UNIQUE, 0, op,
+               "ERROR Detected, party numbers are inconsistant, can't join 
party %s",params);
+             return 1;
+         }
            if(tmpparty->passwd[0] == '\0') {
              new_draw_info_format(NDI_UNIQUE, 0, op,
                        "You have joined party: %s",tmpparty->partyname);
              op->contr->party_number = tmpparty->partyid;
              snprintf( buf, MAX_BUF, "%s joins party %s", op->name, 
tmpparty->partyname );


other options include extending pl->party_number and partylist->partyid to be 
sint32's (that will make the exploit take longer to occur, but doesn't fix 
the underlying problem). This also adresses point 2.


Index: include/player.h
===================================================================
RCS file: /cvsroot/crossfire/crossfire/include/player.h,v
retrieving revision 1.37
diff -C5 -r1.37 player.h
*** include/player.h    15 Aug 2004 17:03:07 -0000      1.37
--- include/player.h    28 Apr 2005 03:33:18 -0000
***************
*** 170,181 ****
      time_t    last_save_time;
  #endif /* SAVE_INTERVAL */
  #ifdef AUTOSAVE
      uint32    last_save_tick;
  #endif
!     sint16    party_number;       /* Party number this player is part of */
!     sint16    party_number_to_join; /* used when player wants to join a 
party */
                                    /* but we will have to get password first 
*/
                                    /* so we have to remember which party to 
*/
                                    /* join */
      char    search_str[MAX_BUF];    /* Item we are looking for */
      sint16  encumbrance;          /* How much our player is encumbered  */
--- 170,181 ----
      time_t    last_save_time;
  #endif /* SAVE_INTERVAL */
  #ifdef AUTOSAVE
      uint32    last_save_tick;
  #endif
!     sint32    party_number;       /* Party number this player is part of */
!     sint32    party_number_to_join; /* used when player wants to join a 
party */
                                    /* but we will have to get password first 
*/
                                    /* so we have to remember which party to 
*/
                                    /* join */
      char    search_str[MAX_BUF];    /* Item we are looking for */
      sint16  encumbrance;          /* How much our player is encumbered  */
***************
*** 190,200 ****
  } player;


  /* not really the player, but tied pretty closely */
  typedef struct party_struct {
!     sint16 partyid;
      char * partyleader;
      char passwd[9];
      struct party_struct *next;
      char *partyname;

--- 190,200 ----
  } player;


  /* not really the player, but tied pretty closely */
  typedef struct party_struct {
!     sint32 partyid;
      char * partyleader;
      char passwd[9];
      struct party_struct *next;
      char *partyname;


This still doesn't deal with point 1 though, which may be more an issue with 
the network code.

possible things to do;
limit the number of parties a player can create (but without party removal 
that may be considered bad).

limit the global number of parties (that could mean the list could be spammed 
stopping party formation).

have party removal (harder to implement, though possible), and autoremove 
parties that no longer have active members (but then if they all log off, 
their party will go).

have a party player counter, which only changes when the party a player is a 
member of does, use this for party removal, and save the current party in the 
save file (but then there are abuses involving parties that gain passwords, 
or newly formed parties after a server reset).