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

[ Thread Index | Date Index | More Archives ]


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 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



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);
 		/* 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: */

Mail converted by MHonArc 2.6.19+