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

[David Shaw]

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

On Wed, May 24, 2006 at 03:35:46AM +0100, Israel G. Lugo wrote:
> 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 think the proposed fix is a bit of overkill.  I'd rather do
something simpler, like (not tested yet):

Index: exec.c
===================================================================
- --- exec.c      (revision 4145)
+++ exec.c      (working copy)
@@ -129,8 +129,11 @@
   if(tmp==NULL)
     {
 #if defined (_WIN32)
- -      tmp=xmalloc(256);
- -      if(GetTempPath(256,tmp)==0)
+      char tmp[MAX_PATH+1];
+      int err;
+
+      err=GetTempPath(MAX_PATH+1,tmp);
+      if(err==0 || err>MAX_PATH+1)
        strcpy(tmp,"c:\\windows\\temp");
       else
        {
@@ -165,10 +168,6 @@

   sprintf(info->tempdir,"%s" DIRSEP_S "gpg-XXXXXX",tmp);

- -#if defined (_WIN32)
- -  xfree(tmp);
- -#endif
- -
   if(mkdtemp(info->tempdir)==NULL)
     log_error(_("can't create directory `%s': %s\n"),
              info->tempdir,strerror(errno));

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

There is no need to get the Windows directory specially, as
GetTempPath will return the Windows directory as a last resort.  It
tries $TMP, $TEMP, $USERPROFILE, and finally the Windows directory.  I
must admit I'm not terribly concerned that some version of Windows
somewhere can't handle c:\windows\temp.  If we get that far, it means
GetTempPath failed in some manner, and if GetTempPath fails with
MAX_PATH+1, then the system is sufficiently broken that we likely
don't *want* to continue executing. ;)

David
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.4-svn4114 (GNU/Linux)

iGoEARECACoFAkRz6qsjGGh0dHA6Ly93d3cuamFiYmVyd29ja3kuY29tL2tleS5h
c2MACgkQ4mZch0nhy8lSDgCeIJox4iiLBwcbANsT4oiHFkSnFX4An1iNht1egSrN
egRmrxXfKGwp0kKW
=znhe
-----END PGP SIGNATURE-----