Merge branch 'jc/apply-beyond-symlink'
authorJunio C Hamano <gitster@pobox.com>
Tue, 3 Mar 2015 22:37:01 +0000 (14:37 -0800)
committerJunio C Hamano <gitster@pobox.com>
Tue, 3 Mar 2015 22:37:01 +0000 (14:37 -0800)
"git apply" was not very careful about reading from, removing,
updating and creating paths outside the working tree (under
--index/--cached) or the current directory (when used as a
replacement for GNU patch).

* jc/apply-beyond-symlink:
  apply: do not touch a file beyond a symbolic link
  apply: do not read from beyond a symbolic link
  apply: do not read from the filesystem under --index
  apply: reject input that touches outside the working area

Documentation/git-apply.txt
builtin/apply.c
t/t4122-apply-symlink-inside.sh
t/t4139-apply-escape.sh [new file with mode: 0755]

index f605327..9489664 100644 (file)
@@ -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,16 @@ 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
+       (either a Git controlled working tree, or the current working
+       directory when "git apply" is used as a replacement of GNU
+       patch) is rejected as a mistake (or a mischief).
++
+When `git apply` is used as a "better GNU patch", the user can pass
+the `--unsafe-paths` option to override this safety check.  This option
+has no effect when `--index` or `--cached` is in use.
+
 Configuration
 -------------
 
index 955cc79..525db4a 100644 (file)
@@ -51,6 +51,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;
@@ -3221,7 +3222,7 @@ static int load_patch_target(struct strbuf *buf,
                             const char *name,
                             unsigned expected_mode)
 {
-       if (cached) {
+       if (cached || check_index) {
                if (read_file_or_gitlink(ce, buf))
                        return error(_("read of %s failed"), name);
        } else if (name) {
@@ -3230,6 +3231,8 @@ static int load_patch_target(struct strbuf *buf,
                                return read_file_or_gitlink(ce, buf);
                        else
                                return SUBMODULE_PATCH_WITHOUT_INDEX;
+               } else if (has_symlink_leading_path(name, strlen(name))) {
+                       return error(_("reading from '%s' beyond a symbolic link"), name);
                } else {
                        if (read_old_data(st, name, buf))
                                return error(_("read of %s failed"), name);
@@ -3569,6 +3572,121 @@ static int check_to_create(const char *new_name, int ok_if_exists)
        return 0;
 }
 
+/*
+ * We need to keep track of how symlinks in the preimage are
+ * manipulated by the patches.  A patch to add a/b/c where a/b
+ * is a symlink should not be allowed to affect the directory
+ * the symlink points at, but if the same patch removes a/b,
+ * it is perfectly fine, as the patch removes a/b to make room
+ * to create a directory a/b so that a/b/c can be created.
+ */
+static struct string_list symlink_changes;
+#define SYMLINK_GOES_AWAY 01
+#define SYMLINK_IN_RESULT 02
+
+static uintptr_t register_symlink_changes(const char *path, uintptr_t what)
+{
+       struct string_list_item *ent;
+
+       ent = string_list_lookup(&symlink_changes, path);
+       if (!ent) {
+               ent = string_list_insert(&symlink_changes, path);
+               ent->util = (void *)0;
+       }
+       ent->util = (void *)(what | ((uintptr_t)ent->util));
+       return (uintptr_t)ent->util;
+}
+
+static uintptr_t check_symlink_changes(const char *path)
+{
+       struct string_list_item *ent;
+
+       ent = string_list_lookup(&symlink_changes, path);
+       if (!ent)
+               return 0;
+       return (uintptr_t)ent->util;
+}
+
+static void prepare_symlink_changes(struct patch *patch)
+{
+       for ( ; patch; patch = patch->next) {
+               if ((patch->old_name && S_ISLNK(patch->old_mode)) &&
+                   (patch->is_rename || patch->is_delete))
+                       /* the symlink at patch->old_name is removed */
+                       register_symlink_changes(patch->old_name, SYMLINK_GOES_AWAY);
+
+               if (patch->new_name && S_ISLNK(patch->new_mode))
+                       /* the symlink at patch->new_name is created or remains */
+                       register_symlink_changes(patch->new_name, SYMLINK_IN_RESULT);
+       }
+}
+
+static int path_is_beyond_symlink_1(struct strbuf *name)
+{
+       do {
+               unsigned int change;
+
+               while (--name->len && name->buf[name->len] != '/')
+                       ; /* scan backwards */
+               if (!name->len)
+                       break;
+               name->buf[name->len] = '\0';
+               change = check_symlink_changes(name->buf);
+               if (change & SYMLINK_IN_RESULT)
+                       return 1;
+               if (change & SYMLINK_GOES_AWAY)
+                       /*
+                        * This cannot be "return 0", because we may
+                        * see a new one created at a higher level.
+                        */
+                       continue;
+
+               /* otherwise, check the preimage */
+               if (check_index) {
+                       struct cache_entry *ce;
+
+                       ce = cache_file_exists(name->buf, name->len, ignore_case);
+                       if (ce && S_ISLNK(ce->ce_mode))
+                               return 1;
+               } else {
+                       struct stat st;
+                       if (!lstat(name->buf, &st) && S_ISLNK(st.st_mode))
+                               return 1;
+               }
+       } while (1);
+       return 0;
+}
+
+static int path_is_beyond_symlink(const char *name_)
+{
+       int ret;
+       struct strbuf name = STRBUF_INIT;
+
+       assert(*name_ != '\0');
+       strbuf_addstr(&name, name_);
+       ret = path_is_beyond_symlink_1(&name);
+       strbuf_release(&name);
+
+       return ret;
+}
+
+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.
@@ -3656,6 +3774,22 @@ static int check_patch(struct patch *patch)
                }
        }
 
+       if (!unsafe_paths)
+               die_on_unsafe_path(patch);
+
+       /*
+        * An attempt to read from or delete a path that is beyond a
+        * symbolic link will be prevented by load_patch_target() that
+        * is called at the beginning of apply_data() so we do not
+        * have to worry about a patch marked with "is_delete" bit
+        * here.  We however need to make sure that the patch result
+        * is not deposited to a path that is beyond a symbolic link
+        * here.
+        */
+       if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
+               return error(_("affected file '%s' is beyond a symbolic link"),
+                            patch->new_name);
+
        if (apply_data(patch, &st, ce) < 0)
                return error(_("%s: patch does not apply"), name);
        patch->rejected = 0;
@@ -3666,6 +3800,7 @@ static int check_patch_list(struct patch *patch)
 {
        int err = 0;
 
+       prepare_symlink_changes(patch);
        prepare_fn_table(patch);
        while (patch) {
                if (apply_verbosely)
@@ -4404,6 +4539,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 that touches 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,
@@ -4476,6 +4613,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;
index 1814647..4acb3f3 100755 (executable)
@@ -45,4 +45,110 @@ test_expect_success 'check result' '
 
 '
 
+test_expect_success SYMLINKS 'do not read from beyond symbolic link' '
+       git reset --hard &&
+       mkdir -p arch/x86_64/dir &&
+       >arch/x86_64/dir/file &&
+       git add arch/x86_64/dir/file &&
+       echo line >arch/x86_64/dir/file &&
+       git diff >patch &&
+       git reset --hard &&
+
+       mkdir arch/i386/dir &&
+       >arch/i386/dir/file &&
+       ln -s ../i386/dir arch/x86_64/dir &&
+
+       test_must_fail git apply patch &&
+       test_must_fail git apply --cached patch &&
+       test_must_fail git apply --index patch
+
+'
+
+test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '
+
+       rm -rf arch/i386/dir arch/x86_64/dir &&
+       git reset --hard &&
+       ln -s ../i386/dir arch/x86_64/dir &&
+       git add arch/x86_64/dir &&
+       git diff HEAD >add_symlink.patch &&
+       git reset --hard &&
+
+       mkdir arch/x86_64/dir &&
+       >arch/x86_64/dir/file &&
+       git add arch/x86_64/dir/file &&
+       git diff HEAD >add_file.patch &&
+       git diff -R HEAD >del_file.patch &&
+       git reset --hard &&
+       rm -fr arch/x86_64/dir &&
+
+       cat add_symlink.patch add_file.patch >patch &&
+       cat add_symlink.patch del_file.patch >tricky_del &&
+
+       mkdir arch/i386/dir
+'
+
+test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '
+
+       # same input creates a confusing symbolic link
+       test_must_fail git apply patch 2>error-wt &&
+       test_i18ngrep "beyond a symbolic link" error-wt &&
+       test_path_is_missing arch/x86_64/dir &&
+       test_path_is_missing arch/i386/dir/file &&
+
+       test_must_fail git apply --index patch 2>error-ix &&
+       test_i18ngrep "beyond a symbolic link" error-ix &&
+       test_path_is_missing arch/x86_64/dir &&
+       test_path_is_missing arch/i386/dir/file &&
+       test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+       test_must_fail git ls-files --error-unmatch arch/i386/dir &&
+
+       test_must_fail git apply --cached patch 2>error-ct &&
+       test_i18ngrep "beyond a symbolic link" error-ct &&
+       test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+       test_must_fail git ls-files --error-unmatch arch/i386/dir &&
+
+       >arch/i386/dir/file &&
+       git add arch/i386/dir/file &&
+
+       test_must_fail git apply tricky_del &&
+       test_path_is_file arch/i386/dir/file &&
+
+       test_must_fail git apply --index tricky_del &&
+       test_path_is_file arch/i386/dir/file &&
+       test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+       git ls-files --error-unmatch arch/i386/dir &&
+
+       test_must_fail git apply --cached tricky_del &&
+       test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+       git ls-files --error-unmatch arch/i386/dir
+'
+
+test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '
+
+       # existing symbolic link
+       git reset --hard &&
+       ln -s ../i386/dir arch/x86_64/dir &&
+       git add arch/x86_64/dir &&
+
+       test_must_fail git apply add_file.patch 2>error-wt-add &&
+       test_i18ngrep "beyond a symbolic link" error-wt-add &&
+       test_path_is_missing arch/i386/dir/file &&
+
+       mkdir arch/i386/dir &&
+       >arch/i386/dir/file &&
+       test_must_fail git apply del_file.patch 2>error-wt-del &&
+       test_i18ngrep "beyond a symbolic link" error-wt-del &&
+       test_path_is_file arch/i386/dir/file &&
+       rm arch/i386/dir/file &&
+
+       test_must_fail git apply --index add_file.patch 2>error-ix-add &&
+       test_i18ngrep "beyond a symbolic link" error-ix-add &&
+       test_path_is_missing arch/i386/dir/file &&
+       test_must_fail git ls-files --error-unmatch arch/i386/dir &&
+
+       test_must_fail git apply --cached add_file.patch 2>error-ct-file &&
+       test_i18ngrep "beyond a symbolic link" error-ct-file &&
+       test_must_fail git ls-files --error-unmatch arch/i386/dir
+'
+
 test_done
diff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh
new file mode 100755 (executable)
index 0000000..45b5660
--- /dev/null
@@ -0,0 +1,141 @@
+#!/bin/sh
+
+test_description='paths written by git-apply cannot escape the working tree'
+. ./test-lib.sh
+
+# tests will try to write to ../foo, and we do not
+# want them to escape the trash directory when they
+# fail
+test_expect_success 'bump git repo one level down' '
+       mkdir inside &&
+       mv .git inside/ &&
+       cd inside
+'
+
+# $1 = name of file
+# $2 = current path to file (if different)
+mkpatch_add () {
+       rm -f "${2:-$1}" &&
+       cat <<-EOF
+       diff --git a/$1 b/$1
+       new file mode 100644
+       index 0000000..53c74cd
+       --- /dev/null
+       +++ b/$1
+       @@ -0,0 +1 @@
+       +evil
+       EOF
+}
+
+mkpatch_del () {
+       echo evil >"${2:-$1}" &&
+       cat <<-EOF
+       diff --git a/$1 b/$1
+       deleted file mode 100644
+       index 53c74cd..0000000
+       --- a/$1
+       +++ /dev/null
+       @@ -1 +0,0 @@
+       -evil
+       EOF
+}
+
+# $1 = name of file
+# $2 = content of symlink
+mkpatch_symlink () {
+       rm -f "$1" &&
+       cat <<-EOF
+       diff --git a/$1 b/$1
+       new file mode 120000
+       index 0000000..$(printf "%s" "$2" | git hash-object --stdin)
+       --- /dev/null
+       +++ b/$1
+       @@ -0,0 +1 @@
+       +$2
+       \ No newline at end of file
+       EOF
+}
+
+test_expect_success 'cannot create file containing ..' '
+       mkpatch_add ../foo >patch &&
+       test_must_fail git apply patch &&
+       test_path_is_missing ../foo
+'
+
+test_expect_success 'can create file containing .. with --unsafe-paths' '
+       mkpatch_add ../foo >patch &&
+       git apply --unsafe-paths patch &&
+       test_path_is_file ../foo
+'
+
+test_expect_success  'cannot create file containing .. (index)' '
+       mkpatch_add ../foo >patch &&
+       test_must_fail git apply --index patch &&
+       test_path_is_missing ../foo
+'
+
+test_expect_success  'cannot create file containing .. with --unsafe-paths (index)' '
+       mkpatch_add ../foo >patch &&
+       test_must_fail git apply --index --unsafe-paths patch &&
+       test_path_is_missing ../foo
+'
+
+test_expect_success 'cannot delete file containing ..' '
+       mkpatch_del ../foo >patch &&
+       test_must_fail git apply patch &&
+       test_path_is_file ../foo
+'
+
+test_expect_success 'can delete file containing .. with --unsafe-paths' '
+       mkpatch_del ../foo >patch &&
+       git apply --unsafe-paths patch &&
+       test_path_is_missing ../foo
+'
+
+test_expect_success 'cannot delete file containing .. (index)' '
+       mkpatch_del ../foo >patch &&
+       test_must_fail git apply --index patch &&
+       test_path_is_file ../foo
+'
+
+test_expect_success SYMLINKS 'symlink escape via ..' '
+       {
+               mkpatch_symlink tmp .. &&
+               mkpatch_add tmp/foo ../foo
+       } >patch &&
+       test_must_fail git apply patch &&
+       test_path_is_missing tmp &&
+       test_path_is_missing ../foo
+'
+
+test_expect_success SYMLINKS 'symlink escape via .. (index)' '
+       {
+               mkpatch_symlink tmp .. &&
+               mkpatch_add tmp/foo ../foo
+       } >patch &&
+       test_must_fail git apply --index patch &&
+       test_path_is_missing tmp &&
+       test_path_is_missing ../foo
+'
+
+test_expect_success SYMLINKS 'symlink escape via absolute path' '
+       {
+               mkpatch_symlink tmp "$(pwd)" &&
+               mkpatch_add tmp/foo ../foo
+       } >patch &&
+       test_must_fail git apply patch &&
+       test_path_is_missing tmp &&
+       test_path_is_missing ../foo
+'
+
+test_expect_success SYMLINKS 'symlink escape via absolute path (index)' '
+       {
+               mkpatch_symlink tmp "$(pwd)" &&
+               mkpatch_add tmp/foo ../foo
+       } >patch &&
+       test_must_fail git apply --index patch &&
+       test_path_is_missing tmp &&
+       test_path_is_missing ../foo
+'
+
+test_done