[RFC PATCH 0/2] Allow adding .git files and directories

[RFC PATCH 0/2] Allow adding .git files and directories

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

Hello Everyone,
There have been some requests in the past to allow adding .git files and
directries and nested git repositories as files to a git repository. Usecases
range from including prepared git repos to run tests on to managing arbitrary
files (which may contain git repos) with git/git-annex.

These patches allow this and work well in a quick test. Of course some tests
fail because with this the handling of nested git repos changed. For example
t0008-ignores.sh fails, because in the test repo, the nested git repo is not
added as a submodule and now the files in it aren't ignored.

Now, should the new handling be put behind a configuration variable (allow-dotgit?)
or is the change ok as is and the tests should be fixed?

Regards,
Lukas Straub

Lukas Straub (2):
  dir/read-cache: Allow adding .git files and directories
  dir: Recurse into nested git repos if they aren't submodules

 dir.c        |  9 ++++----
 read-cache.c | 59 +++++++++++++++++++++++++++++++---------------------
 2 files changed, 39 insertions(+), 29 deletions(-)

-- 
2.28.0.217.ge805fe7219

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC PATCH 1/2] dir/read-cache: Allow adding .git files and directories

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 4431 bytes --]

Prevent adding .git/.gitmodules only in the root of the repository.

This allows adding .git files and directories as long as they are
not in the root.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 dir.c        |  3 ++-
 read-cache.c | 59 +++++++++++++++++++++++++++++++---------------------
 2 files changed, 37 insertions(+), 25 deletions(-)

diff --git a/dir.c b/dir.c
index fe64be30ed..a959885f50 100644
--- a/dir.c
+++ b/dir.c
@@ -2145,7 +2145,8 @@ static enum path_treatment treat_path(struct dir_struct *dir,
 	if (!cdir->d_name)
 		return treat_path_fast(dir, untracked, cdir, istate, path,
 				       baselen, pathspec);
-	if (is_dot_or_dotdot(cdir->d_name) || !fspathcmp(cdir->d_name, ".git"))
+	if (is_dot_or_dotdot(cdir->d_name) ||
+	    (path->len == 0 && !fspathcmp(cdir->d_name, ".git")))
 		return path_none;
 	strbuf_setlen(path, baselen);
 	strbuf_addstr(path, cdir->d_name);
diff --git a/read-cache.c b/read-cache.c
index 8ed1c29b54..7fb25c23d5 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -906,12 +906,7 @@ int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
 }
 
 /*
- * We fundamentally don't like some paths: we don't want
- * dot or dot-dot anywhere, and for obvious reasons don't
- * want to recurse into ".git" either.
- *
- * Also, we don't want double slashes or slashes at the
- * end that can make pathnames ambiguous.
+ * We don't want dot or dot-dot anywhere
  */
 static int verify_dotfile(const char *rest, unsigned mode)
 {
@@ -922,10 +917,24 @@ static int verify_dotfile(const char *rest, unsigned mode)
 	 */
 
 	/* "." is not allowed */
-	if (*rest == '\0' || is_dir_sep(*rest))
+	if (rest[0] == '\0' || is_dir_sep(rest[0]))
+		return 0;
+
+	/* ".." is not allowed */
+	if (rest[0] == '.' && (rest[1] == '\0' || is_dir_sep(rest[1])))
 		return 0;
 
-	switch (*rest) {
+	return 1;
+}
+
+static int verify_dotgit(const char *rest, unsigned mode)
+{
+	/*
+	 * The first character was '.', but that
+	 * has already been discarded, we now test
+	 * the rest.
+	 */
+
 	/*
 	 * ".git" followed by NUL or slash is bad. Note that we match
 	 * case-insensitively here, even if ignore_case is not set.
@@ -935,12 +944,11 @@ static int verify_dotfile(const char *rest, unsigned mode)
 	 * Once we've seen ".git", we can also find ".gitmodules", etc (also
 	 * case-insensitively).
 	 */
-	case 'g':
-	case 'G':
+	if (rest[0] == 'g' || rest[0] == 'G') {
 		if (rest[1] != 'i' && rest[1] != 'I')
-			break;
+			return 1;
 		if (rest[2] != 't' && rest[2] != 'T')
-			break;
+			return 1;
 		if (rest[3] == '\0' || is_dir_sep(rest[3]))
 			return 0;
 		if (S_ISLNK(mode)) {
@@ -949,17 +957,16 @@ static int verify_dotfile(const char *rest, unsigned mode)
 			    (*rest == '\0' || is_dir_sep(*rest)))
 				return 0;
 		}
-		break;
-	case '.':
-		if (rest[1] == '\0' || is_dir_sep(rest[1]))
-			return 0;
 	}
+
 	return 1;
 }
 
 int verify_path(const char *path, unsigned mode)
 {
+	const char *orig_path = path;
 	char c = 0;
+	#define is_root(len) ((len) == 0)
 
 	if (has_dos_drive_prefix(path))
 		return 0;
@@ -973,8 +980,7 @@ int verify_path(const char *path, unsigned mode)
 			return 1;
 		if (is_dir_sep(c)) {
 inside:
-			if (protect_hfs) {
-
+			if (protect_hfs && is_root(path - orig_path)) {
 				if (is_hfs_dotgit(path))
 					return 0;
 				if (S_ISLNK(mode)) {
@@ -982,11 +988,7 @@ int verify_path(const char *path, unsigned mode)
 						return 0;
 				}
 			}
-			if (protect_ntfs) {
-#ifdef GIT_WINDOWS_NATIVE
-				if (c == '\\')
-					return 0;
-#endif
+			if (protect_ntfs && is_root(path - orig_path)) {
 				if (is_ntfs_dotgit(path))
 					return 0;
 				if (S_ISLNK(mode)) {
@@ -995,11 +997,20 @@ int verify_path(const char *path, unsigned mode)
 				}
 			}
 
+#ifdef GIT_WINDOWS_NATIVE
+			if (protect_ntfs && c == '\\')
+				return 0;
+#endif
+
 			c = *path++;
 			if ((c == '.' && !verify_dotfile(path, mode)) ||
 			    is_dir_sep(c) || c == '\0')
 				return 0;
-		} else if (c == '\\' && protect_ntfs) {
+			if (c == '.' && is_root(path - orig_path -1) &&
+			    !verify_dotgit(path, mode))
+				return 0;
+		} else if (c == '\\' && protect_ntfs &&
+			   is_root(path - orig_path)) {
 			if (is_ntfs_dotgit(path))
 				return 0;
 			if (S_ISLNK(mode)) {
-- 
2.28.0.217.ge805fe7219


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC PATCH 2/2] dir: Recurse into nested git repos if they aren't submodules

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

Only stop recursing into a nested git repository if it actually is
a submodule.

This allows adding nested git repositories as files as usual.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 dir.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/dir.c b/dir.c
index a959885f50..359015748f 100644
--- a/dir.c
+++ b/dir.c
@@ -1787,10 +1787,8 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
 
 	if ((dir->flags & DIR_SKIP_NESTED_GIT) ||
 		!(dir->flags & DIR_NO_GITLINKS)) {
-		struct strbuf sb = STRBUF_INIT;
-		strbuf_addstr(&sb, dirname);
-		nested_repo = is_nonbare_repository_dir(&sb);
-		strbuf_release(&sb);
+		nested_repo = !!submodule_from_path(the_repository, &null_oid,
+						    dirname);
 	}
 	if (nested_repo)
 		return ((dir->flags & DIR_SKIP_NESTED_GIT) ? path_none :
-- 
2.28.0.217.ge805fe7219

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Junio C Hamano]

Lukas Straub <lukasstraub2@web.de> writes:

> These patches allow this and work well in a quick test. Of course some tests
> fail because with this the handling of nested git repos changed.

In other words, this breaks the workflow existing users rely on,
right?  I do not know if such a behaviour ever needs to exist even
as an opt-in feature, but it definitely feels wrong to make the
behaviour these patches introduce the default.

Thanks.

RE: [RFC PATCH 0/2] Allow adding .git files and directories

[Randall S. Becker]

On August 19, 2020 2:04 PM, Junio C Hamano
> To: Lukas Straub <lukasstraub2@web.de>
> Cc: git <git@vger.kernel.org>; Elijah Newren <newren@gmail.com>;
> Brandon Williams <bwilliams.eng@gmail.com>; Johannes Schindelin
> <Johannes.Schindelin@gmx.de>; Jeff King <peff@peff.net>
> Subject: Re: [RFC PATCH 0/2] Allow adding .git files and directories
> 
> Lukas Straub <lukasstraub2@web.de> writes:
> 
> > These patches allow this and work well in a quick test. Of course some
> > tests fail because with this the handling of nested git repos changed.
> 
> In other words, this breaks the workflow existing users rely on, right?  I
do
> not know if such a behaviour ever needs to exist even as an opt-in
feature,
> but it definitely feels wrong to make the behaviour these patches
introduce
> the default.

I am concerned about broader implications. I might be stating the obvious,
but a key security vulnerability that would open up here is to put contents
of files like .git/config into a repository. This capability would allow
scripts to be introduced without the explicit knowledge of the user. While
I'm sure some of the heavy clean/smudge users might appreciate it, this can
represent a vector for the introduction of hostile code into an environment.
While this enhancement seems like a good idea on the surface, if it goes
forward, it should not be the default and should not be under the control of
the upstream repository. You would need loads of warnings about potential
script hazards at the very least presented to the user, beyond what is
already documented in git. This change would not interoperate with JGit -
not that that is a huge concern here, but heavy Jenkins and other pipeline
users could be significantly impacted.

Just putting my CSIO hat on here. We would need a system-wide setting to
prohibit users from using this capability.

Sincerely,
Randall

-- Brief whoami:
 NonStop developer since approximately 211288444200000000
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

On Wed, 19 Aug 2020 11:03:30 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Lukas Straub <lukasstraub2@web.de> writes:
> 
> > These patches allow this and work well in a quick test. Of course some tests
> > fail because with this the handling of nested git repos changed.  
> 
> In other words, this breaks the workflow existing users rely on,
> right?  I do not know if such a behaviour ever needs to exist even
> as an opt-in feature, but it definitely feels wrong to make the
> behaviour these patches introduce the default.

Well, the current behavior is that nested repos (that are not submodules)
are completely ignored and none of the files within can be added. So the
old behavior can be restored with .gitignore. The same goes for files/dirs
named .git.

Of course I don't know what the current policy for behavioral changes in
git is, but I see that there have been such changes in the past.

Regards,
Lukas Straub

> Thanks.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Junio C Hamano]

"Randall S. Becker" <rsbecker@nexbridge.com> writes:

> Just putting my CSIO hat on here. We would need a system-wide setting to
> prohibit users from using this capability.

Or just discard this patch, which is a lot simpler.  I don't see any
need for this one.

RE: [RFC PATCH 0/2] Allow adding .git files and directories

[Randall S. Becker]

On August 19, 2020 2:48 PM, Lukas Straub wrote:
> To: Junio C Hamano <gitster@pobox.com>
> Cc: git <git@vger.kernel.org>; Elijah Newren <newren@gmail.com>;
> Brandon Williams <bwilliams.eng@gmail.com>; Johannes Schindelin
> <Johannes.Schindelin@gmx.de>; Jeff King <peff@peff.net>
> Subject: Re: [RFC PATCH 0/2] Allow adding .git files and directories
> 
> On Wed, 19 Aug 2020 11:03:30 -0700
> Junio C Hamano <gitster@pobox.com> wrote:
> 
> > Lukas Straub <lukasstraub2@web.de> writes:
> >
> > > These patches allow this and work well in a quick test. Of course
> > > some tests fail because with this the handling of nested git repos
> changed.
> >
> > In other words, this breaks the workflow existing users rely on,
> > right?  I do not know if such a behaviour ever needs to exist even as
> > an opt-in feature, but it definitely feels wrong to make the behaviour
> > these patches introduce the default.
> 
> Well, the current behavior is that nested repos (that are not submodules)
are
> completely ignored and none of the files within can be added. So the old
> behavior can be restored with .gitignore. The same goes for files/dirs
named
> .git.
> 
> Of course I don't know what the current policy for behavioral changes in
git
> is, but I see that there have been such changes in the past.

I honestly am concerned about a repeat of things like
https://nvd.nist.gov/vuln/detail/CVE-2019-19604 (the submodule update
problem). This change in behaviour is of serious concern from a risk
standpoint. To be blunt, I don't think users on my platform will move to a
version of git that supports this by default.

Sincerely,
Randall

-- Brief whoami:
 NonStop developer since approximately 211288444200000000
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2396 bytes --]

On Wed, 19 Aug 2020 14:47:18 -0400
"Randall S. Becker" <rsbecker@nexbridge.com> wrote:

> On August 19, 2020 2:04 PM, Junio C Hamano
> > To: Lukas Straub <lukasstraub2@web.de>
> > Cc: git <git@vger.kernel.org>; Elijah Newren <newren@gmail.com>;
> > Brandon Williams <bwilliams.eng@gmail.com>; Johannes Schindelin
> > <Johannes.Schindelin@gmx.de>; Jeff King <peff@peff.net>
> > Subject: Re: [RFC PATCH 0/2] Allow adding .git files and directories
> > 
> > Lukas Straub <lukasstraub2@web.de> writes:
> >   
> > > These patches allow this and work well in a quick test. Of course some
> > > tests fail because with this the handling of nested git repos changed.  
> > 
> > In other words, this breaks the workflow existing users rely on, right?  I  
> do
> > not know if such a behaviour ever needs to exist even as an opt-in  
> feature,
> > but it definitely feels wrong to make the behaviour these patches  
> introduce
> > the default.  
> 
> I am concerned about broader implications. I might be stating the obvious,
> but a key security vulnerability that would open up here is to put contents
> of files like .git/config into a repository. This capability would allow
> scripts to be introduced without the explicit knowledge of the user. While
> I'm sure some of the heavy clean/smudge users might appreciate it, this can
> represent a vector for the introduction of hostile code into an environment.
> While this enhancement seems like a good idea on the surface, if it goes
> forward, it should not be the default and should not be under the control of
> the upstream repository. You would need loads of warnings about potential
> script hazards at the very least presented to the user, beyond what is
> already documented in git. This change would not interoperate with JGit -
> not that that is a huge concern here, but heavy Jenkins and other pipeline
> users could be significantly impacted.
> 
> Just putting my CSIO hat on here. We would need a system-wide setting to
> prohibit users from using this capability.

Good catch, this is a very valid concern. So at least opt-in via git-config is needed.

Regards,
Lukas Straub

> Sincerely,
> Randall
> 
> -- Brief whoami:
>  NonStop developer since approximately 211288444200000000
>  UNIX developer since approximately 421664400
> -- In my real life, I talk too much.
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [RFC PATCH 0/2] Allow adding .git files and directories

[Randall S. Becker]

On August 19, 2020 3:09 PM, Junio C Hamano wrote:
> To: Randall S. Becker <rsbecker@nexbridge.com>
> Cc: 'Lukas Straub' <lukasstraub2@web.de>; 'git' <git@vger.kernel.org>;
> 'Elijah Newren' <newren@gmail.com>; 'Brandon Williams'
> <bwilliams.eng@gmail.com>; 'Johannes Schindelin'
> <Johannes.Schindelin@gmx.de>; 'Jeff King' <peff@peff.net>
> Subject: Re: [RFC PATCH 0/2] Allow adding .git files and directories
> 
> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> 
> > Just putting my CSIO hat on here. We would need a system-wide setting
> > to prohibit users from using this capability.
> 
> Or just discard this patch, which is a lot simpler.  I don't see any need for this
> one.

If I was a committer, that would be my take on it too. 😉

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Jeff King]

On Wed, Aug 19, 2020 at 12:09:11PM -0700, Junio C Hamano wrote:

> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> 
> > Just putting my CSIO hat on here. We would need a system-wide setting to
> > prohibit users from using this capability.
> 
> Or just discard this patch, which is a lot simpler.  I don't see any
> need for this one.

Yes. Configurability is a lot more complicated than you might think.
Because it's not just system-wide, but _ecosystem_ wide.

Right now git-fsck complains about ".git" appearing in a tree, and that
check blocks people from pushing such trees to any hosting sites that
enable transfer.fsckObjects (which includes hosters like GitHub). So
you'd not only need to allow the behavior to be loosened for all of the
people using the feature, but you'd need to convince server-side hosters
to loosen their config. And because part of the purpose is to protect
downstream clients from attacks, I doubt that public hosters like GitHub
would do so.

It _could_ still be useful in a more isolated environment (e.g., your
company server that is serving only internal repos to employees). But I
have misgivings about a feature that lets people intentionally create
repositories whose history cannot ever interact with other users who
haven't set a special config flag. It's one thing to say "to take
advantage of this feature, we must all agree to have version X, or set
flag Y". But it's another to bake that restriction into the repository
history for all time.

-Peff

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It _could_ still be useful in a more isolated environment (e.g., your
> company server that is serving only internal repos to employees). But I
> have misgivings about a feature that lets people intentionally create
> repositories whose history cannot ever interact with other users who
> haven't set a special config flag. It's one thing to say "to take
> advantage of this feature, we must all agree to have version X, or set
> flag Y". But it's another to bake that restriction into the repository
> history for all time.

If people want a pre-prepared repository propagated to CI
environment and keep trakc of the state of such repository over
time, for example, they can use (versioned) tarballs.  Such a
tarball won't automatically get extracted after "git pull" (which
is a feature), but those who want such a pre-prepared repository
for CI can make the extraction step as a part of their CI build
procedure.

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Jeff King]

On Wed, Aug 19, 2020 at 01:32:28PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > It _could_ still be useful in a more isolated environment (e.g., your
> > company server that is serving only internal repos to employees). But I
> > have misgivings about a feature that lets people intentionally create
> > repositories whose history cannot ever interact with other users who
> > haven't set a special config flag. It's one thing to say "to take
> > advantage of this feature, we must all agree to have version X, or set
> > flag Y". But it's another to bake that restriction into the repository
> > history for all time.
> 
> If people want a pre-prepared repository propagated to CI
> environment and keep trakc of the state of such repository over
> time, for example, they can use (versioned) tarballs.  Such a
> tarball won't automatically get extracted after "git pull" (which
> is a feature), but those who want such a pre-prepared repository
> for CI can make the extraction step as a part of their CI build
> procedure.

Yeah, I almost went into more detail there. There are lots of solutions
that make accessing an embedded sub-repository only one command away for
the person who clones. :)  Some others are:

  - just call it "foo.git", and "mv foo.git .git" solves it (you'd
    probably want to "git checkout -f" after that, but even if it were
    embedded it seems silly to hold the data in two separate formats
    anyway

  - just hold a bare repository ("foo.git") and then clone it

etc. I think this is really a solution in search of a problem.

-Peff

RE: [RFC PATCH 0/2] Allow adding .git files and directories

[Randall S. Becker]

On August 19, 2020 4:38 PM, Jeff King wrote:
> On Wed, Aug 19, 2020 at 01:32:28PM -0700, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> >
> > > It _could_ still be useful in a more isolated environment (e.g.,
> > > your company server that is serving only internal repos to
> > > employees). But I have misgivings about a feature that lets people
> > > intentionally create repositories whose history cannot ever interact
> > > with other users who haven't set a special config flag. It's one
> > > thing to say "to take advantage of this feature, we must all agree
> > > to have version X, or set flag Y". But it's another to bake that
> > > restriction into the repository history for all time.
> >
> > If people want a pre-prepared repository propagated to CI environment
> > and keep trakc of the state of such repository over time, for example,
> > they can use (versioned) tarballs.  Such a tarball won't automatically
> > get extracted after "git pull" (which is a feature), but those who
> > want such a pre-prepared repository for CI can make the extraction
> > step as a part of their CI build procedure.
> 
> Yeah, I almost went into more detail there. There are lots of solutions that
> make accessing an embedded sub-repository only one command away for
> the person who clones. :)  Some others are:
> 
>   - just call it "foo.git", and "mv foo.git .git" solves it (you'd
>     probably want to "git checkout -f" after that, but even if it were
>     embedded it seems silly to hold the data in two separate formats
>     anyway
> 
>   - just hold a bare repository ("foo.git") and then clone it

That is a reasonable approach that will not get you ping'd on the CVE database for someone who wants to do this, which is a key concern of mine.

> etc. I think this is really a solution in search of a problem.

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Johannes Schindelin]

Hi,

On Wed, 19 Aug 2020, Jeff King wrote:

> On Wed, Aug 19, 2020 at 01:32:28PM -0700, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > It _could_ still be useful in a more isolated environment (e.g., your
> > > company server that is serving only internal repos to employees). But I
> > > have misgivings about a feature that lets people intentionally create
> > > repositories whose history cannot ever interact with other users who
> > > haven't set a special config flag. It's one thing to say "to take
> > > advantage of this feature, we must all agree to have version X, or set
> > > flag Y". But it's another to bake that restriction into the repository
> > > history for all time.
> >
> > If people want a pre-prepared repository propagated to CI
> > environment and keep trakc of the state of such repository over
> > time, for example, they can use (versioned) tarballs.  Such a
> > tarball won't automatically get extracted after "git pull" (which
> > is a feature), but those who want such a pre-prepared repository
> > for CI can make the extraction step as a part of their CI build
> > procedure.
>
> Yeah, I almost went into more detail there. There are lots of solutions
> that make accessing an embedded sub-repository only one command away for
> the person who clones. :)  Some others are:
>
>   - just call it "foo.git", and "mv foo.git .git" solves it (you'd
>     probably want to "git checkout -f" after that, but even if it were
>     embedded it seems silly to hold the data in two separate formats
>     anyway
>
>   - just hold a bare repository ("foo.git") and then clone it
>
> etc. I think this is really a solution in search of a problem.

As far as testing goes, libgit2 already commits such bare repositories
under names like `shallow.git`.

So yeah, I also do not see the need for this patch series, and I do have
serious doubts about wanting to risk the security implications.

Ciao,
Dscho

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2300 bytes --]

On Wed, 19 Aug 2020 16:38:25 -0400
Jeff King <peff@peff.net> wrote:

> On Wed, Aug 19, 2020 at 01:32:28PM -0700, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> >   
> > > It _could_ still be useful in a more isolated environment (e.g., your
> > > company server that is serving only internal repos to employees). But I
> > > have misgivings about a feature that lets people intentionally create
> > > repositories whose history cannot ever interact with other users who
> > > haven't set a special config flag. It's one thing to say "to take
> > > advantage of this feature, we must all agree to have version X, or set
> > > flag Y". But it's another to bake that restriction into the repository
> > > history for all time.  
> > 
> > If people want a pre-prepared repository propagated to CI
> > environment and keep trakc of the state of such repository over
> > time, for example, they can use (versioned) tarballs.  Such a
> > tarball won't automatically get extracted after "git pull" (which
> > is a feature), but those who want such a pre-prepared repository
> > for CI can make the extraction step as a part of their CI build
> > procedure.  
> 
> Yeah, I almost went into more detail there. There are lots of solutions
> that make accessing an embedded sub-repository only one command away for
> the person who clones. :)  Some others are:
> 
>   - just call it "foo.git", and "mv foo.git .git" solves it (you'd
>     probably want to "git checkout -f" after that, but even if it were
>     embedded it seems silly to hold the data in two separate formats
>     anyway
> 
>   - just hold a bare repository ("foo.git") and then clone it
> 
> etc. I think this is really a solution in search of a problem.
> 
> -Peff

Yes, there are many workarounds and they work well in the CI usecase. However,
for the arbitrary files usecase there is no good workaround. I currently use
a script which iterates over the tree and renames .git -> dotgit before running
any git command and back again afterwards, but it is slow and brittle. I toyed
with the idea of writing a FUSE filesystem to do the renaming, but it is
needlessly complex and hurts performance.

Really, this problem should be solved in git itself.

Regards,
Lukas Straub

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2163 bytes --]

On Wed, 19 Aug 2020 15:16:19 -0400
"Randall S. Becker" <rsbecker@nexbridge.com> wrote:

> On August 19, 2020 2:48 PM, Lukas Straub wrote:
> > To: Junio C Hamano <gitster@pobox.com>
> > Cc: git <git@vger.kernel.org>; Elijah Newren <newren@gmail.com>;
> > Brandon Williams <bwilliams.eng@gmail.com>; Johannes Schindelin
> > <Johannes.Schindelin@gmx.de>; Jeff King <peff@peff.net>
> > Subject: Re: [RFC PATCH 0/2] Allow adding .git files and directories
> > 
> > On Wed, 19 Aug 2020 11:03:30 -0700
> > Junio C Hamano <gitster@pobox.com> wrote:
> >   
> > > Lukas Straub <lukasstraub2@web.de> writes:
> > >  
> > > > These patches allow this and work well in a quick test. Of course
> > > > some tests fail because with this the handling of nested git repos  
> > changed.  
> > >
> > > In other words, this breaks the workflow existing users rely on,
> > > right?  I do not know if such a behaviour ever needs to exist even as
> > > an opt-in feature, but it definitely feels wrong to make the behaviour
> > > these patches introduce the default.  
> > 
> > Well, the current behavior is that nested repos (that are not submodules)  
> are
> > completely ignored and none of the files within can be added. So the old
> > behavior can be restored with .gitignore. The same goes for files/dirs  
> named
> > .git.
> > 
> > Of course I don't know what the current policy for behavioral changes in  
> git
> > is, but I see that there have been such changes in the past.  
> 
> I honestly am concerned about a repeat of things like
> https://nvd.nist.gov/vuln/detail/CVE-2019-19604 (the submodule update
> problem). This change in behaviour is of serious concern from a risk
> standpoint. To be blunt, I don't think users on my platform will move to a
> version of git that supports this by default.

As discussed I will make it opt-in via git-config. I hope this resolves your concerns.

Regards,
Lukas Straub

> Sincerely,
> Randall
>
> -- Brief whoami:
>  NonStop developer since approximately 211288444200000000
>  UNIX developer since approximately 421664400
> -- In my real life, I talk too much.
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2156 bytes --]

On Wed, 19 Aug 2020 16:17:36 -0400
Jeff King <peff@peff.net> wrote:

> On Wed, Aug 19, 2020 at 12:09:11PM -0700, Junio C Hamano wrote:
> 
> > "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> >   
> > > Just putting my CSIO hat on here. We would need a system-wide setting to
> > > prohibit users from using this capability.  
> > 
> > Or just discard this patch, which is a lot simpler.  I don't see any
> > need for this one.  
> 
> Yes. Configurability is a lot more complicated than you might think.
> Because it's not just system-wide, but _ecosystem_ wide.
> 
> Right now git-fsck complains about ".git" appearing in a tree, and that
> check blocks people from pushing such trees to any hosting sites that
> enable transfer.fsckObjects (which includes hosters like GitHub). So
> you'd not only need to allow the behavior to be loosened for all of the
> people using the feature, but you'd need to convince server-side hosters
> to loosen their config. And because part of the purpose is to protect
> downstream clients from attacks, I doubt that public hosters like GitHub
> would do so.

I guess they can add a checkbox to their (secured) web-ui to configure
this.

> It _could_ still be useful in a more isolated environment (e.g., your
> company server that is serving only internal repos to employees). But I
> have misgivings about a feature that lets people intentionally create
> repositories whose history cannot ever interact with other users who
> haven't set a special config flag. It's one thing to say "to take
> advantage of this feature, we must all agree to have version X, or set
> flag Y". But it's another to bake that restriction into the repository
> history for all time.

Good point. I don't know how we can resolve this, so I will add warning about
this (and the security concerns) to the config docs.

In the worst-case where the hosting sites don't adopt this config, the user
enables and uses this feature despite the warnings and then wants to use a
hosting site, he can still rewrite the history. Not nice, but no disaster
either.

Regards,
Lukas Straub

> 
> -Peff

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Jeff King]

On Thu, Aug 20, 2020 at 01:34:45PM +0200, Lukas Straub wrote:

> Yes, there are many workarounds and they work well in the CI usecase. However,
> for the arbitrary files usecase there is no good workaround. I currently use
> a script which iterates over the tree and renames .git -> dotgit before running
> any git command and back again afterwards, but it is slow and brittle. I toyed
> with the idea of writing a FUSE filesystem to do the renaming, but it is
> needlessly complex and hurts performance.
> 
> Really, this problem should be solved in git itself.

It is unclear to me why need to hold many sub-repositories within the
parent one, nor what Git operations you expect to perform over them. And
what disadvantages your script solution has.

Perhaps you can give a more concrete use-case (but before you spend a
lot of time doing so, I'll warn you that I find it pretty unlikely that
it will cross the bar necessary to counter the downsides we've discussed
so far).

-Peff

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Jeff King]

On Thu, Aug 20, 2020 at 02:37:55PM +0200, Lukas Straub wrote:

> > Right now git-fsck complains about ".git" appearing in a tree, and that
> > check blocks people from pushing such trees to any hosting sites that
> > enable transfer.fsckObjects (which includes hosters like GitHub). So
> > you'd not only need to allow the behavior to be loosened for all of the
> > people using the feature, but you'd need to convince server-side hosters
> > to loosen their config. And because part of the purpose is to protect
> > downstream clients from attacks, I doubt that public hosters like GitHub
> > would do so.
> 
> I guess they can add a checkbox to their (secured) web-ui to configure
> this.

No, that would defeat the purpose. Hosting sites aren't protecting users
from themselves. They're concerned about malicious actors pushing up
objects that violate expectations and make the hosting site a vector for
attacks. Either against other parts of the site, or against downstream
users who aren't running fully-patched versions of Git (or perhaps are
running a misconfigured one, if there's a known-unsafe configuration).

I don't know of a version of Git that's vulnerable to ".git" (it should
be blocked from entering the index by verify_path()), but the fsck
checks are one layer of a multiple-layer defense against such problems
(which have helped us contain other path-related vulnerabilities).
Letting the potential attacker turn off that layer makes it pointless.

> In the worst-case where the hosting sites don't adopt this config, the user
> enables and uses this feature despite the warnings and then wants to use a
> hosting site, he can still rewrite the history. Not nice, but no disaster
> either.

In general, I do like to err on the side of providing users tools to
shoot themselves in the foot. But this feels like it crosses even my bar
for a foot-gun, especially when there are other solutions available.

-Peff

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2608 bytes --]

On Thu, 20 Aug 2020 09:01:25 -0400
Jeff King <peff@peff.net> wrote:

> On Thu, Aug 20, 2020 at 01:34:45PM +0200, Lukas Straub wrote:
> 
> > Yes, there are many workarounds and they work well in the CI usecase. However,
> > for the arbitrary files usecase there is no good workaround. I currently use
> > a script which iterates over the tree and renames .git -> dotgit before running
> > any git command and back again afterwards, but it is slow and brittle. I toyed
> > with the idea of writing a FUSE filesystem to do the renaming, but it is
> > needlessly complex and hurts performance.
> > 
> > Really, this problem should be solved in git itself.  
> 
> It is unclear to me why need to hold many sub-repositories within the
> parent one, nor what Git operations you expect to perform over them. And
> what disadvantages your script solution has.
> 
> Perhaps you can give a more concrete use-case (but before you spend a
> lot of time doing so, I'll warn you that I find it pretty unlikely that
> it will cross the bar necessary to counter the downsides we've discussed
> so far).
> 
> -Peff

I store all my files in several git(-annex) repositories. By "files" I mean
anything you might find in your home directory across your devices, anything
on usb thumbdrives, sd cards and maybe your home NAS. And anything you would
usually use google photos, iCloud, dropbox, etc. for.
Concrete examples:
I store the home directories (containing git repos) of two retired machines
in such a repository. I don't store the homes in a archive file as I want to
use git-annex's ability to only have the contents of files I need on my
laptop.
I store my development directory containing several(130) git/svn/unversioned
projects in another repo. This allows me to version and sync everything
(including WIPs) across my machines. I tried alternative workflows and they
didn't work out.

In both cases, the script introduces a delay of ~10 seconds to every git
command I run in the outer repository and I have to remember to use the script
on these repos.
Moreover, I can't use the git-annex assistant, which watches the repo and
automatically commits and syncs file changes.
In my opinion, I want to be able to set the allow-dotgit config and it'll
just work without the delay and without having to remember the script.

The downsides we discussed don't apply in this usecase. These are mostly
personal files, so I wont upload them to any hosting site (not even private
ones). There is no security impact as I only sync with trusted devices.

Regards,
Lukas Straub

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [RFC PATCH 0/2] Allow adding .git files and directories

[Randall S. Becker]

On August 21, 2020 8:40 AM, Lukas Straub wrote:
> On Thu, 20 Aug 2020 09:01:25 -0400
> Jeff King <peff@peff.net> wrote:
> 
> > On Thu, Aug 20, 2020 at 01:34:45PM +0200, Lukas Straub wrote:
> >
> > > Yes, there are many workarounds and they work well in the CI
> > > usecase. However, for the arbitrary files usecase there is no good
> > > workaround. I currently use a script which iterates over the tree
> > > and renames .git -> dotgit before running any git command and back
> > > again afterwards, but it is slow and brittle. I toyed with the idea
> > > of writing a FUSE filesystem to do the renaming, but it is needlessly
> complex and hurts performance.
> > >
> > > Really, this problem should be solved in git itself.
> >
> > It is unclear to me why need to hold many sub-repositories within the
> > parent one, nor what Git operations you expect to perform over them.
> > And what disadvantages your script solution has.
> >
> > Perhaps you can give a more concrete use-case (but before you spend a
> > lot of time doing so, I'll warn you that I find it pretty unlikely
> > that it will cross the bar necessary to counter the downsides we've
> > discussed so far).
> >
> > -Peff
> 
> I store all my files in several git(-annex) repositories. By "files" I
mean
> anything you might find in your home directory across your devices,
anything
> on usb thumbdrives, sd cards and maybe your home NAS. And anything you
> would usually use google photos, iCloud, dropbox, etc. for.
> Concrete examples:
> I store the home directories (containing git repos) of two retired
machines in
> such a repository. I don't store the homes in a archive file as I want to
use
> git-annex's ability to only have the contents of files I need on my
laptop.
> I store my development directory containing several(130)
> git/svn/unversioned projects in another repo. This allows me to version
and
> sync everything (including WIPs) across my machines. I tried alternative
> workflows and they didn't work out.
> 
> In both cases, the script introduces a delay of ~10 seconds to every git
> command I run in the outer repository and I have to remember to use the
> script on these repos.
> Moreover, I can't use the git-annex assistant, which watches the repo and
> automatically commits and syncs file changes.
> In my opinion, I want to be able to set the allow-dotgit config and it'll
just
> work without the delay and without having to remember the script.
> 
> The downsides we discussed don't apply in this usecase. These are mostly
> personal files, so I wont upload them to any hosting site (not even
private
> ones). There is no security impact as I only sync with trusted devices.

Have you considered git clone --mirror? This might solve your situation in a
different way, but should keep your repositories in sync with each other. It
also allows you to have different .git/config files on different platforms
(rather important when doing multiplatform development particularly with
submodules).

Randall

-- Brief whoami:
 NonStop developer since approximately 211288444200000000
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

On 2020-08-21 at 12:39:41, Lukas Straub wrote:
> The downsides we discussed don't apply in this usecase. These are mostly
> personal files, so I wont upload them to any hosting site (not even private
> ones). There is no security impact as I only sync with trusted devices.

I realize this works for you, but in general Git's security model does
not permit untrusted configuration files or hooks.  Configuration can
have numerous different commands that Git may execute and it is not, in
general, safe to share across users.  This is why Git does not provide a
way to sync whole repositories, only the objects within them.

Adding the ability to transport configuration through a repository is a
security problem because it allows an attacker to potentially execute
arbitrary code on the user's machine, and I can tell you that many, many
people do clone untrusted repositories.  Just because you are aware of
the risks, are comfortable with them, and are the only user in this
scenario does not mean that this feature is a prudent one to add to Git.
It violates our own security model, and as such, isn't a feature we're
going to want to add.

I want to be clear that it is not that we don't see your use case as
valuable or important, only that we can't see a way to implement it
securely as proposed.  Warning users unfortunately isn't sufficient
because users tend not to read documentation.

Multiple core contributors representing various aspects of the Git
community have weighed in, and it looks like the answer is unanimous.

Sorry for the bad news.
-- 
brian m. carlson: Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1886 bytes --]

On Fri, 21 Aug 2020 22:52:37 +0000
"brian m. carlson" <sandals@crustytoothpaste.net> wrote:

> On 2020-08-21 at 12:39:41, Lukas Straub wrote:
> > The downsides we discussed don't apply in this usecase. These are mostly
> > personal files, so I wont upload them to any hosting site (not even private
> > ones). There is no security impact as I only sync with trusted devices.  
> 
> I realize this works for you, but in general Git's security model does
> not permit untrusted configuration files or hooks.  Configuration can
> have numerous different commands that Git may execute and it is not, in
> general, safe to share across users.  This is why Git does not provide a
> way to sync whole repositories, only the objects within them.
> 
> Adding the ability to transport configuration through a repository is a
> security problem because it allows an attacker to potentially execute
> arbitrary code on the user's machine, and I can tell you that many, many
> people do clone untrusted repositories.  Just because you are aware of
> the risks, are comfortable with them, and are the only user in this
> scenario does not mean that this feature is a prudent one to add to Git.
> It violates our own security model, and as such, isn't a feature we're
> going to want to add.

I don't understand. If the attacker gets the user to set git config options,
then all hope is lost anyways, no?

Regards,
Lukas Straub

> I want to be clear that it is not that we don't see your use case as
> valuable or important, only that we can't see a way to implement it
> securely as proposed.  Warning users unfortunately isn't sufficient
> because users tend not to read documentation.
> 
> Multiple core contributors representing various aspects of the Git
> community have weighed in, and it looks like the answer is unanimous.
> 
> Sorry for the bad news.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1761 bytes --]

On 2020-08-22 at 14:21:52, Lukas Straub wrote:
> On Fri, 21 Aug 2020 22:52:37 +0000
> "brian m. carlson" <sandals@crustytoothpaste.net> wrote:
> 
> > On 2020-08-21 at 12:39:41, Lukas Straub wrote:
> > > The downsides we discussed don't apply in this usecase. These are mostly
> > > personal files, so I wont upload them to any hosting site (not even private
> > > ones). There is no security impact as I only sync with trusted devices.  
> > 
> > I realize this works for you, but in general Git's security model does
> > not permit untrusted configuration files or hooks.  Configuration can
> > have numerous different commands that Git may execute and it is not, in
> > general, safe to share across users.  This is why Git does not provide a
> > way to sync whole repositories, only the objects within them.
> > 
> > Adding the ability to transport configuration through a repository is a
> > security problem because it allows an attacker to potentially execute
> > arbitrary code on the user's machine, and I can tell you that many, many
> > people do clone untrusted repositories.  Just because you are aware of
> > the risks, are comfortable with them, and are the only user in this
> > scenario does not mean that this feature is a prudent one to add to Git.
> > It violates our own security model, and as such, isn't a feature we're
> > going to want to add.
> 
> I don't understand. If the attacker gets the user to set git config options,
> then all hope is lost anyways, no?

When you can embed repositories in other repositories like you're
proposing, those embedded repositories can have configuration files in
them (e.g., .git/config), which leads to the security problem.
-- 
brian m. carlson: Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2278 bytes --]

On Sat, 22 Aug 2020 18:53:07 +0000
"brian m. carlson" <sandals@crustytoothpaste.net> wrote:

> On 2020-08-22 at 14:21:52, Lukas Straub wrote:
> > On Fri, 21 Aug 2020 22:52:37 +0000
> > "brian m. carlson" <sandals@crustytoothpaste.net> wrote:
> >   
> > > On 2020-08-21 at 12:39:41, Lukas Straub wrote:  
> > > > The downsides we discussed don't apply in this usecase. These are mostly
> > > > personal files, so I wont upload them to any hosting site (not even private
> > > > ones). There is no security impact as I only sync with trusted devices.    
> > > 
> > > I realize this works for you, but in general Git's security model does
> > > not permit untrusted configuration files or hooks.  Configuration can
> > > have numerous different commands that Git may execute and it is not, in
> > > general, safe to share across users.  This is why Git does not provide a
> > > way to sync whole repositories, only the objects within them.
> > > 
> > > Adding the ability to transport configuration through a repository is a
> > > security problem because it allows an attacker to potentially execute
> > > arbitrary code on the user's machine, and I can tell you that many, many
> > > people do clone untrusted repositories.  Just because you are aware of
> > > the risks, are comfortable with them, and are the only user in this
> > > scenario does not mean that this feature is a prudent one to add to Git.
> > > It violates our own security model, and as such, isn't a feature we're
> > > going to want to add.  
> > 
> > I don't understand. If the attacker gets the user to set git config options,
> > then all hope is lost anyways, no?  
> 
> When you can embed repositories in other repositories like you're
> proposing, those embedded repositories can have configuration files in
> them (e.g., .git/config), which leads to the security problem.

Yes, I understand that, but the user has to actively allow this via the
allowDotGit config option, which I'll implement in the next patch version.
So the attacker has to get the user to set the option. If the user does this,
the attacker could get the user to set any other option (like core.gitProxy)
anyway and gain remote execution regardless of this patch.

Regards,
Lukas Straub

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 0/2] Allow adding .git files and directories

[Johannes Schindelin]

Hi Lukas,

On Sat, 22 Aug 2020, Lukas Straub wrote:

> On Sat, 22 Aug 2020 18:53:07 +0000
> "brian m. carlson" <sandals@crustytoothpaste.net> wrote:
>
> > On 2020-08-22 at 14:21:52, Lukas Straub wrote:
> > > On Fri, 21 Aug 2020 22:52:37 +0000
> > > "brian m. carlson" <sandals@crustytoothpaste.net> wrote:
> > >
> > > > On 2020-08-21 at 12:39:41, Lukas Straub wrote:
> > > > > The downsides we discussed don't apply in this usecase. These are mostly
> > > > > personal files, so I wont upload them to any hosting site (not even private
> > > > > ones). There is no security impact as I only sync with trusted devices.
> > > >
> > > > I realize this works for you, but in general Git's security model does
> > > > not permit untrusted configuration files or hooks.  Configuration can
> > > > have numerous different commands that Git may execute and it is not, in
> > > > general, safe to share across users.  This is why Git does not provide a
> > > > way to sync whole repositories, only the objects within them.
> > > >
> > > > Adding the ability to transport configuration through a repository is a
> > > > security problem because it allows an attacker to potentially execute
> > > > arbitrary code on the user's machine, and I can tell you that many, many
> > > > people do clone untrusted repositories.  Just because you are aware of
> > > > the risks, are comfortable with them, and are the only user in this
> > > > scenario does not mean that this feature is a prudent one to add to Git.
> > > > It violates our own security model, and as such, isn't a feature we're
> > > > going to want to add.
> > >
> > > I don't understand. If the attacker gets the user to set git config options,
> > > then all hope is lost anyways, no?
> >
> > When you can embed repositories in other repositories like you're
> > proposing, those embedded repositories can have configuration files in
> > them (e.g., .git/config), which leads to the security problem.
>
> Yes, I understand that, but the user has to actively allow this via the
> allowDotGit config option, which I'll implement in the next patch version.
> So the attacker has to get the user to set the option. If the user does this,
> the attacker could get the user to set any other option (like core.gitProxy)
> anyway and gain remote execution regardless of this patch.

Even if your patches were perfect, and even if unrelated patches in the
future would never weaken this via an unintended consequence, it is
_still_ too easy for users to get this wrong, or to forget about a config
option they set.

Having addressed my share of CVEs in Git, I am pretty firmly against
weakening Git's security model in the way you propose.

Ciao,
Johannes

P.S.: Besides, your patch would violate a the principle that unchanged
entities do not cause changes in the objects' hashes. And if you even so
much as `git repack` in one of those repositories you want to check in,
the hashes will change, even if there are no actual changes. It's much
like checking in gzipped files which then delta super badly. And in any
case, the proposed functionality is definitely in conflict with Git's
design.