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

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

[Jeff King]

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@peff.net>
---
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.

 daemon.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index 425aad0507..ff0fa583b0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -160,6 +160,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 {
 	static char rpath[PATH_MAX];
 	static char interp_path[PATH_MAX];
+	size_t rlen;
 	const char *path;
 	const char *dir;
 
@@ -187,8 +188,12 @@ 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);
+			rlen = snprintf(rpath, sizeof(rpath), "%.*s/%s%.*s",
+					namlen, dir, user_path, restlen, slash);
+			if (rlen >= sizeof(rpath)) {
+				logerror("user-path too large: %s", rpath);
+				return NULL;
+			}
 			dir = rpath;
 		}
 	}
@@ -207,7 +212,15 @@ 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);
+
+		rlen = strlcpy(interp_path, expanded_path.buf,
+			       sizeof(interp_path));
+		if (rlen >= sizeof(interp_path)) {
+			logerror("interpolated path too large: %s",
+				 interp_path);
+			return NULL;
+		}
+
 		strbuf_release(&expanded_path);
 		loginfo("Interpolated dir '%s'", interp_path);
 
@@ -219,7 +232,11 @@ 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);
+		rlen = snprintf(rpath, sizeof(rpath), "%s%s", base_path, dir);
+		if (rlen >= sizeof(rpath)) {
+			logerror("base-path too large: %s", rpath);
+			return NULL;
+		}
 		dir = rpath;
 	}
 
-- 
2.10.1.776.ge0e381e

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

[Jeff King]

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@peff.net>
> ---
> 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 */
 }
 

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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@peff.net>
> ---

Sounds sensible.

> 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.

The alternative version did not look bad, either; in fact, the end
result may even be conceptually simpler.

But I agree that this one with the same hard-limit we always had is
a much more minimal change and is sufficient.

Thanks.

>  daemon.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index 425aad0507..ff0fa583b0 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -160,6 +160,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
>  {
>  	static char rpath[PATH_MAX];
>  	static char interp_path[PATH_MAX];
> +	size_t rlen;
>  	const char *path;
>  	const char *dir;
>  
> @@ -187,8 +188,12 @@ 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);
> +			rlen = snprintf(rpath, sizeof(rpath), "%.*s/%s%.*s",
> +					namlen, dir, user_path, restlen, slash);
> +			if (rlen >= sizeof(rpath)) {
> +				logerror("user-path too large: %s", rpath);
> +				return NULL;
> +			}
>  			dir = rpath;
>  		}
>  	}
> @@ -207,7 +212,15 @@ 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);
> +
> +		rlen = strlcpy(interp_path, expanded_path.buf,
> +			       sizeof(interp_path));
> +		if (rlen >= sizeof(interp_path)) {
> +			logerror("interpolated path too large: %s",
> +				 interp_path);
> +			return NULL;
> +		}
> +
>  		strbuf_release(&expanded_path);
>  		loginfo("Interpolated dir '%s'", interp_path);
>  
> @@ -219,7 +232,11 @@ 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);
> +		rlen = snprintf(rpath, sizeof(rpath), "%s%s", base_path, dir);
> +		if (rlen >= sizeof(rpath)) {
> +			logerror("base-path too large: %s", rpath);
> +			return NULL;
> +		}
>  		dir = rpath;
>  	}