[patch] Fix g10/exec.c (make_tempdir) [_WIN32] not properly handling GetTempPath

Israel G. Lugo ilugo at bridonsecurity.com
Wed May 24 04:35:46 CEST 2006


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
 
Greetings,

There is a problem in GnuPG, in the way g10/exec.c's make_tempdir()
obtains the temporary directory path on Windows systems. There is a
hardcoded limit of 256 characters (including the '\0') for the length
of the path (if a hard-limit is to be imposed, it should at least be
MAX_PATH+1). What's worse, there is no verification being made as to
whether the buffer was large enough to fit the path. In the event that
the hardcoded 256 bytes are not enough to hold the path plus '\0',
GetTempPath will do nothing and return the size of the buffer required
to hold the path. The current code will take that as success and
continue on, using uninitialized data as the temporary directory path.

The problem exists in both the stable and development branches, both
in the CVS and in the releases (1.4.3 and 1.9.20); the relevant
portion of code is the same in all of them.

I am attaching a patch that will make the appropriate corrections. As
mentioned the code is the same in both the stable and development
branches, so the patch is applicable to both. ChangeLog entry follows:

2006-05-24  Israel G. Lugo  <ilugo at bridonsecurity.com>

    * exec.c (make_tempdir) [_WIN32]: Modified to properly handle
    arbitrarily long temporary directory paths. Fix lack of verification
    that would cause uninitialized data to be used as temporary directory
    path if the path was too long to fit in the buffer.



I also saw something which I believe could be changed, in the same
function: in case of failure to obtain the temporary directory
(GetTempPath failure), make_tempdir() is using the hardcoded string
"c:\\windows\\temp". This will break on NT/2000 systems, for example
(the Windows directory is \WINNT), on some non-English systems and, of
course, on systems where the user chose to customize his/her
installation directory (or installed to a partition other than C:). I
would propose doing away with the hardcoded string and obtaining the
Windows directory from the API. If that fails as well then the
hardcoded string could be used as a last resort. I would be more than
willing to produce such a patch, if it was agreeable to the
development team.

Regards,
Israel G. Lugo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
 
iD8DBQFEc8aCgn9msFnMGkcRAmYWAKCz3Zdx9X/CUQf4QrCzCVUyTUCsIQCfVgUe
ny8uWCw9HVcXcqAeI42nX6Q=
=NyfZ
-----END PGP SIGNATURE-----

-------------- next part --------------
diff -durp gnupg-1.4.3/g10/exec.c gnupg-1.4.3-gettemppath/g10/exec.c
--- gnupg-1.4.3/g10/exec.c	Wed Sep  7 16:23:10 2005
+++ gnupg-1.4.3-gettemppath/g10/exec.c	Wed May 24 02:38:58 2006
@@ -129,13 +129,31 @@ static int make_tempdir(struct exec_info
   if(tmp==NULL)
     {
 #if defined (_WIN32)
-      tmp=xmalloc(256);
-      if(GetTempPath(256,tmp)==0)
-	strcpy(tmp,"c:\\windows\\temp");
-      else
+      /* Get size of the required buffer (including '\0') */
+      int len=(int)GetTempPath(0, NULL);
+      int tmp_siz;
+
+      /* The tmp path could change between us polling its length and
+       * getting it; loop just in case (should work at first pass). */
+      do
 	{
-	  int len=strlen(tmp);
+	  tmp_siz=len;
+
+	  if (tmp!=NULL)
+	    xfree(tmp);
+	  tmp=xmalloc(tmp_siz);
 
+	  len=(int)GetTempPath(tmp_siz,tmp);
+        }
+      while(len>tmp_siz);
+      /* len is the number of characters copied to tmp, not counting '\0' */
+      if(len==0)
+      {
+        xfree(tmp);
+        tmp=xstrdup("c:\\windows\\temp");
+      }
+      else
+	{
 	  /* GetTempPath may return with \ on the end */
 	  while(len>0 && tmp[len-1]=='\\')
 	    {


More information about the Gnupg-devel mailing list