Skip to content

Commit 5244a31

Browse files
gitstergitster
committed
Merge branch 'jc/apply-beyond-symlink' into maint
"git apply" was not very careful about reading from, removing, updating and creating paths outside the working tree (under --index/--cached) or the current directory (when used as a replacement for GNU patch). * jc/apply-beyond-symlink: apply: do not touch a file beyond a symbolic link apply: do not read from beyond a symbolic link apply: do not read from the filesystem under --index apply: reject input that touches outside the working area
2 parents 1469d99 + e0d201b commit 5244a31

File tree

4 files changed

+399
-2
lines changed

4 files changed

+399
-2
lines changed

‎Documentation/git-apply.txt

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ SYNOPSIS
1616
[--ignore-space-change | --ignore-whitespace ]
1717
[--whitespace=(nowarn|warn|fix|error|error-all)]
1818
[--exclude=<path>] [--include=<path>] [--directory=<root>]
19-
[--verbose] [<patch>...]
19+
[--verbose] [--unsafe-paths] [<patch>...]
2020

2121
DESCRIPTION
2222
-----------
@@ -229,6 +229,16 @@ For example, a patch that talks about updating `a/git-gui.sh` to `b/git-gui.sh`
229229
can be applied to the file in the working tree `modules/git-gui/git-gui.sh` by
230230
running `git apply --directory=modules/git-gui`.
231231

232+
--unsafe-paths::
233+
By default, a patch that affects outside the working area
234+
(either a Git controlled working tree, or the current working
235+
directory when "git apply" is used as a replacement of GNU
236+
patch) is rejected as a mistake (or a mischief).
237+
+
238+
When `git apply` is used as a "better GNU patch", the user can pass
239+
the `--unsafe-paths` option to override this safety check. This option
240+
has no effect when `--index` or `--cached` is in use.
241+
232242
Configuration
233243
-------------
234244

‎builtin/apply.c

Lines changed: 141 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ static int apply_verbosely;
5151
static int allow_overlap;
5252
static int no_add;
5353
static int threeway;
54+
static int unsafe_paths;
5455
static const char *fake_ancestor;
5556
static int line_termination = '\n';
5657
static unsigned int p_context = UINT_MAX;
@@ -3221,7 +3222,7 @@ static int load_patch_target(struct strbuf *buf,
32213222
const char *name,
32223223
unsigned expected_mode)
32233224
{
3224-
if (cached) {
3225+
if (cached || check_index) {
32253226
if (read_file_or_gitlink(ce, buf))
32263227
return error(_("read of %s failed"), name);
32273228
} else if (name) {
@@ -3230,6 +3231,8 @@ static int load_patch_target(struct strbuf *buf,
32303231
return read_file_or_gitlink(ce, buf);
32313232
else
32323233
return SUBMODULE_PATCH_WITHOUT_INDEX;
3234+
} else if (has_symlink_leading_path(name, strlen(name))) {
3235+
return error(_("reading from '%s' beyond a symbolic link"), name);
32333236
} else {
32343237
if (read_old_data(st, name, buf))
32353238
return error(_("read of %s failed"), name);
@@ -3569,6 +3572,121 @@ static int check_to_create(const char *new_name, int ok_if_exists)
35693572
return 0;
35703573
}
35713574

3575+
/*
3576+
* We need to keep track of how symlinks in the preimage are
3577+
* manipulated by the patches. A patch to add a/b/c where a/b
3578+
* is a symlink should not be allowed to affect the directory
3579+
* the symlink points at, but if the same patch removes a/b,
3580+
* it is perfectly fine, as the patch removes a/b to make room
3581+
* to create a directory a/b so that a/b/c can be created.
3582+
*/
3583+
static struct string_list symlink_changes;
3584+
#define SYMLINK_GOES_AWAY 01
3585+
#define SYMLINK_IN_RESULT 02
3586+
3587+
static uintptr_t register_symlink_changes(const char *path, uintptr_t what)
3588+
{
3589+
struct string_list_item *ent;
3590+
3591+
ent = string_list_lookup(&symlink_changes, path);
3592+
if (!ent) {
3593+
ent = string_list_insert(&symlink_changes, path);
3594+
ent->util = (void *)0;
3595+
}
3596+
ent->util = (void *)(what | ((uintptr_t)ent->util));
3597+
return (uintptr_t)ent->util;
3598+
}
3599+
3600+
static uintptr_t check_symlink_changes(const char *path)
3601+
{
3602+
struct string_list_item *ent;
3603+
3604+
ent = string_list_lookup(&symlink_changes, path);
3605+
if (!ent)
3606+
return 0;
3607+
return (uintptr_t)ent->util;
3608+
}
3609+
3610+
static void prepare_symlink_changes(struct patch *patch)
3611+
{
3612+
for ( ; patch; patch = patch->next) {
3613+
if ((patch->old_name && S_ISLNK(patch->old_mode)) &&
3614+
(patch->is_rename || patch->is_delete))
3615+
/* the symlink at patch->old_name is removed */
3616+
register_symlink_changes(patch->old_name, SYMLINK_GOES_AWAY);
3617+
3618+
if (patch->new_name && S_ISLNK(patch->new_mode))
3619+
/* the symlink at patch->new_name is created or remains */
3620+
register_symlink_changes(patch->new_name, SYMLINK_IN_RESULT);
3621+
}
3622+
}
3623+
3624+
static int path_is_beyond_symlink_1(struct strbuf *name)
3625+
{
3626+
do {
3627+
unsigned int change;
3628+
3629+
while (--name->len && name->buf[name->len] != '/')
3630+
; /* scan backwards */
3631+
if (!name->len)
3632+
break;
3633+
name->buf[name->len] = '\0';
3634+
change = check_symlink_changes(name->buf);
3635+
if (change & SYMLINK_IN_RESULT)
3636+
return 1;
3637+
if (change & SYMLINK_GOES_AWAY)
3638+
/*
3639+
* This cannot be "return 0", because we may
3640+
* see a new one created at a higher level.
3641+
*/
3642+
continue;
3643+
3644+
/* otherwise, check the preimage */
3645+
if (check_index) {
3646+
struct cache_entry *ce;
3647+
3648+
ce = cache_file_exists(name->buf, name->len, ignore_case);
3649+
if (ce && S_ISLNK(ce->ce_mode))
3650+
return 1;
3651+
} else {
3652+
struct stat st;
3653+
if (!lstat(name->buf, &st) && S_ISLNK(st.st_mode))
3654+
return 1;
3655+
}
3656+
} while (1);
3657+
return 0;
3658+
}
3659+
3660+
static int path_is_beyond_symlink(const char *name_)
3661+
{
3662+
int ret;
3663+
struct strbuf name = STRBUF_INIT;
3664+
3665+
assert(*name_ != '\0');
3666+
strbuf_addstr(&name, name_);
3667+
ret = path_is_beyond_symlink_1(&name);
3668+
strbuf_release(&name);
3669+
3670+
return ret;
3671+
}
3672+
3673+
static void die_on_unsafe_path(struct patch *patch)
3674+
{
3675+
const char *old_name = NULL;
3676+
const char *new_name = NULL;
3677+
if (patch->is_delete)
3678+
old_name = patch->old_name;
3679+
else if (!patch->is_new && !patch->is_copy)
3680+
old_name = patch->old_name;
3681+
if (!patch->is_delete)
3682+
new_name = patch->new_name;
3683+
3684+
if (old_name && !verify_path(old_name))
3685+
die(_("invalid path '%s'"), old_name);
3686+
if (new_name && !verify_path(new_name))
3687+
die(_("invalid path '%s'"), new_name);
3688+
}
3689+
35723690
/*
35733691
* Check and apply the patch in-core; leave the result in patch->result
35743692
* for the caller to write it out to the final destination.
@@ -3656,6 +3774,22 @@ static int check_patch(struct patch *patch)
36563774
}
36573775
}
36583776

3777+
if (!unsafe_paths)
3778+
die_on_unsafe_path(patch);
3779+
3780+
/*
3781+
* An attempt to read from or delete a path that is beyond a
3782+
* symbolic link will be prevented by load_patch_target() that
3783+
* is called at the beginning of apply_data() so we do not
3784+
* have to worry about a patch marked with "is_delete" bit
3785+
* here. We however need to make sure that the patch result
3786+
* is not deposited to a path that is beyond a symbolic link
3787+
* here.
3788+
*/
3789+
if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
3790+
return error(_("affected file '%s' is beyond a symbolic link"),
3791+
patch->new_name);
3792+
36593793
if (apply_data(patch, &st, ce) < 0)
36603794
return error(_("%s: patch does not apply"), name);
36613795
patch->rejected = 0;
@@ -3666,6 +3800,7 @@ static int check_patch_list(struct patch *patch)
36663800
{
36673801
int err = 0;
36683802

3803+
prepare_symlink_changes(patch);
36693804
prepare_fn_table(patch);
36703805
while (patch) {
36713806
if (apply_verbosely)
@@ -4404,6 +4539,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
44044539
N_("make sure the patch is applicable to the current index")),
44054540
OPT_BOOL(0, "cached", &cached,
44064541
N_("apply a patch without touching the working tree")),
4542+
OPT_BOOL(0, "unsafe-paths", &unsafe_paths,
4543+
N_("accept a patch that touches outside the working area")),
44074544
OPT_BOOL(0, "apply", &force_apply,
44084545
N_("also apply the patch (use with --stat/--summary/--check)")),
44094546
OPT_BOOL('3', "3way", &threeway,
@@ -4476,6 +4613,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
44764613
die(_("--cached outside a repository"));
44774614
check_index = 1;
44784615
}
4616+
if (check_index)
4617+
unsafe_paths = 0;
4618+
44794619
for (i = 0; i < argc; i++) {
44804620
const char *arg = argv[i];
44814621
int fd;

‎t/t4122-apply-symlink-inside.sh

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,4 +52,110 @@ test_expect_success 'check result' '
5252
5353
'
5454

55+
test_expect_success SYMLINKS 'do not read from beyond symbolic link' '
56+
git reset --hard &&
57+
mkdir -p arch/x86_64/dir &&
58+
>arch/x86_64/dir/file &&
59+
git add arch/x86_64/dir/file &&
60+
echo line >arch/x86_64/dir/file &&
61+
git diff >patch &&
62+
git reset --hard &&
63+
64+
mkdir arch/i386/dir &&
65+
>arch/i386/dir/file &&
66+
ln -s ../i386/dir arch/x86_64/dir &&
67+
68+
test_must_fail git apply patch &&
69+
test_must_fail git apply --cached patch &&
70+
test_must_fail git apply --index patch
71+
72+
'
73+
74+
test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '
75+
76+
rm -rf arch/i386/dir arch/x86_64/dir &&
77+
git reset --hard &&
78+
ln -s ../i386/dir arch/x86_64/dir &&
79+
git add arch/x86_64/dir &&
80+
git diff HEAD >add_symlink.patch &&
81+
git reset --hard &&
82+
83+
mkdir arch/x86_64/dir &&
84+
>arch/x86_64/dir/file &&
85+
git add arch/x86_64/dir/file &&
86+
git diff HEAD >add_file.patch &&
87+
git diff -R HEAD >del_file.patch &&
88+
git reset --hard &&
89+
rm -fr arch/x86_64/dir &&
90+
91+
cat add_symlink.patch add_file.patch >patch &&
92+
cat add_symlink.patch del_file.patch >tricky_del &&
93+
94+
mkdir arch/i386/dir
95+
'
96+
97+
test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '
98+
99+
# same input creates a confusing symbolic link
100+
test_must_fail git apply patch 2>error-wt &&
101+
test_i18ngrep "beyond a symbolic link" error-wt &&
102+
test_path_is_missing arch/x86_64/dir &&
103+
test_path_is_missing arch/i386/dir/file &&
104+
105+
test_must_fail git apply --index patch 2>error-ix &&
106+
test_i18ngrep "beyond a symbolic link" error-ix &&
107+
test_path_is_missing arch/x86_64/dir &&
108+
test_path_is_missing arch/i386/dir/file &&
109+
test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
110+
test_must_fail git ls-files --error-unmatch arch/i386/dir &&
111+
112+
test_must_fail git apply --cached patch 2>error-ct &&
113+
test_i18ngrep "beyond a symbolic link" error-ct &&
114+
test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
115+
test_must_fail git ls-files --error-unmatch arch/i386/dir &&
116+
117+
>arch/i386/dir/file &&
118+
git add arch/i386/dir/file &&
119+
120+
test_must_fail git apply tricky_del &&
121+
test_path_is_file arch/i386/dir/file &&
122+
123+
test_must_fail git apply --index tricky_del &&
124+
test_path_is_file arch/i386/dir/file &&
125+
test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
126+
git ls-files --error-unmatch arch/i386/dir &&
127+
128+
test_must_fail git apply --cached tricky_del &&
129+
test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
130+
git ls-files --error-unmatch arch/i386/dir
131+
'
132+
133+
test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '
134+
135+
# existing symbolic link
136+
git reset --hard &&
137+
ln -s ../i386/dir arch/x86_64/dir &&
138+
git add arch/x86_64/dir &&
139+
140+
test_must_fail git apply add_file.patch 2>error-wt-add &&
141+
test_i18ngrep "beyond a symbolic link" error-wt-add &&
142+
test_path_is_missing arch/i386/dir/file &&
143+
144+
mkdir arch/i386/dir &&
145+
>arch/i386/dir/file &&
146+
test_must_fail git apply del_file.patch 2>error-wt-del &&
147+
test_i18ngrep "beyond a symbolic link" error-wt-del &&
148+
test_path_is_file arch/i386/dir/file &&
149+
rm arch/i386/dir/file &&
150+
151+
test_must_fail git apply --index add_file.patch 2>error-ix-add &&
152+
test_i18ngrep "beyond a symbolic link" error-ix-add &&
153+
test_path_is_missing arch/i386/dir/file &&
154+
test_must_fail git ls-files --error-unmatch arch/i386/dir &&
155+
156+
test_must_fail git apply --cached add_file.patch 2>error-ct-file &&
157+
test_i18ngrep "beyond a symbolic link" error-ct-file &&
158+
test_must_fail git ls-files --error-unmatch arch/i386/dir
159+
'
160+
55161
test_done

0 commit comments

Comments
 (0)