git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Dec 2, 2025, 14:56 UTC
Message-ID
<038dc330-3353-25a7-aa58-f4989cdd391c@gmx.de>
In-Reply-To
<8570a129-d66a-465a-905e-0a077c69c409@gmail.com>
Hi Phillip,
On Wed, 15 Jan 2025, Phillip Wood wrote:
Show 16 quoted lines
> 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
> 
>
Previous: Phillip WoodNext: Johannes Schindelin via GitGitGadget
Message 17 of 86 in “Sanitize sideband channel messages”
  1. 0/3 Sanitize sideband channel messagesJohannes Schindelin via GitGitGadget, Jan 14, 2025
  2. 1/3 sideband: mask control charactersJohannes Schindelin via GitGitGadget, Jan 14, 2025
  3. Phillip WoodJan 15, 2025
  4. Johannes SchindelinDec 2, 2025
  5. Andreas SchwabJan 15, 2025
  6. Junio C HamanoJan 15, 2025
  7. 2/3 sideband: introduce an "escape hatch" to allow control charactersJohannes Schindelin via GitGitGadget, Jan 14, 2025
  8. 3/3 sideband: do allow ANSI color sequences by defaultJohannes Schindelin via GitGitGadget, Jan 14, 2025
  9. brian m. carlsonJan 14, 2025
  10. Junio C HamanoJan 16, 2025
  11. Ondrej PohorelskyJan 28, 2025
  12. Junio C HamanoJan 31, 2025
  13. Johannes SchindelinDec 2, 2025
  14. brian m. carlsonDec 3, 2025
  15. Johannes SchindelinDec 3, 2025
  16. Phillip WoodJan 15, 2025
  17. Johannes SchindelinDec 2, 2025
  18. 0/4 Sanitize sideband channel messagesJohannes Schindelin via GitGitGadget, Dec 17, 2025
  19. 1/4 sideband: mask control charactersJohannes Schindelin via GitGitGadget, Dec 17, 2025
  20. Patrick SteinhardtJan 9, 2026
  21. Johannes SchindelinJan 16, 2026
  22. 2/4 sideband: introduce an "escape hatch" to allow control charactersJohannes Schindelin via GitGitGadget, Dec 17, 2025
  23. Junio C HamanoDec 18, 2025
  24. Johannes SchindelinDec 18, 2025
  25. Junio C HamanoDec 19, 2025
  26. Johannes SchindelinJan 16, 2026
  27. Patrick SteinhardtJan 9, 2026
  28. 3/4 sideband: do allow ANSI color sequences by defaultJohannes Schindelin via GitGitGadget, Dec 17, 2025
  29. Patrick SteinhardtJan 9, 2026
  30. Johannes SchindelinJan 16, 2026
  31. 4/4 sideband: add options to allow more control sequences to be passed throughJohannes Schindelin via GitGitGadget, Dec 17, 2025
  32. Patrick SteinhardtJan 9, 2026
  33. brian m. carlsonJan 10, 2026
  34. Jeff KingJan 15, 2026
  35. Junio C HamanoJan 15, 2026
  36. Johannes SchindelinJan 15, 2026
  37. Patrick SteinhardtJan 16, 2026
  38. Ondrej PohorelskyJan 16, 2026
  39. Junio C HamanoJan 16, 2026
  40. Johannes SchindelinJan 16, 2026
  41. Junio C HamanoJan 16, 2026
  42. Patrick SteinhardtJan 19, 2026
  43. brian m. carlsonJan 19, 2026
  44. D. Ben KnobleJan 20, 2026
  45. Junio C HamanoJan 20, 2026
  46. Jeff KingJan 20, 2026
  47. Junio C HamanoJan 20, 2026
  48. Patrick SteinhardtJan 21, 2026
  49. Johannes SchindelinJan 22, 2026
  50. Junio C HamanoJan 22, 2026
  51. brian m. carlsonJan 15, 2026
  52. Junio C HamanoFeb 3, 2026
  53. Johannes SchindelinFeb 3, 2026
  54. Junio C HamanoFeb 3, 2026
  55. Junio C HamanoFeb 4, 2026
  56. Johannes SchindelinJan 16, 2026
  57. 0/5 Sanitize sideband channel messagesJohannes Schindelin via GitGitGadget, Jan 16, 2026
  58. 1/5 sideband: mask control charactersJohannes Schindelin via GitGitGadget, Jan 16, 2026
  59. 2/5 sideband: introduce an "escape hatch" to allow control charactersJohannes Schindelin via GitGitGadget, Jan 16, 2026
  60. 3/5 sideband: do allow ANSI color sequences by defaultJohannes Schindelin via GitGitGadget, Jan 16, 2026
  61. 4/5 sideband: add options to allow more control sequences to be passed throughJohannes Schindelin via GitGitGadget, Jan 16, 2026
  62. 5/5 sideband: offer to configure sanitizing on a per-URL basisJohannes Schindelin via GitGitGadget, Jan 16, 2026
  63. Johannes SchindelinJan 16, 2026
  64. 0/6 Sanitize sideband channel messagesJohannes Schindelin via GitGitGadget, Feb 3, 2026
  65. 1/6 sideband: mask control charactersJohannes Schindelin via GitGitGadget, Feb 3, 2026
  66. 2/6 sideband: introduce an "escape hatch" to allow control charactersJohannes Schindelin via GitGitGadget, Feb 3, 2026
  67. 3/6 sideband: do allow ANSI color sequences by defaultJohannes Schindelin via GitGitGadget, Feb 3, 2026
  68. 4/6 sideband: add options to allow more control sequences to be passed throughJohannes Schindelin via GitGitGadget, Feb 3, 2026
  69. 5/6 sideband: offer to configure sanitizing on a per-URL basisJohannes Schindelin via GitGitGadget, Feb 3, 2026
  70. 6/6 sideband: delay sanitizing by default to Git v3.0Johannes Schindelin via GitGitGadget, Feb 3, 2026
  71. Junio C HamanoFeb 4, 2026
  72. Junio C HamanoFeb 5, 2026
  73. Junio C HamanoFeb 13, 2026
  74. 0/3 Sanitizing sideband outputJunio C Hamano, Mar 2, 2026
  75. 1/3 sideband: drop 'default' configurationJunio C Hamano, Mar 2, 2026
  76. 2/3 sideband: delay sanitizing by default to Git v3.0Junio C Hamano, Mar 2, 2026
  77. 3/3 sideband: conditional documentation fixJunio C Hamano, Mar 2, 2026
  78. 0/7 Sanitizing sideband outputJunio C Hamano, Mar 5, 2026
  79. 1/7 sideband: mask control charactersJunio C Hamano, Mar 5, 2026
  80. 2/7 sideband: introduce an "escape hatch" to allow control charactersJunio C Hamano, Mar 5, 2026
  81. 3/7 sideband: do allow ANSI color sequences by defaultJunio C Hamano, Mar 5, 2026
  82. 4/7 sideband: add options to allow more control sequences to be passed throughJunio C Hamano, Mar 5, 2026
  83. 5/7 sideband: offer to configure sanitizing on a per-URL basisJunio C Hamano, Mar 5, 2026
  84. 6/7 sideband: drop 'default' configurationJunio C Hamano, Mar 5, 2026
  85. 7/7 sideband: delay sanitizing by default to Git v3.0Junio C Hamano, Mar 5, 2026
  86. Shipping 2.55 with stricter "neuter sideband" topicJunio C Hamano, Jun 11, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.