Skip to contentSkip to conversation

git-daemon: path validation, export all option

PATCH H. Peter Anvin 4 participants 7 messages Jun 15, 2016

lore
Share & tools
lore
#0 lore raw

A first attempt to make git-daemon a bit more suitable for kernel.org use: it allows the user to specify a whitelist of directories, rejects paths which have . or .. in them (to avoid bypassing the whitelist), and allows for an --export-all option.

Support a modicum of path validation, and allow an export all trees option.

---
commit 4ae95682694a1cd05ee2029fe241ad90d43c8c0e
tree 4188c26501c852ba9c1b1a3f39276d3ac7dc3f8a
parent 152da3dfcf2c16d7c240a0dbdcb8a3ae1d332d81
author H. Peter Anvin <[email protected]> Mon, 26 Sep 2005 19:10:55 -0700 committer H. Peter Anvin <[email protected]> Mon, 26 Sep 2005 19:10:55 -0700

 daemon.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 67 insertions(+), 5 deletions(-)
daemon.c +67 -5
@@ -12,7 +12,13 @@
 static int log_syslog;
 static int verbose;
 
-static const char daemon_usage[] = "git-daemon [--verbose] [--syslog] [--inetd | --port=n]";
+static const char daemon_usage[] = "git-daemon [--verbose] [--syslog] [--inetd | --port=n] [--export-all] [directory...]";
+
+/* List of acceptable pathname prefixes */
+static char **ok_paths = NULL;
+
+/* If this is set, git-daemon-export-ok is not required */
+static int export_all_trees = 0;
 
 
 static void logreport(int priority, const char *err, va_list params)
@@ -69,15 +75,61 @@ void loginfo(const char *err, ...)
 	va_end(params);
 }
 
+static int path_ok(const char *dir)
+{
+	const char *p = dir;
+	char **pp;
+	int sl = 1, ndot = 0;
+
+	for (;;) {
+		if ( *p == '.' ) {
+			ndot++;
+		} else if ( *p == '/' || *p == '\0' ) {
+			if ( sl && ndot > 0 && ndot < 3 )
+				return 0; /* . or .. in path */
+			sl = 1;
+			if ( *p == '\0' )
+				break; /* End of string and all is good */
+		} else {
+			sl = ndot = 0;
+		}
+		p++;
+	}
+
+	if ( ok_paths && *ok_paths ) {
+		int ok = 0;
+		int dirlen = strlen(dir); /* read_packet_line can return embedded \0 */
+
+		for ( pp = ok_paths ; *pp ; pp++ ) {
+			int len = strlen(*pp);
+			if ( len <= dirlen &&
+			     !strncmp(*pp, dir, len) &&
+			     (dir[len] == '/' || dir[len] == '\0') ) {
+				ok = 1;
+				break;
+			}
+		}
+
+		if ( !ok )
+			return 0; /* Path not in whitelist */
+	}
+
+	return 1;		/* Path acceptable */
+}
 
 static int upload(char *dir, int dirlen)
 {
 	loginfo("Request for '%s'", dir);
+
+	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");
 
 	/*
 	 * Security on the cheap.
@@ -86,10 +138,10 @@ static int upload(char *dir, int dirlen)
 	 * a "git-daemon-export-ok" flag that says that the other side
 	 * is ok with us doing this.
 	 */
-	if (access("git-daemon-export-ok", F_OK) ||
+	if ((!export_all_trees && access("git-daemon-export-ok", F_OK)) ||
 	    access("objects/00", X_OK) ||
 	    access("HEAD", R_OK)) {
-		logerror("Not a valid gitd-enabled repository: '%s'", dir);
+		logerror("Not a valid git-daemon-enabled repository: '%s'", dir);
 		return -1;
 	}
 
@@ -441,7 +493,6 @@ int main(int argc, char **argv)
 				continue;
 			}
 		}
-
 		if (!strcmp(arg, "--inetd")) {
 			inetd_mode = 1;
 			continue;
@@ -455,6 +506,17 @@ int main(int argc, char **argv)
 			openlog("git-daemon", 0, LOG_DAEMON);
 			continue;
 		}
+		if (!strcmp(arg, "--export-all")) {
+			export_all_trees = 1;
+			continue;
+		}
+		if (!strcmp(arg, "--")) {
+			ok_paths = &argv[i+1];
+			break;
+		} else if (arg[0] != '-') {
+			ok_paths = &argv[i];
+			break;
+		}
 
 		usage(daemon_usage);
 	}
#1 lore raw
Quoted 6 lines · H. Peter Anvin

A first attempt to make git-daemon a bit more suitable for kernel.org
use: it allows the user to specify a whitelist of directories, rejects paths which have . or .. in them (to avoid bypassing the whitelist), and allows for an --export-all option.

I understand the motivation behind --export-all and directory whitelist and these changes look good. Thanks.

daemon.c void loginfo(const char *err, ...) · line 118

Quoted 7 lines
daemon.c +7 -0
+	if ( ok_paths && *ok_paths ) {
+		int ok = 0;
+...
+	}
+
+	return 1;		/* Path acceptable */
+}

A microNit. You could lose 'int ok' and return 1 directly where you assign 1 to it and break.

daemon.c void loginfo(const char *err, ...) · line 80

Quoted 1 line
daemon.c +0 -1
-	chdir(".git");

I am unsure about this removal of "minor convenience feature". Although I do not think git-daemon is widely used on the field, this change breaks existing setup if there is any.

#2 lore raw
Quoted 23 lines · Junio C Hamano
Quoted 6 lines · H. Peter Anvin

A first attempt to make git-daemon a bit more suitable for kernel.org
use: it allows the user to specify a whitelist of directories, rejects paths which have . or .. in them (to avoid bypassing the whitelist), and allows for an --export-all option.

I understand the motivation behind --export-all and directory whitelist and these changes look good. Thanks.

daemon.c void loginfo(const char *err, ...) · line 118

Quoted 7 lines
daemon.c +7 -0
+	if ( ok_paths && *ok_paths ) {
+		int ok = 0;
+...
+	}
+
+	return 1;		/* Path acceptable */
+}

A microNit. You could lose 'int ok' and return 1 directly where you assign 1 to it and break.

Quoted 5 lines

daemon.c void loginfo(const char *err, ...) · line 80

Quoted 1 line
daemon.c +0 -1
-	chdir(".git");

I am unsure about this removal of "minor convenience feature". Although I do not think git-daemon is widely used on the field, this change breaks existing setup if there is any.

Please drop this one line change. It certainly breaks my personal setup. And all git tools are happy with being given the "master" directory or the "master/.git" so there is no reason for git-daemon not to accept that, too.

If hpa really can't live with the chdir, maybe we could add a "--strict-git-paths" option or something that will not do the chdir? It would be only a few lines of code in git-daemon to parse the option and then the chdir would become

if (!strict_git_paths)

	chdir(".git");

Best regards,

Anton

Signature · 4 lines
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/
#3 lore raw
In reply to Junio C Hamano
Quoted 4 lines · Junio C Hamano

A microNit. You could lose 'int ok' and return 1 directly where you assign 1 to it and break.

I guess I personally prefer the coding style where the straigh-line flow of control is the normal one. It prevents the "oops" of someone wanting to add code to it later.

Quoted 6 lines

daemon.c void loginfo(const char *err, ...) · line 80

Quoted 2 lines
daemon.c +0 -1
-	chdir(".git");

I am unsure about this removal of "minor convenience feature". Although I do not think git-daemon is widely used on the field, this change breaks existing setup if there is any.

I have restored this and make the requested RPM changes. I have left a pullable tree at:

master.kernel.org:/home/hpa/git/daemon.git

... in order to preserve the commit structure.

-hpa

#4 lore raw
Quoted 6 lines · H. Peter Anvin

I have restored this and make the requested RPM changes. I have left a pullable tree at:

master.kernel.org:/home/hpa/git/daemon.git

... in order to preserve the commit structure.

Thanks. Will pull tonight.

#5 lore raw
In reply to H. Peter Anvin
Quoted 5 lines · H. Peter Anvin

A first attempt to make git-daemon a bit more suitable for kernel.org
use: it allows the user to specify a whitelist of directories, rejects paths which have . or .. in them (to avoid bypassing the whitelist), and allows for an --export-all option.

Removing the "chdir(".git")" thing is very wrong, though. Why do it?

It's very much on purpose: you can export even "regular" git trees (ie trees you have checked out) without the other side having to say

git clone machine.com:/home/torvalds/v2.6/linux/.git

where the final "/.git" is just stupid.

Linus

#6 lore raw
Quoted 9 lines · Linus Torvalds

Removing the "chdir(".git")" thing is very wrong, though. Why do it?

It's very much on purpose: you can export even "regular" git trees (ie trees you have checked out) without the other side having to say

git clone machine.com:/home/torvalds/v2.6/linux/.git

where the final "/.git" is just stupid.

Agreed. I wasn't thinking too hard about it, and it doesn't do any harm since failure is ignored.

-hpa