From c536c0755f6450b7bcce499cfda171f8c6d1e593 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 29 Jan 2015 15:35:24 -0800 Subject: apply: reject input that touches outside the working area By default, a patch that affects outside the working area (either a Git controlled working tree, or the current working directory when "git apply" is used as a replacement of GNU patch) is rejected as a mistake (or a mischief). Git itself does not create such a patch, unless the user bends over backwards and specifies a non-standard prefix to "git diff" and friends. When `git apply` is used as a "better GNU patch", the user can pass the `--unsafe-paths` option to override this safety check. This option has no effect when `--index` or `--cached` is in use. The new test was stolen from Jeff King with slight enhancements. Note that a few new tests for touching outside the working area by following a symbolic link are still expected to fail at this step, but will be fixed in later steps. Signed-off-by: Junio C Hamano diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt index f605327..9489664 100644 --- a/Documentation/git-apply.txt +++ b/Documentation/git-apply.txt @@ -16,7 +16,7 @@ SYNOPSIS [--ignore-space-change | --ignore-whitespace ] [--whitespace=(nowarn|warn|fix|error|error-all)] [--exclude=] [--include=] [--directory=] - [--verbose] [...] + [--verbose] [--unsafe-paths] [...] DESCRIPTION ----------- @@ -229,6 +229,16 @@ For example, a patch that talks about updating `a/git-gui.sh` to `b/git-gui.sh` can be applied to the file in the working tree `modules/git-gui/git-gui.sh` by running `git apply --directory=modules/git-gui`. +--unsafe-paths:: + By default, a patch that affects outside the working area + (either a Git controlled working tree, or the current working + directory when "git apply" is used as a replacement of GNU + patch) is rejected as a mistake (or a mischief). ++ +When `git apply` is used as a "better GNU patch", the user can pass +the `--unsafe-paths` option to override this safety check. This option +has no effect when `--index` or `--cached` is in use. + Configuration ------------- diff --git a/builtin/apply.c b/builtin/apply.c index ef32e4f..8561236 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -50,6 +50,7 @@ static int apply_verbosely; static int allow_overlap; static int no_add; static int threeway; +static int unsafe_paths; static const char *fake_ancestor; static int line_termination = '\n'; static unsigned int p_context = UINT_MAX; @@ -3483,6 +3484,23 @@ static int check_to_create(const char *new_name, int ok_if_exists) return 0; } +static void die_on_unsafe_path(struct patch *patch) +{ + const char *old_name = NULL; + const char *new_name = NULL; + if (patch->is_delete) + old_name = patch->old_name; + else if (!patch->is_new && !patch->is_copy) + old_name = patch->old_name; + if (!patch->is_delete) + new_name = patch->new_name; + + if (old_name && !verify_path(old_name)) + die(_("invalid path '%s'"), old_name); + if (new_name && !verify_path(new_name)) + die(_("invalid path '%s'"), new_name); +} + /* * Check and apply the patch in-core; leave the result in patch->result * for the caller to write it out to the final destination. @@ -3570,6 +3588,9 @@ static int check_patch(struct patch *patch) } } + if (!unsafe_paths) + die_on_unsafe_path(patch); + if (apply_data(patch, &st, ce) < 0) return error(_("%s: patch does not apply"), name); patch->rejected = 0; @@ -4379,6 +4400,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_) N_("make sure the patch is applicable to the current index")), OPT_BOOL(0, "cached", &cached, N_("apply a patch without touching the working tree")), + OPT_BOOL(0, "unsafe-paths", &unsafe_paths, + N_("accept a patch that touches outside the working area")), OPT_BOOL(0, "apply", &force_apply, N_("also apply the patch (use with --stat/--summary/--check)")), OPT_BOOL('3', "3way", &threeway, @@ -4451,6 +4474,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix_) die(_("--cached outside a repository")); check_index = 1; } + if (check_index) + unsafe_paths = 0; + for (i = 0; i < argc; i++) { const char *arg = argv[i]; int fd; diff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh new file mode 100755 index 0000000..02b97b3 --- /dev/null +++ b/t/t4139-apply-escape.sh @@ -0,0 +1,141 @@ +#!/bin/sh + +test_description='paths written by git-apply cannot escape the working tree' +. ./test-lib.sh + +# tests will try to write to ../foo, and we do not +# want them to escape the trash directory when they +# fail +test_expect_success 'bump git repo one level down' ' + mkdir inside && + mv .git inside/ && + cd inside +' + +# $1 = name of file +# $2 = current path to file (if different) +mkpatch_add () { + rm -f "${2:-$1}" && + cat <<-EOF + diff --git a/$1 b/$1 + new file mode 100644 + index 0000000..53c74cd + --- /dev/null + +++ b/$1 + @@ -0,0 +1 @@ + +evil + EOF +} + +mkpatch_del () { + echo evil >"${2:-$1}" && + cat <<-EOF + diff --git a/$1 b/$1 + deleted file mode 100644 + index 53c74cd..0000000 + --- a/$1 + +++ /dev/null + @@ -1 +0,0 @@ + -evil + EOF +} + +# $1 = name of file +# $2 = content of symlink +mkpatch_symlink () { + rm -f "$1" && + cat <<-EOF + diff --git a/$1 b/$1 + new file mode 120000 + index 0000000..$(printf "%s" "$2" | git hash-object --stdin) + --- /dev/null + +++ b/$1 + @@ -0,0 +1 @@ + +$2 + \ No newline at end of file + EOF +} + +test_expect_success 'cannot create file containing ..' ' + mkpatch_add ../foo >patch && + test_must_fail git apply patch && + test_path_is_missing ../foo +' + +test_expect_success 'can create file containing .. with --unsafe-paths' ' + mkpatch_add ../foo >patch && + git apply --unsafe-paths patch && + test_path_is_file ../foo +' + +test_expect_success 'cannot create file containing .. (index)' ' + mkpatch_add ../foo >patch && + test_must_fail git apply --index patch && + test_path_is_missing ../foo +' + +test_expect_success 'cannot create file containing .. with --unsafe-paths (index)' ' + mkpatch_add ../foo >patch && + test_must_fail git apply --index --unsafe-paths patch && + test_path_is_missing ../foo +' + +test_expect_success 'cannot delete file containing ..' ' + mkpatch_del ../foo >patch && + test_must_fail git apply patch && + test_path_is_file ../foo +' + +test_expect_success 'can delete file containing .. with --unsafe-paths' ' + mkpatch_del ../foo >patch && + git apply --unsafe-paths patch && + test_path_is_missing ../foo +' + +test_expect_success 'cannot delete file containing .. (index)' ' + mkpatch_del ../foo >patch && + test_must_fail git apply --index patch && + test_path_is_file ../foo +' + +test_expect_failure SYMLINKS 'symlink escape via ..' ' + { + mkpatch_symlink tmp .. && + mkpatch_add tmp/foo ../foo + } >patch && + test_must_fail git apply patch && + test_path_is_missing tmp && + test_path_is_missing ../foo +' + +test_expect_failure SYMLINKS 'symlink escape via .. (index)' ' + { + mkpatch_symlink tmp .. && + mkpatch_add tmp/foo ../foo + } >patch && + test_must_fail git apply --index patch && + test_path_is_missing tmp && + test_path_is_missing ../foo +' + +test_expect_failure SYMLINKS 'symlink escape via absolute path' ' + { + mkpatch_symlink tmp "$(pwd)" && + mkpatch_add tmp/foo ../foo + } >patch && + test_must_fail git apply patch && + test_path_is_missing tmp && + test_path_is_missing ../foo +' + +test_expect_failure SYMLINKS 'symlink escape via absolute path (index)' ' + { + mkpatch_symlink tmp "$(pwd)" && + mkpatch_add tmp/foo ../foo + } >patch && + test_must_fail git apply --index patch && + test_path_is_missing tmp && + test_path_is_missing ../foo +' + +test_done -- cgit v0.10.2-6-g49f6 From 3c37a2e339e695c7cc41048fe0921cbc8b48b0f0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 30 Jan 2015 15:15:59 -0800 Subject: apply: do not read from the filesystem under --index We currently read the preimage to apply a patch from the index only when the --cached option is given. Do so also when the command is running under the --index option. With --index, the index entry and the working tree file for a path that is involved in a patch must be identical, so this should not affect the result, but by reading from the index, we will get the protection to avoid reading an unintended path beyond a symbolic link automatically. Signed-off-by: Junio C Hamano diff --git a/builtin/apply.c b/builtin/apply.c index 8561236..21e45a0 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -3136,7 +3136,7 @@ static int load_patch_target(struct strbuf *buf, const char *name, unsigned expected_mode) { - if (cached) { + if (cached || check_index) { if (read_file_or_gitlink(ce, buf)) return error(_("read of %s failed"), name); } else if (name) { -- cgit v0.10.2-6-g49f6 From fdc2c3a926c21e24986677abd02c8bc568a5de32 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 30 Jan 2015 15:34:13 -0800 Subject: apply: do not read from beyond a symbolic link We should reject a patch, whether it renames/copies dir/file to elsewhere with or without modificiation, or updates dir/file in place, if "dir/" part is actually a symbolic link to elsewhere, by making sure that the code to read the preimage does not read from a path that is beyond a symbolic link. Signed-off-by: Junio C Hamano diff --git a/builtin/apply.c b/builtin/apply.c index 21e45a0..422e4ce 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -3145,6 +3145,8 @@ static int load_patch_target(struct strbuf *buf, return read_file_or_gitlink(ce, buf); else return SUBMODULE_PATCH_WITHOUT_INDEX; + } else if (has_symlink_leading_path(name, strlen(name))) { + return error(_("reading from '%s' beyond a symbolic link"), name); } else { if (read_old_data(st, name, buf)) return error(_("read of %s failed"), name); diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh index 70b3a06..035c080 100755 --- a/t/t4122-apply-symlink-inside.sh +++ b/t/t4122-apply-symlink-inside.sh @@ -52,4 +52,23 @@ test_expect_success 'check result' ' ' +test_expect_success SYMLINKS 'do not read from beyond symbolic link' ' + git reset --hard && + mkdir -p arch/x86_64/dir && + >arch/x86_64/dir/file && + git add arch/x86_64/dir/file && + echo line >arch/x86_64/dir/file && + git diff >patch && + git reset --hard && + + mkdir arch/i386/dir && + >arch/i386/dir/file && + ln -s ../i386/dir arch/x86_64/dir && + + test_must_fail git apply patch && + test_must_fail git apply --cached patch && + test_must_fail git apply --index patch + +' + test_done -- cgit v0.10.2-6-g49f6 From e0d201b61601e17e24ed00cc3d16e8e25ca68596 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 29 Jan 2015 12:41:22 -0800 Subject: apply: do not touch a file beyond a symbolic link Because Git tracks symbolic links as symbolic links, a path that has a symbolic link in its leading part (e.g. path/to/dir/file, where path/to/dir is a symbolic link to somewhere else, be it inside or outside the working tree) can never appear in a patch that validly applies, unless the same patch first removes the symbolic link to allow a directory to be created there. Detect and reject such a patch. Things to note: - Unfortunately, we cannot reuse the has_symlink_leading_path() from dir.c, as that is only about the working tree, but "git apply" can be told to apply the patch only to the index or to both the index and to the working tree. - We cannot directly use has_symlink_leading_path() even when we are applying only to the working tree, as an early patch of a valid input may remove a symbolic link path/to/dir and then a later patch of the input may create a path path/to/dir/file, but "git apply" first checks the input without touching either the index or the working tree. The leading symbolic link check must be done on the interim result we compute in-core (i.e. after the first patch, there is no path/to/dir symbolic link and it is perfectly valid to create path/to/dir/file). Similarly, when an input creates a symbolic link path/to/dir and then creates a file path/to/dir/file, we need to flag it as an error without actually creating path/to/dir symbolic link in the filesystem. Instead, for any patch in the input that leaves a path (i.e. a non deletion) in the result, we check all leading paths against the resulting tree that the patch would create by inspecting all the patches in the input and then the target of patch application (either the index or the working tree). This way, we catch a mischief or a mistake to add a symbolic link path/to/dir and a file path/to/dir/file at the same time, while allowing a valid patch that removes a symbolic link path/to/dir and then adds a file path/to/dir/file. Signed-off-by: Junio C Hamano diff --git a/builtin/apply.c b/builtin/apply.c index 422e4ce..c2c6fda 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -3486,6 +3486,104 @@ static int check_to_create(const char *new_name, int ok_if_exists) return 0; } +/* + * We need to keep track of how symlinks in the preimage are + * manipulated by the patches. A patch to add a/b/c where a/b + * is a symlink should not be allowed to affect the directory + * the symlink points at, but if the same patch removes a/b, + * it is perfectly fine, as the patch removes a/b to make room + * to create a directory a/b so that a/b/c can be created. + */ +static struct string_list symlink_changes; +#define SYMLINK_GOES_AWAY 01 +#define SYMLINK_IN_RESULT 02 + +static uintptr_t register_symlink_changes(const char *path, uintptr_t what) +{ + struct string_list_item *ent; + + ent = string_list_lookup(&symlink_changes, path); + if (!ent) { + ent = string_list_insert(&symlink_changes, path); + ent->util = (void *)0; + } + ent->util = (void *)(what | ((uintptr_t)ent->util)); + return (uintptr_t)ent->util; +} + +static uintptr_t check_symlink_changes(const char *path) +{ + struct string_list_item *ent; + + ent = string_list_lookup(&symlink_changes, path); + if (!ent) + return 0; + return (uintptr_t)ent->util; +} + +static void prepare_symlink_changes(struct patch *patch) +{ + for ( ; patch; patch = patch->next) { + if ((patch->old_name && S_ISLNK(patch->old_mode)) && + (patch->is_rename || patch->is_delete)) + /* the symlink at patch->old_name is removed */ + register_symlink_changes(patch->old_name, SYMLINK_GOES_AWAY); + + if (patch->new_name && S_ISLNK(patch->new_mode)) + /* the symlink at patch->new_name is created or remains */ + register_symlink_changes(patch->new_name, SYMLINK_IN_RESULT); + } +} + +static int path_is_beyond_symlink_1(struct strbuf *name) +{ + do { + unsigned int change; + + while (--name->len && name->buf[name->len] != '/') + ; /* scan backwards */ + if (!name->len) + break; + name->buf[name->len] = '\0'; + change = check_symlink_changes(name->buf); + if (change & SYMLINK_IN_RESULT) + return 1; + if (change & SYMLINK_GOES_AWAY) + /* + * This cannot be "return 0", because we may + * see a new one created at a higher level. + */ + continue; + + /* otherwise, check the preimage */ + if (check_index) { + struct cache_entry *ce; + + ce = cache_file_exists(name->buf, name->len, ignore_case); + if (ce && S_ISLNK(ce->ce_mode)) + return 1; + } else { + struct stat st; + if (!lstat(name->buf, &st) && S_ISLNK(st.st_mode)) + return 1; + } + } while (1); + return 0; +} + +static int path_is_beyond_symlink(const char *name_) +{ + int ret; + struct strbuf name = STRBUF_INIT; + + assert(*name_ != '\0'); + strbuf_addstr(&name, name_); + ret = path_is_beyond_symlink_1(&name); + strbuf_release(&name); + + return ret; +} + static void die_on_unsafe_path(struct patch *patch) { const char *old_name = NULL; @@ -3593,6 +3691,19 @@ static int check_patch(struct patch *patch) if (!unsafe_paths) die_on_unsafe_path(patch); + /* + * An attempt to read from or delete a path that is beyond a + * symbolic link will be prevented by load_patch_target() that + * is called at the beginning of apply_data() so we do not + * have to worry about a patch marked with "is_delete" bit + * here. We however need to make sure that the patch result + * is not deposited to a path that is beyond a symbolic link + * here. + */ + if (!patch->is_delete && path_is_beyond_symlink(patch->new_name)) + return error(_("affected file '%s' is beyond a symbolic link"), + patch->new_name); + if (apply_data(patch, &st, ce) < 0) return error(_("%s: patch does not apply"), name); patch->rejected = 0; @@ -3603,6 +3714,7 @@ static int check_patch_list(struct patch *patch) { int err = 0; + prepare_symlink_changes(patch); prepare_fn_table(patch); while (patch) { if (apply_verbosely) diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh index 035c080..1779c0a 100755 --- a/t/t4122-apply-symlink-inside.sh +++ b/t/t4122-apply-symlink-inside.sh @@ -71,4 +71,91 @@ test_expect_success SYMLINKS 'do not read from beyond symbolic link' ' ' +test_expect_success SYMLINKS 'do not follow symbolic link (setup)' ' + + rm -rf arch/i386/dir arch/x86_64/dir && + git reset --hard && + ln -s ../i386/dir arch/x86_64/dir && + git add arch/x86_64/dir && + git diff HEAD >add_symlink.patch && + git reset --hard && + + mkdir arch/x86_64/dir && + >arch/x86_64/dir/file && + git add arch/x86_64/dir/file && + git diff HEAD >add_file.patch && + git diff -R HEAD >del_file.patch && + git reset --hard && + rm -fr arch/x86_64/dir && + + cat add_symlink.patch add_file.patch >patch && + cat add_symlink.patch del_file.patch >tricky_del && + + mkdir arch/i386/dir +' + +test_expect_success SYMLINKS 'do not follow symbolic link (same input)' ' + + # same input creates a confusing symbolic link + test_must_fail git apply patch 2>error-wt && + test_i18ngrep "beyond a symbolic link" error-wt && + test_path_is_missing arch/x86_64/dir && + test_path_is_missing arch/i386/dir/file && + + test_must_fail git apply --index patch 2>error-ix && + test_i18ngrep "beyond a symbolic link" error-ix && + test_path_is_missing arch/x86_64/dir && + test_path_is_missing arch/i386/dir/file && + test_must_fail git ls-files --error-unmatch arch/x86_64/dir && + test_must_fail git ls-files --error-unmatch arch/i386/dir && + + test_must_fail git apply --cached patch 2>error-ct && + test_i18ngrep "beyond a symbolic link" error-ct && + test_must_fail git ls-files --error-unmatch arch/x86_64/dir && + test_must_fail git ls-files --error-unmatch arch/i386/dir && + + >arch/i386/dir/file && + git add arch/i386/dir/file && + + test_must_fail git apply tricky_del && + test_path_is_file arch/i386/dir/file && + + test_must_fail git apply --index tricky_del && + test_path_is_file arch/i386/dir/file && + test_must_fail git ls-files --error-unmatch arch/x86_64/dir && + git ls-files --error-unmatch arch/i386/dir && + + test_must_fail git apply --cached tricky_del && + test_must_fail git ls-files --error-unmatch arch/x86_64/dir && + git ls-files --error-unmatch arch/i386/dir +' + +test_expect_success SYMLINKS 'do not follow symbolic link (existing)' ' + + # existing symbolic link + git reset --hard && + ln -s ../i386/dir arch/x86_64/dir && + git add arch/x86_64/dir && + + test_must_fail git apply add_file.patch 2>error-wt-add && + test_i18ngrep "beyond a symbolic link" error-wt-add && + test_path_is_missing arch/i386/dir/file && + + mkdir arch/i386/dir && + >arch/i386/dir/file && + test_must_fail git apply del_file.patch 2>error-wt-del && + test_i18ngrep "beyond a symbolic link" error-wt-del && + test_path_is_file arch/i386/dir/file && + rm arch/i386/dir/file && + + test_must_fail git apply --index add_file.patch 2>error-ix-add && + test_i18ngrep "beyond a symbolic link" error-ix-add && + test_path_is_missing arch/i386/dir/file && + test_must_fail git ls-files --error-unmatch arch/i386/dir && + + test_must_fail git apply --cached add_file.patch 2>error-ct-file && + test_i18ngrep "beyond a symbolic link" error-ct-file && + test_must_fail git ls-files --error-unmatch arch/i386/dir +' + test_done diff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh index 02b97b3..45b5660 100755 --- a/t/t4139-apply-escape.sh +++ b/t/t4139-apply-escape.sh @@ -98,7 +98,7 @@ test_expect_success 'cannot delete file containing .. (index)' ' test_path_is_file ../foo ' -test_expect_failure SYMLINKS 'symlink escape via ..' ' +test_expect_success SYMLINKS 'symlink escape via ..' ' { mkpatch_symlink tmp .. && mkpatch_add tmp/foo ../foo @@ -108,7 +108,7 @@ test_expect_failure SYMLINKS 'symlink escape via ..' ' test_path_is_missing ../foo ' -test_expect_failure SYMLINKS 'symlink escape via .. (index)' ' +test_expect_success SYMLINKS 'symlink escape via .. (index)' ' { mkpatch_symlink tmp .. && mkpatch_add tmp/foo ../foo @@ -118,7 +118,7 @@ test_expect_failure SYMLINKS 'symlink escape via .. (index)' ' test_path_is_missing ../foo ' -test_expect_failure SYMLINKS 'symlink escape via absolute path' ' +test_expect_success SYMLINKS 'symlink escape via absolute path' ' { mkpatch_symlink tmp "$(pwd)" && mkpatch_add tmp/foo ../foo @@ -128,7 +128,7 @@ test_expect_failure SYMLINKS 'symlink escape via absolute path' ' test_path_is_missing ../foo ' -test_expect_failure SYMLINKS 'symlink escape via absolute path (index)' ' +test_expect_success SYMLINKS 'symlink escape via absolute path (index)' ' { mkpatch_symlink tmp "$(pwd)" && mkpatch_add tmp/foo ../foo -- cgit v0.10.2-6-g49f6