Sanitize sideband channel messages #1853
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| 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 (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. | ||
| true:: | ||
| Allow all control characters to be sent to the terminal. | ||
| -- |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,47 @@ static struct keyword_entry keywords[] = { | |
| { "error", GIT_COLOR_BOLD_RED }, | ||
| }; | ||
|
|
||
| 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<<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) | ||
| { | ||
|
|
@@ -38,6 +79,22 @@ 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 (!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)) | ||
|
|
@@ -65,6 +122,93 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref | |
| list_config_item(list, prefix, keywords[i].keyword); | ||
|
There was a problem hiding this comment. 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
There was a problem hiding this comment. On the Git mailing list, Johannes Schindelin wrote (reply to this): 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 <[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() ?
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.There was a problem hiding this comment. 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."There was a problem hiding this comment. 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_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. | ||
| * | ||
| * 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 (n < 3 || src[0] != '\x1b' || src[1] != '[') | ||
| return 0; | ||
|
|
||
| for (i = 2; i < n; i++) { | ||
| 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; | ||
| } | ||
| 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 (allow_control_characters != ALLOW_NO_CONTROL_CHARACTERS && | ||
| (i = handle_ansi_sequence(dest, src, n))) { | ||
| src += i; | ||
| n -= i; | ||
| } 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 +224,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_sanitized(dest, src, n); | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -113,7 +257,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) | |
| } | ||
| } | ||
|
|
||
| strbuf_add_sanitized(dest, src, n); | ||
| } | ||
|
|
||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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):
There was a problem hiding this comment.
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, Johannes Schindelin wrote (reply to this):
There was a problem hiding this comment.
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):