darcs

Issue 177 defense against malicious patch file oversteps its bounds

Title defense against malicious patch file oversteps its bounds
Priority bug Status resolved
Milestone Resolved in
Superseder Nosy List darcs-devel, dmitry.kurochkin, kowey, silverdirk, thorkilnaur, tommy, zooko
Assigned To tommy
Topics

Created on 2006-05-24.02:48:04 by zooko, last changed 2009-10-24.09:10:07 by admin.

Messages
msg671 (view) Author: zooko Date: 2006-05-24.02:47:59
I'm using darcs to manage, among other things, another darcs repository.  The
recent security feature (in 1.0.6 a.k.a. 1.0.7pre1) has caused a problem
because it notices that one of the patches is modifying
"./trunk/_darcs/prefs/defaults" and aborts.

It makes sense to forbid patch files from modifying the _darcs directory of
their *own* darcs repository, but it is wrong to forbid them to modify any
directory whose name is "_darcs"!

Just to be clear, the "./trunk/_darcs" is not the metadir for this darcs repo.
That would be "./_darcs".

Regards,

Zooko
msg675 (view) Author: tommy Date: 2006-05-25.16:59:28
This was tricky.

The reason _darcs is "forbidden" also in subdirs is in the case
of nested repositories (and it was an easy way to handle tricks
like "./foo/../_darcs/bar"). But how can darcs tell a nested
darcs repo from a managed darcs repo?

Just brain storming: a flag --allow-sub-darcs-dirs, a list of
safe _darcs paths _darcs/prefs/managed_darcs_repos.
msg676 (view) Author: zooko Date: 2006-05-25.17:06:24
Apparently I cannot tell the difference!  What is the difference?
msg677 (view) Author: tommy Date: 2006-05-25.17:45:53
On Thu, May 25, 2006 at 05:06:26PM +0000, Zooko wrote:
> Apparently I cannot tell the difference!  What is the difference?

It's only a difference in usage or thinking.

I once thought that it was best for nested darcs repos to be
isolated and not modify each others trees, so that darcs
(--look-for-adds &c) would prune subrepos (dirs containing a
_darcs dir), just like it prunes its own _darcs dir and skip
boring things. My reason for thinking so was that sharing and/or
mixing files between repos gives a rather complicated structure
that has considerably more potential trouble than usefulness.

But managing a bunch of sub repos' _darcs/prefs/defaults files
is perhaps a case of high usefulness and low trouble potential.

Still, anything bad a malicious patch can do to its own _darcs
dir, it can also do to subrepos' _darcs dirs.

How about this: make darcs prune nested repos by default but
give it a pref that enables a repo to manage files within its
subrepos (including their _darcs dirs)?
msg679 (view) Author: zooko Date: 2006-05-25.18:01:07
Personally I don't feel like it is worth it for darcs to attempt to help the
user in this way.  Instead, I would prefer that darcs not do any
pattern-matching or special-casing of the files which I'm asking it to operate on.

And I do use nested repos quite a lot.

However, I don't feel very strongly about this issue, so if other people have
some reason for darcs to behave differently by default then I don't mind having
to specially configure darcs for my uses.
msg742 (view) Author: dagit Date: 2006-07-03.20:51:26
Does it matter which subdirectory (of the top most darcs repository)
you are working in when you run the darcs command?  If so it may be a
possible to work around this by specifying the repo path on the
command line.

On 5/23/06, Zooko <bugs@darcs.net> wrote:
>
> New submission from Zooko <zooko@zooko.com>:
>
> I'm using darcs to manage, among other things, another darcs repository.  The
> recent security feature (in 1.0.6 a.k.a. 1.0.7pre1) has caused a problem
> because it notices that one of the patches is modifying
> "./trunk/_darcs/prefs/defaults" and aborts.
>
> It makes sense to forbid patch files from modifying the _darcs directory of
> their *own* darcs repository, but it is wrong to forbid them to modify any
> directory whose name is "_darcs"!
>
> Just to be clear, the "./trunk/_darcs" is not the metadir for this darcs repo.
> That would be "./_darcs".
>
> Regards,
>
> Zooko
>
> ----------
> messages: 671
> nosy: droundy, tommy, zooko
> status: unread
> title: defense against malicious patch file oversteps its bounds
>
> ____________________________________
> Darcs issue tracker <bugs@darcs.net>
> <http://bugs.darcs.net/issue177>
> ____________________________________
>
> _______________________________________________
> darcs-devel mailing list
> darcs-devel@darcs.net
> http://www.abridgegame.org/cgi-bin/mailman/listinfo/darcs-devel
>
>
msg1144 (view) Author: silverdirk Date: 2006-10-24.21:05:57
I just ran into this problem as well.  I keep all my personal projects in 
various subdirectories of my home directory, like "~/code/CmdlineGL" "~/code/
lib/UDT" "~/school/AdvCompiler/Assign3".  Each is a separate repo.

I once proposed a "darcs addsubrepo" command.  Instead I just went with a 
"darcsaddrepo" script and a post-hook script that looks for empty "_darcs" 
directories and does a "darcs init" on the parent. (while juggling any files in 
this directory to end up in the newly created one, because of course darcs 
won't "init" if it sees a "_darcs" already)

The purpose is to allow me to set "lastrepo" to the URL of that project on my 
repo server.  When I want one of my projects, I pull all changes to my home 
directory, then go to the project's directory and "darcs pull", which pulls 
that project to that location.

With this new "security feature", I have effectively lost all my sub-repo 
patches.   I will need to devise a new name to hold the "lastrepo" and other 
"prefs", and then have my script look for that dir name instead.  I will have 
to re-record my entire tree of projects (admittedly not that hard).

The most annoying part though, and unfixable, is that darcs in the parent 
directory won't be able to see changes made to the _darcs of an individual 
project.  If I want to change a project's prefs, I will have to manually copy 
files from _darcs to .darcs_subrepo (or whatever I name it).  The only way I 
see to automate this would be to have a darcs pre-hook which copies all files 
from _darcs and _darcs/prefs to .darcs_subrepo, and this would be particularly 
slow accross all my sub-repos.

If you want darcs to prevent destruction of a repo, all you need to protect are 
"_darcs/inventory" and "_darcs/patches".
msg1147 (view) Author: tommy Date: 2006-10-25.16:24:22
> If you want darcs to prevent destruction of a repo, all you need to protect are 
> "_darcs/inventory" and "_darcs/patches".

That is unfortunately not true. But allowing changes to
_darcs/prefs should be "safe". I'd still want this kind of
"advanced" usage to be a non-default option. A normal looking
long and boring patch could contain a hunk that adds "test rm
-rf /" to _darcs/prefs/prefs.

If anyone has time to make such change it is still time to get
it into darcs 1.0.9. (Record it against darcs stable, so it
doesn't depend on any unwanted patches in darcs unstable.)
Otherwise I'll try to do it myself next week.
msg1149 (view) Author: zooko Date: 2006-10-25.18:05:14
I think of this issue as a standard issue of "in-band signalling vs. out-of-band
signalling", i.e. of encapsulation.  darcs should offer an interface to the rest
of the world that says "I will perform the following operations on data that you
hand over to me, no matter what that data is.  Neither the names, locations,
metadata, nor contents of the files or directories will cause me to behave
differently than my standard behavior.".

Also, let's try to sketch out a principled approach for security, so that
darcs's behavior with respect to security will be consistent and easily
understood by users.  One principled stand is: "Darcs will not exercise any
authority except for authority over the (transitive) contents of all directories
which have ever been registered with it, but it offers no other guarantees, so
if you apply a patch that has been authored or tampered with by a malicious
person then darcs might do absolutely anything to the transitive contents of all
of those directories.".

A stronger (more useful, and harder to implement) guarantee would be "Applying a
patch will have only the effects indicated by the patch semantics, so if you've
read the patch and understand what it will do, then you can safely apply it
regardless of authorship.".  Unfortunately, darcs currently cannot offer this
guarantee because of the problem that the same patch ID can map to different
patches.  (There may be other reasons why darcs can't offer this guarantee, but
one reason is sufficient to prove that it currently does not offer it.)


I don't object to an "advisory" kind of UI that warns users that adding
"./_darcs" into darcs's control may lead to confusion.  Darcs could refuse to
add such a file unless the user adds an option meaning
"--control-own-metadirectory".  It kind of seems to me that this check should
*not* trigger on darcs metadirectories other than the metadirectory used by
*this* repository.
msg1151 (view) Author: tommy Date: 2006-10-25.19:08:36
> I think of this issue as a standard issue of "in-band signalling vs. out-of-band
> signalling", i.e. of encapsulation.  darcs should offer an interface to the rest
> of the world that says "I will perform the following operations on data that you
> hand over to me, no matter what that data is.  Neither the names, locations,
> metadata, nor contents of the files or directories will cause me to behave
> differently than my standard behavior.".

Well, it would be so much easier to implement, just bypassing
the current safety checks with an option, and so much easier to
get it "correct".

But looking for more options: why is darcs used in this way, or
rather, could there be any other way of doing it? I'm afraid I
don't fully understand what you all are doing. Would it help if
darcs supported a global prefs dir instead of just the global
defaults file?
msg1152 (view) Author: tommy Date: 2006-10-27.08:41:04
[...]
> In this case I meant that if you wanted to prevent accidental destruction of
> a repo inside the current repo you only need to protect the inventory and
> patches.

I'm not sure if we agree, and how a repo inside a current repo
is any different. Everything in any _darcs dir except for
_darcs/prefs are darcs' internals, and can possibly destroy the
repo if it gets inconsistent. For example: modifying a file in
pristine (or current) before recording in that repo will result
in invalid patches. And the "inventory" is currently stored in
three places (including checkpoints), but more files are added
as darcs evolves; the unstable version uses a new
tentative_inventory. And we'll also get a repo format file that
tells darcs how to interpret the contents of _darcs in a
different way. It would not be useful to version control any of
this, I think. But it would be much simpler to allow it than to
only allow management of other repos' _darcs/prefs. And thinking
about it, it might be good for experimenting with far out uses
of darcs too. And for backward compatibility, as you pointed
out. It seems very unfortunate to me now that darcs became
"unusable" for some people in this way.

So I suggest adding an option and a big fat warning (which I'm
willing to implement myself) to completely remove the file path
checks, allowing darcs to manage just anything anywhere in its
reach. I don't quite understand how the overlapping repos are
used; is it only _darcs/prefs that is "cross-managed"? A more
elaborate patch with limited access escalation to allow only
this intended use (whatever it is) would be welcome, but I
wouldn't know how to write it myself.
msg1153 (view) Author: zooko Date: 2006-10-27.08:46:57
My use case was a tool to do bidirectional sync between SVN and darcs (with the
help of tailor).  If I recall correctly, it was only _darcs/prefs/defaults that
was managed.  I would prefer not to use a feature of darcs which was specific
for this use, though, as that would be a bit too detailed for my tastes.  I
would prefer to use the general-purpose "disable file path checks" feature.
msg1154 (view) Author: silverdirk Date: 2006-10-27.15:44:28
Tommy Pettersson <ptp@lysator.liu.se> added the comment:
> But looking for more options: why is darcs used in this way, or
> rather, could there be any other way of doing it? I'm afraid I
> don't fully understand what you all are doing. Would it help if
> darcs supported a global prefs dir instead of just the global
> defaults file?

Ok, I'll mention a few full use-cases.  Note that there's nothing going on
here that can't be done using ordinary tools- but using darcs for it is
slick and stylish.  All of these (for me) involve multiple machines.

=== Laptop ===
$ cd school/AlgorithmsII
$ darcsaddrepo assign3/
  # assign3 is already a darcs repo, on this machine ]]
  # my script then adds ~/school/AlgorithmsII/assign3/_darcs/ and
~/school/AlgorithmsII/assign3/_darcs/prefs/defaultrepo after setting
defaultrepo to my server's repo root + this path  ]]
$ darcs rec -a -m "Addded assignment 3 for algorithms class" .
$ darcs push -a
  # pushes the patch set  to my repo server, to my repo-root directory
  # upon applying, the '_darcs' directory is noticed and my server runs
'darcs init' with some other scripting that gets the prefs/defualtrepo file
into position
$ cd assign3
$ darcs push -a
  # pushes this repo into the newly created one on my server

This is how I push subdirectories onto my server, and get them set up as
darcs repos.  It also sets all the paths for me, which is convenient.  But
the more interesting part is when i go to another machine where I want one
of my projects:

=== Shared Student-group Server ===
$ darcs pull -a
  # adds the ~school/AlgorithmsII/assign3 repo, darcs-inits it, and gives me
the correct defaultrepo
$ cd school/AlgorithmsII/assign3/
$ darcs pull -a

I now have my tree of assignments set up exactly as I have on my server and
on my laptop, and I didn't have to type a single path or URL to get it.  In
addition, I get any other projects which exist in my tree, so when i want to
go work on something I don't have to remember what the exact name of it was,
or whether it was uppercase or lowercase, or log into my server to remind
myself.  I just browse into the dir (almost like gentoo's portage, or BSD's
ports) and "darcs pull -a".

In short, i want to version my entire home directory so that I can
synchronize important parts of it form machine to machine.  I don't want to
pull every file from every project into each machine I use, though, (nor
would I want all my project files to be in the same darcs repo) so I use the
sub-repo concept.  When I want a particular project, I navigate there and
pull its patches.
msg1209 (view) Author: kowey Date: 2006-11-13.03:54:19
> In short, i want to version my entire home directory so that I can
> synchronize important parts of it form machine to machine.  I don't want to
> pull every file from every project into each machine I use, though, (nor
> would I want all my project files to be in the same darcs repo) so I use the
> sub-repo concept.  When I want a particular project, I navigate there and
> pull its patches.

Hmm... it seems like what we need here is a proper subrepo concept.  I'd
prefer to keep the checking stuff as simple as possible.

Slick and stylish should probably wait until such a sub-repo concept has
been introduced.
msg1769 (view) Author: tommy Date: 2007-06-29.19:42:08
fixed in 1.0.9
History
Date User Action Args
2006-05-24 02:48:04zookocreate
2006-05-25 16:59:30tommysetstatus: unread -> unknown
nosy: droundy, tommy, zooko
messages: + msg675
2006-05-25 17:06:26zookosetnosy: droundy, tommy, zooko
messages: + msg676
2006-05-25 17:45:56tommysetnosy: droundy, tommy, zooko
messages: + msg677
2006-05-25 18:01:10zookosetnosy: droundy, tommy, zooko
messages: + msg679
2006-07-03 20:51:28dagitsetnosy: + dagit
messages: + msg742
2006-10-24 21:06:05silverdirksetnosy: + silverdirk, kowey
messages: + msg1144
2006-10-25 16:24:34tommysetnosy: droundy, tommy, kowey, zooko, dagit, silverdirk
messages: + msg1147
2006-10-25 18:05:22zookosetnosy: droundy, tommy, kowey, zooko, dagit, silverdirk
messages: + msg1149
2006-10-25 19:08:44tommysetnosy: droundy, tommy, kowey, zooko, dagit, silverdirk
messages: + msg1151
2006-10-27 08:41:10tommysetnosy: droundy, tommy, kowey, zooko, dagit, silverdirk
messages: + msg1152
2006-10-27 08:46:58zookosetnosy: droundy, tommy, kowey, zooko, dagit, silverdirk
messages: + msg1153
2006-10-27 15:44:37conradmesetnosy: + conradme
messages: + msg1154
2006-11-09 13:56:38tommysetnosy: droundy, tommy, kowey, zooko, dagit, silverdirk, conradme
2006-11-13 03:54:27koweysetnosy: droundy, tommy, kowey, zooko, dagit, silverdirk, conradme
messages: + msg1209
2006-11-17 15:27:55tommysetstatus: unknown -> resolved-in-stable
nosy: droundy, tommy, kowey, zooko, dagit, silverdirk, conradme
2007-06-29 19:42:09tommysetstatus: resolved-in-stable -> resolved
nosy: + beschmi
messages: + msg1769
2009-08-06 17:35:00adminsetnosy: + markstos, jast, Serware, dmitry.kurochkin, darcs-devel, mornfall, simon, thorkilnaur, - droundy, silverdirk, conradme
2009-08-06 20:32:12adminsetnosy: - beschmi
2009-08-10 21:45:27adminsetnosy: + silverdirk, conradme, - markstos, darcs-devel, jast, Serware, mornfall
2009-08-10 23:54:16adminsetnosy: - dagit
2009-08-25 17:49:24adminsetnosy: + darcs-devel, - simon
2009-08-27 13:49:17adminsetnosy: tommy, kowey, darcs-devel, zooko, silverdirk, conradme, thorkilnaur, dmitry.kurochkin
2009-10-24 09:10:07adminsetnosy: - conradme