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

[Israel G. Lugo]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
 
David Shaw wrote:
> we're not talking about a fatal flaw in a cipher here.  We're
> talking about *picking a temporary directory*.  (By the way, on
> Win32, GnuPG has MAX_PATH bytes hardcoded in multiple places.  It's
> not just temporary directories.)
The fall back to GetWindowsDirectory was just a lateral suggestion,
not the point in itself - I did agree, when you noted that GetTempPath
tries the Windows directory before giving up, that a fallback to
GetWindowsDirectory might be somewhat overdoing it. I would, however,
note that picking a temporary directory in itself is not the problem:
the problem is if we pick one which does not exist (such as
c:\windows\temp on any NT/2000 system) and fail because of that. I'm
thinking for example --refresh-keys; that would fail from this. I just
wanted to make the program as robust as possible and avoid failures
that could potentially be recovered from. But yes, it is may be
overkill, considering that GetTempPath already tries the Windows
directory.


> As I have said, I completely agree with you about the problem with
> temporary paths over 256 bytes.  There was never any question about
>  that, and in fact it has already been fixed.  The improvement that
> you proposed in the same mail, however, I disagree with.  I don't
> think that the cost/benefit ratio justifies it.  It's complex,
> involving (potentially) multiple passes just to get a temporary
> directory.  You also suggested fallbacks if GetTempDir did not
> succeed.  If this was a crucial part of the system, I'd probably be
> more eager to dive into it, but again, we're talking about picking
> a temporary directory here.
The passes thing is only there to take care of the race condition
where $TMP might change between us polling the temp length and
actually obtaining it the second time around. It is highly unlikely
that it would happen (except for some sort of timed attack), but in
the event that it does, I wanted the code to be protected against it.
This is, after all, a security program. In other words, I did not want
to see CERT publishing "race condition bug in GnuPG leads to crash
with potential data leak" or whatever one year from now, due to my
additions to the code.

In practical terms there would only ever be one pass, save for some
sort of timed attack (from which we would be safe thanks to the
passes) or some *really* bad timing from the user happening to change
$TMP precisely at that instant.

I just don't think we should treat a valid $TMP that happens to be
longer than FOO (256, MAX_PATH, whatever) as if it was an error. In
case of error, GetTempPath returns 0. If it returned a value larger
than our self-imposed FOO, there was no error; our FOO simply was not
big enough. We shouldn't die or fall back to some last-resort
hardcoded string that might work on some systems and might not work on
others... We have a tmp path right there. We just need to fetch it
properly. Now, as doing a bunch of trial and error attempts would be
rather silly, the simple and straight solution would seem to be simply
to poll for the size, malloc the correct amount, then fetch the
string. Nothing complicated about that. The one thing that does add a
couple of extra lines to the code would be the loop, but that is just
a necessary precaution to safeguard against the (very unlikely but
possible nonetheless) event of the tmp length changing between the
poll and the fetch.

Sure, it would look nicer if we didn't have to check after the fetch
to make sure we weren't sidetracked along the way, and it would look
even nicer if GetTempPath returned a malloc'ed string with the right
size already, letting us do it all in one single API call. But that's
not what we have to work with. What we do have is an API that returns
a tmp path of a size which, to us, needs to be treated as arbitrary,
since there is no documented maximum (further supported by the fact
that the function even has a mechanism to obtain the length at
runtime). How do we deal with such a function?

   1. Take a guess, think up some number that we reckon is large
      enough, and arbitrarily reject anything larger than that -
      "640KB of RAM is more than anyone will ever need"
   2. Use what the function provides us with. Poll it to get the
      required buffer size, then fetch the string with the proper buffer.


Yes, if GetTempPath returns an error because it can't find the temp
directory, we can just use the hardcoded string as a last-ditch
resort, for simplicity's sake - since GetTempPath will have already
tried to use the Windows directory before giving up. We can't be sure
that not finding the temp dir is the only possible reason for
GetTempPath to fail, though (if only the W32 API reference was kind
enough to at least hint at the possible error causes like real API
references do...). But still, for simplicity's sake, and since it's
not really a life-or-death part of the program, I suppose it does make
sense to just try hardcoded (at least it's better than outright dieing).

That is different, however, from a $TMP happening to be longer than a
given arbitrary length. That should not be treated the same as an
error; if GetTempPath is happy with it (doesn't return 0), why
shouldn't we be?


> I'm sorry you interpret someone disagreeing with you as an
> argument.
That is a straw man assertion... I do not interpret someone
disagreeing with me as an argument.

It is simply that I had made the report, worked on the patch and
offered the solution, with the best of intents, wanting to help by
fixing a bug and improving a given piece of code to make it more
robust. And, to be honest, the feeling I got from your replies was
more that of being dismissive, or defensive, than just a technical
disagreement (had you for example said initially what you said in the
paragraphs I've quoted in this email, I would've been left with a
totally different feeling). It can be, however, quite difficult to
properly interpret (and convey) tone and intent through text, more so
between people who do not know each other. So, if I misinterpreted
you, I do apologize.

Regards,
Israel
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
 
iD8DBQFEdUQcgn9msFnMGkcRAqUfAKDSn43sNTL8XgAwtO5NgLZDx5W++ACeOdXH
syvTdrGuIbv6DjRA21wHAXQ=
=lRvr
-----END PGP SIGNATURE-----