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

Re: [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore

From
Junio C Hamano <gitster@pobox.com>
Date
Oct 27, 2020, 22:00 UTC
Message-ID
<xmqqv9ev9vnf.fsf@gitster.c.googlers.com>
In-Reply-To
<20201027075853.GH3005508@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> diff --git a/environment.c b/environment.c
> index bb518c61cd..7c233e0e0e 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -73,6 +73,7 @@ int merge_log_config = -1;
>  int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
>  unsigned long pack_size_limit_cfg;
>  enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;
> +int allow_external_symlinks = 1;
OK, so by default it is not blocked...
Show 8 quoted lines
> +static int symlink_leaves_repo(const char *target, const char *linkpath)
> +{
> +	/*
> +	 * Absolute paths are always considered to leave the repository (even
> +	 * if they happen to point to the working tree path).
> +	 */
> +	if (is_absolute_path(target))
> +		return 1;
Very sensible.
Show 63 quoted lines
> +	/*
> +	 * Allow relative paths that start with a sequence of "../",
> +	 * as long as they do not break out of the symlink's root.
> +	 * This loop will detect break-out cases and return; otherwise, at the
> +	 * end of the loop "target" will point to the first non-".." component.
> +	 *
> +	 * We count the depth of linkpath by eating up directory components left
> +	 * to right. Technically the symlink would resolve right-to-left, but
> +	 * we don't care about the actual values, only the number.
> +	 */
> +	while (target[0] == '.') {
> +		if (!target[1]) {
> +			/* trailing "." -- ignore */
> +			target++;
> +		} else if (is_dir_sep(target[1])) {
> +			/* "./" -- ignore */
> +			target += 2;
> +		} else if (target[1] == '.' &&
> +			   (!target[2] || is_dir_sep(target[2]))) {
> +			/* ".." or "../" -- drop one from linkpath depth */
> +			while (!is_dir_sep(*linkpath)) {
> +				/* end-of-string; target exceeded our depth */
> +				if (!*linkpath)
> +					return 1;
> +				linkpath++;
> +			}
> +			/* skip final "/" */
> +			linkpath++;
> +
> +			/* skip past ".." */
> +			target += 2;
> +			/* and "/" if present */
> +			if (is_dir_sep(*target))
> +				target++;
> +		}
> +	}
> +
> +	/*
> +	 * Now we have a path in "target" that only go down into the tree.
> +	 * Disallow any interior "../", like "foo/../bar". These might be
> +	 * OK, but we cannot know unless we know whether "foo" is itself a
> +	 * symlink. So err on the side of caution.
> +	 */
> +	while (*target) {
> +		const char *v;
> +		if (skip_prefix(target, "..", &v) && (!*v || is_dir_sep(*v)))
> +			return 1;
> +		target++;
> +	}
> +
> +	return 0;
> +}
> +
> +int safe_symlink(const char *target, const char *linkpath)
> +{
> +	if (!allow_external_symlinks &&
> +	    symlink_leaves_repo(target, linkpath)) {
> +		errno = EPERM;
> +		return -1;
> +	}
> +
> +	return symlink(target, linkpath);
> +}

OK. This is only about blocking creation of new symbolic links that goes outside the working tree. It obviously is a good thing to do.

We have some "symlink safety" in various parts of the system [*1*], and I wonder if we can somehow consolidate the support to a more central place.

Thanks.
[Footnote]
*1* For example, apply tries to be careful not to take the "path"
    recorded in the incoming patch blindly, and instead checks if
    any path component in it is a symbolic link before touching.
    Similarly, callers of has_symlink_leading_path() all try to be
    careful when the "path" they want to use to access a filesystem
    entity has a symbolic link in the middle on the filesystem.
Previous: Jeff KingNext: Jeff King
Message 39 of 55 in “forbidding symlinked .gitattributes and .gitignore”
  1. 0/7 forbidding symlinked .gitattributes and .gitignoreJeff King, Oct 5, 2020
  2. 1/7 fsck_tree(): fix shadowed variableJeff King, Oct 5, 2020
  3. Jonathan NiederOct 5, 2020
  4. Jeff KingOct 5, 2020
  5. Jonathan NiederOct 5, 2020
  6. 2/7 fsck_tree(): wrap some long linesJeff King, Oct 5, 2020
  7. Jonathan NiederOct 5, 2020
  8. 3/7 t7415: rename to expand scopeJeff King, Oct 5, 2020
  9. Jonathan NiederOct 5, 2020
  10. Jeff KingOct 5, 2020
  11. Jonathan NiederOct 5, 2020
  12. Jeff KingOct 5, 2020
  13. 4/7 t7450: test verify_path() handling of gitmodulesJeff King, Oct 5, 2020
  14. Jonathan NiederOct 5, 2020
  15. Jeff KingOct 5, 2020
  16. Jonathan NiederOct 5, 2020
  17. 5/7 t0060: test obscured .gitattributes and .gitignore matchingJeff King, Oct 5, 2020
  18. Jonathan NiederOct 5, 2020
  19. Jeff KingOct 5, 2020
  20. Johannes SchindelinOct 5, 2020
  21. Jeff KingOct 6, 2020
  22. 6/7 verify_path(): disallow symlinks in .gitattributes and .gitignoreJeff King, Oct 5, 2020
  23. Jonathan NiederOct 5, 2020
  24. Jeff KingOct 5, 2020
  25. 7/7 fsck: complain when .gitattributes or .gitignore is a symlinkJeff King, Oct 5, 2020
  26. Jonathan NiederOct 5, 2020
  27. Jeff KingOct 5, 2020
  28. Jonathan NiederOct 5, 2020
  29. Jeff KingOct 5, 2020
  30. 0/8 forbidding symlinked .gitattributes and .gitignoreJeff King, Oct 5, 2020
  31. 1/8 fsck_tree(): fix shadowed variableJeff King, Oct 5, 2020
  32. 2/8 fsck_tree(): wrap some long linesJeff King, Oct 5, 2020
  33. 3/8 t7415: rename to expand scopeJeff King, Oct 5, 2020
  34. 4/8 t7450: test verify_path() handling of gitmodulesJeff King, Oct 5, 2020
  35. 5/8 t7450: test .gitmodules symlink matching against obscured namesJeff King, Oct 5, 2020
  36. 7/8 verify_path(): disallow symlinks in .gitattributes and .gitignoreJeff King, Oct 5, 2020
  37. Jonathan NiederOct 27, 2020
  38. Jeff KingOct 27, 2020
  39. Junio C HamanoOct 27, 2020
  40. Jeff KingOct 28, 2020
  41. Jonathan NiederOct 27, 2020
  42. Junio C HamanoOct 28, 2020
  43. 6/8 t0060: test obscured .gitattributes and .gitignore matchingJeff King, Oct 5, 2020
  44. 8/8 fsck: complain when .gitattributes or .gitignore is a symlinkJeff King, Oct 5, 2020
  45. Junio C HamanoOct 6, 2020
  46. Philip OakleyOct 20, 2020
  47. documentation symlink restrictions for .git* filesJeff King, Oct 23, 2020
  48. Jeff KingOct 23, 2020
  49. Philip OakleyOct 26, 2020
  50. Jeff KingOct 26, 2020
  51. Junio C HamanoOct 26, 2020
  52. Jeff KingOct 27, 2020
  53. Junio C HamanoOct 27, 2020
  54. Philip OakleyOct 27, 2020
  55. Junio C HamanoOct 28, 2020

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.