Re: [PATCH] daemon: detect and reject too-long paths

[Jeff King <peff@xxxxxxxx>]

On Sat, Oct 22, 2016 at 12:59:38AM -0400, Jeff King wrote:

> When we are checking the path via path_ok(), we use some
> fixed PATH_MAX buffers. We write into them via snprintf(),
> so there's no possibility of overflow, but it does mean we
> may silently truncate the path, leading to potentially
> confusing errors when the partial path does not exist.
> 
> We're better off to reject the path explicitly.
> 
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> Another option would be to switch to strbufs here. That potentially
> introduces cases where a client can convince us to just keep allocating
> memory, but I don't think so in practice; the paths and interpolated
> data items all have to come in 64K pkt-lines, which places a hard
> limit. This is a much more minimal change, though, and I don't hear
> anybody complaining about the inability to use large paths.

For reference, the switch to dynamic memory looks something like this.
We don't even need strbufs, and we can get rid of the static variables
entirely (they weren't about buffer reuse, but just about extending the
lifetime past the return value).

Though we do have to add some free()s to avoid leaking error cases, this
looks simpler to me (the return value _is_ leaked in the success case,
because the caller doesn't know if we returned the original value or a
newly allocated one. In practice it doesn't matter because we call this
function once per process; compare to the 8K of BSS being wasted in the
original).

diff --git a/daemon.c b/daemon.c
index 425aad0507..4575ce5 100644
--- a/daemon.c
+++ b/daemon.c
@@ -158,8 +158,7 @@ static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx)
 
 static const char *path_ok(const char *directory, struct hostinfo *hi)
 {
-	static char rpath[PATH_MAX];
-	static char interp_path[PATH_MAX];
+	char *to_free = NULL;
 	const char *path;
 	const char *dir;
 
@@ -187,9 +186,9 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 			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;
+			dir = to_free = xstrfmt("%.*s/%s%.*s",
+						namlen, dir, user_path,
+						restlen, slash);
 		}
 	}
 	else if (interpolated_path && hi->saw_extended_args) {
@@ -207,11 +206,8 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 
 		strbuf_expand(&expanded_path, interpolated_path,
 			      expand_path, &context);
-		strlcpy(interp_path, expanded_path.buf, PATH_MAX);
-		strbuf_release(&expanded_path);
-		loginfo("Interpolated dir '%s'", interp_path);
-
-		dir = interp_path;
+		dir = to_free = strbuf_detach(&expanded_path, NULL);
+		loginfo("Interpolated dir '%s'", dir);
 	}
 	else if (base_path) {
 		if (*dir != '/') {
@@ -219,8 +215,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 			logerror("'%s': Non-absolute path denied (base-path active)", dir);
 			return NULL;
 		}
-		snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
-		dir = rpath;
+		dir = to_free = xstrfmt("%s%s", base_path, dir);
 	}
 
 	path = enter_repo(dir, strict_paths);
@@ -229,12 +224,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 		 * if we fail and base_path_relaxed is enabled, try without
 		 * prefixing the base path
 		 */
+		free(to_free);
+		to_free = NULL;
 		dir = directory;
 		path = enter_repo(dir, strict_paths);
 	}
 
 	if (!path) {
 		logerror("'%s' does not appear to be a git repository", dir);
+		free(to_free);
 		return NULL;
 	}
 
@@ -265,6 +263,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 	}
 
 	logerror("'%s': not in whitelist", path);
+	free(to_free);
 	return NULL;		/* Fallthrough. Deny by default */
 }