Skip to content

Commit b9e128a

Browse files
gitstergitster
committed
Merge branch 'jk/symlinked-dotgitx-files' into next
"git fsck" and the corresponding check done during the transport learned to ensure that in-tree files like `.gitignore` and `.gitattributes` are not symbolic links. * jk/symlinked-dotgitx-files: fsck: complain when .gitattributes or .gitignore is a symlink verify_path(): disallow symlinks in .gitattributes and .gitignore t0060: test obscured .gitattributes and .gitignore matching t7450: test .gitmodules symlink matching against obscured names t7450: test verify_path() handling of gitmodules t7415: rename to expand scope fsck_tree(): wrap some long lines fsck_tree(): fix shadowed variable
2 parents 0729586 + b77ec0c commit b9e128a

File tree

5 files changed

+187
-64
lines changed

5 files changed

+187
-64
lines changed

‎fsck.c

Lines changed: 59 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ static struct oidset gitmodules_done = OIDSET_INIT;
6767
FUNC(GITMODULES_URL, ERROR) \
6868
FUNC(GITMODULES_PATH, ERROR) \
6969
FUNC(GITMODULES_UPDATE, ERROR) \
70+
FUNC(GITIGNORE_SYMLINK, ERROR) \
71+
FUNC(GITATTRIBUTES_SYMLINK, ERROR) \
7072
/* warnings */ \
7173
FUNC(BAD_FILEMODE, WARN) \
7274
FUNC(EMPTY_NAME, WARN) \
@@ -633,7 +635,7 @@ static int verify_ordered(unsigned mode1, const char *name1,
633635
return c1 < c2 ? 0 : TREE_UNORDERED;
634636
}
635637

636-
static int fsck_tree(const struct object_id *oid,
638+
static int fsck_tree(const struct object_id *tree_oid,
637639
const char *buffer, unsigned long size,
638640
struct fsck_options *options)
639641
{
@@ -654,7 +656,9 @@ static int fsck_tree(const struct object_id *oid,
654656
struct name_stack df_dup_candidates = { NULL };
655657

656658
if (init_tree_desc_gently(&desc, buffer, size)) {
657-
retval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
659+
retval += report(options, tree_oid, OBJ_TREE,
660+
FSCK_MSG_BAD_TREE,
661+
"cannot be parsed as a tree");
658662
return retval;
659663
}
660664

@@ -664,11 +668,11 @@ static int fsck_tree(const struct object_id *oid,
664668
while (desc.size) {
665669
unsigned short mode;
666670
const char *name, *backslash;
667-
const struct object_id *oid;
671+
const struct object_id *entry_oid;
668672

669-
oid = tree_entry_extract(&desc, &name, &mode);
673+
entry_oid = tree_entry_extract(&desc, &name, &mode);
670674

671-
has_null_sha1 |= is_null_oid(oid);
675+
has_null_sha1 |= is_null_oid(entry_oid);
672676
has_full_path |= !!strchr(name, '/');
673677
has_empty_name |= !*name;
674678
has_dot |= !strcmp(name, ".");
@@ -678,23 +682,36 @@ static int fsck_tree(const struct object_id *oid,
678682

679683
if (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {
680684
if (!S_ISLNK(mode))
681-
oidset_insert(&gitmodules_found, oid);
685+
oidset_insert(&gitmodules_found, entry_oid);
682686
else
683687
retval += report(options,
684-
oid, OBJ_TREE,
688+
tree_oid, OBJ_TREE,
685689
FSCK_MSG_GITMODULES_SYMLINK,
686690
".gitmodules is a symbolic link");
687691
}
688692

693+
if (S_ISLNK(mode)) {
694+
if (is_hfs_dotgitignore(name) ||
695+
is_ntfs_dotgitignore(name))
696+
retval += report(options, tree_oid, OBJ_TREE,
697+
FSCK_MSG_GITIGNORE_SYMLINK,
698+
".gitignore is a symlink");
699+
if (is_hfs_dotgitattributes(name) ||
700+
is_ntfs_dotgitattributes(name))
701+
retval += report(options, tree_oid, OBJ_TREE,
702+
FSCK_MSG_GITATTRIBUTES_SYMLINK,
703+
".gitattributes is a symlink");
704+
}
705+
689706
if ((backslash = strchr(name, '\\'))) {
690707
while (backslash) {
691708
backslash++;
692709
has_dotgit |= is_ntfs_dotgit(backslash);
693710
if (is_ntfs_dotgitmodules(backslash)) {
694711
if (!S_ISLNK(mode))
695-
oidset_insert(&gitmodules_found, oid);
712+
oidset_insert(&gitmodules_found, entry_oid);
696713
else
697-
retval += report(options, oid, OBJ_TREE,
714+
retval += report(options, tree_oid, OBJ_TREE,
698715
FSCK_MSG_GITMODULES_SYMLINK,
699716
".gitmodules is a symbolic link");
700717
}
@@ -703,7 +720,9 @@ static int fsck_tree(const struct object_id *oid,
703720
}
704721

705722
if (update_tree_entry_gently(&desc)) {
706-
retval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
723+
retval += report(options, tree_oid, OBJ_TREE,
724+
FSCK_MSG_BAD_TREE,
725+
"cannot be parsed as a tree");
707726
break;
708727
}
709728

@@ -751,25 +770,45 @@ static int fsck_tree(const struct object_id *oid,
751770
name_stack_clear(&df_dup_candidates);
752771

753772
if (has_null_sha1)
754-
retval += report(options, oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, "contains entries pointing to null sha1");
773+
retval += report(options, tree_oid, OBJ_TREE,
774+
FSCK_MSG_NULL_SHA1,
775+
"contains entries pointing to null sha1");
755776
if (has_full_path)
756-
retval += report(options, oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, "contains full pathnames");
777+
retval += report(options, tree_oid, OBJ_TREE,
778+
FSCK_MSG_FULL_PATHNAME,
779+
"contains full pathnames");
757780
if (has_empty_name)
758-
retval += report(options, oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, "contains empty pathname");
781+
retval += report(options, tree_oid, OBJ_TREE,
782+
FSCK_MSG_EMPTY_NAME,
783+
"contains empty pathname");
759784
if (has_dot)
760-
retval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOT, "contains '.'");
785+
retval += report(options, tree_oid, OBJ_TREE,
786+
FSCK_MSG_HAS_DOT,
787+
"contains '.'");
761788
if (has_dotdot)
762-
retval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, "contains '..'");
789+
retval += report(options, tree_oid, OBJ_TREE,
790+
FSCK_MSG_HAS_DOTDOT,
791+
"contains '..'");
763792
if (has_dotgit)
764-
retval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, "contains '.git'");
793+
retval += report(options, tree_oid, OBJ_TREE,
794+
FSCK_MSG_HAS_DOTGIT,
795+
"contains '.git'");
765796
if (has_zero_pad)
766-
retval += report(options, oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, "contains zero-padded file modes");
797+
retval += report(options, tree_oid, OBJ_TREE,
798+
FSCK_MSG_ZERO_PADDED_FILEMODE,
799+
"contains zero-padded file modes");
767800
if (has_bad_modes)
768-
retval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, "contains bad file modes");
801+
retval += report(options, tree_oid, OBJ_TREE,
802+
FSCK_MSG_BAD_FILEMODE,
803+
"contains bad file modes");
769804
if (has_dup_entries)
770-
retval += report(options, oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, "contains duplicate file entries");
805+
retval += report(options, tree_oid, OBJ_TREE,
806+
FSCK_MSG_DUPLICATE_ENTRIES,
807+
"contains duplicate file entries");
771808
if (not_properly_sorted)
772-
retval += report(options, oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, "not properly sorted");
809+
retval += report(options, tree_oid, OBJ_TREE,
810+
FSCK_MSG_TREE_NOT_SORTED,
811+
"not properly sorted");
773812
return retval;
774813
}
775814

‎read-cache.c

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -951,7 +951,9 @@ static int verify_dotfile(const char *rest, unsigned mode)
951951
return 0;
952952
if (S_ISLNK(mode)) {
953953
rest += 3;
954-
if (skip_iprefix(rest, "modules", &rest) &&
954+
if ((skip_iprefix(rest, "modules", &rest) ||
955+
skip_iprefix(rest, "ignore", &rest) ||
956+
skip_iprefix(rest, "attributes", &rest)) &&
955957
(*rest == '\0' || is_dir_sep(*rest)))
956958
return 0;
957959
}
@@ -984,7 +986,9 @@ int verify_path(const char *path, unsigned mode)
984986
if (is_hfs_dotgit(path))
985987
return 0;
986988
if (S_ISLNK(mode)) {
987-
if (is_hfs_dotgitmodules(path))
989+
if (is_hfs_dotgitmodules(path) ||
990+
is_hfs_dotgitignore(path) ||
991+
is_hfs_dotgitattributes(path))
988992
return 0;
989993
}
990994
}
@@ -996,7 +1000,9 @@ int verify_path(const char *path, unsigned mode)
9961000
if (is_ntfs_dotgit(path))
9971001
return 0;
9981002
if (S_ISLNK(mode)) {
999-
if (is_ntfs_dotgitmodules(path))
1003+
if (is_ntfs_dotgitmodules(path) ||
1004+
is_ntfs_dotgitignore(path) ||
1005+
is_ntfs_dotgitattributes(path))
10001006
return 0;
10011007
}
10021008
}

‎t/helper/test-path-utils.c

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -172,9 +172,22 @@ static struct test_data dirname_data[] = {
172172
{ NULL, NULL }
173173
};
174174

175-
static int is_dotgitmodules(const char *path)
175+
static int check_dotgitx(const char *x, const char **argv,
176+
int (*is_hfs)(const char *),
177+
int (*is_ntfs)(const char *))
176178
{
177-
return is_hfs_dotgitmodules(path) || is_ntfs_dotgitmodules(path);
179+
int res = 0, expect = 1;
180+
for (; *argv; argv++) {
181+
if (!strcmp("--not", *argv))
182+
expect = !expect;
183+
else if (expect != (is_hfs(*argv) || is_ntfs(*argv)))
184+
res = error("'%s' is %s.git%s", *argv,
185+
expect ? "not " : "", x);
186+
else
187+
fprintf(stderr, "ok: '%s' is %s.git%s\n",
188+
*argv, expect ? "" : "not ", x);
189+
}
190+
return !!res;
178191
}
179192

180193
static int cmp_by_st_size(const void *a, const void *b)
@@ -382,17 +395,19 @@ int cmd__path_utils(int argc, const char **argv)
382395
return test_function(dirname_data, posix_dirname, argv[1]);
383396

384397
if (argc > 2 && !strcmp(argv[1], "is_dotgitmodules")) {
385-
int res = 0, expect = 1, i;
386-
for (i = 2; i < argc; i++)
387-
if (!strcmp("--not", argv[i]))
388-
expect = !expect;
389-
else if (expect != is_dotgitmodules(argv[i]))
390-
res = error("'%s' is %s.gitmodules", argv[i],
391-
expect ? "not " : "");
392-
else
393-
fprintf(stderr, "ok: '%s' is %s.gitmodules\n",
394-
argv[i], expect ? "" : "not ");
395-
return !!res;
398+
return check_dotgitx("modules", argv + 2,
399+
is_hfs_dotgitmodules,
400+
is_ntfs_dotgitmodules);
401+
}
402+
if (argc > 2 && !strcmp(argv[1], "is_dotgitignore")) {
403+
return check_dotgitx("ignore", argv + 2,
404+
is_hfs_dotgitignore,
405+
is_ntfs_dotgitignore);
406+
}
407+
if (argc > 2 && !strcmp(argv[1], "is_dotgitattributes")) {
408+
return check_dotgitx("attributes", argv + 2,
409+
is_hfs_dotgitattributes,
410+
is_ntfs_dotgitattributes);
396411
}
397412

398413
if (argc > 2 && !strcmp(argv[1], "file-size")) {

‎t/t0060-path-utils.sh

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -468,6 +468,26 @@ test_expect_success 'match .gitmodules' '
468468
.gitmodules,:\$DATA
469469
'
470470

471+
test_expect_success 'match .gitattributes' '
472+
test-tool path-utils is_dotgitattributes \
473+
.gitattributes \
474+
.git${u200c}attributes \
475+
.Gitattributes \
476+
.gitattributeS \
477+
GITATT~1 \
478+
GI7D29~1
479+
'
480+
481+
test_expect_success 'match .gitignore' '
482+
test-tool path-utils is_dotgitignore \
483+
.gitignore \
484+
.git${u200c}ignore \
485+
.Gitignore \
486+
.gitignorE \
487+
GITIGN~1 \
488+
GI250A~1
489+
'
490+
471491
test_expect_success MINGW 'is_valid_path() on Windows' '
472492
test-tool path-utils is_valid_path \
473493
win32 \

‎t/t7415-submodule-names.sh renamed to ‎t/t7450-bad-dotgitx-files.sh

Lines changed: 71 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,16 @@
11
#!/bin/sh
22

3-
test_description='check handling of .. in submodule names
3+
test_description='check forbidden or malicious patterns in .git* files
44
5-
Exercise the name-checking function on a variety of names, and then give a
6-
real-world setup that confirms we catch this in practice.
5+
Such as:
6+
7+
- presence of .. in submodule names;
8+
Exercise the name-checking function on a variety of names, and then give a
9+
real-world setup that confirms we catch this in practice.
10+
11+
- nested submodule names
12+
13+
- symlinked .gitmodules, etc
714
'
815
. ./test-lib.sh
916
. "$TEST_DIRECTORY"/lib-pack.sh
@@ -132,31 +139,67 @@ test_expect_success 'index-pack --strict works for non-repo pack' '
132139
grep gitmodulesName output
133140
'
134141

135-
test_expect_success 'fsck detects symlinked .gitmodules file' '
136-
git init symlink &&
137-
(
138-
cd symlink &&
139-
140-
# Make the tree directly to avoid index restrictions.
141-
#
142-
# Because symlinks store the target as a blob, choose
143-
# a pathname that could be parsed as a .gitmodules file
144-
# to trick naive non-symlink-aware checking.
145-
tricky="[foo]bar=true" &&
146-
content=$(git hash-object -w ../.gitmodules) &&
147-
target=$(printf "$tricky" | git hash-object -w --stdin) &&
148-
{
149-
printf "100644 blob $content\t$tricky\n" &&
150-
printf "120000 blob $target\t.gitmodules\n"
151-
} | git mktree &&
152-
153-
# Check not only that we fail, but that it is due to the
154-
# symlink detector; this grep string comes from the config
155-
# variable name and will not be translated.
156-
test_must_fail git fsck 2>output &&
157-
test_i18ngrep gitmodulesSymlink output
158-
)
159-
'
142+
check_forbidden_symlink () {
143+
name=$1
144+
type=$2
145+
path=$3
146+
dir=symlink-$name-$type
147+
148+
test_expect_success "set up repo with symlinked $name ($type)" '
149+
git init $dir &&
150+
(
151+
cd $dir &&
152+
153+
# Make the tree directly to avoid index restrictions.
154+
#
155+
# Because symlinks store the target as a blob, choose
156+
# a pathname that could be parsed as a .gitmodules file
157+
# to trick naive non-symlink-aware checking.
158+
tricky="[foo]bar=true" &&
159+
content=$(git hash-object -w ../.gitmodules) &&
160+
target=$(printf "$tricky" | git hash-object -w --stdin) &&
161+
{
162+
printf "100644 blob $content\t$tricky\n" &&
163+
printf "120000 blob $target\t$path\n"
164+
} >bad-tree
165+
) &&
166+
tree=$(git -C $dir mktree <$dir/bad-tree)
167+
'
168+
169+
test_expect_success "fsck detects symlinked $name ($type)" '
170+
(
171+
cd $dir &&
172+
173+
# Check not only that we fail, but that it is due to the
174+
# symlink detector
175+
test_must_fail git fsck 2>output &&
176+
test_i18ngrep "tree $tree: ${name}Symlink" output
177+
)
178+
'
179+
180+
test_expect_success "refuse to load symlinked $name into index ($type)" '
181+
test_must_fail \
182+
git -C $dir \
183+
-c core.protectntfs \
184+
-c core.protecthfs \
185+
read-tree $tree 2>err &&
186+
test_i18ngrep "invalid path.*$name" err &&
187+
git -C $dir ls-files -s >out &&
188+
test_must_be_empty out
189+
'
190+
}
191+
192+
check_forbidden_symlink gitmodules vanilla .gitmodules
193+
check_forbidden_symlink gitmodules ntfs ".gitmodules ."
194+
check_forbidden_symlink gitmodules hfs ".${u200c}gitmodules"
195+
196+
check_forbidden_symlink gitattributes vanilla .gitattributes
197+
check_forbidden_symlink gitattributes ntfs ".gitattributes ."
198+
check_forbidden_symlink gitattributes hfs ".${u200c}gitattributes"
199+
200+
check_forbidden_symlink gitignore vanilla .gitignore
201+
check_forbidden_symlink gitignore ntfs ".gitignore ."
202+
check_forbidden_symlink gitignore hfs ".${u200c}gitignore"
160203

161204
test_expect_success 'fsck detects non-blob .gitmodules' '
162205
git init non-blob &&

0 commit comments

Comments
 (0)