threads / patch / 3232
Subject: [PATCH 2/3] daemon: Set SO_REUSEADDR on listening sockets.
[PATCH 0/3] git-daemon hacking
I've just been playing around a bit with the git-daemon. My main objective was to implement the feature in the third patch, namely to allow users to publish their own repositories even though the daemon was mainly locked down to serving from a particular central tree. This is the same idea as allowing a web server to serve files in a user's ~/public_html directory.
But I also fixed a couple of bugs I found on the way.
-- [mdw]
[PATCH 1/3] daemon: Provide missing argument for logerror() call.
From: Mark Wooding <mdw@distorted.org.uk>
Could cause a crash if --base-path set. Unlikely to be a security the concern: message doesn't go to the client, so we can't leak anything (except by dumping core), and we've already forked, so it's not a denial of service.
Signed-off-by: Mark Wooding <mdw@distorted.org.uk> ---
daemon.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/daemon.c b/daemon.c index bb014fa..532bb0c 100644 --- a/daemon.c +++ b/daemon.c @@ -147,7 +147,7 @@ static char *path_ok(char *dir) static char rpath[PATH_MAX]; if (*dir != '/') { /* Forbid possible base-path evasion using ~paths. */ - logerror("'%s': Non-absolute path denied (base-path active)"); + logerror("'%s': Non-absolute path denied (base-path active)", dir); return NULL; } snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
From: Mark Wooding <mdw@distorted.org.uk>
Without this, you can silently lose the ability to receive IPv4 connections if you stop and restart the daemon.
Signed-off-by: Mark Wooding <mdw@distorted.org.uk> ---
daemon.c | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/daemon.c b/daemon.c index 532bb0c..6b88c0c 100644 --- a/daemon.c +++ b/daemon.c @@ -454,6 +454,7 @@ static int socksetup(int port, int **soc int socknum = 0, *socklist = NULL; int maxfd = -1; char pbuf[NI_MAXSERV]; + int yes = 1; struct addrinfo hints, *ai0, *ai; int gai; @@ -491,6 +492,12 @@ static int socksetup(int port, int **soc } #endif + if (setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, + &yes, sizeof(yes))) { + close(sockfd); + return 0; /* not fatal */ + } + if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) { close(sockfd); continue; /* not fatal */ @@ -523,6 +530,7 @@ static int socksetup(int port, int **soc { struct sockaddr_in sin; int sockfd; + int yes = 1; sockfd = socket(AF_INET, SOCK_STREAM, 0); if (sockfd < 0) @@ -533,6 +541,12 @@ static int socksetup(int port, int **soc sin.sin_addr.s_addr = htonl(INADDR_ANY); sin.sin_port = htons(port); + if (setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, + &yes, sizeof(yes))) { + close(sockfd); + return 0; + } + if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) { close(sockfd); return 0;
Junio C HamanoRe: [PATCH 2/3] daemon: Set SO_REUSEADDR on listening sockets.
Mark Wooding <mdw@distorted.org.uk> writes:
> From: Mark Wooding <mdw@distorted.org.uk> > > Without this, you can silently lose the ability to receive IPv4 > connections if you stop and restart the daemon. > > Signed-off-by: Mark Wooding <mdw@distorted.org.uk>
I've always wanted to ask HPA and Linus about this why we did not do SO_REUSEADDR. I've seen some non-git servers also not using it, giving me an excuse to take a coffee break during work ;-). Is it because they predate REUSEADDR, or is there a valid reason to avoid using REUSEADDR under certain conditions?
Junio C HamanoRe: [PATCH 2/3] daemon: Set SO_REUSEADDR on listening sockets.
Mark Wooding <mdw@distorted.org.uk> writes:
> From: Mark Wooding <mdw@distorted.org.uk> > > Without this, you can silently lose the ability to receive IPv4 > connections if you stop and restart the daemon. > > Signed-off-by: Mark Wooding <mdw@distorted.org.uk>
But with that, you expose yourself to the confusion TIME_WAIT was designed to protect you from, so how about making it optional like this?
Tested, of course ;-).
-- >8 -- From nobody Mon Sep 17 00:00:00 2001 From: Mark Wooding <mdw@distorted.org.uk> Date: Fri Feb 3 20:27:04 2006 +0000 Subject: [PATCH] daemon: Set SO_REUSEADDR on listening sockets.
Without this, you can silently lose the ability to receive IPv4 connections if you stop and restart the daemon.
[jc: tweaked code organization a bit and made this controllable from a command line option.]
Signed-off-by: Mark Wooding <mdw@distorted.org.uk> Signed-off-by: Junio C Hamano <junkio@cox.net>
---
daemon.c | 27 ++++++++++++++++++++++++++- 1 files changed, 26 insertions(+), 1 deletions(-)
bb1527c884bbb9bf6a5d06c1dd409ea6c2045a91
diff --git a/daemon.c b/daemon.c index 324bb04..dab8c2c 100644 --- a/daemon.c +++ b/daemon.c @@ -13,11 +13,12 @@ static int log_syslog; static int verbose; +static int reuseaddr; static const char daemon_usage[] = "git-daemon [--verbose] [--syslog] [--inetd | --port=n] [--export-all]\n" " [--timeout=n] [--init-timeout=n] [--strict-paths]\n" -" [--base-path=path] [directory...]"; +" [--base-path=path] [--reuseaddr] [directory...]"; /* List of acceptable pathname prefixes */ static char **ok_paths = NULL; @@ -451,6 +452,16 @@ static void child_handler(int signo) } } +static int set_reuse_addr(int sockfd) +{ + int on = 1; + + if (!reuseaddr) + return 0; + return setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, + &on, sizeof(on)); +} + #ifndef NO_IPV6 static int socksetup(int port, int **socklist_p) @@ -495,6 +506,11 @@ static int socksetup(int port, int **soc } #endif + if (set_reuse_addr(sockfd)) { + close(sockfd); + return 0; /* not fatal */ + } + if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) { close(sockfd); continue; /* not fatal */ @@ -537,6 +553,11 @@ static int socksetup(int port, int **soc sin.sin_addr.s_addr = htonl(INADDR_ANY); sin.sin_port = htons(port); + if (set_reuse_addr(sockfd)) { + close(sockfd); + return 0; + } + if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) { close(sockfd); return 0; @@ -663,6 +684,10 @@ int main(int argc, char **argv) base_path = arg+12; continue; } + if (!strcmp(arg, "--reuseaddr")) { + reuseaddr = 1; + continue; + } if (!strcmp(arg, "--")) { ok_paths = &argv[i+1]; break;
-- 1.1.6.gf7ef
Re: [PATCH 2/3] daemon: Set SO_REUSEADDR on listening sockets.
Junio C Hamano <junkio@cox.net> wrote:
> But with that, you expose yourself to the confusion TIME_WAIT > was designed to protect you from, so how about making it > optional like this?
No objections there.
-- [mdw]
[PATCH 3/3] daemon: Support a --user-path option.
From: Mark Wooding <mwooding@ponder.ncipher.com>
If we're invoked with --user-path=FOO option, then a URL of the form git://~USER/PATH/... resolves to the path HOME/FOO/PATH/..., where HOME is USER's home directory. This is done instead of any transformation due to --base-path, so you can use both at the same time. This lets users set up their own git repositories to be served by a central daemon, without them all having to be in the same place, and without the git-daemon being allowed to roam the entire filesystem freely, or exposing details of filesystem layout on URLs.
Signed-off-by: Mark Wooding <mdw@distorted.org.uk> ---
Documentation/git-daemon.txt | 11 +++++++++-- daemon.c | 36 +++++++++++++++++++++++++++++++++--- 2 files changed, 42 insertions(+), 5 deletions(-)
Documentation/git-daemon.txt, daemon.c
diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt index a20e053..2e48a10 100644 --- a/Documentation/git-daemon.txt +++ b/Documentation/git-daemon.txt @@ -10,7 +10,7 @@ SYNOPSIS [verse] 'git-daemon' [--verbose] [--syslog] [--inetd | --port=n] [--export-all] [--timeout=n] [--init-timeout=n] [--strict-paths] - [--base-path=path] [directory...] + [--base-path=path] [--user-path=path] [directory...] DESCRIPTION ----------- @@ -43,7 +43,7 @@ OPTIONS '--base-path=/srv/git' on example.com, then if you later try to pull 'git://example.com/hello.git', `git-daemon` will interpret the path as '/srv/git/hello.git'. Home directories (the '~login' notation) - access is disabled. + access is disabled unless '--user-path' is also given. --export-all:: Allow pulling from all directories that look like GIT repositories @@ -70,6 +70,13 @@ OPTIONS Log to syslog instead of stderr. Note that this option does not imply --verbose, thus by default only error conditions will be logged. +--user-path:: + Rewrite a request for "~user/something" to + "home/user-path/something". Useful in conjunction with + '--base-path', if you want to restrict the daemon from roaming + the entire filesystem without preventing users from publishing + their own repositories. + --verbose:: Log details about the incoming connections and requested files. diff --git a/daemon.c b/daemon.c index 6b88c0c..95b9c7e 100644 --- a/daemon.c +++ b/daemon.c @@ -6,6 +6,7 @@ #include <netdb.h> #include <netinet/in.h> #include <arpa/inet.h> +#include <pwd.h> #include <syslog.h> #include "pkt-line.h" #include "cache.h" @@ -17,7 +18,7 @@ static int verbose; static const char daemon_usage[] = "git-daemon [--verbose] [--syslog] [--inetd | --port=n] [--export-all]\n" " [--timeout=n] [--init-timeout=n] [--strict-paths]\n" -" [--base-path=path] [directory...]"; +" [--base-path=path] [--user-path=path] [directory...]"; /* List of acceptable pathname prefixes */ static char **ok_paths = NULL; @@ -28,6 +29,7 @@ static int export_all_trees = 0; /* Take all paths relative to this one if non-NULL */ static char *base_path = NULL; +static char *user_path = NULL; /* Timeout, and initial timeout */ static unsigned int timeout = 0; @@ -137,14 +139,34 @@ static int avoid_alias(char *p) static char *path_ok(char *dir) { char *path; + static char rpath[PATH_MAX]; if (avoid_alias(dir)) { logerror("'%s': aliased", dir); return NULL; } - if (base_path) { - static char rpath[PATH_MAX]; + if (user_path && *dir == '~') { + struct passwd *pw; + char *u, *p; + + u = dir + 1; + p = strchr(u, '/'); + if (!p) { + logerror("'%s': Missing / after user name", dir); + return NULL; + } + *p = 0; + pw = getpwnam(u); + *p++ = '/'; + if (!pw) { + logerror("'%s': User not found", u); + return NULL; + } + snprintf(rpath, PATH_MAX, "%s/%s/%s", + pw->pw_dir, user_path, p); + dir = rpath; + } else if (base_path) { if (*dir != '/') { /* Forbid possible base-path evasion using ~paths. */ logerror("'%s': Non-absolute path denied (base-path active)", dir); @@ -491,6 +513,10 @@ static int socksetup(int port, int **soc /* Note: error is not fatal */ } #endif + if (setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof(yes))) { + close(sockfd); + continue; /* not fatal */ + } if (setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof(yes))) { @@ -673,6 +699,10 @@ int main(int argc, char **argv) base_path = arg+12; continue; } + if (!strncmp(arg, "--user-path=", 12)) { + user_path = arg+12; + continue; + } if (!strcmp(arg, "--")) { ok_paths = &argv[i+1]; break;
Junio C HamanoRe: [PATCH 3/3] daemon: Support a --user-path option.
Mark Wooding <mdw@distorted.org.uk> writes:
> If we're invoked with --user-path=FOO option, then a URL of the form > git://~USER/PATH/... resolves to the path HOME/FOO/PATH/..., where HOME > is USER's home directory.
I am probably slow as usual but I do not see how this is useful.
Wouldn't loosening the "request must be absolute if you use --base-path" check in the area your first patch in the series touches to also allow paths that start with a '~' be enough? That way ~alice/foo would remain to be /home/alice/foo (with /home/alice being alice's $HOME) and ~becky/bar would be /home2/becky/bar (with /home2/becky being becky's $HOME).
I suppose you are doing something similar to ~/public_html, but I think that is an independent feature.
Junio C HamanoRe: [PATCH 3/3] daemon: Support a --user-path option.
Junio C Hamano <junkio@cox.net> writes:
> Wouldn't loosening the "request must be absolute if you use > --base-path" check in the area your first patch in the series > touches to also allow paths that start with a '~' be enough?
That is, something like this is what I mean.
Tested, of course ;-).
diff --git a/daemon.c b/daemon.c index 532bb0c..324bb04 100644 --- a/daemon.c +++ b/daemon.c @@ -145,13 +145,17 @@ static char *path_ok(char *dir) if (base_path) { static char rpath[PATH_MAX]; - if (*dir != '/') { - /* Forbid possible base-path evasion using ~paths. */ + if (!strict_paths && *dir == '~') + ; /* allow user relative paths */ + else if (*dir != '/') { + /* otherwise allow only absolute */ logerror("'%s': Non-absolute path denied (base-path active)", dir); return NULL; } - snprintf(rpath, PATH_MAX, "%s%s", base_path, dir); - dir = rpath; + else { + snprintf(rpath, PATH_MAX, "%s%s", base_path, dir); + dir = rpath; + } } path = enter_repo(dir, strict_paths);
Re: [PATCH 3/3] daemon: Support a --user-path option.
Junio C Hamano <junkio@cox.net> wrote:
> I am probably slow as usual but I do not see how this is useful.
I don't want the git-daemon roaming all over the file system. Partly, as a systems administrator, it makes me nervous about security (not for any particularly good reason, I admit), but mainly because I don't want to be exposing my local filesystem structure in my git://... namespace -- it just seems like a bad idea. This is what --base-path is all about.
I do still want users to be able to publish their repositories. But I also don't want git-daemon wandering all over their home directories -- restriction to sensible places is what --base-path is for, after all.
> Wouldn't loosening the "request must be absolute if you use > --base-path" check in the area your first patch in the series > touches to also allow paths that start with a '~' be enough? > That way ~alice/foo would remain to be /home/alice/foo (with > /home/alice being alice's $HOME) and ~becky/bar would be > /home2/becky/bar (with /home2/becky being becky's $HOME).
That would still expose the structure of everyone's home directories in git://~user URLs, which is rather unfortunate. It's better than nothing, though.
> I suppose you are doing something similar to ~/public_html, but > I think that is an independent feature.
This is what I'm after, yes. The above can be achieved straightforwardly with --user-path=. if that's what you actually wanted. (Indeed, --user-path= works too, but this is harder to explain.)
I think I'd probably either run with --user-path=public-git or --user-path=public_html/git -- I've not made my mind up.
-- [mdw]
Junio C HamanoRe: [PATCH 3/3] daemon: Support a --user-path option.
Mark Wooding <mdw@distorted.org.uk> writes:
> This is what I'm after, yes. The above can be achieved > straightforwardly with --user-path=. if that's what you actually wanted. > (Indeed, --user-path= works too, but this is harder to explain.) > > I think I'd probably either run with --user-path=public-git or > --user-path=public_html/git -- I've not made my mind up.
I made that conditional to --strict, but come to think of it, an independent option like --user-path makes more sense, whether that option does the public_html-like path munging or not.
I think for personal repositories public_html-like limiting may be simpler to manage than the current approach of using git-daemon-export-ok flag file (the latter is more flexible but most people probably do not need that flexibility).
In my simplistic view, --base-path serves something like /pub hierarchy of an ftp server or /var/www of an http server. It goes hand-in-hand with the whitelist and everything under it are exported without having to mark individual directories with git-daemon-export-ok (or having a name like public_html to mark it exportable). For ~user/ based paths, it is natural to wish to limit the parts of home directories but there currently is not a good way to do so. We could probably extend the whitelist to take path glob patterns and say "~*/public-git/" or something silly like that, but that still means the request must be in the form "git://host/~alice/public-git/frotz.git/" (which may not be such a bad thing); "git://host/~alice/frotz.git/" might look nicer. Your path munging idea is one way to do so. Another would be for alice to have $HOME/frotz.git/git-daemon-export-ok. Personally I do not think either would make too much of a difference from usability point of view.
So I am not dismissing what you are trying to achieve here. However, I am not happy about having <pwd.h> there and majorly duplicating what enter_repo() does in that part of the code.
Re: [PATCH 3/3] daemon: Support a --user-path option.
Junio C Hamano <junkio@cox.net> wrote:
> In my simplistic view, --base-path serves something like /pub > hierarchy of an ftp server or /var/www of an http server.
Yes, that's the way I think of it too. I'll probably point it at /var/www/git or something when I finally get my tuits.
> For ~user/ based paths, it is natural to wish to limit the parts of > home directories but there currently is not a good way to do so. We > could probably extend the whitelist to take path glob patterns and say > "~*/public-git/" or something silly like that, but that still means > the request must be in the form > "git://host/~alice/public-git/frotz.git/" (which may not be such a bad > thing); "git://host/~alice/frotz.git/" might look nicer.
Putting `public-git' in the URLs seems to exposing an unnecessary detail. Or it's saying something utterly obvious -- of course it's a public GIT -- otherwise you wouldn't be asking for it.
> Your path munging idea is one way to do so. Another would be for > alice to have $HOME/frotz.git/git-daemon-export-ok. Personally I do > not think either would make too much of a difference from usability > point of view.
The idea of filling my home directory with all my GIT repositories isn't one I'm particularly keen on. For one thing, I can just tell that I'm going to get confused between ~/src/foo/.git and ~/foo.git some day; and besides, I like `ls ~' to be nice and short.
> So I am not dismissing what you are trying to achieve here. > However, I am not happy about having <pwd.h> there and majorly > duplicating what enter_repo() does in that part of the code.
OK, then: how about putting the user_path logic into enter_repo? Like this, perhaps:
Documentation/git-daemon.txt | 11 +++++++++-- cache.h | 2 +- daemon.c | 16 +++++++++++----- path.c | 34 ++++++++++++++++++++++++++++------ receive-pack.c | 2 +- upload-pack.c | 2 +- 6 files changed, 51 insertions(+), 16 deletions(-)
Documentation/git-daemon.txt, cache.h, daemon.c, path.c, receive-pack.c, upload-pack.c
diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt index a20e053..2e48a10 100644 --- a/Documentation/git-daemon.txt +++ b/Documentation/git-daemon.txt @@ -10,7 +10,7 @@ SYNOPSIS [verse] 'git-daemon' [--verbose] [--syslog] [--inetd | --port=n] [--export-all] [--timeout=n] [--init-timeout=n] [--strict-paths] - [--base-path=path] [directory...] + [--base-path=path] [--user-path=path] [directory...] DESCRIPTION ----------- @@ -43,7 +43,7 @@ OPTIONS '--base-path=/srv/git' on example.com, then if you later try to pull 'git://example.com/hello.git', `git-daemon` will interpret the path as '/srv/git/hello.git'. Home directories (the '~login' notation) - access is disabled. + access is disabled unless '--user-path' is also given. --export-all:: Allow pulling from all directories that look like GIT repositories @@ -70,6 +70,13 @@ OPTIONS Log to syslog instead of stderr. Note that this option does not imply --verbose, thus by default only error conditions will be logged. +--user-path:: + Rewrite a request for "~user/something" to + "home/user-path/something". Useful in conjunction with + '--base-path', if you want to restrict the daemon from roaming + the entire filesystem without preventing users from publishing + their own repositories. + --verbose:: Log details about the incoming connections and requested files. diff --git a/cache.h b/cache.h index bdbe2d6..b6c9cda 100644 --- a/cache.h +++ b/cache.h @@ -191,7 +191,7 @@ int git_mkstemp(char *path, size_t n, co int adjust_shared_perm(const char *path); int safe_create_leading_directories(char *path); char *safe_strncpy(char *, const char *, size_t); -char *enter_repo(char *path, int strict); +char *enter_repo(char *path, const char *u_path, int strict); /* Read and unpack a sha1 file into memory, write memory to a sha1 file */ extern int unpack_sha1_header(z_stream *stream, void *map, unsigned long mapsize, void *buffer, unsigned long size); diff --git a/daemon.c b/daemon.c index bb014fa..22d133d 100644 --- a/daemon.c +++ b/daemon.c @@ -17,7 +17,7 @@ static int verbose; static const char daemon_usage[] = "git-daemon [--verbose] [--syslog] [--inetd | --port=n] [--export-all]\n" " [--timeout=n] [--init-timeout=n] [--strict-paths]\n" -" [--base-path=path] [directory...]"; +" [--base-path=path] [--user-path=path] [directory...]"; /* List of acceptable pathname prefixes */ static char **ok_paths = NULL; @@ -28,6 +28,7 @@ static int export_all_trees = 0; /* Take all paths relative to this one if non-NULL */ static char *base_path = NULL; +static char *user_path = NULL; /* Timeout, and initial timeout */ static unsigned int timeout = 0; @@ -137,14 +138,16 @@ static int avoid_alias(char *p) static char *path_ok(char *dir) { char *path; + static char rpath[PATH_MAX]; if (avoid_alias(dir)) { logerror("'%s': aliased", dir); return NULL; } - if (base_path) { - static char rpath[PATH_MAX]; + if (user_path && dir[0] == '~') + /* okay */; + else if (base_path) { if (*dir != '/') { /* Forbid possible base-path evasion using ~paths. */ logerror("'%s': Non-absolute path denied (base-path active)"); @@ -154,7 +157,7 @@ static char *path_ok(char *dir) dir = rpath; } - path = enter_repo(dir, strict_paths); + path = enter_repo(dir, user_path, strict_paths); if (!path) { logerror("'%s': unable to chdir or not a git archive", dir); @@ -490,7 +493,6 @@ static int socksetup(int port, int **soc /* Note: error is not fatal */ } #endif - if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) { close(sockfd); continue; /* not fatal */ @@ -659,6 +661,10 @@ int main(int argc, char **argv) base_path = arg+12; continue; } + if (!strncmp(arg, "--user-path=", 12)) { + user_path = arg+12; + continue; + } if (!strcmp(arg, "--")) { ok_paths = &argv[i+1]; break; diff --git a/path.c b/path.c index 334b2bd..596e37e 100644 --- a/path.c +++ b/path.c @@ -131,7 +131,17 @@ int validate_symref(const char *path) return -1; } -static char *user_path(char *buf, char *path, int sz) +static void kill_trailing_slashes(char *buf, int *lenp) +{ + int len = *lenp; + while ((1 < len) && (buf[len-1] == '/')) { + buf[len-1] = 0; + len--; + } + *lenp = len; +} + +static char *user_path(char *buf, char *path, const char *u_path, int sz) { struct passwd *pw; char *slash; @@ -157,9 +167,17 @@ static char *user_path(char *buf, char * return NULL; baselen = strlen(pw->pw_dir); memcpy(buf, pw->pw_dir, baselen); - while ((1 < baselen) && (buf[baselen-1] == '/')) { - buf[baselen-1] = 0; - baselen--; + kill_trailing_slashes(buf, &baselen); + if (u_path) { + while (*u_path == '/') + u_path++; + len = strlen(u_path); + if (sz <= baselen + len + 1) + return NULL; + buf[baselen++] = '/'; + memcpy(buf + baselen, u_path, len); + baselen += len; + kill_trailing_slashes(buf, &baselen); } if (slash && slash[1]) { len = strlen(slash); @@ -184,6 +202,10 @@ static char *user_path(char *buf, char * * "%s/.git", "%s.git", "%s" in this order. The first one that exists is * what we try. * + * If "u_path" is given, and the path we're resolving has the form ~/path + * or ~user/path, then we resolve to home/u_path/path (where home is the + * appropriate user's home directory). + * * Second, we try chdir() to that. Upon failure, we return NULL. * * Then, we try if the current directory is a valid git repository. @@ -194,7 +216,7 @@ static char *user_path(char *buf, char * * links. User relative paths are also returned as they are given, * except DWIM suffixing. */ -char *enter_repo(char *path, int strict) +char *enter_repo(char *path, const char *u_path, int strict) { static char used_path[PATH_MAX]; static char validated_path[PATH_MAX]; @@ -215,7 +237,7 @@ char *enter_repo(char *path, int strict) if (PATH_MAX <= len) return NULL; if (path[0] == '~') { - if (!user_path(used_path, path, PATH_MAX)) + if (!user_path(used_path, path, u_path, PATH_MAX)) return NULL; strcpy(validated_path, path); path = used_path; diff --git a/receive-pack.c b/receive-pack.c index eae31e3..d411ab2 100644 --- a/receive-pack.c +++ b/receive-pack.c @@ -317,7 +317,7 @@ int main(int argc, char **argv) if (!dir) usage(receive_pack_usage); - if(!enter_repo(dir, 0)) + if(!enter_repo(dir, NULL, 0)) die("'%s': unable to chdir or not a git archive", dir); write_head_info(); diff --git a/upload-pack.c b/upload-pack.c index d198055..3468de1 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -276,7 +276,7 @@ int main(int argc, char **argv) usage(upload_pack_usage); dir = argv[i]; - if (!enter_repo(dir, strict)) + if (!enter_repo(dir, NULL, strict)) die("'%s': unable to chdir or not a git archive", dir); upload_pack(); -- [mdw]
Junio C HamanoRe: [PATCH 3/3] daemon: Support a --user-path option.
Mark Wooding <mdw@distorted.org.uk> writes:
> OK, then: how about putting the user_path logic into enter_repo? Like > this, perhaps:
I'd like to *REALLY* leave enter_repo alone unless necessary. The last round to finalize enter_repo() was painful enough to think its security implications through. And this particular "public_html" insertion change can be done without touching it.
I think the attached patch would essentially do the same thing. Only lightly tested, but tested nevertheless ;-). This comes on top of what I have in 'pu'.
One thing I have not done yet but I consider bould be a good change is to perhaps change the whitelist logic in path_ok() after enter_repo() says what canonicalized path it decided to use, so that it allows any user-relative paths without checking the whitelist when user_path is in effect (and perhaps not empty, so giving --user-path="public_git" would allow everybody while giving --user-path alone or --user-path="" would not). Otherwise, as the code currently stands (regardless of any of the patches in the discussion in this thread) I think you need to list all users on the whitelist line. I think --user-path is about giving all users ability to export their stuff. What do you think?
-- >8 --
diff --git a/daemon.c b/daemon.c index dab8c2c..a1ccda3 100644 --- a/daemon.c +++ b/daemon.c @@ -18,7 +18,8 @@ static int reuseaddr; static const char daemon_usage[] = "git-daemon [--verbose] [--syslog] [--inetd | --port=n] [--export-all]\n" " [--timeout=n] [--init-timeout=n] [--strict-paths]\n" -" [--base-path=path] [--reuseaddr] [directory...]"; +" [--base-path=path] [--user-path | --user-path=path]\n" +" [--reuseaddr] [directory...]"; /* List of acceptable pathname prefixes */ static char **ok_paths = NULL; @@ -30,6 +31,12 @@ static int export_all_trees = 0; /* Take all paths relative to this one if non-NULL */ static char *base_path = NULL; +/* If defined, ~user notation is allowed and the string is inserted + * after ~user/. E.g. a request to git://host/~alice/frotz would + * go to /home/alice/pub_git/frotz with --user-path=pub_git. + */ +static char *user_path = NULL; + /* Timeout, and initial timeout */ static unsigned int timeout = 0; static unsigned int init_timeout = 0; @@ -137,6 +144,7 @@ static int avoid_alias(char *p) static char *path_ok(char *dir) { + static char rpath[PATH_MAX]; char *path; if (avoid_alias(dir)) { @@ -144,12 +152,31 @@ static char *path_ok(char *dir) return NULL; } - if (base_path) { - static char rpath[PATH_MAX]; - if (!strict_paths && *dir == '~') - ; /* allow user relative paths */ - else if (*dir != '/') { - /* otherwise allow only absolute */ + if (*dir == '~') { + if (!user_path) { + logerror("'%s': User-path not allowed", dir); + return NULL; + } + if (*user_path) { + /* Got either "~alice" or "~alice/foo"; + * rewrite them to "~alice/%s" or + * "~alice/%s/foo". + */ + int namlen, restlen = strlen(dir); + char *slash = strchr(dir, '/'); + if (!slash) + slash = dir + restlen; + namlen = slash - dir; + restlen -= namlen; + loginfo("userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash); + snprintf(rpath, PATH_MAX, "%.*s/%s%.*s", + namlen, dir, user_path, restlen, slash); + dir = rpath; + } + } + else if (base_path) { + if (*dir != '/') { + /* Allow only absolute */ logerror("'%s': Non-absolute path denied (base-path active)", dir); return NULL; } @@ -688,6 +715,14 @@ int main(int argc, char **argv) reuseaddr = 1; continue; } + if (!strcmp(arg, "--user-path")) { + user_path = ""; + continue; + } + if (!strncmp(arg, "--user-path=", 12)) { + user_path = arg + 12; + continue; + } if (!strcmp(arg, "--")) { ok_paths = &argv[i+1]; break;