Re: [PATCH/RFC] fsck: complain when .gitignore and .gitattributes are symlinks

[Jeff King <peff@peff.net>]

On Mon, Jan 14, 2019 at 03:09:02PM -0800, Jonathan Nieder wrote:

> From: Jeff King <peff@peff.net>
> 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;
 				}
 			}