Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd

2015-02-03 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 On Tue, Feb 03, 2015 at 01:40:12PM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  But wouldn't we still fail writing foo/bar at that point if foo is a
  regular file (again, we should never do this, but that is the point of
  the test).
 
 The point of the test is not to create foo, whether it is a symlink
 or an emulating regular file, in the first place.

 I thought the point was not to create ../bar, when foo points to
 ...

That is not what the new test you added was doing, though.  You are
calling tmp in that test foo and ../foo in the test is called
../bar in the message I am responding to, so that is confusing,
but in these tests you added, I do not see anything that prepares a
symbolic link ON the filesystem and wait for git apply to get
fooled.

 I agree that the way you have implemented it is that we would
 never even write foo, and the test checks for that, but to me that is
 the least interesting bit of what is being tested.

They are both interesting.  When rejecting an input, git apply
must be atomic.  When checking an input, git apply should notice
and reject a patch that tries to follow a symbolic link.

Taken together:

 (1) If a patchset tries to create a symbolic link and then tries to
 follow it, the latter principle should make git apply to
 reject the patchset, and the former should make sure there is
 no external effect.

 (2) If a patchset tries to affect a path, and the target of the
 patch application has a symlink that may divert the operation
 to the original path the patchset wants to affect, the latter
 principle should make git apply to reject the patchset, too.

And my observation is that the new tests that are not protected by
SYMLINKS prerequisite (i.e. all of them) in your new test 4139, are
all the former kind.  As git aplly must be atomic, rejection must
be decided without touching the filesystem at all.  Hence there is
no need for the filesystem to even support symbolic links.

But some bozo may try to break git apply in the future and try to
incrementally apply the patch in a patchset, and at that point, the
existing test_must_fail git apply may pass not because we
correctly decide not to follow symbolic links but because his broken
version of git apply would try to create a symbolic link (which we
would turn into a regular file) and then the filesystem would fail
to follow that symbolic link mimic, and as the result the test may
still pass.

In order to prevent that from happening, we may want to make sure
that

 - test_must_fail git apply

 - There is no foo (or tmp); the input to 'git apply' is the
   only thing that could create, as you do not create symlinks as
   traps before running 'git apply', so this will catch the 'make it
   incremental and then break the do-not-follow logic'.

 - There is no ../bar (or ../foo).

The second one is missing from your tests, I think, and it would be
a very good addition, even on a platform with SYMLINKS prerequisite
satisfied.  The future change might be trying to make it incremental
and impelent do-not-follow logic by observing what is in the
filesystem, and we do want to catch such a broken implementation.





--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd

2015-02-03 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 On Mon, Feb 02, 2015 at 03:27:27PM -0800, Junio C Hamano wrote:

 +test_expect_failure 'symlink escape via ..' '
 +{
 +mkpatch_symlink tmp .. 
 +mkpatch_add tmp/foo ../foo
 +} patch 
 +test_must_fail git apply patch 
 +test_path_is_missing ../foo
 +'

 By the way, does this patch (and the other symlink-escape ones) need to
 be marked with the SYMLINKS prereq? For a pure-index application, it
 should work anywhere, but I have a feeling that this git apply patch
 may try to write the symlink to the filesystem, fail, and report failure
 for the wrong reason.  I don't have a SYMLINK-challenged filesystem to
 test on, though.

We check the links to be created by the patch itself in-core before
going to the filesystem, and the symbolic links you are creating
using mkpatch_symlink should be caught before we invoke symlink(2),
I think.

In other words, this series attempts to stick to the verify
everything in-core before deciding that it is OK to touch the
working tree or the index.

A few new tests in t4122 do try to see that the command is not
fooled by existihng symbolic links on the filesystem and they need
to be marked with SYMLINKS prerequisite.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd

2015-02-03 Thread Jeff King 
On Tue, Feb 03, 2015 at 12:23:28PM -0800, Junio C Hamano wrote:

  By the way, does this patch (and the other symlink-escape ones) need to
  be marked with the SYMLINKS prereq? For a pure-index application, it
  should work anywhere, but I have a feeling that this git apply patch
  may try to write the symlink to the filesystem, fail, and report failure
  for the wrong reason.  I don't have a SYMLINK-challenged filesystem to
  test on, though.
 
 We check the links to be created by the patch itself in-core before
 going to the filesystem, and the symbolic links you are creating
 using mkpatch_symlink should be caught before we invoke symlink(2),
 I think.
 
 In other words, this series attempts to stick to the verify
 everything in-core before deciding that it is OK to touch the
 working tree or the index.

Right, I do not think these tests will _fail_ when the filesystem does
not support symlinks. But nor are they actually testing anything
interesting. They would pass on such a system even without your patch,
as we would fail to apply even the symlink creation part of the patch.

I can live with leaving them unmarked, though. It gets the code
exercised on more systems, which gives a slightly higher chance of
catching some other unexpected breakage.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd

2015-02-03 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 Right, I do not think these tests will _fail_ when the filesystem does
 not support symlinks. But nor are they actually testing anything
 interesting. They would pass on such a system even without your patch,
 as we would fail to apply even the symlink creation part of the patch.

I thought we write out the contents of the symbolic link as a
regular file on such a filesystem, and as long as we do not expect
test -h expected-to-be-symlink-we-just-created to succeed we would
be fine.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd

2015-02-03 Thread Jeff King 
On Tue, Feb 03, 2015 at 01:23:15PM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  Right, I do not think these tests will _fail_ when the filesystem does
  not support symlinks. But nor are they actually testing anything
  interesting. They would pass on such a system even without your patch,
  as we would fail to apply even the symlink creation part of the patch.
 
 I thought we write out the contents of the symbolic link as a
 regular file on such a filesystem, and as long as we do not expect
 test -h expected-to-be-symlink-we-just-created to succeed we would
 be fine.

But wouldn't we still fail writing foo/bar at that point if foo is a
regular file (again, we should never do this, but that is the point of
the test).

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd

2015-02-03 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 But wouldn't we still fail writing foo/bar at that point if foo is a
 regular file (again, we should never do this, but that is the point of
 the test).

The point of the test is not to create foo, whether it is a symlink
or an emulating regular file, in the first place.

In this piece:

 +test_expect_failure 'symlink escape via ..' '
 +{
 +mkpatch_symlink tmp .. 

This is a patch to create tmp that points at ..

 +mkpatch_add tmp/foo ../foo

And this is a patch to create tmp/foo, and make sure ../foo does
not exist in the filesystem to prepare for the test.

 +} patch 
 +test_must_fail git apply patch 

And this patch, if the rejection logic were to be broken in the
future, might create tmp and then try to follow it to affect
../foo.

 +test_path_is_missing ../foo

So if this test makes sure if tmp is missing, then it would be
alright, no?  The follow the symlink to affect ../foo may not
happen on a filesystem without symlinks, but verifying that tmp
is missing will assure us that the patch application is atomic,
i.e. if we see tmp on the filesystem after seeing git apply
failed, that is a sign that git apply failed to be atomic.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd

2015-02-03 Thread Jeff King 
On Tue, Feb 03, 2015 at 01:40:12PM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  But wouldn't we still fail writing foo/bar at that point if foo is a
  regular file (again, we should never do this, but that is the point of
  the test).
 
 The point of the test is not to create foo, whether it is a symlink
 or an emulating regular file, in the first place.

I thought the point was not to create ../bar, when foo points to
... I agree that the way you have implemented it is that we would
never even write foo, and the test checks for that, but to me that is
the least interesting bit of what is being tested. Crossing a symlink
boundary and escaping from the tree are interesting, and the atomicity
is a side note. We could also realize that treating foo as a file
would fail and cancel the whole operation atomically, too.

But I think we are getting into contrasting our mental models, which is
probably not productive. I am OK with leaving it without the SYMLINKS
flag, as it should pass on systems that do not handle symlinks. And if
it does not, then that will be a good cross-check that our analysis was
sane. :)

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd

2015-02-02 Thread Jeff King 
On Mon, Feb 02, 2015 at 03:27:27PM -0800, Junio C Hamano wrote:

 By default, a patch that affects outside the working area is
 rejected as a mistake (or a mischief); Git itself does not create
 such a patch, unless the user bends backwards and specifies a
 non-standard prefix to git diff and friends.
 
 When `git apply` is used without either `--index` or `--cached`
 option as a better GNU patch, the user can pass `--unsafe-paths`
 option to override this safety check.  This cannot be used to escape
 outside the working tree when using `--index` or `--cached` to apply
 the patch to the index.
 
 The new test was stolen from Jeff King with slight enhancements.

I notice that this includes the symlink-crossing tests, marked as
failures. Reading the series, I know what is going to happen later, but
do you want to leave a note like:

  Note that we also add tests for leaving the working directory by
  crossing symlink boundaries, which is not addressed in this patch.
  That is a separate issue caused following symlinks, which will come
  later.

or something to help later readers of git log?

 +--unsafe-paths::
 + By default, a patch that affects outside the working area is
 + rejected as a mistake (or a mischief); Git itself never
 + creates such a patch unless the user bends backwards and
 + specifies nonstandard prefix to git diff and friends.

Minor wordsmithing: the usual idiom is bend over backwards, and you
probably want s/specifies/ a/.

 ++
 +When `git apply` is used without either `--index` or `--cached`
 +option as a better GNU patch, the user can pass `--unsafe-paths`
 +option to override this safety check.

Similarly, probably every instance of foo option would read better as
the foo option.

 This cannot be used to escape
 +outside the working tree when using `--index` or `--cached` to apply
 +the patch to the index.

I had trouble figuring out what this meant. Would it be simpler to just
say:

  This option has no effect when `--index` or `--cached` is in use.

Or is there some other subtlety that you are trying to convey that I am
missing?

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/4] apply: reject input that touches outside $cwd

2015-02-02 Thread Junio C Hamano 
By default, a patch that affects outside the working area is
rejected as a mistake (or a mischief); Git itself does not create
such a patch, unless the user bends backwards and specifies a
non-standard prefix to git diff and friends.

When `git apply` is used without either `--index` or `--cached`
option as a better GNU patch, the user can pass `--unsafe-paths`
option to override this safety check.  This cannot be used to escape
outside the working tree when using `--index` or `--cached` to apply
the patch to the index.

The new test was stolen from Jeff King with slight enhancements.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
 Documentation/git-apply.txt |  14 -
 builtin/apply.c |  26 +
 t/t4139-apply-escape.sh | 137 
 3 files changed, 176 insertions(+), 1 deletion(-)
 create mode 100755 t/t4139-apply-escape.sh

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index f605327..a6e83a9 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -16,7 +16,7 @@ SYNOPSIS
  [--ignore-space-change | --ignore-whitespace ]
  [--whitespace=(nowarn|warn|fix|error|error-all)]
  [--exclude=path] [--include=path] [--directory=root]
- [--verbose] [patch...]
+ [--verbose] [--unsafe-paths] [patch...]
 
 DESCRIPTION
 ---
@@ -229,6 +229,18 @@ For example, a patch that talks about updating 
`a/git-gui.sh` to `b/git-gui.sh`
 can be applied to the file in the working tree `modules/git-gui/git-gui.sh` by
 running `git apply --directory=modules/git-gui`.
 
+--unsafe-paths::
+   By default, a patch that affects outside the working area is
+   rejected as a mistake (or a mischief); Git itself never
+   creates such a patch unless the user bends backwards and
+   specifies nonstandard prefix to git diff and friends.
++
+When `git apply` is used without either `--index` or `--cached`
+option as a better GNU patch, the user can pass `--unsafe-paths`
+option to override this safety check.  This cannot be used to escape
+outside the working tree when using `--index` or `--cached` to apply
+the patch to the index.
+
 Configuration
 -
 
diff --git a/builtin/apply.c b/builtin/apply.c
index ef32e4f..c751ddf 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -50,6 +50,7 @@ static int apply_verbosely;
 static int allow_overlap;
 static int no_add;
 static int threeway;
+static int unsafe_paths;
 static const char *fake_ancestor;
 static int line_termination = '\n';
 static unsigned int p_context = UINT_MAX;
@@ -3483,6 +3484,23 @@ static int check_to_create(const char *new_name, int 
ok_if_exists)
return 0;
 }
 
+static void die_on_unsafe_path(struct patch *patch)
+{
+   const char *old_name = NULL;
+   const char *new_name = NULL;
+   if (patch-is_delete)
+   old_name = patch-old_name;
+   else if (!patch-is_new  !patch-is_copy)
+   old_name = patch-old_name;
+   if (!patch-is_delete)
+   new_name = patch-new_name;
+
+   if (old_name  !verify_path(old_name))
+   die(_(invalid path '%s'), old_name);
+   if (new_name  !verify_path(new_name))
+   die(_(invalid path '%s'), new_name);
+}
+
 /*
  * Check and apply the patch in-core; leave the result in patch-result
  * for the caller to write it out to the final destination.
@@ -3570,6 +3588,9 @@ static int check_patch(struct patch *patch)
}
}
 
+   if (!unsafe_paths)
+   die_on_unsafe_path(patch);
+
if (apply_data(patch, st, ce)  0)
return error(_(%s: patch does not apply), name);
patch-rejected = 0;
@@ -4379,6 +4400,8 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix_)
N_(make sure the patch is applicable to the current 
index)),
OPT_BOOL(0, cached, cached,
N_(apply a patch without touching the working tree)),
+   OPT_BOOL(0, unsafe-paths, unsafe_paths,
+   N_(accept a patch to touch outside the working area)),
OPT_BOOL(0, apply, force_apply,
N_(also apply the patch (use with 
--stat/--summary/--check))),
OPT_BOOL('3', 3way, threeway,
@@ -4451,6 +4474,9 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix_)
die(_(--cached outside a repository));
check_index = 1;
}
+   if (check_index)
+   unsafe_paths = 0;
+
for (i = 0; i  argc; i++) {
const char *arg = argv[i];
int fd;
diff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh
new file mode 100755
index 000..5e67179
--- /dev/null
+++ b/t/t4139-apply-escape.sh
@@ -0,0 +1,137 @@
+#!/bin/sh
+
+test_description='paths written by git-apply cannot escape the working tree'
+. ./test-lib.sh
+
+# tests

Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd

2015-02-02 Thread Jeff King 
On Mon, Feb 02, 2015 at 03:27:27PM -0800, Junio C Hamano wrote:

 +test_expect_failure 'symlink escape via ..' '
 + {
 + mkpatch_symlink tmp .. 
 + mkpatch_add tmp/foo ../foo
 + } patch 
 + test_must_fail git apply patch 
 + test_path_is_missing ../foo
 +'

By the way, does this patch (and the other symlink-escape ones) need to
be marked with the SYMLINKS prereq? For a pure-index application, it
should work anywhere, but I have a feeling that this git apply patch
may try to write the symlink to the filesystem, fail, and report failure
for the wrong reason.  I don't have a SYMLINK-challenged filesystem to
test on, though.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd

2015-02-02 Thread Torsten Bögershausen 

If I am allowed to to some load thinking:

The commit msh header says:
 reject input that touches outside $cwd

The commit message says:
  By default, a patch that affects outside the working area

And the new command line option is this:
  --unsafe-paths
(Which may be a good choice to pretend people from using it without
  having read the documentaion)

(And isn't working area the same as work space ?)

I have the slight feeling that there may be more unsafe-path situations coming
up in the future..

Will
--allow-outside-ws
or
--patch-outside-ws
be better ?

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html