{"thread":{"id":"44412","subject":"[PATCH 0/5] disallow symlinks for .gitignore and .gitattributes","startedAt":"2016-11-02T13:05:00Z","lastAt":"2016-11-10T11:01:32Z","messageCount":22,"participants":["Jeff King","Stefan Beller","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"305286","messageId":"20161102130432.d3zprdul4sqgcfwu@sigill.intra.peff.net","threadId":"44412","inReplyTo":null,"subject":"[PATCH 0/5] disallow symlinks for .gitignore and .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-02T13:04:32Z","receivedAt":"2016-11-02T13:05:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I noticed in a nearby discussion that we will follow in-filesystem\nsymlinks for in-tree .gitignore and .gitattributes files, but not when\nthose files are read out of the index (e.g., when switching branches).\n\nThis series teaches git to open those files with O_NOFOLLOW (when it is\navailable) to get more consistent behavior. Note that this only applies\nto the in-tree versions; you can still symlink $GIT_DIR/info/attributes,\netc.\n\nI stopped short of warning about symlinked entries in git-fsck, but\nperhaps we would want to do that as well (doing it completely is tricky\nbecause of all of the case-folding issues around matching pathnames).\n\n  [1/5]: add open_nofollow() helper\n  [2/5]: attr: convert \"macro_ok\" into a flags field\n  [3/5]: exclude: convert \"check_index\" into a flags field\n  [4/5]: attr: do not respect symlinks for in-tree .gitattributes\n  [5/5]: exclude: do not respect symlinks for in-tree .gitignore\n\n attr.c                | 58 ++++++++++++++++++++++++++++++++-------------------\n dir.c                 | 20 +++++++++++++-----\n dir.h                 |  2 +-\n git-compat-util.h     |  3 +++\n t/t0003-attributes.sh | 31 +++++++++++++++++++++++++++\n t/t0008-ignores.sh    | 29 ++++++++++++++++++++++++++\n wrapper.c             |  8 +++++++\n 7 files changed, 123 insertions(+), 28 deletions(-)\n\n-Peff\n"},{"id":"305287","messageId":"20161102130629.ijpugqeawbkj3s57@sigill.intra.peff.net","threadId":"44412","inReplyTo":"20161102130432.d3zprdul4sqgcfwu@sigill.intra.peff.net","subject":"[PATCH 1/5] add open_nofollow() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-02T13:06:30Z","receivedAt":"2016-11-02T13:06:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some callers of open() would like to optionally use\nO_NOFOLLOW, but it is not available on all platforms. We\ncould abstract this by publicly defining O_NOFOLLOW to 0 on\nthose platforms, but that leaves us no room for any\nworkarounds (e.g., by checking the file type via lstat()).\n\nInstead, let's abstract it into its own function. We don't\nimplement any workarounds here, but it it would be easy to\nadd them later.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI didn't add the workaround because I think the current callers are OK\nwith \"best effort\", and doing it non-racily is quite tricky (though we\nmight also be OK with a racy version; we are not trying to beat\n/tmp races, but just making sure a checkout that we did is sane).\n\n git-compat-util.h | 3 +++\n wrapper.c         | 8 ++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 87237b092..a2cc33ebc 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1080,6 +1080,9 @@ int access_or_die(const char *path, int mode, unsigned flag);\n /* Warn on an inaccessible file that ought to be accessible */\n void warn_on_inaccessible(const char *path);\n \n+/* Open with O_NOFOLLOW, if available on this platform */\n+int open_nofollow(const char *path, int flags);\n+\n #ifdef GMTIME_UNRELIABLE_ERRORS\n struct tm *git_gmtime(const time_t *);\n struct tm *git_gmtime_r(const time_t *, struct tm *);\ndiff --git a/wrapper.c b/wrapper.c\nindex e7f197996..6bc7f24f5 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -679,3 +679,11 @@ void sleep_millisec(int millisec)\n {\n \tpoll(NULL, 0, millisec);\n }\n+\n+#ifndef O_NOFOLLOW\n+#define O_NOFOLLOW 0\n+#endif\n+int open_nofollow(const char *path, int flags)\n+{\n+\treturn open(path, flags | O_NOFOLLOW);\n+}\n-- \n2.11.0.rc0.258.gf434c15\n\n"},{"id":"305288","messageId":"20161102130644.4qwr4xr2ahljuny7@sigill.intra.peff.net","threadId":"44412","inReplyTo":"20161102130432.d3zprdul4sqgcfwu@sigill.intra.peff.net","subject":"[PATCH 2/5] attr: convert \"macro_ok\" into a flags field","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-02T13:06:44Z","receivedAt":"2016-11-02T13:06:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The attribute code can have a rather deep callstack, through\nwhich we have to pass the \"macro_ok\" flag. In anticipation\nof adding other flags, let's convert this to a generic\nbit-field.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n attr.c | 43 ++++++++++++++++++++++++-------------------\n 1 file changed, 24 insertions(+), 19 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 1fcf042b8..79bd89226 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -151,6 +151,9 @@ struct match_attr {\n \n static const char blank[] = \" \\t\\r\\n\";\n \n+/* Flags usable in read_attr() and parse_attr_line() family of functions. */\n+#define READ_ATTR_MACRO_OK (1<<0)\n+\n /*\n  * Parse a whitespace-delimited attribute state (i.e., \"attr\",\n  * \"-attr\", \"!attr\", or \"attr=value\") from the string starting at src.\n@@ -200,7 +203,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,\n }\n \n static struct match_attr *parse_attr_line(const char *line, const char *src,\n-\t\t\t\t\t  int lineno, int macro_ok)\n+\t\t\t\t\t  int lineno, unsigned flags)\n {\n \tint namelen;\n \tint num_attr, i;\n@@ -215,7 +218,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \tnamelen = strcspn(name, blank);\n \tif (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&\n \t    starts_with(name, ATTRIBUTE_MACRO_PREFIX)) {\n-\t\tif (!macro_ok) {\n+\t\tif (!(flags & READ_ATTR_MACRO_OK)) {\n \t\t\tfprintf(stderr, \"%s not allowed: %s:%d\\n\",\n \t\t\t\tname, src, lineno);\n \t\t\treturn NULL;\n@@ -339,11 +342,11 @@ static void handle_attr_line(struct attr_stack *res,\n \t\t\t     const char *line,\n \t\t\t     const char *src,\n \t\t\t     int lineno,\n-\t\t\t     int macro_ok)\n+\t\t\t     unsigned flags)\n {\n \tstruct match_attr *a;\n \n-\ta = parse_attr_line(line, src, lineno, macro_ok);\n+\ta = parse_attr_line(line, src, lineno, flags);\n \tif (!a)\n \t\treturn;\n \tALLOC_GROW(res->attrs, res->num_matches + 1, res->alloc);\n@@ -358,14 +361,15 @@ static struct attr_stack *read_attr_from_array(const char **list)\n \n \tres = xcalloc(1, sizeof(*res));\n \twhile ((line = *(list++)) != NULL)\n-\t\thandle_attr_line(res, line, \"[builtin]\", ++lineno, 1);\n+\t\thandle_attr_line(res, line, \"[builtin]\", ++lineno,\n+\t\t\t\t READ_ATTR_MACRO_OK);\n \treturn res;\n }\n \n static enum git_attr_direction direction;\n static struct index_state *use_index;\n \n-static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)\n+static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)\n {\n \tFILE *fp = fopen(path, \"r\");\n \tstruct attr_stack *res;\n@@ -382,13 +386,13 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)\n \t\tchar *bufp = buf;\n \t\tif (!lineno)\n \t\t\tskip_utf8_bom(&bufp, strlen(bufp));\n-\t\thandle_attr_line(res, bufp, path, ++lineno, macro_ok);\n+\t\thandle_attr_line(res, bufp, path, ++lineno, flags);\n \t}\n \tfclose(fp);\n \treturn res;\n }\n \n-static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)\n+static struct attr_stack *read_attr_from_index(const char *path, unsigned flags)\n {\n \tstruct attr_stack *res;\n \tchar *buf, *sp;\n@@ -406,34 +410,34 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)\n \t\t\t;\n \t\tmore = (*ep == '\\n');\n \t\t*ep = '\\0';\n-\t\thandle_attr_line(res, sp, path, ++lineno, macro_ok);\n+\t\thandle_attr_line(res, sp, path, ++lineno, flags);\n \t\tsp = ep + more;\n \t}\n \tfree(buf);\n \treturn res;\n }\n \n-static struct attr_stack *read_attr(const char *path, int macro_ok)\n+static struct attr_stack *read_attr(const char *path, unsigned flags)\n {\n \tstruct attr_stack *res;\n \n \tif (direction == GIT_ATTR_CHECKOUT) {\n-\t\tres = read_attr_from_index(path, macro_ok);\n+\t\tres = read_attr_from_index(path, flags);\n \t\tif (!res)\n-\t\t\tres = read_attr_from_file(path, macro_ok);\n+\t\t\tres = read_attr_from_file(path, flags);\n \t}\n \telse if (direction == GIT_ATTR_CHECKIN) {\n-\t\tres = read_attr_from_file(path, macro_ok);\n+\t\tres = read_attr_from_file(path, flags);\n \t\tif (!res)\n \t\t\t/*\n \t\t\t * There is no checked out .gitattributes file there, but\n \t\t\t * we might have it in the index.  We allow operation in a\n \t\t\t * sparsely checked out work tree, so read from it.\n \t\t\t */\n-\t\t\tres = read_attr_from_index(path, macro_ok);\n+\t\t\tres = read_attr_from_index(path, flags);\n \t}\n \telse\n-\t\tres = read_attr_from_index(path, macro_ok);\n+\t\tres = read_attr_from_index(path, flags);\n \tif (!res)\n \t\tres = xcalloc(1, sizeof(*res));\n \treturn res;\n@@ -493,6 +497,7 @@ static GIT_PATH_FUNC(git_path_info_attributes, INFOATTRIBUTES_FILE)\n static void bootstrap_attr_stack(void)\n {\n \tstruct attr_stack *elem;\n+\tunsigned flags = READ_ATTR_MACRO_OK;\n \n \tif (attr_stack)\n \t\treturn;\n@@ -503,7 +508,7 @@ static void bootstrap_attr_stack(void)\n \tattr_stack = elem;\n \n \tif (git_attr_system()) {\n-\t\telem = read_attr_from_file(git_etc_gitattributes(), 1);\n+\t\telem = read_attr_from_file(git_etc_gitattributes(), flags);\n \t\tif (elem) {\n \t\t\telem->origin = NULL;\n \t\t\telem->prev = attr_stack;\n@@ -514,7 +519,7 @@ static void bootstrap_attr_stack(void)\n \tif (!git_attributes_file)\n \t\tgit_attributes_file = xdg_config_home(\"attributes\");\n \tif (git_attributes_file) {\n-\t\telem = read_attr_from_file(git_attributes_file, 1);\n+\t\telem = read_attr_from_file(git_attributes_file, flags);\n \t\tif (elem) {\n \t\t\telem->origin = NULL;\n \t\t\telem->prev = attr_stack;\n@@ -523,7 +528,7 @@ static void bootstrap_attr_stack(void)\n \t}\n \n \tif (!is_bare_repository() || direction == GIT_ATTR_INDEX) {\n-\t\telem = read_attr(GITATTRIBUTES_FILE, 1);\n+\t\telem = read_attr(GITATTRIBUTES_FILE, flags);\n \t\telem->origin = xstrdup(\"\");\n \t\telem->originlen = 0;\n \t\telem->prev = attr_stack;\n@@ -532,7 +537,7 @@ static void bootstrap_attr_stack(void)\n \t}\n \n \tif (startup_info->have_repository)\n-\t\telem = read_attr_from_file(git_path_info_attributes(), 1);\n+\t\telem = read_attr_from_file(git_path_info_attributes(), flags);\n \telse\n \t\telem = NULL;\n \n-- \n2.11.0.rc0.258.gf434c15\n\n"},{"id":"305289","messageId":"20161102130709.5dm5hmrgknadf5zm@sigill.intra.peff.net","threadId":"44412","inReplyTo":"20161102130432.d3zprdul4sqgcfwu@sigill.intra.peff.net","subject":"[PATCH 3/5] exclude: convert \"check_index\" into a flags field","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-02T13:07:10Z","receivedAt":"2016-11-02T13:07:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We pass the \"check_index\" flag through the variants of\nadd_excludes(). Let's turn this into a full flags bit-field,\nso that we can add more flags to it without affecting the\nfunction signature.\n\nNote that only one caller actually needs to use the new flag\nname, as the rest all were passing \"0\" already.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n dir.c | 13 +++++++++----\n dir.h |  2 +-\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex bfa8c8a9a..4fa1ca109 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -691,6 +691,9 @@ static void invalidate_directory(struct untracked_cache *uc,\n \t\tdir->dirs[i]->recurse = 0;\n }\n \n+/* Flags for add_excludes() */\n+#define EXCLUDE_CHECK_INDEX (1<<0)\n+\n /*\n  * Given a file with name \"fname\", read it (either from disk, or from\n  * the index if \"check_index\" is non-zero), parse it and store the\n@@ -701,9 +704,10 @@ static void invalidate_directory(struct untracked_cache *uc,\n  * ss_valid is non-zero, \"ss\" must contain good value as input.\n  */\n static int add_excludes(const char *fname, const char *base, int baselen,\n-\t\t\tstruct exclude_list *el, int check_index,\n+\t\t\tstruct exclude_list *el, unsigned flags,\n \t\t\tstruct sha1_stat *sha1_stat)\n {\n+\tint check_index = !!(flags & EXCLUDE_CHECK_INDEX);\n \tstruct stat st;\n \tint fd, i, lineno = 1;\n \tsize_t size = 0;\n@@ -787,9 +791,9 @@ static int add_excludes(const char *fname, const char *base, int baselen,\n \n int add_excludes_from_file_to_list(const char *fname, const char *base,\n \t\t\t\t   int baselen, struct exclude_list *el,\n-\t\t\t\t   int check_index)\n+\t\t\t\t   unsigned flags)\n {\n-\treturn add_excludes(fname, base, baselen, el, check_index, NULL);\n+\treturn add_excludes(fname, base, baselen, el, flags, NULL);\n }\n \n struct exclude_list *add_exclude_list(struct dir_struct *dir,\n@@ -1125,7 +1129,8 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)\n \t\t\tstrbuf_addbuf(&sb, &dir->basebuf);\n \t\t\tstrbuf_addstr(&sb, dir->exclude_per_dir);\n \t\t\tel->src = strbuf_detach(&sb, NULL);\n-\t\t\tadd_excludes(el->src, el->src, stk->baselen, el, 1,\n+\t\t\tadd_excludes(el->src, el->src, stk->baselen, el,\n+\t\t\t\t     EXCLUDE_CHECK_INDEX,\n \t\t\t\t     untracked ? &sha1_stat : NULL);\n \t\t}\n \t\t/*\ndiff --git a/dir.h b/dir.h\nindex 97c83bb38..ba7eb924c 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -239,7 +239,7 @@ extern int is_excluded(struct dir_struct *dir, const char *name, int *dtype);\n extern struct exclude_list *add_exclude_list(struct dir_struct *dir,\n \t\t\t\t\t     int group_type, const char *src);\n extern int add_excludes_from_file_to_list(const char *fname, const char *base, int baselen,\n-\t\t\t\t\t  struct exclude_list *el, int check_index);\n+\t\t\t\t\t  struct exclude_list *el, unsigned flags);\n extern void add_excludes_from_file(struct dir_struct *, const char *fname);\n extern void parse_exclude_pattern(const char **string, int *patternlen, unsigned *flags, int *nowildcardlen);\n extern void add_exclude(const char *string, const char *base,\n-- \n2.11.0.rc0.258.gf434c15\n\n"},{"id":"305290","messageId":"20161102130848.qpigt4hxpoyfjf7x@sigill.intra.peff.net","threadId":"44412","inReplyTo":"20161102130432.d3zprdul4sqgcfwu@sigill.intra.peff.net","subject":"[PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-02T13:08:49Z","receivedAt":"2016-11-02T13:08:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The attributes system may sometimes read in-tree files from\nthe filesystem, and sometimes from the index. In the latter\ncase, we do not resolve symbolic links (and are not likely\nto ever start doing so). Let's open filesystem links with\nO_NOFOLLOW so that the two cases behave consistently.\n\nAs a bonus, this means that git will not follow such\nsymlinks to read and parse out-of-tree paths. It's unlikely\nthat this could have any security implications (a malicious\nrepository can already feed arbitrary content to the\nattribute parser, and any disclosure of the out-of-tree\ncontents happens only to stderr). But it's one less oddball\nthing to worry about.\n\nNote that O_NOFOLLOW only prevents following links for the\npath itself, not intermediate directories in the path.  At\nfirst glance, it seems like\n\n  ln -s /some/path in-repo\n\nmight still look at \"in-repo/.gitattributes\", following the\nsymlink to \"/some/path/.gitattributes\". However, if\n\"in-repo\" is a symbolic link, then we know that it has no\ngit paths below it, and will never look at its\n.gitattributes file.\n\nWe will continue to support out-of-tree symbolic links\n(e.g., in $GIT_DIR/info/attributes); this just affects\nin-tree links. When a symbolic link is encountered, the\ncontents are ignored and a warning is printed. POSIX\nspecifies ELOOP in this case, so the user would generally\nsee something like:\n\n  warning: unable to access '.gitattributes': Too many levels of symbolic links\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n attr.c                | 17 +++++++++++++----\n t/t0003-attributes.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 4 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 79bd89226..abf375c8a 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -153,6 +153,7 @@ static const char blank[] = \" \\t\\r\\n\";\n \n /* Flags usable in read_attr() and parse_attr_line() family of functions. */\n #define READ_ATTR_MACRO_OK (1<<0)\n+#define READ_ATTR_NOFOLLOW (1<<1)\n \n /*\n  * Parse a whitespace-delimited attribute state (i.e., \"attr\",\n@@ -371,16 +372,24 @@ static struct index_state *use_index;\n \n static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)\n {\n-\tFILE *fp = fopen(path, \"r\");\n+\tint fd;\n+\tFILE *fp;\n \tstruct attr_stack *res;\n \tchar buf[2048];\n \tint lineno = 0;\n \n-\tif (!fp) {\n+\tif (flags & READ_ATTR_NOFOLLOW)\n+\t\tfd = open_nofollow(path, O_RDONLY);\n+\telse\n+\t\tfd = open(path, O_RDONLY);\n+\n+\tif (fd < 0) {\n \t\tif (errno != ENOENT && errno != ENOTDIR)\n \t\t\twarn_on_inaccessible(path);\n \t\treturn NULL;\n \t}\n+\tfp = xfdopen(fd, \"r\");\n+\n \tres = xcalloc(1, sizeof(*res));\n \twhile (fgets(buf, sizeof(buf), fp)) {\n \t\tchar *bufp = buf;\n@@ -528,7 +537,7 @@ static void bootstrap_attr_stack(void)\n \t}\n \n \tif (!is_bare_repository() || direction == GIT_ATTR_INDEX) {\n-\t\telem = read_attr(GITATTRIBUTES_FILE, flags);\n+\t\telem = read_attr(GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW);\n \t\telem->origin = xstrdup(\"\");\n \t\telem->originlen = 0;\n \t\telem->prev = attr_stack;\n@@ -620,7 +629,7 @@ static void prepare_attr_stack(const char *path, int dirlen)\n \t\t\tstrbuf_add(&pathbuf, path, cp - path);\n \t\t\tstrbuf_addch(&pathbuf, '/');\n \t\t\tstrbuf_addstr(&pathbuf, GITATTRIBUTES_FILE);\n-\t\t\telem = read_attr(pathbuf.buf, 0);\n+\t\t\telem = read_attr(pathbuf.buf, READ_ATTR_NOFOLLOW);\n \t\t\tstrbuf_setlen(&pathbuf, cp - path);\n \t\t\telem->origin = strbuf_detach(&pathbuf, &elem->originlen);\n \t\t\telem->prev = attr_stack;\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex f0fbb4255..bd01945e5 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -297,4 +297,35 @@ test_expect_success 'bare repository: test info/attributes' '\n \t)\n '\n \n+check_symlink () {\n+\techo \"$1: symlink: $2\" >expect &&\n+\tgit check-attr symlink \"$1\" >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success SYMLINKS 'set up attr file for symlink tests' '\n+\techo \"* symlink\" >attr\n+'\n+\n+test_expect_success SYMLINKS 'symlinks respected in core.attributesFile' '\n+\ttest_when_finished \"rm symlink\" &&\n+\tln -s attr symlink &&\n+\ttest_config core.attributesFile \"$(pwd)/symlink\" &&\n+\tcheck_symlink file set\n+'\n+\n+test_expect_success SYMLINKS 'symlinks respected in info/attributes' '\n+\ttest_when_finished \"rm .git/info/attributes\" &&\n+\tln -s ../../attr .git/info/attributes &&\n+\tcheck_symlink file set\n+'\n+\n+test_expect_success SYMLINKS 'symlinks not respected in-tree' '\n+\ttest_when_finished \"rm .gitattributes\" &&\n+\tln -sf attr .gitattributes &&\n+\tmkdir subdir &&\n+\tln -sf ../attr subdir/.gitattributes &&\n+\tcheck_symlink subdir/file unspecified\n+'\n+\n test_done\n-- \n2.11.0.rc0.258.gf434c15\n\n"},{"id":"305291","messageId":"20161102130911.gj2h6cya3mzx6cwt@sigill.intra.peff.net","threadId":"44412","inReplyTo":"20161102130432.d3zprdul4sqgcfwu@sigill.intra.peff.net","subject":"[PATCH 5/5] exclude: do not respect symlinks for in-tree .gitignore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-02T13:09:11Z","receivedAt":"2016-11-02T13:09:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Like .gitattributes, we would like to make sure that\n.gitignore files are handled consistently whether read from\nthe index or from the filesystem. We can do so by using\nO_NOFOLLOW when opening the files.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n dir.c              |  9 +++++++--\n t/t0008-ignores.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 4fa1ca109..cf3fde005 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -693,6 +693,7 @@ static void invalidate_directory(struct untracked_cache *uc,\n \n /* Flags for add_excludes() */\n #define EXCLUDE_CHECK_INDEX (1<<0)\n+#define EXCLUDE_NOFOLLOW (1<<1)\n \n /*\n  * Given a file with name \"fname\", read it (either from disk, or from\n@@ -713,7 +714,11 @@ static int add_excludes(const char *fname, const char *base, int baselen,\n \tsize_t size = 0;\n \tchar *buf, *entry;\n \n-\tfd = open(fname, O_RDONLY);\n+\tif (flags & EXCLUDE_NOFOLLOW)\n+\t\tfd = open_nofollow(fname, O_RDONLY);\n+\telse\n+\t\tfd = open(fname, O_RDONLY);\n+\n \tif (fd < 0 || fstat(fd, &st) < 0) {\n \t\tif (errno != ENOENT)\n \t\t\twarn_on_inaccessible(fname);\n@@ -1130,7 +1135,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)\n \t\t\tstrbuf_addstr(&sb, dir->exclude_per_dir);\n \t\t\tel->src = strbuf_detach(&sb, NULL);\n \t\t\tadd_excludes(el->src, el->src, stk->baselen, el,\n-\t\t\t\t     EXCLUDE_CHECK_INDEX,\n+\t\t\t\t     EXCLUDE_CHECK_INDEX | EXCLUDE_NOFOLLOW,\n \t\t\t\t     untracked ? &sha1_stat : NULL);\n \t\t}\n \t\t/*\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex d27f438bf..7348b8e6a 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -841,4 +841,33 @@ test_expect_success 'info/exclude trumps core.excludesfile' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success SYMLINKS 'set up ignore file for symlink tests' '\n+\techo \"*\" >ignore\n+'\n+\n+test_expect_success SYMLINKS 'symlinks respected in core.excludesFile' '\n+\ttest_when_finished \"rm symlink\" &&\n+\tln -s ignore symlink &&\n+\ttest_config core.excludesFile \"$(pwd)/symlink\" &&\n+\techo file >expect &&\n+\tgit check-ignore file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success SYMLINKS 'symlinks respected in info/exclude' '\n+\ttest_when_finished \"rm .git/info/exclude\" &&\n+\tln -sf ../../ignore .git/info/exclude &&\n+\techo file >expect &&\n+\tgit check-ignore file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success SYMLINKS 'symlinks not respected in-tree' '\n+\ttest_when_finished \"rm .gitignore\" &&\n+\tln -sf ignore .gitignore &&\n+\t>expect &&\n+\ttest_must_fail git check-ignore file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.11.0.rc0.258.gf434c15\n"},{"id":"305313","messageId":"CAGZ79kY195Bff5bUVm44Bx7+XzDDehYbRxgdii4s040pQXRRSQ@mail.gmail.com","threadId":"44412","inReplyTo":"20161102130432.d3zprdul4sqgcfwu@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] disallow symlinks for .gitignore and .gitattributes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-02T19:46:28Z","receivedAt":"2016-11-02T19:46:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 2, 2016 at 6:04 AM, Jeff King <peff@peff.net> wrote:\n\n>\n>  attr.c                | 58 ++++++++++++++++++++++++++++++++-------------------\n\n$ git diff --stat origin/master..origin/sb/attr  |grep attr.c\n attr.c                                             |  531 +-\n\nFrom a cursory read of your series this may result in a merge\nconflict, but would be\neasily fixable (changed signature of functions that clash).\n"},{"id":"305314","messageId":"20161102195615.3muvgr2q2lnn7sg4@sigill.intra.peff.net","threadId":"44412","inReplyTo":"CAGZ79kY195Bff5bUVm44Bx7+XzDDehYbRxgdii4s040pQXRRSQ@mail.gmail.com","subject":"Re: [PATCH 0/5] disallow symlinks for .gitignore and .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-02T19:56:16Z","receivedAt":"2016-11-02T19:56:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 02, 2016 at 12:46:28PM -0700, Stefan Beller wrote:\n\n> On Wed, Nov 2, 2016 at 6:04 AM, Jeff King <peff@peff.net> wrote:\n> \n> >\n> >  attr.c                | 58 ++++++++++++++++++++++++++++++++-------------------\n> \n> $ git diff --stat origin/master..origin/sb/attr  |grep attr.c\n>  attr.c                                             |  531 +-\n> \n> From a cursory read of your series this may result in a merge\n> conflict, but would be\n> easily fixable (changed signature of functions that clash).\n\nYeah, I knew you guys were doing some refactoring of the attribute code\nelsewhere, but hadn't actually seen how bad the damage was. I just did\nthe merge with sb/attr, though, and the conflicts are quite trivial\n(mostly s/1/flags/ in bootstrap_attr_stack()).\n\nI'm happy to re-roll on a different base if sb/attr graduates first, but\nI suspect Junio can just resolve the conflicts at merge time.\n\n-Peff\n"},{"id":"305481","messageId":"CACsJy8AO2KtpxFu=wRjW1DoCA9bfpF1VoJUn__2ib-ML0XT66w@mail.gmail.com","threadId":"44412","inReplyTo":"20161102130848.qpigt4hxpoyfjf7x@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-11-07T10:03:42Z","receivedAt":"2016-11-07T10:04:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Nov 2, 2016 at 8:08 PM, Jeff King <peff@peff.net> wrote:\n> The attributes system may sometimes read in-tree files from\n> the filesystem, and sometimes from the index. In the latter\n> case, we do not resolve symbolic links (and are not likely\n> to ever start doing so). Let's open filesystem links with\n> O_NOFOLLOW so that the two cases behave consistently.\n\nThis sounds backward to me. The major use case is reading\n.gitattributes on worktree, which follows symlinks so far. Only\ngit-archive has a special need to read index-only versions. The\nworktree behavior should influence the in-index one, not the other way\naround. If we could die(\"BUG\" when git-archive is used on symlinks\n(without --worktree-attributes). If people are annoyed by it, they can\nimplement symlink folllowing (to another version in index, not on\nworktree).\n\nThe story is similar for .gitignore where in-index version is merely\nan optimization. If it's symlinks and we can't follow, we should fall\nback to worktree version.\n-- \nDuy\n"},{"id":"305514","messageId":"20161107211010.xo3243egggdgscou@sigill.intra.peff.net","threadId":"44412","inReplyTo":"CACsJy8AO2KtpxFu=wRjW1DoCA9bfpF1VoJUn__2ib-ML0XT66w@mail.gmail.com","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-07T21:10:10Z","receivedAt":"2016-11-07T21:10:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 07, 2016 at 05:03:42PM +0700, Duy Nguyen wrote:\n\n> On Wed, Nov 2, 2016 at 8:08 PM, Jeff King <peff@peff.net> wrote:\n> > The attributes system may sometimes read in-tree files from\n> > the filesystem, and sometimes from the index. In the latter\n> > case, we do not resolve symbolic links (and are not likely\n> > to ever start doing so). Let's open filesystem links with\n> > O_NOFOLLOW so that the two cases behave consistently.\n> \n> This sounds backward to me. The major use case is reading\n> .gitattributes on worktree, which follows symlinks so far. Only\n> git-archive has a special need to read index-only versions. The\n> worktree behavior should influence the in-index one, not the other way\n> around. If we could die(\"BUG\" when git-archive is used on symlinks\n> (without --worktree-attributes). If people are annoyed by it, they can\n> implement symlink folllowing (to another version in index, not on\n> worktree).\n\nI agree it feels a little backwards, as we are choosing the\nlowest-common denominator of the two (so it would be reasonable to have\nthe in-index version follow symbolic links, or at least do so on\nplatforms where core.symlinks is true).\n\nAnd I'll admit my main motivation is not that index/filesystem parity,\nbut rather just that:\n\n  git clone git://host.com/malicious-repo.git\n  git log\n\nmight create and read symlinks to arbitrary files on the cloner's box.\nI'm not sure to what degree to be worried about that. It's not like you\ncan't make other arbitrary symlinks which are likely to be read if the\nuser actually starts looking at checked-out files. It's just that we\nusually try to make a clone+log of a malicious repository safe.\n\nThat being said, I'm not convinced that reading the index version of\n.gitattributes and .gitignore is just an optimization. Don't we read the\ndestination attributes when checking out a new tree? And doesn't merge\nneed to use the in-index version when we see conflicts?\n\nSo I was hoping that this was a practice that is unlikely to be in wide\nuse, and that we could simply ban in order to keep the attribute and\nignore code simpler and safer, both now and if we change them to do more\nin-index lookups.\n\n-Peff\n"},{"id":"305515","messageId":"20161107211522.vzl4zpsu5cpembgc@sigill.intra.peff.net","threadId":"44412","inReplyTo":"20161107211010.xo3243egggdgscou@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-07T21:15:22Z","receivedAt":"2016-11-07T21:15:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 07, 2016 at 04:10:10PM -0500, Jeff King wrote:\n\n> And I'll admit my main motivation is not that index/filesystem parity,\n> but rather just that:\n> \n>   git clone git://host.com/malicious-repo.git\n>   git log\n> \n> might create and read symlinks to arbitrary files on the cloner's box.\n> I'm not sure to what degree to be worried about that. It's not like you\n> can't make other arbitrary symlinks which are likely to be read if the\n> user actually starts looking at checked-out files. It's just that we\n> usually try to make a clone+log of a malicious repository safe.\n\nAnother approach is to have a config option to disallow symlinks to\ndestinations outside of the repository tree (I'm not sure if it should\nbe on or off by default, though).\n\nAgain, I don't know that there is a specific security issue, but it\nmakes things easier for services which might clone untrusted\nrepositories (e.g., things like CI). They'd obviously have to be careful\nwith the contents of the repositories anyway, but it's one less thing to\nhave to worry about.\n\n-Peff\n"},{"id":"305531","messageId":"CACsJy8BoEXDjwe=ZX5ZOC_mvaMjYrB3i7wcMmiOP3mm5-rwC5Q@mail.gmail.com","threadId":"44412","inReplyTo":"20161107211522.vzl4zpsu5cpembgc@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-11-08T01:38:55Z","receivedAt":"2016-11-08T01:39:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 8, 2016 at 4:15 AM, Jeff King <peff@peff.net> wrote:\n> On Mon, Nov 07, 2016 at 04:10:10PM -0500, Jeff King wrote:\n>\n>> And I'll admit my main motivation is not that index/filesystem parity,\n>> but rather just that:\n>>\n>>   git clone git://host.com/malicious-repo.git\n>>   git log\n>>\n>> might create and read symlinks to arbitrary files on the cloner's box.\n>> I'm not sure to what degree to be worried about that. It's not like you\n>> can't make other arbitrary symlinks which are likely to be read if the\n>> user actually starts looking at checked-out files. It's just that we\n>> usually try to make a clone+log of a malicious repository safe.\n\nThis I can buy.\n\n> Another approach is to have a config option to disallow symlinks to\n> destinations outside of the repository tree (I'm not sure if it should\n> be on or off by default, though).\n\nLet's err on the safe side and disable symlinks to outside repo by\ndefault (or even all symlinks on .gitattributes and .gitignore as the\nfirst step)\n\nWhat I learned from my changes in .gitignore is, if we have not\nforbidden something, people likely find some creative use for it. As\nlong as it's can be turned on or off, i guess those minority will stay\nhappy.\n\n> Again, I don't know that there is a specific security issue, but it\n> makes things easier for services which might clone untrusted\n> repositories (e.g., things like CI). They'd obviously have to be careful\n> with the contents of the repositories anyway, but it's one less thing to\n> have to worry about.\n>\n> -Peff\n\n\n\n-- \nDuy\n"},{"id":"305591","messageId":"20161108222127.mejb74maewzhn3qg@sigill.intra.peff.net","threadId":"44412","inReplyTo":"CACsJy8BoEXDjwe=ZX5ZOC_mvaMjYrB3i7wcMmiOP3mm5-rwC5Q@mail.gmail.com","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-08T22:21:27Z","receivedAt":"2016-11-08T22:21:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 08, 2016 at 08:38:55AM +0700, Duy Nguyen wrote:\n\n> > Another approach is to have a config option to disallow symlinks to\n> > destinations outside of the repository tree (I'm not sure if it should\n> > be on or off by default, though).\n> \n> Let's err on the safe side and disable symlinks to outside repo by\n> default (or even all symlinks on .gitattributes and .gitignore as the\n> first step)\n\nBoth of those are actually much harder than you might think.\n\nFor matching specific names, we have to deal with case-folding.  It's\neasy to hit the common ones like \".GITIGNORE\" with fspathcmp(). But if\nthis is actually protection against malicious repositories, we have to\nmatch all of the horrible filesystem-specific junk that we did for\n\".git\".\n\nSymlinks are likewise tricky.  If we see that a symlink points to\n\"foo/../bar\", then we don't know if it leaves the repository unless we\nalso look at \"foo\" to see if it is also a symlink. So you really end up\nhaving to resolve the symlink yourself (and when checking out multiple\nfiles, there's an ordering dependency).\n\nI think it might be enough to check:\n\n  - leading \"../\" tokens in the symlink's destination can be checked\n    against the symlink's path. So \"../foo\" is OK for path \"one/two\",\n    but not for path \"one\".\n\n  - interior \"../\" can be disallowed entirely. Technically\n    \"foo/../bar/../baz\" _can_ be a fine symlink destination, but why?\n    It's identical to \"baz\" unless you are following a bunch of interior\n    symlinks. And if those are interior symlinks, it's still confusing\n    and unnecessarily obfuscated, and a good sign that somebody is\n    trying to do something tricky.\n\nSo one reasonable fix might be to have a config option like\n\"core.saneSymlinks\" that enforces both of those rules for _all_ symlinks\nthat we checkout to the working tree. And it could either refuse to\ncheck them out, or replace them with a file containing the symlink\ncontent (as we do on systems that don't support symlinks, IIRC).\n\nIt could even be off by default (for backwards compatibility, as there\nreally are uses for symlinks reaching out of the repository in some\ncases), but people cloning untrusted repos could flip it on. That seems\nlike an improvement over the current state.\n\n> What I learned from my changes in .gitignore is, if we have not\n> forbidden something, people likely find some creative use for it. As\n> long as it's can be turned on or off, i guess those minority will stay\n> happy.\n\nYes, it's one of the fun things about working on a 10-year-old project.\n:)\n\n-Peff\n"},{"id":"305620","messageId":"CACsJy8ASMBk+Yak7LyybANFYkoU_Poi1ZGY=ufKtq1vSkoYCXQ@mail.gmail.com","threadId":"44412","inReplyTo":"20161108222127.mejb74maewzhn3qg@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-11-09T09:22:12Z","receivedAt":"2016-11-09T09:22:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Nov 9, 2016 at 5:21 AM, Jeff King <peff@peff.net> wrote:\n> On Tue, Nov 08, 2016 at 08:38:55AM +0700, Duy Nguyen wrote:\n>\n>> > Another approach is to have a config option to disallow symlinks to\n>> > destinations outside of the repository tree (I'm not sure if it should\n>> > be on or off by default, though).\n>>\n>> Let's err on the safe side and disable symlinks to outside repo by\n>> default (or even all symlinks on .gitattributes and .gitignore as the\n>> first step)\n>\n> Both of those are actually much harder than you might think.\n>\n> For matching specific names, we have to deal with case-folding.  It's\n> easy to hit the common ones like \".GITIGNORE\" with fspathcmp(). But if\n> this is actually protection against malicious repositories, we have to\n> match all of the horrible filesystem-specific junk that we did for\n> \".git\".\n\nWe could realpath() it and check if the result path is inside\nrealpath($GIT_WORK_TREE). The real work would be done by OS. We will\nneed to check if it points to .git/something, but I think we have that\ncovered. The approach is a bit heavy for such a sanity check though\n\n> Symlinks are likewise tricky.  If we see that a symlink points to\n> \"foo/../bar\", then we don't know if it leaves the repository unless we\n> also look at \"foo\" to see if it is also a symlink. So you really end up\n> having to resolve the symlink yourself (and when checking out multiple\n> files, there's an ordering dependency).\n\nWe do have this dependency problem right now (e.g. files A and\n.gitattributes are checked out at the same time and .gitattributes has\nsome attribute on A). It looks like we resolve it by reading the index\nversion at checkout time. We probably can do the same for gitattribute\nsymlinks.\n\n> I think it might be enough to check:\n>\n>   - leading \"../\" tokens in the symlink's destination can be checked\n>     against the symlink's path. So \"../foo\" is OK for path \"one/two\",\n>     but not for path \"one\".\n>\n>   - interior \"../\" can be disallowed entirely. Technically\n>     \"foo/../bar/../baz\" _can_ be a fine symlink destination, but why?\n>     It's identical to \"baz\" unless you are following a bunch of interior\n>     symlinks. And if those are interior symlinks, it's still confusing\n>     and unnecessarily obfuscated, and a good sign that somebody is\n>     trying to do something tricky.\n\nSounds good.\n\n> So one reasonable fix might be to have a config option like\n> \"core.saneSymlinks\" that enforces both of those rules for _all_ symlinks\n> that we checkout to the working tree. And it could either refuse to\n> check them out, or replace them with a file containing the symlink\n> content (as we do on systems that don't support symlinks, IIRC).\n\nI wonder if anyone want core.saneSymlinks on, but they have some links\nthat do not meet the above checks and still want to follow them\nanyway. One way to add such an exception is mark the path with an\nattribute \"follow\". Yeah I have a dependency loop :(\n-- \nDuy\n"},{"id":"305637","messageId":"20161109164539.fn4c6fj3dyvceb7m@sigill.intra.peff.net","threadId":"44412","inReplyTo":"CACsJy8ASMBk+Yak7LyybANFYkoU_Poi1ZGY=ufKtq1vSkoYCXQ@mail.gmail.com","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-09T16:45:39Z","receivedAt":"2016-11-09T16:45:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 09, 2016 at 04:22:12PM +0700, Duy Nguyen wrote:\n\n> > Symlinks are likewise tricky.  If we see that a symlink points to\n> > \"foo/../bar\", then we don't know if it leaves the repository unless we\n> > also look at \"foo\" to see if it is also a symlink. So you really end up\n> > having to resolve the symlink yourself (and when checking out multiple\n> > files, there's an ordering dependency).\n> \n> We do have this dependency problem right now (e.g. files A and\n> .gitattributes are checked out at the same time and .gitattributes has\n> some attribute on A). It looks like we resolve it by reading the index\n> version at checkout time. We probably can do the same for gitattribute\n> symlinks.\n\nRight, but then we can't use filesystem functions like realpath() to do\nthe lookup. I guess we could do a pass after the checkout is done to\n\"fix\" any out-of-tree symlinks we just created.\n\nThis is exactly the sort of complexity I was trying to avoid with my\noriginal series. :)\n\nIf that isn't an option, I think I prefer something like the\ncore.saneSymlinks approach I mentioned. It has the additional bonus of\nprotecting not just git commands, but other commands that might inspect\nthe filesystem.\n\n> > So one reasonable fix might be to have a config option like\n> > \"core.saneSymlinks\" that enforces both of those rules for _all_ symlinks\n> > that we checkout to the working tree. And it could either refuse to\n> > check them out, or replace them with a file containing the symlink\n> > content (as we do on systems that don't support symlinks, IIRC).\n> \n> I wonder if anyone want core.saneSymlinks on, but they have some links\n> that do not meet the above checks and still want to follow them\n> anyway. One way to add such an exception is mark the path with an\n> attribute \"follow\". Yeah I have a dependency loop :(\n\nThat could come later if somebody wants it, I think (especially if the\nconfig option is not on by default). I have a feeling that callers will\neither care about out-of-tree symlinks or not. Trusting the repository\nto say \"but these ones are OK\" doesn't work for the paranoid ones, and\neverybody else just assumes the repository is sane.\n\n-Peff\n"},{"id":"305643","messageId":"xmqqd1i49yj1.fsf@gitster.mtv.corp.google.com","threadId":"44412","inReplyTo":"20161108222127.mejb74maewzhn3qg@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-09T20:41:22Z","receivedAt":"2016-11-09T20:41:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> For matching specific names, we have to deal with case-folding.  It's\n> easy to hit the common ones like \".GITIGNORE\" with fspathcmp(). But if\n> this is actually protection against malicious repositories, we have to\n> match all of the horrible filesystem-specific junk that we did for\n> \".git\".\n>\n> Symlinks are likewise tricky.\n\nWouldn't it be the simplest to say these:\n\n (1) The code attempts to read \".gitignore\" (or \".git<something>\")\n     in general from the filesystem, or the index, or a tree. No\n     case permutations are attempted.\n\n (2) When the code tries to do the above, we open with nofollow (or\n     protect racily with lstat(2) which may be the best we could do)\n     when reading from the filesystem, or check the ce_mode type\n     when reading from the index or from a tree, and ignore if the\n     path we are using is a symbolic link.\n\nThat way, case funny filesystems that cause trouble like the \".git\"\nthing would not have a chance to interfere and fool us, no?\n"},{"id":"305644","messageId":"20161109204351.dflnqifjn6div5at@sigill.intra.peff.net","threadId":"44412","inReplyTo":"xmqqd1i49yj1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-09T20:43:52Z","receivedAt":"2016-11-09T20:43:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 09, 2016 at 12:41:22PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > For matching specific names, we have to deal with case-folding.  It's\n> > easy to hit the common ones like \".GITIGNORE\" with fspathcmp(). But if\n> > this is actually protection against malicious repositories, we have to\n> > match all of the horrible filesystem-specific junk that we did for\n> > \".git\".\n> >\n> > Symlinks are likewise tricky.\n> \n> Wouldn't it be the simplest to say these:\n> \n>  (1) The code attempts to read \".gitignore\" (or \".git<something>\")\n>      in general from the filesystem, or the index, or a tree. No\n>      case permutations are attempted.\n> \n>  (2) When the code tries to do the above, we open with nofollow (or\n>      protect racily with lstat(2) which may be the best we could do)\n>      when reading from the filesystem, or check the ce_mode type\n>      when reading from the index or from a tree, and ignore if the\n>      path we are using is a symbolic link.\n> \n> That way, case funny filesystems that cause trouble like the \".git\"\n> thing would not have a chance to interfere and fool us, no?\n\nThat's what my series does. The question is just whether people will be\nunhappy with (2), because they are using symbolic links in their\n.gitignore files.\n\n-Peff\n"},{"id":"305653","messageId":"xmqqmvh88dlu.fsf@gitster.mtv.corp.google.com","threadId":"44412","inReplyTo":"CACsJy8BoEXDjwe=ZX5ZOC_mvaMjYrB3i7wcMmiOP3mm5-rwC5Q@mail.gmail.com","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-09T22:58:37Z","receivedAt":"2016-11-09T22:58:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> Let's err on the safe side and disable symlinks to outside repo by\n> default (or even all symlinks on .gitattributes and .gitignore as the\n> first step)\n>\n> What I learned from my changes in .gitignore is, if we have not\n> forbidden something, people likely find some creative use for it.\n\nYup.  Supporting any symlink in-tree is like requiring Git to be\nused only on symlink-capable filesystems.  Not allowing it sounds\nlike a very sensible option and unlike true contents, there is no\ndownside to give that limitation to things like .git<anything>.\n\nShouldn't we do the same for .gitmodules while we are at it?\n\n"},{"id":"305658","messageId":"20161109231720.luuhezzziuhx4r75@sigill.intra.peff.net","threadId":"44412","inReplyTo":"xmqqmvh88dlu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-09T23:17:21Z","receivedAt":"2016-11-09T23:17:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:\n\n> Duy Nguyen <pclouds@gmail.com> writes:\n> \n> > Let's err on the safe side and disable symlinks to outside repo by\n> > default (or even all symlinks on .gitattributes and .gitignore as the\n> > first step)\n> >\n> > What I learned from my changes in .gitignore is, if we have not\n> > forbidden something, people likely find some creative use for it.\n> \n> Yup.  Supporting any symlink in-tree is like requiring Git to be\n> used only on symlink-capable filesystems.  Not allowing it sounds\n> like a very sensible option and unlike true contents, there is no\n> downside to give that limitation to things like .git<anything>.\n\nI'm slightly confused. Did you mean \"supporting any in-tree symlink to\nan out-of-tree destination\" in your first sentence?\n\n> Shouldn't we do the same for .gitmodules while we are at it?\n\nGood catch. Though I am inclined to have a flag that just covers all\nout-of-tree symlinks, regardless of names.\n\n-Peff\n"},{"id":"305663","messageId":"20161110002335.z2z2stjtzgspikxe@sigill.intra.peff.net","threadId":"44412","inReplyTo":"xmqqoa1o6vca.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-10T00:23:35Z","receivedAt":"2016-11-10T00:23:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 09, 2016 at 04:18:29PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:\n> >\n> > I'm slightly confused. Did you mean \"supporting any in-tree symlink to\n> > an out-of-tree destination\" in your first sentence?\n> \n> I was trying to say that these \"control files used solely by git\"\n> have no business being a symbolic link pointing at anywhere, even\n> inside the same tree; actually, especially if it is inside the same\n> tree.\n\nOK. That is what my patch does (modulo .gitmodules, which I did not\nthink of). But I think that is the opposite of Duy's opinion, as his\nreview seemed to object to that.\n\nAs you know my ulterior motive is dealing with malicious out-of-tree\nsymlinks, and I would be happy to deal with that directly. That still\nleaves symlinked \".gitmodules\" etc in a funny state (they work in the\nfilesystem but not in the index), but since nobody is _complaining_,\nit's a bug we could leave for another day.\n\nSo I dunno.\n\n-Peff\n"},{"id":"305664","messageId":"xmqqoa1o6vca.fsf@gitster.mtv.corp.google.com","threadId":"44412","inReplyTo":"20161109231720.luuhezzziuhx4r75@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-10T00:18:29Z","receivedAt":"2016-11-10T00:25:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:\n>\n> I'm slightly confused. Did you mean \"supporting any in-tree symlink to\n> an out-of-tree destination\" in your first sentence?\n\nI was trying to say that these \"control files used solely by git\"\nhave no business being a symbolic link pointing at anywhere, even\ninside the same tree; actually, especially if it is inside the same\ntree.\n\n"},{"id":"305671","messageId":"CACsJy8BJkV+0wbrYuVrwixfbnOKw0DrwLHEtH-fzOHtEHyiXBw@mail.gmail.com","threadId":"44412","inReplyTo":"20161110002335.z2z2stjtzgspikxe@sigill.intra.peff.net","subject":"Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-11-10T11:00:51Z","receivedAt":"2016-11-10T11:01:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Nov 10, 2016 at 7:23 AM, Jeff King <peff@peff.net> wrote:\n> On Wed, Nov 09, 2016 at 04:18:29PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>>\n>> > On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:\n>> >\n>> > I'm slightly confused. Did you mean \"supporting any in-tree symlink to\n>> > an out-of-tree destination\" in your first sentence?\n>>\n>> I was trying to say that these \"control files used solely by git\"\n>> have no business being a symbolic link pointing at anywhere, even\n>> inside the same tree; actually, especially if it is inside the same\n>> tree.\n>\n> OK. That is what my patch does (modulo .gitmodules, which I did not\n> think of). But I think that is the opposite of Duy's opinion, as his\n> review seemed to object to that.\n\nI only objected the rationale (to be consistent with reading index).\nIf you sold it as malicious symlinks, or even put it like Junio \"no,\nthe design makes more sense to be this way\", I would be ok.\n\nOn the implementation side, we should print something friendlier than\nstrerror(ELOOP) if we decide that \"symlinks on .git* files are wrong\".\nThe standard ELOOP message does not communicate our design decision\nwell to the users. But this is a minor thing and can be ignored.\n\n> As you know my ulterior motive is dealing with malicious out-of-tree\n> symlinks, and I would be happy to deal with that directly. That still\n> leaves symlinked \".gitmodules\" etc in a funny state (they work in the\n> filesystem but not in the index), but since nobody is _complaining_,\n> it's a bug we could leave for another day.\n\nThe discussion trailed off a bit to symlinks in general in worktree\ntoo, I think. But it's not my itch, we can leave it for another day.\n-- \nDuy\n"}]}