* [PATCH] Sanitize escape char sequences coming from server @ 2018-06-21 12:10 Sebastian Kisela 2018-06-21 17:41 ` Jeff King 0 siblings, 1 reply; 4+ messages in thread From: Sebastian Kisela @ 2018-06-21 12:10 UTC (permalink / raw) To: git; +Cc: nico, larsxschneider, lfleischer, skisela From: Sebastian Kisela <skisela@redhat.com> Fix volnurability against MITM attacks on client side by replacing non printable and non white space characters by "?". Fixes: CVE-2018-1000021 Signed-off-by: Sebastian Kisela <skisela@redhat.com> --- sideband.c | 20 ++++++++++++++++++++ t/t5401-update-hooks.sh | 23 +++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/sideband.c b/sideband.c index 325bf0e97..8c9d74ace 100644 --- a/sideband.c +++ b/sideband.c @@ -1,3 +1,4 @@ +#include <wchar.h> #include "cache.h" #include "pkt-line.h" #include "sideband.h" @@ -18,6 +19,20 @@ #define ANSI_SUFFIX "\033[K" #define DUMB_SUFFIX " " +int sanitize_server_message(struct strbuf *outbuf) +{ + wchar_t *wcstring = xmalloc(sizeof(wchar_t) * outbuf->len); + int len = mbstowcs(wcstring, outbuf->buf, outbuf->len); + if (len == -1) + return 1; + for(int i = 0; i <= len; i++) + if(!isprint(wcstring[i]) && !isspace(wcstring[i]) ) + wcstring[i] = '?'; + if (wcstombs(outbuf->buf, wcstring, outbuf->len) == -1) + return 1; + return 0; +} + int recv_sideband(const char *me, int in_stream, int out) { const char *suffix; @@ -74,6 +89,9 @@ int recv_sideband(const char *me, int in_stream, int out) } else { strbuf_addch(&outbuf, *brk); } + + if (sanitize_server_message(&outbuf)) + retval = SIDEBAND_REMOTE_ERROR; xwrite(2, outbuf.buf, outbuf.len); strbuf_reset(&outbuf); @@ -97,6 +115,8 @@ int recv_sideband(const char *me, int in_stream, int out) if (outbuf.len) { strbuf_addch(&outbuf, '\n'); + if (sanitize_server_message(&outbuf)) + retval = SIDEBAND_REMOTE_ERROR; xwrite(2, outbuf.buf, outbuf.len); } strbuf_release(&outbuf); diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh index 7f278d8ce..cc1f6ca29 100755 --- a/t/t5401-update-hooks.sh +++ b/t/t5401-update-hooks.sh @@ -148,4 +148,27 @@ test_expect_success 'pre-receive hook that forgets to read its input' ' git push ./victim.git "+refs/heads/*:refs/heads/*" ' +cat <<EOF >expect +remote: foo?[0;31mbar?[0m +To ./victim.git + * [new branch] victim_branch -> victim_branch +EOF +cat >victim.git/hooks/pre-receive <<'EOF' +#!/bin/sh + printf "foo\033[0;31mbar\033[0m" + exit 0 +EOF +chmod u+x victim.git/hooks/pre-receive +test_expect_success 'pre-receive stderr contains ANSI colors' ' + rm -f victim.git/hooks/update victim.git/hooks/post-receive && + + git branch victim_branch master && + git push ./victim.git "+refs/heads/victim_branch:refs/heads/victim_branch"\ + >send.out 2>send.err && + + cat send.err > actual && + + test_cmp expect actual +' + test_done -- 2.14.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Sanitize escape char sequences coming from server 2018-06-21 12:10 [PATCH] Sanitize escape char sequences coming from server Sebastian Kisela @ 2018-06-21 17:41 ` Jeff King 2018-06-21 18:09 ` Pavel Cahyna 0 siblings, 1 reply; 4+ messages in thread From: Jeff King @ 2018-06-21 17:41 UTC (permalink / raw) To: Sebastian Kisela; +Cc: git, nico, larsxschneider, lfleischer On Thu, Jun 21, 2018 at 02:10:30PM +0200, Sebastian Kisela wrote: > From: Sebastian Kisela <skisela@redhat.com> > > Fix volnurability against MITM attacks on client side > by replacing non printable and non white space characters > by "?". > > Fixes: CVE-2018-1000021 I'm not sure if this is a productive direction to pursue or not. If you're worried about a malicious server (or MITM) sending you bad messages, you'd also need to worry about them sending you bad repository content, as we may print filenames to stderr or stdout (and occasionally file content, too, though typically only as part of a diff, which _usually_ goes through a pager). But it's unclear to me if this is worth worrying about or not. Ultimately it's a vulnerability in the terminal if untrusted output can do bad things. It's hard to make an evaluation of whether this plugs the vulnerability because we haven't really defined a threat model. What are we protecting against exactly? As for the patch itself: > diff --git a/sideband.c b/sideband.c > index 325bf0e97..8c9d74ace 100644 > --- a/sideband.c > +++ b/sideband.c > @@ -1,3 +1,4 @@ > +#include <wchar.h> > #include "cache.h" System includes should go into git-compat-util.h (because ordering is often important there). More importantly, though, does everybody have wchar.h? > @@ -18,6 +19,20 @@ > #define ANSI_SUFFIX "\033[K" > #define DUMB_SUFFIX " " > > +int sanitize_server_message(struct strbuf *outbuf) > +{ > + wchar_t *wcstring = xmalloc(sizeof(wchar_t) * outbuf->len); This is a potential integer overflow that can lead to a buffer overflow (e.g., imagine a system where both wchar_t and size_t are 32-bits; a 1GB string will wrap around and we'll allocate a much smaller buffer than we expected). > + int len = mbstowcs(wcstring, outbuf->buf, outbuf->len); I don't think mbstowcs() is always going to do the right thing there. We're looking at a string that was sent from the remote server. What encoding is it in? Using mbstowcs() is going to use whatever is in LC_CTYPE on the local machine. Also, the return type of mbstowcs is a size_t. > + for(int i = 0; i <= len; i++) > + if(!isprint(wcstring[i]) && !isspace(wcstring[i]) ) > + wcstring[i] = '?'; > + if (wcstombs(outbuf->buf, wcstring, outbuf->len) == -1) > + return 1; Funny indentation. I think the second line is supposed to _not_ be in the loop, so this is just funny indentation and not wrong code. Using isprint() here probably doesn't do what you expect, because Git uses its own locale-agnostic ctype replacements. I didn't check, but I suspect any non-ascii characters will be marked as non-printable, making the whole wchar thing pointless. Your replacement allows existing spaces, which is good; many servers send carriage-returns as part of progress output (and recv_sideband detects these and makes sure the line remains prefixed with "remote:"). > @@ -74,6 +89,9 @@ int recv_sideband(const char *me, int in_stream, int out) > } else { > strbuf_addch(&outbuf, *brk); > } > + > + if (sanitize_server_message(&outbuf)) > + retval = SIDEBAND_REMOTE_ERROR; "outbuf" may contain partially-received lines at various points, meaning multi-byte characters could be cut off. I _think_ it's OK to look at it here, as we'd always be breaking on a "\r" or "\n" at this point. > @@ -97,6 +115,8 @@ int recv_sideband(const char *me, int in_stream, int out) > > if (outbuf.len) { > strbuf_addch(&outbuf, '\n'); > + if (sanitize_server_message(&outbuf)) > + retval = SIDEBAND_REMOTE_ERROR; > xwrite(2, outbuf.buf, outbuf.len); > } Here I think we could get cut off. Since it's the end of input, showing the partial cutoff character is the best we can do. However, I think we'd cause mbstowcs() in your sanitize function to report failure, meaning we wouldn't show the remote message at all. > diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh > index 7f278d8ce..cc1f6ca29 100755 > --- a/t/t5401-update-hooks.sh > +++ b/t/t5401-update-hooks.sh > @@ -148,4 +148,27 @@ test_expect_success 'pre-receive hook that forgets to read its input' ' > git push ./victim.git "+refs/heads/*:refs/heads/*" > ' > > +cat <<EOF >expect > +remote: foo?[0;31mbar?[0m > +To ./victim.git > + * [new branch] victim_branch -> victim_branch > +EOF I know some tests in this script are already guilty of this, but please avoid adding commands outside of a test_expect block. And use "<<-\EOF" to allow proper indentation of the here-doc, and inhibit interpolation when you don't need it. > +cat >victim.git/hooks/pre-receive <<'EOF' > +#!/bin/sh > + printf "foo\033[0;31mbar\033[0m" > + exit 0 > +EOF > +chmod u+x victim.git/hooks/pre-receive This should use write_script, since the $SHELL_PATH may not be /bin/sh. See nearby tests. > +test_expect_success 'pre-receive stderr contains ANSI colors' ' > + rm -f victim.git/hooks/update victim.git/hooks/post-receive && > + > + git branch victim_branch master && Funny non-tab indentation (here and elsewhere). > + git push ./victim.git "+refs/heads/victim_branch:refs/heads/victim_branch"\ > + >send.out 2>send.err && Style: include a space before the backslash continuation. However, this should be the same as "git push victim.git +victim_branch", which eliminates the need for continuation. > + cat send.err > actual && Style: we avoid whitespace between redirection operations and filenames (so just ">actual"). -Peff ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Sanitize escape char sequences coming from server 2018-06-21 17:41 ` Jeff King @ 2018-06-21 18:09 ` Pavel Cahyna 2018-06-21 18:51 ` Jeff King 0 siblings, 1 reply; 4+ messages in thread From: Pavel Cahyna @ 2018-06-21 18:09 UTC (permalink / raw) To: Jeff King; +Cc: Sebastian Kisela, git, nico, larsxschneider, lfleischer Hello, On Thu, Jun 21, 2018 at 01:41:22PM -0400, Jeff King wrote: > On Thu, Jun 21, 2018 at 02:10:30PM +0200, Sebastian Kisela wrote: > > > From: Sebastian Kisela <skisela@redhat.com> > > + int len = mbstowcs(wcstring, outbuf->buf, outbuf->len); > > I don't think mbstowcs() is always going to do the right thing there. > We're looking at a string that was sent from the remote server. What > encoding is it in? Using mbstowcs() is going to use whatever is in > LC_CTYPE on the local machine. Exactly. The point is, everything should continue to work if the local machine and the server agreed on the encoding. Imagine a non-English-speaking site where the administrators configured the Git server to output non-ASCII messages and the clients are configured with a matching locale which allows the users to see them. We should ensure everything keeps working in this case. > > + for(int i = 0; i <= len; i++) > > + if(!isprint(wcstring[i]) && !isspace(wcstring[i]) ) > > + wcstring[i] = '?'; > > + if (wcstombs(outbuf->buf, wcstring, outbuf->len) == -1) > > + return 1; > > Funny indentation. I think the second line is supposed to _not_ be in > the loop, so this is just funny indentation and not wrong code. > > Using isprint() here probably doesn't do what you expect, because Git > uses its own locale-agnostic ctype replacements. I didn't check, but I > suspect any non-ascii characters will be marked as non-printable, making > the whole wchar thing pointless. isw*() was probably intended instead of is*() > Your replacement allows existing spaces, which is good; many servers > send carriage-returns as part of progress output (and recv_sideband > detects these and makes sure the line remains prefixed with "remote:"). > > > @@ -74,6 +89,9 @@ int recv_sideband(const char *me, int in_stream, int out) > > } else { > > strbuf_addch(&outbuf, *brk); > > } > > + > > + if (sanitize_server_message(&outbuf)) > > + retval = SIDEBAND_REMOTE_ERROR; > > "outbuf" may contain partially-received lines at various points, meaning > multi-byte characters could be cut off. I _think_ it's OK to look at it > here, as we'd always be breaking on a "\r" or "\n" at this point. Maybe sanitize_server_message should return a mbstate_t to keep state between invocations? Thanks, Pavel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Sanitize escape char sequences coming from server 2018-06-21 18:09 ` Pavel Cahyna @ 2018-06-21 18:51 ` Jeff King 0 siblings, 0 replies; 4+ messages in thread From: Jeff King @ 2018-06-21 18:51 UTC (permalink / raw) To: Sebastian Kisela, git, nico, larsxschneider, lfleischer On Thu, Jun 21, 2018 at 08:09:43PM +0200, Pavel Cahyna wrote: > > > + int len = mbstowcs(wcstring, outbuf->buf, outbuf->len); > > > > I don't think mbstowcs() is always going to do the right thing there. > > We're looking at a string that was sent from the remote server. What > > encoding is it in? Using mbstowcs() is going to use whatever is in > > LC_CTYPE on the local machine. > > Exactly. The point is, everything should continue to work if the local > machine and the server agreed on the encoding. Imagine a > non-English-speaking site where the administrators configured the Git > server to output non-ASCII messages and the clients are configured with > a matching locale which allows the users to see them. We should ensure > everything keeps working in this case. What if they don't agree on the encoding? Right now you might get some mojibake. After this patch, we'd return an error. I thought at first we'd stop showing the message, which would be a regression. But looking at the caller, it does not actually break on seeing the error. Which means that the whole sanitizing process can be skipped simply by the attacker including a bogus multibyte sequence. > > Using isprint() here probably doesn't do what you expect, because Git > > uses its own locale-agnostic ctype replacements. I didn't check, but I > > suspect any non-ascii characters will be marked as non-printable, making > > the whole wchar thing pointless. > > isw*() was probably intended instead of is*() Yes, we don't override the isw* functions, so that would work (I still think that assuming the server messages are in our local charset is somewhat questionable). > > > + > > > + if (sanitize_server_message(&outbuf)) > > > + retval = SIDEBAND_REMOTE_ERROR; > > > > "outbuf" may contain partially-received lines at various points, meaning > > multi-byte characters could be cut off. I _think_ it's OK to look at it > > here, as we'd always be breaking on a "\r" or "\n" at this point. > > Maybe sanitize_server_message should return a mbstate_t to keep state > between invocations? I think this site is OK because of the CR/LF breaking. For the "final" one where it's not OK, there's no point in keeping state since we know we hit EOF already. -Peff ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-06-21 18:51 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-06-21 12:10 [PATCH] Sanitize escape char sequences coming from server Sebastian Kisela 2018-06-21 17:41 ` Jeff King 2018-06-21 18:09 ` Pavel Cahyna 2018-06-21 18:51 ` Jeff King
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 NNTP newsgroup(s).