Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize sideband channel messages #1853

Open
wants to merge 3 commits into
base: maint-2.47
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Documentation/config.txt
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,8 @@ include::config/sequencer.txt[]

include::config/showbranch.txt[]

include::config/sideband.txt[]

include::config/sparse.txt[]

include::config/splitindex.txt[]
Expand Down
16 changes: 16 additions & 0 deletions Documentation/config/sideband.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
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:
+
--
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.
--
78 changes: 76 additions & 2 deletions sideband.c
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ static struct keyword_entry keywords[] = {
{ "error", GIT_COLOR_BOLD_RED },
};

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)
{
Expand All @@ -38,6 +44,25 @@ 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 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);
else if (!git_config_get_string_tmp("color.ui", &value))
Expand Down Expand Up @@ -65,6 +90,55 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref
list_config_item(list, prefix, keywords[i].keyword);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Dscho

Just a couple of small comments

On 14/01/2025 18:19, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
> > +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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Andreas Schwab wrote (reply to this):

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, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Andreas Schwab <[email protected]> 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).

}

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)
{
int i;

if (allow_control_characters == ALLOW_ALL_CONTROL_CHARACTERS) {
strbuf_add(dest, src, n);
return;
}

strbuf_grow(dest, 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))) {
src += i;
n -= i;
} 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
Expand All @@ -80,7 +154,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;
}

Expand Down Expand Up @@ -113,7 +187,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);
}


Expand Down
30 changes: 30 additions & 0 deletions t/t5409-colorize-remote-messages.sh
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,34 @@ 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?\\a\\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_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 &&
tr -dc "\\007" <stderr >actual &&
test_file_not_empty actual
'

test_done
Loading