Re: [Nmh-workers] thoughts on tmp*

[Peter Maydell]

Lyndon Nerenberg wrote:
>The only sane solution to this problem is to build an internal version  
>of mkstemp() that acts like this:
>
>   tmp = NULL;
>   tmp = mhgetvar("tmpdir");
>   if (tmp == NULL) {
>     tmp = getenv("TMPDIR");
>   }
>   if (tmp == NULL) {
>     tmp = "/tmp";
>   }
>   /* do mkstemp-like file creation behavior */
>
>but with modifications to accommodate the mis-behavior that wants the  
>files to stick around.

Unfortunately you can't 'accommodate the mis-behavior' without leaving
security holes open (unless it all happens in a private-to-the-user
directory like ~/Mail...)

>This business of passing state around through scratch files needs to  
>be examined. State-passing is a distinct operation from storing  
>scratch data, and this makes me think there's a need to examine  
>everything that uses "/tmp" currently to disambiguate the two. My gut  
>feeling is that isolating the two operations will make it clear what  
>can be handled by mktemp() and friends vs. what needs to be  
>incorporated into the existing state tracking mechanism (such as it is).

and Joel wrote:
>If something is going to be fixed, I think it should be fixed properly.
>I haven't looked at this part of the code, but would it *really* be
>that difficult to change it to passing a file descriptor around instead
>of a name?

To pick a random example, in post.c if you've given a Bcc: header
then we create a temporary file and write out the message as seen
by the Bcc: receipients. Then later we call a function that says
"now send the message in this file". (Is that "scratch data" or
"state tracking" ? I'm not sure what distinction you're trying to
make there.) So yes, in theory you could fix this to do a 'seek
back to start' and have the post() function take an fd rather than
a filename (and change the other uses of it). But this is just one
of the 50 or so uses of m_scratch() or m_tmpfil(), and a fairly
straightforward one at that. Basically, the idea that you dump stuff
into a temp file and then use the name of the temp file later is very
ingrained in the way nmh is written. Inverting it all to use fds by
default would be a hugely invasive patch.

If we do want to do it that way the best approach I think would be
to start by providing a new tempfile-creating function with a
better API, something like:
 int m_mkstemp(char *template)
which returns the fd of an already-unlink()ed temporary file.
Then we can change uses of m_tmpfil and m_scratch over gradually.

Lyndon again:
>And in the short term, the immediate issue with securing scratch files  
>is best dealt with by incorporating a portable version of mkstemp()  
>into the source tree that uses an implied sub-directory for its files.  
>E.g.:
>
>       int foo = nmhtmp("mumbleXXXXX", 0);
>
>would create (in the default case) ${tmp}/nmh-1001/mumble10546-1 based  
>on the invokers real uid being 1001, a pid of 10546, and this being  
>iteration 1.

>From that URL I gave:

# However, as Michal Zalewski notes, [creating a subdir in /tmp/] is 
# a bad idea if there are tmp cleaners in use; instead, use a directory
# inside the user's HOME. 

Unless you change nmh so it can always work with just the fd of the
tempfile (and not its name) then putting things in /tmp/ at all is
insecure.

-- PMM