Login
User Name:

Password:



Register
Forgot your password?
Vote for Us!
parse description bug
Dec 15, 2017, 10:08 pm
By Remcon
Couple bugs
Dec 12, 2017, 5:42 pm
By Remcon
Bug in disarm( )
Nov 12, 2017, 6:54 pm
By GatewaySysop
Bug in will_fall( )
Oct 23, 2017, 1:35 am
By GatewaySysop
Bug in do_zap( ), do_brandish( )
Oct 18, 2017, 1:52 pm
By GatewaySysop
LOP 1.45
Author: Remcon
Submitted by: Remcon
LOP Heroes Edition
Author: Vladaar
Submitted by: Vladaar
Heroes sound extras
Author: Vladaar
Submitted by: Vladaar
6Dragons 4.3
Author: Vladaar
Submitted by: Vladaar
Memwatch
Author: Johan Lindh
Submitted by: Vladaar
Users Online
CommonCrawl, DotBot, Yandex

Members: 0
Guests: 8
Stats
Files
Topics
Posts
Members
Newest Member
477
3,706
19,240
608
LAntorcha
Today's Birthdays
There are no member birthdays today.
Related Links
» SmaugMuds.org » Bugfix Lists » SWFOTE FUSS Bugfix List » [Bug] File security issues in...
Forum Rules | Mark all | Recent Posts

[Bug] File security issues in multiple modules
< Newer Topic :: Older Topic >

Pages:<< prev 1 next >>
Post is unread #1 Aug 19, 2006, 2:26 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
JoinedJan 1, 2002

Bug: File security issues in multiple modules
Danger: Critical - Data loss potential. Potential for unauthorized file access.
Found by: Remcon
Fixed by: Remcon/Samson

---

Note: This is going to be a long one. Make sure you have enough time and aren't half asleep or something. A mistake in applying this fix can be very costly.

db.c

Locate:
char *str_dup( char const *str )
{
   static char *ret;
   int len;

   if( !str )
      return NULL;

   len = strlen( str ) + 1;

   CREATE( ret, char, len );
   mudstrlcpy( ret, str, MAX_STRING_LENGTH );
   return ret;
}


Below that, add:
bool is_valid_filename( CHAR_DATA *ch, const char *direct, const char *filename )
{
   char newfilename[256];
   struct stat fst;

   /* Length restrictions */
   if( !filename || filename[0] == '\0' || strlen( filename ) < 3 )
   {
      if( !filename || !str_cmp( filename, "" ) )
         send_to_char( "Empty filename is not valid.\r\n", ch );
      else
         ch_printf( ch, "%s: Filename is too short.\r\n", filename );
      return FALSE;
   }

   /* Illegal characters */
   if( strstr( filename, ".." ) || strstr( filename, "/" ) || strstr( filename, "\\" ) )
   {
      send_to_char( "A filename may not contain a '..', '/', or '\\' in it.\r\n", ch );
      return FALSE;
   }

   /* If that filename is already being used lets not allow it now to be on the safe side */
   snprintf( newfilename, sizeof( newfilename ), "%s%s", direct, filename );
   if( stat( newfilename, &fst ) != -1 )
   {
      ch_printf( ch, "%s is already an existing filename.\r\n", newfilename );
      return FALSE;
   }

   /* If we got here assume its valid */
   return TRUE;
}


mud.h

Locate:
void show_file args( ( CHAR_DATA * ch, char *filename ) );


Below that, add:
bool is_valid_filename( CHAR_DATA *ch, const char *direct, const char *filename );


clans.c, do_setclan

Locate:
   if( !str_cmp( arg2, "name" ) )
   {
      STRFREE( clan->name );
      clan->name = STRALLOC( argument );
      send_to_char( "Done.\r\n", ch );
      save_clan( clan );
      return;
   }

   if( !str_cmp( arg2, "filename" ) )
   {
      DISPOSE( clan->filename );
      clan->filename = str_dup( argument );
      send_to_char( "Done.\r\n", ch );
      save_clan( clan );
      write_clan_list(  );
      return;
   }


Change to:
   if( !str_cmp( arg2, "name" ) )
   {
      CLAN_DATA *uclan = NULL;

      if( !argument || argument[0] == '\0' )
      {
         send_to_char( "You can't name a clan nothing.\r\n", ch );
         return;
      }
      if( ( uclan = get_clan( argument ) ) )
      {
         send_to_char( "There is already another clan with that name.\r\n", ch );
         return;
      }
      STRFREE( clan->name );
      clan->name = STRALLOC( argument );
      send_to_char( "Done.\r\n", ch );
      save_clan( clan );
      return;
   }

   if( !str_cmp( arg2, "filename" ) )
   {
      char filename[256];

      if( !is_valid_filename( ch, CLAN_DIR, argument ) )
         return;

      snprintf( filename, sizeof( filename ), "%s%s", CLAN_DIR, clan->filename );
      if( !remove( filename ) )
         send_to_char( "Old clan file deleted.\r\n", ch );

      DISPOSE( clan->filename );
      clan->filename = str_dup( argument );
      send_to_char( "Done.\r\n", ch );
      save_clan( clan );
      write_clan_list(  );
      return;
   }


build.c, do_aset

Locate:
   if( !str_cmp( arg2, "name" ) )
   {
      DISPOSE( tarea->name );
      tarea->name = str_dup( argument );
      send_to_char( "Done.\r\n", ch );
      return;
   }


Change to:
   if( !str_cmp( arg2, "name" ) )
   {
      AREA_DATA *uarea;

      if( !argument || argument[0] == '\0' )
      {
         send_to_char( "You can't set an area's name to nothing.\r\n", ch );
         return;
      }

      for( uarea = first_area; uarea; uarea = uarea->next )
      {
         if( !str_cmp( uarea->name, argument ) )
         {
            send_to_char( "There is already an installed area with that name.\r\n", ch );
            return;
         }
      }

      for( uarea = first_build; uarea; uarea = uarea->next )
      {
         if( !str_cmp( uarea->name, argument ) )
         {
            send_to_char( "There is already a prototype area with that name.\r\n", ch );
            return;
         }
      }
      DISPOSE( tarea->name );
      tarea->name = str_dup( argument );
      send_to_char( "Done.\r\n", ch );
      return;
   }


Locate:
   if( !str_cmp( arg2, "filename" ) )
   {
      DISPOSE( tarea->filename );
      tarea->filename = str_dup( argument );
      write_area_list(  );
      fold_area( tarea, tarea->filename, TRUE );
      send_to_char( "Done.\r\n", ch );
      return;
   }


Change to:
   if( !str_cmp( arg2, "filename" ) )
   {
      char filename[256];

      if( proto )
      {
         send_to_char( "You should only change the filename of installed areas.\r\n", ch );
         return;
      }

      if( !is_valid_filename( ch, "", argument ) )
         return;

      strncpy( filename, tarea->filename, 256 );
      DISPOSE( tarea->filename );
      tarea->filename = str_dup( argument );
      rename( filename, tarea->filename );
      write_area_list(  );
      send_to_char( "Done.\r\n", ch );
      return;
   }


boards.c, do_bset

Locate:
   if( !str_cmp( arg2, "filename" ) )
   {
      if( !argument || argument[0] == '\0' )
      {
         send_to_char( "No filename specified.\r\n", ch );
         return;
      }
      DISPOSE( board->note_file );
      board->note_file = str_dup( argument );
      write_boards_txt(  );
      send_to_char( "Done.\r\n", ch );
      return;
   }


Change to:
   if( !str_cmp( arg2, "filename" ) )
   {
      char filename[256];

      if( !is_valid_filename( ch, BOARD_DIR, argument ) )
         return;

      snprintf( filename, sizeof( filename ), "%s%s", BOARD_DIR, board->note_file );
      if( !remove( filename ) )
         send_to_char( "Old board file deleted.\r\n", ch );

      DISPOSE( board->note_file );
      board->note_file = str_dup( argument );
      write_boards_txt(  );
      send_to_char( "Done.  (board's filename set)\r\n", ch );
      return;
   }


color.c, do_color

Locate:
   if( !str_cmp( arg, "savetheme" ) && IS_IMMORTAL( ch ) )
   {
      FILE *fp;
      char filename[256];
      int x;

      if( !argument || argument[0] == '\0' )
      {
         send_to_char( "You must specify a name for this theme to save it.\r\n", ch );
         return;
      }

      snprintf( filename, 256, "%s%s", COLOR_DIR, argument );
      if( !( fp = fopen( filename, "w" ) ) )
      {
         ch_printf( ch, "Unable to write to color file %s\r\n", filename );
         return;
      }
      fprintf( fp, "%s", "#COLORTHEME\n" );
      fprintf( fp, "Name         %s~\n", argument );
      fprintf( fp, "MaxColors    %d\n", MAX_COLORS );
      fprintf( fp, "%s", "Colors      " );
      for( x = 0; x < MAX_COLORS; ++x )
         fprintf( fp, " %d", ch->colors[x] );
      fprintf( fp, "%s", "\nEnd\n" );
      fclose( fp );
      fp = NULL;
      ch_printf( ch, "Color theme %s saved.\r\n", argument );
      return;
   }

   if( !str_cmp( arg, "theme" ) )
   {
      FILE *fp;
      char filename[256];
      int max_colors = 0;

      if( !argument || argument[0] == '\0' )
      {
         show_colorthemes( ch );
         return;
      }

      snprintf( filename, 256, "%s%s", COLOR_DIR, argument );
      if( !( fp = fopen( filename, "r" ) ) )
      {
         ch_printf( ch, "There is no theme called %s.\r\n", argument );
         return;
      }

      while( !feof( fp ) )
      {
         char *word = fread_word( fp );
         if( !str_cmp( word, "MaxColors" ) )
         {
            max_colors = fread_number( fp );
            continue;
         }
         if( !str_cmp( word, "Colors" ) )
         {
            int x;

            for( x = 0; x < max_colors; ++x )
               ch->colors[x] = fread_number( fp );
            continue;
         }
         if( !str_cmp( word, "End" ) )
         {
            fclose( fp );
            fp = NULL;
            ch_printf( ch, "Color theme has been changed to %s.\r\n", argument );
            save_char_obj( ch );
            return;
         }
      }
      fclose( fp );
      fp = NULL;
      ch_printf( ch, "An error occured while trying to set color theme %s.\r\n", argument );
      return;
   }


Change to:
   if( !str_cmp( arg, "savetheme" ) && IS_IMMORTAL( ch ) )
   {
      FILE *fp;
      char filename[256];
      int x;

      if( !argument || argument[0] == '\0' )
      {
         send_to_char( "You must specify a name for this theme to save it.\r\n", ch );
         return;
      }

      if( strstr( argument, ".." ) || strstr( argument, "/" ) || strstr( argument, "\\" ) )
      {
         send_to_char( "Invalid theme name.\r\n", ch );
         return;
      }

      snprintf( filename, sizeof( filename ), "%s%s", COLOR_DIR, argument );
      if( !( fp = fopen( filename, "w" ) ) )
      {
         ch_printf( ch, "Unable to write to color file %s\r\n", filename );
         return;
      }
      fprintf( fp, "%s", "#COLORTHEME\n" );
      fprintf( fp, "Name         %s~\n", argument );
      fprintf( fp, "MaxColors    %d\n", MAX_COLORS );
      fprintf( fp, "%s", "Colors      " );
      for( x = 0; x < MAX_COLORS; ++x )
         fprintf( fp, " %d", ch->colors[x] );
      fprintf( fp, "%s", "\nEnd\n" );
      fclose( fp );
      fp = NULL;
      ch_printf( ch, "Color theme %s saved.\r\n", argument );
      return;
   }

   if( !str_cmp( arg, "theme" ) )
   {
      FILE *fp;
      char filename[256];
      int max_colors = 0;

      if( !argument || argument[0] == '\0' )
      {
         show_colorthemes( ch );
         return;
      }

      if( strstr( argument, ".." ) || strstr( argument, "/" ) || strstr( argument, "\\" ) )
      {
         send_to_char( "Invalid theme.\r\n", ch );
         return;
      }

      snprintf( filename, sizeof( filename ), "%s%s", COLOR_DIR, argument );
      if( !( fp = fopen( filename, "r" ) ) )
      {
         ch_printf( ch, "There is no theme called %s.\r\n", argument );
         return;
      }

      while( !feof( fp ) )
      {
         char *word = fread_word( fp );
         if( !str_cmp( word, "MaxColors" ) )
         {
            max_colors = fread_number( fp );
            continue;
         }
         if( !str_cmp( word, "Colors" ) )
         {
            int x;

            for( x = 0; x < max_colors; ++x )
               ch->colors[x] = fread_number( fp );
            continue;
         }
         if( !str_cmp( word, "End" ) )
         {
            fclose( fp );
            fp = NULL;
            ch_printf( ch, "Color theme has been changed to %s.\r\n", argument );
            save_char_obj( ch );
            return;
         }
      }
      fclose( fp );
      fp = NULL;
      ch_printf( ch, "An error occured while trying to set color theme %s.\r\n", argument );
      return;
   }


There is a great deal of insecure filename handling throughout the Smaug code. Some of it has already been addressed in past fixes. This fix is an attempt to address the issue once and for all by providing a standard method for blocking invalid filenames that can cause overwrites of unintended files and the reading of unauthorized files. The main thrust of the fix is to block the following sets of characters: '..', '\', and '/'. Without these 3 character sets being allowed, no filename should be able to get saved to an improper directory and no files should be readable outside of the intended locations. The fix also goes one step further and blocks renaming files to something which already exists. This too has been a source of serious data loss in Smaug for some time. Apparently the Smaug devs intended for everyone to be totally aware of existing filenames wherever they may be.

This fix is a combination of things Remcon did and my adjustments to it. For instance, you might notice that the single '.' character is not blocked. This is because in many instances, naming a file requires a . to be in it somewhere for the extension. One may also notice that the directory is not checked, only the filename itself. This is because the code should be able to assume that the hardcoded directory definitions are valid. If those are not, well, then that's just stupidity on the part of the person who modifies them. Stupidity as we all know isn't something that can be patched out.
       
Pages:<< prev 1 next >>