{"thread":{"id":"38488","subject":"[PATCH v2 0/4] \"git apply\" safety","startedAt":"2015-02-02T23:27:26Z","lastAt":"2015-02-10T22:36:11Z","messageCount":34,"participants":["Junio C Hamano","Stefan Beller","Jeff King","Torsten Bögershausen","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"255544","messageId":"1422919650-13346-1-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":null,"subject":"[PATCH v2 0/4] \"git apply\" safety","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-02T23:27:26Z","receivedAt":"2015-02-02T23:27:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git apply\" have been fairly careless about letting the input follow\nsymbolic links, especially when used without the --index/--cached\noptions (which was more or less deliberate to mimic what \"patch\"\nused to do). When the input tells it to modify a/b/c, and lstat(2)\nsaid that there is \"a/b/c\" that matches the preimage in the input,\nwe happily overwrote it, even when a/b is a symbolic link that\npointed somewhere, even outside the working tree.\n\nThis series tightens things a bit for safety.\n\n (1) By default, we reject patches to \".git/file\", \"../some/where\",\n \"./this/././that\", etc., i.e. the names you cannot add to the\n index. Those who use \"git apply\" (without --index/--cached) as\n a replacement for GNU patch can use --unsafe-paths option to\n override this safety. This is what patch 1/4 does.\n\n (2) We do not allow a patch to depend on a location beyond a\n symbolic link (this includes \"a patch to remove a path beyond a\n symbolic link\"). This is patch 2/4 and 3/4.\n\n (3) We do not allow a patch to create result on a location beyond a\n symbolic link. This is patch 4/4.\n\nThere is no knob to override the latter two points, as this is not a\nsafety but is a correctness issue. Because Git keeps track of and\ncan express changes to symbolic links, a patch that expects a file\n\"a/b/c\" to be tracked (either the patch adds it, or it modifies an\nexisting file tehre) implicitly expects that there is no symbolic\nlink \"a/b\", so attempting to apply such a patch to a tree with a\nsymbolic link at \"a/b\", even when the link points at some directory,\nmust detect that the target tree does not match what the patch's\npreimage expects and fail.\n\nThe previous attempt begins at around here:\n\n http://thread.gmane.org/gmane.linux.kernel/1874498/focus=1878385\n\nJunio C Hamano (4):\n apply: reject input that touches outside $cwd\n apply: do not read from the filesystem under --index\n apply: do not read from beyond a symbolic link\n apply: do not touch a file beyond a symbolic link\n\n Documentation/git-apply.txt | 14 +++-\n builtin/apply.c | 139 +++++++++++++++++++++++++++++++++++++++-\n t/t4122-apply-symlink-inside.sh | 89 +++++++++++++++++++++++++\n t/t4139-apply-escape.sh | 137 +++++++++++++++++++++++++++++++++++++++\n 4 files changed, 377 insertions(+), 2 deletions(-)\n create mode 100755 t/t4139-apply-escape.sh\n\n-- \n2.3.0-rc2-164-g799cdce\n"},{"id":"255545","messageId":"1422919650-13346-2-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1422919650-13346-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 1/4] apply: reject input that touches outside $cwd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-02T23:27:27Z","receivedAt":"2015-02-02T23:27:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"By default, a patch that affects outside the working area is\nrejected as a mistake (or a mischief); Git itself does not create\nsuch a patch, unless the user bends backwards and specifies a\nnon-standard prefix to \"git diff\" and friends.\n\nWhen `git apply` is used without either `--index` or `--cached`\noption as a \"better GNU patch\", the user can pass `--unsafe-paths`\noption to override this safety check. This cannot be used to escape\noutside the working tree when using `--index` or `--cached` to apply\nthe patch to the index.\n\nThe new test was stolen from Jeff King with slight enhancements.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-apply.txt | 14 ++++-\n builtin/apply.c | 26 +++++++++\n t/t4139-apply-escape.sh | 137 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 176 insertions(+), 1 deletion(-)\n create mode 100755 t/t4139-apply-escape.sh\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex f605327..a6e83a9 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -16,7 +16,7 @@ SYNOPSIS\n \t [--ignore-space-change | --ignore-whitespace ]\n \t [--whitespace=(nowarn|warn|fix|error|error-all)]\n \t [--exclude=] [--include=] [--directory=]\n-\t [--verbose] [...]\n+\t [--verbose] [--unsafe-paths] [...]\n \n DESCRIPTION\n -----------\n@@ -229,6 +229,18 @@ For example, a patch that talks about updating `a/git-gui.sh` to `b/git-gui.sh`\n can be applied to the file in the working tree `modules/git-gui/git-gui.sh` by\n running `git apply --directory=modules/git-gui`.\n \n+--unsafe-paths::\n+\tBy default, a patch that affects outside the working area is\n+\trejected as a mistake (or a mischief); Git itself never\n+\tcreates such a patch unless the user bends backwards and\n+\tspecifies nonstandard prefix to \"git diff\" and friends.\n++\n+When `git apply` is used without either `--index` or `--cached`\n+option as a \"better GNU patch\", the user can pass `--unsafe-paths`\n+option to override this safety check. This cannot be used to escape\n+outside the working tree when using `--index` or `--cached` to apply\n+the patch to the index.\n+\n Configuration\n -------------\n \ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex ef32e4f..c751ddf 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -50,6 +50,7 @@ static int apply_verbosely;\n static int allow_overlap;\n static int no_add;\n static int threeway;\n+static int unsafe_paths;\n static const char *fake_ancestor;\n static int line_termination = '\\n';\n static unsigned int p_context = UINT_MAX;\n@@ -3483,6 +3484,23 @@ static int check_to_create(const char *new_name, int ok_if_exists)\n \treturn 0;\n }\n \n+static void die_on_unsafe_path(struct patch *patch)\n+{\n+\tconst char *old_name = NULL;\n+\tconst char *new_name = NULL;\n+\tif (patch->is_delete)\n+\t\told_name = patch->old_name;\n+\telse if (!patch->is_new && !patch->is_copy)\n+\t\told_name = patch->old_name;\n+\tif (!patch->is_delete)\n+\t\tnew_name = patch->new_name;\n+\n+\tif (old_name && !verify_path(old_name))\n+\t\tdie(_(\"invalid path '%s'\"), old_name);\n+\tif (new_name && !verify_path(new_name))\n+\t\tdie(_(\"invalid path '%s'\"), new_name);\n+}\n+\n /*\n * Check and apply the patch in-core; leave the result in patch->result\n * for the caller to write it out to the final destination.\n@@ -3570,6 +3588,9 @@ static int check_patch(struct patch *patch)\n \t\t}\n \t}\n \n+\tif (!unsafe_paths)\n+\t\tdie_on_unsafe_path(patch);\n+\n \tif (apply_data(patch, &st, ce) < 0)\n \t\treturn error(_(\"%s: patch does not apply\"), name);\n \tpatch->rejected = 0;\n@@ -4379,6 +4400,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)\n \t\t\tN_(\"make sure the patch is applicable to the current index\")),\n \t\tOPT_BOOL(0, \"cached\", &cached,\n \t\t\tN_(\"apply a patch without touching the working tree\")),\n+\t\tOPT_BOOL(0, \"unsafe-paths\", &unsafe_paths,\n+\t\t\tN_(\"accept a patch to touch outside the working area\")),\n \t\tOPT_BOOL(0, \"apply\", &force_apply,\n \t\t\tN_(\"also apply the patch (use with --stat/--summary/--check)\")),\n \t\tOPT_BOOL('3', \"3way\", &threeway,\n@@ -4451,6 +4474,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)\n \t\t\tdie(_(\"--cached outside a repository\"));\n \t\tcheck_index = 1;\n \t}\n+\tif (check_index)\n+\t\tunsafe_paths = 0;\n+\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tint fd;\ndiff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh\nnew file mode 100755\nindex 0000000..5e67179\n--- /dev/null\n+++ b/t/t4139-apply-escape.sh\n@@ -0,0 +1,137 @@\n+#!/bin/sh\n+\n+test_description='paths written by git-apply cannot escape the working tree'\n+. ./test-lib.sh\n+\n+# tests will try to write to ../foo, and we do not\n+# want them to escape the trash directory when they\n+# fail\n+test_expect_success 'bump git repo one level down' '\n+\tmkdir inside &&\n+\tmv .git inside/ &&\n+\tcd inside\n+'\n+\n+# $1 = name of file\n+# $2 = current path to file (if different)\n+mkpatch_add() {\n+\trm -f \"${2:-$1}\" &&\n+\tcat <<-EOF\n+\tdiff --git a/$1 b/$1\n+\tnew file mode 100644\n+\tindex 0000000..53c74cd\n+\t--- /dev/null\n+\t+++ b/$1\n+\t@@ -0,0 +1 @@\n+\t+evil\n+\tEOF\n+}\n+\n+mkpatch_del() {\n+\techo evil >\"${2:-$1}\" &&\n+\tcat <<-EOF\n+\tdiff --git a/$1 b/$1\n+\tdeleted file mode 100644\n+\tindex 53c74cd..0000000\n+\t--- a/$1\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-evil\n+\tEOF\n+}\n+\n+# $1 = name of file\n+# $2 = content of symlink\n+mkpatch_symlink() {\n+\trm -f \"$1\" &&\n+\tcat <<-EOF\n+\tdiff --git a/$1 b/$1\n+\tnew file mode 120000\n+\tindex 0000000..$(printf \"%s\" \"$2\" | git hash-object --stdin)\n+\t--- /dev/null\n+\t+++ b/$1\n+\t@@ -0,0 +1 @@\n+\t+$2\n+\t\\ No newline at end of file\n+\tEOF\n+}\n+\n+test_expect_success 'cannot add file containing ..' '\n+\tmkpatch_add ../foo >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'can add file containing .. with --unsafe-paths' '\n+\tmkpatch_add ../foo >patch &&\n+\tgit apply --unsafe-paths patch &&\n+\ttest_path_is_file ../foo\n+'\n+\n+test_expect_success 'cannot add file containing .. (index)' '\n+\tmkpatch_add ../foo >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'cannot add file containing .. with --unsafe-paths (index)' '\n+\tmkpatch_add ../foo >patch &&\n+\ttest_must_fail git apply --index --unsafe-paths patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'cannot del file containing ..' '\n+\tmkpatch_del ../foo >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_file ../foo\n+'\n+\n+test_expect_success 'can del file containing .. with --unsafe-paths' '\n+\tmkpatch_del ../foo >patch &&\n+\tgit apply --unsafe-paths patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'cannot del file containing .. (index)' '\n+\tmkpatch_del ../foo >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_file ../foo\n+'\n+\n+test_expect_failure 'symlink escape via ..' '\n+\t{\n+\t\tmkpatch_symlink tmp .. &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_failure 'symlink escape via .. (index)' '\n+\t{\n+\t\tmkpatch_symlink tmp .. &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_failure 'symlink escape via absolute path' '\n+\t{\n+\t\tmkpatch_symlink tmp \"$(pwd)\" &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'symlink escape via absolute path (index)' '\n+\t{\n+\t\tmkpatch_symlink tmp \"$(pwd)\" &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_done\n-- \n2.3.0-rc2-164-g799cdce\n"},{"id":"255546","messageId":"1422919650-13346-3-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1422919650-13346-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 2/4] apply: do not read from the filesystem under --index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-02T23:27:28Z","receivedAt":"2015-02-02T23:27:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We currently read the preimage to apply a patch from the index only\nwhen the --cached option is given. Do so also when the command is\nrunning under the --index option. With --index, the index entry and\nthe working tree file for a path that is involved in a patch must be\nidentical, so this should not affect the result, but by reading from\nthe index, we will get the protection to avoid reading an unintended\npath beyond a symbolic link automatically.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/apply.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex c751ddf..05eaf54 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3136,7 +3136,7 @@ static int load_patch_target(struct strbuf *buf,\n \t\t\t const char *name,\n \t\t\t unsigned expected_mode)\n {\n-\tif (cached) {\n+\tif (cached || check_index) {\n \t\tif (read_file_or_gitlink(ce, buf))\n \t\t\treturn error(_(\"read of %s failed\"), name);\n \t} else if (name) {\n-- \n2.3.0-rc2-164-g799cdce\n"},{"id":"255547","messageId":"1422919650-13346-4-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1422919650-13346-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 3/4] apply: do not read from beyond a symbolic link","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-02T23:27:29Z","receivedAt":"2015-02-02T23:27:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We should reject a patch, whether it renames/copies dir/file to\nelsewhere with or without modificiation, or updates dir/file in\nplace, if \"dir/\" part is actually a symbolic link to elsewhere,\nby making sure that the code to read the preimage does not read\nfrom a path that is beyond a symbolic link.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/apply.c | 2 ++\n t/t4122-apply-symlink-inside.sh | 19 +++++++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 05eaf54..60d821c 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3145,6 +3145,8 @@ static int load_patch_target(struct strbuf *buf,\n \t\t\t\treturn read_file_or_gitlink(ce, buf);\n \t\t\telse\n \t\t\t\treturn SUBMODULE_PATCH_WITHOUT_INDEX;\n+\t\t} else if (has_symlink_leading_path(name, strlen(name))) {\n+\t\t\treturn error(_(\"reading from '%s' beyond a symbolic link\"), name);\n \t\t} else {\n \t\t\tif (read_old_data(st, name, buf))\n \t\t\t\treturn error(_(\"read of %s failed\"), name);\ndiff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\nindex 70b3a06..035c080 100755\n--- a/t/t4122-apply-symlink-inside.sh\n+++ b/t/t4122-apply-symlink-inside.sh\n@@ -52,4 +52,23 @@ test_expect_success 'check result' '\n \n '\n \n+test_expect_success SYMLINKS 'do not read from beyond symbolic link' '\n+\tgit reset --hard &&\n+\tmkdir -p arch/x86_64/dir &&\n+\t>arch/x86_64/dir/file &&\n+\tgit add arch/x86_64/dir/file &&\n+\techo line >arch/x86_64/dir/file &&\n+\tgit diff >patch &&\n+\tgit reset --hard &&\n+\n+\tmkdir arch/i386/dir &&\n+\t>arch/i386/dir/file &&\n+\tln -s ../i386/dir arch/x86_64/dir &&\n+\n+\ttest_must_fail git apply patch &&\n+\ttest_must_fail git apply --cached patch &&\n+\ttest_must_fail git apply --index patch\n+\n+'\n+\n test_done\n-- \n2.3.0-rc2-164-g799cdce\n"},{"id":"255548","messageId":"1422919650-13346-5-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1422919650-13346-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 4/4] apply: do not touch a file beyond a symbolic link","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-02T23:27:30Z","receivedAt":"2015-02-02T23:27:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Because Git tracks symbolic links as symbolic links, a path that\nhas a symbolic link in its leading part (e.g. path/to/dir/file,\nwhere path/to/dir is a symbolic link to somewhere else, be it\ninside or outside the working tree) can never appear in a patch\nthat validly applies, unless the same patch first removes the\nsymbolic link to allow a directory to be created there.\n\nDetect and reject such a patch.\n\nThings to note:\n\n - Unfortunately, we cannot reuse the has_symlink_leading_path()\n from dir.c, as that is only about the working tree, but \"git\n apply\" can be told to apply the patch only to the index or to\n both the index and to the working tree.\n\n - We cannot directly use has_symlink_leading_path() even when we\n are applying only to the working tree, as an early patch of a\n valid input may remove a symbolic link path/to/dir and then a\n later patch of the input may create a path path/to/dir/file, but\n \"git apply\" first checks the input without touching either the\n index or the working tree. The leading symbolic link check must\n be done on the interim result we compute in-core (i.e. after the\n first patch, there is no path/to/dir symbolic link and it is\n perfectly valid to create path/to/dir/file).\n\n Similarly, when an input creates a symbolic link path/to/dir and\n then creates a file path/to/dir/file, we need to flag it as an\n error without actually creating path/to/dir symbolic link in the\n filesystem.\n\nInstead, for any patch in the input that leaves a path (i.e. a non\ndeletion) in the result, we check all leading paths against the\nresulting tree that the patch would create by inspecting all the\npatches in the input and then the target of patch application\n(either the index or the working tree).\n\nThis way, we catch a mischief or a mistake to add a symbolic link\npath/to/dir and a file path/to/dir/file at the same time, while\nallowing a valid patch that removes a symbolic link path/to/dir and\nthen adds a file path/to/dir/file.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/apply.c | 109 ++++++++++++++++++++++++++++++++++++++++\n t/t4122-apply-symlink-inside.sh | 70 ++++++++++++++++++++++++++\n t/t4139-apply-escape.sh | 6 +--\n 3 files changed, 182 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 60d821c..a760d97 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3486,6 +3486,103 @@ static int check_to_create(const char *new_name, int ok_if_exists)\n \treturn 0;\n }\n \n+/*\n+ * We need to keep track of how symlinks in the preimage are\n+ * manipulated by the patches. A patch to add a/b/c where a/b\n+ * is a symlink should not be allowed to affect the directory\n+ * the symlink points at, but if the same patch removes a/b,\n+ * it is perfectly fine, as the patch removes a/b to make room\n+ * to create a directory a/b so that a/b/c can be created.\n+ */\n+static struct string_list symlink_changes;\n+#define SYMLINK_GOES_AWAY 01\n+#define SYMLINK_IN_RESULT 02\n+\n+static uintptr_t register_symlink_changes(const char *path, uintptr_t what)\n+{\n+\tstruct string_list_item *ent;\n+\n+\tent = string_list_lookup(&symlink_changes, path);\n+\tif (!ent) {\n+\t\tent = string_list_insert(&symlink_changes, path);\n+\t\tent->util = (void *)0;\n+\t}\n+\tent->util = (void *)(what | ((uintptr_t)ent->util));\n+\treturn (uintptr_t)ent->util;\n+}\n+\n+static uintptr_t check_symlink_changes(const char *path)\n+{\n+\tstruct string_list_item *ent;\n+\n+\tent = string_list_lookup(&symlink_changes, path);\n+\tif (!ent)\n+\t\treturn 0;\n+\treturn (uintptr_t)ent->util;\n+}\n+\n+static void prepare_symlink_changes(struct patch *patch)\n+{\n+\tfor ( ; patch; patch = patch->next) {\n+\t\tif ((patch->old_name && S_ISLNK(patch->old_mode)) &&\n+\t\t (patch->is_rename || patch->is_delete))\n+\t\t\t/* the symlink at patch->old_name is removed */\n+\t\t\tregister_symlink_changes(patch->old_name, SYMLINK_GOES_AWAY);\n+\n+\t\tif (patch->new_name && S_ISLNK(patch->new_mode))\n+\t\t\t/* the symlink at patch->new_name is created or remains */\n+\t\t\tregister_symlink_changes(patch->new_name, SYMLINK_IN_RESULT);\n+\t}\n+}\n+\n+static int path_is_beyond_symlink_1(struct strbuf *name)\n+{\n+\tdo {\n+\t\tunsigned int change;\n+\n+\t\twhile (--name->len && name->buf[name->len] != '/')\n+\t\t\t; /* scan backwards */\n+\t\tif (!name->len)\n+\t\t\tbreak;\n+\t\tname->buf[name->len] = '\\0';\n+\t\tchange = check_symlink_changes(name->buf);\n+\t\tif (change & SYMLINK_IN_RESULT)\n+\t\t\treturn 1;\n+\t\tif (change & SYMLINK_GOES_AWAY)\n+\t\t\t/*\n+\t\t\t * This cannot be \"return 0\", because we may\n+\t\t\t * see a new one created at a higher level.\n+\t\t\t */\n+\t\t\tcontinue;\n+\n+\t\t/* otherwise, check the preimage */\n+\t\tif (check_index) {\n+\t\t\tint pos = cache_name_pos(name->buf, name->len);\n+\t\t\tif (0 <= pos &&\n+\t\t\t S_ISLNK(active_cache[pos]->ce_mode))\n+\t\t\t\treturn 1;\n+\t\t} else {\n+\t\t\tstruct stat st;\n+\t\t\tif (!lstat(name->buf, &st) && S_ISLNK(st.st_mode))\n+\t\t\t\treturn 1;\n+\t\t}\n+\t} while (1);\n+\treturn 0;\n+}\n+\n+static int path_is_beyond_symlink(const char *name_)\n+{\n+\tint ret;\n+\tstruct strbuf name = STRBUF_INIT;\n+\n+\tassert(*name_ != '\\0');\n+\tstrbuf_addstr(&name, name_);\n+\tret = path_is_beyond_symlink_1(&name);\n+\tstrbuf_release(&name);\n+\n+\treturn ret;\n+}\n+\n static void die_on_unsafe_path(struct patch *patch)\n {\n \tconst char *old_name = NULL;\n@@ -3593,6 +3690,17 @@ static int check_patch(struct patch *patch)\n \tif (!unsafe_paths)\n \t\tdie_on_unsafe_path(patch);\n \n+\t/*\n+\t * An attempt to read from or delete a path that is beyond\n+\t * a symbolic link will be prevented by load_patch_target()\n+\t * that is called at the beginning of apply_data(). We need\n+\t * to make sure that the patch result is not deposited to\n+\t * a path that is beyond a symbolic link ourselves.\n+\t */\n+\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n+\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n+\t\t\t patch->new_name);\n+\n \tif (apply_data(patch, &st, ce) < 0)\n \t\treturn error(_(\"%s: patch does not apply\"), name);\n \tpatch->rejected = 0;\n@@ -3603,6 +3711,7 @@ static int check_patch_list(struct patch *patch)\n {\n \tint err = 0;\n \n+\tprepare_symlink_changes(patch);\n \tprepare_fn_table(patch);\n \twhile (patch) {\n \t\tif (apply_verbosely)\ndiff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\nindex 035c080..942c5cb 100755\n--- a/t/t4122-apply-symlink-inside.sh\n+++ b/t/t4122-apply-symlink-inside.sh\n@@ -71,4 +71,74 @@ test_expect_success SYMLINKS 'do not read from beyond symbolic link' '\n \n '\n \n+test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '\n+\n+\trm -rf arch/i386/dir arch/x86_64/dir &&\n+\tgit reset --hard &&\n+\tln -s ../i386/dir arch/x86_64/dir &&\n+\tgit add arch/x86_64/dir &&\n+\tgit diff HEAD >add_symlink.patch &&\n+\tgit reset --hard &&\n+\n+\tmkdir arch/x86_64/dir &&\n+\t>arch/x86_64/dir/file &&\n+\tgit add arch/x86_64/dir/file &&\n+\tgit diff HEAD >add_file.patch &&\n+\tgit diff -R HEAD >del_file.patch &&\n+\tgit reset --hard &&\n+\trm -fr arch/x86_64/dir &&\n+\n+\tcat add_symlink.patch add_file.patch >patch &&\n+\n+\tmkdir arch/i386/dir\n+'\n+\n+test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '\n+\n+\t# same input creates a confusing symbolic link\n+\ttest_must_fail git apply patch 2>error-wt &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-wt &&\n+\ttest_path_is_missing arch/x86_64/dir &&\n+\ttest_path_is_missing arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply --index patch 2>error-ix &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ix &&\n+\ttest_path_is_missing arch/x86_64/dir &&\n+\ttest_path_is_missing arch/i386/dir/file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir &&\n+\n+\ttest_must_fail git apply --cached patch 2>error-ct &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ct &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir\n+'\n+\n+test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '\n+\n+\t# existing symbolic link\n+\tgit reset --hard &&\n+\tln -s ../i386/dir arch/x86_64/dir &&\n+\tgit add arch/x86_64/dir &&\n+\n+\ttest_must_fail git apply add_file.patch 2>error-wt-add &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-wt-add &&\n+\ttest_path_is_missing arch/i386/dir/file &&\n+\n+\t>arch/i386/dir/file &&\n+\ttest_must_fail git apply del_file.patch 2>error-wt-del &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-wt-del &&\n+\ttest_path_is_file arch/i386/dir/file &&\n+\trm arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply --index add_file.patch 2>error-ix-add &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ix-add &&\n+\ttest_path_is_missing arch/i386/dir/file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir &&\n+\n+\ttest_must_fail git apply --cached add_file.patch 2>error-ct-file &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ct-file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir\n+'\n+\n test_done\ndiff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh\nindex 5e67179..25917cc 100755\n--- a/t/t4139-apply-escape.sh\n+++ b/t/t4139-apply-escape.sh\n@@ -98,7 +98,7 @@ test_expect_success 'cannot del file containing .. (index)' '\n \ttest_path_is_file ../foo\n '\n \n-test_expect_failure 'symlink escape via ..' '\n+test_expect_success 'symlink escape via ..' '\n \t{\n \t\tmkpatch_symlink tmp .. &&\n \t\tmkpatch_add tmp/foo ../foo\n@@ -107,7 +107,7 @@ test_expect_failure 'symlink escape via ..' '\n \ttest_path_is_missing ../foo\n '\n \n-test_expect_failure 'symlink escape via .. (index)' '\n+test_expect_success 'symlink escape via .. (index)' '\n \t{\n \t\tmkpatch_symlink tmp .. &&\n \t\tmkpatch_add tmp/foo ../foo\n@@ -116,7 +116,7 @@ test_expect_failure 'symlink escape via .. (index)' '\n \ttest_path_is_missing ../foo\n '\n \n-test_expect_failure 'symlink escape via absolute path' '\n+test_expect_success 'symlink escape via absolute path' '\n \t{\n \t\tmkpatch_symlink tmp \"$(pwd)\" &&\n \t\tmkpatch_add tmp/foo ../foo\n-- \n2.3.0-rc2-164-g799cdce\n"},{"id":"255549","messageId":"CAGZ79kbE2frDRgXkS0zGvufR1GP15wjgf9t49U87jPT83aTF0w@mail.gmail.com","threadId":"38488","inReplyTo":"1422919650-13346-4-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 3/4] apply: do not read from beyond a symbolic link","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-03T00:08:49Z","receivedAt":"2015-02-03T00:08:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 2, 2015 at 3:27 PM, Junio C Hamano wrote:\n> + test_must_fail git apply --index patch\n> +\n> +'\n\nIs the empty line between the last test_must_fail and the closing `'`\nintentional?\n"},{"id":"255550","messageId":"20150203004507.GA31946@peff.net","threadId":"38488","inReplyTo":"1422919650-13346-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-03T00:45:08Z","receivedAt":"2015-02-03T00:45:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 02, 2015 at 03:27:27PM -0800, Junio C Hamano wrote:\n\n> By default, a patch that affects outside the working area is\n> rejected as a mistake (or a mischief); Git itself does not create\n> such a patch, unless the user bends backwards and specifies a\n> non-standard prefix to \"git diff\" and friends.\n> \n> When `git apply` is used without either `--index` or `--cached`\n> option as a \"better GNU patch\", the user can pass `--unsafe-paths`\n> option to override this safety check. This cannot be used to escape\n> outside the working tree when using `--index` or `--cached` to apply\n> the patch to the index.\n> \n> The new test was stolen from Jeff King with slight enhancements.\n\nI notice that this includes the symlink-crossing tests, marked as\nfailures. Reading the series, I know what is going to happen later, but\ndo you want to leave a note like:\n\n Note that we also add tests for leaving the working directory by\n crossing symlink boundaries, which is not addressed in this patch.\n That is a separate issue caused following symlinks, which will come\n later.\n\nor something to help later readers of \"git log\"?\n\n> +--unsafe-paths::\n> +\tBy default, a patch that affects outside the working area is\n> +\trejected as a mistake (or a mischief); Git itself never\n> +\tcreates such a patch unless the user bends backwards and\n> +\tspecifies nonstandard prefix to \"git diff\" and friends.\n\nMinor wordsmithing: the usual idiom is \"bend over backwards\", and you\nprobably want s/specifies/& a/.\n\n> ++\n> +When `git apply` is used without either `--index` or `--cached`\n> +option as a \"better GNU patch\", the user can pass `--unsafe-paths`\n> +option to override this safety check.\n\nSimilarly, probably every instance of \"foo option\" would read better as\n\"the foo option\".\n\n> This cannot be used to escape\n> +outside the working tree when using `--index` or `--cached` to apply\n> +the patch to the index.\n\nI had trouble figuring out what this meant. Would it be simpler to just\nsay:\n\n This option has no effect when `--index` or `--cached` is in use.\n\nOr is there some other subtlety that you are trying to convey that I am\nmissing?\n\n-Peff\n"},{"id":"255551","messageId":"20150203005005.GB31946@peff.net","threadId":"38488","inReplyTo":"1422919650-13346-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-03T00:50:05Z","receivedAt":"2015-02-03T00:50:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 02, 2015 at 03:27:27PM -0800, Junio C Hamano wrote:\n\n> +test_expect_failure 'symlink escape via ..' '\n> +\t{\n> +\t\tmkpatch_symlink tmp .. &&\n> +\t\tmkpatch_add tmp/foo ../foo\n> +\t} >patch &&\n> +\ttest_must_fail git apply patch &&\n> +\ttest_path_is_missing ../foo\n> +'\n\nBy the way, does this patch (and the other symlink-escape ones) need to\nbe marked with the SYMLINKS prereq? For a pure-index application, it\nshould work anywhere, but I have a feeling that this \"git apply patch\"\nmay try to write the symlink to the filesystem, fail, and report failure\nfor the wrong reason. I don't have a SYMLINK-challenged filesystem to\ntest on, though.\n\n-Peff\n"},{"id":"255552","messageId":"20150203011139.GC31946@peff.net","threadId":"38488","inReplyTo":"1422919650-13346-5-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 4/4] apply: do not touch a file beyond a symbolic link","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-03T01:11:40Z","receivedAt":"2015-02-03T01:11:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 02, 2015 at 03:27:30PM -0800, Junio C Hamano wrote:\n\n> +static struct string_list symlink_changes;\n\nI notice we don't duplicate strings for this list. Are the paths we\nregister here always stable? I think they should be, as they are part of\nthe \"struct patch\".\n\n> +#define SYMLINK_GOES_AWAY 01\n> +#define SYMLINK_IN_RESULT 02\n> +\n> +static uintptr_t register_symlink_changes(const char *path, uintptr_t what)\n> +{\n> +\tstruct string_list_item *ent;\n> +\n> +\tent = string_list_lookup(&symlink_changes, path);\n> +\tif (!ent) {\n> +\t\tent = string_list_insert(&symlink_changes, path);\n> +\t\tent->util = (void *)0;\n> +\t}\n> +\tent->util = (void *)(what | ((uintptr_t)ent->util));\n> +\treturn (uintptr_t)ent->util;\n> +}\n\nI was surprised to see this as a bit-field and not a \"current state\" as\nwe walk through the set of patches to apply. I think this means we'll be\noverly cautious with a patch that does:\n\n 1. add foo as a symlink\n\n 2. remove foo\n\n 3. add foo/bar\n\nwhich is perfectly OK, but we'll reject. I suspect this is making things\nmuch simpler for you, because now we don't have to worry about order of\napplication that we were discussing the other day. If that is the\nreason, then I think patches like the above are an acceptable casualty.\nIt seems rather far-fetched in the first place for a real patch (as\nopposed to a mischievous one).\n\n> +\t/*\n> +\t * An attempt to read from or delete a path that is beyond\n> +\t * a symbolic link will be prevented by load_patch_target()\n> +\t * that is called at the beginning of apply_data(). We need\n> +\t * to make sure that the patch result is not deposited to\n> +\t * a path that is beyond a symbolic link ourselves.\n> +\t */\n> +\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n> +\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n> +\t\t\t patch->new_name);\n\nDo we need to check the patch->is_delete case here (with patch->old_name)?\n\nI had a suspicion that the new patch 3/4 to check the reading-side might\nhelp with that, but the comment here sounds like we do need to check\nhere, too (and I am not clear on how 3/4 handles deleting from a patch\non the far side of a symlink we have just created).\n\nIt does seem to work, though. I'm just not sure how. :)\n\nHere's the test addition I came up with, because it didn't look like we\nwere covering this case. \n\ndiff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\nindex 942c5cb..fbba8dd 100755\n--- a/t/t4122-apply-symlink-inside.sh\n+++ b/t/t4122-apply-symlink-inside.sh\n@@ -89,6 +89,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '\n \trm -fr arch/x86_64/dir &&\n \n \tcat add_symlink.patch add_file.patch >patch &&\n+\tcat add_symlink.patch del_file.patch >tricky_del &&\n \n \tmkdir arch/i386/dir\n '\n@@ -112,6 +113,20 @@ test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '\n \ttest_i18ngrep \"beyond a symbolic link\" error-ct &&\n \ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n \ttest_must_fail git ls-files --error-unmatch arch/i386/dir\n+\n+\t>arch/i386/dir/file &&\n+\tgit add arch/i386/dir/file &&\n+\ttest_must_fail git apply tricky_del &&\n+\ttest_path_is_file arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply --index tricky_del &&\n+\ttest_path_is_file arch/i386/dir/file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\tgit ls-files --error-unmatch arch/i386/dir &&\n+\n+\ttest_must_fail git apply --cached tricky_del &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\tgit ls-files --error-unmatch arch/i386/dir\n '\n \n test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '\n@@ -125,6 +140,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '\n \ttest_i18ngrep \"beyond a symbolic link\" error-wt-add &&\n \ttest_path_is_missing arch/i386/dir/file &&\n \n+\tmkdir arch/i386/dir &&\n \t>arch/i386/dir/file &&\n \ttest_must_fail git apply del_file.patch 2>error-wt-del &&\n \ttest_i18ngrep \"beyond a symbolic link\" error-wt-del &&\n"},{"id":"255553","messageId":"xmqq61bjsqoo.fsf@gitster.dls.corp.google.com","threadId":"38488","inReplyTo":"20150203011139.GC31946@peff.net","subject":"Re: [PATCH v2 4/4] apply: do not touch a file beyond a symbolic link","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-03T01:56:55Z","receivedAt":"2015-02-03T01:56:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Feb 02, 2015 at 03:27:30PM -0800, Junio C Hamano wrote:\n>\n>> +static struct string_list symlink_changes;\n>\n> I notice we don't duplicate strings for this list. Are the paths we\n> register here always stable? I think they should be, as they are part of\n> the \"struct patch\".\n\nYeah, and I also forgot to free this string-list itself. Needs\nfixing.\n\n> I think this means we'll be\n> overly cautious with a patch that does:\n>\n> 1. add foo as a symlink\n>\n> 2. remove foo\n>\n> 3. add foo/bar\n>\n> which is perfectly OK\n\nNo, such a patchset is broken.\n\nA valid \"git apply\" input must *not* depend on the order of patches\nin it. The consequence is that \"an input to 'git apply' must not\nmention the fate of each path at most once.\"\n\n\"create B by copying A\" and \"modify A in-place\" can appear in the\nsame patchset in any order, and the new file B will have the\ncontents from the original A, not the result of modifying A\nin-place, which is what will be in the resulting A. That is how\n\"git diff\" expresses renames and copies, and that is why rearranging\nthe patchset using \"git diff -Oorderfile\" is safe.\n\n> but we'll reject. I suspect this is making things\n> much simpler for you, because now we don't have to worry about order of\n> application that we were discussing the other day.\n\nIt is not \"now this new decision made things simpler\". \"git diff\"\noutput and \"git apply\" application have been designed to work that\nway from day one. At least from day one of rename/copy feature.\n\nWe probably should start thinking about ripping out the fn_table[]\ncrud. It fundamentally cannot correctly work on an input that\nconcatenates more than one \"git diff\" outputs that have renames\nand/or copies of the same file, and it never will, and that is not\ndue to a bug in the implementation.\n\nThe reason why the incremental application is a fundamentally flawed\nconcept in the context of \"git apply\" is because you cannot tell the\nboundary between the original \"git diff\" outputs.\n\nImagine that you have three versions, A, B and C, and the original\ntwo \"git diff -M A B\" and \"git diff -M B C\" output said this:\n\n (A -> B)\n edit X in place and add two lines at the beginning\n create Z by copying X\n\n (B -> C)\n create Y by renaming X and add a line at the end\n\nIf you take \"git diff -M A C\", it should say:\n\n (A -> C)\n edit X in place and add two lines at the beginning\n create Y by copying X and add two lines at the beginning\n and a line at the end\n create Z by copying X\n\nNow, if you concatenate two \"git diff\" outputs and feed it to \"git\napply\", you would want it to express a patchset that goes from A to\nC, but think if you can really get such a semantics.\n\n edit X in place and add two lines at the beginning\n create Z by copying X\n create Y by renaming X and add a line at the end\n\nYou fundamentally cannot tell that Z needs to be a copy of X before\nthe change to X (which is what the usual \"git apply\" does), but Y\nneeds to start from a copy of X after the change to X. There is no\nclue to tell \"git apply\" that there is a boundary between the first\ntwo operations and the third one. It is impossible for the\nconcatenated patch to express the same thing as \"(A -> C)\" patch\ndoes, without inventng some \"I am now switching to a new basis\"\nmarker in the \"git apply\" input stream.\n\n>> +\t/*\n>> +\t * An attempt to read from or delete a path that is beyond\n>> +\t * a symbolic link will be prevented by load_patch_target()\n>> +\t * that is called at the beginning of apply_data(). We need\n>> +\t * to make sure that the patch result is not deposited to\n>> +\t * a path that is beyond a symbolic link ourselves.\n>> +\t */\n>> +\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n>> +\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n>> +\t\t\t patch->new_name);\n>\n> Do we need to check the patch->is_delete case here (with patch->old_name)?\n\n> I had a suspicion that the new patch 3/4 to check the reading-side might\n> help with that, but the comment here sounds like we do need to check\n> here, too\n\nHmm, the comment above was meant to tell you that we do not have to\nworry about the deletion case (because load_patch_target() will try\nto read the original to verify we are deleting what we expect to\ndelete at the beginning of apply_data(), and it will notice that\nold_name is beyond a symbolic link), but we still need to check the\nnon-deletion case. Strictly speaking, modify-in-place case does not\nhave to depend on this code (the same load_patch_target() check will\ncatch it because it wants to check the preimage).\n\nMay need rephrasing to clarify but I thought it was clear enough.\n"},{"id":"255555","messageId":"20150203020440.GA4917@peff.net","threadId":"38488","inReplyTo":"xmqq61bjsqoo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 4/4] apply: do not touch a file beyond a symbolic link","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-03T02:04:42Z","receivedAt":"2015-02-03T02:04:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 02, 2015 at 05:56:55PM -0800, Junio C Hamano wrote:\n\n> > I think this means we'll be\n> > overly cautious with a patch that does:\n> >\n> > 1. add foo as a symlink\n> >\n> > 2. remove foo\n> >\n> > 3. add foo/bar\n> >\n> > which is perfectly OK\n> \n> No, such a patchset is broken.\n> \n> A valid \"git apply\" input must *not* depend on the order of patches\n> in it. The consequence is that \"an input to 'git apply' must not\n> mention the fate of each path at most once.\"\n\nAh, right, I forgot we covered this already in the earlier discussion\n(but thanks for elaborating; I think the reason I forgot is that I did\nnot really understand all of the implications). If we do not have to\nworry about that, then it's not a problem.\n\n> >> +\t/*\n> >> +\t * An attempt to read from or delete a path that is beyond\n> >> +\t * a symbolic link will be prevented by load_patch_target()\n> >> +\t * that is called at the beginning of apply_data(). We need\n> >> +\t * to make sure that the patch result is not deposited to\n> >> +\t * a path that is beyond a symbolic link ourselves.\n> >> +\t */\n> >> +\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n> >> +\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n> >> +\t\t\t patch->new_name);\n> >\n> > Do we need to check the patch->is_delete case here (with patch->old_name)?\n> \n> > I had a suspicion that the new patch 3/4 to check the reading-side might\n> > help with that, but the comment here sounds like we do need to check\n> > here, too\n> \n> Hmm, the comment above was meant to tell you that we do not have to\n> worry about the deletion case (because load_patch_target() will try\n> to read the original to verify we are deleting what we expect to\n> delete at the beginning of apply_data(), and it will notice that\n> old_name is beyond a symbolic link), but we still need to check the\n> non-deletion case. Strictly speaking, modify-in-place case does not\n> have to depend on this code (the same load_patch_target() check will\n> catch it because it wants to check the preimage).\n> \n> May need rephrasing to clarify but I thought it was clear enough.\n\nAh, OK. I totally misread it, thinking that load_patch_target was what\nset up the symlink-table, and that was what you were referring to. It\nmight be more clear after \"...that is called at the beginning of\napply_data()\" to add \"...so we do not have to worry about that case\nhere\".\n\n-Peff\n"},{"id":"255561","messageId":"54D06303.7050100@web.de","threadId":"38488","inReplyTo":"1422919650-13346-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-02-03T05:56:19Z","receivedAt":"2015-02-03T05:56:19Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"If I am allowed to to some load thinking:\n\nThe commit msh header says:\n reject input that touches outside $cwd\n\nThe commit message says:\n By default, a patch that affects outside the working area\n\nAnd the new command line option is this:\n --unsafe-paths\n(Which may be a good choice to pretend people from using it without\n having read the documentaion)\n\n(And isn't working area the same as \"work space\" ?)\n\nI have the slight feeling that there may be more unsafe-path situations coming\nup in the future..\n\nWill\n--allow-outside-ws\nor\n--patch-outside-ws\nbe better ?\n"},{"id":"255580","messageId":"xmqq1tm6ss4z.fsf@gitster.dls.corp.google.com","threadId":"38488","inReplyTo":"CAGZ79kbE2frDRgXkS0zGvufR1GP15wjgf9t49U87jPT83aTF0w@mail.gmail.com","subject":"Re: [PATCH v2 3/4] apply: do not read from beyond a symbolic link","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-03T19:37:48Z","receivedAt":"2015-02-03T19:37:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Mon, Feb 2, 2015 at 3:27 PM, Junio C Hamano wrote:\n>> + test_must_fail git apply --index patch\n>> +\n>> +'\n>\n> Is the empty line between the last test_must_fail and the closing `'`\n> intentional?\n\nI think I just mimicked the previous existing one, but I can go with\nthe version without.\n"},{"id":"255581","messageId":"CAGZ79kYkT_1cKgn8_JseHovnXoeO1Hi5FFPE9k7T6qDXLVY_FA@mail.gmail.com","threadId":"38488","inReplyTo":"xmqq1tm6ss4z.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 3/4] apply: do not read from beyond a symbolic link","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-03T19:44:26Z","receivedAt":"2015-02-03T19:44:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Feb 3, 2015 at 11:37 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Mon, Feb 2, 2015 at 3:27 PM, Junio C Hamano wrote:\n>>> + test_must_fail git apply --index patch\n>>> +\n>>> +'\n>>\n>> Is the empty line between the last test_must_fail and the closing `'`\n>> intentional?\n>\n> I think I just mimicked the previous existing one, but I can go with\n> the version without.\n\nIt was really a honest question. I've seen and written and sent tests\nwithout such an ending\nempty line and it seemed to be ok.\n\nMaybe my email was more of \"Note to self: this is also a valid style\nused in the test suite\nat some places\"\n"},{"id":"255583","messageId":"xmqqpp9qrbgf.fsf@gitster.dls.corp.google.com","threadId":"38488","inReplyTo":"20150203005005.GB31946@peff.net","subject":"Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-03T20:23:28Z","receivedAt":"2015-02-03T20:23:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Feb 02, 2015 at 03:27:27PM -0800, Junio C Hamano wrote:\n>\n>> +test_expect_failure 'symlink escape via ..' '\n>> +\t{\n>> +\t\tmkpatch_symlink tmp .. &&\n>> +\t\tmkpatch_add tmp/foo ../foo\n>> +\t} >patch &&\n>> +\ttest_must_fail git apply patch &&\n>> +\ttest_path_is_missing ../foo\n>> +'\n>\n> By the way, does this patch (and the other symlink-escape ones) need to\n> be marked with the SYMLINKS prereq? For a pure-index application, it\n> should work anywhere, but I have a feeling that this \"git apply patch\"\n> may try to write the symlink to the filesystem, fail, and report failure\n> for the wrong reason. I don't have a SYMLINK-challenged filesystem to\n> test on, though.\n\nWe check the links to be created by the patch itself in-core before\ngoing to the filesystem, and the symbolic links you are creating\nusing mkpatch_symlink should be caught before we invoke symlink(2),\nI think.\n\nIn other words, this series attempts to stick to the \"verify\neverything in-core before deciding that it is OK to touch the\nworking tree or the index\".\n\nA few new tests in t4122 do try to see that the command is not\nfooled by existihng symbolic links on the filesystem and they need\nto be marked with SYMLINKS prerequisite.\n"},{"id":"255584","messageId":"xmqqiofirb29.fsf@gitster.dls.corp.google.com","threadId":"38488","inReplyTo":"CAGZ79kYkT_1cKgn8_JseHovnXoeO1Hi5FFPE9k7T6qDXLVY_FA@mail.gmail.com","subject":"Re: [PATCH v2 3/4] apply: do not read from beyond a symbolic link","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-03T20:31:58Z","receivedAt":"2015-02-03T20:31:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, Feb 3, 2015 at 11:37 AM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> On Mon, Feb 2, 2015 at 3:27 PM, Junio C Hamano wrote:\n>>>> + test_must_fail git apply --index patch\n>>>> +\n>>>> +'\n>>>\n>>> Is the empty line between the last test_must_fail and the closing `'`\n>>> intentional?\n>>\n>> I think I just mimicked the previous existing one, but I can go with\n>> the version without.\n>\n> It was really a honest question. I've seen and written and sent\n> tests without such an ending empty line and it seemed to be ok.\n\nIt was a honest answer, too.\n\nI see existing new-style tests [*1*] that have and/or lack the empty\nline at the end of the command, and I do not have particular\npreference either way. Having these variations did not bother me\ntoo much, at least until now. We might want to make them consistent,\nand the time to be careful is when we add new tests, so it was very\nvalid for you to bring it up against this patch. But I do not know\nif we as the Git developer community have preference either way, and\nI myself don't either, so...\n\n\n[Footnote]\n\n*1* The really ancient style we want to clean-up goes like\n\n cmd for setup > expect\n \n test_expect_success \\\n 'title of the test' \\\n 'command && more command &&\n yet more command > actual &&\n test_cmp expect actual'\n\n cmd for cleanup\n\n and we want them to read more like\n\n test_expect_success 'title of the test' '\n test_when_finished \"cmd for cleanup\" &&\n cmd for setup >expect &&\n command &&\n more command &&\n yet more command >actual &&\n test_cmp expect actual\n '\n"},{"id":"255585","messageId":"xmqqy4oepv59.fsf@gitster.dls.corp.google.com","threadId":"38488","inReplyTo":"20150203011139.GC31946@peff.net","subject":"Re: [PATCH v2 4/4] apply: do not touch a file beyond a symbolic link","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-03T21:01:06Z","receivedAt":"2015-02-03T21:01:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Here's the test addition I came up with, because it didn't look like we\n> were covering this case. \n\nThanks.\n\n> diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\n> index 942c5cb..fbba8dd 100755\n> --- a/t/t4122-apply-symlink-inside.sh\n> +++ b/t/t4122-apply-symlink-inside.sh\n> @@ -89,6 +89,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '\n> \trm -fr arch/x86_64/dir &&\n> \n> \tcat add_symlink.patch add_file.patch >patch &&\n> +\tcat add_symlink.patch del_file.patch >tricky_del &&\n\nThis new patch\n\n (1) creates a symlink arch/x86_64/dir pointing at ../i386/dir\n (2) deletes arch/x86_64/dir/file\n\nIt can be a valid patch to be applied to a tree where arch/x86_64/dir/file\nis in the index (either as a regular file, a symlink, or even a submodule)\nand nothing else is in arch/x86_64/dir directory.\n\n\n> @@ -112,6 +113,20 @@ test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '\n> \ttest_i18ngrep \"beyond a symbolic link\" error-ct &&\n> \ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n> \ttest_must_fail git ls-files --error-unmatch arch/i386/dir\n> +\n> +\t>arch/i386/dir/file &&\n> +\tgit add arch/i386/dir/file &&\n\nAt this point, the target of the patch application has:\n\n\tarch/i386/boot/Makefile\n\tarch/i386/dir/file\n\tarch/x86_64/boot/Makefile\n\nall of which are regular files. The index and the working tree\nmatch.\n\n> +\ttest_must_fail git apply tricky_del &&\n\nThe reason why this does not apply has nothing to do with the topic\nof this series, I think. It wants to delete arch/x86_64/dir/file,\nwhich does not exist in the target, and the patch is rejected.\n\nIt is a good test to make sure that we do not \"incrementally\" apply\nand get fooled by arch/x86_64/dir that will become a symbolic link,\nmaking arch/x86_64/dir/file to appear as arch/i386/dir/file that\ndoes exist in the preimage.\n\n> +\ttest_path_is_file arch/i386/dir/file &&\n\nWhen we reject the entire patch, we do so without touching the\noutside world, of course ;-), which is good.\n\n> +\ttest_must_fail git apply --index tricky_del &&\n> +\ttest_path_is_file arch/i386/dir/file &&\n> +\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n> +\tgit ls-files --error-unmatch arch/i386/dir &&\n> +\n> +\ttest_must_fail git apply --cached tricky_del &&\n> +\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n> +\tgit ls-files --error-unmatch arch/i386/dir\n> '\n\nIn both of the above, \"git apply\" rejects its input for the same\nreason. The file it wants to remove does not exist in the target.\n\n> test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '\n> @@ -125,6 +140,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '\n> \ttest_i18ngrep \"beyond a symbolic link\" error-wt-add &&\n> \ttest_path_is_missing arch/i386/dir/file &&\n> \n> +\tmkdir arch/i386/dir &&\n\nThanks for spotting this one ;-)\n\n> \t>arch/i386/dir/file &&\n> \ttest_must_fail git apply del_file.patch 2>error-wt-del &&\n\ndel_file.patch wants to remove arch/x86_64/dir/file, and arch/x86_64/dir\nis a symbolic link to ../i386/dir in the target at this point, so it\nis trying to delete beyond the symbolic link, which gets rejected by\nthis series. Good.\n\n> \ttest_i18ngrep \"beyond a symbolic link\" error-wt-del &&\n"},{"id":"255586","messageId":"20150203210140.GA20594@peff.net","threadId":"38488","inReplyTo":"xmqqpp9qrbgf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-03T21:01:40Z","receivedAt":"2015-02-03T21:01:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 03, 2015 at 12:23:28PM -0800, Junio C Hamano wrote:\n\n> > By the way, does this patch (and the other symlink-escape ones) need to\n> > be marked with the SYMLINKS prereq? For a pure-index application, it\n> > should work anywhere, but I have a feeling that this \"git apply patch\"\n> > may try to write the symlink to the filesystem, fail, and report failure\n> > for the wrong reason. I don't have a SYMLINK-challenged filesystem to\n> > test on, though.\n> \n> We check the links to be created by the patch itself in-core before\n> going to the filesystem, and the symbolic links you are creating\n> using mkpatch_symlink should be caught before we invoke symlink(2),\n> I think.\n> \n> In other words, this series attempts to stick to the \"verify\n> everything in-core before deciding that it is OK to touch the\n> working tree or the index\".\n\nRight, I do not think these tests will _fail_ when the filesystem does\nnot support symlinks. But nor are they actually testing anything\ninteresting. They would pass on such a system even without your patch,\nas we would fail to apply even the symlink creation part of the patch.\n\nI can live with leaving them unmarked, though. It gets the code\nexercised on more systems, which gives a slightly higher chance of\ncatching some other unexpected breakage.\n\n-Peff\n"},{"id":"255587","messageId":"xmqqtwz2pu4c.fsf@gitster.dls.corp.google.com","threadId":"38488","inReplyTo":"20150203210140.GA20594@peff.net","subject":"Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-03T21:23:15Z","receivedAt":"2015-02-03T21:23:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Right, I do not think these tests will _fail_ when the filesystem does\n> not support symlinks. But nor are they actually testing anything\n> interesting. They would pass on such a system even without your patch,\n> as we would fail to apply even the symlink creation part of the patch.\n\nI thought we write out the contents of the symbolic link as a\nregular file on such a filesystem, and as long as we do not expect\n\"test -h expected-to-be-symlink-we-just-created\" to succeed we would\nbe fine.\n"},{"id":"255588","messageId":"20150203212450.GC20594@peff.net","threadId":"38488","inReplyTo":"xmqqtwz2pu4c.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-03T21:24:50Z","receivedAt":"2015-02-03T21:24:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 03, 2015 at 01:23:15PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Right, I do not think these tests will _fail_ when the filesystem does\n> > not support symlinks. But nor are they actually testing anything\n> > interesting. They would pass on such a system even without your patch,\n> > as we would fail to apply even the symlink creation part of the patch.\n> \n> I thought we write out the contents of the symbolic link as a\n> regular file on such a filesystem, and as long as we do not expect\n> \"test -h expected-to-be-symlink-we-just-created\" to succeed we would\n> be fine.\n\nBut wouldn't we still fail writing \"foo/bar\" at that point if \"foo\" is a\nregular file (again, we should never do this, but that is the point of\nthe test).\n\n-Peff\n"},{"id":"255589","messageId":"xmqqpp9qptc3.fsf@gitster.dls.corp.google.com","threadId":"38488","inReplyTo":"20150203212450.GC20594@peff.net","subject":"Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-03T21:40:12Z","receivedAt":"2015-02-03T21:40:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But wouldn't we still fail writing \"foo/bar\" at that point if \"foo\" is a\n> regular file (again, we should never do this, but that is the point of\n> the test).\n\nThe point of the test is not to create foo, whether it is a symlink\nor an emulating regular file, in the first place.\n\nIn this piece:\n\n>> +test_expect_failure 'symlink escape via ..' '\n>> +\t{\n>> +\t\tmkpatch_symlink tmp .. &&\n\nThis is a patch to create \"tmp\" that points at \"..\"\n\n>> +\t\tmkpatch_add tmp/foo ../foo\n\nAnd this is a patch to create \"tmp/foo\", and make sure ../foo does\nnot exist in the filesystem to prepare for the test.\n\n>> +\t} >patch &&\n>> +\ttest_must_fail git apply patch &&\n\nAnd this patch, if the rejection logic were to be broken in the\nfuture, might create \"tmp\" and then try to follow it to affect\n\"../foo\".\n\n>> +\ttest_path_is_missing ../foo\n\nSo if this test makes sure if \"tmp\" is missing, then it would be\nalright, no? The \"follow the symlink to affect ../foo\" may not\nhappen on a filesystem without symlinks, but verifying that \"tmp\"\nis missing will assure us that the patch application is atomic,\ni.e. if we see \"tmp\" on the filesystem after seeing \"git apply\"\nfailed, that is a sign that \"git apply\" failed to be atomic.\n"},{"id":"255591","messageId":"20150203215043.GA21357@peff.net","threadId":"38488","inReplyTo":"xmqqpp9qptc3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-03T21:50:43Z","receivedAt":"2015-02-03T21:50:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 03, 2015 at 01:40:12PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But wouldn't we still fail writing \"foo/bar\" at that point if \"foo\" is a\n> > regular file (again, we should never do this, but that is the point of\n> > the test).\n> \n> The point of the test is not to create foo, whether it is a symlink\n> or an emulating regular file, in the first place.\n\nI thought the point was not to create \"../bar\", when \"foo\" points to\n\"..\". I agree that the way you have implemented it is that we would\nnever even write \"foo\", and the test checks for that, but to me that is\nthe least interesting bit of what is being tested. Crossing a symlink\nboundary and escaping from the tree are interesting, and the atomicity\nis a side note. We could also realize that treating \"foo\" as a file\nwould fail and cancel the whole operation atomically, too.\n\nBut I think we are getting into contrasting our mental models, which is\nprobably not productive. I am OK with leaving it without the SYMLINKS\nflag, as it should pass on systems that do not handle symlinks. And if\nit does not, then that will be a good cross-check that our analysis was\nsane. :)\n\n-Peff\n"},{"id":"255592","messageId":"xmqqh9v2prvm.fsf@gitster.dls.corp.google.com","threadId":"38488","inReplyTo":"20150203215043.GA21357@peff.net","subject":"Re: [PATCH v2 1/4] apply: reject input that touches outside $cwd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-03T22:11:41Z","receivedAt":"2015-02-03T22:11:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 03, 2015 at 01:40:12PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > But wouldn't we still fail writing \"foo/bar\" at that point if \"foo\" is a\n>> > regular file (again, we should never do this, but that is the point of\n>> > the test).\n>> \n>> The point of the test is not to create foo, whether it is a symlink\n>> or an emulating regular file, in the first place.\n>\n> I thought the point was not to create \"../bar\", when \"foo\" points to\n> \"..\".\n\nThat is not what the new test you added was doing, though. You are\ncalling \"tmp\" in that test \"foo\" and \"../foo\" in the test is called\n\"../bar\" in the message I am responding to, so that is confusing,\nbut in these tests you added, I do not see anything that prepares a\nsymbolic link ON the filesystem and wait for \"git apply\" to get\nfooled.\n\n> I agree that the way you have implemented it is that we would\n> never even write \"foo\", and the test checks for that, but to me that is\n> the least interesting bit of what is being tested.\n\nThey are both interesting. When rejecting an input, \"git apply\"\nmust be atomic. When checking an input, \"git apply\" should notice\nand reject a patch that tries to follow a symbolic link.\n\nTaken together:\n\n (1) If a patchset tries to create a symbolic link and then tries to\n follow it, the latter principle should make \"git apply\" to\n reject the patchset, and the former should make sure there is\n no external effect.\n\n (2) If a patchset tries to affect a path, and the target of the\n patch application has a symlink that may divert the operation\n to the original path the patchset wants to affect, the latter\n principle should make \"git apply\" to reject the patchset, too.\n\nAnd my observation is that the new tests that are not protected by\nSYMLINKS prerequisite (i.e. all of them) in your new test 4139, are\nall the former kind. As \"git aplly\" must be atomic, rejection must\nbe decided without touching the filesystem at all. Hence there is\nno need for the filesystem to even support symbolic links.\n\nBut some bozo may try to break \"git apply\" in the future and try to\nincrementally apply the patch in a patchset, and at that point, the\nexisting \"test_must_fail git apply\" may pass not because we\ncorrectly decide not to follow symbolic links but because his broken\nversion of \"git apply\" would try to create a symbolic link (which we\nwould turn into a regular file) and then the filesystem would fail\nto follow that symbolic link mimic, and as the result the test may\nstill pass.\n\nIn order to prevent that from happening, we may want to make sure\nthat\n\n - \"test_must_fail git apply\"\n\n - There is no \"foo\" (or \"tmp\"); the input to 'git apply' is the\n only thing that could create, as you do not create symlinks as\n traps before running 'git apply', so this will catch the 'make it\n incremental and then break the do-not-follow logic'.\n\n - There is no \"../bar\" (or \"../foo\").\n\nThe second one is missing from your tests, I think, and it would be\na very good addition, even on a platform with SYMLINKS prerequisite\nsatisfied. The future change might be trying to make it incremental\nand impelent do-not-follow logic by observing what is in the\nfilesystem, and we do want to catch such a broken implementation.\n\n\n\n\n\t\n"},{"id":"255593","messageId":"CAPig+cRw==UoX7-zjykuXZQ43YP3cGKYtGCmc2rkOtV8JeEJGQ@mail.gmail.com","threadId":"38488","inReplyTo":"xmqqy4oepv59.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 4/4] apply: do not touch a file beyond a symbolic link","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-02-03T23:40:37Z","receivedAt":"2015-02-03T23:40:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 3, 2015 at 4:01 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>> diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\n>> index 942c5cb..fbba8dd 100755\n>> --- a/t/t4122-apply-symlink-inside.sh\n>> +++ b/t/t4122-apply-symlink-inside.sh\n>> @@ -112,6 +113,20 @@ test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '\n>> test_i18ngrep \"beyond a symbolic link\" error-ct &&\n>> test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n>> test_must_fail git ls-files --error-unmatch arch/i386/dir\n\nBroken &&-chain.\n\n>> +\n>> + >arch/i386/dir/file &&\n>> + git add arch/i386/dir/file &&\n>\n> At this point, the target of the patch application has:\n>\n> arch/i386/boot/Makefile\n> arch/i386/dir/file\n> arch/x86_64/boot/Makefile\n>\n> all of which are regular files. The index and the working tree\n> match.\n"},{"id":"255594","messageId":"1423010662-26497-1-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1422919650-13346-1-git-send-email-gitster@pobox.com","subject":"[PATCH v3 0/4] \"git apply\" safety","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-04T00:44:18Z","receivedAt":"2015-02-04T00:44:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The design and implementation haven't changed.\n\nThis round primarily is to update documentation and comments with\nrephrases and grammofixes and tighten some tests.\n\nThe previous round begins here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/263291\n\nJunio C Hamano (4):\n apply: reject input that touches outside the working area\n apply: do not read from the filesystem under --index\n apply: do not read from beyond a symbolic link\n apply: do not touch a file beyond a symbolic link\n\n Documentation/git-apply.txt | 15 ++++-\n builtin/apply.c | 141 +++++++++++++++++++++++++++++++++++++++-\n t/t4122-apply-symlink-inside.sh | 106 ++++++++++++++++++++++++++++++\n t/t4139-apply-escape.sh | 137 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 397 insertions(+), 2 deletions(-)\n create mode 100755 t/t4139-apply-escape.sh\n\n-- \n2.3.0-rc2-168-g106c876\n"},{"id":"255596","messageId":"1423010662-26497-2-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1423010662-26497-1-git-send-email-gitster@pobox.com","subject":"[PATCH v3 1/4] apply: reject input that touches outside the working area","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-04T00:44:19Z","receivedAt":"2015-02-04T00:44:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"By default, a patch that affects outside the working area\n(either a Git controlled working tree, or the current working\ndirectory when \"git apply\" is used as a replacement of GNU\npatch) rejected as a mistake (or a mischief); Git itself does\nnot create such a patch, unless the user bends over backwards\nand specifies a non-standard prefix to \"git diff\" and friends.\n\nWhen `git apply` is used without either the `--index` or the\n`--cached` option as a \"better GNU patch\", the user can pass\nthe `--unsafe-paths` option to override this safety check. This\noption has no effect when `--index` or `--cached` is in use.\n\nThe new test was stolen from Jeff King with slight enhancements.\nNote that a few new tests for touching outside the working area by\nfollowing a symbolic link are still expected to fail at this step,\nbut will be fixed in later steps.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Documentation rephrased with grammofixes courtesy of Peff.\n\n Documentation/git-apply.txt | 15 ++++-\n builtin/apply.c | 26 +++++++++\n t/t4139-apply-escape.sh | 137 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 177 insertions(+), 1 deletion(-)\n create mode 100755 t/t4139-apply-escape.sh\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex f605327..a2cdc4a 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -16,7 +16,7 @@ SYNOPSIS\n \t [--ignore-space-change | --ignore-whitespace ]\n \t [--whitespace=(nowarn|warn|fix|error|error-all)]\n \t [--exclude=] [--include=] [--directory=]\n-\t [--verbose] [...]\n+\t [--verbose] [--unsafe-paths] [...]\n \n DESCRIPTION\n -----------\n@@ -229,6 +229,19 @@ For example, a patch that talks about updating `a/git-gui.sh` to `b/git-gui.sh`\n can be applied to the file in the working tree `modules/git-gui/git-gui.sh` by\n running `git apply --directory=modules/git-gui`.\n \n+--unsafe-paths::\n+\tBy default, a patch that affects outside the working area\n+\t(either a Git controlled working tree, or the current working\n+\tdirectory when \"git apply\" is used as a replacement of GNU\n+\tpatch) rejected as a mistake (or a mischief); Git itself does\n+\tnot create such a patch, unless the user bends over backwards\n+\tand specifies a nonstandard prefix to \"git diff\" and friends.\n++\n+When `git apply` is used without either the `--index` or the `--cached`\n+option as a \"better GNU patch\", the user can pass the `--unsafe-paths`\n+option to override this safety check. This option has no effect when\n+`--index` or `--cached` is in use.\n+\n Configuration\n -------------\n \ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex ef32e4f..c751ddf 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -50,6 +50,7 @@ static int apply_verbosely;\n static int allow_overlap;\n static int no_add;\n static int threeway;\n+static int unsafe_paths;\n static const char *fake_ancestor;\n static int line_termination = '\\n';\n static unsigned int p_context = UINT_MAX;\n@@ -3483,6 +3484,23 @@ static int check_to_create(const char *new_name, int ok_if_exists)\n \treturn 0;\n }\n \n+static void die_on_unsafe_path(struct patch *patch)\n+{\n+\tconst char *old_name = NULL;\n+\tconst char *new_name = NULL;\n+\tif (patch->is_delete)\n+\t\told_name = patch->old_name;\n+\telse if (!patch->is_new && !patch->is_copy)\n+\t\told_name = patch->old_name;\n+\tif (!patch->is_delete)\n+\t\tnew_name = patch->new_name;\n+\n+\tif (old_name && !verify_path(old_name))\n+\t\tdie(_(\"invalid path '%s'\"), old_name);\n+\tif (new_name && !verify_path(new_name))\n+\t\tdie(_(\"invalid path '%s'\"), new_name);\n+}\n+\n /*\n * Check and apply the patch in-core; leave the result in patch->result\n * for the caller to write it out to the final destination.\n@@ -3570,6 +3588,9 @@ static int check_patch(struct patch *patch)\n \t\t}\n \t}\n \n+\tif (!unsafe_paths)\n+\t\tdie_on_unsafe_path(patch);\n+\n \tif (apply_data(patch, &st, ce) < 0)\n \t\treturn error(_(\"%s: patch does not apply\"), name);\n \tpatch->rejected = 0;\n@@ -4379,6 +4400,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)\n \t\t\tN_(\"make sure the patch is applicable to the current index\")),\n \t\tOPT_BOOL(0, \"cached\", &cached,\n \t\t\tN_(\"apply a patch without touching the working tree\")),\n+\t\tOPT_BOOL(0, \"unsafe-paths\", &unsafe_paths,\n+\t\t\tN_(\"accept a patch to touch outside the working area\")),\n \t\tOPT_BOOL(0, \"apply\", &force_apply,\n \t\t\tN_(\"also apply the patch (use with --stat/--summary/--check)\")),\n \t\tOPT_BOOL('3', \"3way\", &threeway,\n@@ -4451,6 +4474,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)\n \t\t\tdie(_(\"--cached outside a repository\"));\n \t\tcheck_index = 1;\n \t}\n+\tif (check_index)\n+\t\tunsafe_paths = 0;\n+\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tint fd;\ndiff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh\nnew file mode 100755\nindex 0000000..5e67179\n--- /dev/null\n+++ b/t/t4139-apply-escape.sh\n@@ -0,0 +1,137 @@\n+#!/bin/sh\n+\n+test_description='paths written by git-apply cannot escape the working tree'\n+. ./test-lib.sh\n+\n+# tests will try to write to ../foo, and we do not\n+# want them to escape the trash directory when they\n+# fail\n+test_expect_success 'bump git repo one level down' '\n+\tmkdir inside &&\n+\tmv .git inside/ &&\n+\tcd inside\n+'\n+\n+# $1 = name of file\n+# $2 = current path to file (if different)\n+mkpatch_add() {\n+\trm -f \"${2:-$1}\" &&\n+\tcat <<-EOF\n+\tdiff --git a/$1 b/$1\n+\tnew file mode 100644\n+\tindex 0000000..53c74cd\n+\t--- /dev/null\n+\t+++ b/$1\n+\t@@ -0,0 +1 @@\n+\t+evil\n+\tEOF\n+}\n+\n+mkpatch_del() {\n+\techo evil >\"${2:-$1}\" &&\n+\tcat <<-EOF\n+\tdiff --git a/$1 b/$1\n+\tdeleted file mode 100644\n+\tindex 53c74cd..0000000\n+\t--- a/$1\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-evil\n+\tEOF\n+}\n+\n+# $1 = name of file\n+# $2 = content of symlink\n+mkpatch_symlink() {\n+\trm -f \"$1\" &&\n+\tcat <<-EOF\n+\tdiff --git a/$1 b/$1\n+\tnew file mode 120000\n+\tindex 0000000..$(printf \"%s\" \"$2\" | git hash-object --stdin)\n+\t--- /dev/null\n+\t+++ b/$1\n+\t@@ -0,0 +1 @@\n+\t+$2\n+\t\\ No newline at end of file\n+\tEOF\n+}\n+\n+test_expect_success 'cannot add file containing ..' '\n+\tmkpatch_add ../foo >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'can add file containing .. with --unsafe-paths' '\n+\tmkpatch_add ../foo >patch &&\n+\tgit apply --unsafe-paths patch &&\n+\ttest_path_is_file ../foo\n+'\n+\n+test_expect_success 'cannot add file containing .. (index)' '\n+\tmkpatch_add ../foo >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'cannot add file containing .. with --unsafe-paths (index)' '\n+\tmkpatch_add ../foo >patch &&\n+\ttest_must_fail git apply --index --unsafe-paths patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'cannot del file containing ..' '\n+\tmkpatch_del ../foo >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_file ../foo\n+'\n+\n+test_expect_success 'can del file containing .. with --unsafe-paths' '\n+\tmkpatch_del ../foo >patch &&\n+\tgit apply --unsafe-paths patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'cannot del file containing .. (index)' '\n+\tmkpatch_del ../foo >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_file ../foo\n+'\n+\n+test_expect_failure 'symlink escape via ..' '\n+\t{\n+\t\tmkpatch_symlink tmp .. &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_failure 'symlink escape via .. (index)' '\n+\t{\n+\t\tmkpatch_symlink tmp .. &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_failure 'symlink escape via absolute path' '\n+\t{\n+\t\tmkpatch_symlink tmp \"$(pwd)\" &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'symlink escape via absolute path (index)' '\n+\t{\n+\t\tmkpatch_symlink tmp \"$(pwd)\" &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_done\n-- \n2.3.0-rc2-168-g106c876\n"},{"id":"255595","messageId":"1423010662-26497-3-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1423010662-26497-1-git-send-email-gitster@pobox.com","subject":"[PATCH v3 2/4] apply: do not read from the filesystem under --index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-04T00:44:20Z","receivedAt":"2015-02-04T00:44:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We currently read the preimage to apply a patch from the index only\nwhen the --cached option is given. Do so also when the command is\nrunning under the --index option. With --index, the index entry and\nthe working tree file for a path that is involved in a patch must be\nidentical, so this should not affect the result, but by reading from\nthe index, we will get the protection to avoid reading an unintended\npath beyond a symbolic link automatically.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Same as v2\n\n builtin/apply.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex c751ddf..05eaf54 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3136,7 +3136,7 @@ static int load_patch_target(struct strbuf *buf,\n \t\t\t const char *name,\n \t\t\t unsigned expected_mode)\n {\n-\tif (cached) {\n+\tif (cached || check_index) {\n \t\tif (read_file_or_gitlink(ce, buf))\n \t\t\treturn error(_(\"read of %s failed\"), name);\n \t} else if (name) {\n-- \n2.3.0-rc2-168-g106c876\n"},{"id":"255597","messageId":"1423010662-26497-4-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1423010662-26497-1-git-send-email-gitster@pobox.com","subject":"[PATCH v3 3/4] apply: do not read from beyond a symbolic link","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-04T00:44:21Z","receivedAt":"2015-02-04T00:44:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We should reject a patch, whether it renames/copies dir/file to\nelsewhere with or without modificiation, or updates dir/file in\nplace, if \"dir/\" part is actually a symbolic link to elsewhere,\nby making sure that the code to read the preimage does not read\nfrom a path that is beyond a symbolic link.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Same as v2\n\n builtin/apply.c | 2 ++\n t/t4122-apply-symlink-inside.sh | 19 +++++++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 05eaf54..60d821c 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3145,6 +3145,8 @@ static int load_patch_target(struct strbuf *buf,\n \t\t\t\treturn read_file_or_gitlink(ce, buf);\n \t\t\telse\n \t\t\t\treturn SUBMODULE_PATCH_WITHOUT_INDEX;\n+\t\t} else if (has_symlink_leading_path(name, strlen(name))) {\n+\t\t\treturn error(_(\"reading from '%s' beyond a symbolic link\"), name);\n \t\t} else {\n \t\t\tif (read_old_data(st, name, buf))\n \t\t\t\treturn error(_(\"read of %s failed\"), name);\ndiff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\nindex 70b3a06..035c080 100755\n--- a/t/t4122-apply-symlink-inside.sh\n+++ b/t/t4122-apply-symlink-inside.sh\n@@ -52,4 +52,23 @@ test_expect_success 'check result' '\n \n '\n \n+test_expect_success SYMLINKS 'do not read from beyond symbolic link' '\n+\tgit reset --hard &&\n+\tmkdir -p arch/x86_64/dir &&\n+\t>arch/x86_64/dir/file &&\n+\tgit add arch/x86_64/dir/file &&\n+\techo line >arch/x86_64/dir/file &&\n+\tgit diff >patch &&\n+\tgit reset --hard &&\n+\n+\tmkdir arch/i386/dir &&\n+\t>arch/i386/dir/file &&\n+\tln -s ../i386/dir arch/x86_64/dir &&\n+\n+\ttest_must_fail git apply patch &&\n+\ttest_must_fail git apply --cached patch &&\n+\ttest_must_fail git apply --index patch\n+\n+'\n+\n test_done\n-- \n2.3.0-rc2-168-g106c876\n"},{"id":"255598","messageId":"1423010662-26497-5-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1423010662-26497-1-git-send-email-gitster@pobox.com","subject":"[PATCH v3 4/4] apply: do not touch a file beyond a symbolic link","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-04T00:44:22Z","receivedAt":"2015-02-04T00:44:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Because Git tracks symbolic links as symbolic links, a path that\nhas a symbolic link in its leading part (e.g. path/to/dir/file,\nwhere path/to/dir is a symbolic link to somewhere else, be it\ninside or outside the working tree) can never appear in a patch\nthat validly applies, unless the same patch first removes the\nsymbolic link to allow a directory to be created there.\n\nDetect and reject such a patch.\n\nThings to note:\n\n - Unfortunately, we cannot reuse the has_symlink_leading_path()\n from dir.c, as that is only about the working tree, but \"git\n apply\" can be told to apply the patch only to the index or to\n both the index and to the working tree.\n\n - We cannot directly use has_symlink_leading_path() even when we\n are applying only to the working tree, as an early patch of a\n valid input may remove a symbolic link path/to/dir and then a\n later patch of the input may create a path path/to/dir/file, but\n \"git apply\" first checks the input without touching either the\n index or the working tree. The leading symbolic link check must\n be done on the interim result we compute in-core (i.e. after the\n first patch, there is no path/to/dir symbolic link and it is\n perfectly valid to create path/to/dir/file).\n\n Similarly, when an input creates a symbolic link path/to/dir and\n then creates a file path/to/dir/file, we need to flag it as an\n error without actually creating path/to/dir symbolic link in the\n filesystem.\n\nInstead, for any patch in the input that leaves a path (i.e. a non\ndeletion) in the result, we check all leading paths against the\nresulting tree that the patch would create by inspecting all the\npatches in the input and then the target of patch application\n(either the index or the working tree).\n\nThis way, we catch a mischief or a mistake to add a symbolic link\npath/to/dir and a file path/to/dir/file at the same time, while\nallowing a valid patch that removes a symbolic link path/to/dir and\nthen adds a file path/to/dir/file.\n\nSigned-off-by: Junio C Hamano \n---\n\n * In code comment to check_patch() rephrased to clarify why we\n don't have to check a is_delete patch there while we have to\n check others.\n\n An additional test case stolen from Peff.\n\n builtin/apply.c | 111 ++++++++++++++++++++++++++++++++++++++++\n t/t4122-apply-symlink-inside.sh | 87 +++++++++++++++++++++++++++++++\n t/t4139-apply-escape.sh | 6 +--\n 3 files changed, 201 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 60d821c..28975e6 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3486,6 +3486,103 @@ static int check_to_create(const char *new_name, int ok_if_exists)\n \treturn 0;\n }\n \n+/*\n+ * We need to keep track of how symlinks in the preimage are\n+ * manipulated by the patches. A patch to add a/b/c where a/b\n+ * is a symlink should not be allowed to affect the directory\n+ * the symlink points at, but if the same patch removes a/b,\n+ * it is perfectly fine, as the patch removes a/b to make room\n+ * to create a directory a/b so that a/b/c can be created.\n+ */\n+static struct string_list symlink_changes;\n+#define SYMLINK_GOES_AWAY 01\n+#define SYMLINK_IN_RESULT 02\n+\n+static uintptr_t register_symlink_changes(const char *path, uintptr_t what)\n+{\n+\tstruct string_list_item *ent;\n+\n+\tent = string_list_lookup(&symlink_changes, path);\n+\tif (!ent) {\n+\t\tent = string_list_insert(&symlink_changes, path);\n+\t\tent->util = (void *)0;\n+\t}\n+\tent->util = (void *)(what | ((uintptr_t)ent->util));\n+\treturn (uintptr_t)ent->util;\n+}\n+\n+static uintptr_t check_symlink_changes(const char *path)\n+{\n+\tstruct string_list_item *ent;\n+\n+\tent = string_list_lookup(&symlink_changes, path);\n+\tif (!ent)\n+\t\treturn 0;\n+\treturn (uintptr_t)ent->util;\n+}\n+\n+static void prepare_symlink_changes(struct patch *patch)\n+{\n+\tfor ( ; patch; patch = patch->next) {\n+\t\tif ((patch->old_name && S_ISLNK(patch->old_mode)) &&\n+\t\t (patch->is_rename || patch->is_delete))\n+\t\t\t/* the symlink at patch->old_name is removed */\n+\t\t\tregister_symlink_changes(patch->old_name, SYMLINK_GOES_AWAY);\n+\n+\t\tif (patch->new_name && S_ISLNK(patch->new_mode))\n+\t\t\t/* the symlink at patch->new_name is created or remains */\n+\t\t\tregister_symlink_changes(patch->new_name, SYMLINK_IN_RESULT);\n+\t}\n+}\n+\n+static int path_is_beyond_symlink_1(struct strbuf *name)\n+{\n+\tdo {\n+\t\tunsigned int change;\n+\n+\t\twhile (--name->len && name->buf[name->len] != '/')\n+\t\t\t; /* scan backwards */\n+\t\tif (!name->len)\n+\t\t\tbreak;\n+\t\tname->buf[name->len] = '\\0';\n+\t\tchange = check_symlink_changes(name->buf);\n+\t\tif (change & SYMLINK_IN_RESULT)\n+\t\t\treturn 1;\n+\t\tif (change & SYMLINK_GOES_AWAY)\n+\t\t\t/*\n+\t\t\t * This cannot be \"return 0\", because we may\n+\t\t\t * see a new one created at a higher level.\n+\t\t\t */\n+\t\t\tcontinue;\n+\n+\t\t/* otherwise, check the preimage */\n+\t\tif (check_index) {\n+\t\t\tint pos = cache_name_pos(name->buf, name->len);\n+\t\t\tif (0 <= pos &&\n+\t\t\t S_ISLNK(active_cache[pos]->ce_mode))\n+\t\t\t\treturn 1;\n+\t\t} else {\n+\t\t\tstruct stat st;\n+\t\t\tif (!lstat(name->buf, &st) && S_ISLNK(st.st_mode))\n+\t\t\t\treturn 1;\n+\t\t}\n+\t} while (1);\n+\treturn 0;\n+}\n+\n+static int path_is_beyond_symlink(const char *name_)\n+{\n+\tint ret;\n+\tstruct strbuf name = STRBUF_INIT;\n+\n+\tassert(*name_ != '\\0');\n+\tstrbuf_addstr(&name, name_);\n+\tret = path_is_beyond_symlink_1(&name);\n+\tstrbuf_release(&name);\n+\n+\treturn ret;\n+}\n+\n static void die_on_unsafe_path(struct patch *patch)\n {\n \tconst char *old_name = NULL;\n@@ -3593,6 +3690,19 @@ static int check_patch(struct patch *patch)\n \tif (!unsafe_paths)\n \t\tdie_on_unsafe_path(patch);\n \n+\t/*\n+\t * An attempt to read from or delete a path that is beyond a\n+\t * symbolic link will be prevented by load_patch_target() that\n+\t * is called at the beginning of apply_data() so we do not\n+\t * have to worry about a patch marked with \"is_delete\" bit\n+\t * here. We however need to make sure that the patch result\n+\t * is not deposited to a path that is beyond a symbolic link\n+\t * here.\n+\t */\n+\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n+\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n+\t\t\t patch->new_name);\n+\n \tif (apply_data(patch, &st, ce) < 0)\n \t\treturn error(_(\"%s: patch does not apply\"), name);\n \tpatch->rejected = 0;\n@@ -3603,6 +3713,7 @@ static int check_patch_list(struct patch *patch)\n {\n \tint err = 0;\n \n+\tprepare_symlink_changes(patch);\n \tprepare_fn_table(patch);\n \twhile (patch) {\n \t\tif (apply_verbosely)\ndiff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\nindex 035c080..1779c0a 100755\n--- a/t/t4122-apply-symlink-inside.sh\n+++ b/t/t4122-apply-symlink-inside.sh\n@@ -71,4 +71,91 @@ test_expect_success SYMLINKS 'do not read from beyond symbolic link' '\n \n '\n \n+test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '\n+\n+\trm -rf arch/i386/dir arch/x86_64/dir &&\n+\tgit reset --hard &&\n+\tln -s ../i386/dir arch/x86_64/dir &&\n+\tgit add arch/x86_64/dir &&\n+\tgit diff HEAD >add_symlink.patch &&\n+\tgit reset --hard &&\n+\n+\tmkdir arch/x86_64/dir &&\n+\t>arch/x86_64/dir/file &&\n+\tgit add arch/x86_64/dir/file &&\n+\tgit diff HEAD >add_file.patch &&\n+\tgit diff -R HEAD >del_file.patch &&\n+\tgit reset --hard &&\n+\trm -fr arch/x86_64/dir &&\n+\n+\tcat add_symlink.patch add_file.patch >patch &&\n+\tcat add_symlink.patch del_file.patch >tricky_del &&\n+\n+\tmkdir arch/i386/dir\n+'\n+\n+test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '\n+\n+\t# same input creates a confusing symbolic link\n+\ttest_must_fail git apply patch 2>error-wt &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-wt &&\n+\ttest_path_is_missing arch/x86_64/dir &&\n+\ttest_path_is_missing arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply --index patch 2>error-ix &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ix &&\n+\ttest_path_is_missing arch/x86_64/dir &&\n+\ttest_path_is_missing arch/i386/dir/file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir &&\n+\n+\ttest_must_fail git apply --cached patch 2>error-ct &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ct &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir &&\n+\n+\t>arch/i386/dir/file &&\n+\tgit add arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply tricky_del &&\n+\ttest_path_is_file arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply --index tricky_del &&\n+\ttest_path_is_file arch/i386/dir/file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\tgit ls-files --error-unmatch arch/i386/dir &&\n+\n+\ttest_must_fail git apply --cached tricky_del &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\tgit ls-files --error-unmatch arch/i386/dir\n+'\n+\n+test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '\n+\n+\t# existing symbolic link\n+\tgit reset --hard &&\n+\tln -s ../i386/dir arch/x86_64/dir &&\n+\tgit add arch/x86_64/dir &&\n+\n+\ttest_must_fail git apply add_file.patch 2>error-wt-add &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-wt-add &&\n+\ttest_path_is_missing arch/i386/dir/file &&\n+\n+\tmkdir arch/i386/dir &&\n+\t>arch/i386/dir/file &&\n+\ttest_must_fail git apply del_file.patch 2>error-wt-del &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-wt-del &&\n+\ttest_path_is_file arch/i386/dir/file &&\n+\trm arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply --index add_file.patch 2>error-ix-add &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ix-add &&\n+\ttest_path_is_missing arch/i386/dir/file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir &&\n+\n+\ttest_must_fail git apply --cached add_file.patch 2>error-ct-file &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ct-file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir\n+'\n+\n test_done\ndiff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh\nindex 5e67179..25917cc 100755\n--- a/t/t4139-apply-escape.sh\n+++ b/t/t4139-apply-escape.sh\n@@ -98,7 +98,7 @@ test_expect_success 'cannot del file containing .. (index)' '\n \ttest_path_is_file ../foo\n '\n \n-test_expect_failure 'symlink escape via ..' '\n+test_expect_success 'symlink escape via ..' '\n \t{\n \t\tmkpatch_symlink tmp .. &&\n \t\tmkpatch_add tmp/foo ../foo\n@@ -107,7 +107,7 @@ test_expect_failure 'symlink escape via ..' '\n \ttest_path_is_missing ../foo\n '\n \n-test_expect_failure 'symlink escape via .. (index)' '\n+test_expect_success 'symlink escape via .. (index)' '\n \t{\n \t\tmkpatch_symlink tmp .. &&\n \t\tmkpatch_add tmp/foo ../foo\n@@ -116,7 +116,7 @@ test_expect_failure 'symlink escape via .. (index)' '\n \ttest_path_is_missing ../foo\n '\n \n-test_expect_failure 'symlink escape via absolute path' '\n+test_expect_success 'symlink escape via absolute path' '\n \t{\n \t\tmkpatch_symlink tmp \"$(pwd)\" &&\n \t\tmkpatch_add tmp/foo ../foo\n-- \n2.3.0-rc2-168-g106c876\n"},{"id":"255904","messageId":"1423607771-27157-1-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1423010662-26497-1-git-send-email-gitster@pobox.com","subject":"[PATCH v4 0/4] \"git apply\" safety","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-10T22:36:07Z","receivedAt":"2015-02-10T22:36:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Git tracks symbolic links; e.g. you can remove files that have been\ntracked in a directory \"dir/file*\" and then creates a symbolic link\nat \"dir\" to point elsewhere, express such a change as a patchset and\nthen apply it to the original tree. Consequently, applying a patch\nto update dir/file, when you have \"dir\" as a symbolic link pointing\nsomewhere, must fail, just like a patch whose preimage does not\nmatch what you have in tree you are trying to apply the patch to\ngets rejected. Also, we fundamentally do not like to touch a path\nthat contains \".git\" as a path component.\n\nThis round uses cache_file_exists() in the last patch to cope with\ncase insensitive filesystems better.\n\nThe previous round begins here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/263341\n\nJunio C Hamano (4):\n apply: reject input that touches outside the working area\n apply: do not read from the filesystem under --index\n apply: do not read from beyond a symbolic link\n apply: do not touch a file beyond a symbolic link\n\n Documentation/git-apply.txt | 12 +++-\n builtin/apply.c | 142 +++++++++++++++++++++++++++++++++++++++-\n t/t4122-apply-symlink-inside.sh | 106 ++++++++++++++++++++++++++++++\n t/t4139-apply-escape.sh | 141 +++++++++++++++++++++++++++++++++++++++\n 4 files changed, 399 insertions(+), 2 deletions(-)\n create mode 100755 t/t4139-apply-escape.sh\n\n-- \n2.3.0-185-g073f588\n"},{"id":"255903","messageId":"1423607771-27157-2-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1423607771-27157-1-git-send-email-gitster@pobox.com","subject":"[PATCH v4 1/4] apply: reject input that touches outside the working area","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-10T22:36:08Z","receivedAt":"2015-02-10T22:36:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"By default, a patch that affects outside the working area (either a\nGit controlled working tree, or the current working directory when\n\"git apply\" is used as a replacement of GNU patch) is rejected as a\nmistake (or a mischief). Git itself does not create such a patch,\nunless the user bends over backwards and specifies a non-standard\nprefix to \"git diff\" and friends.\n\nWhen `git apply` is used as a \"better GNU patch\", the user can pass\nthe `--unsafe-paths` option to override this safety check. This\noption has no effect when `--index` or `--cached` is in use.\n\nThe new test was stolen from Jeff King with slight enhancements.\nNote that a few new tests for touching outside the working area by\nfollowing a symbolic link are still expected to fail at this step,\nbut will be fixed in later steps.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-apply.txt | 12 +++-\n builtin/apply.c | 26 ++++++++\n t/t4139-apply-escape.sh | 141 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 178 insertions(+), 1 deletion(-)\n create mode 100755 t/t4139-apply-escape.sh\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex f605327..9489664 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -16,7 +16,7 @@ SYNOPSIS\n \t [--ignore-space-change | --ignore-whitespace ]\n \t [--whitespace=(nowarn|warn|fix|error|error-all)]\n \t [--exclude=] [--include=] [--directory=]\n-\t [--verbose] [...]\n+\t [--verbose] [--unsafe-paths] [...]\n \n DESCRIPTION\n -----------\n@@ -229,6 +229,16 @@ For example, a patch that talks about updating `a/git-gui.sh` to `b/git-gui.sh`\n can be applied to the file in the working tree `modules/git-gui/git-gui.sh` by\n running `git apply --directory=modules/git-gui`.\n \n+--unsafe-paths::\n+\tBy default, a patch that affects outside the working area\n+\t(either a Git controlled working tree, or the current working\n+\tdirectory when \"git apply\" is used as a replacement of GNU\n+\tpatch) is rejected as a mistake (or a mischief).\n++\n+When `git apply` is used as a \"better GNU patch\", the user can pass\n+the `--unsafe-paths` option to override this safety check. This option\n+has no effect when `--index` or `--cached` is in use.\n+\n Configuration\n -------------\n \ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex ef32e4f..8561236 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -50,6 +50,7 @@ static int apply_verbosely;\n static int allow_overlap;\n static int no_add;\n static int threeway;\n+static int unsafe_paths;\n static const char *fake_ancestor;\n static int line_termination = '\\n';\n static unsigned int p_context = UINT_MAX;\n@@ -3483,6 +3484,23 @@ static int check_to_create(const char *new_name, int ok_if_exists)\n \treturn 0;\n }\n \n+static void die_on_unsafe_path(struct patch *patch)\n+{\n+\tconst char *old_name = NULL;\n+\tconst char *new_name = NULL;\n+\tif (patch->is_delete)\n+\t\told_name = patch->old_name;\n+\telse if (!patch->is_new && !patch->is_copy)\n+\t\told_name = patch->old_name;\n+\tif (!patch->is_delete)\n+\t\tnew_name = patch->new_name;\n+\n+\tif (old_name && !verify_path(old_name))\n+\t\tdie(_(\"invalid path '%s'\"), old_name);\n+\tif (new_name && !verify_path(new_name))\n+\t\tdie(_(\"invalid path '%s'\"), new_name);\n+}\n+\n /*\n * Check and apply the patch in-core; leave the result in patch->result\n * for the caller to write it out to the final destination.\n@@ -3570,6 +3588,9 @@ static int check_patch(struct patch *patch)\n \t\t}\n \t}\n \n+\tif (!unsafe_paths)\n+\t\tdie_on_unsafe_path(patch);\n+\n \tif (apply_data(patch, &st, ce) < 0)\n \t\treturn error(_(\"%s: patch does not apply\"), name);\n \tpatch->rejected = 0;\n@@ -4379,6 +4400,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)\n \t\t\tN_(\"make sure the patch is applicable to the current index\")),\n \t\tOPT_BOOL(0, \"cached\", &cached,\n \t\t\tN_(\"apply a patch without touching the working tree\")),\n+\t\tOPT_BOOL(0, \"unsafe-paths\", &unsafe_paths,\n+\t\t\tN_(\"accept a patch that touches outside the working area\")),\n \t\tOPT_BOOL(0, \"apply\", &force_apply,\n \t\t\tN_(\"also apply the patch (use with --stat/--summary/--check)\")),\n \t\tOPT_BOOL('3', \"3way\", &threeway,\n@@ -4451,6 +4474,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)\n \t\t\tdie(_(\"--cached outside a repository\"));\n \t\tcheck_index = 1;\n \t}\n+\tif (check_index)\n+\t\tunsafe_paths = 0;\n+\n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tint fd;\ndiff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh\nnew file mode 100755\nindex 0000000..02b97b3\n--- /dev/null\n+++ b/t/t4139-apply-escape.sh\n@@ -0,0 +1,141 @@\n+#!/bin/sh\n+\n+test_description='paths written by git-apply cannot escape the working tree'\n+. ./test-lib.sh\n+\n+# tests will try to write to ../foo, and we do not\n+# want them to escape the trash directory when they\n+# fail\n+test_expect_success 'bump git repo one level down' '\n+\tmkdir inside &&\n+\tmv .git inside/ &&\n+\tcd inside\n+'\n+\n+# $1 = name of file\n+# $2 = current path to file (if different)\n+mkpatch_add () {\n+\trm -f \"${2:-$1}\" &&\n+\tcat <<-EOF\n+\tdiff --git a/$1 b/$1\n+\tnew file mode 100644\n+\tindex 0000000..53c74cd\n+\t--- /dev/null\n+\t+++ b/$1\n+\t@@ -0,0 +1 @@\n+\t+evil\n+\tEOF\n+}\n+\n+mkpatch_del () {\n+\techo evil >\"${2:-$1}\" &&\n+\tcat <<-EOF\n+\tdiff --git a/$1 b/$1\n+\tdeleted file mode 100644\n+\tindex 53c74cd..0000000\n+\t--- a/$1\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-evil\n+\tEOF\n+}\n+\n+# $1 = name of file\n+# $2 = content of symlink\n+mkpatch_symlink () {\n+\trm -f \"$1\" &&\n+\tcat <<-EOF\n+\tdiff --git a/$1 b/$1\n+\tnew file mode 120000\n+\tindex 0000000..$(printf \"%s\" \"$2\" | git hash-object --stdin)\n+\t--- /dev/null\n+\t+++ b/$1\n+\t@@ -0,0 +1 @@\n+\t+$2\n+\t\\ No newline at end of file\n+\tEOF\n+}\n+\n+test_expect_success 'cannot create file containing ..' '\n+\tmkpatch_add ../foo >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'can create file containing .. with --unsafe-paths' '\n+\tmkpatch_add ../foo >patch &&\n+\tgit apply --unsafe-paths patch &&\n+\ttest_path_is_file ../foo\n+'\n+\n+test_expect_success 'cannot create file containing .. (index)' '\n+\tmkpatch_add ../foo >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'cannot create file containing .. with --unsafe-paths (index)' '\n+\tmkpatch_add ../foo >patch &&\n+\ttest_must_fail git apply --index --unsafe-paths patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'cannot delete file containing ..' '\n+\tmkpatch_del ../foo >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_file ../foo\n+'\n+\n+test_expect_success 'can delete file containing .. with --unsafe-paths' '\n+\tmkpatch_del ../foo >patch &&\n+\tgit apply --unsafe-paths patch &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_success 'cannot delete file containing .. (index)' '\n+\tmkpatch_del ../foo >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_file ../foo\n+'\n+\n+test_expect_failure SYMLINKS 'symlink escape via ..' '\n+\t{\n+\t\tmkpatch_symlink tmp .. &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_missing tmp &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_failure SYMLINKS 'symlink escape via .. (index)' '\n+\t{\n+\t\tmkpatch_symlink tmp .. &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_missing tmp &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_failure SYMLINKS 'symlink escape via absolute path' '\n+\t{\n+\t\tmkpatch_symlink tmp \"$(pwd)\" &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply patch &&\n+\ttest_path_is_missing tmp &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_expect_failure SYMLINKS 'symlink escape via absolute path (index)' '\n+\t{\n+\t\tmkpatch_symlink tmp \"$(pwd)\" &&\n+\t\tmkpatch_add tmp/foo ../foo\n+\t} >patch &&\n+\ttest_must_fail git apply --index patch &&\n+\ttest_path_is_missing tmp &&\n+\ttest_path_is_missing ../foo\n+'\n+\n+test_done\n-- \n2.3.0-185-g073f588\n"},{"id":"255906","messageId":"1423607771-27157-3-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1423607771-27157-1-git-send-email-gitster@pobox.com","subject":"[PATCH v4 2/4] apply: do not read from the filesystem under --index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-10T22:36:09Z","receivedAt":"2015-02-10T22:36:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We currently read the preimage to apply a patch from the index only\nwhen the --cached option is given. Do so also when the command is\nrunning under the --index option. With --index, the index entry and\nthe working tree file for a path that is involved in a patch must be\nidentical, so this should not affect the result, but by reading from\nthe index, we will get the protection to avoid reading an unintended\npath beyond a symbolic link automatically.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/apply.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 8561236..21e45a0 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3136,7 +3136,7 @@ static int load_patch_target(struct strbuf *buf,\n \t\t\t const char *name,\n \t\t\t unsigned expected_mode)\n {\n-\tif (cached) {\n+\tif (cached || check_index) {\n \t\tif (read_file_or_gitlink(ce, buf))\n \t\t\treturn error(_(\"read of %s failed\"), name);\n \t} else if (name) {\n-- \n2.3.0-185-g073f588\n"},{"id":"255907","messageId":"1423607771-27157-4-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1423607771-27157-1-git-send-email-gitster@pobox.com","subject":"[PATCH v4 3/4] apply: do not read from beyond a symbolic link","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-10T22:36:10Z","receivedAt":"2015-02-10T22:36:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We should reject a patch, whether it renames/copies dir/file to\nelsewhere with or without modificiation, or updates dir/file in\nplace, if \"dir/\" part is actually a symbolic link to elsewhere,\nby making sure that the code to read the preimage does not read\nfrom a path that is beyond a symbolic link.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/apply.c | 2 ++\n t/t4122-apply-symlink-inside.sh | 19 +++++++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 21e45a0..422e4ce 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3145,6 +3145,8 @@ static int load_patch_target(struct strbuf *buf,\n \t\t\t\treturn read_file_or_gitlink(ce, buf);\n \t\t\telse\n \t\t\t\treturn SUBMODULE_PATCH_WITHOUT_INDEX;\n+\t\t} else if (has_symlink_leading_path(name, strlen(name))) {\n+\t\t\treturn error(_(\"reading from '%s' beyond a symbolic link\"), name);\n \t\t} else {\n \t\t\tif (read_old_data(st, name, buf))\n \t\t\t\treturn error(_(\"read of %s failed\"), name);\ndiff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\nindex 70b3a06..035c080 100755\n--- a/t/t4122-apply-symlink-inside.sh\n+++ b/t/t4122-apply-symlink-inside.sh\n@@ -52,4 +52,23 @@ test_expect_success 'check result' '\n \n '\n \n+test_expect_success SYMLINKS 'do not read from beyond symbolic link' '\n+\tgit reset --hard &&\n+\tmkdir -p arch/x86_64/dir &&\n+\t>arch/x86_64/dir/file &&\n+\tgit add arch/x86_64/dir/file &&\n+\techo line >arch/x86_64/dir/file &&\n+\tgit diff >patch &&\n+\tgit reset --hard &&\n+\n+\tmkdir arch/i386/dir &&\n+\t>arch/i386/dir/file &&\n+\tln -s ../i386/dir arch/x86_64/dir &&\n+\n+\ttest_must_fail git apply patch &&\n+\ttest_must_fail git apply --cached patch &&\n+\ttest_must_fail git apply --index patch\n+\n+'\n+\n test_done\n-- \n2.3.0-185-g073f588\n"},{"id":"255905","messageId":"1423607771-27157-5-git-send-email-gitster@pobox.com","threadId":"38488","inReplyTo":"1423607771-27157-1-git-send-email-gitster@pobox.com","subject":"[PATCH v4 4/4] apply: do not touch a file beyond a symbolic link","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-10T22:36:11Z","receivedAt":"2015-02-10T22:36:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Because Git tracks symbolic links as symbolic links, a path that\nhas a symbolic link in its leading part (e.g. path/to/dir/file,\nwhere path/to/dir is a symbolic link to somewhere else, be it\ninside or outside the working tree) can never appear in a patch\nthat validly applies, unless the same patch first removes the\nsymbolic link to allow a directory to be created there.\n\nDetect and reject such a patch.\n\nThings to note:\n\n - Unfortunately, we cannot reuse the has_symlink_leading_path()\n from dir.c, as that is only about the working tree, but \"git\n apply\" can be told to apply the patch only to the index or to\n both the index and to the working tree.\n\n - We cannot directly use has_symlink_leading_path() even when we\n are applying only to the working tree, as an early patch of a\n valid input may remove a symbolic link path/to/dir and then a\n later patch of the input may create a path path/to/dir/file, but\n \"git apply\" first checks the input without touching either the\n index or the working tree. The leading symbolic link check must\n be done on the interim result we compute in-core (i.e. after the\n first patch, there is no path/to/dir symbolic link and it is\n perfectly valid to create path/to/dir/file).\n\n Similarly, when an input creates a symbolic link path/to/dir and\n then creates a file path/to/dir/file, we need to flag it as an\n error without actually creating path/to/dir symbolic link in the\n filesystem.\n\nInstead, for any patch in the input that leaves a path (i.e. a non\ndeletion) in the result, we check all leading paths against the\nresulting tree that the patch would create by inspecting all the\npatches in the input and then the target of patch application\n(either the index or the working tree).\n\nThis way, we catch a mischief or a mistake to add a symbolic link\npath/to/dir and a file path/to/dir/file at the same time, while\nallowing a valid patch that removes a symbolic link path/to/dir and\nthen adds a file path/to/dir/file.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/apply.c | 112 ++++++++++++++++++++++++++++++++++++++++\n t/t4122-apply-symlink-inside.sh | 87 +++++++++++++++++++++++++++++++\n t/t4139-apply-escape.sh | 8 +--\n 3 files changed, 203 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 422e4ce..c2c6fda 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3486,6 +3486,104 @@ static int check_to_create(const char *new_name, int ok_if_exists)\n \treturn 0;\n }\n \n+/*\n+ * We need to keep track of how symlinks in the preimage are\n+ * manipulated by the patches. A patch to add a/b/c where a/b\n+ * is a symlink should not be allowed to affect the directory\n+ * the symlink points at, but if the same patch removes a/b,\n+ * it is perfectly fine, as the patch removes a/b to make room\n+ * to create a directory a/b so that a/b/c can be created.\n+ */\n+static struct string_list symlink_changes;\n+#define SYMLINK_GOES_AWAY 01\n+#define SYMLINK_IN_RESULT 02\n+\n+static uintptr_t register_symlink_changes(const char *path, uintptr_t what)\n+{\n+\tstruct string_list_item *ent;\n+\n+\tent = string_list_lookup(&symlink_changes, path);\n+\tif (!ent) {\n+\t\tent = string_list_insert(&symlink_changes, path);\n+\t\tent->util = (void *)0;\n+\t}\n+\tent->util = (void *)(what | ((uintptr_t)ent->util));\n+\treturn (uintptr_t)ent->util;\n+}\n+\n+static uintptr_t check_symlink_changes(const char *path)\n+{\n+\tstruct string_list_item *ent;\n+\n+\tent = string_list_lookup(&symlink_changes, path);\n+\tif (!ent)\n+\t\treturn 0;\n+\treturn (uintptr_t)ent->util;\n+}\n+\n+static void prepare_symlink_changes(struct patch *patch)\n+{\n+\tfor ( ; patch; patch = patch->next) {\n+\t\tif ((patch->old_name && S_ISLNK(patch->old_mode)) &&\n+\t\t (patch->is_rename || patch->is_delete))\n+\t\t\t/* the symlink at patch->old_name is removed */\n+\t\t\tregister_symlink_changes(patch->old_name, SYMLINK_GOES_AWAY);\n+\n+\t\tif (patch->new_name && S_ISLNK(patch->new_mode))\n+\t\t\t/* the symlink at patch->new_name is created or remains */\n+\t\t\tregister_symlink_changes(patch->new_name, SYMLINK_IN_RESULT);\n+\t}\n+}\n+\n+static int path_is_beyond_symlink_1(struct strbuf *name)\n+{\n+\tdo {\n+\t\tunsigned int change;\n+\n+\t\twhile (--name->len && name->buf[name->len] != '/')\n+\t\t\t; /* scan backwards */\n+\t\tif (!name->len)\n+\t\t\tbreak;\n+\t\tname->buf[name->len] = '\\0';\n+\t\tchange = check_symlink_changes(name->buf);\n+\t\tif (change & SYMLINK_IN_RESULT)\n+\t\t\treturn 1;\n+\t\tif (change & SYMLINK_GOES_AWAY)\n+\t\t\t/*\n+\t\t\t * This cannot be \"return 0\", because we may\n+\t\t\t * see a new one created at a higher level.\n+\t\t\t */\n+\t\t\tcontinue;\n+\n+\t\t/* otherwise, check the preimage */\n+\t\tif (check_index) {\n+\t\t\tstruct cache_entry *ce;\n+\n+\t\t\tce = cache_file_exists(name->buf, name->len, ignore_case);\n+\t\t\tif (ce && S_ISLNK(ce->ce_mode))\n+\t\t\t\treturn 1;\n+\t\t} else {\n+\t\t\tstruct stat st;\n+\t\t\tif (!lstat(name->buf, &st) && S_ISLNK(st.st_mode))\n+\t\t\t\treturn 1;\n+\t\t}\n+\t} while (1);\n+\treturn 0;\n+}\n+\n+static int path_is_beyond_symlink(const char *name_)\n+{\n+\tint ret;\n+\tstruct strbuf name = STRBUF_INIT;\n+\n+\tassert(*name_ != '\\0');\n+\tstrbuf_addstr(&name, name_);\n+\tret = path_is_beyond_symlink_1(&name);\n+\tstrbuf_release(&name);\n+\n+\treturn ret;\n+}\n+\n static void die_on_unsafe_path(struct patch *patch)\n {\n \tconst char *old_name = NULL;\n@@ -3593,6 +3691,19 @@ static int check_patch(struct patch *patch)\n \tif (!unsafe_paths)\n \t\tdie_on_unsafe_path(patch);\n \n+\t/*\n+\t * An attempt to read from or delete a path that is beyond a\n+\t * symbolic link will be prevented by load_patch_target() that\n+\t * is called at the beginning of apply_data() so we do not\n+\t * have to worry about a patch marked with \"is_delete\" bit\n+\t * here. We however need to make sure that the patch result\n+\t * is not deposited to a path that is beyond a symbolic link\n+\t * here.\n+\t */\n+\tif (!patch->is_delete && path_is_beyond_symlink(patch->new_name))\n+\t\treturn error(_(\"affected file '%s' is beyond a symbolic link\"),\n+\t\t\t patch->new_name);\n+\n \tif (apply_data(patch, &st, ce) < 0)\n \t\treturn error(_(\"%s: patch does not apply\"), name);\n \tpatch->rejected = 0;\n@@ -3603,6 +3714,7 @@ static int check_patch_list(struct patch *patch)\n {\n \tint err = 0;\n \n+\tprepare_symlink_changes(patch);\n \tprepare_fn_table(patch);\n \twhile (patch) {\n \t\tif (apply_verbosely)\ndiff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh\nindex 035c080..1779c0a 100755\n--- a/t/t4122-apply-symlink-inside.sh\n+++ b/t/t4122-apply-symlink-inside.sh\n@@ -71,4 +71,91 @@ test_expect_success SYMLINKS 'do not read from beyond symbolic link' '\n \n '\n \n+test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '\n+\n+\trm -rf arch/i386/dir arch/x86_64/dir &&\n+\tgit reset --hard &&\n+\tln -s ../i386/dir arch/x86_64/dir &&\n+\tgit add arch/x86_64/dir &&\n+\tgit diff HEAD >add_symlink.patch &&\n+\tgit reset --hard &&\n+\n+\tmkdir arch/x86_64/dir &&\n+\t>arch/x86_64/dir/file &&\n+\tgit add arch/x86_64/dir/file &&\n+\tgit diff HEAD >add_file.patch &&\n+\tgit diff -R HEAD >del_file.patch &&\n+\tgit reset --hard &&\n+\trm -fr arch/x86_64/dir &&\n+\n+\tcat add_symlink.patch add_file.patch >patch &&\n+\tcat add_symlink.patch del_file.patch >tricky_del &&\n+\n+\tmkdir arch/i386/dir\n+'\n+\n+test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '\n+\n+\t# same input creates a confusing symbolic link\n+\ttest_must_fail git apply patch 2>error-wt &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-wt &&\n+\ttest_path_is_missing arch/x86_64/dir &&\n+\ttest_path_is_missing arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply --index patch 2>error-ix &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ix &&\n+\ttest_path_is_missing arch/x86_64/dir &&\n+\ttest_path_is_missing arch/i386/dir/file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir &&\n+\n+\ttest_must_fail git apply --cached patch 2>error-ct &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ct &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir &&\n+\n+\t>arch/i386/dir/file &&\n+\tgit add arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply tricky_del &&\n+\ttest_path_is_file arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply --index tricky_del &&\n+\ttest_path_is_file arch/i386/dir/file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\tgit ls-files --error-unmatch arch/i386/dir &&\n+\n+\ttest_must_fail git apply --cached tricky_del &&\n+\ttest_must_fail git ls-files --error-unmatch arch/x86_64/dir &&\n+\tgit ls-files --error-unmatch arch/i386/dir\n+'\n+\n+test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '\n+\n+\t# existing symbolic link\n+\tgit reset --hard &&\n+\tln -s ../i386/dir arch/x86_64/dir &&\n+\tgit add arch/x86_64/dir &&\n+\n+\ttest_must_fail git apply add_file.patch 2>error-wt-add &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-wt-add &&\n+\ttest_path_is_missing arch/i386/dir/file &&\n+\n+\tmkdir arch/i386/dir &&\n+\t>arch/i386/dir/file &&\n+\ttest_must_fail git apply del_file.patch 2>error-wt-del &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-wt-del &&\n+\ttest_path_is_file arch/i386/dir/file &&\n+\trm arch/i386/dir/file &&\n+\n+\ttest_must_fail git apply --index add_file.patch 2>error-ix-add &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ix-add &&\n+\ttest_path_is_missing arch/i386/dir/file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir &&\n+\n+\ttest_must_fail git apply --cached add_file.patch 2>error-ct-file &&\n+\ttest_i18ngrep \"beyond a symbolic link\" error-ct-file &&\n+\ttest_must_fail git ls-files --error-unmatch arch/i386/dir\n+'\n+\n test_done\ndiff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh\nindex 02b97b3..45b5660 100755\n--- a/t/t4139-apply-escape.sh\n+++ b/t/t4139-apply-escape.sh\n@@ -98,7 +98,7 @@ test_expect_success 'cannot delete file containing .. (index)' '\n \ttest_path_is_file ../foo\n '\n \n-test_expect_failure SYMLINKS 'symlink escape via ..' '\n+test_expect_success SYMLINKS 'symlink escape via ..' '\n \t{\n \t\tmkpatch_symlink tmp .. &&\n \t\tmkpatch_add tmp/foo ../foo\n@@ -108,7 +108,7 @@ test_expect_failure SYMLINKS 'symlink escape via ..' '\n \ttest_path_is_missing ../foo\n '\n \n-test_expect_failure SYMLINKS 'symlink escape via .. (index)' '\n+test_expect_success SYMLINKS 'symlink escape via .. (index)' '\n \t{\n \t\tmkpatch_symlink tmp .. &&\n \t\tmkpatch_add tmp/foo ../foo\n@@ -118,7 +118,7 @@ test_expect_failure SYMLINKS 'symlink escape via .. (index)' '\n \ttest_path_is_missing ../foo\n '\n \n-test_expect_failure SYMLINKS 'symlink escape via absolute path' '\n+test_expect_success SYMLINKS 'symlink escape via absolute path' '\n \t{\n \t\tmkpatch_symlink tmp \"$(pwd)\" &&\n \t\tmkpatch_add tmp/foo ../foo\n@@ -128,7 +128,7 @@ test_expect_failure SYMLINKS 'symlink escape via absolute path' '\n \ttest_path_is_missing ../foo\n '\n \n-test_expect_failure SYMLINKS 'symlink escape via absolute path (index)' '\n+test_expect_success SYMLINKS 'symlink escape via absolute path (index)' '\n \t{\n \t\tmkpatch_symlink tmp \"$(pwd)\" &&\n \t\tmkpatch_add tmp/foo ../foo\n-- \n2.3.0-185-g073f588\n"}]}