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

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

From: Jeff King <redacted>
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;
 				}
 			}