git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Brandon Williams <bmwill@google.com>
Cc: git@vger.kernel.org, Stefan Beller <sbeller@google.com>
Subject: Re: [RFC] submodule: munge paths to submodule git directories
Date: Tue, 7 Aug 2018 16:25:24 -0700
Message-ID: <20180807232524.GB249457@aiede.svl.corp.google.com> (raw)
In-Reply-To: <20180807230637.247200-1-bmwill@google.com>

Hi,

Brandon Williams wrote:

> Commit 0383bbb901 (submodule-config: verify submodule names as paths,
> 2018-04-30) introduced some checks to ensure that submodule names don't
> include directory traversal components (e.g. "../").
>
> This addresses the vulnerability identified in 0383bbb901 but the root
> cause is that we use submodule names to construct paths to the
> submodule's git directory.  What we really should do is munge the
> submodule name before using it to construct a path.
>
> Introduce a function "strbuf_submodule_gitdir()" which callers can use
> to build a path to a submodule's gitdir.  This allows for a single
> location where we can munge the submodule name (by url encoding it)
> before using it as part of a path.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> Using submodule names as is continues to be not such a good idea.  Maybe
> we could apply something like this to stop using them as is.  url
> encoding seems like the easiest approach, but I've also heard
> suggestions that would could use the SHA1 of the submodule name.
>
> Any thoughts?

I like this idea.  It avoids the security and complexity problems of
funny nested directories, while still making the submodule git dirs
easy to find.

The current behavior has been particularly a problem in practice when
submodule names are nested:

	[submodule "a"]
		url = https://www.example.com/a
		path = a/1

	[submodule "a/b"]
		url = https://www.example.com/a/b
		path = a/2

We don't enforce any constraint on submodule names to prevent that,
but it causes hard to diagnose errors at clone time:

	fatal: not a git repository: superproject/a/1/../../.git/modules/a
	Unable to fetch in submodule path 'a/1'
	fatal: not a git repository: superproject/a/1/../../.git/modules/a
	fatal: not a git repository: superproject/a/1/../../.git/modules/a
	fatal: not a git repository: superproject/a/1/../../.git/modules/a
	Fetched in submodule 'a/1', but it did not contain 55ca6286e3e4f4fba5d0448333fa99fc5a404a73. Direct fetching of that commit failed.

because the fetch in .git/modules/a is interfered with by
.git/modules/a/b.

[...]
> --- a/submodule.c
> +++ b/submodule.c
[...]
> @@ -1933,9 +1938,29 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
>  			goto cleanup;
>  		}
>  		strbuf_reset(buf);
> -		strbuf_git_path(buf, "%s/%s", "modules", sub->name);
> +		strbuf_submodule_gitdir(buf, the_repository, sub->name);
>  	}
>  
>  cleanup:
>  	return ret;
>  }
> +
> +void strbuf_submodule_gitdir(struct strbuf *buf, struct repository *r,
> +			     const char *submodule_name)
> +{
> +	int modules_len;

nit: size_t

> +
> +	strbuf_git_common_path(buf, r, "modules/");
> +	modules_len = buf->len;
> +	strbuf_addstr(buf, submodule_name);
> +
> +	/*
> +	 * If the submodule gitdir already exists using the old location then
> +	 * return that.
> +	 */

nit: "old-fashioned location" or something.  Maybe the function could
use an API comment describing what's going on (that there are two
naming conventions and we try first the old, then the new).

Should we validate the submodule_name here when accessing following the old
convention?

> +	if (!access(buf->buf, F_OK))
> +		return;
> +
> +	strbuf_setlen(buf, modules_len);
> +	strbuf_addstr_urlencode(buf, submodule_name, 1);
> +}
[...]
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -932,7 +932,7 @@ test_expect_success 'recursive relative submodules stay relative' '
>  		cd clone2 &&
>  		git submodule update --init --recursive &&
>  		echo "gitdir: ../.git/modules/sub3" >./sub3/.git_expect &&
> -		echo "gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub" >./sub3/dirdir/subsub/.git_expect
> +		echo "gitdir: ../../../.git/modules/sub3/modules/dirdir%2fsubsub" >./sub3/dirdir/subsub/.git_expect
>  	) &&
>  	test_cmp clone2/sub3/.git_expect clone2/sub3/.git &&
>  	test_cmp clone2/sub3/dirdir/subsub/.git_expect clone2/sub3/dirdir/subsub/.git

Sensible.

Can there be a test of the compatibility code as well?  (I mean a test
that manually sets up a submodule in .git/modules/dirdir/subsub and
ensures that it gets reused.)

I'll apply this, experiment with it, and report back.  Thanks for
writing it.

Sincerely,
Jonathan

  reply index

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-07 23:06 Brandon Williams
2018-08-07 23:25 ` Jonathan Nieder [this message]
2018-08-08  0:14 ` Junio C Hamano
2018-08-08 22:33 ` [PATCH 0/2] munge submodule names Brandon Williams
2018-08-08 22:33   ` [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs Brandon Williams
2018-08-08 23:21     ` Stefan Beller
2018-08-09  0:45       ` Brandon Williams
2018-08-10 21:27     ` Junio C Hamano
2018-08-10 21:45       ` Brandon Williams
2018-08-08 22:33   ` [PATCH 2/2] submodule: munge paths to submodule git directories Brandon Williams
2018-08-09 21:26     ` Jeff King
2018-08-14 18:04       ` Brandon Williams
2018-08-14 18:57         ` Jonathan Nieder
2018-08-14 21:08           ` Stefan Beller
2018-08-14 21:12             ` Jonathan Nieder
2018-08-14 22:34               ` Stefan Beller
2018-08-16  2:34                 ` Jonathan Nieder
2018-08-16  2:39                   ` Stefan Beller
2018-08-16  2:47                     ` Jonathan Nieder
2018-08-16 17:34                       ` Brandon Williams
2018-08-16 18:19                       ` [PATCH] submodule: add config for where gitdirs are located Brandon Williams
2018-08-20 22:03                         ` Junio C Hamano
2018-08-16 15:07                   ` [PATCH 2/2] submodule: munge paths to submodule git directories Junio C Hamano
2018-08-14 18:58         ` Jeff King
2018-08-28 21:35         ` Stefan Beller
2018-08-29  5:25           ` Jeff King
2018-08-29 18:10             ` Stefan Beller
2018-08-29 21:03               ` Jeff King
2018-08-29 21:10                 ` Stefan Beller
2018-08-29 21:18                   ` Jonathan Nieder
2018-08-29 21:27                     ` Stefan Beller
2018-08-29 21:30                   ` Jeff King
2018-08-29 21:09             ` Jonathan Nieder
2018-08-29 21:14               ` Stefan Beller
2018-08-29 21:25                 ` Brandon Williams
2018-08-29 21:32               ` Jeff King
2018-08-16  0:19     ` Aaron Schrab
2019-01-15  1:25 ` [RFC] " Jonathan Nieder
2019-01-17 17:32   ` Jeff King
2019-01-17 17:57     ` Stefan Beller

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180807232524.GB249457@aiede.svl.corp.google.com \
    --to=jrnieder@gmail.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox