* [PATCH] daemon: Skip unknown "extra arg" information @ 2009-06-04 22:08 Shawn O. Pearce 2009-06-05 0:50 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Shawn O. Pearce @ 2009-06-04 22:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: git If we don't recognize an extra arg supplied hidden behind the command, we should skip it and look at the next extra arg, in case we recognize the next one. For example, we currently don't recognize the "user=" extra arg, but we should still be able to start this connection anyway: perl -e ' $s="git-upload-pack /.git\0user=me\0host=localhost\0"; printf "%4.4x%s",4+length $s,$s ' | ./git-daemon --inetd --base-path=`pwd` --export-all Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- This should go in maint. daemon.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-) diff --git a/daemon.c b/daemon.c index daa4c8e..a9a4f02 100644 --- a/daemon.c +++ b/daemon.c @@ -411,14 +411,13 @@ static char *xstrdup_tolower(const char *str) static void parse_extra_args(char *extra_args, int buflen) { char *val; - int vallen; char *end = extra_args + buflen; while (extra_args < end && *extra_args) { + int arglen = strlen(extra_args); saw_extended_args = 1; if (strncasecmp("host=", extra_args, 5) == 0) { val = extra_args + 5; - vallen = strlen(val) + 1; if (*val) { /* Split <host>:<port> at colon. */ char *host = val; @@ -432,10 +431,10 @@ static void parse_extra_args(char *extra_args, int buflen) free(hostname); hostname = xstrdup_tolower(host); } - - /* On to the next one */ - extra_args = val + vallen; } + + /* On to the next one */ + extra_args += arglen + 1; } /* -- 1.6.3.1.333.g3ebba7 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] daemon: Skip unknown "extra arg" information 2009-06-04 22:08 [PATCH] daemon: Skip unknown "extra arg" information Shawn O. Pearce @ 2009-06-05 0:50 ` Junio C Hamano 2009-06-05 1:33 ` Shawn O. Pearce 0 siblings, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2009-06-05 0:50 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: git "Shawn O. Pearce" <spearce@spearce.org> writes: > If we don't recognize an extra arg supplied hidden behind the > command, we should skip it and look at the next extra arg, in > case we recognize the next one. > > For example, we currently don't recognize the "user=" extra arg, > but we should still be able to start this connection anyway: I do not necessarily agree 100% with that argument. When there is a room for "protocol negotiation", it is clear that somebody asking for what we said we do not support is bogus, and we should reject. Unfortunately, there is no room in the protocol for git-daemon to announce its capabilities, so the initial exchange can only be "unilaterally ask and hope for the best" from the client's point of view. However, in such a protocol exchange, there should be a way for a client to say "if you do not understand this request, it is _NOT_ OK to continue, pretending as if I never asked for it; instead please disconnect to avoid doing any further harm". We do something similar in the index extension. In that area, obviously there is no way for the writer who writes the index file to negotiate with the reader who will later read from the index to know what the reader can understand. Extension sections are marked with section names, whose case allows the reader to tell if it is Ok to simply ignore the section, or understanding of the section contents is required for the proper operation by the reader. Perhaps we should be doing something similar. > perl -e ' > $s="git-upload-pack /.git\0user=me\0host=localhost\0"; > printf "%4.4x%s",4+length $s,$s > ' | ./git-daemon --inetd --base-path=`pwd` --export-all > > Signed-off-by: Shawn O. Pearce <spearce@spearce.org> > --- > > This should go in maint. In its current form, I do not think so. Rejecting unknown is safer and maint is about safety, not about feature. Setting a precedent to accept anything unknown will make it harder to later say "the server must understand extension tokens that begin with uppercase". It always is easier to start stricter and then later loosen the rules in a controlled way. > daemon.c | 9 ++++----- > 1 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/daemon.c b/daemon.c > index daa4c8e..a9a4f02 100644 > --- a/daemon.c > +++ b/daemon.c > @@ -411,14 +411,13 @@ static char *xstrdup_tolower(const char *str) > static void parse_extra_args(char *extra_args, int buflen) > { > char *val; > - int vallen; > char *end = extra_args + buflen; > > while (extra_args < end && *extra_args) { > + int arglen = strlen(extra_args); > saw_extended_args = 1; > if (strncasecmp("host=", extra_args, 5) == 0) { > val = extra_args + 5; > - vallen = strlen(val) + 1; > if (*val) { > /* Split <host>:<port> at colon. */ > char *host = val; > @@ -432,10 +431,10 @@ static void parse_extra_args(char *extra_args, int buflen) > free(hostname); > hostname = xstrdup_tolower(host); > } > - > - /* On to the next one */ > - extra_args = val + vallen; > } > + > + /* On to the next one */ > + extra_args += arglen + 1; > } > > /* > -- > 1.6.3.1.333.g3ebba7 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] daemon: Skip unknown "extra arg" information 2009-06-05 0:50 ` Junio C Hamano @ 2009-06-05 1:33 ` Shawn O. Pearce 2009-06-05 8:41 ` Jakub Narebski ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Shawn O. Pearce @ 2009-06-05 1:33 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> wrote: > "Shawn O. Pearce" <spearce@spearce.org> writes: > > > If we don't recognize an extra arg supplied hidden behind the > > command, we should skip it and look at the next extra arg, in > > case we recognize the next one. > > > > For example, we currently don't recognize the "user=" extra arg, > > but we should still be able to start this connection anyway: > > I do not necessarily agree 100% with that argument. Actually, we're already f'kd. We can't change the protocol like we had hoped. I still think this should go in maint. --8<-- daemon: Strictly parse the "extra arg" part of the command Since 1.4.4.5 (49ba83fb67 "Add virtualization support to git-daemon") git daemon enters an infinite loop and never terminates if a client hides any extra arguments in the initial request line which is not exactly "\0host=blah\0". Since that change, a client must never insert additional extra arguments, or attempt to use any argument other than "host=", as any daemon will get stuck parsing the request line and will never complete the request. Since the client can't tell if the daemon is patched or not, it is not possible to know if additional extra args might actually be able to be safely requested. If we ever need to extend the git daemon protocol to support a new feature, we may have to do something like this to the exchange: # If both support git:// v2 # C: 000cgit://v2 S: 0010ok host user C: 0018host git.kernel.org C: 0027git-upload-pack /pub/linux-2.6.git S: ...git-upload-pack header... # If client supports git:// v2, server does not: # C: 000cgit://v2 S: <EOF> C: 003bgit-upload-pack /pub/linux-2.6.git\0host=git.kernel.org\0 S: ...git-upload-pack header... This requires the client to create two TCP connections to talk to an older git daemon, however all daemons since the introduction of daemon.c will safely reject the unknown "git://v2" command request, so the client can quite easily determine the server supports an older protocol. Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- connect.c | 5 ++++- daemon.c | 10 ++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/connect.c b/connect.c index f6b8ba6..958c831 100644 --- a/connect.c +++ b/connect.c @@ -579,7 +579,10 @@ struct child_process *git_connect(int fd[2], const char *url_orig, git_tcp_connect(fd, host, flags); /* * Separate original protocol components prog and path - * from extended components with a NUL byte. + * from extended host header with a NUL byte. + * + * Note: Do not add any other headers here! Doing so + * will cause older git-daemon servers to crash. */ packet_write(fd[1], "%s %s%chost=%s%c", diff --git a/daemon.c b/daemon.c index daa4c8e..b2babcc 100644 --- a/daemon.c +++ b/daemon.c @@ -406,15 +406,15 @@ static char *xstrdup_tolower(const char *str) } /* - * Separate the "extra args" information as supplied by the client connection. + * Read the host as supplied by the client connection. */ -static void parse_extra_args(char *extra_args, int buflen) +static void parse_host_arg(char *extra_args, int buflen) { char *val; int vallen; char *end = extra_args + buflen; - while (extra_args < end && *extra_args) { + if (extra_args < end && *extra_args) { saw_extended_args = 1; if (strncasecmp("host=", extra_args, 5) == 0) { val = extra_args + 5; @@ -436,6 +436,8 @@ static void parse_extra_args(char *extra_args, int buflen) /* On to the next one */ extra_args = val + vallen; } + if (extra_args < end && *extra_args) + die("Invalid request"); } /* @@ -545,7 +547,7 @@ static int execute(struct sockaddr *addr) hostname = canon_hostname = ip_address = tcp_port = NULL; if (len != pktlen) - parse_extra_args(line + len + 1, pktlen - len - 1); + parse_host_arg(line + len + 1, pktlen - len - 1); for (i = 0; i < ARRAY_SIZE(daemon_service); i++) { struct daemon_service *s = &(daemon_service[i]); -- 1.6.3.2.277.gd10543 -- Shawn. ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] daemon: Skip unknown "extra arg" information 2009-06-05 1:33 ` Shawn O. Pearce @ 2009-06-05 8:41 ` Jakub Narebski 2009-06-05 13:16 ` Sergey Vlasov 2009-06-07 18:56 ` Johannes Sixt 2 siblings, 0 replies; 7+ messages in thread From: Jakub Narebski @ 2009-06-05 8:41 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: Junio C Hamano, git "Shawn O. Pearce" <spearce@spearce.org> writes: > Since 1.4.4.5 (49ba83fb67 "Add virtualization support to git-daemon") > git daemon enters an infinite loop and never terminates if a client > hides any extra arguments in the initial request line which is not > exactly "\0host=blah\0". When running $ git daemon --export-all --verbose ... and connecting with dummy client / netcat which adds extra arguments, it doesn't look as it never terminates; it still accepts new connections. -- Jakub Narebski Poland ShadeHawk on #git ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] daemon: Skip unknown "extra arg" information 2009-06-05 1:33 ` Shawn O. Pearce 2009-06-05 8:41 ` Jakub Narebski @ 2009-06-05 13:16 ` Sergey Vlasov 2009-06-07 18:56 ` Johannes Sixt 2 siblings, 0 replies; 7+ messages in thread From: Sergey Vlasov @ 2009-06-05 13:16 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 1196 bytes --] On Thu, 4 Jun 2009 18:33:32 -0700 Shawn O. Pearce wrote: > Junio C Hamano <gitster@pobox.com> wrote: > > "Shawn O. Pearce" <spearce@spearce.org> writes: > > > > > If we don't recognize an extra arg supplied hidden behind the > > > command, we should skip it and look at the next extra arg, in > > > case we recognize the next one. > > > > > > For example, we currently don't recognize the "user=" extra arg, > > > but we should still be able to start this connection anyway: > > > > I do not necessarily agree 100% with that argument. > > Actually, we're already f'kd. We can't change the protocol like > we had hoped. There is always a place for another ugly workaround :) Add an extra \0 before additional parameters: "\0host=example.com\0\0param1=value1\0param2=value2\0" (the buggy loop will still terminate on double \0, and maybe the code we will add to parse the rest of data will work correctly). This will be enough for optional parameters (when the old server may silently ignore them without breaking the protocol). For mandatory parameters a change in the preceding "git-upload-pack" part will be necessary (like the "git://v2" suggestion). [-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] daemon: Skip unknown "extra arg" information 2009-06-05 1:33 ` Shawn O. Pearce 2009-06-05 8:41 ` Jakub Narebski 2009-06-05 13:16 ` Sergey Vlasov @ 2009-06-07 18:56 ` Johannes Sixt 2009-06-07 20:29 ` Junio C Hamano 2 siblings, 1 reply; 7+ messages in thread From: Johannes Sixt @ 2009-06-07 18:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: Shawn O. Pearce, git On Freitag, 5. Juni 2009, Shawn O. Pearce wrote: > Actually, we're already f'kd. We can't change the protocol like > we had hoped. I still think this should go in maint. > > --8<-- > daemon: Strictly parse the "extra arg" part of the command > > Since 1.4.4.5 (49ba83fb67 "Add virtualization support to git-daemon") > git daemon enters an infinite loop and never terminates if a client > hides any extra arguments in the initial request line which is not > exactly "\0host=blah\0". I see you applied this to maint. Since this patch actually fixes a DoS-exploitable bug, shouldn't it be applied all the way back to 1.4.4.5? -- Hannes ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] daemon: Skip unknown "extra arg" information 2009-06-07 18:56 ` Johannes Sixt @ 2009-06-07 20:29 ` Junio C Hamano 0 siblings, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2009-06-07 20:29 UTC (permalink / raw) To: Johannes Sixt; +Cc: Shawn O. Pearce, git Johannes Sixt <j6t@kdbg.org> writes: > On Freitag, 5. Juni 2009, Shawn O. Pearce wrote: >> Actually, we're already f'kd. We can't change the protocol like >> we had hoped. I still think this should go in maint. >> >> --8<-- >> daemon: Strictly parse the "extra arg" part of the command >> >> Since 1.4.4.5 (49ba83fb67 "Add virtualization support to git-daemon") >> git daemon enters an infinite loop and never terminates if a client >> hides any extra arguments in the initial request line which is not >> exactly "\0host=blah\0". > > I see you applied this to maint. Since this patch actually fixes a > DoS-exploitable bug, shouldn't it be applied all the way back to 1.4.4.5? I personally do not have the bandwidth to worry about anything older than say 1.6.0. Interested parties (read: distro packagers who pride themselves for their LTS) can do that themselves. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-06-07 20:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-06-04 22:08 [PATCH] daemon: Skip unknown "extra arg" information Shawn O. Pearce 2009-06-05 0:50 ` Junio C Hamano 2009-06-05 1:33 ` Shawn O. Pearce 2009-06-05 8:41 ` Jakub Narebski 2009-06-05 13:16 ` Sergey Vlasov 2009-06-07 18:56 ` Johannes Sixt 2009-06-07 20:29 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).