git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 17, 2019, 20:13 UTC
Message-ID
<xmqq1s5bniuf.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<20190117170005.GA27667@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 27 quoted lines
> Hmm. I think this commit message isn't quite right, because we also
> skipped the patches to touch gitignore/gitattributes in verify_path().
>
> Are you thinking we should resurrect that behavior[1], too, or just
> protect at the fsck level?
>
>> It was omitted from that series because it does not address any known
>> exploit, but to me it seems worthwhile anyway:
>> 
>> - if a client enables transfer.fsckObjects, this helps them protect
>>   themselves against weird input that does *not* have a known exploit
>>   attached, to
>> 
>> - it generally feels more simple and robust.  Git-related tools can
>>   benefit from this kind of check as an indication of input they can
>>   bail out on instead of trying to support.
>
> I think I may just be restating your two points above, but what I'd
> argue is:
>
>   - even though there's no known-interesting exploit, this can cause Git
>     to unexpectedly read arbitrary files outside of the repository
>     directory. That in itself isn't necessarily evil, but it's weird.
>
>   - there are potentially non-malicious bugs here, where we try to read
>     .gitattributes out of the index, but obviously don't follow symlinks
>     there

FWIW, you two can count me as the third person who agrees with the above points.

Show 19 quoted lines
> [1] This wasn't a separate patch, but just an early iteration of the
>     "ban symlinks in .gitmodules" patch. I think the incremental is
>     just:
>
> diff --git a/read-cache.c b/read-cache.c
> index bfff271a3d..121c0bec69 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -937,7 +937,9 @@ static int verify_dotfile(const char *rest, unsigned mode)
>  			return 0;
>  		if (S_ISLNK(mode)) {
>  			rest += 3;
> -			if (skip_iprefix(rest, "modules", &rest) &&
> +			if ((skip_iprefix(rest, "modules", &rest) ||
> +			     skip_iprefix(rest, "ignore", &rest) ||
> +			     skip_iprefix(rest, "attributes", &rest)) &&
>  			    (*rest == '\0' || is_dir_sep(*rest)))
>  				return 0;
>  		}
OK.
Show 20 quoted lines
> @@ -966,7 +968,9 @@ int verify_path(const char *path, unsigned mode)
>  				if (is_hfs_dotgit(path))
>  					return 0;
>  				if (S_ISLNK(mode)) {
> -					if (is_hfs_dotgitmodules(path))
> +					if (is_hfs_dotgitmodules(path) ||
> +					    is_hfs_dotgitignore(path) ||
> +					    is_hfs_dotgitattributes(path))
>  						return 0;
>  				}
>  			}
> @@ -974,7 +978,9 @@ int verify_path(const char *path, unsigned mode)
>  				if (is_ntfs_dotgit(path))
>  					return 0;
>  				if (S_ISLNK(mode)) {
> -					if (is_ntfs_dotgitmodules(path))
> +					if (is_ntfs_dotgitmodules(path) ||
> +					    is_ntfs_dotgitignore(path) ||
> +					    is_ntfs_dotgitattributes(path))
>  						return 0;

Curious that we already have these helpers, nobody seems to call them in the current codebase, and we haven't seen the "these are unused" linter message on the list for a while ;-).

Previous: Jeff KingNext: Jeff King
Message 3 of 7 in “fsck: complain when .gitignore and .gitattributes are symlinks”
  1. fsck: complain when .gitignore and .gitattributes are symlinksJonathan Nieder, Jan 14, 2019
  2. Jeff KingJan 17, 2019
  3. Junio C HamanoJan 17, 2019
  4. Jeff KingJan 17, 2019
  5. Ramsay JonesJan 18, 2019
  6. Jeff KingJan 22, 2019
  7. Ramsay JonesJan 22, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.