{"thread":{"id":"44355","subject":"[PATCH] daemon: detect and reject too-long paths","startedAt":"2016-10-22T04:59:45Z","lastAt":"2016-10-22T16:37:16Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"304668","messageId":"20161022045938.h3xa3yapzlg427vy@sigill.intra.peff.net","threadId":"44355","inReplyTo":null,"subject":"[PATCH] daemon: detect and reject too-long paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-22T04:59:38Z","receivedAt":"2016-10-22T04:59:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are checking the path via path_ok(), we use some\nfixed PATH_MAX buffers. We write into them via snprintf(),\nso there's no possibility of overflow, but it does mean we\nmay silently truncate the path, leading to potentially\nconfusing errors when the partial path does not exist.\n\nWe're better off to reject the path explicitly.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAnother option would be to switch to strbufs here. That potentially\nintroduces cases where a client can convince us to just keep allocating\nmemory, but I don't think so in practice; the paths and interpolated\ndata items all have to come in 64K pkt-lines, which places a hard\nlimit. This is a much more minimal change, though, and I don't hear\nanybody complaining about the inability to use large paths.\n\n daemon.c | 25 +++++++++++++++++++++----\n 1 file changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 425aad0507..ff0fa583b0 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -160,6 +160,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n {\n \tstatic char rpath[PATH_MAX];\n \tstatic char interp_path[PATH_MAX];\n+\tsize_t rlen;\n \tconst char *path;\n \tconst char *dir;\n \n@@ -187,8 +188,12 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t\t\tnamlen = slash - dir;\n \t\t\trestlen -= namlen;\n \t\t\tloginfo(\"userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>\", user_path, dir, namlen, restlen, slash);\n-\t\t\tsnprintf(rpath, PATH_MAX, \"%.*s/%s%.*s\",\n-\t\t\t\t namlen, dir, user_path, restlen, slash);\n+\t\t\trlen = snprintf(rpath, sizeof(rpath), \"%.*s/%s%.*s\",\n+\t\t\t\t\tnamlen, dir, user_path, restlen, slash);\n+\t\t\tif (rlen >= sizeof(rpath)) {\n+\t\t\t\tlogerror(\"user-path too large: %s\", rpath);\n+\t\t\t\treturn NULL;\n+\t\t\t}\n \t\t\tdir = rpath;\n \t\t}\n \t}\n@@ -207,7 +212,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \n \t\tstrbuf_expand(&expanded_path, interpolated_path,\n \t\t\t      expand_path, &context);\n-\t\tstrlcpy(interp_path, expanded_path.buf, PATH_MAX);\n+\n+\t\trlen = strlcpy(interp_path, expanded_path.buf,\n+\t\t\t       sizeof(interp_path));\n+\t\tif (rlen >= sizeof(interp_path)) {\n+\t\t\tlogerror(\"interpolated path too large: %s\",\n+\t\t\t\t interp_path);\n+\t\t\treturn NULL;\n+\t\t}\n+\n \t\tstrbuf_release(&expanded_path);\n \t\tloginfo(\"Interpolated dir '%s'\", interp_path);\n \n@@ -219,7 +232,11 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t\t\tlogerror(\"'%s': Non-absolute path denied (base-path active)\", dir);\n \t\t\treturn NULL;\n \t\t}\n-\t\tsnprintf(rpath, PATH_MAX, \"%s%s\", base_path, dir);\n+\t\trlen = snprintf(rpath, sizeof(rpath), \"%s%s\", base_path, dir);\n+\t\tif (rlen >= sizeof(rpath)) {\n+\t\t\tlogerror(\"base-path too large: %s\", rpath);\n+\t\t\treturn NULL;\n+\t\t}\n \t\tdir = rpath;\n \t}\n \n-- \n2.10.1.776.ge0e381e\n"},{"id":"304671","messageId":"20161022052657.tbaoopai4ud5v7m7@sigill.intra.peff.net","threadId":"44355","inReplyTo":"20161022045938.h3xa3yapzlg427vy@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: detect and reject too-long paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-22T05:26:57Z","receivedAt":"2016-10-22T05:27:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 22, 2016 at 12:59:38AM -0400, Jeff King wrote:\n\n> When we are checking the path via path_ok(), we use some\n> fixed PATH_MAX buffers. We write into them via snprintf(),\n> so there's no possibility of overflow, but it does mean we\n> may silently truncate the path, leading to potentially\n> confusing errors when the partial path does not exist.\n> \n> We're better off to reject the path explicitly.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Another option would be to switch to strbufs here. That potentially\n> introduces cases where a client can convince us to just keep allocating\n> memory, but I don't think so in practice; the paths and interpolated\n> data items all have to come in 64K pkt-lines, which places a hard\n> limit. This is a much more minimal change, though, and I don't hear\n> anybody complaining about the inability to use large paths.\n\nFor reference, the switch to dynamic memory looks something like this.\nWe don't even need strbufs, and we can get rid of the static variables\nentirely (they weren't about buffer reuse, but just about extending the\nlifetime past the return value).\n\nThough we do have to add some free()s to avoid leaking error cases, this\nlooks simpler to me (the return value _is_ leaked in the success case,\nbecause the caller doesn't know if we returned the original value or a\nnewly allocated one. In practice it doesn't matter because we call this\nfunction once per process; compare to the 8K of BSS being wasted in the\noriginal).\n\ndiff --git a/daemon.c b/daemon.c\nindex 425aad0507..4575ce5 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -158,8 +158,7 @@ static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx)\n \n static const char *path_ok(const char *directory, struct hostinfo *hi)\n {\n-\tstatic char rpath[PATH_MAX];\n-\tstatic char interp_path[PATH_MAX];\n+\tchar *to_free = NULL;\n \tconst char *path;\n \tconst char *dir;\n \n@@ -187,9 +186,9 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t\t\tnamlen = slash - dir;\n \t\t\trestlen -= namlen;\n \t\t\tloginfo(\"userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>\", user_path, dir, namlen, restlen, slash);\n-\t\t\tsnprintf(rpath, PATH_MAX, \"%.*s/%s%.*s\",\n-\t\t\t\t namlen, dir, user_path, restlen, slash);\n-\t\t\tdir = rpath;\n+\t\t\tdir = to_free = xstrfmt(\"%.*s/%s%.*s\",\n+\t\t\t\t\t\tnamlen, dir, user_path,\n+\t\t\t\t\t\trestlen, slash);\n \t\t}\n \t}\n \telse if (interpolated_path && hi->saw_extended_args) {\n@@ -207,11 +206,8 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \n \t\tstrbuf_expand(&expanded_path, interpolated_path,\n \t\t\t      expand_path, &context);\n-\t\tstrlcpy(interp_path, expanded_path.buf, PATH_MAX);\n-\t\tstrbuf_release(&expanded_path);\n-\t\tloginfo(\"Interpolated dir '%s'\", interp_path);\n-\n-\t\tdir = interp_path;\n+\t\tdir = to_free = strbuf_detach(&expanded_path, NULL);\n+\t\tloginfo(\"Interpolated dir '%s'\", dir);\n \t}\n \telse if (base_path) {\n \t\tif (*dir != '/') {\n@@ -219,8 +215,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t\t\tlogerror(\"'%s': Non-absolute path denied (base-path active)\", dir);\n \t\t\treturn NULL;\n \t\t}\n-\t\tsnprintf(rpath, PATH_MAX, \"%s%s\", base_path, dir);\n-\t\tdir = rpath;\n+\t\tdir = to_free = xstrfmt(\"%s%s\", base_path, dir);\n \t}\n \n \tpath = enter_repo(dir, strict_paths);\n@@ -229,12 +224,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t\t * if we fail and base_path_relaxed is enabled, try without\n \t\t * prefixing the base path\n \t\t */\n+\t\tfree(to_free);\n+\t\tto_free = NULL;\n \t\tdir = directory;\n \t\tpath = enter_repo(dir, strict_paths);\n \t}\n \n \tif (!path) {\n \t\tlogerror(\"'%s' does not appear to be a git repository\", dir);\n+\t\tfree(to_free);\n \t\treturn NULL;\n \t}\n \n@@ -265,6 +263,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t}\n \n \tlogerror(\"'%s': not in whitelist\", path);\n+\tfree(to_free);\n \treturn NULL;\t\t/* Fallthrough. Deny by default */\n }\n \n"},{"id":"304680","messageId":"xmqqzilw8hn0.fsf@gitster.mtv.corp.google.com","threadId":"44355","inReplyTo":"20161022045938.h3xa3yapzlg427vy@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: detect and reject too-long paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-22T16:37:07Z","receivedAt":"2016-10-22T16:37:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> When we are checking the path via path_ok(), we use some\n> fixed PATH_MAX buffers. We write into them via snprintf(),\n> so there's no possibility of overflow, but it does mean we\n> may silently truncate the path, leading to potentially\n> confusing errors when the partial path does not exist.\n>\n> We're better off to reject the path explicitly.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n\nSounds sensible.\n\n> Another option would be to switch to strbufs here. That potentially\n> introduces cases where a client can convince us to just keep allocating\n> memory, but I don't think so in practice; the paths and interpolated\n> data items all have to come in 64K pkt-lines, which places a hard\n> limit. This is a much more minimal change, though, and I don't hear\n> anybody complaining about the inability to use large paths.\n\nThe alternative version did not look bad, either; in fact, the end\nresult may even be conceptually simpler.\n\nBut I agree that this one with the same hard-limit we always had is\na much more minimal change and is sufficient.\n\nThanks.\n\n>  daemon.c | 25 +++++++++++++++++++++----\n>  1 file changed, 21 insertions(+), 4 deletions(-)\n>\n> diff --git a/daemon.c b/daemon.c\n> index 425aad0507..ff0fa583b0 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -160,6 +160,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n>  {\n>  \tstatic char rpath[PATH_MAX];\n>  \tstatic char interp_path[PATH_MAX];\n> +\tsize_t rlen;\n>  \tconst char *path;\n>  \tconst char *dir;\n>  \n> @@ -187,8 +188,12 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n>  \t\t\tnamlen = slash - dir;\n>  \t\t\trestlen -= namlen;\n>  \t\t\tloginfo(\"userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>\", user_path, dir, namlen, restlen, slash);\n> -\t\t\tsnprintf(rpath, PATH_MAX, \"%.*s/%s%.*s\",\n> -\t\t\t\t namlen, dir, user_path, restlen, slash);\n> +\t\t\trlen = snprintf(rpath, sizeof(rpath), \"%.*s/%s%.*s\",\n> +\t\t\t\t\tnamlen, dir, user_path, restlen, slash);\n> +\t\t\tif (rlen >= sizeof(rpath)) {\n> +\t\t\t\tlogerror(\"user-path too large: %s\", rpath);\n> +\t\t\t\treturn NULL;\n> +\t\t\t}\n>  \t\t\tdir = rpath;\n>  \t\t}\n>  \t}\n> @@ -207,7 +212,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n>  \n>  \t\tstrbuf_expand(&expanded_path, interpolated_path,\n>  \t\t\t      expand_path, &context);\n> -\t\tstrlcpy(interp_path, expanded_path.buf, PATH_MAX);\n> +\n> +\t\trlen = strlcpy(interp_path, expanded_path.buf,\n> +\t\t\t       sizeof(interp_path));\n> +\t\tif (rlen >= sizeof(interp_path)) {\n> +\t\t\tlogerror(\"interpolated path too large: %s\",\n> +\t\t\t\t interp_path);\n> +\t\t\treturn NULL;\n> +\t\t}\n> +\n>  \t\tstrbuf_release(&expanded_path);\n>  \t\tloginfo(\"Interpolated dir '%s'\", interp_path);\n>  \n> @@ -219,7 +232,11 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n>  \t\t\tlogerror(\"'%s': Non-absolute path denied (base-path active)\", dir);\n>  \t\t\treturn NULL;\n>  \t\t}\n> -\t\tsnprintf(rpath, PATH_MAX, \"%s%s\", base_path, dir);\n> +\t\trlen = snprintf(rpath, sizeof(rpath), \"%s%s\", base_path, dir);\n> +\t\tif (rlen >= sizeof(rpath)) {\n> +\t\t\tlogerror(\"base-path too large: %s\", rpath);\n> +\t\t\treturn NULL;\n> +\t\t}\n>  \t\tdir = rpath;\n>  \t}\n"}]}