Dear Andy I've done an analysis of #331060 and come the exact same conclusions as you did.
The string management overhaul is currently being done upstream.
In the meantime I have attached a patch to #331060 which simply
increases the length of MAX_STR_LEN to 512. This is obviously not a
proper fix but reduces the likelihood of the problem appearing.
Cheers
Andree
On Fri, 2005-10-28 at 11:44 -0400, C. Andy Martin wrote:
> I have observed this bug, too, when increasing my exclusion list length
> from 275 to 299. Because it's so frustrating I spent some time digging
> through the source. It didn't take long to locate the cause: poor string
> management (at least for the Debian package source for 2.04-4, I didn't
> get the source straight from upstream).
>
> Here are the relevant lines of code (some are reordered for clarity)
> with a description of the problems and a calculation on the maximum
> length string that can be safely passed via the -E switch to
> mondoarchive at the end:
>
> from mystuff.h:
> #define MAX_STR_LEN 380
> [snip]
> #define malloc_string(x) { x = malloc(MAX_STR_LEN); if (!x) {
> fatal_error("Unable to malloc"); } x[0] = x[1] = '\0'; }
>
> from mondo/common/mondostructures.h:
>
> struct s_bkpinfo
> {
> [snip]
> /**
> * Directories to NOT back up. Ignored if make_filelist == FALSE.
> * Multiple directories should be separated by spaces. /tmp, /proc,
> * the scratchdir, and the tempdir are automatically excluded.
> */
> char exclude_paths[MAX_STR_LEN];
> [snip]
>
> from mondo/mondoarchive/mondo-cli.c:
>
> if (flag_set['E'])
> {
> if (bkpinfo->exclude_paths[0])
> { strcat(bkpinfo->exclude_paths," "); }
> strncpy (bkpinfo->exclude_paths+strlen(bkpinfo->exclude_paths),
> flag_val['E'], MAX_STR_LEN - strlen(bkpinfo->exclude_paths));
> }
> [snip]
> if (flag_set['N']) // exclude NFS mounts & devices
> {
> // strncpy(psz, list_of_NFS_devices_and_mounts(), MAX_STR_LEN);
> strncpy(psz, list_of_NFS_mounts_only(), MAX_STR_LEN);
> if (bkpinfo->exclude_paths[0]) { strncat(bkpinfo->exclude_paths, "
> ", MAX_STR_LEN); }
> strncat(bkpinfo->exclude_paths, psz, MAX_STR_LEN);
> log_msg(3, "-N means we're now excluding %s", bkpinfo->exclude_paths);
> }
> if (strlen(bkpinfo->exclude_paths) >= MAX_STR_LEN)
> {
> fatal_error("Your '-E' parameter is too long. Please use '-J'.
> (See manual.)");
> }
>
> from mondo/common/libmondo-filelist.c:
>
> int
> prepare_filelist (struct s_bkpinfo *bkpinfo)
> {
> [snip]
> res = mondo_makefilelist(
> MONDO_LOGFILE, bkpinfo->tmpdir, bkpinfo->scratchdir,
> bkpinfo->include_paths,
> bkpinfo->exclude_paths,
> bkpinfo->differential,
> NULL);
>
> [snip]
> int mondo_makefilelist(char*logfile, char*tmpdir, char*scratchdir,
> char*include_paths, char*excp, int differential, char *userdef_filelist)
> {
> [snip]
> char *sz_filelist, *exclude_paths, *tmp;
> [snip]
> if (!(exclude_paths = malloc(1000))) { fatal_error("Cannot malloc
> exclude_paths"); }
> [snip]
> sprintf(exclude_paths, " %s %s %s %s %s %s . .. \
> "MNT_CDROM" "MNT_FLOPPY" /media/cdrom /media/cdrecorder \
> /proc /sys /tmp /root/images/mondo /root/images/mindi ",
> excp,
> call_program_and_get_last_line_of_output("locate /win386.swp 2>
> /dev/null"),
> call_program_and_get_last_line_of_output("locate /hiberfil.sys 2>
> /dev/null"),
> call_program_and_get_last_line_of_output("locate /pagefile.sys 2>
> /dev/null"),
> (tmpdir[0]=='/' && tmpdir[1]=='/')?(tmpdir+1):tmpdir,
> (scratchdir[0]=='/' && scratchdir[1]=='/')?(scratchdir+1):scratchdir
> );
> log_msg(2, "Excluding paths = '%s'", exclude_paths);
> [snip]
>
> from mondo/common/libmondo-archive.c:
> int
> call_mindi_to_supply_boot_disks (struct s_bkpinfo *bkpinfo)
> {
> char *tmp;
> [snip]
> malloc_string ( tmp );
> [snip]
> sprintf (tmp,
> "echo \"%s\" | tr -s ' ' '\n' | grep -x \"/dev/.*\" | tr -s '\n' '
> ' | awk '{print $0\"\\n\";}'",
> bkpinfo->exclude_paths);
> [snip]
>
>
> OK, so from this code you can see many problems already with the way
> strings are handled in mondo, especially this exclusion path list string
> which can be lengthy for some systems. (For instance, we have many
> directories we exclude from backup because they contain downloads, or
> other cached elements which don't need to be restored and take up tons
> of space).
>
> There are several limits on the size of the exclusion string. The first
> is MAX_STR_LEN, which here is defined as 380 bytes (379 characters to
> leave space for the NUL). (This limit applies to the command line
> parameter + the NFS paths discovered, if the NFS option is enabled but
> no NFS paths are there, a space is appended). The code checks for
> overflow on the string (which is bad because the string would have
> _already_ overflowed!) and prints a fatal error on detection of the
> condition. Also, later on the exclusion string is amplified internally
> with a list of known exclusion points (such as /proc in Linux) with an
> internal limit of 1000 characters. This string is not protected from
> overflow at all, so we must hope the extra appended stuff is less than
> 1000-380=620 bytes (which proves true on my machine in my configuration,
> but...). The real kicker is when mindi is run, the 'tmp' string used to
> construct the command is limited also to 380 bytes, but contains the
> _entire_ exclusion path as an argument. This causes a buffer overflow
> here if the command string plus the length of the exclusion path is
> greater than 379 (which could readily be fixed by using a longer
> temporary). The extra length of the command is 83 characters, leaving a
> safe length on the command line of 296 (380-9-1 for the NUL terminator).
> If the NFS option is specified, this length is reduced by at least 1,
> more if NFS paths are found. As you can see, when I expanded my
> exclusion list from 275 to 299 characters I crossed this limit, and that
> is why the program segfaulted on mindi execution.
>
> Note: the '-J' switch printed above in the code is not very useful for
> _excluding_ paths, as it must be a list of all paths and files to
> _include_ (which means a script of program such as 'find' would need to
> be used to list all paths that are desired to backup while excluding the
> ones not desired).
>
> This should be enough information to forward to upstream to get at least
> the nasty mindi bug mentioned fixed. Obviously, there are many other
> potential buffer overflows here. The string management code could use an
> overhaul.
>
> -Andy
>
--
Andree Leidenfrost
Sydney - Australia
signature.asc
Description: This is a digitally signed message part
