I get the following problem when running changes on a remote repo, if I'm
not currently in a darcs repository:

$ darcs changes --repo=http://darcs.net

darcs failed:  Not a repository: http://darcs.net (Failed to create
    temporary file darcs: permission denied (Permission denied))

I suspect that what's happening is that darcs is setting the working
directory to the root directory in a naive attempt to put ourselves into
the main directory of a repository, and my user doesn't have write
permissions there for creating a temporary file.

If I run this command in a darcs repository, there's no problem.

And by the way, I haven't looked at the source code for this at all (and am
not sure when I'll have time to look into this).

I only tested this using:

$ darcs --exact-version
darcs compiled on Oct 31 2006, at 01:58:00
# configured Mon Oct  9 15:12:44 EDT 2006
./configure --no-create --no-recursion

Context:

[TAG 1.0.9rc1
Tommy Pettersson <ptp@lysator.liu.se>**20061008175207]

so I don't know if this bug might be fixed already (although I doubt it).

David

When specifying a remote repository, the path to the temporary file was
'//'.  So it would try to create files in the root of the filesystem.
If you run "darcs changes --last 1 --repo http://darcs.net" as root it
works because of this.

Attached is a patch that uses tempdir_loc for withTemp.  Can anyone
think of a reason that makes this a bad idea?

I wrote a test for this too, but I'm not sure how to go about making it
fail.  To do so, we have to be outside of the repository completely
(running in tests/ doesn't work because it goes back a dir to find a
repo).  I'm not sure how we can guarantee we will be outside of the
repository.  If anyone has any ideas, I'll update the test and send it.

Attachments

• issue385-fix.dpatch

On Tue, Jan 30, 2007 at 09:44:06AM -0500, Zachary P. Landau wrote:
> > I suspect that what's happening is that darcs is setting the working
> > directory to the root directory in a naive attempt to put ourselves into
> > the main directory of a repository, and my user doesn't have write
> > permissions there for creating a temporary file.
> 
> When specifying a remote repository, the path to the temporary file was
> '//'.  So it would try to create files in the root of the filesystem.
> If you run "darcs changes --last 1 --repo http://darcs.net" as root it
> works because of this.
> 
> Attached is a patch that uses tempdir_loc for withTemp.  Can anyone
> think of a reason that makes this a bad idea?

tempdir_loc can sometimes fall back to returning current dir,
but of course noting is lost by that. The only thing I can think
of is if the command is run in an existing repo's root dir, in
which case tempdir_loc can sometimes fall back to that repo's
locally specified temp dir (specified in
_darcs/prefs/...something). Not that there's anything wrong with
that... but you know how users are, they put some trigger on
that special dir to light up the Christmas tree and won't
understand why on earth it lights up when they access a totally
different, remote repo... :-)

> I wrote a test for this too, but I'm not sure how to go about making it
> fail.  To do so, we have to be outside of the repository completely
> (running in tests/ doesn't work because it goes back a dir to find a
> repo).  I'm not sure how we can guarantee we will be outside of the
> repository.  If anyone has any ideas, I'll update the test and send it.

You could test for the presence of a /tmp directory (and make
sure it doesn't contain a _darcs dir) and use it or else just
skip the test.

On Tue, Jan 30, 2007 at 09:44:06AM -0500, Zachary P. Landau wrote:
>  withTemp :: (String -> IO a) -> IO a
>  withTemp = bracket get_empty_file removeFileMayNotExist
> - -    where get_empty_file = do (h,f) <- mkstemp "darcs"
> +    where get_empty_file = do tmpdir <- tempdir_loc
> +                              (h,f) <- mkstemp (tmpdir ++ "darcs")
>                                hClose h
>                                return f

This change would introduce temporary file creation bugs.  We currently
rely on withTemp by default creating files in a non-world-writeable
directory in order to avoid the need to audit all our tempfile creation
code.  If we introduced this change, we'd have to go through and also
audit both every use of withTemp, and the code of withTemp itself, if we
wanted to regain the current level of security.  And these audits would
need to be done by someone who is more competent than me.

So far as I know, there's no secure way to pass a temp file generated in
/tmp to an exec'ed program that requires it as a command-line parameter.

It's just a whole can of worms.  I'm sure it doesn't matter to most of our
users, but some fraction I'm sure use darcs on trusted systems that allow
untrusted users to simultaneously run, and we don't want those users to be
able to modify the execution of darcs in any way.

I don't mean there's no solution, but that I am not aware of a solution,
and will want to have the assurance of someone with experience that we
aren't shooting ourselves in the foot.

Actually, now that I think about it, I suspect that withTemp is inherently
insecure, based on what I know, if the temp file is created in /tmp.  My
understanding is that any use of a filename in /tmp is a bug, and of course
withTemp only allows use of the filename.  I don't know all the tricks that
can be used to take advantage of insecure temp file handling, but that's my
understanding.  Which is why we don't use /tmp for most of our temp files.
-- 
David Roundy
Department of Physics
Oregon State University

Attachments

• unnamed

On Wed, Jan 31, 2007 at 12:46:43AM +0000, David Roundy wrote:
> 
> David Roundy <droundy@darcs.net> added the comment:
> 
> On Tue, Jan 30, 2007 at 09:44:06AM -0500, Zachary P. Landau wrote:
> >  withTemp :: (String -> IO a) -> IO a
> >  withTemp = bracket get_empty_file removeFileMayNotExist
> > - -    where get_empty_file = do (h,f) <- mkstemp "darcs"
> > +    where get_empty_file = do tmpdir <- tempdir_loc
> > +                              (h,f) <- mkstemp (tmpdir ++ "darcs")
> >                                hClose h
> >                                return f
> 
> This change would introduce temporary file creation bugs.  We currently
> rely on withTemp by default creating files in a non-world-writeable
> directory in order to avoid the need to audit all our tempfile creation
> code.  If we introduced this change, we'd have to go through and also
> audit both every use of withTemp, and the code of withTemp itself, if we
> wanted to regain the current level of security.  And these audits would
> need to be done by someone who is more competent than me.
> 
> So far as I know, there's no secure way to pass a temp file generated in
> /tmp to an exec'ed program that requires it as a command-line parameter.

I thought that was the entire point of mkstemp.  It returns a file
handle, not a file name.  As in, it has made sure that you are the owner
of the file and that nobody could race you to opening it.  Can someone
verify this?

On Tue, Jan 30, 2007 at 08:44:06PM -0500, Zachary P. Landau wrote:
> On Wed, Jan 31, 2007 at 12:46:43AM +0000, David Roundy wrote:
> > 
> > David Roundy <droundy@darcs.net> added the comment:
> > 
> > On Tue, Jan 30, 2007 at 09:44:06AM -0500, Zachary P. Landau wrote:
> > >  withTemp :: (String -> IO a) -> IO a
> > >  withTemp = bracket get_empty_file removeFileMayNotExist
> > > - -    where get_empty_file = do (h,f) <- mkstemp "darcs"
> > > +    where get_empty_file = do tmpdir <- tempdir_loc
> > > +                              (h,f) <- mkstemp (tmpdir ++ "darcs")
> > >                                hClose h
> > >                                return f
> > 
> > This change would introduce temporary file creation bugs.  We currently
> > rely on withTemp by default creating files in a non-world-writeable
> > directory in order to avoid the need to audit all our tempfile creation
> > code.  If we introduced this change, we'd have to go through and also
> > audit both every use of withTemp, and the code of withTemp itself, if we
> > wanted to regain the current level of security.  And these audits would
> > need to be done by someone who is more competent than me.
> > 
> > So far as I know, there's no secure way to pass a temp file generated in
> > /tmp to an exec'ed program that requires it as a command-line parameter.
> 
> I thought that was the entire point of mkstemp.  It returns a file
> handle, not a file name.  As in, it has made sure that you are the owner
> of the file and that nobody could race you to opening it.  Can someone
> verify this?

That's correct, but if you look at this code, we immediately close the
handle and only use the name.  The reason is that this code is used when
interacting with other programs (e.g. emacs), and there's no safe way to do
that in /tmp that I'm aware of.  We also use it in several other cases
where we're sloppy, and those cases could be moved to safely use /tmp, and
that's what should be done.  This may require a bit of a refactoring of our
API.
-- 
David Roundy
Department of Physics
Oregon State University

> > I thought that was the entire point of mkstemp.  It returns a file
> > handle, not a file name.  As in, it has made sure that you are the owner
> > of the file and that nobody could race you to opening it.  Can someone
> > verify this?
> 
> That's correct, but if you look at this code, we immediately close the
> handle and only use the name.  The reason is that this code is used when
> interacting with other programs (e.g. emacs), and there's no safe way to do
> that in /tmp that I'm aware of.  We also use it in several other cases
> where we're sloppy, and those cases could be moved to safely use /tmp, and
> that's what should be done.  This may require a bit of a refactoring of our
> API.

Whoops, I was only focusing on that first line.  I see your point.

I wonder how kosher it would be to create a directory called something
like $HOME/.darcs/tmp/.  (And by $HOME I mean whatever makes sense for
your OS, which is often a problem in itself).  Plenty of programs make
their own little space in $HOME/.programname, so I don't see why we
couldn't.

If that were the case, we could hopefully just always use that directory
for temporary files.  Otherwise, we'll have to start looking at the API,
as you said, to determine when we can use the current directory and when
we have to use some other directory.

On Tue, Jan 30, 2007 at 09:54:46PM -0500, Zachary P. Landau wrote:
> > That's correct, but if you look at this code, we immediately close the
> > handle and only use the name.  The reason is that this code is used
> > when interacting with other programs (e.g. emacs), and there's no safe
> > way to do that in /tmp that I'm aware of.  We also use it in several
> > other cases where we're sloppy, and those cases could be moved to
> > safely use /tmp, and that's what should be done.  This may require a
> > bit of a refactoring of our API.
> 
> Whoops, I was only focusing on that first line.  I see your point.
> 
> I wonder how kosher it would be to create a directory called something
> like $HOME/.darcs/tmp/.  (And by $HOME I mean whatever makes sense for
> your OS, which is often a problem in itself).  Plenty of programs make
> their own little space in $HOME/.programname, so I don't see why we
> couldn't.
> 
> If that were the case, we could hopefully just always use that directory
> for temporary files.  Otherwise, we'll have to start looking at the API,
> as you said, to determine when we can use the current directory and when
> we have to use some other directory.

I suspect that would be all right, although I'd prefer to use it only in
emergencies (i.e. if the place we currently try to write the temp file
can't be written to).  And if DARCS_TMP is set, I think it'd be okay to use
that.
-- 
David Roundy
http://www.darcs.net

> Actually, now that I think about it, I suspect that withTemp is inherently
> insecure, based on what I know, if the temp file is created in /tmp.  My
> understanding is that any use of a filename in /tmp is a bug, and of course
> withTemp only allows use of the filename.  I don't know all the tricks that
> can be used to take advantage of insecure temp file handling, but that's my
> understanding.  Which is why we don't use /tmp for most of our temp files.

Today I realized that I was actually trying to solve the wrong problem.
The issue of where and how to create temporary files is something that
might have a better solution.  But with the current logic, connecting to
a remote repository outside of a local repository should still be able
to make a temporary file in $TMPDIR, $DARCS_TMP, or the current
directory.

Darcs tries to create the temporary file at the top of the root
directory.  I believe this is because darcs first tries to find a
directory somewhere in our current path.  The seekPos function keeps
changing the directory until it gets back to /, and then returns saying
it couldn't find a repository.  I believe it'd be better if, when
seekPos couldn't find a repository, it restored the directory it started
in.

I'll try to get a patch together tomorrow.  As for tonight, I just got
off of work and I have beer to drink and movies to watch.

On Sat, Feb 03, 2007 at 12:43:47 -0500, Zachary P. Landau wrote:
> Okay, the attached patch should fix the real cause of issue385.  With
> the patch, the working directory that seekRepo started in will be
> restored on failure.  So if we are outside of a repository, the current
> directory won't be changed to /.

This seems reasonable enough.  Accepting.

That being said, it would be nice to see tempdir-related stuff fixed in
general.  But that's just speaking from an ssh-cm point of view.

So this particular problem is resolved with Zachary's patch.

But there is still an open variant, issue459, where the current directory is
itself not writeable.  We will still have to deal with the temporary files
issue, it seems.

See also issue250

Hello,

This bug appears once more in darcs 2.0.0~pre3-1 from
http://cyber.com.au/~twb/darcs2.deb/.

It fails with:

$ darcs changes --summary --repodir=http://chadok.info/darcs/ciml/

darcs: http://chadok.info/darcs/ciml: setCurrentDirectory: does not exist (No
such file or directory)

It works well with darcs 1.0.9.

darcs --exact-version says:
darcs compiled on Feb  2 2008, at 06:31:14

I don't have time these days to do tests with a freshly built binary.

Thanks in advance,

Olivier

The following patch updated the status of issue385 to be resolved in the unstable branch:

* resolve issue385: don't worry if we can't get local changes. 

You can view the patch details online here: 
http://darcs.net/cgi-bin/darcs.cgi/unstable/?c=annotate&p=20080408151823-72aca-e1dfec344e9588b130b0b0e322523938cbc621c6.gz