[darcs-users] darcs patch: Create temporary files in temporary dire... (and 1 more)

[David Roundy]

On Fri, May 9, 2008 at 5:37 AM, Simon Marlow <marlowsd at gmail.com> wrote:
> David Roundy wrote:
>> On Thu, May 08, 2008 at 03:54:01PM -0700, Eric Kow wrote:
>>> Warning: I don't actually know if these work.
>>> They may even break something.
>>>
>>> Thu May  8 23:40:42 BST 2008  Eric Kow <E.Y.Kow at brighton.ac.uk>
>>>   * Create temporary files in temporary directory.
>>>
>>>   This is related to issue770 and possibly others: darcs sometimes attempts to
>>>   create temporary files in the current directory.  If the current directory is
>>>   one where the user does not have permission to write to, this may fail.  Using
>>>   a known temporary directory, i.e. specifically marked for that purpose, should
>>>   work better.
>>
>> The problem with this change is that we use the current directory for
>> security reasons, since it's very hard to safely use the /tmp directory
>> when communicating with external programs.  e.g. every time we run darcs
>> push, darcs creates the patch bundle in a temporary file before applying
>> it.  If we create this file in /tmp, then a malicious user might be able to
>> cleverly create a substitute patch bundle with the same name, which would
>> subsequently be applied to our repository.  There is a lot of literature on
>> safely creating temp files, but having only perused that literature, it's
>> not clear to me that what we'd want to do is possible.  The safe thing is
>> to create the temp files in a directory that evil people don't have write
>> access to, e.g. our repository directory.  So that's what we do.
>
> System.IO.openTempFile does the right thing, as long as the temporary
> directory is not NFS-mounted.  The trick with avoiding the races is to open
> the file with O_CREAT|O_EXCL, so if you manage to open it successfully you
> are guaranteed to have the only FD to a new empty file, and you choose the
> permissions so that it is only readable/writable by the current user
> (System.IO.openTempFile does all this).

That guarantees that the file is the "right" file when it's opened.
However, in the presence of TMPDIR cleaners, it can't guarantee that
if you re-open a file using the same name you get the same file as you
created.  Moreover, if you call an external program (e.g. an editor),
if that editor deletes the file and creates a new one with the same
name, there's a possible race condition.  Perhaps you'll claim that's
a bug in the editor, but if the editor wasn't explicitly written for
editing temporary files, then it's not a security bug in the editor
itself, it's only elevated to be a security bug when that editor is
used with darcs.

I know that there are safe ways to use /tmp, but I also know that I
don't know enough to determine what those safe approaches are--and
that there are some very clever tricks (to attack temp-file access)
that I don't understand.

Perhaps it's not a big deal.  But since darcs is one of those programs
that does by its very nature interact with the (potentially hostile)
outside world, I prefer to err on the side of caution.

> I am not completely sure that these guarantees apply on Windows, however.

It's certainly true that on Windows one can't atomically update a
file, which means an editor like emacs wouldn't be safe.  But
apparently Windows now has per-user temp directories, which makes this
issue simpler.

David