[PATCH v3 0/5] Sanitize sideband channel messages

["Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>]

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. Integrating this into newer
versions is a bit cumbersome; I pushed a version of the branch as rebased to
v2.53.0-rc0 here:
https://github.com/dscho/git/tree/refs/heads/sanitize-sideband-2.53.0-rc0

Changes since v2:

 * Added curly brackets around a single-line if clause.
 * Enclosed the values in the documentation within backticks.
 * Aligned the enum values for better readability.
 * Added support for sideband.<url>.allowControlCharacters (à la
   http.<url>.*) on top of sideband.allowControlCharacters.

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 (5):
  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
  sideband: offer to configure sanitizing on a per-URL basis

 Documentation/config.txt            |   2 +
 Documentation/config/sideband.txt   |  28 +++++
 sideband.c                          | 181 +++++++++++++++++++++++++++-
 sideband.h                          |  14 +++
 t/t5409-colorize-remote-messages.sh |  92 ++++++++++++++
 transport.c                         |   3 +
 6 files changed, 318 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-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1853/dscho/sanitize-sideband-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1853

Range-diff vs v2:

 1:  8d70476559 ! 1:  e6b71af0ca sideband: mask control characters
     @@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons
      +{
      +	strbuf_grow(dest, n);
      +	for (; n && *src; src++, n--) {
     -+		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
     ++		if (!iscntrl(*src) || *src == '\t' || *src == '\n') {
      +			strbuf_addch(dest, *src);
     -+		else {
     ++		} else {
      +			strbuf_addch(dest, '^');
      +			strbuf_addch(dest, *src == 0x7f ? '?' : 0x40 + *src);
      +		}
 2:  2615abd8c5 ! 2:  8f64d65844 sideband: introduce an "escape hatch" to allow control characters
     @@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons
      +
       	strbuf_grow(dest, n);
       	for (; n && *src; src++, n--) {
     - 		if (!iscntrl(*src) || *src == '\t' || *src == '\n')
     + 		if (!iscntrl(*src) || *src == '\t' || *src == '\n') {
      
       ## t/t5409-colorize-remote-messages.sh ##
      @@ t/t5409-colorize-remote-messages.sh: test_expect_success 'disallow (color) control sequences in sideband' '
 3:  44585ba1f4 ! 3:  44838acacc sideband: do allow ANSI color sequences by default
     @@ Documentation/config/sideband.txt
      +	this config setting to override this behavior:
      ++
      +--
     -+	default::
     -+	color::
     ++	`default`::
     ++	`color`::
      +		Allow ANSI color sequences, line feeds and horizontal tabs,
      +		but mask all other control characters. This is the default.
     -+	false::
     ++	`false`::
      +		Mask all control characters other than line feeds and
      +		horizontal tabs.
     -+	true::
     ++	`true`::
      +		Allow all control characters to be sent to the terminal.
      +--
      
     @@ sideband.c: static struct keyword_entry keywords[] = {
       
      -static int allow_control_characters;
      +static enum {
     -+	ALLOW_NO_CONTROL_CHARACTERS = 0,
     -+	ALLOW_ANSI_COLOR_SEQUENCES = 1<<0,
     ++	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;
     @@ sideband.c: void list_config_color_sideband_slots(struct string_list *list, cons
       	}
      @@ sideband.c: 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')
     + 		if (!iscntrl(*src) || *src == '\t' || *src == '\n') {
       			strbuf_addch(dest, *src);
     --		else {
     -+		else if ((i = handle_ansi_color_sequence(dest, src, n))) {
     ++		} else if ((i = handle_ansi_color_sequence(dest, src, n))) {
      +			src += i;
      +			n -= i;
     -+		} else {
     + 		} else {
       			strbuf_addch(dest, '^');
       			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' '
 4:  fe109cd331 ! 4:  cc578465b9 sideband: add options to allow more control sequences to be passed through
     @@ Documentation/config/sideband.txt: sideband.allowControlCharacters::
      +	a comma-separated list of the following keywords):
       +
       --
     - 	default::
     - 	color::
     + 	`default`::
     + 	`color`::
       		Allow ANSI color sequences, line feeds and horizontal tabs,
       		but mask all other control characters. This is the default.
     -+	cursor::
     ++	`cursor:`:
      +		Allow control sequences that move the cursor. This is
      +		disabled by default.
     -+	erase::
     ++	`erase`::
      +		Allow control sequences that erase charactrs. This is
      +		disabled by default.
     - 	false::
     + 	`false`::
       		Mask all control characters other than line feeds and
       		horizontal tabs.
      
       ## sideband.c ##
      @@ sideband.c: 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_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;
     @@ sideband.c: static void strbuf_add_sanitized(struct strbuf *dest, const char *sr
       	}
      @@ sideband.c: 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')
     + 		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))) {
     +-		} 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 {
 -:  ---------- > 5:  f2eb0a758c sideband: offer to configure sanitizing on a per-URL basis

-- 
gitgitgadget