From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 09 Jan 2026 12:38:11 GMT
Subject: Re: [PATCH v2 1/4] sideband: mask control characters
Message-ID: <aWD2s5RyhxYSLmfc@pks.im>
In-Reply-To: <8d7047655933592939dd1395f5b1ead595cee4ee.1765981422.git.gitgitgadget@gmail.com>

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.

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

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

Patrick