From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 16 Jan 2026 19:29:58 GMT
Subject: Re: [PATCH v2 1/4] sideband: mask control characters
Message-ID: <f3891ea0-56f0-b6ff-afac-e06fc143ecc5@gmx.de>
In-Reply-To: <aWD2s5RyhxYSLmfc@pks.im>

Hi Patrick,

On Fri, 9 Jan 2026, Patrick Steinhardt wrote:

> On Wed, Dec 17, 2025 at 02:23:39PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 
> > The output of `git clone` is a vital component for understanding what
> > has happened when things go wrong. However, these logs are partially
> > under the control of the remote server (via the "sideband", which
> > typically contains what the remote `git pack-objects` process sends to
> > `stderr`), and is currently not sanitized by Git.
> > 
> > This makes Git susceptible to ANSI escape sequence injection (see
> > CWE-150, https://cwe.mitre.org/data/definitions/150.html), which allows
> > attackers to corrupt terminal state, to hide information, and even to
> > insert characters into the input buffer (i.e. as if the user had typed
> > those characters).
> > 
> > To plug this vulnerability, disallow any control character in the
> > sideband, replacing them instead with the common `^<letter/symbol>`
> > (e.g. `^[` for `\x1b`, `^A` for `\x01`).
> > 
> > There is likely a need for more fine-grained controls instead of using a
> > "heavy hammer" like this, which will be introduced subsequently.
> 
> Most notably color codes, I assume.

Precisely.

> > diff --git a/sideband.c b/sideband.c
> > index 02805573fa..fc1805dcf8 100644
> > --- a/sideband.c
> > +++ b/sideband.c
> > @@ -65,6 +65,19 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
> >  		list_config_item(list, prefix, keywords[i].keyword);
> >  }
> >  
> > +static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
> 
> Shouldn't `n` be of type `size_t`? I guess the answer is "maybe", as
> `maybe_colorize_sideband()` also accepts `int n` with a big comment
> explaining why that's okay. Ultimately, the reason is that we accept
> pkt-lines, so every line is limited to at most 64kB anyway.

Exactly. I did not want to use a different data type for the parameter
that is essentially just passed through.

> > +{
> > +	strbuf_grow(dest, n);
> > +	for (; n && *src; src++, n--) {
> > +		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
> > +			strbuf_addch(dest, *src);
> > +		else {
> 
> Tiny nit, not worth addressing on its own: the if branch should also
> have curly braces.

Will address in the next iteration.

Ciao,
Johannes