[PATCH 0/5] disallow symlinks for .gitignore and .gitattributes

[PATCH 0/5] disallow symlinks for .gitignore and .gitattributes

[Jeff King]

I noticed in a nearby discussion that we will follow in-filesystem
symlinks for in-tree .gitignore and .gitattributes files, but not when
those files are read out of the index (e.g., when switching branches).

This series teaches git to open those files with O_NOFOLLOW (when it is
available) to get more consistent behavior. Note that this only applies
to the in-tree versions; you can still symlink $GIT_DIR/info/attributes,
etc.

I stopped short of warning about symlinked entries in git-fsck, but
perhaps we would want to do that as well (doing it completely is tricky
because of all of the case-folding issues around matching pathnames).

  [1/5]: add open_nofollow() helper
  [2/5]: attr: convert "macro_ok" into a flags field
  [3/5]: exclude: convert "check_index" into a flags field
  [4/5]: attr: do not respect symlinks for in-tree .gitattributes
  [5/5]: exclude: do not respect symlinks for in-tree .gitignore

 attr.c                | 58 ++++++++++++++++++++++++++++++++-------------------
 dir.c                 | 20 +++++++++++++-----
 dir.h                 |  2 +-
 git-compat-util.h     |  3 +++
 t/t0003-attributes.sh | 31 +++++++++++++++++++++++++++
 t/t0008-ignores.sh    | 29 ++++++++++++++++++++++++++
 wrapper.c             |  8 +++++++
 7 files changed, 123 insertions(+), 28 deletions(-)

-Peff

[PATCH 1/5] add open_nofollow() helper

[Jeff King]

Some callers of open() would like to optionally use
O_NOFOLLOW, but it is not available on all platforms. We
could abstract this by publicly defining O_NOFOLLOW to 0 on
those platforms, but that leaves us no room for any
workarounds (e.g., by checking the file type via lstat()).

Instead, let's abstract it into its own function. We don't
implement any workarounds here, but it it would be easy to
add them later.

Signed-off-by: Jeff King <peff@peff.net>
---
I didn't add the workaround because I think the current callers are OK
with "best effort", and doing it non-racily is quite tricky (though we
might also be OK with a racy version; we are not trying to beat
/tmp races, but just making sure a checkout that we did is sane).

 git-compat-util.h | 3 +++
 wrapper.c         | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092..a2cc33ebc 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1080,6 +1080,9 @@ int access_or_die(const char *path, int mode, unsigned flag);
 /* Warn on an inaccessible file that ought to be accessible */
 void warn_on_inaccessible(const char *path);
 
+/* Open with O_NOFOLLOW, if available on this platform */
+int open_nofollow(const char *path, int flags);
+
 #ifdef GMTIME_UNRELIABLE_ERRORS
 struct tm *git_gmtime(const time_t *);
 struct tm *git_gmtime_r(const time_t *, struct tm *);
diff --git a/wrapper.c b/wrapper.c
index e7f197996..6bc7f24f5 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -679,3 +679,11 @@ void sleep_millisec(int millisec)
 {
 	poll(NULL, 0, millisec);
 }
+
+#ifndef O_NOFOLLOW
+#define O_NOFOLLOW 0
+#endif
+int open_nofollow(const char *path, int flags)
+{
+	return open(path, flags | O_NOFOLLOW);
+}
-- 
2.11.0.rc0.258.gf434c15

[PATCH 2/5] attr: convert "macro_ok" into a flags field

[Jeff King]

The attribute code can have a rather deep callstack, through
which we have to pass the "macro_ok" flag. In anticipation
of adding other flags, let's convert this to a generic
bit-field.

Signed-off-by: Jeff King <peff@peff.net>
---
 attr.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/attr.c b/attr.c
index 1fcf042b8..79bd89226 100644
--- a/attr.c
+++ b/attr.c
@@ -151,6 +151,9 @@ struct match_attr {
 
 static const char blank[] = " \t\r\n";
 
+/* Flags usable in read_attr() and parse_attr_line() family of functions. */
+#define READ_ATTR_MACRO_OK (1<<0)
+
 /*
  * Parse a whitespace-delimited attribute state (i.e., "attr",
  * "-attr", "!attr", or "attr=value") from the string starting at src.
@@ -200,7 +203,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
 }
 
 static struct match_attr *parse_attr_line(const char *line, const char *src,
-					  int lineno, int macro_ok)
+					  int lineno, unsigned flags)
 {
 	int namelen;
 	int num_attr, i;
@@ -215,7 +218,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 	namelen = strcspn(name, blank);
 	if (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&
 	    starts_with(name, ATTRIBUTE_MACRO_PREFIX)) {
-		if (!macro_ok) {
+		if (!(flags & READ_ATTR_MACRO_OK)) {
 			fprintf(stderr, "%s not allowed: %s:%d\n",
 				name, src, lineno);
 			return NULL;
@@ -339,11 +342,11 @@ static void handle_attr_line(struct attr_stack *res,
 			     const char *line,
 			     const char *src,
 			     int lineno,
-			     int macro_ok)
+			     unsigned flags)
 {
 	struct match_attr *a;
 
-	a = parse_attr_line(line, src, lineno, macro_ok);
+	a = parse_attr_line(line, src, lineno, flags);
 	if (!a)
 		return;
 	ALLOC_GROW(res->attrs, res->num_matches + 1, res->alloc);
@@ -358,14 +361,15 @@ static struct attr_stack *read_attr_from_array(const char **list)
 
 	res = xcalloc(1, sizeof(*res));
 	while ((line = *(list++)) != NULL)
-		handle_attr_line(res, line, "[builtin]", ++lineno, 1);
+		handle_attr_line(res, line, "[builtin]", ++lineno,
+				 READ_ATTR_MACRO_OK);
 	return res;
 }
 
 static enum git_attr_direction direction;
 static struct index_state *use_index;
 
-static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
+static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)
 {
 	FILE *fp = fopen(path, "r");
 	struct attr_stack *res;
@@ -382,13 +386,13 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 		char *bufp = buf;
 		if (!lineno)
 			skip_utf8_bom(&bufp, strlen(bufp));
-		handle_attr_line(res, bufp, path, ++lineno, macro_ok);
+		handle_attr_line(res, bufp, path, ++lineno, flags);
 	}
 	fclose(fp);
 	return res;
 }
 
-static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
+static struct attr_stack *read_attr_from_index(const char *path, unsigned flags)
 {
 	struct attr_stack *res;
 	char *buf, *sp;
@@ -406,34 +410,34 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
 			;
 		more = (*ep == '\n');
 		*ep = '\0';
-		handle_attr_line(res, sp, path, ++lineno, macro_ok);
+		handle_attr_line(res, sp, path, ++lineno, flags);
 		sp = ep + more;
 	}
 	free(buf);
 	return res;
 }
 
-static struct attr_stack *read_attr(const char *path, int macro_ok)
+static struct attr_stack *read_attr(const char *path, unsigned flags)
 {
 	struct attr_stack *res;
 
 	if (direction == GIT_ATTR_CHECKOUT) {
-		res = read_attr_from_index(path, macro_ok);
+		res = read_attr_from_index(path, flags);
 		if (!res)
-			res = read_attr_from_file(path, macro_ok);
+			res = read_attr_from_file(path, flags);
 	}
 	else if (direction == GIT_ATTR_CHECKIN) {
-		res = read_attr_from_file(path, macro_ok);
+		res = read_attr_from_file(path, flags);
 		if (!res)
 			/*
 			 * There is no checked out .gitattributes file there, but
 			 * we might have it in the index.  We allow operation in a
 			 * sparsely checked out work tree, so read from it.
 			 */
-			res = read_attr_from_index(path, macro_ok);
+			res = read_attr_from_index(path, flags);
 	}
 	else
-		res = read_attr_from_index(path, macro_ok);
+		res = read_attr_from_index(path, flags);
 	if (!res)
 		res = xcalloc(1, sizeof(*res));
 	return res;
@@ -493,6 +497,7 @@ static GIT_PATH_FUNC(git_path_info_attributes, INFOATTRIBUTES_FILE)
 static void bootstrap_attr_stack(void)
 {
 	struct attr_stack *elem;
+	unsigned flags = READ_ATTR_MACRO_OK;
 
 	if (attr_stack)
 		return;
@@ -503,7 +508,7 @@ static void bootstrap_attr_stack(void)
 	attr_stack = elem;
 
 	if (git_attr_system()) {
-		elem = read_attr_from_file(git_etc_gitattributes(), 1);
+		elem = read_attr_from_file(git_etc_gitattributes(), flags);
 		if (elem) {
 			elem->origin = NULL;
 			elem->prev = attr_stack;
@@ -514,7 +519,7 @@ static void bootstrap_attr_stack(void)
 	if (!git_attributes_file)
 		git_attributes_file = xdg_config_home("attributes");
 	if (git_attributes_file) {
-		elem = read_attr_from_file(git_attributes_file, 1);
+		elem = read_attr_from_file(git_attributes_file, flags);
 		if (elem) {
 			elem->origin = NULL;
 			elem->prev = attr_stack;
@@ -523,7 +528,7 @@ static void bootstrap_attr_stack(void)
 	}
 
 	if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
-		elem = read_attr(GITATTRIBUTES_FILE, 1);
+		elem = read_attr(GITATTRIBUTES_FILE, flags);
 		elem->origin = xstrdup("");
 		elem->originlen = 0;
 		elem->prev = attr_stack;
@@ -532,7 +537,7 @@ static void bootstrap_attr_stack(void)
 	}
 
 	if (startup_info->have_repository)
-		elem = read_attr_from_file(git_path_info_attributes(), 1);
+		elem = read_attr_from_file(git_path_info_attributes(), flags);
 	else
 		elem = NULL;
 
-- 
2.11.0.rc0.258.gf434c15

[PATCH 3/5] exclude: convert "check_index" into a flags field

[Jeff King]

We pass the "check_index" flag through the variants of
add_excludes(). Let's turn this into a full flags bit-field,
so that we can add more flags to it without affecting the
function signature.

Note that only one caller actually needs to use the new flag
name, as the rest all were passing "0" already.

Signed-off-by: Jeff King <peff@peff.net>
---
 dir.c | 13 +++++++++----
 dir.h |  2 +-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/dir.c b/dir.c
index bfa8c8a9a..4fa1ca109 100644
--- a/dir.c
+++ b/dir.c
@@ -691,6 +691,9 @@ static void invalidate_directory(struct untracked_cache *uc,
 		dir->dirs[i]->recurse = 0;
 }
 
+/* Flags for add_excludes() */
+#define EXCLUDE_CHECK_INDEX (1<<0)
+
 /*
  * Given a file with name "fname", read it (either from disk, or from
  * the index if "check_index" is non-zero), parse it and store the
@@ -701,9 +704,10 @@ static void invalidate_directory(struct untracked_cache *uc,
  * ss_valid is non-zero, "ss" must contain good value as input.
  */
 static int add_excludes(const char *fname, const char *base, int baselen,
-			struct exclude_list *el, int check_index,
+			struct exclude_list *el, unsigned flags,
 			struct sha1_stat *sha1_stat)
 {
+	int check_index = !!(flags & EXCLUDE_CHECK_INDEX);
 	struct stat st;
 	int fd, i, lineno = 1;
 	size_t size = 0;
@@ -787,9 +791,9 @@ static int add_excludes(const char *fname, const char *base, int baselen,
 
 int add_excludes_from_file_to_list(const char *fname, const char *base,
 				   int baselen, struct exclude_list *el,
-				   int check_index)
+				   unsigned flags)
 {
-	return add_excludes(fname, base, baselen, el, check_index, NULL);
+	return add_excludes(fname, base, baselen, el, flags, NULL);
 }
 
 struct exclude_list *add_exclude_list(struct dir_struct *dir,
@@ -1125,7 +1129,8 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 			strbuf_addbuf(&sb, &dir->basebuf);
 			strbuf_addstr(&sb, dir->exclude_per_dir);
 			el->src = strbuf_detach(&sb, NULL);
-			add_excludes(el->src, el->src, stk->baselen, el, 1,
+			add_excludes(el->src, el->src, stk->baselen, el,
+				     EXCLUDE_CHECK_INDEX,
 				     untracked ? &sha1_stat : NULL);
 		}
 		/*
diff --git a/dir.h b/dir.h
index 97c83bb38..ba7eb924c 100644
--- a/dir.h
+++ b/dir.h
@@ -239,7 +239,7 @@ extern int is_excluded(struct dir_struct *dir, const char *name, int *dtype);
 extern struct exclude_list *add_exclude_list(struct dir_struct *dir,
 					     int group_type, const char *src);
 extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,
-					  struct exclude_list *el, int check_index);
+					  struct exclude_list *el, unsigned flags);
 extern void add_excludes_from_file(struct dir_struct *, const char *fname);
 extern void parse_exclude_pattern(const char **string, int *patternlen, unsigned *flags, int *nowildcardlen);
 extern void add_exclude(const char *string, const char *base,
-- 
2.11.0.rc0.258.gf434c15

[PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Jeff King]

The attributes system may sometimes read in-tree files from
the filesystem, and sometimes from the index. In the latter
case, we do not resolve symbolic links (and are not likely
to ever start doing so). Let's open filesystem links with
O_NOFOLLOW so that the two cases behave consistently.

As a bonus, this means that git will not follow such
symlinks to read and parse out-of-tree paths. It's unlikely
that this could have any security implications (a malicious
repository can already feed arbitrary content to the
attribute parser, and any disclosure of the out-of-tree
contents happens only to stderr). But it's one less oddball
thing to worry about.

Note that O_NOFOLLOW only prevents following links for the
path itself, not intermediate directories in the path.  At
first glance, it seems like

  ln -s /some/path in-repo

might still look at "in-repo/.gitattributes", following the
symlink to "/some/path/.gitattributes". However, if
"in-repo" is a symbolic link, then we know that it has no
git paths below it, and will never look at its
.gitattributes file.

We will continue to support out-of-tree symbolic links
(e.g., in $GIT_DIR/info/attributes); this just affects
in-tree links. When a symbolic link is encountered, the
contents are ignored and a warning is printed. POSIX
specifies ELOOP in this case, so the user would generally
see something like:

  warning: unable to access '.gitattributes': Too many levels of symbolic links

Signed-off-by: Jeff King <peff@peff.net>
---
 attr.c                | 17 +++++++++++++----
 t/t0003-attributes.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/attr.c b/attr.c
index 79bd89226..abf375c8a 100644
--- a/attr.c
+++ b/attr.c
@@ -153,6 +153,7 @@ static const char blank[] = " \t\r\n";
 
 /* Flags usable in read_attr() and parse_attr_line() family of functions. */
 #define READ_ATTR_MACRO_OK (1<<0)
+#define READ_ATTR_NOFOLLOW (1<<1)
 
 /*
  * Parse a whitespace-delimited attribute state (i.e., "attr",
@@ -371,16 +372,24 @@ static struct index_state *use_index;
 
 static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)
 {
-	FILE *fp = fopen(path, "r");
+	int fd;
+	FILE *fp;
 	struct attr_stack *res;
 	char buf[2048];
 	int lineno = 0;
 
-	if (!fp) {
+	if (flags & READ_ATTR_NOFOLLOW)
+		fd = open_nofollow(path, O_RDONLY);
+	else
+		fd = open(path, O_RDONLY);
+
+	if (fd < 0) {
 		if (errno != ENOENT && errno != ENOTDIR)
 			warn_on_inaccessible(path);
 		return NULL;
 	}
+	fp = xfdopen(fd, "r");
+
 	res = xcalloc(1, sizeof(*res));
 	while (fgets(buf, sizeof(buf), fp)) {
 		char *bufp = buf;
@@ -528,7 +537,7 @@ static void bootstrap_attr_stack(void)
 	}
 
 	if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
-		elem = read_attr(GITATTRIBUTES_FILE, flags);
+		elem = read_attr(GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW);
 		elem->origin = xstrdup("");
 		elem->originlen = 0;
 		elem->prev = attr_stack;
@@ -620,7 +629,7 @@ static void prepare_attr_stack(const char *path, int dirlen)
 			strbuf_add(&pathbuf, path, cp - path);
 			strbuf_addch(&pathbuf, '/');
 			strbuf_addstr(&pathbuf, GITATTRIBUTES_FILE);
-			elem = read_attr(pathbuf.buf, 0);
+			elem = read_attr(pathbuf.buf, READ_ATTR_NOFOLLOW);
 			strbuf_setlen(&pathbuf, cp - path);
 			elem->origin = strbuf_detach(&pathbuf, &elem->originlen);
 			elem->prev = attr_stack;
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index f0fbb4255..bd01945e5 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -297,4 +297,35 @@ test_expect_success 'bare repository: test info/attributes' '
 	)
 '
 
+check_symlink () {
+	echo "$1: symlink: $2" >expect &&
+	git check-attr symlink "$1" >actual &&
+	test_cmp expect actual
+}
+
+test_expect_success SYMLINKS 'set up attr file for symlink tests' '
+	echo "* symlink" >attr
+'
+
+test_expect_success SYMLINKS 'symlinks respected in core.attributesFile' '
+	test_when_finished "rm symlink" &&
+	ln -s attr symlink &&
+	test_config core.attributesFile "$(pwd)/symlink" &&
+	check_symlink file set
+'
+
+test_expect_success SYMLINKS 'symlinks respected in info/attributes' '
+	test_when_finished "rm .git/info/attributes" &&
+	ln -s ../../attr .git/info/attributes &&
+	check_symlink file set
+'
+
+test_expect_success SYMLINKS 'symlinks not respected in-tree' '
+	test_when_finished "rm .gitattributes" &&
+	ln -sf attr .gitattributes &&
+	mkdir subdir &&
+	ln -sf ../attr subdir/.gitattributes &&
+	check_symlink subdir/file unspecified
+'
+
 test_done
-- 
2.11.0.rc0.258.gf434c15

[PATCH 5/5] exclude: do not respect symlinks for in-tree .gitignore

[Jeff King]

Like .gitattributes, we would like to make sure that
.gitignore files are handled consistently whether read from
the index or from the filesystem. We can do so by using
O_NOFOLLOW when opening the files.

Signed-off-by: Jeff King <peff@peff.net>
---
 dir.c              |  9 +++++++--
 t/t0008-ignores.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 4fa1ca109..cf3fde005 100644
--- a/dir.c
+++ b/dir.c
@@ -693,6 +693,7 @@ static void invalidate_directory(struct untracked_cache *uc,
 
 /* Flags for add_excludes() */
 #define EXCLUDE_CHECK_INDEX (1<<0)
+#define EXCLUDE_NOFOLLOW (1<<1)
 
 /*
  * Given a file with name "fname", read it (either from disk, or from
@@ -713,7 +714,11 @@ static int add_excludes(const char *fname, const char *base, int baselen,
 	size_t size = 0;
 	char *buf, *entry;
 
-	fd = open(fname, O_RDONLY);
+	if (flags & EXCLUDE_NOFOLLOW)
+		fd = open_nofollow(fname, O_RDONLY);
+	else
+		fd = open(fname, O_RDONLY);
+
 	if (fd < 0 || fstat(fd, &st) < 0) {
 		if (errno != ENOENT)
 			warn_on_inaccessible(fname);
@@ -1130,7 +1135,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 			strbuf_addstr(&sb, dir->exclude_per_dir);
 			el->src = strbuf_detach(&sb, NULL);
 			add_excludes(el->src, el->src, stk->baselen, el,
-				     EXCLUDE_CHECK_INDEX,
+				     EXCLUDE_CHECK_INDEX | EXCLUDE_NOFOLLOW,
 				     untracked ? &sha1_stat : NULL);
 		}
 		/*
diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index d27f438bf..7348b8e6a 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -841,4 +841,33 @@ test_expect_success 'info/exclude trumps core.excludesfile' '
 	test_cmp expect actual
 '
 
+test_expect_success SYMLINKS 'set up ignore file for symlink tests' '
+	echo "*" >ignore
+'
+
+test_expect_success SYMLINKS 'symlinks respected in core.excludesFile' '
+	test_when_finished "rm symlink" &&
+	ln -s ignore symlink &&
+	test_config core.excludesFile "$(pwd)/symlink" &&
+	echo file >expect &&
+	git check-ignore file >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success SYMLINKS 'symlinks respected in info/exclude' '
+	test_when_finished "rm .git/info/exclude" &&
+	ln -sf ../../ignore .git/info/exclude &&
+	echo file >expect &&
+	git check-ignore file >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success SYMLINKS 'symlinks not respected in-tree' '
+	test_when_finished "rm .gitignore" &&
+	ln -sf ignore .gitignore &&
+	>expect &&
+	test_must_fail git check-ignore file >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.rc0.258.gf434c15

Re: [PATCH 0/5] disallow symlinks for .gitignore and .gitattributes

[Stefan Beller]

On Wed, Nov 2, 2016 at 6:04 AM, Jeff King <peff@peff.net> wrote:

>
>  attr.c                | 58 ++++++++++++++++++++++++++++++++-------------------

$ git diff --stat origin/master..origin/sb/attr  |grep attr.c
 attr.c                                             |  531 +-

From a cursory read of your series this may result in a merge
conflict, but would be
easily fixable (changed signature of functions that clash).

Re: [PATCH 0/5] disallow symlinks for .gitignore and .gitattributes

[Jeff King]

On Wed, Nov 02, 2016 at 12:46:28PM -0700, Stefan Beller wrote:

> On Wed, Nov 2, 2016 at 6:04 AM, Jeff King <peff@peff.net> wrote:
> 
> >
> >  attr.c                | 58 ++++++++++++++++++++++++++++++++-------------------
> 
> $ git diff --stat origin/master..origin/sb/attr  |grep attr.c
>  attr.c                                             |  531 +-
> 
> From a cursory read of your series this may result in a merge
> conflict, but would be
> easily fixable (changed signature of functions that clash).

Yeah, I knew you guys were doing some refactoring of the attribute code
elsewhere, but hadn't actually seen how bad the damage was. I just did
the merge with sb/attr, though, and the conflicts are quite trivial
(mostly s/1/flags/ in bootstrap_attr_stack()).

I'm happy to re-roll on a different base if sb/attr graduates first, but
I suspect Junio can just resolve the conflicts at merge time.

-Peff

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Duy Nguyen]

On Wed, Nov 2, 2016 at 8:08 PM, Jeff King <peff@peff.net> wrote:
> The attributes system may sometimes read in-tree files from
> the filesystem, and sometimes from the index. In the latter
> case, we do not resolve symbolic links (and are not likely
> to ever start doing so). Let's open filesystem links with
> O_NOFOLLOW so that the two cases behave consistently.

This sounds backward to me. The major use case is reading
.gitattributes on worktree, which follows symlinks so far. Only
git-archive has a special need to read index-only versions. The
worktree behavior should influence the in-index one, not the other way
around. If we could die("BUG" when git-archive is used on symlinks
(without --worktree-attributes). If people are annoyed by it, they can
implement symlink folllowing (to another version in index, not on
worktree).

The story is similar for .gitignore where in-index version is merely
an optimization. If it's symlinks and we can't follow, we should fall
back to worktree version.
-- 
Duy

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Jeff King]

On Mon, Nov 07, 2016 at 05:03:42PM +0700, Duy Nguyen wrote:

> On Wed, Nov 2, 2016 at 8:08 PM, Jeff King <peff@peff.net> wrote:
> > The attributes system may sometimes read in-tree files from
> > the filesystem, and sometimes from the index. In the latter
> > case, we do not resolve symbolic links (and are not likely
> > to ever start doing so). Let's open filesystem links with
> > O_NOFOLLOW so that the two cases behave consistently.
> 
> This sounds backward to me. The major use case is reading
> .gitattributes on worktree, which follows symlinks so far. Only
> git-archive has a special need to read index-only versions. The
> worktree behavior should influence the in-index one, not the other way
> around. If we could die("BUG" when git-archive is used on symlinks
> (without --worktree-attributes). If people are annoyed by it, they can
> implement symlink folllowing (to another version in index, not on
> worktree).

I agree it feels a little backwards, as we are choosing the
lowest-common denominator of the two (so it would be reasonable to have
the in-index version follow symbolic links, or at least do so on
platforms where core.symlinks is true).

And I'll admit my main motivation is not that index/filesystem parity,
but rather just that:

  git clone git://host.com/malicious-repo.git
  git log

might create and read symlinks to arbitrary files on the cloner's box.
I'm not sure to what degree to be worried about that. It's not like you
can't make other arbitrary symlinks which are likely to be read if the
user actually starts looking at checked-out files. It's just that we
usually try to make a clone+log of a malicious repository safe.

That being said, I'm not convinced that reading the index version of
.gitattributes and .gitignore is just an optimization. Don't we read the
destination attributes when checking out a new tree? And doesn't merge
need to use the in-index version when we see conflicts?

So I was hoping that this was a practice that is unlikely to be in wide
use, and that we could simply ban in order to keep the attribute and
ignore code simpler and safer, both now and if we change them to do more
in-index lookups.

-Peff

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Jeff King]

On Mon, Nov 07, 2016 at 04:10:10PM -0500, Jeff King wrote:

> And I'll admit my main motivation is not that index/filesystem parity,
> but rather just that:
> 
>   git clone git://host.com/malicious-repo.git
>   git log
> 
> might create and read symlinks to arbitrary files on the cloner's box.
> I'm not sure to what degree to be worried about that. It's not like you
> can't make other arbitrary symlinks which are likely to be read if the
> user actually starts looking at checked-out files. It's just that we
> usually try to make a clone+log of a malicious repository safe.

Another approach is to have a config option to disallow symlinks to
destinations outside of the repository tree (I'm not sure if it should
be on or off by default, though).

Again, I don't know that there is a specific security issue, but it
makes things easier for services which might clone untrusted
repositories (e.g., things like CI). They'd obviously have to be careful
with the contents of the repositories anyway, but it's one less thing to
have to worry about.

-Peff

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Duy Nguyen]

On Tue, Nov 8, 2016 at 4:15 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Nov 07, 2016 at 04:10:10PM -0500, Jeff King wrote:
>
>> And I'll admit my main motivation is not that index/filesystem parity,
>> but rather just that:
>>
>>   git clone git://host.com/malicious-repo.git
>>   git log
>>
>> might create and read symlinks to arbitrary files on the cloner's box.
>> I'm not sure to what degree to be worried about that. It's not like you
>> can't make other arbitrary symlinks which are likely to be read if the
>> user actually starts looking at checked-out files. It's just that we
>> usually try to make a clone+log of a malicious repository safe.

This I can buy.

> Another approach is to have a config option to disallow symlinks to
> destinations outside of the repository tree (I'm not sure if it should
> be on or off by default, though).

Let's err on the safe side and disable symlinks to outside repo by
default (or even all symlinks on .gitattributes and .gitignore as the
first step)

What I learned from my changes in .gitignore is, if we have not
forbidden something, people likely find some creative use for it. As
long as it's can be turned on or off, i guess those minority will stay
happy.

> Again, I don't know that there is a specific security issue, but it
> makes things easier for services which might clone untrusted
> repositories (e.g., things like CI). They'd obviously have to be careful
> with the contents of the repositories anyway, but it's one less thing to
> have to worry about.
>
> -Peff



-- 
Duy

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Jeff King]

On Tue, Nov 08, 2016 at 08:38:55AM +0700, Duy Nguyen wrote:

> > Another approach is to have a config option to disallow symlinks to
> > destinations outside of the repository tree (I'm not sure if it should
> > be on or off by default, though).
> 
> Let's err on the safe side and disable symlinks to outside repo by
> default (or even all symlinks on .gitattributes and .gitignore as the
> first step)

Both of those are actually much harder than you might think.

For matching specific names, we have to deal with case-folding.  It's
easy to hit the common ones like ".GITIGNORE" with fspathcmp(). But if
this is actually protection against malicious repositories, we have to
match all of the horrible filesystem-specific junk that we did for
".git".

Symlinks are likewise tricky.  If we see that a symlink points to
"foo/../bar", then we don't know if it leaves the repository unless we
also look at "foo" to see if it is also a symlink. So you really end up
having to resolve the symlink yourself (and when checking out multiple
files, there's an ordering dependency).

I think it might be enough to check:

  - leading "../" tokens in the symlink's destination can be checked
    against the symlink's path. So "../foo" is OK for path "one/two",
    but not for path "one".

  - interior "../" can be disallowed entirely. Technically
    "foo/../bar/../baz" _can_ be a fine symlink destination, but why?
    It's identical to "baz" unless you are following a bunch of interior
    symlinks. And if those are interior symlinks, it's still confusing
    and unnecessarily obfuscated, and a good sign that somebody is
    trying to do something tricky.

So one reasonable fix might be to have a config option like
"core.saneSymlinks" that enforces both of those rules for _all_ symlinks
that we checkout to the working tree. And it could either refuse to
check them out, or replace them with a file containing the symlink
content (as we do on systems that don't support symlinks, IIRC).

It could even be off by default (for backwards compatibility, as there
really are uses for symlinks reaching out of the repository in some
cases), but people cloning untrusted repos could flip it on. That seems
like an improvement over the current state.

> What I learned from my changes in .gitignore is, if we have not
> forbidden something, people likely find some creative use for it. As
> long as it's can be turned on or off, i guess those minority will stay
> happy.

Yes, it's one of the fun things about working on a 10-year-old project.
:)

-Peff

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Duy Nguyen]

On Wed, Nov 9, 2016 at 5:21 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 08, 2016 at 08:38:55AM +0700, Duy Nguyen wrote:
>
>> > Another approach is to have a config option to disallow symlinks to
>> > destinations outside of the repository tree (I'm not sure if it should
>> > be on or off by default, though).
>>
>> Let's err on the safe side and disable symlinks to outside repo by
>> default (or even all symlinks on .gitattributes and .gitignore as the
>> first step)
>
> Both of those are actually much harder than you might think.
>
> For matching specific names, we have to deal with case-folding.  It's
> easy to hit the common ones like ".GITIGNORE" with fspathcmp(). But if
> this is actually protection against malicious repositories, we have to
> match all of the horrible filesystem-specific junk that we did for
> ".git".

We could realpath() it and check if the result path is inside
realpath($GIT_WORK_TREE). The real work would be done by OS. We will
need to check if it points to .git/something, but I think we have that
covered. The approach is a bit heavy for such a sanity check though

> Symlinks are likewise tricky.  If we see that a symlink points to
> "foo/../bar", then we don't know if it leaves the repository unless we
> also look at "foo" to see if it is also a symlink. So you really end up
> having to resolve the symlink yourself (and when checking out multiple
> files, there's an ordering dependency).

We do have this dependency problem right now (e.g. files A and
.gitattributes are checked out at the same time and .gitattributes has
some attribute on A). It looks like we resolve it by reading the index
version at checkout time. We probably can do the same for gitattribute
symlinks.

> I think it might be enough to check:
>
>   - leading "../" tokens in the symlink's destination can be checked
>     against the symlink's path. So "../foo" is OK for path "one/two",
>     but not for path "one".
>
>   - interior "../" can be disallowed entirely. Technically
>     "foo/../bar/../baz" _can_ be a fine symlink destination, but why?
>     It's identical to "baz" unless you are following a bunch of interior
>     symlinks. And if those are interior symlinks, it's still confusing
>     and unnecessarily obfuscated, and a good sign that somebody is
>     trying to do something tricky.

Sounds good.

> So one reasonable fix might be to have a config option like
> "core.saneSymlinks" that enforces both of those rules for _all_ symlinks
> that we checkout to the working tree. And it could either refuse to
> check them out, or replace them with a file containing the symlink
> content (as we do on systems that don't support symlinks, IIRC).

I wonder if anyone want core.saneSymlinks on, but they have some links
that do not meet the above checks and still want to follow them
anyway. One way to add such an exception is mark the path with an
attribute "follow". Yeah I have a dependency loop :(
-- 
Duy

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Jeff King]

On Wed, Nov 09, 2016 at 04:22:12PM +0700, Duy Nguyen wrote:

> > Symlinks are likewise tricky.  If we see that a symlink points to
> > "foo/../bar", then we don't know if it leaves the repository unless we
> > also look at "foo" to see if it is also a symlink. So you really end up
> > having to resolve the symlink yourself (and when checking out multiple
> > files, there's an ordering dependency).
> 
> We do have this dependency problem right now (e.g. files A and
> .gitattributes are checked out at the same time and .gitattributes has
> some attribute on A). It looks like we resolve it by reading the index
> version at checkout time. We probably can do the same for gitattribute
> symlinks.

Right, but then we can't use filesystem functions like realpath() to do
the lookup. I guess we could do a pass after the checkout is done to
"fix" any out-of-tree symlinks we just created.

This is exactly the sort of complexity I was trying to avoid with my
original series. :)

If that isn't an option, I think I prefer something like the
core.saneSymlinks approach I mentioned. It has the additional bonus of
protecting not just git commands, but other commands that might inspect
the filesystem.

> > So one reasonable fix might be to have a config option like
> > "core.saneSymlinks" that enforces both of those rules for _all_ symlinks
> > that we checkout to the working tree. And it could either refuse to
> > check them out, or replace them with a file containing the symlink
> > content (as we do on systems that don't support symlinks, IIRC).
> 
> I wonder if anyone want core.saneSymlinks on, but they have some links
> that do not meet the above checks and still want to follow them
> anyway. One way to add such an exception is mark the path with an
> attribute "follow". Yeah I have a dependency loop :(

That could come later if somebody wants it, I think (especially if the
config option is not on by default). I have a feeling that callers will
either care about out-of-tree symlinks or not. Trusting the repository
to say "but these ones are OK" doesn't work for the paranoid ones, and
everybody else just assumes the repository is sane.

-Peff

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> For matching specific names, we have to deal with case-folding.  It's
> easy to hit the common ones like ".GITIGNORE" with fspathcmp(). But if
> this is actually protection against malicious repositories, we have to
> match all of the horrible filesystem-specific junk that we did for
> ".git".
>
> Symlinks are likewise tricky.

Wouldn't it be the simplest to say these:

 (1) The code attempts to read ".gitignore" (or ".git<something>")
     in general from the filesystem, or the index, or a tree. No
     case permutations are attempted.

 (2) When the code tries to do the above, we open with nofollow (or
     protect racily with lstat(2) which may be the best we could do)
     when reading from the filesystem, or check the ce_mode type
     when reading from the index or from a tree, and ignore if the
     path we are using is a symbolic link.

That way, case funny filesystems that cause trouble like the ".git"
thing would not have a chance to interfere and fool us, no?

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Jeff King]

On Wed, Nov 09, 2016 at 12:41:22PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > For matching specific names, we have to deal with case-folding.  It's
> > easy to hit the common ones like ".GITIGNORE" with fspathcmp(). But if
> > this is actually protection against malicious repositories, we have to
> > match all of the horrible filesystem-specific junk that we did for
> > ".git".
> >
> > Symlinks are likewise tricky.
> 
> Wouldn't it be the simplest to say these:
> 
>  (1) The code attempts to read ".gitignore" (or ".git<something>")
>      in general from the filesystem, or the index, or a tree. No
>      case permutations are attempted.
> 
>  (2) When the code tries to do the above, we open with nofollow (or
>      protect racily with lstat(2) which may be the best we could do)
>      when reading from the filesystem, or check the ce_mode type
>      when reading from the index or from a tree, and ignore if the
>      path we are using is a symbolic link.
> 
> That way, case funny filesystems that cause trouble like the ".git"
> thing would not have a chance to interfere and fool us, no?

That's what my series does. The question is just whether people will be
unhappy with (2), because they are using symbolic links in their
.gitignore files.

-Peff

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> Let's err on the safe side and disable symlinks to outside repo by
> default (or even all symlinks on .gitattributes and .gitignore as the
> first step)
>
> What I learned from my changes in .gitignore is, if we have not
> forbidden something, people likely find some creative use for it.

Yup.  Supporting any symlink in-tree is like requiring Git to be
used only on symlink-capable filesystems.  Not allowing it sounds
like a very sensible option and unlike true contents, there is no
downside to give that limitation to things like .git<anything>.

Shouldn't we do the same for .gitmodules while we are at it?

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Jeff King]

On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:

> Duy Nguyen <pclouds@gmail.com> writes:
> 
> > Let's err on the safe side and disable symlinks to outside repo by
> > default (or even all symlinks on .gitattributes and .gitignore as the
> > first step)
> >
> > What I learned from my changes in .gitignore is, if we have not
> > forbidden something, people likely find some creative use for it.
> 
> Yup.  Supporting any symlink in-tree is like requiring Git to be
> used only on symlink-capable filesystems.  Not allowing it sounds
> like a very sensible option and unlike true contents, there is no
> downside to give that limitation to things like .git<anything>.

I'm slightly confused. Did you mean "supporting any in-tree symlink to
an out-of-tree destination" in your first sentence?

> Shouldn't we do the same for .gitmodules while we are at it?

Good catch. Though I am inclined to have a flag that just covers all
out-of-tree symlinks, regardless of names.

-Peff

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:
>
> I'm slightly confused. Did you mean "supporting any in-tree symlink to
> an out-of-tree destination" in your first sentence?

I was trying to say that these "control files used solely by git"
have no business being a symbolic link pointing at anywhere, even
inside the same tree; actually, especially if it is inside the same
tree.

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Jeff King]

On Wed, Nov 09, 2016 at 04:18:29PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:
> >
> > I'm slightly confused. Did you mean "supporting any in-tree symlink to
> > an out-of-tree destination" in your first sentence?
> 
> I was trying to say that these "control files used solely by git"
> have no business being a symbolic link pointing at anywhere, even
> inside the same tree; actually, especially if it is inside the same
> tree.

OK. That is what my patch does (modulo .gitmodules, which I did not
think of). But I think that is the opposite of Duy's opinion, as his
review seemed to object to that.

As you know my ulterior motive is dealing with malicious out-of-tree
symlinks, and I would be happy to deal with that directly. That still
leaves symlinked ".gitmodules" etc in a funny state (they work in the
filesystem but not in the index), but since nobody is _complaining_,
it's a bug we could leave for another day.

So I dunno.

-Peff

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Duy Nguyen]

On Thu, Nov 10, 2016 at 7:23 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Nov 09, 2016 at 04:18:29PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:
>> >
>> > I'm slightly confused. Did you mean "supporting any in-tree symlink to
>> > an out-of-tree destination" in your first sentence?
>>
>> I was trying to say that these "control files used solely by git"
>> have no business being a symbolic link pointing at anywhere, even
>> inside the same tree; actually, especially if it is inside the same
>> tree.
>
> OK. That is what my patch does (modulo .gitmodules, which I did not
> think of). But I think that is the opposite of Duy's opinion, as his
> review seemed to object to that.

I only objected the rationale (to be consistent with reading index).
If you sold it as malicious symlinks, or even put it like Junio "no,
the design makes more sense to be this way", I would be ok.

On the implementation side, we should print something friendlier than
strerror(ELOOP) if we decide that "symlinks on .git* files are wrong".
The standard ELOOP message does not communicate our design decision
well to the users. But this is a minor thing and can be ignored.

> As you know my ulterior motive is dealing with malicious out-of-tree
> symlinks, and I would be happy to deal with that directly. That still
> leaves symlinked ".gitmodules" etc in a funny state (they work in the
> filesystem but not in the index), but since nobody is _complaining_,
> it's a bug we could leave for another day.

The discussion trailed off a bit to symlinks in general in worktree
too, I think. But it's not my itch, we can leave it for another day.
-- 
Duy