threads / patch / 44412

patch, 5 partsdisallow symlinks for .gitignore and .gitattributes

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

## tl;dr

22 messages between Nov 2, 2016 and Nov 10, 2016. Diffs are folded; open one to read it.

replies: 21people: 4as markdown or json

Jeff King· Nov 2, 2016, 13:04 UTC · lore

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
Jeff King· Nov 2, 2016, 13:06 UTC · re: Jeff King · lore

[PATCH 1/5] add open_nofollow() helper

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(+)
Show changes to 2 files +11 −0

git-compat-util.h, wrapper.c

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
Jeff King· Nov 2, 2016, 13:06 UTC · re: Jeff King · lore

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

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(-)
Show changes to attr.c +24 −19
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
Jeff King· Nov 2, 2016, 13:07 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +10 −5

dir.c, dir.h

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
Jeff King· Nov 2, 2016, 13:08 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +44 −4

attr.c, t/t0003-attributes.sh

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
Duy Nguyen· Nov 7, 2016, 10:03 UTC · re: Jeff King · lore

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

On Wed, Nov 2, 2016 at 8:08 PM, Jeff King <peff@peff.net> wrote:
Show 5 quoted lines
> 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
Jeff King· Nov 7, 2016, 21:10 UTC · re: Duy Nguyen · lore

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

On Mon, Nov 07, 2016 at 05:03:42PM +0700, Duy Nguyen wrote:
Show 15 quoted lines
> 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
Jeff King· Nov 7, 2016, 21:15 UTC · re: Jeff King · lore

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

On Mon, Nov 07, 2016 at 04:10:10PM -0500, Jeff King wrote:
Show 11 quoted lines
> 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
Duy Nguyen· Nov 8, 2016, 01:38 UTC · re: Jeff King · lore

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

On Tue, Nov 8, 2016 at 4:15 AM, Jeff King <peff@peff.net> wrote:
Show 13 quoted lines
> 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.

Show 7 quoted lines
> 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
Jeff King· Nov 8, 2016, 22:21 UTC · re: Duy Nguyen · lore

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

On Tue, Nov 08, 2016 at 08:38:55AM +0700, Duy Nguyen wrote:
Show 7 quoted lines
> > 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
Duy Nguyen· Nov 9, 2016, 09:22 UTC · re: Jeff King · lore

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

On Wed, Nov 9, 2016 at 5:21 AM, Jeff King <peff@peff.net> wrote:
Show 17 quoted lines
> 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

Show 5 quoted lines
> 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.

Show 12 quoted lines
> 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.
Show 5 quoted lines
> 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
Jeff King· Nov 9, 2016, 16:45 UTC · re: Duy Nguyen · lore

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

On Wed, Nov 09, 2016 at 04:22:12PM +0700, Duy Nguyen wrote:
Show 11 quoted lines
> > 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.

Show 10 quoted lines
> > 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
Junio C Hamano· Nov 9, 2016, 20:41 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> 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?

Jeff King· Nov 9, 2016, 20:43 UTC · re: Junio C Hamano · lore

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

On Wed, Nov 09, 2016 at 12:41:22PM -0800, Junio C Hamano wrote:
Show 24 quoted lines
> 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
Junio C Hamano· Nov 9, 2016, 22:58 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
Show 6 quoted lines
> 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?
Jeff King· Nov 9, 2016, 23:17 UTC · re: Junio C Hamano · lore

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

On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> 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
Junio C Hamano· Nov 10, 2016, 00:18 UTC · re: Jeff King · lore

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

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.

Jeff King· Nov 10, 2016, 00:23 UTC · re: Junio C Hamano · lore

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

On Wed, Nov 09, 2016 at 04:18:29PM -0800, Junio C Hamano wrote:
Show 11 quoted lines
> 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
Duy Nguyen· Nov 10, 2016, 11:00 UTC · re: Jeff King · lore

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

On Thu, Nov 10, 2016 at 7:23 AM, Jeff King <peff@peff.net> wrote:
Show 17 quoted lines
> 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.

Show 5 quoted lines
> 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
Jeff King· Nov 2, 2016, 13:09 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +36 −2

dir.c, t/t0008-ignores.sh

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
Stefan Beller· Nov 2, 2016, 19:46 UTC · re: Jeff King · lore

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

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).

Jeff King· Nov 2, 2016, 19:56 UTC · re: Stefan Beller · lore

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

On Wed, Nov 02, 2016 at 12:46:28PM -0700, Stefan Beller wrote:
Show 11 quoted lines
> 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

← back to recent threads