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 <gitster@pobox.com>
Junio C Hamano committed
Jan 29, 2015 at 12:41 UTC
e0d201b61601e17e24ed00cc3d16e8e25ca68596
3 files changed
+203
-4
builtin/apply.c
+112
@@ -3486,6 +3486,104 @@ static int check_to_create(const char *new_name, int ok_if_exists)
3486
return 0;
3487
}
3488
3489
+/*
3490
+ * We need to keep track of how symlinks in the preimage are
3491
+ * manipulated by the patches. A patch to add a/b/c where a/b
3492
+ * is a symlink should not be allowed to affect the directory
3493
+ * the symlink points at, but if the same patch removes a/b,
3494
+ * it is perfectly fine, as the patch removes a/b to make room
3495
+ * to create a directory a/b so that a/b/c can be created.
3496
+ */
3497
+static struct string_list symlink_changes;
3498
+#define SYMLINK_GOES_AWAY 01
3499
+#define SYMLINK_IN_RESULT 02
3500
+
3501
+static uintptr_t register_symlink_changes(const char *path, uintptr_t what)
3502
+{
3503
+ struct string_list_item *ent;
3504
+
3505
+ ent = string_list_lookup(&symlink_changes, path);
3506
+ if (!ent) {
3507
+ ent = string_list_insert(&symlink_changes, path);
3508
+ ent->util = (void *)0;
3509
+ }
3510
+ ent->util = (void *)(what | ((uintptr_t)ent->util));
3511
+ return (uintptr_t)ent->util;
3512
+}
3513
+
3514
+static uintptr_t check_symlink_changes(const char *path)
3515
+{
3516
+ struct string_list_item *ent;
3517
+
3518
+ ent = string_list_lookup(&symlink_changes, path);
3519
+ if (!ent)
3520
+ return 0;
3521
+ return (uintptr_t)ent->util;
3522
+}
3523
+
3524
+static void prepare_symlink_changes(struct patch *patch)
3525
+{
3526
+ for ( ; patch; patch = patch->next) {
3527
+ if ((patch->old_name && S_ISLNK(patch->old_mode)) &&
3528
+ (patch->is_rename || patch->is_delete))
3529
+ /* the symlink at patch->old_name is removed */
3530
+ register_symlink_changes(patch->old_name, SYMLINK_GOES_AWAY);
3531
+
3532
+ if (patch->new_name && S_ISLNK(patch->new_mode))
3533
+ /* the symlink at patch->new_name is created or remains */
3534
+ register_symlink_changes(patch->new_name, SYMLINK_IN_RESULT);
3535
+ }
3536
+}
3537
+
3538
+static int path_is_beyond_symlink_1(struct strbuf *name)
3539
+{
3540
+ do {
3541
+ unsigned int change;
3542
+
3543
+ while (--name->len && name->buf[name->len] != '/')
3544
+ ; /* scan backwards */
3545
+ if (!name->len)
3546
+ break;
3547
+ name->buf[name->len] = '\0';
3548
+ change = check_symlink_changes(name->buf);
3549
+ if (change & SYMLINK_IN_RESULT)
3550
+ return 1;
3551
+ if (change & SYMLINK_GOES_AWAY)
3552
+ /*
3553
+ * This cannot be "return 0", because we may
3554
+ * see a new one created at a higher level.
3555
+ */
3556
+ continue;
3557
+
3558
+ /* otherwise, check the preimage */
3559
+ if (check_index) {
3560
+ struct cache_entry *ce;
3561
+
3562
+ ce = cache_file_exists(name->buf, name->len, ignore_case);
3563
+ if (ce && S_ISLNK(ce->ce_mode))
3564
+ return 1;
3565
+ } else {
3566
+ struct stat st;
3567
+ if (!lstat(name->buf, &st) && S_ISLNK(st.st_mode))
3568
+ return 1;
3569
+ }
3570
+ } while (1);
3571
+ return 0;
3572
+}
3573
+
3574
+static int path_is_beyond_symlink(const char *name_)
3575
+{
3576
+ int ret;
3577
+ struct strbuf name = STRBUF_INIT;
3578
+
3579
+ assert(*name_ != '\0');
3580
+ strbuf_addstr(&name, name_);
3581
+ ret = path_is_beyond_symlink_1(&name);
3582
+ strbuf_release(&name);
3583
+
3584
+ return ret;
3585
+}
3586
+
3587
static void die_on_unsafe_path(struct patch *patch)
3588
{
3589
const char *old_name = NULL;
@@ -3593,6 +3691,19 @@ static int check_patch(struct patch *patch)
3691
if (!unsafe_paths)
3692
die_on_unsafe_path(patch);
3693
3694
+ /*
3695
+ * An attempt to read from or delete a path that is beyond a
3696
+ * symbolic link will be prevented by load_patch_target() that
3697
+ * is called at the beginning of apply_data() so we do not
3698
+ * have to worry about a patch marked with "is_delete" bit
3699
+ * here. We however need to make sure that the patch result
3700
+ * is not deposited to a path that is beyond a symbolic link
3701
+ * here.
3702
+ */
3703
+ if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
3704
+ return error(_("affected file '%s' is beyond a symbolic link"),
3705
+ patch->new_name);
3706
+
3707
if (apply_data(patch, &st, ce) < 0)
3708
return error(_("%s: patch does not apply"), name);
3709
patch->rejected = 0;
@@ -3603,6 +3714,7 @@ static int check_patch_list(struct patch *patch)
3714
{
3715
int err = 0;
3716
3717
+ prepare_symlink_changes(patch);
3718
prepare_fn_table(patch);
3719
while (patch) {
3720
if (apply_verbosely)
t/t4122-apply-symlink-inside.sh
+87
@@ -71,4 +71,91 @@ test_expect_success SYMLINKS 'do not read from beyond symbolic link' '
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
+
161
test_done
t/t4139-apply-escape.sh
+4
-4
@@ -98,7 +98,7 @@ test_expect_success 'cannot delete file containing .. (index)' '
98
test_path_is_file ../foo
99
'
100
101
-test_expect_failure SYMLINKS 'symlink escape via ..' '
101
+test_expect_success SYMLINKS 'symlink escape via ..' '
102
{
103
mkpatch_symlink tmp .. &&
104
mkpatch_add tmp/foo ../foo
@@ -108,7 +108,7 @@ test_expect_failure SYMLINKS 'symlink escape via ..' '
108
test_path_is_missing ../foo
109
'
110
111
-test_expect_failure SYMLINKS 'symlink escape via .. (index)' '
111
+test_expect_success SYMLINKS 'symlink escape via .. (index)' '
112
{
113
mkpatch_symlink tmp .. &&
114
mkpatch_add tmp/foo ../foo
@@ -118,7 +118,7 @@ test_expect_failure SYMLINKS 'symlink escape via .. (index)' '
118
test_path_is_missing ../foo
119
'
120
121
-test_expect_failure SYMLINKS 'symlink escape via absolute path' '
121
+test_expect_success SYMLINKS 'symlink escape via absolute path' '
122
{
123
mkpatch_symlink tmp "$(pwd)" &&
124
mkpatch_add tmp/foo ../foo
@@ -128,7 +128,7 @@ test_expect_failure SYMLINKS 'symlink escape via absolute path' '
128
test_path_is_missing ../foo
129
'
130
131
-test_expect_failure SYMLINKS 'symlink escape via absolute path (index)' '
131
+test_expect_success SYMLINKS 'symlink escape via absolute path (index)' '
132
{
133
mkpatch_symlink tmp "$(pwd)" &&
134
mkpatch_add tmp/foo ../foo