Re: [hatari-devel] Buffer overflow in Paths_Init()

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi,

On 1/17/19 9:17 PM, Christian Zietz wrote:
> for quite some time I have been puzzled why Hatari (snapshot builds for
> Windows from antarctica.no) sometimes worked and sometimes crashed with
> an HEAP_CORRUPTION error directly after start. Today, I finally figured
> out that it works if I start it from the Windows command line with an
> absolute path to the executable, but crashes if I start it with just a
> relative path to the executable.
>
> With that knowledge and a debugger I found the reason: a buffer overflow
> in Paths_Init(). When started from cmd.exe giving a relative path to the
> executable, psExecDir [1] will also only contain a relative path. Notice
> how in [2] only just enough memory is allocated for sDataDir to hold
> psExecDir, the path separator ("\" under Windows) and BIN2DATADIR, which
> is "." for the Windows build.
>
> However, File_MakeAbsoluteName, called from [3], will in the end write
> back the full, absolute filename into that buffer [4], thereby
> overflowing it.

And that function description even says that the given path should
have FILENAME_MAX allocation.


> Since Windows 10 has very good checkers for heap
> corruption (e.g. by buffer overflow), it then almost immediately
> terminates the offending process.

I've always though Linux has better / more accessible ones, but
strangely enough e.g. Valgrind doesn't complain about this.

Or did you mean that heap debugging is enabled by default?

(Glibc has some heap corruption checks always enabled, and Linux
distros enable by default some GCC checks for all packages, but
these are pretty lightweight.)


> Solution: Always allocate a sufficiently sized buffer (FILENAME_MAX+1?)
> for sDataDir.

I think FILENAME_MAX is supposed to include the terminating nil, as
that's also specified part of the filename (given to Linux system
calls).  It's supposed to be large enough for any file systems,
i.e. it's going to be at least 4K.

Attached is patch for you to test.


	- Eero

> Regards
> Christian
>
> [1]
> <https://hg.tuxfamily.org/mercurialroot/hatari/hatari/file/eab3159faa2c/src/paths.c#l312>
> [2]
> <https://hg.tuxfamily.org/mercurialroot/hatari/hatari/file/eab3159faa2c/src/paths.c#l317>
> [3]
> <https://hg.tuxfamily.org/mercurialroot/hatari/hatari/file/eab3159faa2c/src/paths.c#l328>
> [4]
> <https://hg.tuxfamily.org/mercurialroot/hatari/hatari/file/eab3159faa2c/src/file.c#l825>
diff -r eab3159faa2c src/paths.c
--- a/src/paths.c	Tue Jan 15 18:34:26 2019 +0200
+++ b/src/paths.c	Fri Jan 18 01:01:53 2019 +0200
@@ -312,16 +312,16 @@
 	psExecDir = Paths_InitExecDir(argv0);
 
 	/* Now create the datadir path name from the bindir path name: */
+	sDataDir = malloc(FILENAME_MAX);
 	if (psExecDir && strlen(psExecDir) > 0)
 	{
-		sDataDir = Str_Alloc(strlen(psExecDir) + 1 + strlen(BIN2DATADIR));
 		sprintf(sDataDir, "%s%c%s", psExecDir, PATHSEP, BIN2DATADIR);
 	}
 	else
 	{
 		/* bindir could not be determined, let's assume datadir is relative
 		 * to current working directory... */
-		sDataDir = Str_Dup(BIN2DATADIR);
+		strcpy(sDataDir, BIN2DATADIR);
 	}
 
 	/* And finally make a proper absolute path out of datadir: */