On Mon, Jan 14, 2019 at 03:09:02PM -0800, Jonathan Nieder wrote:
> From: Jeff King <peff@xxxxxxxx>
> Date: Sun, 13 May 2018 14:14:34 -0400
>
> This case is already forbidden by verify_path(), so let's
> check it in fsck. It's easier to handle than .gitmodules,
> because we don't care about checking the blob content. This
> is really just about whether the name and mode for the tree
> entry are valid.
Hmm. I think this commit message isn't quite right, because we also
skipped the patches to touch gitignore/gitattributes in verify_path().
Are you thinking we should resurrect that behavior[1], too, or just
protect at the fsck level?
> It was omitted from that series because it does not address any known
> exploit, but to me it seems worthwhile anyway:
>
> - if a client enables transfer.fsckObjects, this helps them protect
> themselves against weird input that does *not* have a known exploit
> attached, to
>
> - it generally feels more simple and robust. Git-related tools can
> benefit from this kind of check as an indication of input they can
> bail out on instead of trying to support.
I think I may just be restating your two points above, but what I'd
argue is:
- even though there's no known-interesting exploit, this can cause Git
to unexpectedly read arbitrary files outside of the repository
directory. That in itself isn't necessarily evil, but it's weird.
- there are potentially non-malicious bugs here, where we try to read
.gitattributes out of the index, but obviously don't follow symlinks
there
-Peff
[1] This wasn't a separate patch, but just an early iteration of the
"ban symlinks in .gitmodules" patch. I think the incremental is
just:
diff --git a/read-cache.c b/read-cache.c
index bfff271a3d..121c0bec69 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -937,7 +937,9 @@ static int verify_dotfile(const char *rest, unsigned mode)
return 0;
if (S_ISLNK(mode)) {
rest += 3;
- if (skip_iprefix(rest, "modules", &rest) &&
+ if ((skip_iprefix(rest, "modules", &rest) ||
+ skip_iprefix(rest, "ignore", &rest) ||
+ skip_iprefix(rest, "attributes", &rest)) &&
(*rest == '\0' || is_dir_sep(*rest)))
return 0;
}
@@ -966,7 +968,9 @@ int verify_path(const char *path, unsigned mode)
if (is_hfs_dotgit(path))
return 0;
if (S_ISLNK(mode)) {
- if (is_hfs_dotgitmodules(path))
+ if (is_hfs_dotgitmodules(path) ||
+ is_hfs_dotgitignore(path) ||
+ is_hfs_dotgitattributes(path))
return 0;
}
}
@@ -974,7 +978,9 @@ int verify_path(const char *path, unsigned mode)
if (is_ntfs_dotgit(path))
return 0;
if (S_ISLNK(mode)) {
- if (is_ntfs_dotgitmodules(path))
+ if (is_ntfs_dotgitmodules(path) ||
+ is_ntfs_dotgitignore(path) ||
+ is_ntfs_dotgitattributes(path))
return 0;
}
}
