Re: [PATCH] daemon: detect and reject too-long paths
From: Junio C Hamano <hidden>
Date: 2016-10-22 16:37:16
Jeff King [off-list ref] 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 <redacted> ---
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.
quoted hunk
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; }