{"thread":{"id":"19666","subject":"[PATCH] daemon: Skip unknown \"extra arg\" information","startedAt":"2009-06-04T22:08:24Z","lastAt":"2009-06-07T20:29:16Z","messageCount":7,"participants":["Shawn O. Pearce","Junio C Hamano","Jakub Narebski","Sergey Vlasov","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"115456","messageId":"20090604220824.GT3355@spearce.org","threadId":"19666","inReplyTo":null,"subject":"[PATCH] daemon: Skip unknown \"extra arg\" information","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-06-04T22:08:24Z","receivedAt":"2009-06-04T22:08:24Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If we don't recognize an extra arg supplied hidden behind the\ncommand, we should skip it and look at the next extra arg, in\ncase we recognize the next one.\n\nFor example, we currently don't recognize the \"user=\" extra arg,\nbut we should still be able to start this connection anyway:\n\n perl -e '\n $s=\"git-upload-pack /.git\\0user=me\\0host=localhost\\0\";\n printf \"%4.4x%s\",4+length $s,$s\n ' | ./git-daemon --inetd --base-path=`pwd` --export-all\n\nSigned-off-by: Shawn O. Pearce \n---\n\n This should go in maint.\n\n daemon.c | 9 ++++-----\n 1 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex daa4c8e..a9a4f02 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -411,14 +411,13 @@ static char *xstrdup_tolower(const char *str)\n static void parse_extra_args(char *extra_args, int buflen)\n {\n \tchar *val;\n-\tint vallen;\n \tchar *end = extra_args + buflen;\n \n \twhile (extra_args < end && *extra_args) {\n+\t\tint arglen = strlen(extra_args);\n \t\tsaw_extended_args = 1;\n \t\tif (strncasecmp(\"host=\", extra_args, 5) == 0) {\n \t\t\tval = extra_args + 5;\n-\t\t\tvallen = strlen(val) + 1;\n \t\t\tif (*val) {\n \t\t\t\t/* Split : at colon. */\n \t\t\t\tchar *host = val;\n@@ -432,10 +431,10 @@ static void parse_extra_args(char *extra_args, int buflen)\n \t\t\t\tfree(hostname);\n \t\t\t\thostname = xstrdup_tolower(host);\n \t\t\t}\n-\n-\t\t\t/* On to the next one */\n-\t\t\textra_args = val + vallen;\n \t\t}\n+\n+\t\t/* On to the next one */\n+\t\textra_args += arglen + 1;\n \t}\n \n \t/*\n-- \n1.6.3.1.333.g3ebba7\n"},{"id":"115463","messageId":"7vbpp3tsg0.fsf@alter.siamese.dyndns.org","threadId":"19666","inReplyTo":"20090604220824.GT3355@spearce.org","subject":"Re: [PATCH] daemon: Skip unknown \"extra arg\" information","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-05T00:50:07Z","receivedAt":"2009-06-05T00:50:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> If we don't recognize an extra arg supplied hidden behind the\n> command, we should skip it and look at the next extra arg, in\n> case we recognize the next one.\n>\n> For example, we currently don't recognize the \"user=\" extra arg,\n> but we should still be able to start this connection anyway:\n\nI do not necessarily agree 100% with that argument.\n\nWhen there is a room for \"protocol negotiation\", it is clear that somebody\nasking for what we said we do not support is bogus, and we should reject.\n\nUnfortunately, there is no room in the protocol for git-daemon to announce\nits capabilities, so the initial exchange can only be \"unilaterally ask\nand hope for the best\" from the client's point of view.\n\nHowever, in such a protocol exchange, there should be a way for a client\nto say \"if you do not understand this request, it is _NOT_ OK to continue,\npretending as if I never asked for it; instead please disconnect to avoid\ndoing any further harm\".\n\nWe do something similar in the index extension. In that area, obviously\nthere is no way for the writer who writes the index file to negotiate with\nthe reader who will later read from the index to know what the reader can\nunderstand. Extension sections are marked with section names, whose case\nallows the reader to tell if it is Ok to simply ignore the section, or\nunderstanding of the section contents is required for the proper operation\nby the reader. Perhaps we should be doing something similar.\n\n> perl -e '\n> $s=\"git-upload-pack /.git\\0user=me\\0host=localhost\\0\";\n> printf \"%4.4x%s\",4+length $s,$s\n> ' | ./git-daemon --inetd --base-path=`pwd` --export-all\n>\n> Signed-off-by: Shawn O. Pearce \n> ---\n>\n> This should go in maint.\n\nIn its current form, I do not think so. Rejecting unknown is safer and\nmaint is about safety, not about feature. Setting a precedent to accept\nanything unknown will make it harder to later say \"the server must\nunderstand extension tokens that begin with uppercase\". It always is\neasier to start stricter and then later loosen the rules in a controlled\nway.\n\n> daemon.c | 9 ++++-----\n> 1 files changed, 4 insertions(+), 5 deletions(-)\n>\n> diff --git a/daemon.c b/daemon.c\n> index daa4c8e..a9a4f02 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -411,14 +411,13 @@ static char *xstrdup_tolower(const char *str)\n> static void parse_extra_args(char *extra_args, int buflen)\n> {\n> \tchar *val;\n> -\tint vallen;\n> \tchar *end = extra_args + buflen;\n> \n> \twhile (extra_args < end && *extra_args) {\n> +\t\tint arglen = strlen(extra_args);\n> \t\tsaw_extended_args = 1;\n> \t\tif (strncasecmp(\"host=\", extra_args, 5) == 0) {\n> \t\t\tval = extra_args + 5;\n> -\t\t\tvallen = strlen(val) + 1;\n> \t\t\tif (*val) {\n> \t\t\t\t/* Split : at colon. */\n> \t\t\t\tchar *host = val;\n> @@ -432,10 +431,10 @@ static void parse_extra_args(char *extra_args, int buflen)\n> \t\t\t\tfree(hostname);\n> \t\t\t\thostname = xstrdup_tolower(host);\n> \t\t\t}\n> -\n> -\t\t\t/* On to the next one */\n> -\t\t\textra_args = val + vallen;\n> \t\t}\n> +\n> +\t\t/* On to the next one */\n> +\t\textra_args += arglen + 1;\n> \t}\n> \n> \t/*\n> -- \n> 1.6.3.1.333.g3ebba7\n"},{"id":"115465","messageId":"20090605013332.GV3355@spearce.org","threadId":"19666","inReplyTo":"7vbpp3tsg0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] daemon: Skip unknown \"extra arg\" information","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-06-05T01:33:32Z","receivedAt":"2009-06-05T01:33:32Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > If we don't recognize an extra arg supplied hidden behind the\n> > command, we should skip it and look at the next extra arg, in\n> > case we recognize the next one.\n> >\n> > For example, we currently don't recognize the \"user=\" extra arg,\n> > but we should still be able to start this connection anyway:\n> \n> I do not necessarily agree 100% with that argument.\n\nActually, we're already f'kd. We can't change the protocol like\nwe had hoped. I still think this should go in maint.\n\n--8<--\ndaemon: Strictly parse the \"extra arg\" part of the command\n\nSince 1.4.4.5 (49ba83fb67 \"Add virtualization support to git-daemon\")\ngit daemon enters an infinite loop and never terminates if a client\nhides any extra arguments in the initial request line which is not\nexactly \"\\0host=blah\\0\".\n\nSince that change, a client must never insert additional extra\narguments, or attempt to use any argument other than \"host=\", as\nany daemon will get stuck parsing the request line and will never\ncomplete the request.\n\nSince the client can't tell if the daemon is patched or not, it\nis not possible to know if additional extra args might actually be\nable to be safely requested.\n\nIf we ever need to extend the git daemon protocol to support a new\nfeature, we may have to do something like this to the exchange:\n\n # If both support git:// v2\n #\n C: 000cgit://v2\n S: 0010ok host user\n C: 0018host git.kernel.org\n C: 0027git-upload-pack /pub/linux-2.6.git\n S: ...git-upload-pack header...\n\n # If client supports git:// v2, server does not:\n #\n C: 000cgit://v2\n S: \n\n C: 003bgit-upload-pack /pub/linux-2.6.git\\0host=git.kernel.org\\0\n S: ...git-upload-pack header...\n\nThis requires the client to create two TCP connections to talk to\nan older git daemon, however all daemons since the introduction of\ndaemon.c will safely reject the unknown \"git://v2\" command request,\nso the client can quite easily determine the server supports an\nolder protocol.\n\nSigned-off-by: Shawn O. Pearce \n---\n connect.c | 5 ++++-\n daemon.c | 10 ++++++----\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex f6b8ba6..958c831 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -579,7 +579,10 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\t\tgit_tcp_connect(fd, host, flags);\n \t\t/*\n \t\t * Separate original protocol components prog and path\n-\t\t * from extended components with a NUL byte.\n+\t\t * from extended host header with a NUL byte.\n+\t\t *\n+\t\t * Note: Do not add any other headers here! Doing so\n+\t\t * will cause older git-daemon servers to crash.\n \t\t */\n \t\tpacket_write(fd[1],\n \t\t\t \"%s %s%chost=%s%c\",\ndiff --git a/daemon.c b/daemon.c\nindex daa4c8e..b2babcc 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -406,15 +406,15 @@ static char *xstrdup_tolower(const char *str)\n }\n \n /*\n- * Separate the \"extra args\" information as supplied by the client connection.\n+ * Read the host as supplied by the client connection.\n */\n-static void parse_extra_args(char *extra_args, int buflen)\n+static void parse_host_arg(char *extra_args, int buflen)\n {\n \tchar *val;\n \tint vallen;\n \tchar *end = extra_args + buflen;\n \n-\twhile (extra_args < end && *extra_args) {\n+\tif (extra_args < end && *extra_args) {\n \t\tsaw_extended_args = 1;\n \t\tif (strncasecmp(\"host=\", extra_args, 5) == 0) {\n \t\t\tval = extra_args + 5;\n@@ -436,6 +436,8 @@ static void parse_extra_args(char *extra_args, int buflen)\n \t\t\t/* On to the next one */\n \t\t\textra_args = val + vallen;\n \t\t}\n+\t\tif (extra_args < end && *extra_args)\n+\t\t\tdie(\"Invalid request\");\n \t}\n \n \t/*\n@@ -545,7 +547,7 @@ static int execute(struct sockaddr *addr)\n \thostname = canon_hostname = ip_address = tcp_port = NULL;\n \n \tif (len != pktlen)\n-\t\tparse_extra_args(line + len + 1, pktlen - len - 1);\n+\t\tparse_host_arg(line + len + 1, pktlen - len - 1);\n \n \tfor (i = 0; i < ARRAY_SIZE(daemon_service); i++) {\n \t\tstruct daemon_service *s = &(daemon_service[i]);\n-- \n1.6.3.2.277.gd10543\n\n-- \nShawn.\n"},{"id":"115494","messageId":"m3iqjb6pk5.fsf@localhost.localdomain","threadId":"19666","inReplyTo":"20090605013332.GV3355@spearce.org","subject":"Re: [PATCH] daemon: Skip unknown \"extra arg\" information","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-05T08:41:09Z","receivedAt":"2009-06-05T08:41:09Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Since 1.4.4.5 (49ba83fb67 \"Add virtualization support to git-daemon\")\n> git daemon enters an infinite loop and never terminates if a client\n> hides any extra arguments in the initial request line which is not\n> exactly \"\\0host=blah\\0\".\n\nWhen running\n\n $ git daemon --export-all --verbose ...\n\nand connecting with dummy client / netcat which adds extra arguments,\nit doesn't look as it never terminates; it still accepts new\nconnections.\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"115505","messageId":"20090605171627.d92f6060.vsu@altlinux.ru","threadId":"19666","inReplyTo":"20090605013332.GV3355@spearce.org","subject":"Re: [PATCH] daemon: Skip unknown \"extra arg\" information","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2009-06-05T13:16:27Z","receivedAt":"2009-06-05T13:16:27Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"On Thu, 4 Jun 2009 18:33:32 -0700 Shawn O. Pearce wrote:\n\n> Junio C Hamano wrote:\n> > \"Shawn O. Pearce\" writes:\n> > \n> > > If we don't recognize an extra arg supplied hidden behind the\n> > > command, we should skip it and look at the next extra arg, in\n> > > case we recognize the next one.\n> > >\n> > > For example, we currently don't recognize the \"user=\" extra arg,\n> > > but we should still be able to start this connection anyway:\n> > \n> > I do not necessarily agree 100% with that argument.\n> \n> Actually, we're already f'kd. We can't change the protocol like\n> we had hoped.\n\nThere is always a place for another ugly workaround :)\n\nAdd an extra \\0 before additional parameters:\n\n \"\\0host=example.com\\0\\0param1=value1\\0param2=value2\\0\"\n\n(the buggy loop will still terminate on double \\0, and maybe the code\nwe will add to parse the rest of data will work correctly).\n\nThis will be enough for optional parameters (when the old server may\nsilently ignore them without breaking the protocol). For mandatory\nparameters a change in the preceding \"git-upload-pack\" part will be\nnecessary (like the \"git://v2\" suggestion).\n"},{"id":"115724","messageId":"200906072056.08680.j6t@kdbg.org","threadId":"19666","inReplyTo":"20090605013332.GV3355@spearce.org","subject":"Re: [PATCH] daemon: Skip unknown \"extra arg\" information","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-06-07T18:56:08Z","receivedAt":"2009-06-07T18:56:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 5. Juni 2009, Shawn O. Pearce wrote:\n> Actually, we're already f'kd. We can't change the protocol like\n> we had hoped. I still think this should go in maint.\n>\n> --8<--\n> daemon: Strictly parse the \"extra arg\" part of the command\n>\n> Since 1.4.4.5 (49ba83fb67 \"Add virtualization support to git-daemon\")\n> git daemon enters an infinite loop and never terminates if a client\n> hides any extra arguments in the initial request line which is not\n> exactly \"\\0host=blah\\0\".\n\nI see you applied this to maint. Since this patch actually fixes a \nDoS-exploitable bug, shouldn't it be applied all the way back to 1.4.4.5?\n\n-- Hannes\n"},{"id":"115731","messageId":"7vfxebaiub.fsf@alter.siamese.dyndns.org","threadId":"19666","inReplyTo":"200906072056.08680.j6t@kdbg.org","subject":"Re: [PATCH] daemon: Skip unknown \"extra arg\" information","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-07T20:29:16Z","receivedAt":"2009-06-07T20:29:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> On Freitag, 5. Juni 2009, Shawn O. Pearce wrote:\n>> Actually, we're already f'kd. We can't change the protocol like\n>> we had hoped. I still think this should go in maint.\n>>\n>> --8<--\n>> daemon: Strictly parse the \"extra arg\" part of the command\n>>\n>> Since 1.4.4.5 (49ba83fb67 \"Add virtualization support to git-daemon\")\n>> git daemon enters an infinite loop and never terminates if a client\n>> hides any extra arguments in the initial request line which is not\n>> exactly \"\\0host=blah\\0\".\n>\n> I see you applied this to maint. Since this patch actually fixes a \n> DoS-exploitable bug, shouldn't it be applied all the way back to 1.4.4.5?\n\nI personally do not have the bandwidth to worry about anything older than\nsay 1.6.0. Interested parties (read: distro packagers who pride\nthemselves for their LTS) can do that themselves.\n"}]}