[PATCH] Sanitize escape char sequences coming from server

[PATCH] Sanitize escape char sequences coming from server

[Sebastian Kisela]

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

Re: [PATCH] Sanitize escape char sequences coming from server

[Jeff King]

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

Re: [PATCH] Sanitize escape char sequences coming from server

[Pavel Cahyna]

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

Re: [PATCH] Sanitize escape char sequences coming from server

[Jeff King]

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