{"thread":{"id":"48751","subject":"[PATCH] Sanitize escape char sequences coming from server","startedAt":"2018-06-21T12:10:38Z","lastAt":"2018-06-21T18:51:16Z","messageCount":4,"participants":["Sebastian Kisela","Jeff King","Pavel Cahyna"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"350629","messageId":"20180621121030.1721-1-sebastian.kisela@gmail.com","threadId":"48751","inReplyTo":null,"subject":"[PATCH] Sanitize escape char sequences coming from server","fromName":"Sebastian Kisela","fromEmail":"skisela@redhat.com","sentAt":"2018-06-21T12:10:30Z","receivedAt":"2018-06-21T12:10:38Z","isPatch":true,"sender":{"key":"skisela@redhat.com","avatar":"https://avatars.githubusercontent.com/u/26842433?v=4"},"body":"From: Sebastian Kisela <skisela@redhat.com>\n\nFix volnurability against MITM attacks on client side\nby replacing non printable and non white space characters\nby \"?\".\n\nFixes: CVE-2018-1000021\nSigned-off-by: Sebastian Kisela <skisela@redhat.com>\n---\n sideband.c              | 20 ++++++++++++++++++++\n t/t5401-update-hooks.sh | 23 +++++++++++++++++++++++\n 2 files changed, 43 insertions(+)\n\ndiff --git a/sideband.c b/sideband.c\nindex 325bf0e97..8c9d74ace 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -1,3 +1,4 @@\n+#include <wchar.h>\n #include \"cache.h\"\n #include \"pkt-line.h\"\n #include \"sideband.h\"\n@@ -18,6 +19,20 @@\n #define ANSI_SUFFIX \"\\033[K\"\n #define DUMB_SUFFIX \"        \"\n \n+int sanitize_server_message(struct strbuf *outbuf)\n+{\n+\twchar_t *wcstring = xmalloc(sizeof(wchar_t) * outbuf->len);\n+\tint len = mbstowcs(wcstring, outbuf->buf, outbuf->len);\n+\tif (len == -1)\n+\t\treturn 1;\n+\tfor(int i = 0; i <= len; i++)\n+\t\tif(!isprint(wcstring[i]) && !isspace(wcstring[i]) )\n+\t\t\twcstring[i] = '?';\n+\t\tif (wcstombs(outbuf->buf, wcstring, outbuf->len) == -1)\n+\t\t\treturn 1;\n+\treturn 0;\n+}\n+\n int recv_sideband(const char *me, int in_stream, int out)\n {\n \tconst char *suffix;\n@@ -74,6 +89,9 @@ int recv_sideband(const char *me, int in_stream, int out)\n \t\t\t\t} else {\n \t\t\t\t\tstrbuf_addch(&outbuf, *brk);\n \t\t\t\t}\n+\n+\t\t\t\tif (sanitize_server_message(&outbuf))\n+\t\t\t\t\tretval = SIDEBAND_REMOTE_ERROR;\n \t\t\t\txwrite(2, outbuf.buf, outbuf.len);\n \t\t\t\tstrbuf_reset(&outbuf);\n \n@@ -97,6 +115,8 @@ int recv_sideband(const char *me, int in_stream, int out)\n \n \tif (outbuf.len) {\n \t\tstrbuf_addch(&outbuf, '\\n');\n+\t\tif (sanitize_server_message(&outbuf))\n+\t\t\tretval = SIDEBAND_REMOTE_ERROR;\n \t\txwrite(2, outbuf.buf, outbuf.len);\n \t}\n \tstrbuf_release(&outbuf);\ndiff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh\nindex 7f278d8ce..cc1f6ca29 100755\n--- a/t/t5401-update-hooks.sh\n+++ b/t/t5401-update-hooks.sh\n@@ -148,4 +148,27 @@ test_expect_success 'pre-receive hook that forgets to read its input' '\n \tgit push ./victim.git \"+refs/heads/*:refs/heads/*\"\n '\n \n+cat <<EOF >expect\n+remote: foo?[0;31mbar?[0m\n+To ./victim.git\n+ * [new branch]      victim_branch -> victim_branch\n+EOF\n+cat >victim.git/hooks/pre-receive <<'EOF'\n+#!/bin/sh\n+  printf \"foo\\033[0;31mbar\\033[0m\"\n+  exit 0\n+EOF\n+chmod u+x victim.git/hooks/pre-receive\n+test_expect_success 'pre-receive stderr contains ANSI colors' '\n+\trm -f victim.git/hooks/update victim.git/hooks/post-receive &&\n+\n+  git branch victim_branch master &&\n+\tgit push ./victim.git \"+refs/heads/victim_branch:refs/heads/victim_branch\"\\\n+    >send.out 2>send.err &&\n+\n+  cat send.err > actual &&\n+\n+  test_cmp expect actual\n+'\n+\n test_done\n-- \n2.14.4\n\n"},{"id":"350657","messageId":"20180621174122.GA30249@sigill.intra.peff.net","threadId":"48751","inReplyTo":"20180621121030.1721-1-sebastian.kisela@gmail.com","subject":"Re: [PATCH] Sanitize escape char sequences coming from server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-21T17:41:22Z","receivedAt":"2018-06-21T17:41:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 21, 2018 at 02:10:30PM +0200, Sebastian Kisela wrote:\n\n> From: Sebastian Kisela <skisela@redhat.com>\n> \n> Fix volnurability against MITM attacks on client side\n> by replacing non printable and non white space characters\n> by \"?\".\n> \n> Fixes: CVE-2018-1000021\n\nI'm not sure if this is a productive direction to pursue or not.\n\nIf you're worried about a malicious server (or MITM) sending you bad\nmessages, you'd also need to worry about them sending you bad repository\ncontent, as we may print filenames to stderr or stdout (and occasionally\nfile content, too, though typically only as part of a diff, which\n_usually_ goes through a pager).\n\nBut it's unclear to me if this is worth worrying about or not.\nUltimately it's a vulnerability in the terminal if untrusted output can\ndo bad things.\n\nIt's hard to make an evaluation of whether this plugs the vulnerability\nbecause we haven't really defined a threat model. What are we protecting\nagainst exactly?\n\nAs for the patch itself:\n\n> diff --git a/sideband.c b/sideband.c\n> index 325bf0e97..8c9d74ace 100644\n> --- a/sideband.c\n> +++ b/sideband.c\n> @@ -1,3 +1,4 @@\n> +#include <wchar.h>\n>  #include \"cache.h\"\n\nSystem includes should go into git-compat-util.h (because ordering is\noften important there).\n\nMore importantly, though, does everybody have wchar.h?\n\n> @@ -18,6 +19,20 @@\n>  #define ANSI_SUFFIX \"\\033[K\"\n>  #define DUMB_SUFFIX \"        \"\n>  \n> +int sanitize_server_message(struct strbuf *outbuf)\n> +{\n> +\twchar_t *wcstring = xmalloc(sizeof(wchar_t) * outbuf->len);\n\nThis is a potential integer overflow that can lead to a buffer overflow\n(e.g., imagine a system where both wchar_t and size_t are 32-bits; a 1GB\nstring will wrap around and we'll allocate a much smaller buffer than we\nexpected).\n\n> +\tint len = mbstowcs(wcstring, outbuf->buf, outbuf->len);\n\nI don't think mbstowcs() is always going to do the right thing there.\nWe're looking at a string that was sent from the remote server. What\nencoding is it in? Using mbstowcs() is going to use whatever is in\nLC_CTYPE on the local machine.\n\nAlso, the return type of mbstowcs is a size_t.\n\n> +\tfor(int i = 0; i <= len; i++)\n> +\t\tif(!isprint(wcstring[i]) && !isspace(wcstring[i]) )\n> +\t\t\twcstring[i] = '?';\n> +\t\tif (wcstombs(outbuf->buf, wcstring, outbuf->len) == -1)\n> +\t\t\treturn 1;\n\nFunny indentation. I think the second line is supposed to _not_ be in\nthe loop, so this is just funny indentation and not wrong code.\n\nUsing isprint() here probably doesn't do what you expect, because Git\nuses its own locale-agnostic ctype replacements. I didn't check, but I\nsuspect any non-ascii characters will be marked as non-printable, making\nthe whole wchar thing pointless.\n\nYour replacement allows existing spaces, which is good; many servers\nsend carriage-returns as part of progress output (and recv_sideband\ndetects these and makes sure the line remains prefixed with \"remote:\").\n\n> @@ -74,6 +89,9 @@ int recv_sideband(const char *me, int in_stream, int out)\n>  \t\t\t\t} else {\n>  \t\t\t\t\tstrbuf_addch(&outbuf, *brk);\n>  \t\t\t\t}\n> +\n> +\t\t\t\tif (sanitize_server_message(&outbuf))\n> +\t\t\t\t\tretval = SIDEBAND_REMOTE_ERROR;\n\n\"outbuf\" may contain partially-received lines at various points, meaning\nmulti-byte characters could be cut off. I _think_ it's OK to look at it\nhere, as we'd always be breaking on a \"\\r\" or \"\\n\" at this point.\n\n> @@ -97,6 +115,8 @@ int recv_sideband(const char *me, int in_stream, int out)\n>  \n>  \tif (outbuf.len) {\n>  \t\tstrbuf_addch(&outbuf, '\\n');\n> +\t\tif (sanitize_server_message(&outbuf))\n> +\t\t\tretval = SIDEBAND_REMOTE_ERROR;\n>  \t\txwrite(2, outbuf.buf, outbuf.len);\n>  \t}\n\nHere I think we could get cut off. Since it's the end of input, showing\nthe partial cutoff character is the best we can do. However, I think\nwe'd cause mbstowcs() in your sanitize function to report failure,\nmeaning we wouldn't show the remote message at all.\n\n> diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh\n> index 7f278d8ce..cc1f6ca29 100755\n> --- a/t/t5401-update-hooks.sh\n> +++ b/t/t5401-update-hooks.sh\n> @@ -148,4 +148,27 @@ test_expect_success 'pre-receive hook that forgets to read its input' '\n>  \tgit push ./victim.git \"+refs/heads/*:refs/heads/*\"\n>  '\n>  \n> +cat <<EOF >expect\n> +remote: foo?[0;31mbar?[0m\n> +To ./victim.git\n> + * [new branch]      victim_branch -> victim_branch\n> +EOF\n\nI know some tests in this script are already guilty of this, but please\navoid adding commands outside of a test_expect block. And use \"<<-\\EOF\"\nto allow proper indentation of the here-doc, and inhibit interpolation\nwhen you don't need it.\n\n> +cat >victim.git/hooks/pre-receive <<'EOF'\n> +#!/bin/sh\n> +  printf \"foo\\033[0;31mbar\\033[0m\"\n> +  exit 0\n> +EOF\n> +chmod u+x victim.git/hooks/pre-receive\n\nThis should use write_script, since the $SHELL_PATH may not be /bin/sh.\nSee nearby tests.\n\n> +test_expect_success 'pre-receive stderr contains ANSI colors' '\n> +\trm -f victim.git/hooks/update victim.git/hooks/post-receive &&\n> +\n> +  git branch victim_branch master &&\n\nFunny non-tab indentation (here and elsewhere).\n\n> +\tgit push ./victim.git \"+refs/heads/victim_branch:refs/heads/victim_branch\"\\\n> +    >send.out 2>send.err &&\n\nStyle: include a space before the backslash continuation.\n\nHowever, this should be the same as \"git push victim.git\n+victim_branch\", which eliminates the need for continuation.\n\n> +  cat send.err > actual &&\n\nStyle: we avoid whitespace between redirection operations and filenames\n(so just \">actual\").\n\n-Peff\n"},{"id":"350660","messageId":"20180621180942.GA32506@redhat.com","threadId":"48751","inReplyTo":"20180621174122.GA30249@sigill.intra.peff.net","subject":"Re: [PATCH] Sanitize escape char sequences coming from server","fromName":"Pavel Cahyna","fromEmail":"pcahyna@redhat.com","sentAt":"2018-06-21T18:09:43Z","receivedAt":"2018-06-21T18:09:50Z","isPatch":true,"sender":{"key":"pcahyna@redhat.com","avatar":null},"body":"Hello,\n\nOn Thu, Jun 21, 2018 at 01:41:22PM -0400, Jeff King wrote:\n> On Thu, Jun 21, 2018 at 02:10:30PM +0200, Sebastian Kisela wrote:\n> \n> > From: Sebastian Kisela <skisela@redhat.com>\n> > +\tint len = mbstowcs(wcstring, outbuf->buf, outbuf->len);\n> \n> I don't think mbstowcs() is always going to do the right thing there.\n> We're looking at a string that was sent from the remote server. What\n> encoding is it in? Using mbstowcs() is going to use whatever is in\n> LC_CTYPE on the local machine.\n\nExactly. The point is, everything should continue to work if the local\nmachine and the server agreed on the encoding. Imagine a\nnon-English-speaking site where the administrators configured the Git\nserver to output non-ASCII messages and the clients are configured with\na matching locale which allows the users to see them. We should ensure\neverything keeps working in this case.\n\n> > +\tfor(int i = 0; i <= len; i++)\n> > +\t\tif(!isprint(wcstring[i]) && !isspace(wcstring[i]) )\n> > +\t\t\twcstring[i] = '?';\n> > +\t\tif (wcstombs(outbuf->buf, wcstring, outbuf->len) == -1)\n> > +\t\t\treturn 1;\n> \n> Funny indentation. I think the second line is supposed to _not_ be in\n> the loop, so this is just funny indentation and not wrong code.\n> \n> Using isprint() here probably doesn't do what you expect, because Git\n> uses its own locale-agnostic ctype replacements. I didn't check, but I\n> suspect any non-ascii characters will be marked as non-printable, making\n> the whole wchar thing pointless.\n\nisw*() was probably intended instead of is*()\n\n> Your replacement allows existing spaces, which is good; many servers\n> send carriage-returns as part of progress output (and recv_sideband\n> detects these and makes sure the line remains prefixed with \"remote:\").\n> \n> > @@ -74,6 +89,9 @@ int recv_sideband(const char *me, int in_stream, int out)\n> >  \t\t\t\t} else {\n> >  \t\t\t\t\tstrbuf_addch(&outbuf, *brk);\n> >  \t\t\t\t}\n> > +\n> > +\t\t\t\tif (sanitize_server_message(&outbuf))\n> > +\t\t\t\t\tretval = SIDEBAND_REMOTE_ERROR;\n> \n> \"outbuf\" may contain partially-received lines at various points, meaning\n> multi-byte characters could be cut off. I _think_ it's OK to look at it\n> here, as we'd always be breaking on a \"\\r\" or \"\\n\" at this point.\n\nMaybe sanitize_server_message should return a mbstate_t to keep state\nbetween invocations?\n\nThanks, Pavel\n"},{"id":"350661","messageId":"20180621185111.GA1046@sigill.intra.peff.net","threadId":"48751","inReplyTo":"20180621180942.GA32506@redhat.com","subject":"Re: [PATCH] Sanitize escape char sequences coming from server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-21T18:51:11Z","receivedAt":"2018-06-21T18:51:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 21, 2018 at 08:09:43PM +0200, Pavel Cahyna wrote:\n\n> > > +\tint len = mbstowcs(wcstring, outbuf->buf, outbuf->len);\n> > \n> > I don't think mbstowcs() is always going to do the right thing there.\n> > We're looking at a string that was sent from the remote server. What\n> > encoding is it in? Using mbstowcs() is going to use whatever is in\n> > LC_CTYPE on the local machine.\n> \n> Exactly. The point is, everything should continue to work if the local\n> machine and the server agreed on the encoding. Imagine a\n> non-English-speaking site where the administrators configured the Git\n> server to output non-ASCII messages and the clients are configured with\n> a matching locale which allows the users to see them. We should ensure\n> everything keeps working in this case.\n\nWhat if they don't agree on the encoding? Right now you might get some\nmojibake. After this patch, we'd return an error.\n\nI thought at first we'd stop showing the message, which would be a\nregression. But looking at the caller, it does not actually break on\nseeing the error. Which means that the whole sanitizing process can be\nskipped simply by the attacker including a bogus multibyte sequence.\n\n> > Using isprint() here probably doesn't do what you expect, because Git\n> > uses its own locale-agnostic ctype replacements. I didn't check, but I\n> > suspect any non-ascii characters will be marked as non-printable, making\n> > the whole wchar thing pointless.\n> \n> isw*() was probably intended instead of is*()\n\nYes, we don't override the isw* functions, so that would work (I still\nthink that assuming the server messages are in our local charset is\nsomewhat questionable).\n\n> > > +\n> > > +\t\t\t\tif (sanitize_server_message(&outbuf))\n> > > +\t\t\t\t\tretval = SIDEBAND_REMOTE_ERROR;\n> > \n> > \"outbuf\" may contain partially-received lines at various points, meaning\n> > multi-byte characters could be cut off. I _think_ it's OK to look at it\n> > here, as we'd always be breaking on a \"\\r\" or \"\\n\" at this point.\n> \n> Maybe sanitize_server_message should return a mbstate_t to keep state\n> between invocations?\n\nI think this site is OK because of the CR/LF breaking. For the \"final\"\none where it's not OK, there's no point in keeping state since we know\nwe hit EOF already.\n\n-Peff\n"}]}