[PATCH] git-daemon extra paranoia

[PATCH] git-daemon extra paranoia

[H. Peter Anvin]

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

This patch adds some extra paranoia to the git-daemon filename test.  In 
particular, it now rejects pathnames containing // or ending with /; it 
also adds a redundant test for pathname absoluteness (belts and suspenders.)

Signed-off-by: H. Peter Anvin <hpa@zytor.com>

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1185 bytes --]

Extra paranoia about non-canonical pathnames

---
commit a22f643931e48a319a70af7e91f809648160ecbf
tree 9d6934089c2628253d0690efde3fa7f36a1a8861
parent 4aaa702794447d9b281dd22fe532fd61e02434e1
author Peter Anvin <hpa@tazenda.sc.orionmulti.com> Tue, 18 Oct 2005 13:51:45 -0700
committer Peter Anvin <hpa@tazenda.sc.orionmulti.com> Tue, 18 Oct 2005 13:51:45 -0700

 daemon.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
--- a/daemon.c
+++ b/daemon.c
@@ -80,17 +80,25 @@ static int path_ok(const char *dir)
 {
 	const char *p = dir;
 	char **pp;
-	int sl = 1, ndot = 0;
+	int sl, ndot;
+
+	/* The pathname here should be an absolute path. */
+	if ( *p++ != '/' )
+		return 0;
+
+	sl = 1;  ndot = 0;
 
 	for (;;) {
 		if ( *p == '.' ) {
 			ndot++;
 		} else if ( *p == '/' || *p == '\0' ) {
-			if ( sl && ndot > 0 && ndot < 3 )
-				return 0; /* . or .. in path */
+			if ( sl && ndot < 3 )	/* Refuse "", "." or ".." */
+				return 0;
 			sl = 1;
+
+			/* If this was end of string, we passed all tests */
 			if ( *p == '\0' )
-				break; /* End of string and all is good */
+				break;
 		} else {
 			sl = ndot = 0;
 		}

Re: [PATCH] git-daemon extra paranoia

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> This patch adds some extra paranoia to the git-daemon filename test.  In 
> particular, it now rejects pathnames containing // or ending with /; it 
> also adds a redundant test for pathname absoluteness (belts and suspenders.)
>
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> Extra paranoia about non-canonical pathnames

I would understand rejecting /../, and perhaps /./, but why
reject // in between or / at the end?

Especially, I think this part in daemon.c::upload():

	if (!path_ok(dir)) {
		logerror("Forbidden directory: %s\n", dir);
		return -1;
	}

	if (chdir(dir) < 0) {
		logerror("Cannot chdir('%s'): %s", dir, strerror(errno));
		return -1;
	}

	chdir(".git");

relies on the fact that you can say "/home/junio/git/" for me to
publish "/home/junio/git/.git/" repository, so I would suspect
that it is necessary to allow "ending with /" at least.

Re: [PATCH] git-daemon extra paranoia

[H. Peter Anvin]

Junio C Hamano wrote:
> 
> I would understand rejecting /../, and perhaps /./, but why
> reject // in between or / at the end?
> 

For security, avoiding aliases is highly desirable, and if they're 
useless the easiest way to do that is to reject.  If aliases are 
required, which it sounds like it might be, then canonicalization needs 
to be applied.

This may sound redundant, but a lot of avoiding security holes involves 
applying good practices up front, instead of reactively.

Allowing a terminal slash should be reasonably easy, though.

	-hpa

Re: [PATCH] git-daemon extra paranoia

[H. Peter Anvin]

H. Peter Anvin wrote:
> 
> For security, avoiding aliases is highly desirable, and if they're 
> useless the easiest way to do that is to reject.  If aliases are 
> required, which it sounds like it might be, then canonicalization needs 
> to be applied.
> 
> This may sound redundant, but a lot of avoiding security holes involves 
> applying good practices up front, instead of reactively.
> 

I thought I might want to add a bit of an explanation, just for the 
purpose of illustration.

Right now, we use a whitelist for access control.  Aliases are not a 
problem, because they fail shut.

A year from now, someone decides that they want a "all but" feature, and 
thus adds a blacklist on top of the whitelist.  If aliases are 
permitted, unless the blacklist logic is written very carefully, one 
would then be able to get around the blacklist by using one of the 
aliased paths.

Improper handling of aliases is probably second only to buffer overflows 
and large-string DoS attacks when it comes to security vulnerabilities.

	-hpa

[PATCH] Revised - git-daemon extra paranoia

[H. Peter Anvin]

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

This patch adds some extra paranoia to the git-daemon filename test.  In 
particular, it now rejects pathnames containing //; it also adds a 
redundant test for pathname absoluteness (belts and suspenders.)

A single / at the end of the path is still permitted, however.

Signed-off-by: H. Peter Anvin <hpa@zytor.com>

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 833 bytes --]

diff --git a/daemon.c b/daemon.c
--- a/daemon.c
+++ b/daemon.c
@@ -80,17 +80,29 @@ static int path_ok(const char *dir)
 {
 	const char *p = dir;
 	char **pp;
-	int sl = 1, ndot = 0;
+	int sl, ndot;
+
+	/* The pathname here should be an absolute path. */
+	if ( *p++ != '/' )
+		return 0;
+
+	sl = 1;  ndot = 0;
 
 	for (;;) {
 		if ( *p == '.' ) {
 			ndot++;
-		} else if ( *p == '/' || *p == '\0' ) {
+		} else if ( *p == '\0' ) {
+			/* Reject "." and ".." at the end of the path */
 			if ( sl && ndot > 0 && ndot < 3 )
-				return 0; /* . or .. in path */
+				return 0;
+
+			/* Otherwise OK */
+			break;
+		} else if ( *p == '/' ) {
+			/* Refuse "", "." or ".." */
+			if ( sl && ndot < 3 )
+				return 0;
 			sl = 1;
-			if ( *p == '\0' )
-				break; /* End of string and all is good */
 		} else {
 			sl = ndot = 0;
 		}

Re: [PATCH] git-daemon extra paranoia

[Linus Torvalds]

On Tue, 18 Oct 2005, H. Peter Anvin wrote:
>
> This patch adds some extra paranoia to the git-daemon filename test.  In
> particular, it now rejects pathnames containing // or ending with /; it also
> adds a redundant test for pathname absoluteness (belts and suspenders.)

Hmm. The "not ending in /" is a bad test. 

Especially in light of the fact that the git-pack protocol quite by design 
tends to add a ".git" to the end as a fallback, so that a user that wants 
to specify a particular directory _without_ that fallback needs to have 
the slash at the end.

Now, git-daemon hasn't implemented that, but I think that was just a 
mistake that grew out of it not getting a lot of testing, since it wasn't 
used much. I personally use the "without the final .git" version quite 
often, because it just looks so much nicer for the user.

In fact, here's a patch that makes git-daemon allow it, and thus match the 
behaviour of the ssh transport.

The logic is simple: if the original "chdir()" fails, try another one with 
".git" appended. This is in _addition_ to doing the 'chdir(".git")' later, 
so that if you have a checked-out git repository in /home/linux-2.6.git, 
then doing a

	git pull git://host/home/linux-2.6

will on the remote end do:

	chmod("/home/linux-2.6")	// fails with ENOENT
	chmod("/home/linux-2.6.git")	// works
	chmod(".git")			// works

resulting in it ending up in /home/linux-2.6.git/.git, which is exactly 
correct, and where it wants to be.

I personally find it a nice bit of usability enhancement. You can name 
your git repositories with a ".git" suffix (which can help all kinds of 
automated tasks - like autopacking), but you don't force your users to 
care.

		Linus

---
diff --git a/daemon.c b/daemon.c
index 11fa3ed..a488512 100644
--- a/daemon.c
+++ b/daemon.c
@@ -128,8 +128,13 @@ static int upload(char *dir, int dirlen)
 	}
 
 	if (chdir(dir) < 0) {
-		logerror("Cannot chdir('%s'): %s", dir, strerror(errno));
-		return -1;
+		int err = errno;
+		strcpy(dir + dirlen, ".git");
+		if (err != ENOENT || chdir(dir) < 0) {
+			dir[dirlen] = 0;
+			logerror("Cannot chdir('%s'): %s", dir, strerror(err));
+			return -1;
+		}
 	}
 
 	chdir(".git");
@@ -164,7 +169,12 @@ static int execute(void)
 	static char line[1000];
 	int len;
 
-	len = packet_read_line(0, line, sizeof(line));
+	/*
+	 * Make sure that we leave room for an extra ".git" at
+	 * the end of the line. Note that the packet interfaces
+	 * already guarantee that there is an ending '\0'.
+	 */
+	len = packet_read_line(0, line, sizeof(line)-4);
 
 	if (len && line[len-1] == '\n')
 		line[--len] = 0;

Re: [PATCH] git-daemon extra paranoia

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> 	git pull git://host/home/linux-2.6
>
> will on the remote end do:
>
> 	chdir("/home/linux-2.6")	// fails with ENOENT
> 	chdir("/home/linux-2.6.git")	// works
> 	chdir(".git")			// works
>
> resulting in it ending up in /home/linux-2.6.git/.git, which is exactly 
> correct, and where it wants to be.
>
> I personally find it a nice bit of usability enhancement. You can name 
> your git repositories with a ".git" suffix (which can help all kinds of 
> automated tasks - like autopacking), but you don't force your users to 
> care.

Wouldn't having /home/linux-2.6/.git/ repository with
/home/linux-2.6/ working tree be good enough for that?  Instead
of doing "find / -type d -name '*.git'" you could do "find /
-type d -name .git" for automated tasks.

Re: [PATCH] git-daemon extra paranoia

[Linus Torvalds]

On Tue, 18 Oct 2005, Junio C Hamano wrote:
> 
> Wouldn't having /home/linux-2.6/.git/ repository with
> /home/linux-2.6/ working tree be good enough for that?  Instead
> of doing "find / -type d -name '*.git'" you could do "find /
> -type d -name .git" for automated tasks.

In this case, yes.

But In a mixed environment where you might have "bare" repositories, you 
want to have "reponame.git" as the repository name.

So with the rule that (a) try to first append ".git" and (b) then, after a 
successful chdir, try to go in one more level, you can handle both types, 
without ever having to care whether it's checked-out or not.

And for secondary projects (where git isn't necessarily the primary source 
control method), I actually use the "project.git" naming just to make it 
obvious that this is the "gitified" version of the project.

For example, I keep both my private uemacs and pine source trees as git 
repositories these days, and I have them under "~/src/uemacs.git/" and 
"~/src/pine.git/" even though they are checked out and thus actually have 
another ".git" inside of them.

		Linus

Re: [PATCH] git-daemon extra paranoia

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> Hmm. The "not ending in /" is a bad test. 
> 
> Especially in light of the fact that the git-pack protocol quite by design 
> tends to add a ".git" to the end as a fallback, so that a user that wants 
> to specify a particular directory _without_ that fallback needs to have 
> the slash at the end.
> 
> Now, git-daemon hasn't implemented that, but I think that was just a 
> mistake that grew out of it not getting a lot of testing, since it wasn't 
> used much. I personally use the "without the final .git" version quite 
> often, because it just looks so much nicer for the user.
> 
> In fact, here's a patch that makes git-daemon allow it, and thus match the 
> behaviour of the ssh transport.
> 
> The logic is simple: if the original "chdir()" fails, try another one with 
> ".git" appended. This is in _addition_ to doing the 'chdir(".git")' later, 
> so that if you have a checked-out git repository in /home/linux-2.6.git, 
> then doing a
> 

This is also exactly the kind of DWIM that tends to result in the kind 
of security holes I described earlier.

The DWIM aspect is fine, of course, but it has to be done up front: 
instead of doing just chdir(), each path should be validated through 
path_ok() before even being considered for chdir().  Perhaps the right 
thing to do is to combine the two functions.

	-hpa

Re: [PATCH] git-daemon extra paranoia

[Linus Torvalds]

On Tue, 18 Oct 2005, H. Peter Anvin wrote:
> 
> This is also exactly the kind of DWIM that tends to result in the kind of
> security holes I described earlier.

I don't agree. 

DWIM isn't automatically a security hole. DWIM _can_ be a security hole, 
but so can anything else that is badly designed or specified.

And just appending ".git" is _not_ badly designed/specified. I did think 
about the boundary cases, and it's entirely safe:

 - it can't result in "surprises": if the original pathname doesn't exist, 
   then even if there is a race and it got created in between the two 
   chdir's as a directory and the name had a slash at the end, adding 
   ".git" is actually safe even if it succeeds: it won't take us anywhere 
   surprising. At worst it will take us to the ".git" directory of a newly 
   added git archive, but that's what we wanted anyway, so..

 - you can't create ".." with it - even if the passed-in filename ended 
   with "xyz/.", you'll end up with a perfectly safe "xyz/..git", so any 
   safety checks that were done on the original pathname are still valid 
   when appending ".git" to it.

 - and exactly because we don't append slashes or anything like that, the 
   end result won't even have anything ambiguous like "//" in it.

So it really doesn't have any downsides that I can see.

> The DWIM aspect is fine, of course, but it has to be done up front: instead of
> doing just chdir(), each path should be validated through path_ok() before
> even being considered for chdir().  Perhaps the right thing to do is to
> combine the two functions.

Sure, you could do that, and just replace path_ok + chdir with a 
"safe_chdir()". I don't really see the point, unless you want to walk the 
path one component at a time, though (which is really quite expensive).

If you want to verify that it's still on the same filesystem and didn't 
traverse any dubious symlinks (the only reason to do the component walking 
afaik), it's actually much cheaper to just do the chdir() and then do a 
"getcwd()" to verify that the result matches. At least under Linux.

(That, btw, is likely the right way to do "valid directory checking" 
anyway: if you have a white-list of acceptable directories, just do a 
chdir() blindly without any checking, then do "getcwd()" and check the 
result of that against the whitelist - then you can even allow ".." etc, 
and never even care)

			Linus

Re: [PATCH] git-daemon extra paranoia

[H. Peter Anvin]

Linus Torvalds wrote:
> And just appending ".git" is _not_ badly designed/specified. I did think 
> about the boundary cases, and it's entirely safe:
> 
>  - it can't result in "surprises": if the original pathname doesn't exist, 
>    then even if there is a race and it got created in between the two 
>    chdir's as a directory and the name had a slash at the end, adding 
>    ".git" is actually safe even if it succeeds: it won't take us anywhere 
>    surprising. At worst it will take us to the ".git" directory of a newly 
>    added git archive, but that's what we wanted anyway, so..
> 
>  - you can't create ".." with it - even if the passed-in filename ended 
>    with "xyz/.", you'll end up with a perfectly safe "xyz/..git", so any 
>    safety checks that were done on the original pathname are still valid 
>    when appending ".git" to it.
> 
>  - and exactly because we don't append slashes or anything like that, the 
>    end result won't even have anything ambiguous like "//" in it.
> 
> So it really doesn't have any downsides that I can see.
> 

Consider the whitelist/blacklist scenario I described in the previous 
email.  You have:

whitelist:	/pub/scm
blacklist:	/pub/scm/foo/bar.git

If you can bypass the blacklist by using the pathname /pub/scm/foo/bar, 
that's bad.

> 
>>The DWIM aspect is fine, of course, but it has to be done up front: instead of
>>doing just chdir(), each path should be validated through path_ok() before
>>even being considered for chdir().  Perhaps the right thing to do is to
>>combine the two functions.
> 
> Sure, you could do that, and just replace path_ok + chdir with a 
> "safe_chdir()". I don't really see the point, unless you want to walk the 
> path one component at a time, though (which is really quite expensive).
> 

The only reason to do that is to make it less likely that a future 
programmer would screw it up.

	-hpa

Re: [PATCH] git-daemon extra paranoia

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> Consider the whitelist/blacklist scenario I described in the previous 
> email.  You have:
>
> whitelist:	/pub/scm
> blacklist:	/pub/scm/foo/bar.git
>
> If you can bypass the blacklist by using the pathname /pub/scm/foo/bar, 
> that's bad.

I like the simplicity of the check Linus suggested.  Given
/pub/scm/fora/../foo/bar/, you would end up chdir() to
/pub/scm/foo/bar.git and getcwd() would hit the blacklist
entry.  Which almost means that you do not even need path_ok()
;-).