It is possible to create a patch that overwrites any file in _darcs. This can be
used to execute arbitary commands for example by overwriting the test setpref.

Create malicious patch:
cd a
darcs init
darcs setpref test malicious
mkdir test
touch test/x
darcs add test/x
edit _darcs/patches/pending:
  - remove setpref
  - change adddir and addfile to _darcs/prefs/prefs
darcs rec --no-test

Use patch:
cd b
darcs init
darcs setpref test safe
darcs pull ../a
cat _darcs/prefs/prefs
 > test malicious


darcs -v
1.0.3 (release)
( debian testing )

Ups, i was too fast. Preferences can be changed by a normal patch too. But this
method can be used to change any file in _darcs and do maybe unsafe things.

Files can also be created outside the repository. By editing pending to

adddir ./..
addfile ./../test

one can create and overwrite files outside of the repository.

New patches:

[create ../foo
foo**20051206201037] {
adddir ./..
addfile ./../test
hunk ./../test 1
+foo
}

Context:

Patch bundle hash:
4c343bc2dbbfe80b332854ae419da56cf0d388ed

Thanks for pointing this out!

This is a result of the new improved and more efficient code for applying
patches.  We used to apply patches to a sandbox before modifying the
filesystem, which would have caught issues like this, but was far too
memory intensive for large repositories.

Fortunately, I think this is at least moderately easily fixable.  We use
the ReadableDirectory and WriteableDirectory monads (in DarcsIO), which are
designed to allow us to at least moderately easily implement a software
chroot, since they naturally limit the operations available to code written
in the monad.  I think that a moderately simple modification to the IO
instances of these classes should fix this bug--at least the part about
modifying files outside the repo, modifying contents of _darcs/ is a
separate matter.  The catch is that this will also break setpref handling,
since applying a setpref patch to _darcs/current/ needs to modify
_darcs/prefs/prefs.  A workaround is to allow the application of the patch
to the working directory modify _darcs/prefs/prefs, and actually I suppose
that that would be sufficient.  We'd need to look at the checkpoint
handling to make sure it still works properly, though.

Avoiding patches that modify _darcs/ is another matter (although also a
very good thing).  In this case we need to allow setpref patches, so we
either need to work at a higher level than the Readable/WriteableDirectory
monads, or we need to add in a special exception for that file.  Perhaps
when parsing patches we could reject such patches.  Or probably it would be
better to add to the beginning of apply something like

apply _ (FP f _) | matches_darcs f = error "malicious patch"

which would mean that all filepatches are covered by one safety net.  I
suppose we could add something like that for directory patches as well.
matches_darcs might be tricky, since it would need to handle patches like
./foobar/../_darcs/prefs/defaults.  But we do have normalization code that
simplifies path names which we could use... and actually perhaps we should
just be rejecting any patch that contains ".." as one of its directory
names.  Darcs should never create such a path (at least given reasonable
input).  This would also handle the outside the repository case, but I
still like the chroot monad idea, since it could be extended to protect us
against tricks like symlink attacks, where a symlink points outside the
repo.

So perhaps (and this is just a sketch)

matches_darcs f = take (length "_darcs/") f == "_darcs/"
has_dotdot f = "/../" `substr` f || take 3 f == "../" ||
               take 3 (reverse f) == "../"

apply _ (FP f _) | matches_darcs f = error "malicious patch"
apply _ (FP f _) | has_dotdot f = error "malicious patch"
apply _ (DP f _) | matches_darcs f = error "malicious patch"
apply _ (DP f _) | has_dotdot f = error "malicious patch"
(rest of apply definition)

where we'd of course want a nicer error message.  I think this small change
would fix the bugs pointed out here, and then at our liesure we could make
the Readable/WriteableDirectory monads do chrooting themselves to provide
another level of insurance against bugs like this.
-- 
David Roundy
http://www.darcs.net

Tommy, isn't this fixed in the darcs-stable branch?

Yes. I have a bunch of bugs to comment and close, but I'm a bit
behind schedule.

Fixed in 1.0.6
Sun Feb 19 20:23:28 CET 2006  Tommy Pettersson <ptp@lysator.liu.se>
  * check for malicious path before applying patch (issue48)