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

From: Jeff King <peff@peff.net>
Date: Jun 21, 2018, 18:51 UTC
Message-ID: <20180621185111.GA1046@sigill.intra.peff.net>
In-Reply-To: <20180621180942.GA32506@redhat.com>

On Thu, Jun 21, 2018 at 08:09:43PM +0200, Pavel Cahyna wrote:

Show 13 quoted lines

> > > +	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.

Show 6 quoted lines

> > 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).

Show 10 quoted lines

> > > +
> > > +				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