Re: [PATCH v2 1/4] sideband: mask control characters
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jan 9, 2026, 12:38 UTC
- 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:
Show 20 quoted lines
> 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.
Show 9 quoted lines
> 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.
Show 6 quoted lines
> +{
> + 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