From: Brandon Williams <hidden> Date: 2018-08-07 23:06:44
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 <redacted>
---
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?
builtin/submodule--helper.c | 10 ++++--
dir.c | 2 +-
repository.c | 3 +-
submodule.c | 57 +++++++++++++++++++++++---------
submodule.h | 3 ++
t/t7400-submodule-basic.sh | 2 +-
t/t7406-submodule-update.sh | 21 ++++--------
t/t7410-submodule-checkout-to.sh | 6 ++--
8 files changed, 65 insertions(+), 39 deletions(-)
@@ -1625,20 +1625,22 @@ int submodule_move_head(const char *path,absorb_git_dir_into_superproject("",path,ABSORB_GITDIR_RECURSE_SUBMODULES);}else{-char*gitdir=xstrfmt("%s/modules/%s",-get_git_common_dir(),sub->name);-connect_work_tree_and_git_dir(path,gitdir,0);-free(gitdir);+structstrbufgitdir=STRBUF_INIT;+strbuf_submodule_gitdir(&gitdir,the_repository,+sub->name);+connect_work_tree_and_git_dir(path,gitdir.buf,0);+strbuf_release(&gitdir);/* make sure the index is clean as well */submodule_reset_index(path);}if(old_head&&(flags&SUBMODULE_MOVE_HEAD_FORCE)){-char*gitdir=xstrfmt("%s/modules/%s",-get_git_common_dir(),sub->name);-connect_work_tree_and_git_dir(path,gitdir,1);-free(gitdir);+structstrbufgitdir=STRBUF_INIT;+strbuf_submodule_gitdir(&gitdir,the_repository,+sub->name);+connect_work_tree_and_git_dir(path,gitdir.buf,1);+strbuf_release(&gitdir);}}
@@ -1729,10 +1731,10 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,if(!sub)die(_("could not lookup name for submodule '%s'"),path);-new_git_dir=git_path("modules/%s",sub->name);-if(safe_create_leading_directories_const(new_git_dir)<0)-die(_("could not create directory '%s'"),new_git_dir);-real_new_git_dir=real_pathdup(new_git_dir,1);+strbuf_submodule_gitdir(&new_gitdir,the_repository,sub->name);+if(safe_create_leading_directories_const(new_gitdir.buf)<0)+die(_("could not create directory '%s'"),new_gitdir.buf);+real_new_git_dir=real_pathdup(new_gitdir.buf,1);fprintf(stderr,_("Migrating git directory of '%s%s' from\n'%s' to\n'%s'\n"),get_super_prefix_or_empty(),path,
@@ -1763,6 +1766,7 @@ void absorb_git_dir_into_superproject(const char *prefix,/* Not populated? */if(!sub_git_dir){conststructsubmodule*sub;+structstrbufsub_gitdir=STRBUF_INIT;if(err_code==READ_GITFILE_ERR_STAT_FAILED){/* unpopulated as expected */
@@ -1784,8 +1788,9 @@ void absorb_git_dir_into_superproject(const char *prefix,sub=submodule_from_path(the_repository,&null_oid,path);if(!sub)die(_("could not lookup name for submodule '%s'"),path);-connect_work_tree_and_git_dir(path,-git_path("modules/%s",sub->name),0);+strbuf_submodule_gitdir(&sub_gitdir,the_repository,sub->name);+connect_work_tree_and_git_dir(path,sub_gitdir.buf,0);+strbuf_release(&sub_gitdir);}else{/* Is it already absorbed into the superprojects git dir? */char*real_sub_git_dir=real_pathdup(sub_git_dir,1);
@@ -35,8 +35,10 @@ test_expect_success 'checkout main' \(cdclone/main&&gitworktreeadd"$base_path/default_checkout/main""$rev1_hash_main")'-test_expect_failure'can see submodule diffs just after checkout'\-'(cd default_checkout/main && git diff --submodule master"^!" | grep "file1 updated")'+test_expect_success'can see submodule diffs just after checkout''+git-Cdefault_checkout/maindiff--submodulemaster"^!">out&&+grep"file1 updated"out+' test_expect_success'checkout main and initialize independed clones'\'mkdirfully_cloned_submodule&&
From: Jonathan Nieder <hidden> Date: 2018-08-07 23:25:29
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 <redacted>
---
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.
[...]
+
+ 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?
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
From: Brandon Williams <hidden> Date: 2018-08-08 22:33:29
Here's a more polished series taking into account some of the feedback
on the RFC. As Junio pointed out URL encoding makes the directories
much more human readable, but I'm open to other ideas if we don't think
URL encoding is the right thing to do.
Brandon Williams (2):
submodule: create helper to build paths to submodule gitdirs
submodule: munge paths to submodule git directories
builtin/submodule--helper.c | 28 +++++++++++--
dir.c | 2 +-
git-submodule.sh | 7 ++--
repository.c | 3 +-
submodule.c | 67 ++++++++++++++++++++++----------
submodule.h | 7 ++++
t/t7400-submodule-basic.sh | 32 ++++++++++++++-
t/t7406-submodule-update.sh | 21 +++-------
t/t7410-submodule-checkout-to.sh | 6 ++-
9 files changed, 126 insertions(+), 47 deletions(-)
--
2.18.0.597.ga71716f1ad-goog
From: Brandon Williams <hidden> Date: 2018-08-08 22:33:34
Introduce a helper function "submodule_name_to_gitdir()" (and the
submodule--helper subcommand "gitdir") which constructs a path to a
submodule's gitdir, located in the provided repository's "modules"
directory.
This consolidates the logic needed to build up a path into a
repository's "modules" directory, abstracting away the fact that
submodule git directories are stored in a repository's common gitdir.
This makes it easier to adjust how submodules gitdir are stored in the
"modules" directory in a future patch.
Signed-off-by: Brandon Williams <redacted>
---
builtin/submodule--helper.c | 28 +++++++++++++++--
dir.c | 2 +-
git-submodule.sh | 7 +++--
repository.c | 3 +-
submodule.c | 53 ++++++++++++++++++++------------
submodule.h | 7 +++++
t/t7410-submodule-checkout-to.sh | 6 ++--
7 files changed, 75 insertions(+), 31 deletions(-)
@@ -252,12 +252,13 @@ Use -f if you really want to add it." >&2fielse-iftest-d".git/modules/$sm_name"+sm_gitdir="$(gitsubmodule--helpergitdir"$sm_name")"+iftest-d"$sm_gitdir"theniftest-z"$force"theneval_gettextln>&2"A git directory for '\$sm_name' is found locally with remote(s):"-GIT_DIR=".git/modules/$sm_name"GIT_WORK_TREE=.gitremote-v|grep'(fetch)'|sed-es,^," ",-es,' (fetch)',,>&2+GIT_DIR="$sm_gitdir"GIT_WORK_TREE=.gitremote-v|grep'(fetch)'|sed-es,^," ",-es,' (fetch)',,>&2die"$(eval_gettextln"\ Ifyouwanttoreusethislocalgitdirectoryinsteadofcloningagainfrom\$realrepo
@@ -577,7 +578,7 @@ cmd_update()die"$(eval_gettext"Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"fi-if!$(gitconfig-f"$(gitrev-parse--git-common-dir)/modules/$name/config"core.worktree)2>/dev/null+if!$(gitconfig-f"$(gitsubmodule--helpergitdir"$name")/config"core.worktree)2>/dev/nullthengitsubmodule--helperconnect-gitdir-workingtree"$name""$sm_path"fi
@@ -1536,14 +1536,15 @@ int bad_to_remove_submodule(const char *path, unsigned flags)voidsubmodule_unset_core_worktree(conststructsubmodule*sub){-char*config_path=xstrfmt("%s/modules/%s/config",-get_git_common_dir(),sub->name);+structstrbufconfig_path=STRBUF_INIT;+submodule_name_to_gitdir(&config_path,the_repository,sub->name);+strbuf_addstr(&config_path,"/config");-if(git_config_set_in_file_gently(config_path,"core.worktree",NULL))+if(git_config_set_in_file_gently(config_path.buf,"core.worktree",NULL))warning(_("Could not unset core.worktree setting in submodule '%s'"),sub->path);-free(config_path);+strbuf_release(&config_path);}staticconstchar*get_super_prefix_or_empty(void)
@@ -1639,20 +1640,22 @@ int submodule_move_head(const char *path,absorb_git_dir_into_superproject("",path,ABSORB_GITDIR_RECURSE_SUBMODULES);}else{-char*gitdir=xstrfmt("%s/modules/%s",-get_git_common_dir(),sub->name);-connect_work_tree_and_git_dir(path,gitdir,0);-free(gitdir);+structstrbufgitdir=STRBUF_INIT;+submodule_name_to_gitdir(&gitdir,the_repository,+sub->name);+connect_work_tree_and_git_dir(path,gitdir.buf,0);+strbuf_release(&gitdir);/* make sure the index is clean as well */submodule_reset_index(path);}if(old_head&&(flags&SUBMODULE_MOVE_HEAD_FORCE)){-char*gitdir=xstrfmt("%s/modules/%s",-get_git_common_dir(),sub->name);-connect_work_tree_and_git_dir(path,gitdir,1);-free(gitdir);+structstrbufgitdir=STRBUF_INIT;+submodule_name_to_gitdir(&gitdir,the_repository,+sub->name);+connect_work_tree_and_git_dir(path,gitdir.buf,1);+strbuf_release(&gitdir);}}
@@ -1745,10 +1748,10 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,if(!sub)die(_("could not lookup name for submodule '%s'"),path);-new_git_dir=git_path("modules/%s",sub->name);-if(safe_create_leading_directories_const(new_git_dir)<0)-die(_("could not create directory '%s'"),new_git_dir);-real_new_git_dir=real_pathdup(new_git_dir,1);+submodule_name_to_gitdir(&new_gitdir,the_repository,sub->name);+if(safe_create_leading_directories_const(new_gitdir.buf)<0)+die(_("could not create directory '%s'"),new_gitdir.buf);+real_new_git_dir=real_pathdup(new_gitdir.buf,1);fprintf(stderr,_("Migrating git directory of '%s%s' from\n'%s' to\n'%s'\n"),get_super_prefix_or_empty(),path,
@@ -1779,6 +1783,7 @@ void absorb_git_dir_into_superproject(const char *prefix,/* Not populated? */if(!sub_git_dir){conststructsubmodule*sub;+structstrbufsub_gitdir=STRBUF_INIT;if(err_code==READ_GITFILE_ERR_STAT_FAILED){/* unpopulated as expected */
@@ -1800,8 +1805,9 @@ void absorb_git_dir_into_superproject(const char *prefix,sub=submodule_from_path(the_repository,&null_oid,path);if(!sub)die(_("could not lookup name for submodule '%s'"),path);-connect_work_tree_and_git_dir(path,-git_path("modules/%s",sub->name),0);+submodule_name_to_gitdir(&sub_gitdir,the_repository,sub->name);+connect_work_tree_and_git_dir(path,sub_gitdir.buf,0);+strbuf_release(&sub_gitdir);}else{/* Is it already absorbed into the superprojects git dir? */char*real_sub_git_dir=real_pathdup(sub_git_dir,1);
@@ -35,8 +35,10 @@ test_expect_success 'checkout main' \(cdclone/main&&gitworktreeadd"$base_path/default_checkout/main""$rev1_hash_main")'-test_expect_failure'can see submodule diffs just after checkout'\-'(cd default_checkout/main && git diff --submodule master"^!" | grep "file1 updated")'+test_expect_success'can see submodule diffs just after checkout''+git-Cdefault_checkout/maindiff--submodulemaster"^!">out&&+grep"file1 updated"out+' test_expect_success'checkout main and initialize independed clones'\'mkdirfully_cloned_submodule&&
From: Brandon Williams <hidden> Date: 2018-08-08 22:33:35
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.
Teach "submodule_name_to_gitdir()" to munge a submodule's name (by url
encoding it) before using it to build a path to the submodule's gitdir.
Signed-off-by: Brandon Williams <redacted>
---
submodule.c | 14 ++++++++++++++
t/t7400-submodule-basic.sh | 32 +++++++++++++++++++++++++++++++-
t/t7406-submodule-update.sh | 21 ++++++---------------
3 files changed, 51 insertions(+), 16 deletions(-)
@@ -1324,4 +1324,34 @@ test_expect_success 'recursive clone respects -q' 'test_must_be_emptyactual'+test_expect_success'resolve submodule gitdir in superprojects modules directory''+test_when_finished"rm -rf superproject submodule"&&++# Create a superproject with a submodule which contains a "/"+test_create_reposubmodule&&+test_commit-Csubmoduleone&&+test_create_reposuperproject&&+git-Csuperprojectsubmoduleadd../submodulesub/module&&+git-Csuperprojectcommit-m"add submodule"&&++# "/" characters in submodule names are properly urlencoded before+# being used to construct a path to the submodules gitdir.+cat>expect<<-EOF&&+$(git-Csuperprojectrev-parse--git-common-dir)/modules/sub%2fmodule+EOF+git-Csuperprojectsubmodule--helpergitdir"sub/module">actual&&+test_cmpexpectactual&&+test_path_is_dir"superproject/.git/modules/sub%2fmodule"&&++# Test the old-fashioned way of storing submodules in the+# "modules" directory by directly renaming the submodules gitdir+mkdirsuperproject/.git/modules/sub/&&+mvsuperproject/.git/modules/sub%2fmodulesuperproject/.git/modules/sub/module&&+cat>expect<<-EOF&&+$(git-Csuperprojectrev-parse--git-common-dir)/modules/sub/module+EOF+git-Csuperprojectsubmodule--helpergitdir"sub/module">actual&&+test_cmpexpectactual+'+ test_done
From: Stefan Beller <hidden> Date: 2018-08-08 23:21:34
On Wed, Aug 8, 2018 at 3:33 PM Brandon Williams [off-list ref] wrote:
Introduce a helper function "submodule_name_to_gitdir()" (and the
submodule--helper subcommand "gitdir") which constructs a path to a
submodule's gitdir, located in the provided repository's "modules"
directory.
Makes sense.
This consolidates the logic needed to build up a path into a
repository's "modules" directory, abstracting away the fact that
submodule git directories are stored in a repository's common gitdir.
This makes it easier to adjust how submodules gitdir are stored in the
"modules" directory in a future patch.
and yet, all places that we touch were and still are broken for old-style
submodules that have their git directory inside the working tree?
Do we need to pay attention to those, too?
@@ -577,7 +578,7 @@ cmd_update() die "$(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")" fi- if ! $(git config -f "$(git rev-parse --git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null+ if ! $(git config -f "$(git submodule--helper gitdir "$name")/config" core.worktree) 2>/dev/null
This will collide with origin/sb/submodule-update-in-c specifically
1c866b9831d (submodule--helper: replace connect-gitdir-workingtree
by ensure-core-worktree, 2018-08-03), but as that removes these lines,
it should be easy to resolve the conflict.
From: Brandon Williams <hidden> Date: 2018-08-09 00:45:52
On 08/08, Stefan Beller wrote:
On Wed, Aug 8, 2018 at 3:33 PM Brandon Williams [off-list ref] wrote:
quoted
Introduce a helper function "submodule_name_to_gitdir()" (and the
submodule--helper subcommand "gitdir") which constructs a path to a
submodule's gitdir, located in the provided repository's "modules"
directory.
Makes sense.
quoted
This consolidates the logic needed to build up a path into a
repository's "modules" directory, abstracting away the fact that
submodule git directories are stored in a repository's common gitdir.
This makes it easier to adjust how submodules gitdir are stored in the
"modules" directory in a future patch.
and yet, all places that we touch were and still are broken for old-style
submodules that have their git directory inside the working tree?
Do we need to pay attention to those, too?
This series only tries to address the issues with submodules stored in
$GITDIR/modules/ and places in our codebase that explicitly reference
submodules stored there.
For those old-old-style submodules, wouldn't the absorb submodule
functions handle that migration?
@@ -577,7 +578,7 @@ cmd_update() die "$(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")" fi- if ! $(git config -f "$(git rev-parse --git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null+ if ! $(git config -f "$(git submodule--helper gitdir "$name")/config" core.worktree) 2>/dev/null
This will collide with origin/sb/submodule-update-in-c specifically
1c866b9831d (submodule--helper: replace connect-gitdir-workingtree
by ensure-core-worktree, 2018-08-03), but as that removes these lines,
it should be easy to resolve the conflict.
From: Jeff King <hidden> Date: 2018-08-09 21:26:07
On Wed, Aug 08, 2018 at 03:33:23PM -0700, 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.
Teach "submodule_name_to_gitdir()" to munge a submodule's name (by url
encoding it) before using it to build a path to the submodule's gitdir.
I like this approach very much, and I think using url encoding is much
better than an opaque hash (purely because it makes debugging and
inspection saner).
Two thoughts, though:
+ modules_len = buf->len;
strbuf_addstr(buf, submodule_name);
+
+ /*
+ * If the submodule gitdir already exists using the old-fashioned
+ * location (which uses the submodule name as-is, without munging it)
+ * then return that.
+ */
+ if (!access(buf->buf, F_OK))
+ return;
I think this backwards-compatibility is necessary to avoid pain. But
until it goes away, I don't think this is helping the vulnerability from
0383bbb901. Because there the issue was that the submodule name pointed
back into the working tree, so this access() would find the untrusted
working tree code and say "ah, an old-fashioned name!".
In theory a fresh clone could set a config option for "I only speak
use new-style modules". And there could even be a conversion program
that moves the modules as appropriate, fixes up the .git files in the
working tree, and then sets that config.
In fact, I think that config option _could_ be done by bumping
core.repositoryformatversion and then setting extensions.submodulenames
to "url" or something. Then you could never run into the confusing case
where you have a clone done by a new version of git (using new-style
names), but using an old-style version gets confused because it can't
find the module directories (instead, it would barf and say "I don't
know about that extension").
I don't know if any of that is worth it, though. We already fixed the
problem from 0383bbb901. There may be a _different_ "break out of the
modules directory" vulnerability, but since we disallow ".." it's hard
to see what it would be (the best I could come up with is maybe pointing
one module into the interior of another module, but I think you'd have
to trouble overwriting anything useful).
And while an old-style version of Git being confused might be annoying,
I suspect that bumping the repository version would be even _more_
annoying, because it would hit every command, not just ones that try to
touch those submodules.
One interesting thing about url-encoding is that it's not one-to-one.
This case could also be %2F, which is a different file (on a
case-sensitive filesystem). I think "%20" and "+" are similarly
interchangeable.
If we were decoding the filenames, that's fine. The round-trip is
lossless.
But that's not quite how the new code behaves. We encode the input and
then check to see if it matches an encoding we previously performed. So
if our urlencode routines ever change, this will subtly break.
I don't know how much it's worth caring about. We're not that likely to
change the routines ourself (though certainly a third-party
implementation would need to know our exact url-encoding decisions).
Some possible actions:
0. Do nothing, and cross our fingers. ;)
1. Don't use strbuf_addstr_urlencode(), but rather our own munging
function which we know will remain stable (or alternatively, a flag
to strbuf_addstr_urlencode to get the consistent behavior).
2. Make sure we have tests which cover this, so at least somebody
changing the urlencode decisions will see a breakage. Your test here
covers the upper/lowercase one, but we might want one that covers
"+". (There may be more ambiguous cases, but those are the ones I
know about).
3. Rather than check for the existence of names, decode what's actually
in the modules/ directory to create an in-memory index of names.
I hesitate to suggest that, because it's obviously way more
complicated, and may perform worse if you have a lot of modules
(since you have to readdir() and decode the whole directory just to
look up one module).
But I think it also gives a more elegant solution to the
backwards-compatibility problem, since we could recognize both new
and old-style names. There's some ambiguity (e.g., is "foo%2fbar"
"foo/bar", or did somebody really have a name with a percent in
it?),. but in theory you could respect either name (giving
preference to new-style in case of a conflict).
And I think the result would be immune to any directory-escape
vulnerabilities, because we'd always start with what actually exists
in $GIT_DIR/modules/, which we know _we_ will have written.
Again, I'm not sure if it's worth the effort, but I thought I'd
throw it out there.
-Peff
From: Brandon Williams <hidden> Date: 2018-08-14 18:04:11
On 08/09, Jeff King wrote:
On Wed, Aug 08, 2018 at 03:33:23PM -0700, Brandon Williams wrote:
quoted
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.
Teach "submodule_name_to_gitdir()" to munge a submodule's name (by url
encoding it) before using it to build a path to the submodule's gitdir.
I like this approach very much, and I think using url encoding is much
better than an opaque hash (purely because it makes debugging and
inspection saner).
Two thoughts, though:
quoted
+ modules_len = buf->len;
strbuf_addstr(buf, submodule_name);
+
+ /*
+ * If the submodule gitdir already exists using the old-fashioned
+ * location (which uses the submodule name as-is, without munging it)
+ * then return that.
+ */
+ if (!access(buf->buf, F_OK))
+ return;
I think this backwards-compatibility is necessary to avoid pain. But
until it goes away, I don't think this is helping the vulnerability from
0383bbb901. Because there the issue was that the submodule name pointed
back into the working tree, so this access() would find the untrusted
working tree code and say "ah, an old-fashioned name!".
In theory a fresh clone could set a config option for "I only speak
use new-style modules". And there could even be a conversion program
that moves the modules as appropriate, fixes up the .git files in the
working tree, and then sets that config.
In fact, I think that config option _could_ be done by bumping
core.repositoryformatversion and then setting extensions.submodulenames
to "url" or something. Then you could never run into the confusing case
where you have a clone done by a new version of git (using new-style
names), but using an old-style version gets confused because it can't
find the module directories (instead, it would barf and say "I don't
know about that extension").
I don't know if any of that is worth it, though. We already fixed the
problem from 0383bbb901. There may be a _different_ "break out of the
modules directory" vulnerability, but since we disallow ".." it's hard
to see what it would be (the best I could come up with is maybe pointing
one module into the interior of another module, but I think you'd have
to trouble overwriting anything useful).
And while an old-style version of Git being confused might be annoying,
I suspect that bumping the repository version would be even _more_
annoying, because it would hit every command, not just ones that try to
touch those submodules.
Oh I know that this doesn't help with that vulnerability. As you've
said we fix it and now disallow ".." at the submodule-config level so
really this path is simply about using what we get out of
submodule-config in a more sane manor.
One interesting thing about url-encoding is that it's not one-to-one.
This case could also be %2F, which is a different file (on a
case-sensitive filesystem). I think "%20" and "+" are similarly
interchangeable.
If we were decoding the filenames, that's fine. The round-trip is
lossless.
But that's not quite how the new code behaves. We encode the input and
then check to see if it matches an encoding we previously performed. So
if our urlencode routines ever change, this will subtly break.
I don't know how much it's worth caring about. We're not that likely to
change the routines ourself (though certainly a third-party
implementation would need to know our exact url-encoding decisions).
This is exactly the reason why I wanted to get some opinions on what the
best thing to do here would be. I _think_ the best thing would probably
be to write a specific routine to do the conversion, and it wouldn't
even have to be all that complex. Basically I'm just interested in
converting '/' characters so that things no longer behave like
nested directories.
Some possible actions:
0. Do nothing, and cross our fingers. ;)
1. Don't use strbuf_addstr_urlencode(), but rather our own munging
function which we know will remain stable (or alternatively, a flag
to strbuf_addstr_urlencode to get the consistent behavior).
2. Make sure we have tests which cover this, so at least somebody
changing the urlencode decisions will see a breakage. Your test here
covers the upper/lowercase one, but we might want one that covers
"+". (There may be more ambiguous cases, but those are the ones I
know about).
3. Rather than check for the existence of names, decode what's actually
in the modules/ directory to create an in-memory index of names.
I hesitate to suggest that, because it's obviously way more
complicated, and may perform worse if you have a lot of modules
(since you have to readdir() and decode the whole directory just to
look up one module).
But I think it also gives a more elegant solution to the
backwards-compatibility problem, since we could recognize both new
and old-style names. There's some ambiguity (e.g., is "foo%2fbar"
"foo/bar", or did somebody really have a name with a percent in
it?),. but in theory you could respect either name (giving
preference to new-style in case of a conflict).
And I think the result would be immune to any directory-escape
vulnerabilities, because we'd always start with what actually exists
in $GIT_DIR/modules/, which we know _we_ will have written.
Again, I'm not sure if it's worth the effort, but I thought I'd
throw it out there.
-Peff
From: Jonathan Nieder <hidden> Date: 2018-08-14 18:57:48
Hi,
Brandon Williams wrote:
On 08/09, Jeff King wrote:
quoted
One interesting thing about url-encoding is that it's not one-to-one.
This case could also be %2F, which is a different file (on a
case-sensitive filesystem). I think "%20" and "+" are similarly
interchangeable.
If we were decoding the filenames, that's fine. The round-trip is
lossless.
But that's not quite how the new code behaves. We encode the input and
then check to see if it matches an encoding we previously performed. So
if our urlencode routines ever change, this will subtly break.
I don't know how much it's worth caring about. We're not that likely to
change the routines ourself (though certainly a third-party
implementation would need to know our exact url-encoding decisions).
This is exactly the reason why I wanted to get some opinions on what the
best thing to do here would be. I _think_ the best thing would probably
be to write a specific routine to do the conversion, and it wouldn't
even have to be all that complex. Basically I'm just interested in
converting '/' characters so that things no longer behave like
nested directories.
First of all, I think the behavior with this patch is already much
better than the previous status quo. I'm using the patch now and am
very happy with it.
Second, what if we store the pathname in config? We already store the
URL there:
[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks
So we could (as a followup patch) do something like
[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks
gitdirname = plugins%2fhooks
and use that for lookups instead of regenerating the directory name.
What do you think?
Thanks,
Jonathan
From: Jeff King <hidden> Date: 2018-08-14 18:58:02
On Tue, Aug 14, 2018 at 11:04:06AM -0700, Brandon Williams wrote:
quoted
I think this backwards-compatibility is necessary to avoid pain. But
until it goes away, I don't think this is helping the vulnerability from
0383bbb901. Because there the issue was that the submodule name pointed
back into the working tree, so this access() would find the untrusted
working tree code and say "ah, an old-fashioned name!".
[...]
Oh I know that this doesn't help with that vulnerability. As you've
said we fix it and now disallow ".." at the submodule-config level so
really this path is simply about using what we get out of
submodule-config in a more sane manor.
OK, I'm alright with that as long as we are all on the same page. I
think I mistook "this addresses the vulnerability" from your commit
message the wrong way. I took it as "this patch", but reading it again,
you simply mean "the '..' handling we already did".
I do think eventually dropping this back-compatibility could save us
from another directory-escape problem, but it's hard to justify the
real-world pain for a hypothetical benefit. Maybe in a few years we
could get rid of it in a major version bump.
quoted
One interesting thing about url-encoding is that it's not one-to-one.
This case could also be %2F, which is a different file (on a
case-sensitive filesystem). I think "%20" and "+" are similarly
interchangeable.
If we were decoding the filenames, that's fine. The round-trip is
lossless.
But that's not quite how the new code behaves. We encode the input and
then check to see if it matches an encoding we previously performed. So
if our urlencode routines ever change, this will subtly break.
I don't know how much it's worth caring about. We're not that likely to
change the routines ourself (though certainly a third-party
implementation would need to know our exact url-encoding decisions).
This is exactly the reason why I wanted to get some opinions on what the
best thing to do here would be. I _think_ the best thing would probably
be to write a specific routine to do the conversion, and it wouldn't
even have to be all that complex. Basically I'm just interested in
converting '/' characters so that things no longer behave like
nested directories.
I think we benefit from catching names that would trigger filesystem
case-folding, too. If I have submodules with names "foo" and "FOO", we
would not want to confuse them (or at least we should confuse them
equally on all platforms). I doubt you can do anything malicious, but it
might simply be annoying.
That implies to me using a custom function (even if its encoded form
ends up being understandable as url-encoding).
-Peff
From: Stefan Beller <hidden> Date: 2018-08-14 21:08:15
On Tue, Aug 14, 2018 at 11:57 AM Jonathan Nieder [off-list ref] wrote:
Hi,
Brandon Williams wrote:
quoted
On 08/09, Jeff King wrote:
quoted
quoted
One interesting thing about url-encoding is that it's not one-to-one.
This case could also be %2F, which is a different file (on a
case-sensitive filesystem). I think "%20" and "+" are similarly
interchangeable.
If we were decoding the filenames, that's fine. The round-trip is
lossless.
But that's not quite how the new code behaves. We encode the input and
then check to see if it matches an encoding we previously performed. So
if our urlencode routines ever change, this will subtly break.
I don't know how much it's worth caring about. We're not that likely to
change the routines ourself (though certainly a third-party
implementation would need to know our exact url-encoding decisions).
This is exactly the reason why I wanted to get some opinions on what the
best thing to do here would be. I _think_ the best thing would probably
be to write a specific routine to do the conversion, and it wouldn't
even have to be all that complex. Basically I'm just interested in
converting '/' characters so that things no longer behave like
nested directories.
First of all, I think the behavior with this patch is already much
better than the previous status quo. I'm using the patch now and am
very happy with it.
Second, what if we store the pathname in config? We already store the
URL there:
[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks
So we could (as a followup patch) do something like
[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks
gitdirname = plugins%2fhooks
and use that for lookups instead of regenerating the directory name.
What do you think?
As I just looked at worktree code, this sounds intriguing for the wrong
reason (again), as a user may want to point the gitdirname to a repository
that they have already on disk outside the actual superproject. They
would be reinventing worktrees in the submodule space. ;-)
This would open up the security hole that we just had, again.
So we'd have to make sure that the gitdirname (instead of the
now meaningless subsection name) is proof to ../ attacks.
I feel uneasy about this as then the user might come in
and move submodules and repoint the gitdirname...
to a not url encoded path. Exposing this knob just
asks for trouble, no?
On the other hand, the only requirement for the "name" is
now uniqueness, and that is implied with subsections,
so I guess it looks elegant.
What would happen if gitdirname is changed as part of
history? (The same problem we have now with changing
the subsection name)
The more I think about it the less appealing this is, but it looks
elegant.
Stefan
From: Jonathan Nieder <hidden> Date: 2018-08-14 21:12:16
Hi,
Stefan Beller wrote:
On Tue, Aug 14, 2018 at 11:57 AM Jonathan Nieder [off-list ref] wrote:
quoted
Second, what if we store the pathname in config? We already store the
URL there:
[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks
So we could (as a followup patch) do something like
[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks
gitdirname = plugins%2fhooks
and use that for lookups instead of regenerating the directory name.
What do you think?
As I just looked at worktree code, this sounds intriguing for the wrong
reason (again), as a user may want to point the gitdirname to a repository
that they have already on disk outside the actual superproject. They
would be reinventing worktrees in the submodule space. ;-)
This would open up the security hole that we just had, again.
So we'd have to make sure that the gitdirname (instead of the
now meaningless subsection name) is proof to ../ attacks.
I feel uneasy about this as then the user might come in
and move submodules and repoint the gitdirname...
to a not url encoded path. Exposing this knob just
asks for trouble, no?
What if we forbid directory separator characters in the gitdirname?
[...]
What would happen if gitdirname is changed as part of
history? (The same problem we have now with changing
the subsection name)
In this proposal, it would only be read from config, not from
.gitmodules.
Thanks,
Jonathan
From: Stefan Beller <hidden> Date: 2018-08-14 22:34:32
On Tue, Aug 14, 2018 at 2:12 PM Jonathan Nieder [off-list ref] wrote:
Hi,
Stefan Beller wrote:
quoted
On Tue, Aug 14, 2018 at 11:57 AM Jonathan Nieder [off-list ref] wrote:
quoted
quoted
Second, what if we store the pathname in config? We already store the
URL there:
[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks
So we could (as a followup patch) do something like
[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks
gitdirname = plugins%2fhooks
and use that for lookups instead of regenerating the directory name.
What do you think?
As I just looked at worktree code, this sounds intriguing for the wrong
reason (again), as a user may want to point the gitdirname to a repository
that they have already on disk outside the actual superproject. They
would be reinventing worktrees in the submodule space. ;-)
This would open up the security hole that we just had, again.
So we'd have to make sure that the gitdirname (instead of the
now meaningless subsection name) is proof to ../ attacks.
I feel uneasy about this as then the user might come in
and move submodules and repoint the gitdirname...
to a not url encoded path. Exposing this knob just
asks for trouble, no?
What if we forbid directory separator characters in the gitdirname?
Fine with me, but ideally we'd want to allow sharding the
submodules. When you have 1000 submodules
we'd want them not all inside the toplevel "modules/" ?
Up to now we could just wave hands and claim the user
(who is clearly experienced with submodules as they
use so many of them) would shard it properly.
With this scheme we loose the ability to shard.
[...]
quoted
What would happen if gitdirname is changed as part of
history? (The same problem we have now with changing
the subsection name)
In this proposal, it would only be read from config, not from
.gitmodules.
Ah good point. That makes sense.
Stepping back a bit regarding the config:
When I clone gerrit (or any repo using submodules)
$ git clone --recurse-submodules \
https://gerrit.googlesource.com/gerrit g2
[...]
$ cat g2/.git/config
[submodule]
active = .
[submodule "plugins/codemirror-editor"]
url = https://gerrit.googlesource.com/plugins/codemirror-editor
[... more urls to follow...]
Originally we have had the url in the config, (a) that we can change
the URLs after the "git submodule init" and "git submodule update"
step that actually clones the submodule if not present and much more
importantly (b) to know which submodule "was initialized/active".
Now that we have the submodule.active or even
submodule.<name>.active flags, we do not need (b) any more.
So the URL turns into a useless piece of cruft that just is unneeded
and might confuse the user.
So maybe I'd want to propose a patch that removes
submodule.<name>.url from the config once it is cloned.
(I just read up on "submodule sync" again, but that might not
even need special care for this new world)
And with all that said, I think if we can avoid having the submodules
gitdir in the config, the config would look much cleaner, too.
But maybe that is the wrong thing to optimize for. ;-)
It just demonstrates that we'd have a submodule specific
thing again in the config.
So my preference would be to do a similar thing as
url-encoding as that solves the issue of slashes and
potentially of case sensitivity (e.g. encode upper case A
as lower case with underscore _a)
However the transition worries me, as it transitions
within the same namespace. Back then when we
transferred from the .git dir inside the submodules
working tree to the embedded version in the superprojects
.git dir, there was no overlap, and any potential directory
in .git/modules/ that was already there, was highly
unusual, so asking the user for help is the reasonable
thing to do.
But now we might run into issues that has overlap between
old(name as is) and new (urlencoded) world.
So maybe we also want to transition from
modules/<name>
to
submodules/<urlencoded(<name>)>
Thanks,
Stefan
At 15:33 -0700 08 Aug 2018, Brandon Williams [off-list ref] wrote:
Teach "submodule_name_to_gitdir()" to munge a submodule's name (by url
encoding it) before using it to build a path to the submodule's gitdir.
Seems like this will be a problem if it results in names that exceed
NAME_MAX? On common systems that's 255, so it's probably not going to be
common; but it certainly could for some repositories.
From: Jonathan Nieder <hidden> Date: 2018-08-16 02:34:51
Hi again,
Stefan Beller wrote:
On Tue, Aug 14, 2018 at 2:12 PM Jonathan Nieder [off-list ref] wrote:
quoted
What if we forbid directory separator characters in the gitdirname?
Fine with me, but ideally we'd want to allow sharding the
submodules. When you have 1000 submodules
we'd want them not all inside the toplevel "modules/" ?
That's a good reason to permit slashes in the gitdirname.
If I understood the rest of your reply correctly, your worry was about
dangerous gitdirname values in .gitmodules. I never had any wish to
read them from there anyway, so this worry hopefully goes away.
[...]
quoted
In this proposal, it would only be read from config, not from
.gitmodules.
Ah good point. That makes sense.
Stepping back a bit regarding the config:
[...]
Now that we have the submodule.active or even
submodule.<name>.active flags, we do not need (b) any more.
So the URL turns into a useless piece of cruft that just is unneeded
and might confuse the user.
So maybe I'd want to propose a patch that removes
submodule.<name>.url from the config once it is cloned.
(I just read up on "submodule sync" again, but that might not
even need special care for this new world)
And with all that said, I think if we can avoid having the submodules
gitdir in the config, the config would look much cleaner, too.
Yes, I understand and agree with this.
I should further spell out my motivation with this gitdirname
suggestion. The issue that some people have mentioned in this thread
is that urlencoding might not be perfect --- it's pretty close to
perfect, but it's likely we'll come up with some unanticipated needs
later (like sharding) that it doesn't solve. Solving those all right
now would not necessarily be wise, since the thing about unanticipated
needs is that you never know in advance what they will be. ;-)
So it would be nice, for future-proofing, if we can change the naming
scheme later.
As a bonus, that would also make interoperability with other
implementations easier. For example, suppose we mess up in JGit and
urlencode a different set of characters than Git does. Then a mixed
Git + JGit installation would have this subtle bug of the submodule
.git directory not being reused when I switch to and from and branch
not containing that submodule, in some circumstances. That sounds
difficult to support.
Whereas if we have a gitdirname configuration variable, then JGit and
libgit2 and go-git do not have to match the naming scheme Git chooses.
They can try, but if one gets it subtly wrong then that is okay
because the submodule's directory name is right there and easy to look
up.
All at the cost of recording a little configuration somewhere. If we
want to decrease the configuration, we can avoid recording it there in
the easy cases (e.g. when name == gitdirname). That's "just" an
optimization.
And then we have the ability later to handle all the edge cases we
haven't handled yet today:
- sharding when the number of submodules is too large
- case-insensitive filesystems
- path name length limits
- different sets of filesystem-special characters
Sane?
Thanks,
Jonathan
From: Stefan Beller <hidden> Date: 2018-08-16 02:39:57
[...]
all good reasons; ship it :-)
All at the cost of recording a little configuration somewhere. If we
want to decrease the configuration, we can avoid recording it there in
the easy cases (e.g. when name == gitdirname). That's "just" an
optimization.
Sounds good, but gerrit for example would not take advantage of such
optimisation as they have slashes in their submodules. :-(
I wonder if we can optimize further and keep slashes if there is
no conflict (as then name == gitdirname, so it can be optimized).
And then we have the ability later to handle all the edge cases we
haven't handled yet today:
- sharding when the number of submodules is too large
- case-insensitive filesystems
- path name length limits
- different sets of filesystem-special characters
Sane?
I'll keep thinking about it.
FYI: the reduction in configuration was just sent out.
Thanks,
Stefan
From: Jonathan Nieder <hidden> Date: 2018-08-16 02:47:38
Stefan Beller wrote:
Jonathan Nieder wrote:
quoted
All at the cost of recording a little configuration somewhere. If we
want to decrease the configuration, we can avoid recording it there in
the easy cases (e.g. when name == gitdirname). That's "just" an
optimization.
Sounds good, but gerrit for example would not take advantage of such
optimisation as they have slashes in their submodules. :-(
I wonder if we can optimize further and keep slashes if there is
no conflict (as then name == gitdirname, so it can be optimized).
One possibility would be to treat gsub("/", "%2f") as another of the
easy cases.
[...]
quoted
And then we have the ability later to handle all the edge cases we
haven't handled yet today:
- sharding when the number of submodules is too large
- case-insensitive filesystems
- path name length limits
- different sets of filesystem-special characters
Sane?
I'll keep thinking about it.
Thanks.
FYI: the reduction in configuration was just sent out.
From: Brandon Williams <hidden> Date: 2018-08-16 17:34:17
On 08/15, Jonathan Nieder wrote:
Stefan Beller wrote:
quoted
Jonathan Nieder wrote:
quoted
quoted
All at the cost of recording a little configuration somewhere. If we
want to decrease the configuration, we can avoid recording it there in
the easy cases (e.g. when name == gitdirname). That's "just" an
optimization.
Sounds good, but gerrit for example would not take advantage of such
optimisation as they have slashes in their submodules. :-(
I wonder if we can optimize further and keep slashes if there is
no conflict (as then name == gitdirname, so it can be optimized).
One possibility would be to treat gsub("/", "%2f") as another of the
easy cases.
[...]
quoted
quoted
And then we have the ability later to handle all the edge cases we
haven't handled yet today:
- sharding when the number of submodules is too large
- case-insensitive filesystems
- path name length limits
- different sets of filesystem-special characters
Sane?
Seems like a sensible thing to do. Let me work up some patches to
implement this using config primarily and these other schemes as
fallbacks.
quoted
I'll keep thinking about it.
Thanks.
quoted
FYI: the reduction in configuration was just sent out.
From: Brandon Williams <hidden> Date: 2018-08-16 18:19:54
Introduce the config "submodule.<name>.gitdirpath" which is used to
indicate where a submodule's gitdir is located inside of a repository's
"modules" directory.
Signed-off-by: Brandon Williams <redacted>
---
Maybe something like this on top? Do you think we should disallow "../"
in this config, even though it is a repository local configuration and
not shipped in .gitmodules?
submodule.c | 13 ++++++++++++-
t/t7400-submodule-basic.sh | 10 ++++++++++
2 files changed, 22 insertions(+), 1 deletion(-)
@@ -1351,6 +1351,16 @@ test_expect_success 'resolve submodule gitdir in superprojects modules directory$(git-Csuperprojectrev-parse--git-common-dir)/modules/sub/moduleEOFgit-Csuperprojectsubmodule--helpergitdir"sub/module">actual&&+test_cmpexpectactual&&++# Test using "submodule.<name>.gitdirpath" config for where the submodules+# gitdir is located inside the superprojecs "modules" directory+mvsuperproject/.git/modules/sub/modulesuperproject/.git/modules/submodule&&+cat>expect<<-EOF&&+$(git-Csuperprojectrev-parse--git-common-dir)/modules/submodule+EOF+git-Csuperprojectconfig"submodule.sub/module.gitdirpath""submodule"&&+git-Csuperprojectsubmodule--helpergitdir"sub/module">actual&&test_cmpexpectactual'
One interesting thing about url-encoding is that it's not one-to-one.
This case could also be %2F, which is a different file (on a
case-sensitive filesystem). I think "%20" and "+" are similarly
interchangeable.
If we were decoding the filenames, that's fine. The round-trip is
lossless.
But that's not quite how the new code behaves. We encode the input and
then check to see if it matches an encoding we previously performed. So
if our urlencode routines ever change, this will subtly break.
And this is the problem:
a) we have a 'complicated' encoding here, which must never change
b) the "encode and check if it matches", will produce ugly code going forward,
as it tries to differentiate between submodules named "url_encoded(a)"
and "a" (e.g. "a%20b" and "a b" would conflict and we have to resolve
the conflict, although those two names are perfectly fine as they do not
have the original problem of having slashes)
Hence I would propose a simpler encoding:
1) / -> _ ( replace a slash by an underscore)
2) _ -> __ (replace any underscore by 2 underscores, this is just the
escaping mechanism to differentiate a/b and a_b)
3) (optional) instead of putting it all in modules/, use another
directory gitmodules/
for example. this will make sure we can tell if a repository has
been converted
or is stuck with a setup of a current git.
This is exactly the reason why I wanted to get some opinions on what the
best thing to do here would be. I _think_ the best thing would probably
be to write a specific routine to do the conversion, and it wouldn't
even have to be all that complex. Basically I'm just interested in
converting '/' characters so that things no longer behave like
nested directories.
Yeah, then let's just convert '/' with as little overhead as possible.
Thanks,
Stefan
From: Jeff King <hidden> Date: 2018-08-29 05:25:22
On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote:
3) (optional) instead of putting it all in modules/, use another
directory gitmodules/ for example. this will make sure we can tell
if a repository has been converted or is stuck with a setup of a
current git.
I actually kind of like that idea, as it makes the interaction between
old and new names much simpler to reason about.
And since old code won't know about the new names anyway, there's in
theory no downside. In practice, of course, the encoding may often be a
noop, and lazy scripts would continue to work most of the time if you
didn't change out the prefix directory. I'm not sure if that is an
argument for the scheme (because it will suss out broken scripts more
consistently) or against it (because 99% of the time those old scripts
would just happen to work).
quoted
This is exactly the reason why I wanted to get some opinions on what the
best thing to do here would be. I _think_ the best thing would probably
be to write a specific routine to do the conversion, and it wouldn't
even have to be all that complex. Basically I'm just interested in
converting '/' characters so that things no longer behave like
nested directories.
Yeah, then let's just convert '/' with as little overhead as possible.
Do you care about case-folding issues (e.g., submodules "FOO" and "foo"
colliding)?
I'm OK if the answer is "no", but if you do want to deal with it, the
time is probably now.
-Peff
From: Stefan Beller <hidden> Date: 2018-08-29 18:11:06
On Tue, Aug 28, 2018 at 10:25 PM Jeff King [off-list ref] wrote:
On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote:
quoted
3) (optional) instead of putting it all in modules/, use another
directory gitmodules/ for example. this will make sure we can tell
if a repository has been converted or is stuck with a setup of a
current git.
I actually kind of like that idea, as it makes the interaction between
old and new names much simpler to reason about.
And since old code won't know about the new names anyway, there's in
theory no downside. In practice, of course, the encoding may often be a
noop, and lazy scripts would continue to work most of the time if you
didn't change out the prefix directory. I'm not sure if that is an
argument for the scheme (because it will suss out broken scripts more
consistently) or against it (because 99% of the time those old scripts
would just happen to work).
quoted
quoted
This is exactly the reason why I wanted to get some opinions on what the
best thing to do here would be. I _think_ the best thing would probably
be to write a specific routine to do the conversion, and it wouldn't
even have to be all that complex. Basically I'm just interested in
converting '/' characters so that things no longer behave like
nested directories.
Yeah, then let's just convert '/' with as little overhead as possible.
Do you care about case-folding issues (e.g., submodules "FOO" and "foo"
colliding)?
I do. :(
2d84f13dcb6 (config: fix case sensitive subsection names on writing, 2018-08-08)
explains the latest episode of case folding with submodules involved.
I'm OK if the answer is "no", but if you do want to deal with it, the
time is probably now.
Good point. But as soon as we start discussing case sensitivity, we
are drawn down the rabbit hole of funny file names. (Try naming
a submodule "CON1" and obtain it on Windows for example)
So we would need to have a file system specific encoding function for
submodule names, which sounds like a maintenance night mare.
The CON1 example shows that URL encoding may not be enough
on Windows and we'd have to extend the encoding if we care about
FS issues.
Another example would be "a" and "a\b" which would be a mess
in Windows as the '\' would work as a dir separator whereas these
two names were ok on linux. This would be fixed with url encoding.
URL encoding would not fix the case-folding issue that you
mentioned above.
So if I was thinking in the scheme presented above, we could just
have another rule that is
[A-Z] -> _[a-z]
(lowercase capital letters and escape them with an underscore)
But with that rule added, we are inventing a really complicated
encoding scheme already.
From: Jeff King <hidden> Date: 2018-08-29 21:03:51
On Wed, Aug 29, 2018 at 11:10:51AM -0700, Stefan Beller wrote:
quoted
Do you care about case-folding issues (e.g., submodules "FOO" and "foo"
colliding)?
I do. :(
2d84f13dcb6 (config: fix case sensitive subsection names on writing, 2018-08-08)
explains the latest episode of case folding with submodules involved.
quoted
I'm OK if the answer is "no", but if you do want to deal with it, the
time is probably now.
Good point. But as soon as we start discussing case sensitivity, we
are drawn down the rabbit hole of funny file names. (Try naming
a submodule "CON1" and obtain it on Windows for example)
So we would need to have a file system specific encoding function for
submodule names, which sounds like a maintenance night mare.
Hmph. I'd hoped that simply escaping metacharacters and doing some
obvious case-folding would be enough. And I think that would cover most
accidental cases. But yeah, Windows reserved names are basically
indistinguishable from reasonable names. They'd probably need
special-cased.
OTOH, I'm not sure how we handle those for entries in the actual tree.
Poking around git-for-windows/git, I think it uses the magic "\\?"
marker to tell the OS to interpret the name literally.
So I wonder if it might be sufficient to just deal with the more obvious
folding issues. Or as you noted, if we just choose lowercase names as
the normalized form, that might also be enough. :)
So if I was thinking in the scheme presented above, we could just
have another rule that is
[A-Z] -> _[a-z]
(lowercase capital letters and escape them with an underscore)
Yes, that makes even the capitalized "CON" issues go away. It's not a
one-to-one mapping, though ("foo-" and "foo_" map to the same entity).
If we want that, too, I think something like url-encoding is fine, with
the caveat that we simply urlencode _more_ things (i.e., anything not in
[a-z_]).
-Peff
From: Jonathan Nieder <hidden> Date: 2018-08-29 21:09:17
Jeff King wrote:
On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote:
quoted
Yeah, then let's just convert '/' with as little overhead as possible.
Do you care about case-folding issues (e.g., submodules "FOO" and "foo"
colliding)?
I'm OK if the answer is "no", but if you do want to deal with it, the
time is probably now.
Have we rejected the config approach? I really liked the attribute of
not having to solve everything right away. I'm getting scared that
we've forgotten that goal.
It mixes well with Stefan's idea of setting up a new .git/submodules/
directory. We could require that everything in .git/submodules/ have
configuration (or that everything in that directory either have
configuration or be the result of a "very simple" transformation) and
that way, all ambiguity goes away.
Part of the definition of "very simple" could be that the submodule
name must consist of some whitelisted list of characters (including no
uppercase), for example.
Thanks,
Jonathan
From: Stefan Beller <hidden> Date: 2018-08-29 21:10:51
Yes, that makes even the capitalized "CON" issues go away. It's not a
one-to-one mapping, though ("foo-" and "foo_" map to the same entity).
foo_ would map to foo__, and foo- would map to something else.
(foo- as we do not rewrite dashes, yet?)
If we want that, too, I think something like url-encoding is fine, with
the caveat that we simply urlencode _more_ things (i.e., anything not in
[a-z_]).
From: Stefan Beller <hidden> Date: 2018-08-29 21:14:45
On Wed, Aug 29, 2018 at 2:09 PM Jonathan Nieder [off-list ref] wrote:
Jeff King wrote:
quoted
On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote:
quoted
quoted
Yeah, then let's just convert '/' with as little overhead as possible.
Do you care about case-folding issues (e.g., submodules "FOO" and "foo"
colliding)?
I'm OK if the answer is "no", but if you do want to deal with it, the
time is probably now.
Have we rejected the config approach?
I did not reject that approach, but am rather waiting for patches. ;-)
I really liked the attribute of
not having to solve everything right away. I'm getting scared that
we've forgotten that goal.
Eh, sorry for side tracking this issue.
I am just under the impression that the URL encoding is not particularly
good for our use case as it solves just one out of many things, whereas
the one thing (having no slashes) can also be solved in an easier way.
It mixes well with Stefan's idea of setting up a new .git/submodules/
directory. We could require that everything in .git/submodules/ have
configuration (or that everything in that directory either have
configuration or be the result of a "very simple" transformation) and
that way, all ambiguity goes away.
I would not want to have a world where we require that config, but I
would agree to the latter, hence we would need to discuss "very simple".
I guess that are 2 or 3 rules at most.
Part of the definition of "very simple" could be that the submodule
name must consist of some whitelisted list of characters (including no
uppercase), for example.
From: Jonathan Nieder <hidden> Date: 2018-08-29 21:18:07
Hi,
Stefan Beller wrote:
quoted
Yes, that makes even the capitalized "CON" issues go away. It's not a
one-to-one mapping, though ("foo-" and "foo_" map to the same entity).
foo_ would map to foo__, and foo- would map to something else.
(foo- as we do not rewrite dashes, yet?)
quoted
If we want that, too, I think something like url-encoding is fine, with
the caveat that we simply urlencode _more_ things (i.e., anything not in
[a-z_]).
Yeah I think we need more than url encoding now.
Can you say more? Perhaps my expectations have been poisoned by tools
like dpkg-buildpackage that use urlencode. As far as I can tell, it
works fine.
Moreover, urlencode has some attributes that make it a good potential
fit: it's intuitive, it's unambiguous (yes, it's one-to-many, but at
least it's not many-to-many), and people know how to deal with it from
their lives using browsers. Can you spell out for me what problem
we're solving with something more custom?
Stepping back, I am very worried about any design that doesn't give us
the ability to tweak things later. See [1] and [2] for more on that
subject.
Thanks,
Jonathan
[1] https://public-inbox.org/git/20180816023446.GA127655@aiede.svl.corp.google.com/
[2] https://public-inbox.org/git/20180829210913.GF7547@aiede.svl.corp.google.com/
From: Brandon Williams <hidden> Date: 2018-08-29 21:25:09
On 08/29, Stefan Beller wrote:
On Wed, Aug 29, 2018 at 2:09 PM Jonathan Nieder [off-list ref] wrote:
quoted
Jeff King wrote:
quoted
On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote:
quoted
quoted
Yeah, then let's just convert '/' with as little overhead as possible.
Do you care about case-folding issues (e.g., submodules "FOO" and "foo"
colliding)?
I'm OK if the answer is "no", but if you do want to deal with it, the
time is probably now.
Have we rejected the config approach?
I did not reject that approach, but am rather waiting for patches. ;-)
From: Stefan Beller <hidden> Date: 2018-08-29 21:27:58
On Wed, Aug 29, 2018 at 2:18 PM Jonathan Nieder [off-list ref] wrote:
Hi,
Stefan Beller wrote:
quoted
quoted
Yes, that makes even the capitalized "CON" issues go away. It's not a
one-to-one mapping, though ("foo-" and "foo_" map to the same entity).
foo_ would map to foo__, and foo- would map to something else.
(foo- as we do not rewrite dashes, yet?)
quoted
If we want that, too, I think something like url-encoding is fine, with
the caveat that we simply urlencode _more_ things (i.e., anything not in
[a-z_]).
From: Jeff King <hidden> Date: 2018-08-29 21:30:15
On Wed, Aug 29, 2018 at 02:10:37PM -0700, Stefan Beller wrote:
quoted
Yes, that makes even the capitalized "CON" issues go away. It's not a
one-to-one mapping, though ("foo-" and "foo_" map to the same entity).
foo_ would map to foo__, and foo- would map to something else.
(foo- as we do not rewrite dashes, yet?)
Ah, OK, I took your:
[A-Z] -> _[a-z]
to mean "A-Z becomes a-z, and everything else becomes underscore".
If you mean a real one-to-one mapping that allows a-z and only a few
safe metacharacters, then yeah, that's what I was thinking, too.
quoted
If we want that, too, I think something like url-encoding is fine, with
the caveat that we simply urlencode _more_ things (i.e., anything not in
[a-z_]).
Yeah I think we need more than url encoding now.
If you take "url encoding" to only be the mechanical transformation of
quoting, not the set of _what_ gets quoting, we can still stick with it.
We don't need to, but it's probably no worse than inventing our own
set of quoting rules.
-Peff
From: Jeff King <hidden> Date: 2018-08-29 21:32:21
On Wed, Aug 29, 2018 at 02:09:13PM -0700, Jonathan Nieder wrote:
Jeff King wrote:
quoted
On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote:
quoted
quoted
Yeah, then let's just convert '/' with as little overhead as possible.
Do you care about case-folding issues (e.g., submodules "FOO" and "foo"
colliding)?
I'm OK if the answer is "no", but if you do want to deal with it, the
time is probably now.
Have we rejected the config approach? I really liked the attribute of
not having to solve everything right away. I'm getting scared that
we've forgotten that goal.
I personally have no problem with that approach, but I also haven't
thought that hard about it (I was mostly ignoring the discussion since
it seemed like submodule-interested folks, but I happened to see what
looked like a potentially bad idea cc'd to me ;) ).
-Peff
From: Jonathan Nieder <hidden> Date: 2019-01-15 01:25:12
Hi,
In August, 2018, 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.
Thanks again for this. I liked the proposal enough to run Git with
patches implementing it for a while. That said, there were some
unaddressed comments in the review.
I've put a summary in https://crbug.com/git/28 to make this easier to
pick up where we left off. Summary from there of the upstream review:
1. Using urlencoding to escape the slashes is fine, but what if we
want to escape some other character (for example to handle
case-insensitive filesystems)?
Proposal: Store the escaping mapping in config[1] so it can be
modified it in the future:
[submodule "plugin/hooks"]
gitdirname = plugins%2fhooks
2. The urlencoded name could conflict with a submodule that has % in
its name in an existing clone created by an older version of Git.
Proposal: Put submodules in a new .git/submodules/ directory
instead of .git/modules/.
3. These gitdirname settings can clutter up .git/config.
Proposal: For the "easy" cases (e.g. submodule name consisting of
[a-z]*), allow omitting the gitdirname setting.
Is that a fair summary? Are there concerns from the review that I
forgot, or would a new version of the series that addresses those
three problems put us in good shape?
Thanks,
Jonathan
[1] https://public-inbox.org/git/20180816181940.46114-1-bmwill@google.com/
From: Jeff King <hidden> Date: 2019-01-17 17:32:20
On Mon, Jan 14, 2019 at 05:25:07PM -0800, Jonathan Nieder wrote:
I've put a summary in https://crbug.com/git/28 to make this easier to
pick up where we left off. Summary from there of the upstream review:
1. Using urlencoding to escape the slashes is fine, but what if we
want to escape some other character (for example to handle
case-insensitive filesystems)?
Proposal: Store the escaping mapping in config[1] so it can be
modified it in the future:
[submodule "plugin/hooks"]
gitdirname = plugins%2fhooks
I think it might be worth dealing with case-sensitivity _now_, since we
know it's a problem. That doesn't make the problem of "what if we want
to change the mapping later" go away, but it does make it a lot less
likely to come up.
2. The urlencoded name could conflict with a submodule that has % in
its name in an existing clone created by an older version of Git.
Proposal: Put submodules in a new .git/submodules/ directory
instead of .git/modules/.
This proposal is orthogonal to (1), right? I.e., if we store the mapping
then that is what tells us we're using the mapped name.
3. These gitdirname settings can clutter up .git/config.
Proposal: For the "easy" cases (e.g. submodule name consisting of
[a-z]*), allow omitting the gitdirname setting.
Not having thought about it too hard, I suspect that may open back up
corner cases with respect to backwards compatibility and ambiguity.
Are you worried about human-readable clutter? I.e., that .git/config
becomes hard to read? If so, then:
- I doubt this is any worse than the existing tracking-branch config.
- it might be reasonable to store it in .git/submodule-config, and
make sure that .git/config contains a single "[include]path =
submodule-config" line. I've been tempted to do that for
tracking-branch config.
Or are you worried about the cost of parsing those entries? Basically
every git command parses config linearly at least once; this normally
isn't noticeable, but at some size it becomes a problem. I have no idea
what that size is.
If so, then I think we'd want submodule config in its own file but
_without_ an include from the normal config file. That would break
compatibility with anything that tries to use "git config
submodule.foo.path", etc.
That's all just musing. I'm actually not really convinced it's a
problem.
Is that a fair summary? Are there concerns from the review that I
forgot, or would a new version of the series that addresses those
three problems put us in good shape?
I don't really have a strong opinion either way. I still think the
one-way transformation that the patch uses is less elegant than a real
encode/decode round-trip (i.e., what I discussed in [1]). But I admit to
not having thought through all of the details of the encode/decode
thing, and certainly have not written the code.
[1] http://public-inbox.org/git/20180809212602.GA11342@sigill.intra.peff.net/
From: Stefan Beller <hidden> Date: 2019-01-17 17:57:22
On Thu, Jan 17, 2019 at 9:32 AM Jeff King [off-list ref] wrote:
On Mon, Jan 14, 2019 at 05:25:07PM -0800, Jonathan Nieder wrote:
quoted
I've put a summary in https://crbug.com/git/28 to make this easier to
pick up where we left off. Summary from there of the upstream review:
1. Using urlencoding to escape the slashes is fine, but what if we
want to escape some other character (for example to handle
case-insensitive filesystems)?
Proposal: Store the escaping mapping in config[1] so it can be
modified it in the future:
[submodule "plugin/hooks"]
gitdirname = plugins%2fhooks
I think it might be worth dealing with case-sensitivity _now_, since we
know it's a problem. That doesn't make the problem of "what if we want
to change the mapping later" go away, but it does make it a lot less
likely to come up.
Makes sense.
quoted
2. The urlencoded name could conflict with a submodule that has % in
its name in an existing clone created by an older version of Git.
Proposal: Put submodules in a new .git/submodules/ directory
instead of .git/modules/.
This proposal is orthogonal to (1), right? I.e., if we store the mapping
then that is what tells us we're using the mapped name.
Technically true, but it allows for easier implementation:
now we have 2 distinct namespaces, such that we can avoid
double booking easier:
Consider 2 submodules "a b" and "a%20b".
Without (2), (1) is hard to explain as the first might have been encoded
to a%20b or there might have been the second put in place from a
current (old) version of Git. So we'd have to reason about these corner cases.
With (2) in place, we'd only ever have the second in a place "a%2520b"
(if I am to trust https://www.urlencoder.org/)
quoted
3. These gitdirname settings can clutter up .git/config.
Proposal: For the "easy" cases (e.g. submodule name consisting of
[a-z]*), allow omitting the gitdirname setting.
Not having thought about it too hard, I suspect that may open back up
corner cases with respect to backwards compatibility and ambiguity.
Are you worried about human-readable clutter? I.e., that .git/config
becomes hard to read? If so, then:
- I doubt this is any worse than the existing tracking-branch config.
- it might be reasonable to store it in .git/submodule-config, and
make sure that .git/config contains a single "[include]path =
submodule-config" line. I've been tempted to do that for
tracking-branch config.
Or are you worried about the cost of parsing those entries? Basically
every git command parses config linearly at least once; this normally
isn't noticeable, but at some size it becomes a problem. I have no idea
what that size is.
If so, then I think we'd want submodule config in its own file but
_without_ an include from the normal config file. That would break
compatibility with anything that tries to use "git config
submodule.foo.path", etc.
That's all just musing. I'm actually not really convinced it's a
problem.
ok, we can deal with the problem once it arises.
quoted
Is that a fair summary? Are there concerns from the review that I
forgot, or would a new version of the series that addresses those
three problems put us in good shape?
I don't really have a strong opinion either way. I still think the
one-way transformation that the patch uses is less elegant than a real
encode/decode round-trip (i.e., what I discussed in [1]). But I admit to
not having thought through all of the details of the encode/decode
thing, and certainly have not written the code.
The suggestion of adding at least a test for url encoding (2. from your mail)
is sensible.
Stefan